879953 - Remove Lookup methods and Xpath elements from lib/toolbars.js

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Jonathan French (:jfrench)]

Split off from bug 503192, this is a supplementary bug to remove and replace any elementslib.Lookup methods and Xpath elements from lib/toolbars.js to make the code base less fragile. We would like to replace Lookup with ID, Selector, or if neither work, Class via NodeCollector.

An example of this approach can be found here in the getElements() method in lib/addons.js
http://hg.mozilla.org/qa/mozmill-tests/file/b8a22c8c9a3b/lib/addons.js#l1026

Comment 1

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v1.0

Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/bccc2ea9ac1e478fa074c699d8db1503
http://mozmill-crowd.blargon7.com/#/functional/report/bccc2ea9ac1e478fa074c699d8da7468
http://mozmill-crowd.blargon7.com/#/functional/report/bccc2ea9ac1e478fa074c699d8da68f7

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Andreea is on PTO so you might want to ask for review from someone else.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 787448 [details] [diff] [review]
patch_v1.0

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

Sorry for taking so long. In general that looks fantastic and I cannot await to see this code landed and all the lookup strings going away. But we might have some more iterations on it. Please see my inline comments for details.

::: lib/toolbars.js
@@ +166,5 @@
>     */
> +  getElement : function autoCompleteResults_getElement(aSpec) {
> +    var elements = this.getElements(aSpec);
> +
> +    var index = (aSpec.subtype && aSpec.subtype === "index") ? aSpec.value : 0;

Please don't add specific code into that method. It's only a dumb wrapper which returns the first item. Instead handle it correctly in getElements. See my comment below.

@@ +209,4 @@
>          break;
>        case "result":
> +        nodeCollector.root = this.getElement({type: "results"}).getNode();
> +        nodeCollector.queryNodes("richlistitem");

This has to return the specific item and not a list of items.

@@ +227,5 @@
>     * @returns Autocomplete result element
>     * @type {ElemBase}
>     */
>    getResult : function autoCompleteResults_getResult(index) {
> +    return this.getElement({type: "result", subtype: "index", value: index});

Keep the line as it was.

@@ +564,2 @@
>        case "contextMenu":
> +        nodeCollector.root = this.getElement({type: "urlbar_input"}).getNode().parentNode;

I would use 'nodeCollector.root = this.getElement({type: "urlbar"}).getNode();' here.

@@ +597,5 @@
> +              nodeCollector.queryAnonymousNode("anonid", "button");
> +              break;
> +            default:
> +              throw new Error(arguments.callee.name + ": Unknown notification element - " + value);
> +          }

Sorry, but why this extra code? It changes the API, right?

@@ +618,4 @@
>          break;
>        case "urlbar_input":
> +        nodeCollector.root = this.getElement({type: "urlbar"}).getNode();
> +        nodeCollector.queryAnonymousNode("anonid", "input");

I would love to see that we test for those getElement calls in a new test under /lib/tests.

@@ +639,5 @@
>  
>    /**
>     * Retrieves the specified element of the door hanger notification bar
>     *
> +   * @param {string} aId

Why this change?

@@ +646,2 @@
>     *        Lookup string of the notification bar's child element
>     *        [optional - default: ""]

Missed to update the jsdoc?

::: tests/functional/restartTests/testPreferences_masterPassword/test1.js
@@ +53,5 @@
>  
>    // After logging in, remember the login information
>    var button = locationBar.getNotificationElement(
>                   "password-save-notification",
> +                 'rememberPasswordButton'

nit: Please be consistent and use double quotes.

::: tests/functional/testPreferences/testPasswordSavedAndDeleted.js
@@ +47,5 @@
>  
>    // After logging in, remember the login information
>    var button = locationBar.getNotificationElement(
>                   "password-save-notification",
> +                 'rememberPasswordButton'

Same here.

Comment 4

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v2.0

(In reply to Henrik Skupin (:whimboo) from comment #3)
> 
> @@ +564,2 @@
> >        case "contextMenu":
> > +        nodeCollector.root = this.getElement({type: "urlbar_input"}).getNode().parentNode;
> 
> I would use 'nodeCollector.root = this.getElement({type:
> "urlbar"}).getNode();' here.
It's not working like that, probably because urlbar is not an xul element, and beside this technique it's used in mozilla to for geting the context menu:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/textbox.xml#256 


> @@ +597,5 @@
> > +              nodeCollector.queryAnonymousNode("anonid", "button");
> > +              break;
> > +            default:
> > +              throw new Error(arguments.callee.name + ": Unknown notification element - " + value);
> > +          }
> 
> Sorry, but why this extra code? It changes the API, right?
Here it was receiving an lookup expression to get the child elements from notification pop-up if it had a second parameter, so instead of a lookup expression I query childs, as a subtype, if you don't like this approach, I will try to find something else.
> 
> @@ +618,4 @@
> >          break;
> >        case "urlbar_input":
> > +        nodeCollector.root = this.getElement({type: "urlbar"}).getNode();
> > +        nodeCollector.queryAnonymousNode("anonid", "input");
> 
> I would love to see that we test for those getElement calls in a new test
> under /lib/tests.
I added the rest of elements in tests, except for notification elements witch needs lots of steps to get and are tested in actual tests.
If it's better now I will bring new reports.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 795379 [details] [diff] [review]
patch_v2.0

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

In general it looks fine. But some things still have to be sorted out.

::: lib/tests/testToolbar.js
@@ +26,5 @@
>    var reloadButton = locationBar.getElement({type: "reloadButton"});
>    expect.equal(reloadButton.getNode().command, "Browser:ReloadOrDuplicate");
> +
> +  var urlbar = locationBar.getElement({type: "urlbar"});
> +  expect.ok(urlbar.exists(), "URL bar has been found");

It's better we would check for the type as we already do above.

::: lib/toolbars.js
@@ +160,5 @@
> +   *                            [optional]
> +   *                  parent  - Parent of the to find element
> +   *                            [optional]
> +   *
> +   * @returns Element which has been found

Would you please create a new bug which should cover the update of the jsdoc for all of those methods across our library to the conformed style? Sub-properties should get their own @param lines.

Don't worry about that in your patch.

@@ +559,5 @@
> +              nodeCollector.queryAnonymousNode("anonid", "button");
> +              break;
> +            default:
> +              throw new Error(arguments.callee.name + ": Unknown notification element - " + value);
> +          }

Why do we have to query multiple times for the rememberPasswordButton button? I would really like to get this special casing out of this method.

@@ +604,5 @@
>     *
>     * @param {string} aType
>     *        Type of the notification bar to look for
> +   * @param {string} aChildElement
> +   *        General type of child element  to retrieve from notification pop-up

What is meant with ´type´ here? The name, any other js property? It's a bit unclear.

Comment 6

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v3.0

I made the changes and made reports too 
Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/3c3cd991de01b704f3f65783a1b33c2c
http://mozmill-crowd.blargon7.com/#/functional/report/3c3cd991de01b704f3f65783a1b3375a
http://mozmill-crowd.blargon7.com/#/functional/report/3c3cd991de01b704f3f65783a1b2f0ba

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 798544 [details] [diff] [review]
patch v3.0

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

In general it looks fine. There is still one thing, which I don't like. Please see my inline comment.

::: lib/tests/testToolbar.js
@@ +27,5 @@
>    expect.equal(reloadButton.getNode().command, "Browser:ReloadOrDuplicate");
> +
> +  var urlbar = locationBar.getElement({type: "urlbar"});
> +  expect.equal(urlbar.getNode().getAttribute("type"), "autocomplete",
> +               "URL bar has been found");

As you can see above when we check for the type of contextMenu we do that via localName. Please also make use of it for all of the elements, and not only this one.

::: lib/toolbars.js
@@ +609,5 @@
> +    if (aChildElement) {
> +      var nodeCollector = new domUtils.nodeCollector(notification.getNode());
> +      switch (aChildElement) {
> +        case "rememberPasswordButton":
> +          nodeCollector.queryAnonymousNode("anonid", "button");

I still haven't gotten an answer yet for my question I have raised in the last review. Why do we have to query twice for the button?

I really don't like the extension of this method with very specific cases.

Comment 8

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #7)
> I still haven't gotten an answer yet for my question I have raised in the
> last review. Why do we have to query twice for the button?
> 
> I really don't like the extension of this method with very specific cases.

In test:
>49 var button = locationBar.getNotificationElement(	    49	  var button = locationBar.getNotificationElement(
>50               "password-save-notification",		    50	                 "password-save-notification",
>51	         '/anon({"anonid":"button"})'		    51	                 "rememberPasswordButton"
>52	        );                                          52	               );

In lib:
>545	  getNotificationElement : function locationBar_getNotificationElement(aType, aLookupString) {
>546	    var lookup = '/id("' + aType + '")';
>547	    lookup = aLookupString ? lookup + aLookupString : lookup;

As you can see above, before in tests if method getNotificationElement had an second optional parameter it returned an child element from notification element by appending a string to look-up expresion.
In order to give up on look-up expressions I had to query for child elements from notification separately, using nodeCollector.

So just to be sure what I should change, should I drop the switch in method and send as as second parameter an object like { type: "anonid", value: "button"} ?

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Something like that, yes. Given that we don't really know which attribute or property we have to query for a dict might be necessary, as you have given above. I just wonder if those elements are always only reachable via queryAnonymousNode or if we also have normal selectors. That would make it a bit more difficult.

Comment 10

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v3.1

I updated the patch.

Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/f54ca65f73fd56845c30b7ab7dd12491
http://mozmill-crowd.blargon7.com/#/functional/report/f54ca65f73fd56845c30b7ab7dd1420a
http://mozmill-crowd.blargon7.com/#/functional/report/f54ca65f73fd56845c30b7ab7dd12bdd

Comment 11

[Andrei Eftimie]

Comment on attachment 819684 [details] [diff] [review]
patch v3.1

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

In principle this is good, there are however some changes I'd like to discuss a bit.

Some nits inline, and the last item is debatable: 
Should we use nodeCollector for all items? Or just for anonymous elements?

::: lib/toolbars.js
@@ +192,5 @@
>      var elem = null;
> +    var parent = spec.parent;
> +    var subtype = spec.subtype;
> +    var type = spec.type;
> +    var value = spec.value;

I'm not sure why we need to declare all of these here instead of calling directly:
> spec.parent
> spec.type
> ...

@@ +197,5 @@
>  
> +    var root = parent ? parent.getNode() : this._controller.window.document;
> +    var nodeCollector = new domUtils.nodeCollector(root);
> +
> +    switch(type) {

nit: space between switch and bracket

@@ +512,5 @@
> +
> +    var parent = spec.parent;
> +    var subtype = spec.subtype;
> +    var type = spec.type;
> +    var value = spec.value;

Same as above, we can omit these declarations

@@ +533,3 @@
>          break;
>        case "favicon":
> +        nodeCollector.queryNodes("#page-proxy-favicon");

I'd like some discussion here.
Do we want to rely on nodeCollector for all items?

We could keep elementslib.* for all queries that aren't anonymous.
I'm not necessarily against it, but we're adding more dependencies then we might need.

The scope of this bug is to get rid of the LookUp expressions.

Comment 12

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: 879953.patch

I did the changes, here are the new reports:
http://mozmill-crowd.blargon7.com/#/functional/report/95c9c51e8c2d9f14917fc749dd6adb76
http://mozmill-crowd.blargon7.com/#/functional/report/95c9c51e8c2d9f14917fc749dd6af5c0
http://mozmill-crowd.blargon7.com/#/functional/report/95c9c51e8c2d9f14917fc749dd6b37eb

Comment 13

[Andrei Eftimie]

Comment on attachment 822280 [details] [diff] [review]
879953.patch

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

I like this much better than using nodeCollector for all elements.

Please check the following failure and a few nits mentioned inline.

In the linux testrun there is 1 failure which I can't find reference on bugzilla for, and that runs some code that has been changed in this patch.
I haven't been able to locally reproduce that failure. Please make sure that it is not a regression.
> /testAwesomeBar/testSuggestBookmarks.js -- The auto-complete result is a bookmark - 'favicon' should contain 'bookmark'

And a note: please also always use the same mozmill version we are using in production when testing patches.
Right now that is 1.5.24 (this shouldn't affect results right now, because changes in mozmill between 1.5.23 and 1.5.24 are minimal, but we should not get in the habit of using obsolete mozmill versions for testing).

::: lib/toolbars.js
@@ +577,5 @@
>     * @param {string} aType
>     *        Type of the notification bar to look for
> +   * @param {object}  aChildElement
> +   *        Configuration of child element to retrieve from notification element
> +   *        [optional]

nit: 2 spaces between {type} and name and if the element is optional wrap the name between square brackets:
@param {object} [aChildElement=""]

@@ +587,5 @@
>     */
> +  getNotificationElement : function locationBar_getNotificationElement(aType, aChildElement) {
> +    var notification = this.getElement({type: "notification_element",
> +                            subtype: aType,
> +                            parent: this.getNotification()});

Please indent this to be aligned with the start of the argument object

@@ +592,2 @@
>  
> +    if (aChildElement) {

Can we make this the other way around?
We should always return early if we can.

if (!aChildElement) { return notification; }
[...]

Comment 14

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v4.0

Done.
Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/8a42e4b8f277f9c0268b061eb8f27d5e
http://mozmill-crowd.blargon7.com/#/functional/report/8a42e4b8f277f9c0268b061eb8f2a4e3
http://mozmill-crowd.blargon7.com/#/functional/report/8a42e4b8f277f9c0268b061eb8f27c10

Comment 15

[Andrei Eftimie]

Comment on attachment 825318 [details] [diff] [review]
patch v4.0

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

You misunderstood my comment about the optional JSDOC param, see inline for details.
You have an r+ from me, but please fix that comment.

We'll fix the rest of the comments in bug 882137.

::: lib/toolbars.js
@@ +577,5 @@
>     * @param {string} aType
>     *        Type of the notification bar to look for
> +   * @param {object} aChildElement
> +   *        Configuration of child element to retrieve from notification element
> +   *        [aChildElement=""]

The square brackets and the default value should have been around the name of the element, not a new line after the description.

Comment 16

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v4.1

I updated the documentation.

Comment 17

[Andrei Eftimie]

Comment on attachment 825790 [details] [diff] [review]
patch v4.1

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

Looks good, landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/7d43ed60222f (default)

We'll revisit all JSDOC comments in bug 882137

Comment 18

[Andrei Eftimie]

I'm not sure if we should backport these changes or not.
I would think rather not.

Henrik, what do you think?

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Most likely not. None of the lookup strings should have to be updated on the older branches given that no ui will change.

Comment 20

[Andrei Eftimie]

Great, I think we're done here.