Closed Bug 997436 Opened 10 years ago Closed 10 years ago

Add "Open in Non-e10s Window" to context menu on tabs

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 32
Tracking Status
e10s + ---
firefox32 --- verified

People

(Reporter: ally, Assigned: hkaimenas)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good-first-bug][lang=js][mentor=ally])

Attachments

(1 file, 2 obsolete files)

add an entry in the context menu so testers can open pages that do not work in e10s (banking sites, etc) so they don't have to turn off e10s to do what they need to do.
Blocks: fxe10s
Blocks: e10s-m1
Whiteboard: [good-first-bug][lang=js][mentor=ally]
I'd like to work on this as a first bug. Could you please point me to the right direction? Also I'd like to clarify something; e10s stands for electrolysis?
(In reply to Harry Kaimenas [hkaimenas] from comment #1)
> I'd like to work on this as a first bug. Could you please point me to the
> right direction? Also I'd like to clarify something; e10s stands for
> electrolysis?

Hi Harry, the first thing to do is to get a Firefox build set up. Instructions are here https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

Once you're up and building, the best place to get help is the #e10s channel on Mozilla's IRC. https://wiki.mozilla.org/IRC
Harry: if you have questions about the Firefox build instructions that Brad linked to, the best place to get timely help is the #introduction channel on Mozilla's IRC server.

And yes "e10s" is shorthand for "Electrolysis". :)
OS: Windows 8 → All
Hardware: x86_64 → All
Summary: Add 'open in none10s window' to context menu on tabs → Add "Open in Non-e10s Window" to context menu on tabs
Welcome! 

If you want to do some reading while your build is doing its thing, 

the context menu code
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#79

It looks funny! that's because its XUL. Our frontend is a mix of xul & html these days.
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Tutorial
Hi all,

Thanks for all the helpful advice/guidance! I'm very happy to join you!

Ally: Before I start coding here are my thoughts. I believe that I need to make something similar to 
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#100

If this is the case I'll need to pass some parameters to 
gBrowser.replaceTabWithWindow(TabContextMenu.contextTab, myExtraParamters); // myExtraParameters contain my customizations.

I know I can pass an options object (or map if you like) as per the definition found here
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#2490

My question is where can I find the possible options that I can pass to this object? I've searched but failed to find anything.
Hey Harry,
In the future, please use the NEEDINFO flag tog et someone's attention. It's really handy It shows up in my bugzilla dashboard & bugzilla emails you if you are late on them, where as requests in comments often get missed in the volume of bugmail we get & there's no followup notice if you miss one.

Have you gotten the front-end ui part up & running and calling a handler function? 
context_openTabInWindow is a good place to start to make sure your xul&localization code works.

In this function the options are passed along whole hog. The extra parameters appear to be used only in one case: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#4608

However, upon looking into replaceTabWithWindow, I find Bug 918634, that SwapFrameLoader is not implemented and :markh's notes indicate that replaceTabWithWindow relies on that. That might be too much for a first bug.

So, we might have to do this the longer, hackier way. The definition of an e10s tab, represented by a <browser> element, is that the attribute remote is true. So, we could launch the new window, remove the remote attribute, and load the uri from the original tab. 

I'm new to the e10s team, so let's check with one of the resident experts. :billm, what are your thoughts on the matter?
Flags: needinfo?(wmccloskey)
Yes, we can't use replaceTabWithWindow directly since it calls SwapFrameLoader. Even when bug 918634 is fixed, we'll still only be able to swap frameloaders where the remote attribute is the same. That is, we won't be able to swap a remote frameloader with a non-remote one, which is what we would need here.

That's fine though. For this bug, we can just open a new window that displays the URL from chosen tab.

I think the tasks necessary to fix this bug are as follows:

1. Create a new menu item in the context menu and hook it up to an event handler. Initially the event handler could just call dump() or alert() so you can make sure it works.

2. Change the event handler so it figures out the current URL of the tab that was selected. From the code that you found, it looks like TabContextMenu.contextTab will give you the tab that was chosen. From there, you can access tab.linkedBrowser.currentURI. This will give you a URI object. You can access .spec on that object to get the URI in string form. Then you can print that out somehow for testing.

