[wip] Ensure that tab/WebContents objects are added to memory as soon…

… as they are created.

In the case of tabs which get discarded, they are replaced with a fresh, clean WebContents. We were not receiving this new object, so when the renderer asked for this new object's tabId to be made active, the cache did not have that object.
We must also handle updating references to the old TabId to the new TabId, when remembering opener tab IDs and historical tab 'active' history for a window.

TODO: discarded tab never gets added to tabState
TODO: fix moving a discarded tab to a new window (the detached event never fires after being detached, the 'new' tab gets destroyed immediately, and the original discarded tab is not in tabState)

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))
+      }
+    }
   }
 }
 

app/browser/reducers/tabsReducer.js

@@ -150,6 +150,9 @@ const tabsReducer = (state, action, immutableAction) => {
       state = tabState.maybeCreateTab(state, action)
       // tabs.debugTabs(state)
       break
+    case appConstants.APP_TAB_REPLACED:
+      tabs.tabIdChanged(action.get('oldTabId'), action.get('newTabId'))
+      break
     case appConstants.APP_TAB_CLOSE_REQUESTED:
       {
         let tabId = action.get('tabId')

app/browser/tabs.js

@@ -458,7 +458,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 +529,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}`)
@@ -847,6 +854,14 @@ const api = {
     }
   },
 
+  tabIdChanged: (oldTabId, newTabId) => {
+    if (shouldDebugTabEvents) {
+      console.log(`Tab [${oldTabId}] changed to tabId ${newTabId}. Updating state references...`)
+    }
+    webContentsCache.tabIdChanged(oldTabId, newTabId)
+    activeTabHistory.tabIdChanged(oldTabId, newTabId)
+  },
+
   pin: (state, tabId, pinned) => {
     const tab = webContentsCache.getWebContents(tabId)
     if (tab && !tab.isDestroyed()) {
@@ -998,6 +1013,9 @@ const api = {
   },
 
   moveTo: (state, tabId, frameOpts, browserOpts, toWindowId) => {
+    if (shouldDebugTabEvents) {
+      console.log(`tabs.moveTo: tab:${tabId} to window:${toWindowId}`)
+    }
     frameOpts = makeImmutable(frameOpts)
     browserOpts = makeImmutable(browserOpts)
     const tab = webContentsCache.getWebContents(tabId)
@@ -1027,8 +1045,13 @@ const api = {
         return
       }
 
-      // detach from current window
-      tab.detach(() => {
+      if (shouldDebugTabEvents) {
+        console.log(`moving tab ${tabId} so first detaching it`)
+      }
+      // after tab has been removed from initial window,
+      // set active tab in old window, and
+      // move tab to new window
+      const attachToNewWindow = () => {
         // handle tab has detached from window
         // handle tab was the active tab of the window
         if (tabValue.get('active')) {
@@ -1039,6 +1062,9 @@ const api = {
           }
         }
         if (toWindowId == null || toWindowId === -1) {
+          if (shouldDebugTabEvents) {
+            console.log('creating new window for moved tab')
+          }
           // move tab to a new window
           frameOpts = frameOpts.set('index', 0)
           appActions.newWindow(frameOpts, browserOpts)
@@ -1049,13 +1075,25 @@ const api = {
         }
         // handle tab has made it to the new window
         tab.once('did-attach', () => {
+          if (shouldDebugTabEvents) {
+            console.log(`Tab 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'))
           }
         })
-      })
+      }
+      // detach
+      if (tabValue.get('discarded')) {
+        // if the tab is discarded, detaching will never complete
+        // callback / 'did-detach' will never fire
+        tab.detach()
+        attachToNewWindow()
+      } else {
+        tab.detach(attachToNewWindow)
+      }
     }
   },
 

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
 }

app/renderer/components/frame/frame.js

@@ -437,8 +437,9 @@ class Frame extends React.Component {
       if (this.frame.isEmpty()) {
         return
       }
-
-      windowActions.frameTabIdChanged(this.frame, this.props.tabId, e.tabID)
+      if (this.props.tabId !== e.tabID) {
+        appActions.tabReplaced(this.props.tabId, e.tabID)
+      }
     }, { passive: true })
     this.webview.addEventListener('guest-ready', (e) => {
       if (this.frame.isEmpty()) {

js/actions/appActions.js

@@ -302,6 +302,15 @@ const appActions = {
     })
   },
 
+  tabReplaced: function (oldTabId, newTabId) {
+    console.log('tab replaced', {oldTabId, newTabId})
+    dispatch({
+      actionType: appConstants.APP_TAB_REPLACED,
+      oldTabId,
+      newTabId
+    })
+  },
+
   /**
    * Dispatches a message to the store to set a new frame as the active frame.
    *

js/constants/appConstants.js

@@ -15,6 +15,7 @@ const appConstants = {
   APP_TAB_STRIP_EMPTY: _,
   APP_TAB_CLOSED: _,
   APP_TAB_UPDATED: _,
+  APP_TAB_REPLACED: _,
   APP_ADD_HISTORY_SITE: _,
   APP_SET_STATE: _,
   APP_REMOVE_HISTORY_SITE: _,

js/stores/windowStore.js

@@ -184,7 +184,12 @@ const frameTabIdChanged = (state, action) => {
 
   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 +268,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: