Merge pull request brave#11311 from brave/11028

18 Unbelievable ways to get rid of dead tabs on tab close

app/browser/reducers/tabsReducer.js

@@ -98,6 +98,27 @@ const tabsReducer = (state, action, immutableAction) => {
  })
  break
  }
+ case appConstants.APP_TAB_PAGE_CLOSE_MENU_ITEM_CLICKED: {
+ const windowId = action.get('windowId')
+ const tabPageIndex = action.get('tabPageIndex')
+ state = tabs.closeTabPage(state, windowId, tabPageIndex)
+ break
+ }
+ case appConstants.APP_CLOSE_TABS_TO_LEFT_MENU_ITEM_CLICKED: {
+ const tabId = action.get('tabId')
+ state = tabs.closeTabsToLeft(state, tabId)
+ break
+ }
+ case appConstants.APP_CLOSE_TABS_TO_RIGHT_MENU_ITEM_CLICKED: {
+ const tabId = action.get('tabId')
+ state = tabs.closeTabsToRight(state, tabId)
+ break
+ }
+ case appConstants.APP_CLOSE_OTHER_TABS_MENU_ITEM_CLICKED: {
+ const tabId = action.get('tabId')
+ state = tabs.closeOtherTabs(state, tabId)
+ break
+ }
  case appConstants.APP_CREATE_TAB_REQUESTED:
  if (action.getIn(['createProperties', 'windowId']) == null) {
  const senderWindowId = action.getIn(['senderWindowId'])
@@ -147,6 +168,12 @@ const tabsReducer = (state, action, immutableAction) => {
  tabs.toggleDevTools(tabId)
  })
  } else {
+ // This check is only needed in case front end tries to close
+ // a tabId it thinks exists but doesn't actually exist anymore.
+ const tabValue = tabState.getByTabId(state, tabId)
+ if (!tabValue) {
+ break
+ }
  const windowId = tabState.getWindowId(state, tabId)
  const nonPinnedTabs = tabState.getNonPinnedTabsByWindowId(state, windowId)
  const pinnedTabs = tabState.getPinnedTabsByWindowId(state, windowId)
@@ -184,6 +211,7 @@ const tabsReducer = (state, action, immutableAction) => {
  tabs.setActive(nextActiveTabId)
  }
  })
+ tabs.forgetTab(tabId)
  }
  break
  case appConstants.APP_ALLOW_FLASH_ONCE:

app/browser/tabs.js

@@ -452,11 +452,6 @@ const api = {
  updateTab(tabId, changeInfo)
  })
 
- process.on('chrome-tabs-removed', (tabId, windowId) => {
- appActions.tabClosed(tabId, windowId)
- cleanupWebContents(tabId)
- })
-
  app.on('web-contents-created', function (event, tab) {
  if (tab.isBackgroundPage() || !tab.isGuest()) {
  return
@@ -548,6 +543,14 @@ const api = {
  windowActions.gotResponseDetails(tabId, {status, newURL, originalURL, httpResponseCode, requestMethod, referrer, resourceType})
  }
  })
+
+ tab.once('will-destroy', () => {
+ const tabValue = getTabValue(tabId)
+ if (tabValue) {
+ const windowId = tabValue.get('windowId')
+ appActions.tabClosed(tabId, windowId)
+ }
+ })
  })
 
  process.on('on-tab-created', (tab, options) => {
@@ -1019,6 +1022,86 @@ const api = {
 
  return nextTabId
  },
+
+ closeTabPage: (state, windowId, tabPageIndex) => {
+ const tabsPerPage = Number(getSetting(settings.TABS_PER_PAGE))
+ const startTabIndex = tabPageIndex * tabsPerPage
+ const pinnedTabsCount = tabState.getPinnedTabsByWindowId(state, windowId).size
+ tabState.getTabsByWindowId(state, windowId)
+ .sort((tab1, tab2) => tab1.get('index') - tab2.get('index'))
+ .filter((tabValue) => !tabValue.get('pinned'))
+ .slice(startTabIndex + pinnedTabsCount, startTabIndex + tabsPerPage + pinnedTabsCount)
+ .forEach((tabValue) => {
+ const tab = getWebContents(tabValue.get('tabId'))
+ if (tab && !tab.isDestroyed()) {
+ tab.forceClose()
+ }
+ })
+ state = api.updateTabsStateForWindow(state, windowId)
+ return state
+ },
+
+ closeTabsToLeft: (state, tabId) => {
+ const tabValue = tabState.getByTabId(state, tabId)
+ if (!tabValue) {
+ return state
+ }
+ const index = tabValue.get('index')
+ const windowId = tabValue.get('windowId')
+ const pinnedTabsCount = tabState.getPinnedTabsByWindowId(state, windowId).size
+ tabState.getTabsByWindowId(state, windowId)
+ .sort((tab1, tab2) => tab1.get('index') - tab2.get('index'))
+ .filter((tabValue) => !tabValue.get('pinned'))
+ .slice(0, index - pinnedTabsCount)
+ .forEach((tabValue) => {
+ const tab = getWebContents(tabValue.get('tabId'))
+ if (tab && !tab.isDestroyed()) {
+ tab.forceClose()
+ }
+ })
+ state = api.updateTabsStateForWindow(state, windowId)
+ return state
+ },
+
+ closeTabsToRight: (state, tabId) => {
+ const tabValue = tabState.getByTabId(state, tabId)
+ if (!tabValue) {
+ return state
+ }
+ const index = tabValue.get('index')
+ const windowId = tabValue.get('windowId')
+ const pinnedTabsCount = tabState.getPinnedTabsByWindowId(state, windowId).size
+ tabState.getTabsByWindowId(state, windowId)
+ .sort((tab1, tab2) => tab1.get('index') - tab2.get('index'))
+ .filter((tabValue) => !tabValue.get('pinned'))
+ .slice(index + 1 - pinnedTabsCount)
+ .forEach((tabValue) => {
+ const tab = getWebContents(tabValue.get('tabId'))
+ if (tab && !tab.isDestroyed()) {
+ tab.forceClose()
+ }
+ })
+ state = api.updateTabsStateForWindow(state, windowId)
+ return state
+ },
+
+ closeOtherTabs: (state, tabId) => {
+ const tabValue = tabState.getByTabId(state, tabId)
+ if (!tabValue) {
+ return state
+ }
+ const windowId = tabValue.get('windowId')
+ tabState.getTabsByWindowId(state, windowId)
+ .forEach((tabValue) => {
+ const tab = getWebContents(tabValue.get('tabId'))
+ if (tab && !tab.isDestroyed() && tabValue.get('tabId') !== tabId && !tabValue.get('pinned')) {
+ tab.forceClose()
+ }
+ })
+ state = api.updateTabsStateForWindow(state, windowId)
+ return state
+ },
+
  debugTabs: (state) => {
  console.log(tabState.getTabs(state)
  .toJS()
@@ -1076,6 +1159,9 @@ const api = {
  }
  })
  return state
+ },
+ forgetTab: (tabId) => {
+ cleanupWebContents(tabId)
  }
 }
 

app/renderer/components/main/main.js

@@ -393,10 +393,6 @@ class Main extends React.Component {
 
  ipc.on(messages.SHORTCUT_UNDO_CLOSED_FRAME, () => windowActions.undoClosedFrame())
 
- ipc.on(messages.SHORTCUT_CLOSE_OTHER_FRAMES, (e, tabId, isCloseRight, isCloseLeft) => {
- windowActions.closeOtherFrames(tabId, isCloseRight, isCloseLeft)
- })
-
  ipc.on(messages.SHOW_DOWNLOADS_TOOLBAR, () => {
  windowActions.setDownloadsToolbarVisible(true)
  })

app/renderer/reducers/contextMenuReducer.js

@@ -40,7 +40,7 @@ const dndData = require('../../../js/dndData')
 const bookmarkFoldersUtil = require('../../common/lib/bookmarkFoldersUtil')
 const historyState = require('../../common/state/historyState')
 const {makeImmutable, isMap} = require('../../common/state/immutableUtil')
-const {getCurrentWindow} = require('../../renderer/currentWindow')
+const {getCurrentWindow, getCurrentWindowId} = require('../../renderer/currentWindow')
 const {getSetting} = require('../../../js/settings')
 
 const validateAction = function (action) {
@@ -95,9 +95,7 @@ const onTabPageMenu = function (state, action) {
  }, {
  label: locale.translation('closeTabPage'),
  click: () => {
- tabPageFrames
- .map((frame) => frame.get('tabId'))
- .forEach((tabId) => appActions.tabCloseRequested(tabId))
+ appActions.tabPageCloseMenuItemClicked(getCurrentWindowId(), index)
  }
  }]
 

app/renderer/reducers/frameReducer.js

@@ -80,22 +80,41 @@ const getLocation = (location) => {
 
 const frameReducer = (state, action, immutableAction) => {
  switch (action.actionType) {
+ case appConstants.APP_TAB_CLOSED: {
+ const tabId = immutableAction.get('tabId')
+ const frame = frameStateUtil.getFrameByTabId(state, tabId)
+ if (frame) {
+ action.frameKey = frame.get('key')
+ state = closeFrame(state, action)
+ const activeFrame = frameStateUtil.getActiveFrame(state)
+ if (activeFrame) {
+ state = frameStateUtil.updateTabPageIndex(state, activeFrame.get('tabId'))
+ }
+ }
+ break
+ }
  case appConstants.APP_TAB_UPDATED:
  // This case will be fired for both tab creation and tab update.
- // being `tabValue` set for tab creation and `changeInfo` set for tab update
  const tab = immutableAction.get('tabValue')
  const changeInfo = immutableAction.get('changeInfo')
  if (!tab) {
  break
  }
  const tabId = tab.get('tabId')
+ if (tabId === -1) {
+ break
+ }
  let frame = frameStateUtil.getFrameByTabId(state, tabId)
  if (!frame) {
  break
  }
  let frames = state.get('frames')
  const index = tab.get('index')
+ // If front end doesn't know about this tabId, then do nothing!
  let sourceFrameIndex = frameStateUtil.getIndexByTabId(state, tabId)
+ if (sourceFrameIndex == null) {
+ break
+ }
  if (index != null &&
  sourceFrameIndex !== index &&
  // Only update the index once the frame is known.
@@ -147,29 +166,26 @@ const frameReducer = (state, action, immutableAction) => {
  }
 
  const active = tab.get('active')
- if (active != null) {
- if (active) {
-  state = frameStateUtil.setActiveFrameKey(state, frame.get('key'))
-  state = frameStateUtil.setFrameLastAccessedTime(state, sourceFrameIndex)
+ if (active && state.get('activeFrameKey') !== frame.get('key')) {
+ state = frameStateUtil.setActiveFrameKey(state, frame.get('key'))
+ state = frameStateUtil.setFrameLastAccessedTime(state, sourceFrameIndex)
+ state = state.set('previewFrameKey', null)
 
-  // Handle tabPage updates and preview cancelation based on tab updated
-  // otherwise tabValue will fire those events each time a tab finish loading
-  // see bug #8429
-  const isNewTab = changeInfo.isEmpty()
-  const activeTabHasUpdated = changeInfo.get('active') != null
-  const hasBeenActivated = frame.get('hasBeenActivated')
+ // Handle tabPage updates and preview cancelation based on tab updated
+ // otherwise tabValue will fire those events each time a tab finish loading
+ // see bug #8429
+ const isNewTab = changeInfo.isEmpty()
+ const activeTabHasUpdated = changeInfo.get('active') != null
+ const hasBeenActivated = frame.get('hasBeenActivated')
 
- if (!isNewTab && activeTabHasUpdated) {
- state = frameStateUtil.updateTabPageIndex(state, tabId)
- state = state.set('previewFrameKey', null)
- // Show the phishing warning if we are showing a data: tab
- // for the first time
- state = state.setIn(['ui', 'siteInfo', 'isVisible'],
- !hasBeenActivated && isPotentialPhishingUrl(tab.get('url')))
- }
- if (!frame.get('hasBeenActivated')) {
- state = state.setIn(['frames', index, 'hasBeenActivated'], true)
- }
+ if (!isNewTab && activeTabHasUpdated) {
+ // Show the phishing warning if we are showing a data: tab
+ // for the first time
+ state = state.setIn(['ui', 'siteInfo', 'isVisible'],
+ !hasBeenActivated && isPotentialPhishingUrl(tab.get('url')))
+ }
+ if (!frame.get('hasBeenActivated')) {
+ state = state.setIn(['frames', index, 'hasBeenActivated'], true)
  }
  }
  break
@@ -220,34 +236,6 @@ const frameReducer = (state, action, immutableAction) => {
  appActions.tabCloseRequested(frame.get('tabId'))
  })
  break
- case windowConstants.WINDOW_CLOSE_OTHER_FRAMES:
- {
- const currentIndex = frameStateUtil.getIndexByTabId(state, action.tabId)
- if (currentIndex === -1) {
- break
- }
-
- let tabs = []
-
- state.get('frames').forEach((frame, i) => {
- if (!frame.get('pinnedLocation') &&
- ((i < currentIndex && action.isCloseLeft) || (i > currentIndex && action.isCloseRight))) {
- if (frame) {
- tabs.push(frame.get('tabId'))
- appActions.tabCloseRequested(frame.get('tabId'))
- }
- }
- })
-
- // TODO(nejc) this can be simplified when states are merged
- const newFrames = state.get('frames').filter(frame => !tabs.includes(frame.get('tabId')))
- let newState = state.set('frames', newFrames)
- newState = frameStateUtil.updateTabPageIndex(newState, action.tabId)
- const index = newState.getIn(['ui', 'tabs', 'tabPageIndex'], 0)
- state = state.setIn(['ui', 'tabs', 'tabPageIndex'], index)
- }
- break
-
  case windowConstants.WINDOW_CLOSE_FRAME:
  state = closeFrame(state, action)
  const activeFrame = frameStateUtil.getActiveFrame(state)

js/actions/appActions.js

@@ -166,6 +166,51 @@ const appActions = {
  })
  },
 
+ /**
+ * Menu item for closing a tab page has been clicked.
+ * @param {Number} tabPageIndex The index of the tab page to close
+ */
+ tabPageCloseMenuItemClicked: function (windowId, tabPageIndex) {
+ dispatch({
+ actionType: appConstants.APP_TAB_PAGE_CLOSE_MENU_ITEM_CLICKED,
+ tabPageIndex,
+ windowId
+ })
+ },
+
+ /**
+ * Menu item for closing tabs to the left has been clicked.
+ * @param {Number} tabId The tabId woh's tabs to the left should be closed.
+ */
+ closeTabsToLeftMenuItemClicked: function (tabId) {
+ dispatch({
+ actionType: appConstants.APP_CLOSE_TABS_TO_LEFT_MENU_ITEM_CLICKED,
+ tabId
+ })
+ },
+
+ /**
+ * Menu item for closing tabs to the right has been clicked.
+ * @param {Number} tabId The tabId woh's tabs to the right should be closed.
+ */
+ closeTabsToRightMenuItemClicked: function (tabId) {
+ dispatch({
+ actionType: appConstants.APP_CLOSE_TABS_TO_RIGHT_MENU_ITEM_CLICKED,
+ tabId
+ })
+ },
+
+ /**
+ * Menu item for closing other tabs than the specified tab.
+ * @param {Number} tabId The tabId woh's tabs to the left should be closed.
+ */
+ closeOtherTabsMenuItemClicked: function (tabId) {
+ dispatch({
+ actionType: appConstants.APP_CLOSE_OTHER_TABS_MENU_ITEM_CLICKED,
+ tabId
+ })
+ },
+
  /**
  * A request for a new tab has been made with the specified createProperties
  * @param {Object} createProperties