Skip to content
This repository was archived by the owner on Dec 11, 2019. It is now read-only.

Commit 6e5a908

Browse files
bbondybbondy
committed
18 Unbelievable ways to get rid of dead tabs on tab close
Fix #11028 Fix #10696 Fix #10905 Fix #10749
1 parent 66eba7d commit 6e5a908

File tree

19 files changed

+339
-119
lines changed

19 files changed

+339
-119
lines changed

‎app/browser/reducers/tabsReducer.js

+23-1
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,27 @@ const tabsReducer = (state, action, immutableAction) => {
9898
})
9999
break
100100
}
101+
case appConstants.APP_TAB_PAGE_CLOSE_MENU_ITEM_CLICKED: {
102+
const windowId = action.get('windowId')
103+
const tabPageIndex = action.get('tabPageIndex')
104+
state = tabs.closeTabPage(state, windowId, tabPageIndex)
105+
break
106+
}
107+
case appConstants.APP_CLOSE_TABS_TO_LEFT_MENU_ITEM_CLICKED: {
108+
const tabId = action.get('tabId')
109+
state = tabs.closeTabsToLeft(state, tabId)
110+
break
111+
}
112+
case appConstants.APP_CLOSE_TABS_TO_RIGHT_MENU_ITEM_CLICKED: {
113+
const tabId = action.get('tabId')
114+
state = tabs.closeTabsToRight(state, tabId)
115+
break
116+
}
117+
case appConstants.APP_CLOSE_OTHER_TABS_MENU_ITEM_CLICKED: {
118+
const tabId = action.get('tabId')
119+
state = tabs.closeOtherTabs(state, tabId)
120+
break
121+
}
101122
case appConstants.APP_CREATE_TAB_REQUESTED:
102123
if (action.getIn(['createProperties', 'windowId']) == null) {
103124
const senderWindowId = action.getIn(['senderWindowId'])
@@ -147,7 +168,7 @@ const tabsReducer = (state, action, immutableAction) => {
147168
tabs.toggleDevTools(tabId)
148169
})
149170
} else {
150-
// This check is only needed because sometimes front end can try to close
171+
// This check is only needed in case front end tries to close
151172
// a tabId it thinks exists but doesn't actually exist anymore.
152173
const tabValue = tabState.getByTabId(state, tabId)
153174
if (!tabValue) {
@@ -190,6 +211,7 @@ const tabsReducer = (state, action, immutableAction) => {
190211
tabs.setActive(nextActiveTabId)
191212
}
192213
})
214+
tabs.forgetTab(tabId)
193215
}
194216
break
195217
case appConstants.APP_ALLOW_FLASH_ONCE:

‎app/browser/tabs.js

+91-6
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
/* This Source Code Form is subject to the terms of the Mozilla Public
22
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
33
* You can obtain one at http://mozilla.org/MPL/2.0/. */
4-
54
const appActions = require('../../js/actions/appActions')
65
const windowActions = require('../../js/actions/windowActions')
76
const tabActions = require('../common/actions/tabActions')
@@ -430,11 +429,6 @@ const api = {
430429
updateTab(tabId, changeInfo)
431430
})
432431

433-
process.on('chrome-tabs-removed', (tabId, windowId) => {
434-
appActions.tabClosed(tabId, windowId)
435-
cleanupWebContents(tabId)
436-
})
437-
438432
app.on('web-contents-created', function (event, tab) {
439433
if (tab.isBackgroundPage() || !tab.isGuest()) {
440434
return
@@ -526,6 +520,14 @@ const api = {
526520
windowActions.gotResponseDetails(tabId, {status, newURL, originalURL, httpResponseCode, requestMethod, referrer, headers, resourceType})
527521
}
528522
})
523+
524+
tab.once('will-destroy', () => {
525+
const tabValue = getTabValue(tabId)
526+
if (tabValue) {
527+
const windowId = tabValue.get('windowId')
528+
appActions.tabClosed(tabId, windowId)
529+
}
530+
})
529531
})
530532

