1542715 - [de-xbl] remove the folder-menupopup binding - convert to <menupopup is="folder-menupopup">

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Magnus Melin [:mkmelin]]

The folder-menupopup binding can converted to a customized built-in <menupopup is="folder-menupopup">.

(The popup is used both under menu and menulist, so can't easily be a customized menulist.)

See bug 1531296 for some samples.

Comment 1

[Magnus Melin [:mkmelin]]

As noticed in bug 1545854, the menulists for folderLocationPopup and such (in the customize toolbar) are now rather messed up. This bug should resolve that.

Comment 2

[:aceman]

FolderWidgets.xml? I have a ton of patches pending there, but let's do it if it is needed (forward-thinking?).
However, chrome://global/content/bindings/popup.xml#popup that folder-menupopup extends, still exists.

Comment 3

[Magnus Melin [:mkmelin]]

popup.xml#popup still exists yes - but conversion of consumers is happening one at a time and I suspect that as such that element won't be directly converted ever (especially since it's deprecated). Bug 1531870 added the new base for menupopups.

Comment 4

[Paul Morris [:pmorris]]

Over in bug 1546309 the appmenu uses this binding in four places (file->getNewMessagesFor, go->folder, message->moveTo, message->copyTo). It would be best if the XBL conversion were done before adapting this folder menu type for use in the appmenu, so I'm marking this as blocking bug 1546309.

Comment 6

[Khushil Mistry [:khushil324]]

Attachment: Bug-1542715_de-xbl_folder-menupopup.patch

Comment 7

[:aceman]

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.

Comment 8

[:aceman]

This will need changes in addons, whether just for needing to import customElements.js, or the changed is= attribute.

Comment 9

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #7)

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

No strict rules. If it's only needed somewhere locally, import it only there and avoid the slight overhead.

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?

"is" is what makes a customized built-in of an element. That is, extends an existing element.

• this.setAttribute("is", "folder-menupopup");

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

When you create a customized built-in through code, the "is" attribute is not automatically there. It's good practice to add it as an attribute for that reason.

Comment 10

[Magnus Melin [:mkmelin]]

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

Comment 11

[Khushil Mistry [:khushil324]]

Attachment: Bug-1542715_dexbl_folder-menupopup.patch

Comment 12

[Khushil Mistry [:khushil324]]

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

::: 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

I have added it in the customElements.js but I some of the window and dialogues are not importing the customElements.js so I have imported folder-menupopup.js on those windows to avoid unnecessary load on those windows. Should I replace/import customElements.js in those windows ?

::: 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?

We have documentation about the attributes through out the code. When we are using some attribute at some place, there is decent amount of documentation of the attribute at the point of usage. So I don't find any need to add it at top.

Comment 13

[Magnus Melin [:mkmelin]]

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)

Comment 14

[:aceman]

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

@@ +278,3 @@

      */

•    _getChildForItem(item, menu) {
  

•      menu = menu || this._menu;
  

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

What did you mean here? Is this the right syntax? Also, is it bad to change the value of an in-argument (we do it in various places) ? If so, then it should be some new name like 'let realMenu = menu || this.menu;' .

@@ +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

Why? _ensureInitialized checks that internally. Also, we want to run _ensureInitialized on each "popupshowing", not just the first one (the menuitems may get destroyed on popup close).

@@ +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

Why? Are custom elements really so crappy to not call disconnectedCallback() (or some other destructor) on window close?

Comment 15

[Khushil Mistry [:khushil324]]

Attachment: Bug-1542715_dexbl_folder-menupopup.patch

Comment 16

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #14)

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

@@ +278,3 @@

      */

•    _getChildForItem(item, menu) {
  

•      menu = menu || this._menu;
  

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

What did you mean here? Is this the right syntax? Also, is it bad to change
the value of an in-argument (we do it in various places) ?

I meant
let menu = menu || this._menu;

(this way we do not change the value of the in-argument)

@@ +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

Why? _ensureInitialized checks that internally.

Yes but we only want to have one event listener to call it, not many.

Why? Are custom elements really so crappy to not call disconnectedCallback()
(or some other destructor) on window close?

disconnectedCallback doesn't run for window close. That's not a problem for normal web event handler, they just go away when the dom object dies. Seems it is a problem for mozilla XPCOM services though.

Comment 17

[Magnus Melin [:mkmelin]]

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 });

Comment 18

[:aceman]

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

What did you mean here? Is this the right syntax? Also, is it bad to change
the value of an in-argument (we do it in various places) ?

I meant
let menu = menu || this._menu;

Is that correct to re-declare the argument?

(this way we do not change the value of the in-argument)

@@ +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
Yes but we only want to have one event listener to call it, not many.

Ok, if the contructor can be called multiple times for the same element. Looks like it can, per bug 1552666, at least with XBL. I do not see this handled in the latest patch. The same for the "unload" event listener.
Will disconectCallback be called before the constructor is run again? Should be easy to check as we construct this twie just by starting TB with having the folder location widget on the toolbar.

Comment 19

[Magnus Melin [:mkmelin]]

Is that correct to re-declare the argument?

Sure, that is listed as a correct example for eslint no-param-reassign: https://eslint.org/docs/rules/no-param-reassign

A constructor is only ever called once.

Comment 20

[:aceman]

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

Is that correct to re-declare the argument?

Sure, that is listed as a correct example for eslint no-param-reassign: https://eslint.org/docs/rules/no-param-reassign

There is no such example on that page.

A constructor is only ever called once.

OK, it seems so. connectCallback is run multiple times. Let's hope this stays so. For XBL, constructor can be run repeatedly, which has changed recently.

Comment 21

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #20)

There is no such example on that page.

There is, the very first one:
function foo(bar) {
var baz = bar;
}

Comment 22

[:aceman]

That one does not re-declare the incoming 'bar' argument.
Your suggestion was:
_getChildForItem(item, menu) {
let menu = menu || this._menu;

Comment 23

[Magnus Melin [:mkmelin]]

Ah, yes. Yeah use a new name

Comment 24

[:aceman]

OK :)

Comment 25

[Khushil Mistry [:khushil324]]

Attachment: Bug-1542715_dexbl_folder-menupopup.patch

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

Comment 26

[Magnus Melin [:mkmelin]]

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

Comment 27

[Magnus Melin [:mkmelin]]

Didn't find anything wrong, but in the console there was one
JavaScript error: chrome://messenger/content/folder-menupopup.js, line 913: NotSupportedError: Operation is not supported

which is the customElements.define(....) line.
Can't reproduce it though.

Related to that I see "extends" with quotes. Not an error, but please remove those.

Comment 28

[Khushil Mistry [:khushil324]]

Attachment: Bug-1542715_de-xbl_folder-menupopup.patch

Comment 29

[Khushil Mistry [:khushil324]]

Attachment: Bug-1542715_dexbl_folder-menupopup.patch

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b492a5b976af
[de-xbl] convert folder-menupopup binding to custom element <menupopup is='folder-menupopup'>. r=mkmelin

Comment 31

[Jorg K (CEST = GMT+2)]

Does this need to go to TB 68? It's blocking bug 1546309 but I don't see the connection.

Comment 32

[Magnus Melin [:mkmelin]]

It does since the folder popup in the new app-menu is hard to make work otherwise.

Comment 33

[Jorg K (CEST = GMT+2)]

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

OK, let's take it then.

Comment 34

[Jorg K (CEST = GMT+2)]

TB 68 beta:
https://hg.mozilla.org/releases/comm-beta/rev/4258ce73811779d5c42f621afb8746f6cba3880a

So much for the most aggressive uplift program ever ;-)

Comment 35

[:aceman]

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.