:aceman
	Comment 7 • 5 years ago

Comment on attachment 9066723 [details] [diff] [review]
Bug-1542715_de-xbl_folder-menupopup.patch

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

Thanks.

::: mail/base/content/FilterListDialog.xul
@@ +27,4 @@
>    <script src="chrome://messenger/content/searchWidgets.js"/>
>    <script src="chrome://messenger/content/FilterListDialog.js"/>
>    <script src="chrome://messenger/content/customElements.js"/>
> +  <script src="chrome://messenger/content/folder-menupopup.js"/>

Do we have any rule for this, which custom elements need to be imported like this, and which go into customElements.js?

@@ -89,2 @@
>                           class="menulist-menupopup"
> -                         type="folder"

I always forget why this churn is needed. Why is type="folder" worse than is="folder-menupopup"? Is There some guideline that all our custom elements should have the 'is' attribute?

::: mailnews/base/content/folder-menupopup.js
@@ +26,5 @@
> +  connectedCallback() {
> +    if (this.delayConnectedCallback()) {
> +      return;
> +    }
> +    this.setAttribute("is", "folder-menupopup");

Why? Isn't this attrib already on the element, otherwise this whole CE wouldn't apply?

@@ +32,5 @@
> +
> +    /**
> +     * - In order to improve performance, we're not going to build any of the
> +     * - menu until we're shown (unless we're the child of a menulist, see
> +     * - note in the constructor).

There is no such note in the constructor.

@@ +269,5 @@
> +    /**
> +     * - True if we have already built our menu-items and are now just
> +     * - listening for changes.
> +     */
> +    this._initialized = false;

Can this be in the contructor? And all the other variables that were in <field> before in XBL.

	Magnus Melin [:mkmelin] Reporter
	Comment 10 • 5 years ago

Comment on attachment 9066723 [details] [diff] [review]
Bug-1542715_de-xbl_folder-menupopup.patch

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

Please also hg mv mailnews/base/content/folderWidgets.xml mailnews/base/content/folder-menupopup.js to preserve some history.

::: mail/base/content/FilterListDialog.xul
@@ +27,4 @@
>    <script src="chrome://messenger/content/searchWidgets.js"/>
>    <script src="chrome://messenger/content/FilterListDialog.js"/>
>    <script src="chrome://messenger/content/customElements.js"/>
> +  <script src="chrome://messenger/content/folder-menupopup.js"/>

this is used almost globally so please just inlude it in customElements.js

::: mail/base/content/mainMailToolbox.inc.xul
@@ +21,5 @@
>                     tooltiptext="&getMsgButton.tooltip;"
>                     oncommand="MsgGetMessagesForAccount();"
>                     observes="button_getNewMessages">
> +      <menupopup is="folder-menupopup"
> +                 id="button-getMsgPopup"

please have is and id on same line for easier greppability ("is" is bascially part of the element definition)

::: mailnews/base/content/folder-menupopup.js
@@ +9,5 @@
> +/**
> + * The MozFolderMenupopup widget is used as a menupopup for selecting
> + * a folder from the list of all the folders from every account. It is also
> + * used for selecting the account from the list of all the accounts. The each
> + * menuitem gets displayed with the folder or account name and icon.

@extends MozElements.MozMenuPopup

can you also document the attributes to configure it?

@@ +42,5 @@
> +      this._ensureInitialized();
> +    }, true);
> +
> +    /**
> +     * - Various filtering modes can be used with this menu-binding.  To use

double spaces,  and also remove the - from the start of the lines

@@ +50,5 @@
> +     * -
> +     * - Note that extensions should feel free to plug in here!
> +     */
> +    this._filters = {
> +      // Returns true if messages can be filed in the folder

please move these to be documentation comments as /** */

also add the ending period where missing.

@@ +139,5 @@
> +     */
> +    this._MAXRECENT = 15;
> +
> +    /**
> +     * - Is this list containing only servers (accounts) and no real folders?

remove -

@@ +145,5 @@
> +    this._serversOnly = true;
> +
> +    /**
> +     * - Our listener to let us know when folders change/appear/disappear so
> +     * - we can know to rebuild ourselves.

@implements {nsIFolderListener}

@@ +279,5 @@
> +    this._initializedSpecials = new Set();
> +
> +    this._displayformat = null;
> +
> +    ChromeUtils.import("resource:///modules/FeedUtils.jsm", this);

please convert these over to the newer style, like
const {FeedUtils} =  ChromeUtils.import("resource:///modules/FeedUtils.jsm");

@@ +434,5 @@
> +        return !excludeServers.includes(aFolder.server.key);
> +      });
> +    }
> +
> +    /* This code block will do the following: Add a menu item that refers

move to // style comment

@@ +520,5 @@
> +      // comparison of names.
> +      folders = folders.sort((a, b) => a.compareSortKeys(b));
> +    }
> +
> +    /* In some cases, the user wants to have a list of subfolders for only

here too //

@@ +755,5 @@
> +    this._initializedSpecials.add(specialType);
> +  }
> +
> +  /**
> +   * - This function adds attributes on menu/menuitems to make it easier for

more of - removal. Like other comments, I'll not mention is more now. Just check for all occurrences.

@@ +912,5 @@
> +    this._teardown();
> +  }
> +}
> +
> +customElements.define("folder-menupopup", MozFolderMenupopup, { "extends": "menupopup" });

please also add the "Wrap in a block to prevent leaking to window scope." wrapper for the whole thing

	Magnus Melin [:mkmelin] Reporter
	Comment 13 • 5 years ago

Comment on attachment 9067448 [details] [diff] [review]
Bug-1542715_dexbl_folder-menupopup.patch

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

Did you do any try run so far?

::: mailnews/base/content/folder-menupopup.js
@@ +37,2 @@
>  
> +      this.setAttribute("is", "folder-menupopup");

we should not set attributes in the constructor (it's basically forbidden), do it in connectedCallback

@@ +47,5 @@
> +       * Note that extensions should feel free to plug in here!
> +       */
> +      this._filters = {
> +        /**
> +         * Returns true if messages can be filed in the folder

add period at the end of the sentence.

@@ +244,5 @@
>                return;
> +            }
> +            /**
> +             * if this folder is already in the recent menu, return.
> +             */

Should be // style comment, and proper sentence with capitalization.

@@ +253,5 @@
> +          } else if (event == "RenameCompleted") {
> +            /**
> +             * Special casing folder renames here, since they require more work
> +             * since sort-order may have changed.
> +             */

this too //

@@ +262,5 @@
>              return;
>            }
> +          /**
> +           * folder renamed, or new recent folder, so rebuild.
> +           */

and this - they are not documentation comments

@@ +273,2 @@
>           *
> +         * @param item  the nsIMsgFolder to check.

@param {nsIMsgFolder} item - the folder to check

@@ +273,3 @@
>           *
> +         * @param item  the nsIMsgFolder to check.
> +         * @param menu  (optional) menu to look in, defaults to this._menu.

@param [menu] - the menu to look in, defaults to this._menu

@@ +278,3 @@
>           */
> +        _getChildForItem(item, menu) {
> +          menu = menu || this._menu;

should probably make this 
let menu || menu || this._menu;

Otherwise eslint no-shadow will complain in the future

@@ +319,5 @@
> +       * menu until we're shown.
> +       *
> +       * @note _ensureInitialized can be called repeatedly without issue, so
> +       *        don't worry about it here.
> +       */

not a documentation comment, so // style please

There's a bunch of these in the patch. Please change all the ones that are not on functions

@@ +322,5 @@
> +       *        don't worry about it here.
> +       */
> +      this.addEventListener("popupshowing", (event) => {
> +        this._ensureInitialized();
> +      }, true);

this must be protected from being run more than once

@@ +407,2 @@
>  
> +      MailServices.mailSession.RemoveFolderListener(this._listener);

to get this removed on window close, may have to add a one time unload listener for it, like https://searchfox.org/comm-central/rev/de2838e06261177a192b6bb6fbd628817e124b3b/mail/components/addrbook/content/menulist-addrbooks.js#86

@@ +676,5 @@
>  
> +          /**
> +           * Create the submenu.
> +           */
> +          let popup = document.createElement("menupopup", { "is": "folder-menupopup" });

createXULElement
(or we crash, so I can't test this patch on tip)

	Magnus Melin [:mkmelin] Reporter
	Comment 17 • 5 years ago

Comment on attachment 9068168 [details] [diff] [review]
Bug-1542715_dexbl_folder-menupopup.patch

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

Seems to work, but hard to test extensively since we use this all over the place. 
Make sure you get a green try run.

::: mailnews/base/content/folder-menupopup.js
@@ +653,5 @@
>  
> +      let popup = document.createXULElement("menupopup");
> +      popup.setAttribute("class", this.getAttribute("class"));
> +      popup.addEventListener("popupshowing",
> +        () => this._buildSpecialSubmenu(menu), { once: true });

for things like these, format it like
popup.addEventListener("popupshowing", (event) => {
  this._buildSpecialSubmenu(menu)
}, { once: true });

	Magnus Melin [:mkmelin] Reporter
	Comment 26 • 5 years ago

Comment on attachment 9069717 [details] [diff] [review]
Bug-1542715_dexbl_folder-menupopup.patch

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

LGTM, r=mkmelin

::: mailnews/extensions/newsblog/content/feed-subscriptions.xul
@@ +125,5 @@
>                          flex="1"
>                          class="folderMenuItem"
>                          hidden="true">
> +                <menupopup is="folder-menupopup"
> +                           id="selectFolderPopup"

nit: move the id to the first line

	Jorg K (CEST = GMT+2)
	Comment 33 • 5 years ago

Comment on attachment 9070197 [details] [diff] [review]
Bug-1542715_dexbl_folder-menupopup.patch

OK, let's take it then.

	:aceman
	Comment 35 • 5 years ago

Comment on attachment 9070197 [details] [diff] [review]
Bug-1542715_dexbl_folder-menupopup.patch

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

::: mail/test/mozmill/folder-widget/test-message-filters.js
@@ +74,4 @@
>  
>    // This one initializes the menuitems, but it's kinda hacky.
>    nntp.node.menupopup._ensureInitialized();
> +  assert_equals(nntp.node.itemCount, 3,

Why did you change this? The folder picker conversion shouldn't have caused changes in behaviour.

Looks like it caused a regression and you can no longer pick the news server itself in the Filter list dialog so you can't define filters on it. This test uncovered this regression but you silenced it without explanation.