531533
process.on('on-tab-created', (tab, options) => {
@@ -985,6 +987,86 @@ const api = {
985987

986988
return nextTabId
987989
},
990+
991+
closeTabPage: (state, windowId, tabPageIndex) => {
992+
const tabsPerPage = Number(getSetting(settings.TABS_PER_PAGE))
993+
const startTabIndex = tabPageIndex * tabsPerPage
994+
const pinnedTabsCount = tabState.getPinnedTabsByWindowId(state, windowId).size
995+
tabState.getTabsByWindowId(state, windowId)
996+
.sort((tab1, tab2) => tab1.get('index') - tab2.get('index'))
997+
.filter((tabValue) => !tabValue.get('pinned'))
998+
.slice(startTabIndex + pinnedTabsCount, startTabIndex + tabsPerPage + pinnedTabsCount)
999+
.forEach((tabValue) => {
1000+
const tab = getWebContents(tabValue.get('tabId'))
1001+
if (tab && !tab.isDestroyed()) {
1002+
tab.forceClose()
1003+
}
1004+
})
1005+
state = api.updateTabsStateForWindow(state, windowId)
1006+
return state
1007+
},
1008+
1009+
closeTabsToLeft: (state, tabId) => {
1010+
const tabValue = tabState.getByTabId(state, tabId)
1011+
if (!tabValue) {
1012+
return state
1013+
}
1014+
const index = tabValue.get('index')
1015+
const windowId = tabValue.get('windowId')
1016+
const pinnedTabsCount = tabState.getPinnedTabsByWindowId(state, windowId).size
1017+
tabState.getTabsByWindowId(state, windowId)
1018+
.sort((tab1, tab2) => tab1.get('index') - tab2.get('index'))
1019+
.filter((tabValue) => !tabValue.get('pinned'))
1020+
.slice(0, index - pinnedTabsCount)
1021+
.forEach((tabValue) => {
1022+
const tab = getWebContents(tabValue.get('tabId'))
1023+
if (tab && !tab.isDestroyed()) {
1024+
tab.forceClose()
1025+
}
1026+
})
1027+
state = api.updateTabsStateForWindow(state, windowId)
1028+
return state
1029+
},
1030+
1031+
closeTabsToRight: (state, tabId) => {
1032+
const tabValue = tabState.getByTabId(state, tabId)
1033+
if (!tabValue) {
1034+
return state
1035+
}
1036+
const index = tabValue.get('index')
1037+
const windowId = tabValue.get('windowId')
1038+
const pinnedTabsCount = tabState.getPinnedTabsByWindowId(state, windowId).size
1039+
tabState.getTabsByWindowId(state, windowId)
1040+
.sort((tab1, tab2) => tab1.get('index') - tab2.get('index'))
1041+
.filter((tabValue) => !tabValue.get('pinned'))
1042+
.slice(index + 1 - pinnedTabsCount)
1043+
.forEach((tabValue) => {
1044+
const tab = getWebContents(tabValue.get('tabId'))
1045+
if (tab && !tab.isDestroyed()) {
1046+
tab.forceClose()
1047+
}
1048+
})
1049+
state = api.updateTabsStateForWindow(state, windowId)
1050+
return state
1051+
},
1052+
1053+
closeOtherTabs: (state, tabId) => {
1054+
const tabValue = tabState.getByTabId(state, tabId)
1055+
if (!tabValue) {
1056+
return state
1057+
}
1058+
const windowId = tabValue.get('windowId')
1059+
tabState.getTabsByWindowId(state, windowId)
1060+
.forEach((tabValue) => {
1061+
const tab = getWebContents(tabValue.get('tabId'))
1062+
if (tab && !tab.isDestroyed() && tabValue.get('tabId') !== tabId && !tabValue.get('pinned')) {
1063+
tab.forceClose()
1064+
}
1065+
})
1066+
state = api.updateTabsStateForWindow(state, windowId)
1067+
return state
1068+
},
1069+
9881070
debugTabs: (state) => {
9891071
console.log(tabState.getTabs(state)
9901072
.toJS()
@@ -1042,6 +1124,9 @@ const api = {
10421124
}
10431125
})
10441126
return state
1127+
},
1128+
forgetTab: (tabId) => {
1129+
cleanupWebContents(tabId)
10451130
}
10461131
}
10471132

‎app/renderer/components/frame/frame.js

+9-7
Original file line numberDiff line numberDiff line change
@@ -431,19 +431,21 @@ class Frame extends React.Component {
431431
}
432432

