Closed Bug 997446 Opened 10 years ago Closed 10 years ago

[e10s] Add "Open e10s Window" icon to hamburger menu

Categories

(Firefox :: Menus, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 31
Tracking Status
e10s + ---

People

(Reporter: cpeterson, Assigned: mconley)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

      No description provided.
Whiteboard: [good-first-bug][lang=js][mentor=ally]
Yoink.
Assignee: nobody → mconley
Hey ally - for now, should this button exist by default in the menu panel, or should it start in the palette for users to move in?

I assume the former, but just wanted to make sure.
Flags: needinfo?(ally)
We probably want the button in the menu panel by default because we want Nightly users to easily test e10s. Note that this button is a Nightly-only testing feature and (AFAIK) won't ride the trains.
Attached patch Patch v1Splinter Review
(In reply to Chris Peterson (:cpeterson) from comment #3)
> We probably want the button in the menu panel by default because we want
> Nightly users to easily test e10s. Note that this button is a Nightly-only
> testing feature and (AFAIK) won't ride the trains.

Okie doke, I'll stick it behind a NIGHTLY_BUILD ifdef then. Thanks.
Flags: needinfo?(ally)
Comment on attachment 8410536 [details] [diff] [review]
Patch v1

Hey Jared - so this button is for spawning e10s windows if we've got browser.tabs.remote.autostart set to false, and for spawning non-e10s windows if we've got browser.tabs.remote.autostart set to true (and shouldn't be around if browser.tabs.remote is false).

It's only for Nightly builds, hence the shoddy l10n work, and the upside-down new window button icon choice (which blassey said was the decision on that).

How's my driving?
Attachment #8410536 - Flags: review?(jaws)
Comment on attachment 8410536 [details] [diff] [review]
Patch v1

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

Your upside down button also means that :hover:active will place the inner shadow at the inside-bottom of the button instead of the inside-top of the button. Just wanted to let you know about that, as I expect a bug will come in about it ;)

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +168,5 @@
> +#ifdef NIGHTLY_BUILD
> +    if (gPalette.has("e10s-button")) {
> +      let newWindowIndex = panelPlacements.indexOf("new-window-button");
> +      if (newWindowIndex > -1) {
> +        panelPlacements.splice(newWindowIndex + 1, 0, "e10s-button");

I thought this would fail the tests, but this didn't. Also, after applying this patch the e10s-button was in my palette. After clicking on "Restore Defaults" the button got moved to the panel though.
Attachment #8410536 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> Comment on attachment 8410536 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 8410536 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Your upside down button also means that :hover:active will place the inner
> shadow at the inside-bottom of the button instead of the inside-top of the
> button. Just wanted to let you know about that, as I expect a bug will come
> in about it ;)
> 
> ::: browser/components/customizableui/src/CustomizableUI.jsm
> @@ +168,5 @@
> > +#ifdef NIGHTLY_BUILD
> > +    if (gPalette.has("e10s-button")) {
> > +      let newWindowIndex = panelPlacements.indexOf("new-window-button");
> > +      if (newWindowIndex > -1) {
> > +        panelPlacements.splice(newWindowIndex + 1, 0, "e10s-button");
> 
> I thought this would fail the tests, but this didn't. Also, after applying
> this patch the e10s-button was in my palette. After clicking on "Restore
> Defaults" the button got moved to the panel though.

Yeah, I don't think we test with browser.tabs.remote set to true by default. I'll bet testing with --e10s would have failures under customizableui, but by the time we start worrying about that, we'll probably be thinking about removing this temporary button anyways.

Thanks for the review, Jared!

remote:   https://hg.mozilla.org/integration/fx-team/rev/4ce5da6bd6d6
Whiteboard: [good-first-bug][lang=js][mentor=ally] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/4ce5da6bd6d6
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
Comment on attachment 8410536 [details] [diff] [review]
Patch v1

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +164,5 @@
>      if (gPalette.has("switch-to-metro-button")) {
>        panelPlacements.push("switch-to-metro-button");
>      }
>  
> +#ifdef NIGHTLY_BUILD

Was there a reason for not using the same checks as the menu option? It seems like they should be handled the same way. See https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js?rev=0ff00dde7e0c#6815
(In reply to Matthew N. [:MattN] from comment #10)
> Comment on attachment 8410536 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 8410536 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/customizableui/src/CustomizableUI.jsm
> @@ +164,5 @@
> >      if (gPalette.has("switch-to-metro-button")) {
> >        panelPlacements.push("switch-to-metro-button");
> >      }
> >  
> > +#ifdef NIGHTLY_BUILD
> 
> Was there a reason for not using the same checks as the menu option? It
> seems like they should be handled the same way. See
> https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.
> js?rev=0ff00dde7e0c#6815

If you mean the "window.location.href != getBrowserURL()" guard, I suppose it's because I assumed if we weren't in browser.xul, we wouldn't have to worry about the widget, since browser.xul is the only place where (as far as I know) we use CustomizableUI...

If I had to guess, I'd say that the guard is being used here because browser/base/content/chatWindow.xul includes global-scripts.inc, which includes browser.js, so we're making sure we don't accidentally enable those menuitems in that case.

I don't think we need to worry about that for the CustomizableUI widget, since the item would only be available if the non-browser.xul page included customizable toolbars with the same ID(s) as the ones in browser.xul, or the menu button.

Or did I miss something?
Flags: needinfo?(MattN+bmo)
(In reply to Mike Conley (:mconley) from comment #11)
> (In reply to Matthew N. [:MattN] from comment #10)
> > Comment on attachment 8410536 [details] [diff] [review]
> > Patch v1
> > 
> > ::: browser/components/customizableui/src/CustomizableUI.jsm
> > > +#ifdef NIGHTLY_BUILD
> > 
> > Was there a reason for not using the same checks as the menu option? It
> > seems like they should be handled the same way. See
> > https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.
> > js?rev=0ff00dde7e0c#6815
> 
> If you mean the "window.location.href != getBrowserURL()" guard…

No, I mean the #ifdef NIGHTLY_BUILD check.
Flags: needinfo?(MattN+bmo)
Flags: needinfo?(mconley)
I guess I just wanted to make absolutely sure that this widget didn't somehow slip past Nightly. The show / hide logic that the menuitems use didn't really seem appropriate here - this felt more straight-forward.

If it eases your mind, bug 1003313 has been filed to remove all of these items before enabling e10s on Nightly.
Flags: needinfo?(mconley)
Depends on: 1003313
(In reply to Mike Conley (:mconley) from comment #13)
> I guess I just wanted to make absolutely sure that this widget didn't
> somehow slip past Nightly. The show / hide logic that the menuitems use
> didn't really seem appropriate here - this felt more straight-forward.

It seemed like the menuitem was implemented by checking the prefs so that e10s could be tested on any channel if the pref was flipped and I believe comment 3 was just an over-simplification.

My understanding is that bug 1003313 is like 6 months away and I don't see why we'd want to be inconsistent with the UI. It's not a big deal but I wanted to point out the inconsistency. Note that it's easier to do cleanup like bug 1003313 if you can search the code for references to the e10s pref name rather than having to remember that some things were behind #ifdef NIGHTLY_BUILD. Since this is temporary (although long term), I guess it's fine but I would have r-'d FWIW.
(In reply to Matthew N. [:MattN] from comment #14)
> Note that it's easier to do cleanup
> like bug 1003313 if you can search the code for references to the e10s pref
> name rather than having to remember that some things were behind #ifdef
> NIGHTLY_BUILD. Since this is temporary (although long term), I guess it's
> fine but I would have r-'d FWIW.

An ifdef is much more practical to use than a pref in code that isn't JS. That said, overloading NIGHTLY_BUILD defeats the purpose of being able to easily clean this up later. See bug 997436 comment 26...
Depends on: 1004430
Filed bug 1004533 about an E10S_TESTING_UI define.
Depends on: 1007324
QA Whiteboard: [good first verify]
No longer depends on: 1003313
You need to log in before you can comment on or make changes to this bug.