From b04a248c522d62f5750e5c9f5dccd334328e1f5e Mon Sep 17 00:00:00 2001 From: Brian Clifton Date: Mon, 12 Jun 2017 00:53:21 -0700 Subject: [PATCH 1/2] "Select its parent tab" setting now works as expected - Don't consider if tab was active or not - Validate tabId before returning requires https://github.com/brave/muon/pull/214 Fixes https://github.com/brave/browser-laptop/issues/9395 Auditors: @bridiver, @BrendanEich --- app/browser/reducers/tabsReducer.js | 5 +-- app/common/state/tabState.js | 10 ++++- .../app/browser/reducers/tabsReducerTest.js | 39 ++++++++++--------- test/unit/app/common/state/tabStateTest.js | 33 ++++++++++------ 4 files changed, 52 insertions(+), 35 deletions(-) diff --git a/app/browser/reducers/tabsReducer.js b/app/browser/reducers/tabsReducer.js index 21780e65951..f6ef4e30f4d 100644 --- a/app/browser/reducers/tabsReducer.js +++ b/app/browser/reducers/tabsReducer.js @@ -51,15 +51,14 @@ const updateActiveTab = (state, closeTabId) => { case tabCloseAction.PARENT: { const openerTabId = tabState.getOpenerTabId(state, closeTabId) - const lastActiveTabId = tabState.getLastActiveTabId(state, windowId) - if (openerTabId === lastActiveTabId) { + if (openerTabId && openerTabId !== tabState.TAB_ID_NONE) { nextTabId = openerTabId } break } } - // always fall back to NEXT + // DEFAULT: always fall back to NEXT if (nextTabId === tabState.TAB_ID_NONE) { nextTabId = tabState.getNextTabIdByIndex(state, windowId, index) if (nextTabId === tabState.TAB_ID_NONE) { diff --git a/app/common/state/tabState.js b/app/common/state/tabState.js index 07cf660b7e1..1e632ecc0f1 100644 --- a/app/common/state/tabState.js +++ b/app/common/state/tabState.js @@ -499,7 +499,15 @@ const tabState = { }, getOpenerTabId: (state, tabId) => { - return tabState.getTabPropertyByTabId(state, tabId, 'openerTabId', tabState.TAB_ID_NONE) + const openerTabId = tabState.getTabPropertyByTabId(state, tabId, 'openerTabId', tabState.TAB_ID_NONE) + if (openerTabId !== tabState.TAB_ID_NONE) { + // Validate that tabId exists + const index = getTabInternalIndexByTabId(state, openerTabId) + if (index !== tabState.TAB_ID_NONE) { + return openerTabId + } + } + return tabState.TAB_ID_NONE }, getIndex: (state, tabId) => { diff --git a/test/unit/app/browser/reducers/tabsReducerTest.js b/test/unit/app/browser/reducers/tabsReducerTest.js index a2f4b79f53d..186b6eb5b5e 100644 --- a/test/unit/app/browser/reducers/tabsReducerTest.js +++ b/test/unit/app/browser/reducers/tabsReducerTest.js @@ -303,6 +303,7 @@ describe('tabsReducer unit tests', function () { after(function () { tabCloseSetting = tabCloseAction.PARENT }) + it('chooses the next tab', function () { const pickNextAction = { actionType: action.actionType, @@ -316,25 +317,8 @@ describe('tabsReducer unit tests', function () { this.clock.tick(1510) assert(this.tabsAPI.setActive.withArgs(4).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) - }) - }) - - describe('when last active tab is not set', function () { - beforeEach(function () { - this.getLastActiveTabIdStub = sinon.stub(this.tabStateAPI, 'getLastActiveTabId') - }) - afterEach(function () { - this.getLastActiveTabIdStub.restore() - }) - it('chooses next unpinned tab if nextTabId is TAB_ID_NONE', function () { + it('chooses next unpinned tab', function () { const pickNextAction = { actionType: action.actionType, tabId: 3 @@ -348,7 +332,7 @@ describe('tabsReducer unit tests', function () { assert(this.tabsAPI.setActive.withArgs(5).calledOnce) }) - it('chooses previous unpinned tab if nextTabId is TAB_ID_NONE', function () { + it('chooses previous unpinned tab', function () { const testState = this.state .setIn(['tabs', 2, 'active'], true) .setIn(['tabs', 3, 'active'], false) @@ -375,6 +359,23 @@ describe('tabsReducer unit tests', function () { }) }) }) + + 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) + }) + + 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) + }) + }) }) }) diff --git a/test/unit/app/common/state/tabStateTest.js b/test/unit/app/common/state/tabStateTest.js index 56fdc6ed70e..18ea5476dd4 100644 --- a/test/unit/app/common/state/tabStateTest.js +++ b/test/unit/app/common/state/tabStateTest.js @@ -13,8 +13,8 @@ const defaultAppState = Immutable.fromJS({ const twoTabsAppState = defaultAppState .set('tabs', Immutable.fromJS([ - { tabId: 1, index: 0, windowId: 1, frameKey: 2 }, - { tabId: 2, index: 1, windowId: 1, frameKey: 1 } + { tabId: 1, index: 0, windowId: 1 }, + { tabId: 2, index: 1, windowId: 1 } ])) .set('tabsInternal', Immutable.fromJS({ index: { @@ -204,7 +204,6 @@ describe('tabState unit tests', function () { let tab = tabState.getByTabId(this.appState, 2) assert(tab) assert.equal(1, tab.get('windowId')) - assert.equal(1, tab.get('frameKey')) assert.equal(2, tab.get('tabId')) }) @@ -333,7 +332,6 @@ describe('tabState unit tests', function () { .set('tabs', Immutable.fromJS([ { windowId: 1, - frameKey: 1, tabId: 1, index: 0, myProp: 'test1', @@ -341,7 +339,6 @@ describe('tabState unit tests', function () { }, { windowId: 1, - frameKey: 1, tabId: 2, index: 1, myProp: 'test2', @@ -364,13 +361,11 @@ describe('tabState unit tests', function () { index: 0, test: 'blue', windowId: 1, - frameKey: 1, myProp: 'test2', myProp2: 'blah' }, { windowId: 1, - frameKey: 1, tabId: 2, index: 1, myProp: 'test2', @@ -391,7 +386,6 @@ describe('tabState unit tests', function () { }, { windowId: 1, - frameKey: 1, tabId: 2, index: 1, myProp: 'test2', @@ -407,7 +401,6 @@ describe('tabState unit tests', function () { assert.equal('test1', tab.get('myProp')) assert.equal('blah', tab.get('myProp2')) assert.equal(1, tab.get('windowId')) - assert.equal(1, tab.get('frameKey')) assert.equal(1, tab.get('tabId')) assert.equal(true, state.get('otherProp')) }) @@ -429,7 +422,6 @@ describe('tabState unit tests', function () { it('removes the field specified', function () { const tab = Immutable.fromJS({ windowId: 1, - frameKey: 1, tabId: 2, loginRequiredDetail: { request: { url: 'someurl' }, @@ -456,7 +448,6 @@ describe('tabState unit tests', function () { before(function () { const tabs = Immutable.fromJS([{ windowId: 1, - frameKey: 1, tabId: 2, loginRequiredDetail: { request: { url: 'someurl' }, @@ -621,7 +612,7 @@ describe('tabState unit tests', function () { ) assert.throws( () => { - tabState.setTabs(defaultAppState, [{ frameKey: 1 }]) + tabState.setTabs(defaultAppState, [{ index: 0 }]) }, AssertionError ) @@ -694,6 +685,24 @@ describe('tabState unit tests', function () { }) }) + describe('getOpenerTabId', function () { + it('returns tabId of the opener', function () { + const tempState = twoTabsAppState + .setIn(['tabs', 1, 'openerTabId'], 1) + assert.equal(tabState.getOpenerTabId(tempState, 2), 1) + }) + + it('defaults to TAB_ID_NONE if not found', function () { + assert.equal(tabState.getOpenerTabId(twoTabsAppState, 2), tabState.TAB_ID_NONE) + }) + + it('returns TAB_ID_NONE if tabId is invalid', function () { + const tempState = twoTabsAppState + .setIn(['tabs', 1, 'openerTabId'], 17) + assert.equal(tabState.getOpenerTabId(tempState, 2), tabState.TAB_ID_NONE) + }) + }) + describe('getActiveTabId', function () { before(function () { this.appState = defaultAppState From b536afb181dd71369a970dc78a33dcfa67c54613 Mon Sep 17 00:00:00 2001 From: Brian Clifton Date: Thu, 15 Jun 2017 16:54:25 -0700 Subject: [PATCH 2/2] Review feedback --- app/browser/reducers/tabsReducer.js | 2 +- app/common/state/tabState.js | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/browser/reducers/tabsReducer.js b/app/browser/reducers/tabsReducer.js index f6ef4e30f4d..f8350e6a3a6 100644 --- a/app/browser/reducers/tabsReducer.js +++ b/app/browser/reducers/tabsReducer.js @@ -51,7 +51,7 @@ const updateActiveTab = (state, closeTabId) => { case tabCloseAction.PARENT: { const openerTabId = tabState.getOpenerTabId(state, closeTabId) - if (openerTabId && openerTabId !== tabState.TAB_ID_NONE) { + if (openerTabId !== tabState.TAB_ID_NONE) { nextTabId = openerTabId } break diff --git a/app/common/state/tabState.js b/app/common/state/tabState.js index 1e632ecc0f1..1ae74c972c5 100644 --- a/app/common/state/tabState.js +++ b/app/common/state/tabState.js @@ -501,9 +501,9 @@ const tabState = { getOpenerTabId: (state, tabId) => { const openerTabId = tabState.getTabPropertyByTabId(state, tabId, 'openerTabId', tabState.TAB_ID_NONE) if (openerTabId !== tabState.TAB_ID_NONE) { - // Validate that tabId exists - const index = getTabInternalIndexByTabId(state, openerTabId) - if (index !== tabState.TAB_ID_NONE) { + // Validate that tab exists + const tab = tabState.getByTabId(state, openerTabId) + if (tab) { return openerTabId } }