1513836 - [de-xbl] Remove search-menulist-abstract, searchoperator and search attribute bindings and related code.

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, task)

Comment 1

[Arshad Khan [:arshad]]

Attachment: search-menulist-bindings.patch

Comment 2

[Arshad Khan [:arshad]]

Attachment: search-menulist-bindings.WIP.patch

I am still stuck at this patch, not sure why custom elements are not loading, when i tried to put it in searchTermOverlay.js then it was loaded into scope, but then I found another issue of racing connectedcallback and disconnectedCallback method.. Pausing the work on this..

Comment 3

[Arshad Khan [:arshad]]

Attachment: search-menulist-bindings.patch

Comment 4

[Arshad Khan [:arshad]]

Comment on attachment 9046149 [details] [diff] [review]
search-menulist-bindings.patch

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

These bindings are only used in seamonkey - https://searchfox.org/comm-central/search?q=searchTermOverlay.xul&path=

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9046149 [details] [diff] [review]
search-menulist-bindings.patch

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

::: mailnews/jar.mn
@@ -95,5 @@
>      content/messenger/junkLog.js                                               (base/content/junkLog.js)
>      content/messenger/jsTreeView.js                                            (base/content/jsTreeView.js)
>  #ifndef MOZ_THUNDERBIRD
>      content/messenger/searchTermOverlay.js                                     (base/search/content/searchTermOverlay.js)
> -    content/messenger/searchTermOverlay.xul                                    (base/search/content/searchTermOverlay.xul)

Though this is now broken right, left, and center, perhaps we should move it to suite/
Could you file another bug to move all the #ifndef MOZ_THUNDERBIRD files to suite?

Comment 6

[Magnus Melin [:mkmelin]]

Actually, I'll do the bug to move these.

Comment 7

[Magnus Melin [:mkmelin]]

Bug 1530171

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9046149 [details] [diff] [review]
search-menulist-bindings.patch

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

Please update now that the removal bug landed.

Also, this patch doesn't remove the bindings. If they can be removed, remove them and also all referenced code (like .properties file entries used only in this)

Comment 9

[Arshad Khan [:arshad]]

Attachment: search-menulist-bindings.patch

Comment 10

[Arshad Khan [:arshad]]

I didn't find any entity or value specific to searchattribute/searchoperator.

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9047860 [details] [diff] [review]
search-menulist-bindings.patch

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

Seems it is used somewhere, though I'm too confused exactly where since grepping doesn't find what I'm looking for. 
Anyway, open the Filter Editor and try to add a new filter - you'll see that where it used to say [Subject] [contains] [ ] 
... there is now just a wide box. Please debug that and find out where this is all set up. Perhaps it's building the name dynamically or some such... fix that too once you find it.

Comment 12

[Arshad Khan [:arshad]]

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

Comment on attachment 9047860 [details] [diff] [review]
search-menulist-bindings.patch

Review of attachment 9047860 [details] [diff] [review]:

Seems it is used somewhere, though I'm too confused exactly where since
grepping doesn't find what I'm looking for.
Anyway, open the Filter Editor and try to add a new filter - you'll see that
where it used to say [Subject] [contains] [ ]
... there is now just a wide box. Please debug that and find out where this
is all set up. Perhaps it's building the name dynamically or some such...
fix that too once you find it.

https://searchfox.org/comm-central/source/mailnews/base/search/content/searchTerm.js#322

Comment 13

[Arshad Khan [:arshad]]

Attachment: search-menulist-bindings.patch

Comment 14

[Arshad Khan [:arshad]]

Attachment: search-menulist-bindings.patch

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 9050000 [details] [diff] [review]
search-menulist-bindings.patch

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

Please update the commit message to say this is converting the x y x bindings to CE.

Most of the below is issues from the original code, but let's still improve it now while converting.

::: mailnews/base/search/content/searchWidgets.js
@@ +4,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  /* global MozXULElement, gFilter, gFilterList */
>  
> +var { MailUtils } = ChromeUtils.import("resource:///modules/MailUtils.jsm");