3. The next step is to open a new window to display the chosen URL. That can be done with code like the following:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#3257
There are two important pieces. First, as part of extraFeatures, you need to pass "non-remote". That will ensure that the new window doesn't use e10s. And for defaultArgs, you need to pass the URL to open.

We might also consider closing the tab in the e10s window. I don't know if that's a good idea, since we'll be losing history information and stuff for that tab. But it probably is what the user would expect to happen, so I dunno.
Flags: needinfo?(wmccloskey)
(In reply to Bill McCloskey (:billm) from comment #7)
> We might also consider closing the tab in the e10s window. I don't know if
> that's a good idea, since we'll be losing history information and stuff for
> that tab. But it probably is what the user would expect to happen, so I
> dunno.

Actually, I think that would be really handy for testing, as that way you could see e10s and non-e10s side by side. I'd vote to leave the original tab alone.
Assignee: nobody → hkaimenas
I've reached the point where I get a new browser window with the correct URL. The problem is that when I go to about:config to check for the remote option it remains true. Here's the line of code that invokes the window:
return window.openDialog("chrome://browser/content/", "_blank", "chrome,all,dialog=no,non-remote", url);

It seems that the two windows, both the original and the new, share the same configuration. If I have about:config open in the originating window and try to navigate to about:config to the new window the browser takes me to the original window's about:config page, implying I can have a single config for all instances.

I've also tried to invoke a non-remote window using:
OpenBrowserWindow({remote: false});

and for the sake of experimentation:
OpenBrowserWindow({non-remote: true}); // This is probably very wrong.

Any advice?
Flags: needinfo?(wmccloskey)
Flags: needinfo?(ally)
Hi Harry. It sounds like you're making good progress!

(In reply to Harry Kaimenas [:hkaimenas] from comment #9)
> I've reached the point where I get a new browser window with the correct
> URL. The problem is that when I go to about:config to check for the remote
> option it remains true. Here's the line of code that invokes the window:
> return window.openDialog("chrome://browser/content/", "_blank",
> "chrome,all,dialog=no,non-remote", url);

The about:config option is not per-window; it applies to all of Firefox. It only controls whether windows are remote *by default*. However, we can still have non-remote windows. If you want to see if a window is remote, visit a normal URL in one of its tabs (like http://mozilla.org) and see if the tab title is underlined. Underlined = remote, not underlined = not remote.

It sounds like your patch is probably working. Would you mind posting it as an attachment and asking Ally and me for feedback?

> It seems that the two windows, both the original and the new, share the same
> configuration. If I have about:config open in the originating window and try
> to navigate to about:config to the new window the browser takes me to the
> original window's about:config page, implying I can have a single config for
> all instances.

That's triggering Firefox's "Switch to tab" feature, which can happen whenever you try to navigate to a URL that's already open in another tab. If you hold down the shift key when you press enter to go to the URL, you will avoid triggering "Switch to tab" and you'll get two copies of the about:config tab.

However, as I said above, Firefox has only one copy of the preferences, so both tabs will show the same data. The only way to see if a window is remote or not is to see if the tab titles are underlined.

> I've also tried to invoke a non-remote window using:
> OpenBrowserWindow({remote: false});

That will open a non-remote window, but it doesn't allow you to specify the URL to open. So openDialog is a little better.

> and for the sake of experimentation:
> OpenBrowserWindow({non-remote: true}); // This is probably very wrong.

That just doesn't work :-).
Flags: needinfo?(wmccloskey)
Attachment #8413356 - Flags: review?(wmccloskey)
Attachment #8413356 - Flags: review?(ally)
Flags: needinfo?(ally)
Comment on attachment 8413356 [details] [diff] [review]
rev1 - Adds new tab context menu option to open in non-remote window.

Review of attachment 8413356 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks very much! I tried out the patch and it works for me. I realized that there's still one major thing that's needed. We want to make sure not to display the menu item in windows already don't use e10s. I've described how to do that below.

::: browser/base/content/browser.xul
@@ +101,5 @@
>                  accesskey="&moveToNewWindow.accesskey;"
>                  tbattr="tabbrowser-multiple"
>                  oncommand="gBrowser.replaceTabWithWindow(TabContextMenu.contextTab);"/>
> +      <menuitem id="context_openNonRemoteWindow" label="&openNonRemoteWindow.label;"
> +                tbattr="tabbrowser-multiple"

This attribute means that the menu item will be enabled if there are multiple tabs in the window. We also want to make sure that the item is not shown in non-e10s windows. To do that, I would suggest changing this line to |tbattr="tabbrowser-remote-multiple"|. Then you can change the code at [1] to handle the tabbrowser-remote-multiple attribute. It would disable the items if:
  1. The window has only one tab, or
  2. if the gMultiProcessBrowser global variable is false.

[1] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6948

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +44,5 @@
>  <!ENTITY  moveToGroup.accesskey              "M">
>  <!ENTITY  moveToNewGroup.label               "New Group">
>  <!ENTITY  moveToNewWindow.label              "Move to New Window">
>  <!ENTITY  moveToNewWindow.accesskey          "W">
> +<!ENTITY  openNonRemoteWindow.label          "Open in new Window - Non Remote">

I would recommend changing the name to "Open in new non-e10s window" since that's closer to the menu items that we already have (e.g., "New e10s window", "New non-e10s window").
Attachment #8413356 - Flags: review?(wmccloskey)
Attachment #8413356 - Flags: review?(ally)
Attachment #8413356 - Flags: feedback+
Attachment #8413356 - Attachment is obsolete: true
Attachment #8413999 - Flags: review?(wmccloskey)
Attachment #8413999 - Flags: review?(ally)
Comment on attachment 8413999 [details] [diff] [review]
rev2 - Adds new tab context menu option to open in non-remote window.

Thanks very much. This looks good to me. A front-end peer [1] has to review this code so I'm switching the review to Felipe.

[1] https://wiki.mozilla.org/Modules/Firefox
Attachment #8413999 - Flags: review?(wmccloskey)
Attachment #8413999 - Flags: review?(felipc)
Attachment #8413999 - Flags: review?(ally)
Comment on attachment 8413999 [details] [diff] [review]
rev2 - Adds new tab context menu option to open in non-remote window.

Review of attachment 8413999 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for sticking with this! It looks pretty good overall.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +44,5 @@
>  <!ENTITY  moveToGroup.accesskey              "M">
>  <!ENTITY  moveToNewGroup.label               "New Group">
>  <!ENTITY  moveToNewWindow.label              "Move to New Window">
>  <!ENTITY  moveToNewWindow.accesskey          "W">
> +<!ENTITY  openNonRemoteWindow.label          "Open in new non-e10s window">

I was hoping I could tell you to re-use an existing entity rather than create a 3rd copy of this string, but it looks like the other two are hardcoded. Ick.

http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/src/CustomizableWidgets.jsm#937

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-menubar.inc#35

I'd support adding a localization note to this new entity in the dtd file, noting that this string should match these other two. 

for example
http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd#26

here's an example of localization notes matching strings in  .properties & .dtdt
http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/devtools/toolbox.properties#55
I wonder if the localization is even necessary - if I'm not mistaken, this menuitem will never ship / go beyond Nightly. In fact, we might want to hide it behind a NIGHTLY_BUILD ifdef to ensure that (I did something like that in bug 997446)
(In reply to Mike Conley (:mconley) from comment #16)
> I wonder if the localization is even necessary - if I'm not mistaken, this
> menuitem will never ship / go beyond Nightly. In fact, we might want to hide
> it behind a NIGHTLY_BUILD ifdef to ensure that (I did something like that in
> bug 997446)

Yup, http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/src/CustomizableWidgets.jsm#937 comments explicitly to that effect.

I wouldn't mind both the comment & the ifdef guard actually. I think both are good ideas.  However, it's felipe's call.

I'm generally in the "If it lands on Nightly it should meet the same standards of quality(I consider comments to be part of quality) that we'd want to ship on GA"

Even if felipe decides it's not required to land because it should never uplift, I want Harry to be aware of localization concerns like reusing entities, structuring comments for the localizer dashboard, and coordinating strings between localization formats that would normally be required of production code. 

Billm, since these entries are not supposed to uplift, do we have a bug on file to remove them? I don't see one in the e10s-m2, and that's when I would expect to find it. If not, we should file one so that we remember to remove them and not get in trouble with l10n >.>
Depends on: 1003313
I filed e10s-m2 bug 1003313 - [e10s] Remove "Open [Non-]e10s Window" menus and strings before enabling e10s on Nightly.
Comment on attachment 8413999 [details] [diff] [review]
rev2 - Adds new tab context menu option to open in non-remote window.

Thanks Harry for the patch! It looks good and is definitely in the right direction, but it needs two things to be changed.

- As we have done in the other two cases, let's hardcode the string in the code (you can do it directly in the label attribute), so that we will avoid having localizers translate it in vain for it to be removed afterwards.
We could do it by backing it out from every uplift before Aurora, but that would be a lot more work for no clear benefit

- Most importantly, instead of just disabling the item for non-e10s windows, we need to have it totally hidden for them. You can add by default a hidden="true" attribute, and have that changed to false in the updateContextMenu function in the right situation.

I'm a bit ambivalent about wrapping it in #ifdef NIGHTLY_BUILD or not, but as it was done in bug 997446, let's do it here too. You can wrap the code added to updateContextMenu, so that if that gets compiled out we will default to hidden="true" from browser.xul
Attachment #8413999 - Flags: review?(felipc) → feedback+
Comment on attachment 8414728 [details] [diff] [review]
rev3 - Adds new tab context menu option to open in non-remote window.

Review of attachment 8414728 [details] [diff] [review]:
-----------------------------------------------------------------

Perfect, thanks!
Attachment #8414728 - Flags: review?(felipc) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/fe53c54ed51c
Keywords: checkin-needed
Whiteboard: [good-first-bug][lang=js][mentor=ally] → [good-first-bug][lang=js][mentor=ally][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/fe53c54ed51c
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [good-first-bug][lang=js][mentor=ally][fixed-in-fx-team] → [good-first-bug][lang=js][mentor=ally]
Target Milestone: --- → Firefox 32
Congratulations on landing your first bug! 

What would you like to work on next?
Thanks for all the help! If you have anything in mind I would gladly do it. Probably something ui-related to consolidate my knowledge although I would like in the future to explore e10s a bit further.
Can we please use an ifdef for these temporary e10s UI things (essentially an alias for NIGHTLY_BUILD) such that we can easily bulk-remove them when they're not needed anymore without leaving cruft behind? That ifdef should wrap all of your temporary code, e.g. also the browser.xul and tabbrowser.xml parts of your patch here.
How about: (no promises about how hard or easy these are, I only skimmed)

https://bugzilla.mozilla.org/show_bug.cgi?id=973565
https://bugzilla.mozilla.org/show_bug.cgi?id=999293
https://bugzilla.mozilla.org/show_bug.cgi?id=941428
https://bugzilla.mozilla.org/show_bug.cgi?id=691608 (see last comment)
https://bugzilla.mozilla.org/show_bug.cgi?id=989875 (if you wanted to go deeper into the e10s part)
https://bugzilla.mozilla.org/show_bug.cgi?id=983189

Tracking bugs if you'd like to have a look for yourself:
https://bugzilla.mozilla.org/show_bug.cgi?id=fxe10s
https://bugzilla.mozilla.org/show_bug.cgi?id=997456
https://bugzilla.mozilla.org/show_bug.cgi?id=997462

Also, I'd really like to encourage you to hop on IRC into the #e10s channel. It's the lifeblood of the group and the fastest way to get help.
Hi,

I was able to reproduce it on Nightly Build 20140430 on Linux x86_64.

And I can confirm the bug is fixed in Mozilla/5.0 (X11; Linux x86_64; rv:32.0) Gecko/20100101 Firefox/32.0 ID:20140502030202 CSet: e2e1b19fcffc

Cheers,
Francesca
Status: RESOLVED → VERIFIED
No longer depends on: 1003313
The same scenario again.
Mozilla implements a broken thing for several (2+) years, and doesn't fix it saying that it'll be 
removed eventually. As a result of that strategy, browser _always_ stays completely broken.

More about said breakage. STR:
1. Open   data:text/html,<body onfocus = "window.i=window.i||window.j||1">
2. Right-click the tab, click "Open in new non-e10s window"

Result:        A new window opens with 5 (FIVE) tabs with irrelevant urls
Expectations:  Only one tab should be in a new window - a tab with url from Step 1
You need to log in before you can comment on or make changes to this bug.