-
Notifications
You must be signed in to change notification settings - Fork 974
Conversation
@@ -233,27 +213,49 @@ function getPartition (frameOpts) { | |||
} | |||
return partition | |||
} | |||
|
|||
function cloneFrame (frameOpts, guestInstanceId) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Awesome PR! This should enable the "move tab to new window" functionality ?? [--- Commented from Asana.com |
yea, this was intended to be a first step towards "move tab to new window" which I'm working on right now |
'isPrivate', | ||
'partitionNumber', | ||
'themeColor', | ||
'computedThemeColor' |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
merging for now, seems like mainly the only thing to do in a follow up is cloneTab in the tab and not the page context menu. |
Great work! :) |
@@ -334,6 +334,14 @@ class Frame extends ImmutableComponent { | |||
} | |||
this.webview.reloadIgnoringCache() | |||
break | |||
case 'clone': | |||
if (this.isAboutPage()) { |
There was a problem hiding this comment.
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?
requires brave/muon@683ea44
cc @bbondy