1558565 - Fix accounts and folders submenus in the new appmenu

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, task, P1)

Description

[Paul Morris [:pmorris]]

As a follow-up on bug 1546309, four of the appmenu submenus need to display nested account/folder items and submenus. These are:

• File > Get New Messages For
• Go > Folder
• Message > Move To
• Message > Copy To

The folder-menupopup custom element handles this for non-appmenu menus. Adapt the folder-menupopup code so it can be used for the appmenu.

This was deferred to a follow-up because the folder-menupopup was not yet de-xbl'd yet at the time, and due to the size of the bug 1546309.

This will need to land for both 68 beta and 69 trunk.

One approach would be to make a custom element for the appmenu submenus that shares some of the code with the folder-menupopup custom element (likely via a mixin, since I don't think the appmenu CE will need or want to extend the MozElements.MozMenuPopup like folder-menupopup does).

Comment 1

[Paul Morris [:pmorris]]

Attachment: part1-appmenu-folders-refactor-0.patch

1 of 3 patches ready for review. This first one is some refactoring I did while getting familiar with this code, to make it easier to understand.

Comment 2

[Paul Morris [:pmorris]]

Attachment: part2-fix-appmenu-folders-0.patch

2 of 3 patches for review. This one fixes this bug.

Comment 3

[Paul Morris [:pmorris]]

Attachment: part3-add-favorites-submenu-0.patch

3 of 3 patches for review. This one just adds a "Favorites" submenu to the appmenu > go > folder menu, to match the main menu bar's go > folder.

Comment 4

[Paul Morris [:pmorris]]

Attachment: part1-appmenu-folders-refactor-1.patch

Fix a few eslint issues that slipped through before.

Comment 5

[Paul Morris [:pmorris]]

Attachment: part2-fix-appmenu-folders-1.patch

Fix a few eslint issues that slipped through before.

Comment 6

[:aceman]

Wtf? The Firefox's appmenu must really infect everything?
What are the real changes in the folder-menupopup code?

Comment 7

[:aceman]

Comment on attachment 9072843 [details] [diff] [review]
part1-appmenu-folders-refactor-1.patch

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

::: mailnews/base/content/folder-menupopup.js
@@ +174,5 @@
>            // xxx we can optimize this later
>            this._clearMenu(this._menu);
>          },
>  
> +        OnItemAdded(RDFParentItem, item) {

Please no 'RDF' in names if possible. RDF is gone.

Comment 8

[Richard Marti (:Paenglab)]

Comment on attachment 9072844 [details] [diff] [review]
part2-fix-appmenu-folders-1.patch

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

::: mailnews/base/content/folder-menupopup.js
@@ +1138,5 @@
> +      this.menuOrToolbarbuttonTagName = "toolbarbutton";
> +      this.menuitemOrToolbarbuttonTagName = "toolbarbutton";
> +      this.menupopupOrPanelviewTagName = "panelview";
> +      this.menuitemOrToolbarbuttonClasses = "folderMenuItem subviewbutton subviewbutton-iconic";
> +      this.menuOrToolbarbuttonClasses = "folderMenuItem subviewbutton subviewbutton-nav";

You need to add here the class 'subviewbutton-iconic' too. Without this the folder menuitems are indented.

Comment 9

[:aceman]

(In reply to Paul Morris [:pmorris] from comment #1)

Created attachment 9072836 [details] [diff] [review]
part1-appmenu-folders-refactor-0.patch

1 of 3 patches ready for review. This first one is some refactoring I did while getting familiar with this code, to make it easier to understand.

This one is not as bad, but part 2 will make it unreadable for others.

Comment 10

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #7)

Please no 'RDF' in names if possible. RDF is gone.

Let's say: On the way out.

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9072843 [details] [diff] [review]
part1-appmenu-folders-refactor-1.patch

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

Seems ok to me, with some comments and nits below

::: mailnews/base/content/folder-menupopup.js
@@ +151,1 @@
>        this._listener = {

you could add // @see {nsIFolderListener}

... and adjust what the params are called (like aceman said, not RDF)

@@ +415,2 @@
>  
> +      if (excludeServers.length > 0) {

doesn't matter too much, but this check is a bit redundant

@@ +475,5 @@
> +      }
> +      if (showRecent || showFavorites) {
> +        // If we added Recent and/or Favorites, separate them from the rest of the items.
> +        const sep = document.createXULElement("menuseparator");
> +        sep.setAttribute("generated", "true");

was wondering what this was about. 
Seems this should be removed (maybe check child.localName == "menuseparator"?), and the comment/code around here adjusted or removed: https://searchfox.org/comm-central/rev/7ced9f2f93c1c8c75e820764f631f58fcabfdcef/mail/base/content/mailCore.js#284

@@ +622,5 @@
> +      let parent = this._parentFolder;
> +      if (parent && (this.getAttribute("showFileHereLabel") == "true" || !mode)) {
> +        let showAccountsFileHere = this.getAttribute("showAccountsFileHere");
> +        if ((!parent.isServer || showAccountsFileHere != "false") &&
> +          (!mode || mode == "newFolder" || parent.noSelect ||

nit: we usually align that like

if ((!parent.isServer
     (!mode

@@ +637,5 @@
> +            menuitem.setAttribute("class", "folderMenuItem menuitem-iconic");
> +            this._setCssSelectors(parent, menuitem);
> +          }
> +          // Eww. have to support some legacy code here.
> +          menuitem.setAttribute("id", parent.URI);

existing code, but it's wrong to set the id here inside the CE. you could have id collisions if the widget is used in more than one place in the document

@@ +700,5 @@
> +          popup._parentFolder = folder;
> +
> +          ["class", "type", "fileHereLabel", "showFileHereLabel",
> +            "oncommand", "mode", "disableServers", "position",
> +          ].forEach(attribute => {

would move the ].forEach(attribute => { to the end of the previous line

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 9072844 [details] [diff] [review]
part2-fix-appmenu-folders-1.patch

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

::: mailnews/base/content/folder-menupopup.js
@@ +19,5 @@
>    const { MailUtils } = ChromeUtils.import("resource:///modules/MailUtils.jsm");
>    const { StringBundle } = ChromeUtils.import("resource:///modules/StringBundle.js");
>  
>    /**
> +   * Returns an element with the "generated" attribute set to "true".

not sure the generated attr is really needed for anything... (should be needed for anything)

@@ +70,5 @@
> +   *
> +   * @param {Class} Base  A class to be extended with shared functionality.
> +   * @return {Class}      A class that extends the first class.
> +   */
> +  const FolderMenuMixin = (Base) => class extends Base {

would use let or var for this

@@ +295,4 @@
>              return null;
>            }
> +          const children = menu.appendTarget.childNodes;
> +          for (let i = 0; i < children.length; i++) {

could switch this over to

for (let child of menu.appendTarget.childNodes) {

}

@@ +513,5 @@
> +        submenu.appendTarget = submenu;
> +      } else {
> +        // We are in the appmenu and `this.localName == "panelview"`.
> +        const subViewId = getUniqueSubViewId("specialFoldersSubView");
> +        submenu.setAttribute("id", subViewId);

what are the ids used for?

@@ +728,2 @@
>            // Grumble, grumble, legacy code support
>            node.setAttribute("id", folder.URI);

I think this is wrong, like noted on another patch

@@ +1046,5 @@
> +      this.menuOrToolbarbuttonTagName = "menu";
> +      this.menuitemOrToolbarbuttonTagName = "menuitem";
> +      this.menupopupOrPanelviewTagName = "menupopup";
> +      this.menuitemOrToolbarbuttonClasses = "folderMenuItem menuitem-iconic";
> +      this.menuOrToolbarbuttonClasses = "folderMenuItem menu-iconic";

it's rather ugly to have all of these. I wonder if you could have a per class function to generate needed nodes instead

@@ +1168,5 @@
> +     * @param {Element} menuItem  The menu item, typically a `toolbarbutton`.
> +     * @return {Element}  The popup, typically a `panelview` element.
> +     */
> +    _getPopupForMenuItem(menuItem) {
> +      return document.getElementById(menuItem.getAttribute("subViewId"));

oh here the id is used I guess. But isn't it always a child you could just query?

Comment 13

[Jorg K (CEST = GMT+2)]

Paul, is this covered somewhere:
11:58:13 - jorgk: darktrojan: Ever seen this one: console.warn: Overlays.jsm: Could not resolve 7 references Array [{},{},{},{},{},{},{}]
11:58:36 - darktrojan: yes, it's Lightning complaining that the appmenu was changed

Comment 14

[Paul Morris [:pmorris]]

aceman:
"RDF" was in pre-existing names, but I can go ahead and remove it in the refactoring patch (part 1). Thanks for the heads up on that. To respond to your other comments, rather than just copy the code and modify it, I'm following DRY principles to avoid code duplication and maintenance overhead. As you can see in the patch there is a lot of shared code between the two cases but also significant differences in the menus that need different handling. For example, <menupopup>s are children of a <menu> leading to a deeply nested structure but <panelview>s are siblings, a flat structure. You can append menu items directly to a <menupopup> but a <panelview> has a <vbox> child where menu items need to be appended. Etc. (Search for "this.localName ==" (menupopup or panelview) to see where there's branching and what the differences are.) While the code likely can be improved with further refactoring, and I have some ideas for this, especially given the time constraints I didn't want to do more refactoring before getting some feedback via code review. I welcome constructive suggestions.

Richard:
Thanks for catching that CSS issue. I'll fix it in the next revisions.

Jorg:
That's tracked here: https://bugzilla.mozilla.org/show_bug.cgi?id=1558599 I started on it yesterday and will add some info to the bug today.

Magnus:
Thanks for the reviews and the quick turn around.

Comment 15

[Paul Morris [:pmorris]]

(In reply to Magnus Melin [:mkmelin] from comment #11)

::: mailnews/base/content/folder-menupopup.js
@@ +151,1 @@

   this._listener = {

you could add // @see {nsIFolderListener}

So the following was already there (although the bugzilla diff view hid it). So I've left it as is.

  /**
   * Our listener to let us know when folders change/appear/disappear so
   * we can know to rebuild ourselves.
   *
   * @implements {nsIFolderListener}
   */
  this._listener = {

... and adjust what the params are called (like aceman said, not RDF)

Sounds good. I've renamed them to remove the "RDF".

@@ +415,2 @@

  if (excludeServers.length > 0) {

doesn't matter too much, but this check is a bit redundant

I've removed the redundant "> 0" part.

@@ +475,5 @@

   sep.setAttribute("generated", "true");

was wondering what this was about.
Seems this should be removed (maybe check child.localName ==
"menuseparator"?), and the comment/code around here adjusted or removed:
https://searchfox.org/comm-central/rev/
7ced9f2f93c1c8c75e820764f631f58fcabfdcef/mail/base/content/mailCore.js#284

I think the "generated" attribute is needed for use in _tearDown:
https://searchfox.org/comm-central/source/mailnews/base/content/folder-menupopup.js#892
If you have <menuitem>s or <menuseparator>s that are not generated dynamically but statically in XUL files then you want to be able to leave them alone in _tearDown and remove only the dynamically generated elements. For example, the File > Get New Messages For popup:
https://searchfox.org/comm-central/source/mail/base/content/mainNavigationToolbox.inc.xul#113
So I have left this as-is.

nit: we usually align that like

if ((!parent.isServer
(!mode

Ah, ok, I've made changes for all if statements in this file.
(He dreams about a future where there is auto-formatting for JS code.)

@@ +637,5 @@

      // Eww. have to support some legacy code here.
      menuitem.setAttribute("id", parent.URI);

existing code, but it's wrong to set the id here inside the CE. you could
have id collisions if the widget is used in more than one place in the
document

Good point. I've removed this in two places. I checked to see that there were no console errors when using a few of these folders submenus. Everything worked as expected. Hopefully the tests will tell us if this breaks the mysterious "legacy code" mentioned in the comments here.

@@ +700,5 @@

      ["class", "type", "fileHereLabel", "showFileHereLabel",
        "oncommand", "mode", "disableServers", "position",
      ].forEach(attribute => {

would move the ].forEach(attribute => { to the end of the previous line

Done.

Will upload the new patch and then move on to the next one.

Comment 16

[Paul Morris [:pmorris]]

Attachment: part1-appmenu-folders-refactor-2.patch

This revision addresses the review comments.

Comment 17

[Magnus Melin [:mkmelin]]

(In reply to Paul Morris [:pmorris] from comment #15)

  if (excludeServers.length > 0) {

doesn't matter too much, but this check is a bit redundant

I've removed the redundant "> 0" part.

If you want to have the check, do keep the > 0. I was referring to the whole if statement. If there's no excludedServers, running the code inside won't do anything either....

Comment 18

[Magnus Melin [:mkmelin]]

(In reply to Paul Morris [:pmorris] from comment #14)

your other comments, rather than just copy the code and modify it, I'm
following DRY
principles

Not sure what the right thing to do here is, but as a general comment, abstracting things also has a quite significant cost in that it's harder to read/understand, and definitely more difficult to change later since you don't easily see the full scope of how some change will affect things at another place.

Comment 19

[:aceman]

Yes, you need to observe generated=true elements and only remove those on teardown.
I also prefer the " > 0" for numeric properties as otherwise it just looks as a test if '.length' exists in the object, regardless of the value.

Comment 20

[Magnus Melin [:mkmelin]]

Would be nicer to remember them by adding a class, but we can do that in some other bug. (I'd like o avoid having random custom attributes for stuff, and especially when they don't have any functionality.)

Comment 21

[Paul Morris [:pmorris]]

Attachment: part1-appmenu-folders-refactor-3.patch

Part 1 again. I thought it was worth keeping the if (excludedFolders.length > 0)
check, but restored the > 0 part.

Comment 22

[Paul Morris [:pmorris]]

(In reply to Magnus Melin [:mkmelin] from comment #12)

::: mailnews/base/content/folder-menupopup.js

 * Returns an element with the "generated" attribute set to "true".

not sure the generated attr is really needed for anything... (should be
needed for anything)

As discussed above, these are still needed. Turning it into a class rather than a custom attribute in another bug makes sense.

• const FolderMenuMixin = (Base) => class extends Base {

would use let or var for this

OK, I've switched these to let. While that's more conventional, I don't see any advantage to using let instead of const for things like this that we don't want to be reassigned. If things get accidentally reassigned, const will give you an error so you can fix it. With let you get no error and bugs can silently creep in. Preferring const also makes code easier to understand because you can see what can be reassigned and what won't.

One objection to const might be a concern about readers getting confused by expecting things bound to const variables to never change, like the properties of an object bound to a const variable, but that's not what const means in JS. It only means the variable can't be reassigned, not that any (sub-)properties can't change. I'd argue that avoiding const because of this potential confusion doesn't help clear up the confusion and just means you don't get to take advantage of one of JS's nice new features.

</soapbox>

@@ +295,4 @@

         return null;
       }
      const children = menu.appendTarget.childNodes;
      for (let i = 0; i < children.length; i++) {

could switch this over to

for (let child of menu.appendTarget.childNodes) {

Good call, and done.

@@ +513,5 @@

    submenu.appendTarget = submenu;
  } else {
    // We are in the appmenu and `this.localName == "panelview"`.
    const subViewId = getUniqueSubViewId("specialFoldersSubView");
    submenu.setAttribute("id", subViewId);

what are the ids used for?

Unlike traditional menus the appmenu uses a flat structure. All the <panelview>s (menus / views) are siblings, and they are shown by calling a function with their ID as an argument. So they each need a unique ID.

Further, elsewhere in this code the ID is a way to preserve the connection between a <toolbarbutton> and the <panelview> it opens. We store the panelview ID on the button so that we can access and tear down the submenu when the button is torn down.

@@ +728,2 @@

       // Grumble, grumble, legacy code support
       node.setAttribute("id", folder.URI);

I think this is wrong, like noted on another patch

Removed. aceman mentioned on IRC that addons may be the 'legacy code' that uses these URIs, and that maybe it could/should be stored in a different attribute on the element. I figured we can do something like that in a separate bug if it turns out it is needed.

@@ +1046,5 @@

  this.menuOrToolbarbuttonTagName = "menu";
  this.menuitemOrToolbarbuttonTagName = "menuitem";
  this.menupopupOrPanelviewTagName = "menupopup";
  this.menuitemOrToolbarbuttonClasses = "folderMenuItem menuitem-iconic";
  this.menuOrToolbarbuttonClasses = "folderMenuItem menu-iconic";

it's rather ugly to have all of these. I wonder if you could have a per
class function to generate needed nodes instead

I've reworked this now to provide much better separation of concerns. The shared code in the mixin class has the logic to determine what should be built/shown and then sends that off to functions like _buildMenuItem in the classes specific to folder-panelview and folder-menupopup that handle how to build/show what needs to be shown, for their respective menu types.

I think this is much nicer and easier to understand. The code paths basically move in one direction from the general shared code to the custom-element-specific code. Much better than using those properties and conditionals interspersed throughout.

@@ +1168,5 @@

 * @param {Element} menuItem  The menu item, typically a `toolbarbutton`.
 * @return {Element}  The popup, typically a `panelview` element.
 */
_getPopupForMenuItem(menuItem) {
  return document.getElementById(menuItem.getAttribute("subViewId"));

oh here the id is used I guess. But isn't it always a child you could just
query?

See above, the appmenu's panelviews are not children of the toolbarbuttons that activate them, so we use this function to find them by their ID. Used for tearing down the submenus when the buttons are torn down.

Comment 23

[Paul Morris [:pmorris]]

Attachment: part2-fix-appmenu-folders-2.patch

Here's the new version of part 2.

Comment 24

[Magnus Melin [:mkmelin]]

Comment on attachment 9073279 [details] [diff] [review]
part2-fix-appmenu-folders-2.patch

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

::: mailnews/base/content/folder-menupopup.js
@@ +1172,5 @@
> +     * Builds a menu item (`toolbarbutton`) element that does not open a submenu.
> +     *
> +     * In the appmenu the `command` and `oncommand` attributes need to be set on
> +     * `toolbarbutton` elements (not on `panelview` elements). So we store the
> +     * values in commandForChildren and oncommandForChildren on the parent

This is rather odd. Any way we can avoid these? I think usually what you do is have the command on the parent, and then let the event bubble up there to be handled there. Can't this work the same?

Comment 25

[Paul Morris [:pmorris]]

(In reply to Magnus Melin [:mkmelin] from comment #24)

 * In the appmenu the `command` and `oncommand` attributes need to be set on
 * `toolbarbutton` elements (not on `panelview` elements). So we store the
 * values in commandForChildren and oncommandForChildren on the parent

This is rather odd. Any way we can avoid these? I think usually what you do
is have the command on the parent, and then let the event bubble up there to
be handled there. Can't this work the same?

I agree it would be good to avoid this awkward shuffling of custom attributes. Here's the problem I ran into that led me to this approach. (I should have added more details about it to the comment)

If the parent <panelview> has a "command" or "oncommand" attribute, then when you click a menu item (<toolbarbutton>) that opens another <panelview> submenu, that click bubbles up and fires the command before we want it to be fired. In other words, some menu items should fire the command and some should not because they are only opening a submenu. (This is another case where the appmenu does not work like other menus.)

Now that I'm looking at this again, a cleaner and more idiomatic solution would be to just stop the event propagation when clicking menu items that open a submenu (and when clicking the back button to go back to the parent menu). I've got that working locally. I'll finish up the changes and upload a new patch.

Comment 26

[Paul Morris [:pmorris]]

Attachment: part2-fix-appmenu-folders-3.patch

This version uses the event.stopPropagation() approach (see previous comment).

Comment 27

[Paul Morris [:pmorris]]

Attachment: part3-add-favorites-submenu-1.patch

A part3 patch that applies on latest part2 patch. No substantive changes to the part3 patch.

Comment 28

[Magnus Melin [:mkmelin]]

Comment on attachment 9074211 [details] [diff] [review]
part2-fix-appmenu-folders-3.patch

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

LGTM! r=mkmelin

::: mailnews/base/content/folder-menupopup.js
@@ +934,5 @@
> +   * via the FolderMenuMixin function.
> +   *
> +   * @extends {MozElements.MozMenuPopup}
> +   */
> +  let MozFolderMenuPopup = FolderMenuMixin(class extends MozElements.MozMenuPopup {

actually, this should be "class MozFolderMenuPopup", right?

@@ +1100,5 @@
> +   * the FolderMenuMixin function.
> +   *
> +   * @extends {MozXULElement}
> +   */
> +  let MozFolderPanelView = FolderMenuMixin(class extends MozXULElement {

class?

Comment 29

[Jorg K (CEST = GMT+2)]

Looks like some tweaks are required here before check-in. Can I also have some beta patches please assuming that this is needed on beta.

Comment 30

[Paul Morris [:pmorris]]

(In reply to Magnus Melin [:mkmelin] from comment #28)

let MozFolderMenuPopup = FolderMenuMixin(class extends MozElements.MozMenuPopup {

actually, this should be "class MozFolderMenuPopup", right?

Well, no, I don't think that would work. So, here I'm using class expressions. I pass an anonymous class (that extends MozElements.MozMenuPopup) to the FolderMenuMixin function, which returns a class, and that gets bound to MozFolderMenuPopup with let.

The class returned by the FolderMenuMixin function extends the anonymous class that is passed into it. This basically lets you do inheritance in reverse, letting a single "mixin" class extend more than one super class.

@@ +1100,5 @@

let MozFolderPanelView = FolderMenuMixin(class extends MozXULElement {

class?

Same as above.

I'll rebase and upload new patches for trunk and 68 beta.

Comment 31

[Paul Morris [:pmorris]]

Attachment: part1-appmenu-folders-refactor-4.patch

1 of 3, rebased on current trunk.

Comment 32

[Paul Morris [:pmorris]]

Attachment: part2-fix-appmenu-folders-4.patch

2 of 3, rebased on current trunk.

Comment 33

[Paul Morris [:pmorris]]

Attachment: part3-add-favorites-submenu-2.patch

3 of 3, rebased on current trunk. Beta patches and a final try run coming up.

Comment 34

[Paul Morris [:pmorris]]

Attachment: beta-part1-appmenu-folders-refactor-0.patch

1 of 3 patches for beta.

Comment 35

[Paul Morris [:pmorris]]

Attachment: beta-part2-fix-appmenu-folders-0.patch

2 of 3 patches for beta.

Comment 36

[Paul Morris [:pmorris]]

Attachment: beta-part3-add-favorites-submenu-0.patch

3 of 3 patches for beta.

Comment 37

[Paul Morris [:pmorris]]

Try server runs. Ready for checkin assuming these look good.

Trunk: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f624956bc54e19f2cbe8ba4e090b0d292e1bbc22

Beta: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=15757422dc52844f1545852f39a15aeebcc1b492

Comment 38

[Paul Morris [:pmorris]]

Looks like some mozmill tests are failing. I'll take a look.

Comment 39

[Paul Morris [:pmorris]]

Attachment: part2-fix-appmenu-folders-5.patch

I've fixed the mozmill tests. It was test-folder-toolbar.js. Basically I refactored to kept the _setCssSelectors function which is used in commandglue.js which was where the test was failing.

Trunk: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c76b17081294d2435be034dcd100c1f05d86c33b
Beta: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a0f10fa706b98eae48c6f561c17a9f32d257d4cf

Uploading new patches parts 2 and 3 for trunk and beta. Part 1 patch is unchanged. (The new part 3 may not be strictly necessary, but uploading it too just for good measure.)

Patch 2 of 3 for trunk.

Comment 40

[Paul Morris [:pmorris]]

Attachment: part3-add-favorites-submenu-3.patch

Patch 3 of 3 for trunk.

Comment 41

[Paul Morris [:pmorris]]

Attachment: beta-part2-fix-appmenu-folders-1.patch

Patch 2 of 3 for beta.

Comment 42

[Paul Morris [:pmorris]]

Attachment: beta-part3-add-favorites-submenu-1.patch

Patch 3 of 3 for beta.

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/50955fe1cb98
Refactor the folder-menupopup code; r=mkmelin
https://hg.mozilla.org/comm-central/rev/71b2391d6c43
Fix accounts and folders submenus in the new appmenu. r=mkmelin
https://hg.mozilla.org/comm-central/rev/ca1b8ae86f9b
Add a 'favorites' submenu to appmenu > go > folder. r=mkmelin

Comment 44

[Jorg K (CEST = GMT+2)]

Comment on attachment 9074518 [details] [diff] [review]
beta-part1-appmenu-folders-refactor-0.patch

Approval for all beta parts.

Comment 45

[Jorg K (CEST = GMT+2)]

TB 68 beta 3:
https://hg.mozilla.org/releases/comm-beta/rev/3e087603171255ca56e4e4fd6b8d57d0a597ba7e
https://hg.mozilla.org/releases/comm-beta/rev/2edd36c7fe2f63d1e00a00959f7fec6d596f3922
https://hg.mozilla.org/releases/comm-beta/rev/48502126313a5b7d13211dbffd1e410aa41579d9