I think no space after/before is more common

@@ +86,5 @@
>  
>      this.appendChild(menulist);
>  
>      document.getAnonymousElementByAttribute(this.closest(".ruleaction"), "class", "ruleactiontype")
> +      .getTemplates(true, menulist);

lining up rows with dots is actually a style we recommend atm, so at least don't change it now.

@@ +216,5 @@
>  customElements.define("ruleactiontarget-wrapper", MozRuleactiontargetWrapper);
> +
> +/**
> + * This class contains methods and properties, that are common to searchoperator and searchattribute
> + * custom elements.

This is an abstract class for search menulist general functionality.

Also add @abstract

@@ +232,5 @@
> +    this.internalValue = -1;
> +  }
> +
> +  connectedCallback() {
> +    if(!this.hasChildNodes()) {

nit: space after if
this is in a few other places too. 
make sure ./mach lint comm/mailnews/base/search/content/searchWidgets.js doesn't complain about anything

@@ +272,5 @@
> +  }
> +
> +  set searchScope(val) {
> +    // if scope isn't changing this is a noop
> +    if (this.internalScope == val) return val;

make this three lines, and add braces

@@ +299,5 @@
> +  }
> +
> +  get targets() {
> +    const forAttrs = this.getAttribute("for");
> +    if (!forAttrs) return null;

to 3 lines

@@ +301,5 @@
> +  get targets() {
> +    const forAttrs = this.getAttribute("for");
> +    if (!forAttrs) return null;
> +    const targetIds = forAttrs.split(",");
> +    if (targetIds.length == 0) return null;

as well as this

@@ +305,5 @@
> +    if (targetIds.length == 0) return null;
> +    const targets = [];
> +    for (let j = 0, i = 0; i < targetIds.length; i++) {
> +      const target = document.getElementById(targetIds[i]);
> +      if (target) targets[j++] = target;

maybe make it  let i = 0, j = 0 (the order threw me off)
Or rather, just do targets.push(target);

OR, even better, map them. Something like

const targets = targetIds.map(id => document.getElementById(id)).filter(e => e != null);

@@ +316,5 @@
> +    if (!forAttrs) return null;
> +    const optargetIds = forAttrs.split(",");
> +    if (optargetIds.length == 0) return null;
> +    const optargets = [];
> +    let j = 0;

and same comment for this and elsewhere

@@ +351,5 @@
> +    return this.internalValue;
> +  }
> +
> +  /**
> +   * label forwards to the internal menulist's "label" attribute

Get's the label of the menulist's selected item.

@@ +388,5 @@
> +    if (!dontRestore)
> +      oldData = this.menulist.value;
> +    // remove the old popup children
> +    while (popup.hasChildNodes())
> +      popup.lastChild.remove();

add braces

@@ +391,5 @@
> +    while (popup.hasChildNodes())
> +      popup.lastChild.remove();
> +    let newSelection;
> +    let customizePos = -1;
> +    for (let i = 0; i < menuItemIds.length; ++i) {

i++ (is more common)

@@ +417,5 @@
> +    //
> +    // If we are either uninitialized, or if we are called because
> +    // of a change in our parent, update the value to the
> +    // default stored in newSelection.
> +    //

remove the two extra // lines

@@ +455,5 @@
> +}
> +
> +/**
> + * MozSearchattribute contains a list of possible message headers. It also have an option to create
> + * custom message header.

The MozSearchattribute widget is typically used in the search and filter dialogs to show a list of possible message headers.

@@ +467,5 @@
> +    initializeTermFromId(this.id);
> +  }
> +
> +  get stringBundle() {
> +    return Services.strings.createBundle("chrome://messenger/locale/search-attributes.properties");

you shouldn't create this each get
cache it instead and use the cached object if available

@@ +483,5 @@
> +        .createInstance(Ci.nsIScriptError);
> +      scriptError.init("Missing custom search term " + this.value,
> +        null, null, 0, 0,
> +        Ci.nsIScriptError.errorFlag,
> +        "component javascript");

funny indention. could all fit on two lines I think

@@ +492,5 @@
> +      this.validityManager.getAttributeProperty(parseInt(this.value)));
> +  }
> +
> +  get valueIds() {
> +    const length = {};

you can just pass in {} instead of naming this unused variable

@@ +499,5 @@
> +    let customEnum = MailServices.filters.getCustomTerms();
> +    while (customEnum && customEnum.hasMoreElements()) {
> +      let customTerm =
> +        customEnum.getNext()
> +          .QueryInterface(Ci.nsIMsgSearchCustomTerm);

these three lines fit on one, right?

@@ +574,5 @@
> +  get valueStrings() {
> +    let strings = [];
> +    let ids = this.valueIds;
> +    for (let i = 0; i < ids.length; i++)
> +      strings[i] = this.stringBundle.GetStringFromID(ids[i]);

add braces for all loops please

@@ +579,5 @@
> +    return strings;
> +  }
> +
> +  set parentValue(val) {
> +    if (this.searchAttribute == val && val != Ci.nsMsgSearchAttrib.OtherHeader) return val;

-> 3 lines

Comment 16

[Arshad Khan [:arshad]]

Attachment: search-menulist-bindings.patch

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 9050599 [details] [diff] [review]
search-menulist-bindings.patch

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

::: mailnews/base/search/content/searchWidgets.js
@@ +304,5 @@
> +    const targetIds = forAttrs.split(",");
> +    if (targetIds.length == 0) {
> +      return null;
> +    }
> +    

nit: trailing whitespace. 
did you check ./mach lint comm/mailnews/base/search/content/searchWidgets.js

@@ +380,5 @@
> +    const popup = this.menupopup;
> +    // save our old "value" so we can restore it later
> +    let oldData;
> +    if (!dontRestore) {
> +        oldData = this.menulist.value;

indention is off

@@ +384,5 @@
> +        oldData = this.menulist.value;
> +    }
> +    // remove the old popup children
> +    while (popup.hasChildNodes()) {
> +        popup.lastChild.remove();

indention is off

@@ +452,5 @@
> +  }
> +}
> +
> +/**
> + * The MozSearchattribute widget is typically used in the search and filter dialogs to show a list 

trailing whitespace

@@ +495,5 @@
> +    let result = this.validityTable.getAvailableAttributes({});
> +    // add any available custom search terms
> +    let customEnum = MailServices.filters.getCustomTerms();
> +    while (customEnum && customEnum.hasMoreElements()) {
> +      let customTerm = customEnum.getNext().QueryInterface(Ci.nsIMsgSearchCustomTerm);

This should be able to just be

for (let customTerm of MailServices.filters.getCustomTerms()) {

}

@@ +549,5 @@
> +    this.searchAttribute = Ci.nsMsgSearchAttrib.Default;
> +  }
> +
> +  get stringBundle() {
> +    return Services.strings.createBundle("chrome://messenger/locale/search-operators.properties");

shouldn't get a new instance for each get. 
I think I commented on this before

Comment 18

[Arshad Khan [:arshad]]

eslitn doesn't pickup this file.. there is not eslintrc file perhaps for file's parent directory.

Comment 19

[Arshad Khan [:arshad]]

Attachment: search-menulist-bindings.patch

Comment 20

[Arshad Khan [:arshad]]

Attachment: search-menulist-bindings.patch

Comment 21

[Arshad Khan [:arshad]]

Attachment: search-menulist-bindings.patch

Comment 22

[Arshad Khan [:arshad]]

Try - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=7107baeccf3bdfd6a3e542028d81d6169e7f9b10

Comment 23

[Magnus Melin [:mkmelin]]

Comment on attachment 9050659 [details] [diff] [review]
search-menulist-bindings.patch

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

Lots of tests failing

::: mailnews/base/search/content/searchWidgets.js
@@ +458,5 @@
> +class MozSearchattribute extends MozSearchMenulistAbstract {
> +  constructor() {
> +    super();
> +
> +    this.stringBundle = 

trailing ws

@@ +466,5 @@
> +  connectedCallback() {
> +    super.connectedCallback();
> +
> +    initializeTermFromId(this.id);
> +    this.stringBundle = 

and here.

But you should hardly do this in both the constructor *and* the connectedCallback()

@@ +537,5 @@
> +customElements.define("searchattribute", MozSearchattribute);
> +
> +/**
> + * MozSearchoperator contains a list of operators that can be applied on searchattribute's and
> + * searchvalue's value.

nit: should be no apos in these. it's attributes, and values

@@ +545,5 @@
> +class MozSearchoperator extends MozSearchMenulistAbstract {
> +  constructor() {
> +    super();
> +
> +    this.stringBundle = 

trailing whitespace

@@ +599,5 @@
> +    return this.searchAttribute;
> +  }
> +}
> +
> +customElements.define("searchoperator", MozSearchoperator);

please remove the empty line befor define. same thing above

Comment 24

[Arshad Khan [:arshad]]

Attachment: search-menulist-bindings.patch

I suppose that the tests that are failing are not because of this patch but some other stuff?

Comment 25

[Arshad Khan [:arshad]]

Lest see another try run - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=72e430c689725a4912c71fc932bf4b632a217694

Comment 26

[Magnus Melin [:mkmelin]]

Oh those test failures look very related to this patch.

Can you also update it to be search-operator, and MozSearchOperator

Comment 27

[Arshad Khan [:arshad]]

Attachment: search-menulist-bindings.patch

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

Comment 28

[Arshad Khan [:arshad]]

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

Comment 29

[Arshad Khan [:arshad]]

Attachment: search-menulist-bindings.patch

I have fixed z4 test failures but z2, and z3 test failures doesn't seem to have any clear connection with searchoperator or searchattribute element. Could you please take a look, I cant figure out what's wrong?

Comment 30

[Arshad Khan [:arshad]]

Attachment: search-menulist-bindings.patch

Tests are fixed now. https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=98a35a701a2fae78fdb34ba6e67b59ccb0447533

Comment 31

[Arshad Khan [:arshad]]

another test run - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f139465151b6635d0c8af9fc2e5291766448ec32

Comment 32

[Magnus Melin [:mkmelin]]

Comment on attachment 9056440 [details] [diff] [review]
search-menulist-bindings.patch

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

::: mailnews/base/search/content/searchWidgets.js
@@ +240,5 @@
> +    }
> +    this._updateAttributes();
> +  }
> +
> +  addChildren() {

can just inline this method, I think

@@ +325,5 @@
> +    }
> +    this.internalValue = val;
> +    this.menulist.selectedItem = this.validMenuitem;
> +    // now notify targets of new parent's value
> +    const targets = this.targets;

you don't need this tmp var

@@ +330,5 @@
> +    if (targets) {
> +      targets.forEach(target => (target.parentValue = val));
> +    }
> +    // now notify optargets of new op parent's value
> +    const optargets = this.optargets;

nor this

@@ +369,5 @@
> +
> +  refreshList(dontRestore) {
> +    if (!this.hasChildNodes()) {
> +      this.addChildren();
> +    }

this doesn't look like it's something needed.

Comment 33

[Arshad Khan [:arshad]]

Attachment: search-menulist-bindings.patch

Comment 34

[Arshad Khan [:arshad]]

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

Comment 35

[Magnus Melin [:mkmelin]]

Comment on attachment 9056731 [details] [diff] [review]
search-menulist-bindings.patch

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

Looks alright. r=mkmelin

Comment 36

[Magnus Melin [:mkmelin]]

The mozmill/content-tabs/test-about-support.js failure looks unrealted (which is reasonably should), and that test works locally for me.

Comment 37

[Jorg K (CEST = GMT+2)]

There is no test failure if you refresh.

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/377b76fdbaa0
Migrate search-menulist-abstract, searchoperator and searchattribute bindings to custom element. r=mkmelin