Merge pull request #9219 from brave/pinned-tab-activeFrameKey

only set the activeFrameKey if the tab is already active
brave · Jun 4, 2017 · 2e14350 · 2e14350
1 parent a784cbd
commit 2e14350
Show file tree

Hide file tree

Showing 7 changed files with 277 additions and 20 deletions.
diff --git a/app/browser/tabs.js b/app/browser/tabs.js
@@ -331,7 +331,7 @@ const api = {
       const frameOpts = {
         location,
         partition: newTab.session.partition,
-        openInForeground: !!newTabValue.get('active'),
+        active: !!newTabValue.get('active'),
         guestInstanceId: newTab.guestInstanceId,
         isPinned: !!newTabValue.get('pinned'),
         openerTabId,

diff --git a/app/sessionStore.js b/app/sessionStore.js
@@ -169,6 +169,10 @@ module.exports.cleanPerWindowData = (perWindowData, isShutdown) => {
     delete frame.adblock
     delete frame.noScript
 
+    // clean up any legacy frame opening props
+    delete frame.openInForeground
+    delete frame.disposition
+
     // Guest instance ID's are not valid after restarting.
     // Electron won't know about them.
     delete frame.guestInstanceId

diff --git a/js/state/frameStateUtil.js b/js/state/frameStateUtil.js
@@ -357,15 +357,9 @@ function addFrame (state, frameOpts, newKey, partitionNumber, openInForeground,
     history: []
   }, frameOpts))
 
-  const result = {
+  return {
     frames: frames.splice(insertionIndex, 0, frame)
   }
-
-  if (openInForeground) {
-    result.activeFrameKey = newKey
-  }
-
-  return result
 }
 
 /**

diff --git a/js/stores/windowStore.js b/js/stores/windowStore.js
@@ -98,7 +98,7 @@ const addToHistory = (frameProps) => {
   return history.slice(-10)
 }
 
-const newFrame = (state, frameOpts, openInForeground) => {
+const newFrame = (state, frameOpts) => {
   if (frameOpts === undefined) {
     frameOpts = {}
   }
@@ -117,12 +117,17 @@ const newFrame = (state, frameOpts, openInForeground) => {
   }
   frameOpts.partitionNumber = frameOpts.partitionNumber || 0
 
-  if (frameOpts.disposition) {
+  const active = frameOpts.active
+  delete frameOpts.active
+  delete frameOpts.openInForeground // clean up any legacy openInForeground props
+  let openInForeground = active
+
+  if (openInForeground == null && frameOpts.disposition) {
     openInForeground = frameOpts.disposition !== 'background-tab'
+    delete frameOpts.disposition
   }
 
-  const activeFrame = frameStateUtil.getActiveFrame(state)
-  if (openInForeground === undefined || !activeFrame) {
+  if (openInForeground == null || state.get('activeFrameKey') == null) {
     openInForeground = true
   }
 
@@ -183,10 +188,13 @@ const newFrame = (state, frameOpts, openInForeground) => {
   state = frameStateUtil.updateFramesInternalIndex(state, insertionIndex)
 
   if (openInForeground) {
-    const activeFrame = frameStateUtil.getActiveFrame(state)
-    const tabId = activeFrame.get('tabId')
-    state = updateTabPageIndex(state, activeFrame)
-    if (tabId) {
+    const tabId = frameOpts.tabId
+    const frame = frameStateUtil.getFrameByTabId(state, tabId)
+    state = frameStateUtil.updateTabPageIndex(state, frame)
+    if (active) {
+      // only set the activeFrameKey if the tab is already active
+      state = state.set('activeFrameKey', frame.get('key'))
+    } else {
       appActions.tabActivateRequested(tabId)
     }
   }
@@ -698,7 +706,7 @@ const doAction = (action) => {
         action.frameOpts.tabId = tabValue.get('tabId')
         action.frameOpts.icon = action.frameOpts.icon || tabValue.get('favIconUrl')
       }
-      windowState = newFrame(windowState, action.frameOpts, action.frameOpts.openInForeground)
+      windowState = newFrame(windowState, action.frameOpts)
       break
     case windowConstants.WINDOW_FRAME_MOUSE_ENTER:
       windowState = windowState.setIn(['ui', 'mouseInFrame'], true)

diff --git a/test/app/sessionStoreTest.js b/test/app/sessionStoreTest.js
@@ -2,7 +2,7 @@
 
 const Brave = require('../lib/brave')
 const Immutable = require('immutable')
-const {urlInput, navigatorBookmarked, navigatorNotBookmarked} = require('../lib/selectors')
+const {urlInput, navigatorBookmarked, navigatorNotBookmarked, pinnedTabsTabs, tabsTabs} = require('../lib/selectors')
 const siteTags = require('../../js/constants/siteTags')
 const siteUtil = require('../../js/state/siteUtil')
 
@@ -62,6 +62,91 @@ describe('sessionStore', function () {
     })
   })
 
+  describe('restore active tab', function () {
+    describe('regular tab', function () {
+      Brave.beforeAllServerSetup(this)
+      before(function * () {
+        this.page1Url = Brave.server.url('page1.html')
+        this.page2Url = Brave.server.url('page2.html')
+        this.activeTabSelector = '.frameWrapper.isActive webview[data-frame-key="1"][src="' + this.page1Url + '"]'
+        yield Brave.startApp()
+        yield setup(Brave.app.client)
+        yield Brave.app.client
+          .waitForBrowserWindow()
+          .waitForUrl(Brave.newTabUrl)
+          .loadUrl(this.page1Url)
+          .windowByUrl(Brave.browserWindowUrl)
+          .newTab({url: this.page2Url})
+          .waitForUrl(this.page2Url)
+          .windowByUrl(Brave.browserWindowUrl)
+          .newTab({url: this.page2Url})
+          .waitForUrl(this.page2Url)
+          .windowByUrl(Brave.browserWindowUrl)
+          .waitForElementCount(tabsTabs, 3)
+          .pinTabByIndex(2, true)
+          .waitForExist(pinnedTabsTabs)
+          .waitForElementCount(pinnedTabsTabs, 1)
+          .waitForElementCount(tabsTabs, 2)
+          .activateTabByIndex(0)
+          .waitForExist(this.activeTabSelector)
+        yield Brave.stopApp(false)
+        yield Brave.startApp()
+        yield setup(Brave.app.client)
+      })
+
+      after(function * () {
+        yield Brave.stopApp()
+      })
+
+      it('restores the last active tab', function * () {
+        yield Brave.app.client
+          .waitForUrl(this.page1Url)
+          // page2Url is lazy loaded
+          .waitForBrowserWindow()
+          .waitForExist(this.activeTabSelector)
+      })
+    })
+
+    describe('pinned tab', function () {
+      Brave.beforeAllServerSetup(this)
+      before(function * () {
+        this.page1Url = Brave.server.url('page1.html')
+        this.page2Url = Brave.server.url('page2.html')
+        this.activeTabSelector = '.frameWrapper.isActive webview[data-frame-key="2"][src="' + this.page2Url + '"]'
+        yield Brave.startApp()
+        yield setup(Brave.app.client)
+        yield Brave.app.client
+          .waitForBrowserWindow()
+          .waitForUrl(Brave.newTabUrl)
+          .loadUrl(this.page1Url)
+          .windowByUrl(Brave.browserWindowUrl)
+          .newTab({url: this.page2Url})
+          .waitForUrl(this.page2Url)
+          .windowByUrl(Brave.browserWindowUrl)
+          .pinTabByIndex(1, true)
+          .waitForExist(pinnedTabsTabs)
+          .waitForElementCount(pinnedTabsTabs, 1)
+          .waitForElementCount(tabsTabs, 1)
+          .waitForExist(this.activeTabSelector)
+        yield Brave.stopApp(false)
+        yield Brave.startApp()
+        yield setup(Brave.app.client)
+      })
+
+      after(function * () {
+        yield Brave.stopApp()
+      })
+
+      it('restores the last active pinned tab', function * () {
+        yield Brave.app.client
+          // page1Url is lazy loaded
+          .waitForUrl(this.page2Url)
+          .waitForBrowserWindow()
+          .waitForExist(this.activeTabSelector)
+      })
+    })
+  })
+
   describe('firstRunTimestamp', function () {
     Brave.beforeAllServerSetup(this)
     before(function * () {

diff --git a/test/lib/brave.js b/test/lib/brave.js
@@ -649,6 +649,15 @@ var exports = {
         }, createProperties)
     })
 
+    this.app.client.addCommand('activateTabByIndex', function (index) {
+      return this.waitForTab({index}).getAppState().then((val) => {
+        const tab = val.value.tabs.find((tab) => tab.index === index)
+        return this.execute(function (tabId) {
+          devTools('appActions').tabActivateRequested(tabId)
+        }, tab.tabId)
+      })
+    })
+
     /**
      * Adds a site to the sites list, such as a bookmarks.
      *