From b7141404c6a7dfd1fc2bc7f8127d42e968f4a0d4 Mon Sep 17 00:00:00 2001 From: Pete Miller Date: Wed, 6 Dec 2017 16:42:22 -0800 Subject: [PATCH] After tab close, set next active tab immediately, and only if different from the tab muon will set. Avoids race condition that can occur after will-destroy, before appState has been fully updated with any tab details that change from muon as a result of a tab closing. Fix #11981 Fix #11526 --- app/browser/activeTabHistory.js | 61 +++++ app/browser/reducers/tabsReducer.js | 7 - app/browser/tabs.js | 199 +++++++++------- app/browser/webContentsCache.js | 21 +- app/browser/windows.js | 2 + .../app/browser/reducers/tabsReducerTest.js | 125 ---------- test/unit/app/browser/tabsTest.js | 224 +++++++++++++++--- test/unit/lib/fakeTab.js | 49 ++++ 8 files changed, 438 insertions(+), 250 deletions(-) create mode 100644 app/browser/activeTabHistory.js create mode 100644 test/unit/lib/fakeTab.js diff --git a/app/browser/activeTabHistory.js b/app/browser/activeTabHistory.js new file mode 100644 index 00000000000..a3650face01 --- /dev/null +++ b/app/browser/activeTabHistory.js @@ -0,0 +1,61 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public +* License, v. 2.0. If a copy of the MPL was not distributed with this file, +* You can obtain one at http://mozilla.org/MPL/2.0/. */ + +// static +const activeTabsByWindow = new Map() + +const api = { + /** + * Inform this store that a tab is active for a window. + * This information will be added to index 0 in the + * active-tab history for the window. + */ + setActiveTabForWindow: function (windowId, tabId) { + const existing = activeTabsByWindow.get(windowId) + if (existing) { + existing.unshift(tabId) + } else { + activeTabsByWindow.set(windowId, [ tabId ]) + } + }, + + /** + * Retrieve the tabId that was active at the specified history index, + * 0 for most recent (default) + */ + getActiveTabForWindow: function (windowId, historyIndex = 0) { + // get history of active tabs for specified window + const windowActiveTabs = activeTabsByWindow.get(windowId) + // handle no history for specified window + if (!windowActiveTabs || !windowActiveTabs.length) { + return null + } + // verify specified index in active-tab history exists + const lastIndex = windowActiveTabs.length - 1 + if (lastIndex < historyIndex) { + return null + } + // get tabId at specified index in active-tab history + return windowActiveTabs[historyIndex] + }, + + /** + * Removes specified tab from active-tab history in specified window + */ + clearTabFromWindow: function (windowId, tabId) { + const windowActiveTabs = activeTabsByWindow.get(windowId) + if (windowActiveTabs && windowActiveTabs.length) { + activeTabsByWindow.set(windowId, windowActiveTabs.filter(previousTabId => previousTabId !== tabId)) + } + }, + + /** + * Forget history of active tabs for specified window + */ + clearTabbedWindow: function (windowId) { + activeTabsByWindow.delete(windowId) + } +} + +module.exports = api diff --git a/app/browser/reducers/tabsReducer.js b/app/browser/reducers/tabsReducer.js index feb3b816909..eda6224f01e 100644 --- a/app/browser/reducers/tabsReducer.js +++ b/app/browser/reducers/tabsReducer.js @@ -201,8 +201,6 @@ const tabsReducer = (state, action, immutableAction) => { if (tabId === tabState.TAB_ID_NONE) { break } - const nextActiveTabId = tabs.getNextActiveTab(state, tabId) - // Must be called before tab is removed // But still check for no tabId because on tab detach there's a dummy tabId const tabValue = tabState.getByTabId(state, tabId) @@ -211,11 +209,6 @@ const tabsReducer = (state, action, immutableAction) => { state = tabs.updateTabsStateForWindow(state, windowIdOfTabBeingRemoved) } state = tabState.removeTabByTabId(state, tabId) - setImmediate(() => { - if (nextActiveTabId !== tabState.TAB_ID_NONE) { - tabs.setActive(nextActiveTabId) - } - }) tabs.forgetTab(tabId) } break diff --git a/app/browser/tabs.js b/app/browser/tabs.js index 2e0b24ae392..c6d4f84c4a7 100644 --- a/app/browser/tabs.js +++ b/app/browser/tabs.js @@ -8,14 +8,13 @@ const tabActions = require('../common/actions/tabActions') const config = require('../../js/constants/config') const Immutable = require('immutable') const tabState = require('../common/state/tabState') -const windowState = require('../common/state/windowState') const {app, BrowserWindow, extensions, session, ipcMain} = require('electron') const {makeImmutable, makeJS} = require('../common/state/immutableUtil') const {getTargetAboutUrl, getSourceAboutUrl, isSourceAboutUrl, newFrameUrl, isTargetAboutUrl, isIntermediateAboutPage, isTargetMagnetUrl, getSourceMagnetUrl} = require('../../js/lib/appUrlUtil') const {isURL, getUrlFromInput, toPDFJSLocation, getDefaultFaviconUrl, isHttpOrHttps, getLocationIfPDF} = require('../../js/lib/urlutil') const {isSessionPartition} = require('../../js/state/frameStateUtil') const {getOrigin} = require('../../js/lib/urlutil') -const {getSetting} = require('../../js/settings') +const settingsStore = require('../../js/settings') const settings = require('../../js/constants/settings') const {getBaseUrl} = require('../../js/lib/appUrlUtil') const siteSettings = require('../../js/state/siteSettings') @@ -27,7 +26,7 @@ const appStore = require('../../js/stores/appStore') const appConfig = require('../../js/constants/appConfig') const {newTabMode} = require('../common/constants/settingsEnums') const {tabCloseAction} = require('../common/constants/settingsEnums') -const {cleanupWebContents, currentWebContents, getWebContents, updateWebContents} = require('./webContentsCache') +const webContentsCache = require('./webContentsCache') const {FilterOptions} = require('ad-block') const {isResourceEnabled} = require('../filtering') const autofill = require('../autofill') @@ -38,6 +37,7 @@ const siteSettingsState = require('../common/state/siteSettingsState') const bookmarkOrderCache = require('../common/cache/bookmarkOrderCache') const ledgerState = require('../common/state/ledgerState') const {getWindow} = require('./windows') +const activeTabHistory = require('./activeTabHistory') let adBlockRegions let currentPartitionNumber = 0 @@ -50,14 +50,14 @@ const normalizeUrl = function (url) { if (isURL(url)) { url = getUrlFromInput(url) } - if (getSetting(settings.PDFJS_ENABLED)) { + if (settingsStore.getSetting(settings.PDFJS_ENABLED)) { url = toPDFJSLocation(url) } return url } const getTabValue = function (tabId) { - let tab = getWebContents(tabId) + let tab = webContentsCache.getWebContents(tabId) if (tab && !tab.isDestroyed()) { let tabValue = makeImmutable(tab.tabValue()) tabValue = tabValue.set('canGoBack', tab.canGoBack()) @@ -124,7 +124,7 @@ const aboutTabUpdateListener = (tabId) => { } for (let tabId in aboutTabs) { - let tab = getWebContents(tabId) + let tab = webContentsCache.getWebContents(tabId) if (!tab || tab.isDestroyed()) { delete aboutTabs[tabId] } else { @@ -190,7 +190,7 @@ const sendAboutDetails = (tabId, type, value, shared = false) => { // use a weak map to avoid holding references to large objects that will never be equal to anything aboutTabs[tabId][type] = aboutTabs[tabId][type] || new WeakMap() if (aboutTabs[tabId] && !aboutTabs[tabId][type].get(value)) { - const tab = getWebContents(tabId) + const tab = webContentsCache.getWebContents(tabId) if (tab && !tab.isDestroyed()) { if (shared) { const handle = muon.shared_memory.create(makeJS(value)) @@ -289,8 +289,8 @@ const updateAboutDetails = (tabId) => { sendAboutDetails(tabId, messages.BRAVERY_DEFAULTS_UPDATED, braveryDefaults.toJS()) } else if (location === 'about:newtab') { const newTabDetail = aboutNewTabState.getData(appState) - const showEmptyPage = getSetting(settings.NEWTAB_MODE) === newTabMode.EMPTY_NEW_TAB - const showImages = getSetting(settings.SHOW_DASHBOARD_IMAGES) && !showEmptyPage + const showEmptyPage = settingsStore.getSetting(settings.NEWTAB_MODE) === newTabMode.EMPTY_NEW_TAB + const showImages = settingsStore.getSetting(settings.SHOW_DASHBOARD_IMAGES) && !showEmptyPage const trackedBlockersCount = appState.getIn(['trackingProtection', 'count'], 0) const httpsUpgradedCount = appState.getIn(['httpsEverywhere', 'count'], 0) const adblockCount = appState.getIn(['adblock', 'count'], 0) @@ -462,7 +462,7 @@ const api = { } const tabId = newTab.getId() - updateWebContents(tabId, newTab) + webContentsCache.updateWebContents(tabId, newTab, openerTabId) let newTabValue = getTabValue(newTab.getId()) let windowId @@ -569,10 +569,25 @@ const api = { appActions.tabMoved(tabId) }) - tab.on('will-attach', () => { + tab.on('will-attach', (e, windowWebContents) => { appActions.tabWillAttach(tab.getId()) }) + tab.on('set-active', (sender, isActive) => { + if (isActive) { + const tabValue = getTabValue(tabId) + if (tabValue) { + const windowId = tabValue.get('windowId') + // set-active could be called multiple times even when the index does not change + // so make sure we only add this to the active-tab trail for the window + // once + if (activeTabHistory.getActiveTabForWindow(windowId, 0) !== tabId) { + activeTabHistory.setActiveTabForWindow(windowId, tabId) + } + } + } + }) + tab.on('tab-strip-empty', () => { // It's only safe to close a window when the last web-contents tab has been // re-attached. A detach which already happens by this point is not enough. @@ -586,7 +601,17 @@ const api = { }) }) - tab.on('did-attach', () => { + tab.on('did-detach', (e, oldTabId) => { + // forget last active trail in window tab + // is detaching from + const oldTab = getTabValue(oldTabId) + const detachedFromWindowId = oldTab.get('windowId') + if (detachedFromWindowId != null) { + activeTabHistory.clearTabFromWindow(detachedFromWindowId, oldTabId) + } + }) + + tab.on('did-attach', (e, tabId) => { appActions.tabAttached(tab.getId()) }) @@ -604,10 +629,24 @@ const api = { } }) - tab.once('will-destroy', () => { + tab.once('will-destroy', (e) => { const tabValue = getTabValue(tabId) if (tabValue) { const windowId = tabValue.get('windowId') + // forget about this tab in the history of active tabs + activeTabHistory.clearTabFromWindow(windowId, tabId) + // handle closed tab being the current active tab for window + if (tabValue.get('active')) { + // set the next active tab, if different from what muon will have set to + // Muon sets it to the next index (immediately above or below) + // But this app can be configured to select the parent tab, + // or the last active tab + let nextTabId = api.getNextActiveTabId(windowId, tabId) + if (nextTabId != null) { + api.setActive(nextTabId) + } + } + // let the state know appActions.tabClosed(tabId, windowId) } }) @@ -631,11 +670,11 @@ const api = { }, sendToAll: (...args) => { - for (let tabId in currentWebContents) { - const tab = currentWebContents[tabId] + for (let tabId in webContentsCache.currentWebContents) { + const tabData = webContentsCache.currentWebContents[tabId] try { - if (tab && !tab.isDestroyed()) { - tab.send(...args) + if (tabData && tabData.tab && !tabData.tab.isDestroyed()) { + tabData.tab.send(...args) } } catch (e) { // ignore exceptions @@ -644,7 +683,7 @@ const api = { }, toggleDevTools: (tabId) => { - const tab = getWebContents(tabId) + const tab = webContentsCache.getWebContents(tabId) if (tab && !tab.isDestroyed()) { if (tab.isDevToolsOpened()) { tab.closeDevTools() @@ -655,28 +694,28 @@ const api = { }, inspectElement: (tabId, x, y) => { - const tab = getWebContents(tabId) + const tab = webContentsCache.getWebContents(tabId) if (tab && !tab.isDestroyed()) { tab.inspectElement(x, y) } }, setActive: (tabId) => { - let tab = getWebContents(tabId) + let tab = webContentsCache.getWebContents(tabId) if (tab && !tab.isDestroyed()) { tab.setActive(true) } }, setTabIndex: (tabId, index) => { - let tab = getWebContents(tabId) + let tab = webContentsCache.getWebContents(tabId) if (tab && !tab.isDestroyed()) { tab.setTabIndex(index) } }, reload: (tabId, ignoreCache = false) => { - const tab = getWebContents(tabId) + const tab = webContentsCache.getWebContents(tabId) if (tab && !tab.isDestroyed()) { // TODO(bridiver) - removeEntryAtIndex for intermediate about pages after loading if (isIntermediateAboutPage(getSourceAboutUrl(tab.getURL()))) { @@ -690,7 +729,7 @@ const api = { loadURL: (action) => { action = makeImmutable(action) const tabId = action.get('tabId') - const tab = getWebContents(tabId) + const tab = webContentsCache.getWebContents(tabId) if (tab && !tab.isDestroyed()) { let url = normalizeUrl(action.get('url')) // We only allow loading URLs explicitly when the origin is @@ -716,7 +755,7 @@ const api = { }, loadURLInTab: (state, tabId, url) => { - const tab = getWebContents(tabId) + const tab = webContentsCache.getWebContents(tabId) if (tab && !tab.isDestroyed()) { url = normalizeUrl(url) tab.loadURL(url) @@ -728,7 +767,7 @@ const api = { action = makeImmutable(action) const muted = action.get('muted') const tabId = action.get('tabId') - const tab = getWebContents(tabId) + const tab = webContentsCache.getWebContents(tabId) if (tab && !tab.isDestroyed()) { tab.setAudioMuted(muted) } @@ -745,7 +784,7 @@ const api = { options = options.set('index', index + 1) } } - const tab = getWebContents(tabId) + const tab = webContentsCache.getWebContents(tabId) if (tab && !tab.isDestroyed()) { tab.clone(options.toJS(), (newTab) => { }) @@ -753,7 +792,7 @@ const api = { }, pin: (state, tabId, pinned) => { - const tab = getWebContents(tabId) + const tab = webContentsCache.getWebContents(tabId) if (tab && !tab.isDestroyed()) { const url = tab.getURL() // For now we only support 1 tab pin per URL @@ -770,7 +809,7 @@ const api = { }, isDevToolsFocused: (tabId) => { - const tab = getWebContents(tabId) + const tab = webContentsCache.getWebContents(tabId) return tab && !tab.isDestroyed() && tab.isDevToolsOpened() && @@ -782,7 +821,7 @@ const api = { if (!tabValue) { return false } - const tab = getWebContents(tabId) + const tab = webContentsCache.getWebContents(tabId) try { if (tab && !tab.isDestroyed()) { if (tabValue.get('pinned') && !forceClosePinned) { @@ -809,7 +848,7 @@ const api = { createProperties.url = createProperties.location delete createProperties.location } - const switchToNewTabsImmediately = getSetting(settings.SWITCH_TO_NEW_TABS) === true + const switchToNewTabsImmediately = settingsStore.getSetting(settings.SWITCH_TO_NEW_TABS) === true if (!isRestore && switchToNewTabsImmediately) { createProperties.active = true } @@ -889,7 +928,7 @@ const api = { moveTo: (state, tabId, frameOpts, browserOpts, toWindowId) => { frameOpts = makeImmutable(frameOpts) browserOpts = makeImmutable(browserOpts) - const tab = getWebContents(tabId) + const tab = webContentsCache.getWebContents(tabId) if (tab && !tab.isDestroyed()) { const tabValue = getTabValue(tabId) const guestInstanceId = tabValue && tabValue.get('guestInstanceId') @@ -915,22 +954,30 @@ const api = { // If the current tab is pinned, then don't allow to drag out return } - const nextActiveTabIdForOldWindow = api.getNextActiveTab(state, tabId) + + // detach from current window tab.detach(() => { + // handle tab has detached from window + // handle tab was the active tab of the window + if (tabValue.get('active')) { + // decide and set next-active-tab muon override + const nextActiveTabIdForOldWindow = api.getNextActiveTabId(currentWindowId, tabId) + if (nextActiveTabIdForOldWindow !== null) { + api.setActive(nextActiveTabIdForOldWindow) + } + } if (toWindowId == null || toWindowId === -1) { + // move tab to a new window frameOpts = frameOpts.set('index', 0) appActions.newWindow(frameOpts, browserOpts) } else { + // ask for tab to be attached (via frame state and webview) to + // specified window appActions.newWebContentsAdded(toWindowId, frameOpts, tabValue) } - - // Setting the next active tab for the old window must happen after re-attach of the new tab. - // This is because muon's tab_strip index for the tab would not be consistent with browser-laptop's - // expectation and it would try to set an invalid index as active, possibly leaivng nothing active. + // handle tab has made it to the new window tab.once('did-attach', () => { - if (nextActiveTabIdForOldWindow !== tabState.TAB_ID_NONE) { - api.setActive(nextActiveTabIdForOldWindow) - } + // put the tab in the desired index position const index = frameOpts.get('index') if (index !== undefined) { api.setTabIndex(tabId, frameOpts.get('index')) @@ -957,14 +1004,14 @@ const api = { }, setWebRTCIPHandlingPolicy: (tabId, policy) => { - const tab = getWebContents(tabId) + const tab = webContentsCache.getWebContents(tabId) if (tab && !tab.isDestroyed()) { tab.setWebRTCIPHandlingPolicy(policy) } }, goBack: (tabId) => { - const tab = getWebContents(tabId) + const tab = webContentsCache.getWebContents(tabId) if (tab && !tab.isDestroyed()) { // TODO(bridiver) - removeEntryAtIndex for intermediate about pages after loading if (tab.controller().canGoToOffset(-2) && isIntermediateAboutPage(getSourceAboutUrl(tab.getURL()))) { @@ -976,14 +1023,14 @@ const api = { }, goForward: (tabId) => { - const tab = getWebContents(tabId) + const tab = webContentsCache.getWebContents(tabId) if (tab && !tab.isDestroyed() && tab.canGoForward()) { tab.goForward() } }, goToIndex: (tabId, index) => { - const tab = getWebContents(tabId) + const tab = webContentsCache.getWebContents(tabId) const validIndex = index >= 0 && index < tab.getEntryCount() if (tab && !tab.isDestroyed() && validIndex) { tab.goToIndex(index) @@ -991,7 +1038,7 @@ const api = { }, getHistoryEntries: (state, action) => { - const tab = getWebContents(action.get('tabId')) + const tab = webContentsCache.getWebContents(action.get('tabId')) const sites = state ? historyState.getSites(state) : null if (tab && !tab.isDestroyed()) { @@ -1036,55 +1083,33 @@ const api = { return null }, - getNextActiveTab: (state, closeTabId) => { - if (!tabState.getByTabId(state, closeTabId)) { - return - } - - const index = tabState.getIndex(state, closeTabId) - if (index === -1) { - return - } - - const windowId = tabState.getWindowId(state, closeTabId) - if (windowId === windowState.WINDOW_ID_NONE) { - return - } + getNextActiveTabId: (windowId, closedTabId) => { + const nextTabActionType = settingsStore.getSetting(settings.TAB_CLOSE_ACTION) - const lastActiveTabId = tabState.getTabsByLastActivated(state, windowId).last() - if (lastActiveTabId !== closeTabId && !tabState.isActive(state, closeTabId)) { - return + if (nextTabActionType === tabCloseAction.LAST_ACTIVE) { + return activeTabHistory.getActiveTabForWindow(windowId) } - let nextTabId = tabState.TAB_ID_NONE - switch (getSetting(settings.TAB_CLOSE_ACTION)) { - case tabCloseAction.LAST_ACTIVE: - nextTabId = tabState.getLastActiveTabId(state, windowId) - break - case tabCloseAction.PARENT: - { - const openerTabId = tabState.getOpenerTabId(state, closeTabId) - if (openerTabId !== tabState.TAB_ID_NONE) { - nextTabId = openerTabId + if (nextTabActionType === tabCloseAction.PARENT) { + const parentTabId = webContentsCache.getOpenerTabId(closedTabId) + if (parentTabId != null) { + const parentTab = getTabValue(parentTabId) + // make sure parent tab still exists + if (parentTab) { + // make sure parent tab is same window (it may have been detached) + // otherwise we'll make it active in the wrong window + if (parentTab.get('windowId') === windowId) { + return parentTabId } - break } - } - - // DEFAULT: always fall back to NEXT - if (nextTabId === tabState.TAB_ID_NONE) { - nextTabId = tabState.getNextTabIdByIndex(state, windowId, index) - if (nextTabId === tabState.TAB_ID_NONE) { - // no unpinned tabs so find the next pinned tab - nextTabId = tabState.getNextTabIdByIndex(state, windowId, index, true) } } - return nextTabId + return null }, closeTabPage: (state, windowId, tabPageIndex) => { - const tabsPerPage = Number(getSetting(settings.TABS_PER_PAGE)) + const tabsPerPage = Number(settingsStore.getSetting(settings.TABS_PER_PAGE)) const startTabIndex = tabPageIndex * tabsPerPage const pinnedTabsCount = tabState.getPinnedTabsByWindowId(state, windowId).size tabState.getTabsByWindowId(state, windowId) @@ -1092,7 +1117,7 @@ const api = { .filter((tabValue) => !tabValue.get('pinned')) .slice(startTabIndex + pinnedTabsCount, startTabIndex + tabsPerPage + pinnedTabsCount) .forEach((tabValue) => { - const tab = getWebContents(tabValue.get('tabId')) + const tab = webContentsCache.getWebContents(tabValue.get('tabId')) if (tab && !tab.isDestroyed()) { tab.forceClose() } @@ -1114,7 +1139,7 @@ const api = { .filter((tabValue) => !tabValue.get('pinned')) .slice(0, index - pinnedTabsCount) .forEach((tabValue) => { - const tab = getWebContents(tabValue.get('tabId')) + const tab = webContentsCache.getWebContents(tabValue.get('tabId')) if (tab && !tab.isDestroyed()) { tab.forceClose() } @@ -1136,7 +1161,7 @@ const api = { .filter((tabValue) => !tabValue.get('pinned')) .slice(index + 1 - pinnedTabsCount) .forEach((tabValue) => { - const tab = getWebContents(tabValue.get('tabId')) + const tab = webContentsCache.getWebContents(tabValue.get('tabId')) if (tab && !tab.isDestroyed()) { tab.forceClose() } @@ -1153,7 +1178,7 @@ const api = { const windowId = tabValue.get('windowId') tabState.getTabsByWindowId(state, windowId) .forEach((tabValue) => { - const tab = getWebContents(tabValue.get('tabId')) + const tab = webContentsCache.getWebContents(tabValue.get('tabId')) if (tab && !tab.isDestroyed() && tabValue.get('tabId') !== tabId && !tabValue.get('pinned')) { tab.forceClose() } @@ -1221,7 +1246,7 @@ const api = { return state }, forgetTab: (tabId) => { - cleanupWebContents(tabId) + webContentsCache.cleanupWebContents(tabId) } } diff --git a/app/browser/webContentsCache.js b/app/browser/webContentsCache.js index 77f35ab6f31..e483ffb3bf6 100644 --- a/app/browser/webContentsCache.js +++ b/app/browser/webContentsCache.js @@ -11,16 +11,31 @@ const cleanupWebContents = (tabId) => { } const getWebContents = (tabId) => { - return currentWebContents[tabId] + const tabData = currentWebContents[tabId] + return tabData ? tabData.tab : null } -const updateWebContents = (tabId, tab) => { - currentWebContents[tabId] = tab +const getOpenerTabId = (tabId) => { + const tabData = currentWebContents[tabId] + return tabData ? tabData.openerTabId : null +} + +const updateWebContents = (tabId, tab, openerTabId) => { + currentWebContents[tabId] = { tab, openerTabId } +} + +const forgetOpenerForTabId = (tabId) => { + const tabData = currentWebContents[tabId] + if (tabData) { + tabData.openerTabId = null + } } module.exports = { cleanupWebContents, getWebContents, + getOpenerTabId, + forgetOpenerForTabId, updateWebContents, currentWebContents } diff --git a/app/browser/windows.js b/app/browser/windows.js index 07eb8577990..5b4aedf12d9 100644 --- a/app/browser/windows.js +++ b/app/browser/windows.js @@ -22,6 +22,7 @@ const platformUtil = require('../common/lib/platformUtil') const windowState = require('../common/state/windowState') const pinnedSitesState = require('../common/state/pinnedSitesState') const {zoomLevel} = require('../common/constants/toolbarUserInterfaceScale') +const activeTabHistory = require('./activeTabHistory') const isDarwin = platformUtil.isDarwin() const {app, BrowserWindow, ipcMain} = electron @@ -43,6 +44,7 @@ const getWindowState = (win) => { const cleanupWindow = (windowId) => { delete currentWindows[windowId] + activeTabHistory.clearTabbedWindow(windowId) } const getWindowValue = (windowId) => { diff --git a/test/unit/app/browser/reducers/tabsReducerTest.js b/test/unit/app/browser/reducers/tabsReducerTest.js index cf59fd9c4d7..112397bc274 100644 --- a/test/unit/app/browser/reducers/tabsReducerTest.js +++ b/test/unit/app/browser/reducers/tabsReducerTest.js @@ -8,13 +8,10 @@ const tabActions = require('../../../../../app/common/actions/tabActions') const dragTypes = require('../../../../../js/constants/dragTypes') const fakeElectron = require('../../../lib/fakeElectron') const fakeAdBlock = require('../../../lib/fakeAdBlock') -const {tabCloseAction} = require('../../../../../app/common/constants/settingsEnums') -const settings = require('../../../../../js/constants/settings') require('../../../braveUnit') describe('tabsReducer unit tests', function () { let tabsReducer - let tabCloseSetting before(function () { mockery.enable({ @@ -86,7 +83,6 @@ describe('tabsReducer unit tests', function () { setWebRTCIPHandlingPolicy: sinon.mock(), toggleDevTools: sinon.mock(), closeTab: sinon.mock(), - setActive: sinon.spy(), moveTo: sinon.mock(), reload: sinon.mock(), updateTabsStateForWindow: sinon.mock(), @@ -97,20 +93,11 @@ describe('tabsReducer unit tests', function () { this.windowsAPI = require('../../../../../app/browser/windows') this.tabStateAPI = require('../../../../../app/common/state/tabState') - tabCloseSetting = tabCloseAction.PARENT - mockery.registerMock('tabs', this.tabsAPI) mockery.registerMock('../tabs', this.tabsAPI) mockery.registerMock('../windows', this.windowsAPI) mockery.registerMock('../../common/state/tabState', this.tabStateAPI) - mockery.registerMock('../../js/settings', { getSetting: (settingKey, settingsCollection, value) => { - if (settingKey === settings.TAB_CLOSE_ACTION) { - return tabCloseSetting - } - return false - }}) tabsReducer = require('../../../../../app/browser/reducers/tabsReducer') - this.realTabsAPI = require('../../../../../app/browser/tabs') this.tabsAPI.getNextActiveTab = this.realTabsAPI.getNextActiveTab }) @@ -267,7 +254,6 @@ describe('tabsReducer unit tests', function () { beforeEach(function () { this.removeTabByTabIdSpy = sinon.stub(this.tabStateAPI, 'removeTabByTabId', (state) => state) - this.tabsAPI.setActive.reset() this.tabsAPI.forgetTab.reset() }) @@ -292,117 +278,6 @@ describe('tabsReducer unit tests', function () { this.clock.tick(1510) assert(this.tabStateAPI.removeTabByTabId.notCalled) }) - - describe('when updating active tab', function () { - describe('when TAB_CLOSE_ACTION is set to LAST_ACTIVE', function () { - before(function () { - tabCloseSetting = tabCloseAction.LAST_ACTIVE - }) - after(function () { - tabCloseSetting = tabCloseAction.PARENT - }) - it('chooses the last active tab', function () { - tabsReducer(this.state, action) - this.clock.tick(1510) - assert(this.tabsAPI.setActive.withArgs(3).calledOnce) - assert.equal(this.tabsAPI.updateTabsStateForWindow.getCall(0).args[1], 2) - assert(this.tabsAPI.forgetTab.withArgs(5).calledOnce) - }) - }) - - describe('when TAB_CLOSE_ACTION is set to NEXT', function () { - before(function () { - tabCloseSetting = tabCloseAction.NEXT - }) - after(function () { - tabCloseSetting = tabCloseAction.PARENT - }) - - it('chooses the next tab', function () { - const pickNextAction = { - actionType: action.actionType, - tabId: 3 - } - const testState = this.state - .setIn(['tabs', 2, 'active'], true) - .setIn(['tabs', 2, 'openerTabId'], 5) - .setIn(['tabsInternal', 'lastActive', '2'], Immutable.fromJS([4, 5, 3])) - tabsReducer(testState, pickNextAction) - this.clock.tick(1510) - assert(this.tabsAPI.setActive.withArgs(4).calledOnce) - assert.equal(this.tabsAPI.updateTabsStateForWindow.getCall(0).args[1], 2) - assert(this.tabsAPI.forgetTab.withArgs(3).calledOnce) - }) - - it('chooses next unpinned tab', function () { - const pickNextAction = { - actionType: action.actionType, - tabId: 3 - } - const testState = this.state - .setIn(['tabs', 2, 'active'], true) - .setIn(['tabs', 3, 'active'], false) - .setIn(['tabs', 3, 'pinned'], true) - tabsReducer(testState, pickNextAction) - this.clock.tick(1510) - assert(this.tabsAPI.setActive.withArgs(5).calledOnce) - assert.equal(this.tabsAPI.updateTabsStateForWindow.getCall(0).args[1], 2) - assert(this.tabsAPI.forgetTab.withArgs(3).calledOnce) - }) - - it('chooses previous unpinned tab', function () { - const testState = this.state - .setIn(['tabs', 2, 'active'], true) - .setIn(['tabs', 3, 'active'], false) - .setIn(['tabs', 3, 'pinned'], true) - tabsReducer(testState, action) - this.clock.tick(1510) - assert(this.tabsAPI.setActive.withArgs(3).calledOnce) - assert.equal(this.tabsAPI.updateTabsStateForWindow.getCall(0).args[1], 2) - assert(this.tabsAPI.forgetTab.withArgs(5).calledOnce) - }) - - describe('if no unpinned tabs come after this', function () { - it('considers pinned tabs which come after this', function () { - const pickNextAction = { - actionType: action.actionType, - tabId: 3 - } - const testState = this.state - .setIn(['tabs', 2, 'active'], true) - .setIn(['tabs', 3, 'pinned'], true) - .setIn(['tabs', 4, 'active'], false) - .setIn(['tabs', 4, 'pinned'], true) - tabsReducer(testState, pickNextAction) - this.clock.tick(1510) - assert(this.tabsAPI.setActive.withArgs(4).calledOnce) - assert.equal(this.tabsAPI.updateTabsStateForWindow.getCall(0).args[1], 2) - assert(this.tabsAPI.forgetTab.withArgs(3).calledOnce) - }) - }) - }) - - describe('when TAB_CLOSE_ACTION is set to PARENT', function () { - it('chooses parent tab id (if parent tab was last active)', function () { - tabsReducer(this.state, action) - this.clock.tick(1510) - assert(this.tabsAPI.setActive.withArgs(4).calledOnce) - assert.equal(this.tabsAPI.updateTabsStateForWindow.getCall(0).args[1], 2) - assert(this.tabsAPI.forgetTab.withArgs(5).calledOnce) - }) - - it('chooses parent tab id (even if parent tab was NOT last active)', function () { - const testState = this.state - .setIn(['tabs', 4, 'openerTabId'], 3) - .setIn(['tabsInternal', 'lastActive', '2'], Immutable.fromJS([])) - tabsReducer(testState, action) - this.clock.tick(1510) - assert(this.tabsAPI.setActive.withArgs(3).calledOnce) - assert.equal(this.tabsAPI.updateTabsStateForWindow.getCall(0).args[1], 2) - assert(this.tabsAPI.forgetTab.withArgs(5).calledOnce) - }) - }) - }) }) describe.skip('APP_ALLOW_FLASH_ONCE', function () { diff --git a/test/unit/app/browser/tabsTest.js b/test/unit/app/browser/tabsTest.js index 82ce97d89cb..3eb5c571fb3 100644 --- a/test/unit/app/browser/tabsTest.js +++ b/test/unit/app/browser/tabsTest.js @@ -2,9 +2,12 @@ const mockery = require('mockery') const sinon = require('sinon') const Immutable = require('immutable') -const assert = require('assert') +const { assert } = require('chai') const dragTypes = require('../../../../js/constants/dragTypes') +const settings = require('../../../../js/constants/settings') +const { tabCloseAction } = require('../../../../app/common/constants/settingsEnums') const fakeElectron = require('../../lib/fakeElectron') +const FakeTab = require('../../lib/fakeTab') const fakeAdBlock = require('../../lib/fakeAdBlock') require('../../braveUnit') @@ -64,39 +67,41 @@ describe('tabs API unit tests', function () { addChangeListener: () => {} } + this.fakeGetWebContents = (tabId) => { + const webContents = { + canGoBack: () => true, + canGoForward: () => true, + session: { + partition: 'default' + }, + tabValue: () => + this.state.get('tabs').find((tab) => tab.get('tabId') === tabId), + isDestroyed: () => false, + detach: (cb) => cb(), + once: (event, cb) => { + setImmediate(cb) + } + } + if (tabId === 1) { + Object.assign(webContents, this.tabWithDevToolsClosed) + } else if (tabId === 2) { + Object.assign(webContents, this.tabWithDevToolsOpened) + } else if (tabId === 3) { + Object.assign(webContents, this.tabWithDevToolsOpenedAndFocused) + } + return webContents + } + this.fakeGetOpenerTabId = () => null + mockery.registerMock('electron', fakeElectron) mockery.registerMock('ad-block', fakeAdBlock) mockery.registerMock('../../js/stores/appStore', this.appStore) mockery.registerMock('../filtering', { isResourceEnabled: (resourceName, url, isPrivate) => false }) - - mockery.registerMock('./webContentsCache', { - getWebContents: (tabId) => { - const webContents = { - canGoBack: () => true, - canGoForward: () => true, - session: { - partition: 'default' - }, - tabValue: () => - this.state.get('tabs').find((tab) => tab.get('tabId') === tabId), - isDestroyed: () => false, - detach: (cb) => cb(), - once: (event, cb) => { - setImmediate(cb) - } - } - if (tabId === 1) { - Object.assign(webContents, this.tabWithDevToolsClosed) - } else if (tabId === 2) { - Object.assign(webContents, this.tabWithDevToolsOpened) - } else if (tabId === 3) { - Object.assign(webContents, this.tabWithDevToolsOpenedAndFocused) - } - return webContents - } - }) + this.actualActiveTabHistory = require('../../../../app/browser/activeTabHistory') + this.actualWebContentsCache = require('../../../../app/browser/webContentsCache') + this.settings = require('../../../../js/settings') tabs = require('../../../../app/browser/tabs') appActions = require('../../../../js/actions/appActions') }) @@ -106,6 +111,16 @@ describe('tabs API unit tests', function () { }) describe('toggleDevTools', function () { + before(function () { + this.getWebContentsStub = sinon.stub(this.actualWebContentsCache, 'getWebContents', this.fakeGetWebContents) + this.getOpenerTabIdStub = sinon.stub(this.actualWebContentsCache, 'getOpenerTabId', this.fakeGetOpenerTabId) + }) + + after(function () { + this.getWebContentsStub.restore() + this.getOpenerTabIdStub.restore() + }) + afterEach(function () { this.tabWithDevToolsClosed.openDevTools.reset() this.tabWithDevToolsClosed.closeDevTools.reset() @@ -135,6 +150,16 @@ describe('tabs API unit tests', function () { }) describe('isDevToolsFocused', function () { + before(function () { + this.getWebContentsStub = sinon.stub(this.actualWebContentsCache, 'getWebContents', this.fakeGetWebContents) + this.getOpenerTabIdStub = sinon.stub(this.actualWebContentsCache, 'getOpenerTabId', this.fakeGetOpenerTabId) + }) + + after(function () { + this.getWebContentsStub.restore() + this.getOpenerTabIdStub.restore() + }) + it('returns false if devtools are opened but not focused', function () { assert.equal(tabs.isDevToolsFocused(1), false) }) @@ -151,15 +176,25 @@ describe('tabs API unit tests', function () { this.browserOpts = { positionByMouseCursor: true } + this.getWebContentsStub = sinon.stub(this.actualWebContentsCache, 'getWebContents', this.fakeGetWebContents) + this.getOpenerTabIdStub = sinon.stub(this.actualWebContentsCache, 'getOpenerTabId', this.fakeGetOpenerTabId) + }) + + after(function () { + this.getWebContentsStub.restore() + this.getOpenerTabIdStub.restore() }) + beforeEach(function () { this.newWindowSpy = sinon.spy(appActions, 'newWindow') this.newWebContentsAddedSpy = sinon.spy(appActions, 'newWebContentsAdded') }) + afterEach(function () { this.newWindowSpy.restore() this.newWebContentsAddedSpy.restore() }) + it('moves tab to a new window', function () { const state = this.state.set('dragData', Immutable.fromJS({ windowId: 1, @@ -276,6 +311,139 @@ describe('tabs API unit tests', function () { }) }) + describe('getNextActiveTabId', function () { + // because we aren't currently mocking + // the tab events, namely tab.on('set-active'), + // we manually call activeTabHistory and webContentsCache functions here, + // which is ok as they are specifically for this function's purpose + // but it tightly couples implementation with + // description of expected results. change these usages if use of those + // singleton-style reference dependencies in the actual code changes + + let tabCloseSettingValue + const windowId = 1 + const tabIdParent = 1 + + before(function () { + // we want to use actual instance of webContentsCache for these tests + mockery.deregisterMock('./webContentsCache') + // override TAB_CLOSE_ACTION setting + this.stubGetSettings = sinon.stub(this.settings, 'getSetting', + (settingKey, settingsCollection, value) => { + if (settingKey === settings.TAB_CLOSE_ACTION) { + return tabCloseSettingValue + } + return false + } + ) + }) + + after(function () { + this.stubGetSettings.restore() + }) + + beforeEach(function () { + // add manual entries to webContentsCache and activeTabHistory + // open a tab + this.actualWebContentsCache.updateWebContents(tabIdParent, new FakeTab(tabIdParent, windowId)) + this.actualActiveTabHistory.setActiveTabForWindow(windowId, tabIdParent) + // open some child tabs + for (const tabId of [2, 3, 4]) { + this.actualWebContentsCache.updateWebContents(tabId, new FakeTab(tabId, windowId), tabIdParent) + this.actualActiveTabHistory.setActiveTabForWindow(windowId, tabId) + } + }) + + afterEach(function () { + // clean up our manual entries in webContentsCache + const webContentsCache = this.actualWebContentsCache.currentWebContents + for (const tabId of Object.keys(webContentsCache)) { + delete webContentsCache[tabId] + } + // clean up our manual entries in activeTabHistory + this.actualActiveTabHistory.clearTabbedWindow(1) + this.actualActiveTabHistory.clearTabbedWindow(2) + }) + + it('switches to parent', function () { + // set preference to activate parent tab of closed tab + tabCloseSettingValue = tabCloseAction.PARENT + // set the middle tab as active + this.actualActiveTabHistory.setActiveTabForWindow(windowId, 3) + // close the tab + this.actualActiveTabHistory.clearTabFromWindow(windowId, 3) + // perform test + const actual = tabs.getNextActiveTabId(windowId, 3) + const expected = tabIdParent + assert.equal(actual, expected, 'next active tab id is parent tab id of closed tab') + }) + + it('does not switch to parent if parent is detached', function () { + // set preference to activate parent tab of closed tab + tabCloseSettingValue = tabCloseAction.PARENT + // set the middle tab as active + this.actualActiveTabHistory.setActiveTabForWindow(windowId, 3) + // detach the parent + this.actualActiveTabHistory.clearTabFromWindow(windowId, tabIdParent) + const newParentWindowId = windowId + 1 + this.actualWebContentsCache.getWebContents(tabIdParent).windowId = newParentWindowId + this.actualActiveTabHistory.setActiveTabForWindow(newParentWindowId, tabIdParent) + // close the tab + this.actualActiveTabHistory.clearTabFromWindow(windowId, 3) + // perform test + const actual = tabs.getNextActiveTabId(windowId, 3) + assert.isNull(actual) + }) + + it('does not switch to parent if child is detached', function () { + // set preference to activate parent tab of closed tab + tabCloseSettingValue = tabCloseAction.PARENT + // set the middle tab as active + this.actualActiveTabHistory.setActiveTabForWindow(windowId, 3) + // detach a child tab + this.actualActiveTabHistory.clearTabFromWindow(windowId, 2) + const newChildWindowId = windowId + 1 + this.actualActiveTabHistory.setActiveTabForWindow(newChildWindowId, 2) + const childTab = this.actualWebContentsCache.getWebContents(2) + childTab.windowId = newChildWindowId + // close the tab + this.actualActiveTabHistory.clearTabFromWindow(newChildWindowId, 2) + // perform test + const actual = tabs.getNextActiveTabId(newChildWindowId, 2) + assert.isNull(actual) + }) + + it('switches to last active', function () { + // set preference to active last activated tab + tabCloseSettingValue = tabCloseAction.LAST_ACTIVE + const activeOrder = [3, 1, 4, 2] + for (const tabId of activeOrder) { + this.actualActiveTabHistory.setActiveTabForWindow(windowId, tabId) + } + // close all but the first active tab, and check that the previously-active gets + // chosen for becoming active + for (let i = activeOrder.length - 1; i > 0; i--) { + const closeTabId = activeOrder[i] + // close tab + this.actualActiveTabHistory.clearTabFromWindow(windowId, closeTabId) + // check + const expectedNextActiveTabId = activeOrder[i - 1] + const actualNextActiveTabId = tabs.getNextActiveTabId(windowId, closeTabId) + assert.equal(actualNextActiveTabId, expectedNextActiveTabId, 'next active tab is the previously active tab') + } + }) + + it('switches to next index, by not overriding', function () { + // set preference to next index + tabCloseSettingValue = tabCloseAction.NEXT + // close active tab + this.actualActiveTabHistory.clearTabFromWindow(windowId, 4) + // shouldn't do anything as muon will handle + const actual = tabs.getNextActiveTabId(windowId, 4) + assert.isNull(actual, 'next active tab is not set') + }) + }) + describe.skip('init', function () { it('todo', function () { }) diff --git a/test/unit/lib/fakeTab.js b/test/unit/lib/fakeTab.js new file mode 100644 index 00000000000..e28b176e949 --- /dev/null +++ b/test/unit/lib/fakeTab.js @@ -0,0 +1,49 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + +const EventEmitter = require('events') +const util = require('util') + +let nextGuestInstanceId = 0 + +function FakeTab (id, windowId, guestInstanceId = nextGuestInstanceId++) { + this.id = id + this.windowId = windowId + this.guestInstanceId = guestInstanceId + this.session = { + partition: 'persist:partition-0' + } + this._isDestroyed = false + this._canGoBack = false + this._canGoForward = false +} + +util.inherits(FakeTab, EventEmitter) + +const proto = FakeTab.prototype + +proto.getId = function () { + return this.id +} + +proto.tabValue = function () { + return { + id: this.id, + windowId: this.windowId + } +} + +proto.isDestroyed = function () { + return this._isDestroyed +} + +proto.canGoBack = function () { + return this._canGoBack +} + +proto.canGoForward = function () { + return this._canGoForward +} + +module.exports = FakeTab