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

add clone tab support #2593

Merged
merged 1 commit into from
Jul 22, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/extensions/brave/locales/en-US/menu.properties
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ editBookmark=Edit Bookmark...
deleteFolder=Delete Folder
deleteBookmark=Delete Bookmark
stop=Stop
cloneTab=Clone Tab
reloadTab=Reload
unpinTab=Unpin
pinTab=Pin
Expand Down
1 change: 1 addition & 0 deletions app/locale.js
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ var rendererIdentifiers = function () {
'reloadTab',
'cleanReload',
'reload',
'cloneTab',
'readingView',
'tabManager',
'textEncoding',
Expand Down
6 changes: 4 additions & 2 deletions docs/windowActions.md
Original file line number Diff line number Diff line change
Expand Up @@ -219,14 +219,16 @@ Dispatches a message to the store to create a new frame



### cloneFrame(frameProps)
### cloneFrame(frameProps, guestInstanceId)

Dispatches a message to the store to create a new frame similar to the passed arg.
Dispatches a message to the store to clone an existing frame

**Parameters**

**frameProps**: `Object`, The properties of the frame to clone

**guestInstanceId**: `number`, The guestInstanceId of the cloned webcontents



### closeFrame(frames, frameProps)
Expand Down
16 changes: 9 additions & 7 deletions js/actions/windowActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -290,16 +290,18 @@ const windowActions = {
},

/**
* Dispatches a message to the store to create a new frame similar to the passed arg.
* Dispatches a message to the store to clone an existing frame
*
* @param {Object} frameProps - The properties of the frame to clone
* @param {number} guestInstanceId - The guestInstanceId of the cloned webcontents
*/
cloneFrame: function (frameProps) {
this.newFrame({
location: frameProps.get('location'),
isPrivate: frameProps.get('isPrivate'),
partitionNumber: frameProps.get('partitionNumber')
}, false)
cloneFrame: function (frameProps, guestInstanceId, openInForeground) {
dispatch({
actionType: WindowConstants.WINDOW_CLONE_FRAME,
frameOpts: frameProps.toJS ? frameProps.toJS() : frameProps,
guestInstanceId,
openInForeground
})
},

/**
Expand Down
8 changes: 8 additions & 0 deletions js/components/frame.js
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,14 @@ class Frame extends ImmutableComponent {
}
this.webview.reloadIgnoringCache()
break
case 'clone':
if (this.isAboutPage()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls also add a similar check on the context menu to not even show the option to avid people thinking it's a bug.
Is it possible to use appUrlUtil's isNavigatableAboutPage here and there instead?

break
}
const newGuest = this.webview.clone()
const newGuestInstanceId = newGuest.getWebPreferences().guestInstanceId
windowActions.cloneFrame(this.props.frame, newGuestInstanceId)
break
case 'explicitLoadURL':
this.webview.loadURL(location)
break
Expand Down
1 change: 1 addition & 0 deletions js/constants/messages.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const messages = {
SHORTCUT_ACTIVE_FRAME_STOP: _,
SHORTCUT_ACTIVE_FRAME_RELOAD: _,
SHORTCUT_ACTIVE_FRAME_CLEAN_RELOAD: _,
SHORTCUT_ACTIVE_FRAME_CLONE: _,
SHORTCUT_ACTIVE_FRAME_ZOOM_IN: _,
SHORTCUT_ACTIVE_FRAME_ZOOM_OUT: _,
SHORTCUT_ACTIVE_FRAME_ZOOM_RESET: _,
Expand Down
1 change: 1 addition & 0 deletions js/constants/windowConstants.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const windowConstants = {
WINDOW_SET_URL: _,
WINDOW_SET_NAVBAR_INPUT: _,
WINDOW_NEW_FRAME: _,
WINDOW_CLONE_FRAME: _,
WINDOW_CLOSE_FRAME: _,
WINDOW_SET_ACTIVE_FRAME: _,
WINDOW_SET_FOCUSED_FRAME: _,
Expand Down
9 changes: 9 additions & 0 deletions js/contextMenus.js
Original file line number Diff line number Diff line change
Expand Up @@ -834,6 +834,15 @@ function mainTemplateInit (nodeProps, frame) {
}
}
},
{
label: locale.translation('cloneTab'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this to the tab context menu instead of the in page one.

click: (item, focusedWindow) => {
if (focusedWindow) {
focusedWindow.webContents.send(messages.SHORTCUT_ACTIVE_FRAME_CLONE)
}
}
},

CommonMenu.separatorMenuItem,
addBookmarkMenuItem('bookmarkPage', siteUtil.getDetailFromFrame(frame, siteTags.BOOKMARK), false), {
label: locale.translation('find'),
Expand Down
82 changes: 47 additions & 35 deletions js/state/frameStateUtil.js
Original file line number Diff line number Diff line change
Expand Up @@ -172,26 +172,6 @@ function getFramePropsIndex (frames, frameProps) {
return frames.findIndex(matchFrame.bind(null, queryInfo))
}

/**
* Converts a feature string into an object.
* @param {String} featureStr A string like, arg=val,arg2=val2
*/
function getFeatures (featureStr) {
return String(featureStr)
.split(',')
.reduce((acc, feature) => {
feature = feature
.split('=')
.map((featureElem) => featureElem.trim())
if (feature.length !== 2) {
return acc
}

acc[decodeURIComponent(feature[0])] = decodeURIComponent(feature[1])
return acc
}, {})
}

/**
* Determines if the specified frame was opened from the specified
* ancestorFrameKey.
Expand Down Expand Up @@ -233,27 +213,63 @@ function getPartition (frameOpts) {
}
return partition
}

function cloneFrame (frameOpts, guestInstanceId) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this actually needs to selectively include properties instead of exclude them. Unlike a webview that is detached and reattached, a cloned webview won't have the same audio playback for instance

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

const cloneableAttributes = [
'audioMuted',
'canGoBack',
'canGoForward',
'icon',
'title',
'isPrivate',
'partitionNumber',
'themeColor',
'computedThemeColor'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about provisionalLocation and maybe src and location.
provisionalLocation I can see maybe being useful on a about:certerror type of page.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it won't clone about pages

]
let clone = {}
cloneableAttributes.forEach((attr) => {
clone[attr] = frameOpts[attr]
})

clone.guestInstanceId = guestInstanceId
// copy the history
clone.history = frameOpts.history.slice(0)
// location is loaded by the webcontents
clone.delayedLoadUrl = frameOpts.location
clone.location = 'about:blank'
clone.src = 'about:blank'
clone.parentFrameKey = frameOpts.key
return clone
}

/**
* Adds a frame specified by frameOpts and newKey and sets the activeFrameKey
* @return Immutable top level application state ready to merge back in
*/
function addFrame (frames, frameOpts, newKey, partitionNumber, activeFrameKey) {
const url = frameOpts.location || config.defaultUrl

// delayedLoadUrl is used as a placeholder when the new frame is created
// from a renderer initiated navigation (window.open, cmd/ctrl-click, etc...)
const delayedLoadUrl = frameOpts.delayedLoadUrl
delete frameOpts.delayedLoadUrl
const navbarFocus = activeFrameKey === newKey &&
url === config.defaultUrl &&
frameOpts.delayedLoadUrl === undefined
const location = frameOpts.delayedLoadUrl || url // page url
delayedLoadUrl === undefined
const location = delayedLoadUrl || url // page url

// Only add pin requests if it's not already added
if (frameOpts.isPinned) {
const isPinned = frameOpts.isPinned
delete frameOpts.isPinned
if (isPinned) {
const alreadyPinnedFrameProps = frames.find((frame) =>
frame.get('pinnedLocation') === location && frame.get('partitionNumber') === partitionNumber)
if (alreadyPinnedFrameProps) {
return {}
}
}

const frame = Immutable.fromJS({
const frame = Immutable.fromJS(Object.assign({
zoomLevel: config.zoom.defaultValue,
audioMuted: false, // frame is muted
canGoBack: false,
Expand All @@ -263,21 +279,17 @@ function addFrame (frames, frameOpts, newKey, partitionNumber, activeFrameKey) {
src: url, // what the iframe src should be
tabId: -1,
// if this is a delayed load then go ahead and start the loading indicator
loading: !!frameOpts.delayedLoadUrl,
startLoadTime: frameOpts.delayedLoadUrl ? new Date().getTime() : null,
loading: !!delayedLoadUrl,
startLoadTime: delayedLoadUrl ? new Date().getTime() : null,
endLoadTime: null,
isPrivate: frameOpts.isPrivate || false,
isPrivate: false,
partitionNumber,
element: frameOpts.element,
features: getFeatures(frameOpts.features),
pinnedLocation: frameOpts.isPinned ? url : undefined,
pinnedLocation: isPinned ? url : undefined,
key: newKey,
parentFrameKey: frameOpts.parentFrameKey,
guestInstanceId: frameOpts.guestInstanceId,
navbar: {
focused: navbarFocus,
urlbar: {
location: frameOpts.delayedLoadUrl || url,
location: delayedLoadUrl || url,
urlPreview: '',
suggestions: {
selectedIndex: 0,
Expand All @@ -300,7 +312,7 @@ function addFrame (frames, frameOpts, newKey, partitionNumber, activeFrameKey) {
},
unloaded: frameOpts.unloaded,
history: []
})
}, frameOpts))

// Find the closest index to the current frame's index which has
// a different ancestor frame key.
Expand Down Expand Up @@ -461,8 +473,8 @@ module.exports = {
findDisplayIndexForFrameKey,
getFramePropsIndex,
getFrameKeysByDisplayIndex,
getFeatures,
getPartition,
cloneFrame,
addFrame,
undoCloseFrame,
removeFrame,
Expand Down
93 changes: 50 additions & 43 deletions js/stores/windowStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,51 @@ const addToHistory = (frameProps) => {
return history.slice(-10)
}

const newFrame = (frameOpts, openInForeground) => {
if (frameOpts === undefined) {
frameOpts = {}
}
if (openInForeground === undefined) {
openInForeground = true
}
frameOpts.location = frameOpts.location || config.defaultUrl
if (frameOpts.location && UrlUtil.isURL(frameOpts.location)) {
frameOpts.location = UrlUtil.getUrlFromInput(frameOpts.location)
} else {
const defaultURL = windowStore.getState().getIn(['searchDetail', 'searchURL'])
if (defaultURL) {
frameOpts.location = defaultURL
.replace('{searchTerms}', encodeURIComponent(frameOpts.location))
} else {
// Bad URLs passed here can actually crash the browser
frameOpts.location = ''
}
}

const nextKey = incrementNextKey()
let nextPartitionNumber = 0
if (frameOpts.partitionNumber) {
nextPartitionNumber = frameOpts.partitionNumber
if (currentPartitionNumber < nextPartitionNumber) {
currentPartitionNumber = nextPartitionNumber
}
} else if (frameOpts.isPartitioned) {
nextPartitionNumber = incrementPartitionNumber()
}
frameOpts.location = setPDFLocation(frameOpts.location)
windowState = windowState.merge(FrameStateUtil.addFrame(windowState.get('frames'), frameOpts,
nextKey, nextPartitionNumber, openInForeground ? nextKey : windowState.get('activeFrameKey')))
if (openInForeground) {
const activeFrame = FrameStateUtil.getActiveFrame(windowState)
updateTabPageIndex(activeFrame)
// For about:newtab we want to have the urlbar focused, not the new frame.
// Otherwise we want to focus the new tab when it is a new frame in the foreground.
if (activeFrame.get('location') !== 'about:newtab') {
focusWebview(activeFrameStatePath())
}
}
}

const windowStore = new WindowStore()
const emitChanges = debounce(windowStore.emitChanges.bind(windowStore), 5)

Expand Down Expand Up @@ -315,48 +360,10 @@ const doAction = (action) => {
}
break
case WindowConstants.WINDOW_NEW_FRAME:
if (action.frameOpts === undefined) {
action.frameOpts = {}
}
if (action.openInForeground === undefined) {
action.openInForeground = true
}
action.frameOpts.location = action.frameOpts.location || config.defaultUrl
if (action.frameOpts.location && UrlUtil.isURL(action.frameOpts.location)) {
action.frameOpts.location = UrlUtil.getUrlFromInput(action.frameOpts.location)
} else {
const defaultURL = windowStore.getState().getIn(['searchDetail', 'searchURL'])
if (defaultURL) {
action.frameOpts.location = defaultURL
.replace('{searchTerms}', encodeURIComponent(action.frameOpts.location))
} else {
// Bad URLs passed here can actually crash the browser
action.frameOpts.location = ''
}
}

const nextKey = incrementNextKey()
let nextPartitionNumber = 0
if (action.frameOpts.partitionNumber) {
nextPartitionNumber = action.frameOpts.partitionNumber
if (currentPartitionNumber < nextPartitionNumber) {
currentPartitionNumber = nextPartitionNumber
}
} else if (action.frameOpts.isPartitioned) {
nextPartitionNumber = incrementPartitionNumber()
}
action.frameOpts.location = setPDFLocation(action.frameOpts.location)
windowState = windowState.merge(FrameStateUtil.addFrame(windowState.get('frames'), action.frameOpts,
nextKey, nextPartitionNumber, action.openInForeground ? nextKey : windowState.get('activeFrameKey')))
if (action.openInForeground) {
const activeFrame = FrameStateUtil.getActiveFrame(windowState)
updateTabPageIndex(activeFrame)
// For about:newtab we want to have the urlbar focused, not the new frame.
// Otherwise we want to focus the new tab when it is a new frame in the foreground.
if (activeFrame.get('location') !== 'about:newtab') {
focusWebview(activeFrameStatePath())
}
}
newFrame(action.frameOpts, action.openInForeground)
break
case WindowConstants.WINDOW_CLONE_FRAME:
newFrame(FrameStateUtil.cloneFrame(action.frameOpts, action.guestInstanceId), action.openInForeground)
break
case WindowConstants.WINDOW_CLOSE_FRAME:
const currentWindow = require('electron').remote.getCurrentWindow()
Expand Down Expand Up @@ -662,7 +669,7 @@ ipc.on(messages.IMPORT_BOOKMARKS, () => {
}
})

const frameShortcuts = ['stop', 'reload', 'zoom-in', 'zoom-out', 'zoom-reset', 'toggle-dev-tools', 'clean-reload', 'view-source', 'mute', 'save', 'print', 'show-findbar', 'copy', 'find-next', 'find-prev']
const frameShortcuts = ['stop', 'reload', 'zoom-in', 'zoom-out', 'zoom-reset', 'toggle-dev-tools', 'clean-reload', 'view-source', 'mute', 'save', 'print', 'show-findbar', 'copy', 'find-next', 'find-prev', 'clone']
frameShortcuts.forEach((shortcut) => {
// Listen for actions on the active frame
ipc.on(`shortcut-active-frame-${shortcut}`, () => {
Expand Down
Loading