(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 @@ > > + 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 Done. Will upload the new patch and then move on to the next one.
Bug 1558565 Comment 15 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
(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.