433433
addEventListeners () {
434-
this.webview.addEventListener('tab-id-changed', (e) => {
435-
if (this.frame.isEmpty()) {
436-
return
437-
}
438-
439-
windowActions.frameTabIdChanged(this.frame, this.props.tabId, e.tabID)
440-
}, { passive: true })
441434
this.webview.addEventListener('guest-ready', (e) => {
442435
if (this.frame.isEmpty()) {
443436
return
444437
}
445438

446439
windowActions.frameGuestInstanceIdChanged(this.frame, this.props.guestInstanceId, e.guestInstanceId)
440+
const tabIdChangedCB = (e) => {
441+
if (this.frame.isEmpty()) {
442+
return
443+
}
444+
445+
windowActions.frameTabIdChanged(this.frame, this.props.tabId, e.tabID)
446+
this.webview.removeEventListener('tab-id-changed', tabIdChangedCB)
447+
}
448+
this.webview.addEventListener('tab-id-changed', tabIdChangedCB, { passive: true })
447449
}, { passive: true })
448450
this.webview.addEventListener('content-blocked', (e) => {
449451
if (this.frame.isEmpty()) {

‎app/renderer/components/main/main.js

-4
Original file line numberDiff line numberDiff line change
@@ -373,10 +373,6 @@ class Main extends React.Component {
373373

374374
ipc.on(messages.SHORTCUT_UNDO_CLOSED_FRAME, () => windowActions.undoClosedFrame())
375375

376-
ipc.on(messages.SHORTCUT_CLOSE_OTHER_FRAMES, (e, tabId, isCloseRight, isCloseLeft) => {
377-
windowActions.closeOtherFrames(tabId, isCloseRight, isCloseLeft)
378-
})
379-
380376
ipc.on(messages.SHOW_DOWNLOADS_TOOLBAR, () => {
381377
windowActions.setDownloadsToolbarVisible(true)
382378
})

‎app/renderer/reducers/contextMenuReducer.js

+2-4
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ const urlUtil = require('../../../js/lib/urlutil')
3838
const frameStateUtil = require('../../../js/state/frameStateUtil')
3939
const dndData = require('../../../js/dndData')
4040
const {makeImmutable, isMap} = require('../../common/state/immutableUtil')
41-
const {getCurrentWindow} = require('../../renderer/currentWindow')
41+
const {getCurrentWindow, getCurrentWindowId} = require('../../renderer/currentWindow')
4242
const {getSetting} = require('../../../js/settings')
4343

4444
const validateAction = function (action) {
@@ -93,9 +93,7 @@ const onTabPageMenu = function (state, action) {
9393
}, {
9494
label: locale.translation('closeTabPage'),
9595
click: () => {
96-
tabPageFrames
97-
.map((frame) => frame.get('tabId'))
98-
.forEach((tabId) => appActions.tabCloseRequested(tabId))
96+
appActions.tabPageCloseMenuItemClicked(getCurrentWindowId(), index)
9997
}
10098
}]
10199

‎app/renderer/reducers/frameReducer.js

+24-46
Original file line numberDiff line numberDiff line change
@@ -79,22 +79,40 @@ const getLocation = (location) => {
7979

8080
const frameReducer = (state, action, immutableAction) => {
8181
switch (action.actionType) {
82+
case appConstants.APP_TAB_CLOSED: {
83+
const tabId = immutableAction.get('tabId')
84+
const frame = frameStateUtil.getFrameByTabId(state, tabId)
85+
if (frame) {
86+
action.frameKey = frame.get('key')
87+
state = closeFrame(state, action)
88+
const activeFrame = frameStateUtil.getActiveFrame(state)
89+
if (activeFrame) {
90+
state = frameStateUtil.updateTabPageIndex(state, activeFrame.get('tabId'))
91+
}
92+
}
93+
break
94+
}
8295
case appConstants.APP_TAB_UPDATED:
8396
// This case will be fired for both tab creation and tab update.
84-
// being `tabValue` set for tab creation and `changeInfo` set for tab update
8597
const tab = immutableAction.get('tabValue')
86-
const changeInfo = immutableAction.get('changeInfo')
8798
if (!tab) {
8899
break
89100
}
90101
const tabId = tab.get('tabId')
102+
if (tabId === -1) {
103+
break
104+
}
91105
let frame = frameStateUtil.getFrameByTabId(state, tabId)
92106
if (!frame) {
93107
break
94108
}
95109
let frames = state.get('frames')
96110
const index = tab.get('index')
111+
// If front end doesn't know about this tabId, then do nothing!
97112
let sourceFrameIndex = frameStateUtil.getIndexByTabId(state, tabId)
113+
if (sourceFrameIndex == null) {
114+
break
115+
}
98116
if (index != null &&
99117
sourceFrameIndex !== index &&
100118
// Only update the index once the frame is known.
@@ -156,22 +174,10 @@ const frameReducer = (state, action, immutableAction) => {
156174
}
157175

158176
const active = tab.get('active')
159-
if (active != null) {
160-
if (active) {
161-
state = frameStateUtil.setActiveFrameKey(state, frame.get('key'))
162-
state = frameStateUtil.setFrameLastAccessedTime(state, sourceFrameIndex)
163-
164-
// Handle tabPage updates and preview cancelation based on tab updated
165-
// otherwise tabValue will fire those events each time a tab finish loading
166-
// see bug #8429
167-
const isNewTab = changeInfo.isEmpty()
168-
const activeTabHasUpdated = changeInfo.get('active') != null
169-
170-
if (!isNewTab && activeTabHasUpdated) {
171-
state = frameStateUtil.updateTabPageIndex(state, tabId)
172-
state = state.set('previewFrameKey', null)
173-
}
174-
}
177+
if (active && state.get('activeFrameKey') !== frame.get('key')) {
178+
state = frameStateUtil.setActiveFrameKey(state, frame.get('key'))
179+
state = frameStateUtil.setFrameLastAccessedTime(state, sourceFrameIndex)
180+
state = state.set('previewFrameKey', null)
175181
}
176182
break
177183
case windowConstants.WINDOW_SET_NAVIGATED:
@@ -224,34 +230,6 @@ const frameReducer = (state, action, immutableAction) => {
224230
appActions.tabCloseRequested(frame.get('tabId'))
225231
})
226232
break
227-
case windowConstants.WINDOW_CLOSE_OTHER_FRAMES:
228-
{
229-
const currentIndex = frameStateUtil.getIndexByTabId(state, action.tabId)
230-
if (currentIndex === -1) {
231-
break
232-
}
233-
234-
let tabs = []
235-
236-
state.get('frames').forEach((frame, i) => {
237-
if (!frame.get('pinnedLocation') &&
238-
((i < currentIndex && action.isCloseLeft) || (i > currentIndex && action.isCloseRight))) {
239-
if (frame) {
240-
tabs.push(frame.get('tabId'))
241-
appActions.tabCloseRequested(frame.get('tabId'))
242-
}
243-
}
244-
})
245-
246-
// TODO(nejc) this can be simplified when states are merged
247-
const newFrames = state.get('frames').filter(frame => !tabs.includes(frame.get('tabId')))
248-
let newState = state.set('frames', newFrames)
249-
newState = frameStateUtil.updateTabPageIndex(newState, action.tabId)
250-
const index = newState.getIn(['ui', 'tabs', 'tabPageIndex'], 0)
251-
state = state.setIn(['ui', 'tabs', 'tabPageIndex'], index)
252-
}
253-
break
254-
255233
case windowConstants.WINDOW_CLOSE_FRAME:
256234
state = closeFrame(state, action)
257235
const activeFrame = frameStateUtil.getActiveFrame(state)

‎js/actions/appActions.js

+45
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,51 @@ const appActions = {
155155
})
156156
},
157157

158+
/**
159+
* Menu item for closing a tab page has been clicked.
160+
* @param {Number} tabPageIndex The index of the tab page to close
161+
*/
162+
tabPageCloseMenuItemClicked: function (windowId, tabPageIndex) {
163+
dispatch({
164+
actionType: appConstants.APP_TAB_PAGE_CLOSE_MENU_ITEM_CLICKED,
165+
tabPageIndex,
166+
windowId
167+
})
168+
},
169+
170+
/**
171+
* Menu item for closing tabs to the left has been clicked.
172+
* @param {Number} tabId The tabId woh's tabs to the left should be closed.
173+
*/
174+
closeTabsToLeftMenuItemClicked: function (tabId) {
175+
dispatch({
176+
actionType: appConstants.APP_CLOSE_TABS_TO_LEFT_MENU_ITEM_CLICKED,
177+
tabId
178+
})
179+
},
180+
181+
/**
182+
* Menu item for closing tabs to the right has been clicked.
183+
* @param {Number} tabId The tabId woh's tabs to the right should be closed.
184+
*/
185+
closeTabsToRightMenuItemClicked: function (tabId) {
186+
dispatch({
187+
actionType: appConstants.APP_CLOSE_TABS_TO_RIGHT_MENU_ITEM_CLICKED,
188+
tabId
189+
})
190+
},
191+
192+
/**
193+
* Menu item for closing other tabs than the specified tab.
194+
* @param {Number} tabId The tabId woh's tabs to the left should be closed.
195+
*/
196+
closeOtherTabsMenuItemClicked: function (tabId) {
197+
dispatch({
198+
actionType: appConstants.APP_CLOSE_OTHER_TABS_MENU_ITEM_CLICKED,
199+
tabId
200+
})
201+
},
202+
158203
/**
159204
* A request for a new tab has been made with the specified createProperties
160205
* @param {Object} createProperties

0 commit comments

Comments
 (0)