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

Commit

Permalink
Set index explicitly on clone
Browse files Browse the repository at this point in the history
Auditors: @bridiver

This fixes automated test: inserts after the tab to clone (107ms)

Fix #6390
  • Loading branch information
bbondy committed Dec 23, 2016
1 parent 93b7c31 commit 3004b9e
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 7 deletions.
33 changes: 27 additions & 6 deletions app/browser/tabs.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
const appActions = require('../../js/actions/appActions')
const messages = require('../..//js/constants/messages')
const Immutable = require('immutable')
const tabState = require('../common/state/tabState')
const {app, extensions} = require('electron')
const { makeImmutable } = require('../common/state/immutableUtil')
Expand Down Expand Up @@ -52,14 +53,20 @@ const api = {
}

const openerTabId = !source.isDestroyed() ? source.getId() : -1
let newTabValue = getTabValue(newTab.getId())
let index
if (newTabValue && newTabValue.get('index') !== -1) {
index = newTabValue.get('index')
}

// TODO(bridiver) - handle pinned property?? - probably through tabValue
const frameOpts = {
location,
partition: newTab.session.partition,
guestInstanceId: newTab.guestInstanceId,
openerTabId,
disposition
disposition,
index
}

if (disposition === 'new-window' || disposition === 'new-popup') {
Expand All @@ -81,6 +88,9 @@ const api = {
tab.on('set-active', function (evt, active) {

This comment has been minimized.

Copy link
@bridiver

bridiver Dec 23, 2016

Collaborator

both of these are actually wrong (set-active and set-tab-index). In both cases the emit happens before the value is set so updateTab won't get the new value. I think I originally did it that way to allow the listener to call e.preventDefault, but I can't really think of a reason why we would do that

This comment has been minimized.

Copy link
@bbondy

bbondy Dec 23, 2016

Author Member

Do you recommend I emit after I set it instead? It works but I guess probably because something else does an updateTab

This comment has been minimized.

Copy link
@bbondy

bbondy Dec 23, 2016

Author Member

I'll go ahead with moving Emit below, I don't see any fallout from that, but will keep it in mind.

updateTab(tabId)
})
tab.on('set-tab-index', function (evt, index) {
updateTab(tabId)
})
tab.on('page-favicon-updated', function (e, favicons) {
if (favicons && favicons.length > 0) {
// tab.setTabValues({
Expand Down Expand Up @@ -138,10 +148,17 @@ const api = {
})

process.on('on-tab-created', (tab, options) => {
if (tab.isDestroyed()) {
return
}

if (options.index !== undefined) {
tab.setTabValues({

This comment has been minimized.

Copy link
@bridiver

bridiver Dec 23, 2016

Collaborator

this won't set the index property you created in TabHelper. You should call setTabIndex here or modify modify the TabHelper to use the index value from the values_ dictionary

This comment has been minimized.

Copy link
@bbondy

bbondy Dec 23, 2016

Author Member

ooops, I'll go with the former

index: options.index
})
}

tab.once('did-attach', () => {
if (tab.isDestroyed()) {
return
}
if (options.back) {
tab.goBack()
} else if (options.forward) {
Expand Down Expand Up @@ -173,10 +190,14 @@ const api = {
clone: (state, action) => {
action = makeImmutable(action)
const tabId = action.get('tabId')
const options = action.get('options')
let options = action.get('options') || Immutable.Map()
let tabValue = getTabValue(tabId)
if (tabValue && tabValue.get('index') !== undefined) {
options = options.set('index', tabValue.get('index') + 1)
}
const tab = api.getWebContents(tabId)
if (tab && !tab.isDestroyed()) {
tab.clone(options && options.toJS() || {}, (newTab) => {
tab.clone(options.toJS(), (newTab) => {
})
}
return state
Expand Down
4 changes: 4 additions & 0 deletions js/components/frame.js
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,7 @@ class Frame extends ImmutableComponent {
componentDidMount () {
const cb = () => {
this.webview.setActive(this.props.isActive)
this.webview.setTabIndex(this.props.tabIndex)
this.updateAboutDetails()
}
this.updateWebview(cb)
Expand Down Expand Up @@ -364,6 +365,9 @@ class Frame extends ImmutableComponent {
this.webview.setWebRTCIPHandlingPolicy(this.getWebRTCPolicy())
}
this.webview.setActive(this.props.isActive)
if (prevProps.tabIndex !== this.props.tabIndex) {
this.webview.setTabIndex(this.props.tabIndex)
}
this.handleShortcut()

// give focus when switching tabs
Expand Down
1 change: 1 addition & 0 deletions js/components/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -1202,6 +1202,7 @@ class Main extends ImmutableComponent {
sortedFrames.map((frame) =>
<Frame
ref={(node) => { this.frames[frame.get('key')] = node }}
tabIndex={FrameStateUtil.getFrameIndex(this.props.windowState, frame.get('key'))}
prefOpenInForeground={getSetting(settings.SWITCH_TO_NEW_TABS)}
onCloseFrame={this.onCloseFrame}
frameKey={frame.get('key')}
Expand Down
4 changes: 3 additions & 1 deletion js/stores/windowStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,9 @@ const newFrame = (frameOpts, openInForeground, insertionIndex, nextKey) => {
frameOpts = frameOpts.toJS ? frameOpts.toJS() : frameOpts

// handle tabs.create properties
insertionIndex = frameOpts.index || insertionIndex
insertionIndex = frameOpts.index !== undefined
? frameOpts.index
: insertionIndex

if (frameOpts.partition) {
frameOpts.isPrivate = frameStateUtil.isPrivatePartition(frameOpts.partition)
Expand Down

0 comments on commit 3004b9e

Please sign in to comment.