diff --git a/app/browser/activeTabHistory.js b/app/browser/activeTabHistory.js index a3650face01..9569c9a5197 100644 --- a/app/browser/activeTabHistory.js +++ b/app/browser/activeTabHistory.js @@ -55,6 +55,18 @@ const api = { */ clearTabbedWindow: function (windowId) { activeTabsByWindow.delete(windowId) + }, + + /** + * Replace all intances of `oldTabId` with `newTabId` in + * active tab history for each window + */ + tabIdChanged: function (oldTabId, newTabId) { + for (const [windowId, windowActiveTabs] of activeTabsByWindow) { + if (windowActiveTabs && windowActiveTabs.length) { + activeTabsByWindow.set(windowId, windowActiveTabs.map(previousTabId => (previousTabId === oldTabId) ? newTabId : previousTabId)) + } + } } } diff --git a/app/browser/reducers/tabsReducer.js b/app/browser/reducers/tabsReducer.js index b2935dbffba..fd6fa21ca0d 100644 --- a/app/browser/reducers/tabsReducer.js +++ b/app/browser/reducers/tabsReducer.js @@ -24,6 +24,7 @@ const tabActionConsts = require('../../common/constants/tabAction') const flash = require('../../../js/flash') const {frameOptsFromFrame} = require('../../../js/state/frameStateUtil') const {isSourceAboutUrl, isTargetAboutUrl, isNavigatableAboutPage} = require('../../../js/lib/appUrlUtil') +const {shouldDebugTabEvents} = require('../../cmdLine') const WEBRTC_DEFAULT = 'default' const WEBRTC_DISABLE_NON_PROXY = 'disable_non_proxied_udp' @@ -150,6 +151,19 @@ const tabsReducer = (state, action, immutableAction) => { state = tabState.maybeCreateTab(state, action) // tabs.debugTabs(state) break + case appConstants.APP_TAB_REPLACED: + if (action.get('isPermanent')) { + if (shouldDebugTabEvents) { + console.log('APP_TAB_REPLACED before') + tabs.debugTabs(state) + } + state = tabState.replaceTabValue(state, action.get('oldTabId'), action.get('newTabValue')) + if (shouldDebugTabEvents) { + console.log('APP_TAB_REPLACED after') + tabs.debugTabs(state) + } + } + break case appConstants.APP_TAB_CLOSE_REQUESTED: { let tabId = action.get('tabId') @@ -335,7 +349,7 @@ const tabsReducer = (state, action, immutableAction) => { break } case appConstants.APP_FRAME_CHANGED: - state = tabState.updateFrame(state, action) + state = tabState.updateFrame(state, action, shouldDebugTabEvents) break case windowConstants.WINDOW_SET_FRAME_ERROR: { diff --git a/app/browser/tabs.js b/app/browser/tabs.js index b5e0da8b6c4..93bb4bbed01 100644 --- a/app/browser/tabs.js +++ b/app/browser/tabs.js @@ -434,6 +434,9 @@ const api = { }) process.on('add-new-contents', (e, source, newTab, disposition, size, userGesture) => { + if (shouldDebugTabEvents) { + console.log(`Contents [${newTab.getId()}] process:add-new-contents`, {userGesture, isBackground: newTab.isBackgroundPage(), disposition}) + } if (userGesture === false) { e.preventDefault() return @@ -458,7 +461,10 @@ const api = { } const tabId = newTab.getId() + + // update our webContents Map with the openerTabId webContentsCache.updateWebContents(tabId, newTab, openerTabId) + let newTabValue = getTabValue(newTab.getId()) let windowId @@ -526,6 +532,10 @@ const api = { } const tabId = tab.getId() + // This is the first event for WebContents so cache the item in the Map + // This is not ideal since we do not know the old tabId here, we cannot + webContentsCache.updateWebContents(tabId, tab, null) + // command-line flag --debug-tab-events if (shouldDebugTabEvents) { console.log(`Tab [${tabId}] created in window ${tab.tabValue().windowId}`) @@ -609,6 +619,29 @@ const api = { } }) + tab.on('tab-replaced-at', (e, windowId, tabIndex, newContents) => { + // if not a placeholder, new contents is permanent replacement, e.g. tab has been discarded + // if is a placeholder, new contents is temporary, and should not be used for tab ID + const isPlaceholder = newContents.isPlaceholder() + const newTabId = newContents.getId() + + if (shouldDebugTabEvents) { + if (isPlaceholder) { + console.log(`Tab [${tabId}] got a new placeholder (${newTabId}), not updating state.`) + } else { + console.log(`Tab [${tabId}] permanently changed to tabId ${newTabId}. Updating state references...`) + } + } + + // update state + appActions.tabReplaced(tabId, getTabValue(newTabId), getTabValue(tabId).get('windowId'), !isPlaceholder) + if (!isPlaceholder) { + // update in-memory caches + webContentsCache.tabIdChanged(tabId, newTabId) + activeTabHistory.tabIdChanged(tabId, newTabId) + } + }) + 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. @@ -873,6 +906,9 @@ const api = { }, closeTab: (tabId, forceClosePinned = false) => { + if (shouldDebugTabEvents) { + console.log(`[${tabId}] tabs.closeTab(forceClosePinned: ${forceClosePinned})`) + } const tabValue = getTabValue(tabId) if (!tabValue) { return false @@ -938,12 +974,6 @@ const api = { const preventAutoDiscard = createProperties.pinned || !isRegularContent if (preventAutoDiscard) { createProperties.autoDiscardable = false - } else { - // (temporarily) forced autoDiscardable to ALWAYS false due to - // inability to switch to auto-discarded tabs. - // See https://github.com/brave/browser-laptop/issues/10673 - // remove this forced 'else' condition when #10673 is resolved - createProperties.autoDiscardable = false } const doCreate = () => { @@ -998,6 +1028,9 @@ const api = { }, moveTo: (state, tabId, frameOpts, browserOpts, toWindowId) => { + if (shouldDebugTabEvents) { + console.log(`Tab ${tabId}] tabs.moveTo(window: ${toWindowId})`) + } frameOpts = makeImmutable(frameOpts) browserOpts = makeImmutable(browserOpts) const tab = webContentsCache.getWebContents(tabId) @@ -1027,8 +1060,24 @@ const api = { return } - // detach from current window + // perform detach from current window + // and handle at `did-detach` tab.detach(() => { + if (shouldDebugTabEvents) { + console.log(`Tab [${tabId}] detached tab guestinstance id`, tab.guestInstanceId) + } + frameOpts = frameOpts.set('guestInstanceId', tab.guestInstanceId) + // handle tab has made it to the new window + tab.once('did-attach', () => { + if (shouldDebugTabEvents) { + console.log(`Tab [${tabId}] attached to a new window, so setting the desired index`) + } + // put the tab in the desired index position + const index = frameOpts.get('index') + if (index !== undefined) { + api.setTabIndex(tabId, frameOpts.get('index')) + } + }) // handle tab has detached from window // handle tab was the active tab of the window if (tabValue.get('active')) { @@ -1038,7 +1087,12 @@ const api = { api.setActive(nextActiveTabIdForOldWindow) } } + // tell another window to add the tab, this will cause the tab to render a webview and + // then attach to that window if (toWindowId == null || toWindowId === -1) { + if (shouldDebugTabEvents) { + console.log('creating new window for detached tab') + } // move tab to a new window frameOpts = frameOpts.set('index', 0) appActions.newWindow(frameOpts, browserOpts) @@ -1047,14 +1101,6 @@ const api = { // specified window appActions.newWebContentsAdded(toWindowId, frameOpts, tabValue) } - // handle tab has made it to the new window - tab.once('did-attach', () => { - // put the tab in the desired index position - const index = frameOpts.get('index') - if (index !== undefined) { - api.setTabIndex(tabId, frameOpts.get('index')) - } - }) }) } }, diff --git a/app/browser/webContentsCache.js b/app/browser/webContentsCache.js index e483ffb3bf6..0c8c580b85b 100644 --- a/app/browser/webContentsCache.js +++ b/app/browser/webContentsCache.js @@ -31,11 +31,23 @@ const forgetOpenerForTabId = (tabId) => { } } +const tabIdChanged = (oldTabId, newTabId) => { + // any tabs referencing the old contents Id as the opener, + // should now reference the new contents Id + for (const tabId in currentWebContents) { + const tabData = currentWebContents[tabId] + if (tabData && tabData.openerTabId != null && tabData.openerTabId === oldTabId) { + tabData.openerTabId = newTabId + } + } +} + module.exports = { cleanupWebContents, getWebContents, getOpenerTabId, forgetOpenerForTabId, updateWebContents, + tabIdChanged, currentWebContents } diff --git a/app/common/state/tabState.js b/app/common/state/tabState.js index 74cb2cf1222..268c3e22982 100644 --- a/app/common/state/tabState.js +++ b/app/common/state/tabState.js @@ -404,6 +404,31 @@ const tabState = { return state.set('tabs', tabs.delete(index).insert(index, tabValue)) }, + replaceTabValue: (state, tabId, newTabValue) => { + state = validateState(state) + newTabValue = makeImmutable(newTabValue) + // update tab + const index = getTabInternalIndexByTabId(state, tabId) + const oldTabValue = state.getIn(['tabs', index]) + if (index == null || index === -1) { + console.error(`tabState: cannot replace tab ${tabId} as tab's index did not exist in state`, { index }) + return state + } + let mergedTabValue = oldTabValue.mergeDeep(newTabValue) + if (mergedTabValue.has('frame')) { + mergedTabValue = mergedTabValue.mergeIn(['frame'], { + tabId: newTabValue.get('tabId'), + guestInstanceId: newTabValue.get('guestInstanceId') + }) + } + mergedTabValue = mergedTabValue.set('windowId', oldTabValue.get('windowId')) + state = state.set('tabs', state.get('tabs').delete(index).insert(index, mergedTabValue)) + // update tabId at tabsInternal index + state = deleteTabsInternalIndex(state, oldTabValue) + state = updateTabsInternalIndex(state, 0) + return state + }, + removeTabField: (state, field) => { state = makeImmutable(state) @@ -417,19 +442,30 @@ const tabState = { return state.set('tabs', tabs) }, - updateFrame: (state, action) => { + updateFrame: (state, action, shouldDebugTabEvents = false) => { state = validateState(state) action = validateAction(action) const tabId = action.getIn(['frame', 'tabId']) + if (!tabId) { + if (shouldDebugTabEvents) { + console.log(`Tab [${tabId}] frame changed for tab - no tabId provided!`) + } return state } let tabValue = tabState.getByTabId(state, tabId) if (!tabValue) { + if (shouldDebugTabEvents) { + console.log(`Tab [${tabId}] frame changed for tab - tab not found in state, probably a temporary frame`) + } return state } + if (shouldDebugTabEvents) { + console.log(`Tab [${tabId}] frame changed for tab`) + } + const bookmarkUtil = require('../lib/bookmarkUtil') const frameLocation = action.getIn(['frame', 'location']) const frameBookmarked = bookmarkUtil.isLocationBookmarked(state, frameLocation) diff --git a/app/renderer/components/frame/frame.js b/app/renderer/components/frame/frame.js index 603a717171f..9b4f92bca23 100644 --- a/app/renderer/components/frame/frame.js +++ b/app/renderer/components/frame/frame.js @@ -433,13 +433,9 @@ class Frame extends React.Component { } addEventListeners () { - this.webview.addEventListener('tab-id-changed', (e) => { - if (this.frame.isEmpty()) { - return - } - - windowActions.frameTabIdChanged(this.frame, this.props.tabId, e.tabID) - }, { passive: true }) + // Webview al exposes the 'tab-id-changed' event, with e.tabID as the new tabId. + // We don't handle that event anymore, in favor of tab-replaced-at in the browser process. + // Keeping this comment here as it is not documented - petemill. this.webview.addEventListener('guest-ready', (e) => { if (this.frame.isEmpty()) { return @@ -929,6 +925,9 @@ class Frame extends React.Component { const transitionClassName = this.getTransitionStateClassName(this.props.transitionState) return
{ const frameTabIdChanged = (state, action) => { action = makeImmutable(action) const oldTabId = action.get('oldTabId') - const newTabId = action.get('newTabId') + const newTabValue = action.get('newTabValue') + const newTabId = newTabValue.get('tabId') if (newTabId == null || oldTabId === newTabId) { + console.error('Invalid action arguments for frameTabIdChanged') return state } let newFrameProps = new Immutable.Map() newFrameProps = newFrameProps.set('tabId', newTabId) - const index = frameStateUtil.getFrameIndex(state, action.getIn(['frameProps', 'key'])) + const frame = frameStateUtil.getFrameByTabId(state, oldTabId) + if (!frame) { + console.error(`Could not find frame with tabId ${oldTabId} in order to replace with new tabId ${newTabId}`) + return state + } + const index = frameStateUtil.getFrameIndex(state, frame.get('key')) state = state.mergeIn(['frames', index], newFrameProps) state = frameStateUtil.deleteTabInternalIndex(state, oldTabId) state = frameStateUtil.updateFramesInternalIndex(state, index) @@ -263,7 +270,7 @@ const doAction = (action) => { } // We should not emit here because the Window already know about the change on startup. return - case windowConstants.WINDOW_FRAME_TAB_ID_CHANGED: + case appConstants.APP_TAB_REPLACED: windowState = frameTabIdChanged(windowState, action) break case windowConstants.WINDOW_FRAME_GUEST_INSTANCE_ID_CHANGED: