Closed Bug 1191442 Opened 7 years ago Closed 6 years ago

Add New Container to File menu

Categories

(Firefox :: Security, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: englehardt, Assigned: englehardt)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 11 obsolete files)

9.67 KB, patch
englehardt
: review+
Details | Diff | Splinter Review
24.52 KB, patch
englehardt
: review+
Details | Diff | Splinter Review
We want users to be able to open a new container/userContext via the File menu, matching the experience of opening a New Tab via the File menu. Users should be able to choose from one of four pre-set containers (Home, Work, Shopping, Banking), which correspond to 4 different userContextIds (Bug 1179557). The new tab should have the userContextId as an attribute which can then be set on the docShell.

See Bug 1191418 for more information on the Containers Project.
Assignee: nobody → senglehardt
Blocks: 1191451
Blocks: 1191455
Blocks: 1191460
Status: NEW → ASSIGNED
Blocks: 1191494
Attached patch 1191442-filemenu.patch (obsolete) — Splinter Review
WIP Patch. privacy.userContext.enabled must be set to true before menu is exposed. PNGs not currently sized properly. Will probably end up splitting this patch up.
Attached patch 1191442.patch (obsolete) — Splinter Review
Fixes problems with previous patch and moves some code that isn't File Menu specific to Bug 1191460.

Tanvi: Reading the comment at the top of all.js (https://dxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.js?from=all.js&offset=200#6) makes me think that I should probably be setting the pref in firefox.js (https://dxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js) instead?
Attachment #8644559 - Attachment is obsolete: true
Flags: needinfo?(tanvi)
No longer blocks: 1191460
Depends on: 1191460
(In reply to Steven Englehardt [:senglehardt] from comment #2)
> Tanvi: Reading the comment at the top of all.js
> (https://dxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.
> js?from=all.js&offset=200#6) makes me think that I should probably be
> setting the pref in firefox.js
> (https://dxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.
> js) instead?

Yes, looks like you are right.  firefox.js it is.
Flags: needinfo?(tanvi)
Attached patch 1191442.patch (obsolete) — Splinter Review
Place pref in firefox.js
Attachment #8647202 - Attachment is obsolete: true
Comment on attachment 8647253 [details] [diff] [review]
1191442.patch

diff --git a/browser/themes/osx/jar.mn b/browser/themes/osx/jar.mn
--- a/browser/themes/osx/jar.mn
+++ b/browser/themes/osx/jar.mn
@@ -565,16 +565,24 @@ browser.jar:
   skin/classic/browser/devtools/app-manager/add.svg                   (../shared/devtools/app-manager/images/add.svg)
   skin/classic/browser/devtools/app-manager/index-icons.svg           (../shared/devtools/app-manager/images/index-icons.svg)
   skin/classic/browser/devtools/app-manager/rocket.svg                (../shared/devtools/app-manager/images/rocket.svg)
   skin/classic/browser/devtools/app-manager/noise.png                 (../shared/devtools/app-manager/images/noise.png)
   skin/classic/browser/devtools/app-manager/default-app-icon.png      (../shared/devtools/app-manager/images/default-app-icon.png)
   skin/classic/browser/devtools/search-clear-failed.svg               (../shared/devtools/images/search-clear-failed.svg)
   skin/classic/browser/devtools/search-clear-light.svg                (../shared/devtools/images/search-clear-light.svg)
   skin/classic/browser/devtools/search-clear-dark.svg                 (../shared/devtools/images/search-clear-dark.svg)
+  skin/classic/browser/userContextMenuPersonal16.png              (../shared/userContextMenuPersonal16.png)
+  skin/classic/browser/userContextMenuPersonal16@2x.png           (../shared/userContextMenuPersonal16@2x.png)
+  skin/classic/browser/userContextMenuWork16.png                  (../shared/userContextMenuWork16.png)
+  skin/classic/browser/userContextMenuWork16@2x.png               (../shared/userContextMenuWork16@2x.png)
+  skin/classic/browser/userContextMenuBanking16.png               (../shared/userContextMenuBanking16.png)
+  skin/classic/browser/userContextMenuBanking16@2x.png            (../shared/userContextMenuBanking16@2x.png)
+  skin/classic/browser/userContextMenuShopping16.png              (../shared/userContextMenuShopping16.png)
+  skin/classic/browser/userContextMenuShopping16@2x.png           (../shared/userContextMenuShopping16@2x.png)
Fix indentation here.



+    <command id="Browser:userContext1" oncommand="openUILinkIn(BROWSER_NEW_TAB_URL, 'tab', {userContextId:1})" reserved="true"/>
+    <command id="Browser:userContext2" oncommand="openUILinkIn(BROWSER_NEW_TAB_URL, 'tab', {userContextId:2})" reserved="true"/>
+    <command id="Browser:userContext3" oncommand="openUILinkIn(BROWSER_NEW_TAB_URL, 'tab', {userContextId:3})" reserved="true"/>
+    <command id="Browser:userContext4" oncommand="openUILinkIn(BROWSER_NEW_TAB_URL, 'tab', {userContextId:4})" reserved="true"/>
Add comment to https://dxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js#167 indicating that it can take a userContextId parameter.
Attachment #8647253 - Flags: feedback+
Attached patch 1191442.patch (obsolete) — Splinter Review
Depends on patch in Bug 1191460.
Attachment #8647253 - Attachment is obsolete: true
Attachment #8647855 - Flags: review?(dolske)
No longer blocks: 1191455
No longer blocks: 1191451
Attachment #8647855 - Flags: review?(dolske) → review?(paolo.mozmail)
Comment on attachment 8647855 [details] [diff] [review]
1191442.patch

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

I did just a quick first pass for the moment.

::: browser/app/profile/firefox.js
@@ +1888,5 @@
>  pref("privacy.trackingprotection.introCount", 0);
>  pref("privacy.trackingprotection.introURL", "https://support.mozilla.org/kb/tracking-protection-firefox");
>  
> +// Enable Contextual Identity Containers
> +pref("privacy.userContext.enabled", false);

So, is the feature name we want to use across the tree "userContext"?

I'm thinking of identifiers like menu_newContainerTab vs. menu_newUserContextTab.

Probably the string "userContext" is more unique than "container" for global searches.

::: browser/base/content/browser-menubar.inc
@@ +11,5 @@
>                  style="border:0px;padding:0px;margin:0px;-moz-appearance:none">
>              <menu id="file-menu" label="&fileMenu.label;"
>                    accesskey="&fileMenu.accesskey;">
> +              <menupopup id="menu_FilePopup"
> +                         onpopupshowing="updateUserContextUIVisibility();">

I'm surprised we don't have other places where we had to control the visibility of File menu entries already.

@@ +21,5 @@
> +                <menu id="menu_newUserContext"
> +                      label="&newUserContext.label;"
> +                      hidden="true">
> +                  <menupopup>
> +                    <menuseparator/>

Is this menu separator needed?

@@ +25,5 @@
> +                    <menuseparator/>
> +                    <menuitem id="menu_userContext1"
> +                              class="menuitem-iconic"
> +                              label="&userContext1.label;"
> +                              command="Browser:userContext1"/>

It's fine to hardcode menu items for now rather than add them dynamically, but we could already use a single handler for all of them that reads the ID from an attribute.

                    <menuitem id="menu_newUserContextTabPersonal"
                              usercontextid="1"
                              class="menuitem-iconic"
                              label="&userContext1.label;"
                              command="Browser:newUserContextTab"/>

::: browser/base/content/browser.js
@@ +4001,5 @@
> +{
> +  let userContextEnabled = Services.prefs.getBoolPref("privacy.userContext.enabled");
> +  let userContextMenu = document.getElementById("menu_newUserContext");
> +  if (userContextEnabled && userContextMenu.hidden) {
> +    userContextMenu.hidden = false;

Just set .hidden without checking the current state.

::: browser/themes/linux/browser.css
@@ +522,5 @@
>  #panelMenu_unsortedBookmarks {
>    list-style-image: url("chrome://browser/skin/places/unsortedBookmarks.png");
>  }
>  
> +#menu_userContext1 {

So this would be "#menu_newUserContextTabPersonal" and so on. Matches the file names better.
Attachment #8647855 - Flags: review?(paolo.mozmail)
(In reply to :Paolo Amadini from comment #7)
> So, is the feature name we want to use across the tree "userContext"?
> 
> I'm thinking of identifiers like menu_newContainerTab vs.
> menu_newUserContextTab.
> 
> Probably the string "userContext" is more unique than "container" for global
> searches.

Yes, we'll want all code to refer to userContext. For now, we'll keep the UI using "Container" but I imagine that may change to something else down the line.

> I'm surprised we don't have other places where we had to control the
> visibility of File menu entries already.

The Edit and View menus call functions to update visibility in the the same way I am. Doesn't seem like there is one for File. If you know of other locations it would be, would you mind to point me in the right direction?

Edit: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-menubar.inc?case=true&from=browser-menubar.inc#116
View: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-menubar.inc?case=true&from=browser-menubar.inc#189

> It's fine to hardcode menu items for now rather than add them dynamically,
> but we could already use a single handler for all of them that reads the ID
> from an attribute.
> 
>                     <menuitem id="menu_newUserContextTabPersonal"
>                               usercontextid="1"
>                               class="menuitem-iconic"
>                               label="&userContext1.label;"
>                               command="Browser:newUserContextTab"/>

I agree that having 4 separate commands is ugly. Do you know of a way to pass an argument to a XUL command?

I could call the same javascript function from all 4 menuitems. The function could check attributes of the menuitems, but since we don't know the caller we won't know which userContextId to set. Since a click on the menuitem calls the command, I don't see an option for setting a common attribute in the menuitem that javascript can check. Looking at the menuitem documentation there doesn't seem to be a separate onclick we can use: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/menuitem.
Flags: needinfo?(paolo.mozmail)
(In reply to Steven Englehardt [:senglehardt] from comment #9)
> (In reply to :Paolo Amadini from comment #7)
> > I'm surprised we don't have other places where we had to control the
> > visibility of File menu entries already.
> 
> The Edit and View menus call functions to update visibility in the the same
> way I am. Doesn't seem like there is one for File.

That's fine, I'd just have expected one to exist already.

> I could call the same javascript function from all 4 menuitems. The function
> could check attributes of the menuitems, but since we don't know the caller
> we won't know which userContextId to set.

I can think of a few ways, though I'm not sure all actually work. In order of most preferred to least preferred:
- Look at the "event" argument from the event handler, get the "target" and check its attributes, assuming the "target" gives you the "menuitem" and not the "command".
- Since "command" elements are similar to "broadcaster", you may actually be able to pass "this" in the "oncommand" handler and that would reference to the "menuitem" as if the "oncommand" was placed on the "menuitem" itself.
- You could place oncommand="myFunction(1);" directly on the menuitem. This is similar to what the Recently Closed Tabs menu does by generating the "oncommand" JavaScript code dynamically.

References:

https://developer.mozilla.org/en-US/docs/Web/Events/command
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/command
Flags: needinfo?(paolo.mozmail)
Attached patch 1191442-pt2-filemenu.patch (obsolete) — Splinter Review
Attachment #8647855 - Attachment is obsolete: true
Attachment #8650053 - Flags: review?(paolo.mozmail)
Attachment #8649395 - Flags: review?(paolo.mozmail)
Blocks: 1191455
Blocks: 1191451
No longer depends on: 1191460
When a tab is swapped between two windows, a new browser is created and the old one removed. When that happens, the browser attribute 'userContextId' that the containers UI is relying on is lost. I added some code to ensure that the userContextId persists during these swaps.

This is one of two ways to achieve this, the other would be to wait until after the docShells are swapped and then read the ID off the swapped docShell.
Attachment #8649395 - Attachment is obsolete: true
Attachment #8649395 - Flags: review?(paolo.mozmail)
Attachment #8650137 - Flags: review?(paolo.mozmail)
Bram points out that "New Container Tab" doesn't work for future iterations of the UI since we'll eventually want to add other container-related options into the menu, such as "Add New Container" or "Edit Containers". We both agree "New Container Tab" is fine for now given the options in this iteration.
Comment on attachment 8650053 [details] [diff] [review]
1191442-pt2-filemenu.patch

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

Looks good, most things to address are details.

I'd like to see if we can use SVG icons for the menus.

::: browser/base/content/browser-menubar.inc
@@ +19,5 @@
>                            key="key_newNavigatorTab"
>                            accesskey="&tabCmd.accesskey;"/>
> +                <menu id="menu_newUserContext"
> +                      label="&newUserContext.label;"
> +                      hidden="true">

We need an accesskey for the main options and the sub-menu options.

@@ +22,5 @@
> +                      label="&newUserContext.label;"
> +                      hidden="true">
> +                  <menupopup>
> +                    <menuitem id="menu_newUserContextTabPersonal"
> +                              usercontextid = "1"

nit: remove space around the equal sign

::: browser/base/content/browser-sets.inc
@@ +91,5 @@
>      <command id="cmd_gestureRotateRight" oncommand="gGestureSupport.rotate(event.sourceEvent)"/>
>      <command id="cmd_gestureRotateEnd" oncommand="gGestureSupport.rotateEnd()"/>
>      <command id="Browser:OpenLocation" oncommand="openLocation();"/>
>      <command id="Browser:RestoreLastSession" oncommand="restoreLastSession();" disabled="true"/>
> +    <command id="Browser:newUserContextTab" oncommand="openNewUserContextTab(event.sourceEvent.composedTarget);" reserved="true"/>

Browser:NewUserContextTab, uppercase like the other commands.

You can use "event.sourceEvent.target", no need for "composedTarget". I also suggest passing "event.sourceEvent" as the argument to openNewUserContextTab, and read "target" from there.

::: browser/base/content/browser.js
@@ +3992,5 @@
>  #endif
>  }
>  
>  /**
> + * User Context UI helper functions

Describe what this specific function does and use @param to describe the argument.

@@ +3997,5 @@
> + */
> +function openNewUserContextTab(menuitem)
> +{
> +  let userContextIdAttr = menuitem.getAttribute('usercontextid');
> +  openUILinkIn(BROWSER_NEW_TAB_URL, 'tab', {userContextId:userContextIdAttr})

Prefer double quotes and add semicolon. Try this:

openUILinkIn(BROWSER_NEW_TAB_URL, "tab", {
  userContextId: event.target.getAttribute("usercontextid"),
});

@@ +4000,5 @@
> +  let userContextIdAttr = menuitem.getAttribute('usercontextid');
> +  openUILinkIn(BROWSER_NEW_TAB_URL, 'tab', {userContextId:userContextIdAttr})
> +}
> +
> +// Updates file menu User Context UI visibility depending on pref state

Use JSDoc (/** */) comment.

@@ +4005,5 @@
> +function updateUserContextUIVisibility()
> +{
> +  let userContextEnabled = Services.prefs.getBoolPref("privacy.userContext.enabled");
> +  let userContextMenu = document.getElementById("menu_newUserContext");
> +  userContextMenu.hidden = userContextEnabled == false;

document.getElementById("menu_newUserContext").hidden = !userContextEnabled;

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +308,5 @@
>  
>  <!ENTITY getMoreDevtoolsCmd.label        "Get More Tools">
>  <!ENTITY getMoreDevtoolsCmd.accesskey    "M">
>  
>  <!ENTITY fileMenu.label         "File"> 

Remove whitespace at end of line.

Don't separate the label from its access key.

Add access keys for the new options as well.

::: browser/themes/linux/jar.mn
@@ -504,5 @@
>  #ifdef E10S_TESTING_ONLY
>    skin/classic/browser/e10s-64@2x.png (../shared/e10s-64@2x.png)
>  #endif
>    skin/classic/browser/warning.svg                        (../shared/warning.svg)
> -

Fix unintentional line removal.
Attachment #8650053 - Flags: review?(paolo.mozmail)
(In reply to :Paolo Amadini from comment #14)
> I'd like to see if we can use SVG icons for the menus.

Okay, so using a SVG list-style-image for menus works, though in order for it to render well on HiDPI you have to use a version of the SVG scaled up to fit a 32px viewport (width="32" height="32" viewBox="0 0 32 32").

I'd rather we only had one SVG asset rather than two PNGs plus one SVG. If for some reason any of the SVGs don't render well when scaled down to 16px, we can have a special adapted version (width="16" height="16" viewBox="0 0 16 16") for them and a CSS rule to use the 16px version on lower resolutions, for a total of two SVGs. Maybe that won't be necessary as the icons from bug 1191455 seem to render well in the URL bar.
Also, please move the assets to a subfolder (lowercase) and shorten the name, since they will be shared by bug 1191455 and will not be specific to the menu, for example shared/usercontext/work.svg, shared/usercontext/banking.svg, ...

If you needed special 16px versions, they would be named work-16.svg, banking-16.svg, ...
Comment on attachment 8650137 [details] [diff] [review]
1191442-pt1-tabbrowser-support.patch

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

::: browser/base/content/tabbrowser.xml
@@ +1691,5 @@
>              b.setAttribute("contextmenu", this.getAttribute("contentcontextmenu"));
>              b.setAttribute("tooltip", this.getAttribute("contenttooltip"));
>  
> +            if (userContextId)
> +              b.setAttribute("userContextId", userContextId);

Hm, we may have to use an all-lowercase attribute name for consistency.

@@ +1838,5 @@
>              if (!b) {
>                // No preloaded browser found, create one.
> +              b = this._createBrowser({remote: remote,
> +                                       uriIsAboutBlank: uriIsAboutBlank,
> +                                       userContextId: aUserContextId});

nit: feel free to keep the shortcuts for "remote" and "uriIsAboutBlank".

@@ +2503,5 @@
>  
> +            // Swap the browsers' userContextId
> +            let ourUserContext = ourBrowser.getAttribute("userContextId");
> +            ourBrowser.setAttribute("userContextId", otherBrowser.getAttribute("userContextId"));
> +            otherBrowser.setAttribute("userContextId", ourUserContext);

This apparently sets the attribute regardless of whether it was originally present (no "hasAttribute" call).

I'd rather us be consistent here: either the attribute is present unconditionally and equal to empty string or zero for normal tabs, or absent for normal tabs.
Attachment #8650137 - Flags: review?(paolo.mozmail)
In my review yesterday I've forgotten to recommend including a shared CSS file for all themes from "browser.css", something like "shared/usercontext/usercontext.inc.css".

You still have to update the jar.mn files individually for all platforms, also adding the new CSS file.

If you still need rules for HiDPI for some reason, you can use an approach using preprocessor directives similar to the one used in "toolbarbuttons.inc.css".
(In reply to :Paolo Amadini from comment #17)

> @@ +1838,5 @@
> >              if (!b) {
> >                // No preloaded browser found, create one.
> > +              b = this._createBrowser({remote: remote,
> > +                                       uriIsAboutBlank: uriIsAboutBlank,
> > +                                       userContextId: aUserContextId});
> 
> nit: feel free to keep the shortcuts for "remote" and "uriIsAboutBlank".

I think it's more readable to keep remote and uriIsAboutBlank there as both keys and values since both key and value are required for userContextId.

> I'd rather us be consistent here: either the attribute is present unconditionally and equal to empty string or zero for normal tabs, or absent for normal tabs.

Agreed. I've added in hasAttribute and changed it from swapping to just pulling the attribute off the old browser and setting in on the new browser (since the old browser is no longer used).
(In reply to :Paolo Amadini from comment #14)

> ::: browser/base/content/browser-menubar.inc
> @@ +19,5 @@
> >                            key="key_newNavigatorTab"
> >                            accesskey="&tabCmd.accesskey;"/>
> > +                <menu id="menu_newUserContext"
> > +                      label="&newUserContext.label;"
> > +                      hidden="true">
> 
> We need an accesskey for the main options and the sub-menu options.

I've added accesskeys. Does that mean I also should add in shortcuts to the menus, or does accesskey have other uses?
Flags: needinfo?(paolo.mozmail)
Attachment #8650137 - Attachment is obsolete: true
Attachment #8655757 - Flags: review?(paolo.mozmail)
Attached patch 1191442-pt2-filemenu.patch (obsolete) — Splinter Review
Attachment #8650053 - Attachment is obsolete: true
Attachment #8655758 - Flags: review?(paolo.mozmail)
Comment on attachment 8655758 [details] [diff] [review]
1191442-pt2-filemenu.patch

(In reply to Steven Englehardt [:senglehardt] from comment #20)
> I've added accesskeys. Does that mean I also should add in shortcuts to the
> menus, or does accesskey have other uses?

If the question is whether you should add a shurtcut key ("key" attribute) usable when the menu is closed, no, you don't need them.
Flags: needinfo?(paolo.mozmail)
Attachment #8655758 - Flags: review?(paolo.mozmail) → review+
Comment on attachment 8655757 [details] [diff] [review]
1191442-pt1-tabbrowser-support.patch

(In reply to Steven Englehardt [:senglehardt] from comment #19)
> Agreed. I've added in hasAttribute and changed it from swapping to just
> pulling the attribute off the old browser and setting in on the new browser
> (since the old browser is no longer used).

I think it's correct, and it works for me.

Tim, I'm not too familiar with this code, do you think you can sanity check it or get feedback from someone who is?
Attachment #8655757 - Flags: review?(paolo.mozmail)
Attachment #8655757 - Flags: review+
Attachment #8655757 - Flags: feedback?(ttaubert)
Comment on attachment 8655757 [details] [diff] [review]
1191442-pt1-tabbrowser-support.patch

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

This looks fine to me, but it can't hurt to have Dao take a look as well.
Attachment #8655757 - Flags: feedback?(ttaubert)
Attachment #8655757 - Flags: feedback?(dao)
Attachment #8655757 - Flags: feedback+
Attachment #8655757 - Flags: feedback?(dao) → feedback+
Carrying over r+ from Paolo. Needed to update after tree refresh.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ba18a86e004
Attachment #8655757 - Attachment is obsolete: true
Attachment #8657378 - Flags: review+
Attached patch 1191442-pt2-filemenu.patch (obsolete) — Splinter Review
Carrying over r+ from Paolo. Needed to updated after tree refresh.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ba18a86e004
Attachment #8655758 - Attachment is obsolete: true
Attachment #8657380 - Flags: review+
Did another push to try with just the two 1191442 patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0484846f2840

4 of the repeated failures are now disabled (and a 5th looks like it should be):
OSX 10.6 debug - dt1		- DISABLED: https://bugzilla.mozilla.org/show_bug.cgi?id=1203895#c72
Windows XP opt - oth 		- DISABLED: https://hg.mozilla.org/mozilla-central/rev/ee0f02a2fc74
Windows XP debug - dt2		- DISABLED: https://bugzilla.mozilla.org/show_bug.cgi?id=1203895#c72
Android 4.3 API11+ debug - 14	- DISABLED: https://bugzilla.mozilla.org/show_bug.cgi?id=1202045#c197
Linux debug - bc3 		- Very Frequent: https://bugzilla.mozilla.org/show_bug.cgi?id=1168398

Thus the only two test failures that are suspect are the two Jetpack timeouts on Linux. (JP on Linux debug and Linux x64 asan). Not sure if / how the changes in this patch are contributing to the timeout, but I'll keep checking into it.
Carrying over r+ from Paolo. Updated for changes in tree.
Attachment #8666675 - Flags: review+
Attachment #8657378 - Attachment is obsolete: true
Carrying over r+ from Paolo. Updated for changes to tree.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a96306a84fc8
Attachment #8657380 - Attachment is obsolete: true
Attachment #8666676 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fa2906b5ee4e
https://hg.mozilla.org/mozilla-central/rev/9eb07902468e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Depends on: 1211716
I'm just a Firefox user trying to make sense of the new menu item "Add New Container" that showed up in the released version of Firefox 44.0. I'm on Mac OS X 10.11.3. On Macs, Firefox uses the Mac menu bar, not the "hamburger". In the "File" menu on the menu bar, there is a new item "Add New Container". I have checked "about:config", and "privacy.userContext.enabled" is set to its default value of "false". Earlier in this thread, it was implied that it needed to be "true" for the menu to be "exposed". Is that not the case anymore? I'd really like to disable this menu item, so that I can continue my previous menu usage pattern to reach "New Window". Did you really have to make the container thing the third menu item?
Sorry, I found the real problem. I had an add-on installed called "Menu Wizard" that I once tried for manipulating the Firefox menu. When I disabled the add-on, the "Add New Container" menu item follows "privacy.userContext.enabled", as designed. I'll let the add-on developer know the problem.
You need to log in before you can comment on or make changes to this bug.