1055453 - Add new ui module for the page-info window

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

Description

[Mihaela Velimiroviciu (:mihaelav)]

Follow up from bug 1025849 comment 22:
> What we would need here is a new ui module for the page-info window. Given the importance of this test, I would be ok to get this done in a follow-up bug.

Comment 1

[Daniel Gherasim]

That's something we already started to implement in bug 863139.
As we will touch a lot of other things there, I'll make this a blocker for that bug and finish up the page-info class here.

* This will also update the tests using it!

Comment 2

[Daniel Gherasim]

Attachment: v1.patch

So this is the library with only 1 test modified.
Let's get a feedback and I'll continue with modifying the other tests. (firefox/tests/remote/testSecurity ones).

Comment 3

[Daniel Gherasim]

Comment on attachment 8501055 [details] [diff] [review]
v1.patch

We discussed to have the implementation different.
I'll do the changes and ask directly for review.

Comment 4

[Daniel Gherasim]

Attachment: v2.patch

Patch with the library and all the tests updated.

Comment 5

[Daniel Gherasim]

Attachment: v2.1.patch

Forgot to update the lib test.

Comment 6

[Andrei Eftimie]

Comment on attachment 8502497 [details] [diff] [review]
v2.1.patch

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

Nice work on the pageInfoWindow class, simplifies a lot of things.

::: firefox/lib/tests/testPageInfoWindow.js
@@ +33,5 @@
> +    },{
> +      name: "tab_security",
> +      value: "radio",
> +      elements: [
> +        {name: "panel_security", value: "vbox"},

Could we please also change this to an object with key/value as name/localname

::: firefox/lib/ui/page-info.js
@@ +152,5 @@
> +  }, {state: "open", type: "Browser:page-info"});
> +}
> +
> +/**
> + * Open the page indo window

typo: info

::: firefox/tests/functional/testSecurity/testGreyLarry.js
@@ +55,5 @@
>                 "The Larry UI is unknown (aka Grey)");
>  
>    // Check the More Information button
>    var moreInfoButton = identityPopup.getElement({type: "moreInfoButton"});
> +  var callbackOpen = () => {

Kind of an antipattern here. We have an anonymous function which we save a reference to...

We could simply do:
> function callbackOpen() {}

I guess the end result is the same, so I'm not really against this... just looks... different :)

::: firefox/tests/functional/testTechnicalTools/testAccessPageInfoDialog.js
@@ +17,5 @@
> +    {button: 'tab_general', panel: 'panel_general'},
> +    {button: 'tab_media', panel: 'panel_media'},
> +    {button: 'tab_permissions', panel: 'panel_permissions'},
> +    {button: 'tab_security', panel: 'panel_security'}
> +  ]

> pageInfoPanels: {
>  tab_general: "panel_general",
>  [...]
>}

@@ +37,2 @@
>  
>    // Open context menu on the html element and select Page Info entry

Remove or merge this comment with the one below, and remove the empty newline between them.

::: firefox/tests/remote/testSecurity/testDVCertificate.js
@@ +116,2 @@
>   */
> +function checkSecurityTab(aWindow) {

Renounce this method, add all it's contents to the main test please, we needed it as a callback before, but with the new pageInfoWindow object, there's no need for it. Same for the other tests please.

::: firefox/tests/remote/testSecurity/testGreenLarry.js
@@ +28,5 @@
>    if (aModule.targetPanel) {
>      aModule.targetPanel.getNode().hidePopup();
>    }
> +
> +  windows.closeAllWindows(browserWindow.controller);

Good idea to add this to all other tests that make use of pageInfoWindow. In case something goes wrong, we should not leave the window open.

Comment 7

[Daniel Gherasim]

Attachment: v3.patch

Thanks Andrei.
I filed bug 1083128 to update the tests in a follow-up bug.

Comment 8

[Andrei Eftimie]

Comment on attachment 8505449 [details] [diff] [review]
v3.patch

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

lgtm

Comment 9

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

Comment on attachment 8505449 [details] [diff] [review]
v3.patch

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

Looks to be a good start. What I wonder is if we should also have a helper method for setting/getting the categories in the page info window. Otherwise lets fix the constructor related behavior. See my comments inline.

::: firefox/lib/tests/testPageInfoWindow.js
@@ +52,5 @@
> +  browserWindow.controller.open(TEST_DATA.url);
> +  browserWindow.controller.waitForPageLoad();
> +
> +  var piW1 = browserWindow.openPageInfoWindow({method: "shortcut"});
> +  piW1.close();

Maybe just calling those variables win1...? I wonder what this is encoded in. :)

::: firefox/lib/ui/browser.js
@@ +7,5 @@
>  Cu.import("resource://gre/modules/PrivateBrowsingUtils.jsm");
>  
>  // Include required modules
>  var baseWindow = require("../../../lib/ui/base-window");
> +var pageInfo = require("page-info");

You may want to make this extra block for ui modules while you are here.

@@ +137,5 @@
> +        aSpec.target.rightClick();
> +        var contextEntry = findElement.ID(this._controller.window.document,
> +                                          "context-viewinfo");
> +        contextEntry.click();
> +        utils.closeContentAreaContextMenu(this._controller);

To make this safer I would suggest you make use of the controller Menu() class.

::: firefox/lib/ui/page-info.js
@@ +14,5 @@
> + * 'PageInfo' Window  class
> + *
> + * @constructor
> + * @param {MozMillController} [aController=mozmill.getBrowserController()]
> + *        Controller of the page info window

The default is wrong. This has to be a controller to a page info window. You cannot use a browser window.

@@ +17,5 @@
> + * @param {MozMillController} [aController=mozmill.getBrowserController()]
> + *        Controller of the page info window
> + */
> +function PageInfoWindow(aController) {
> +  var controller = aController || true;

I don't see why you assign true in case of no controller? Maybe we should call the BaseWindow constructor with `null` as argument, and update it's behavior. True is not a valid controller object.

@@ +24,5 @@
> +  this._dtds = ["chrome://browser/locale/pageInfo.dtd"];
> +}
> +
> +PageInfoWindow.prototype = Object.create(baseWindow.BaseWindow.prototype);
> +PageInfoWindow.constructor = baseWindow.BaseWindow;

Huh? The constructor has to be PageInfoWindow.

@@ +128,5 @@
> +
> +/**
> + * Get page info related property
> + *
> + * @param {String} aPrefName

Please `aName`. The included `pref` part is confusing.

@@ +131,5 @@
> + *
> + * @param {String} aPrefName
> + *        Property name
> + */
> +PageInfoWindow.prototype.getProperty = function PageInfoWindow_getProperty(aPrefName) {

nit: Shorten the internal name to e.g. PIW_

@@ +133,5 @@
> + *        Property name
> + */
> +PageInfoWindow.prototype.getProperty = function PageInfoWindow_getProperty(aPrefName) {
> +  var url = "chrome://browser/locale/pageInfo.properties";
> +  return utils.getProperty(url, aPrefName);

If we have to retrieve a property, it should be handled similar as the dtds. So please update it accordingly including the base window.

@@ +144,5 @@
> + *        Callback function that triggers the opening
> + */
> +PageInfoWindow.prototype.open = function PageInfoWindow_open(aCallback) {
> +  assert.equal(typeof aCallback, "function",
> +               "Callback function has been defined");

nit: you can remove `function`

@@ +152,5 @@
> +  }, {state: "open", type: "Browser:page-info"});
> +}
> +
> +/**
> + * Open the page indo window

nit: spelling mistake.

@@ +155,5 @@
> +/**
> + * Open the page indo window
> + *
> + * @param {function} aCallback
> + *        Callback function that triggers the opening

nit: function

@@ +165,5 @@
> +  pageInfoWindow.open(aCallback);
> +  return pageInfoWindow;
> +}
> +
> +// Export the PageInfoWindow class

No other file is mentioning the class name here.

Comment 10

[Daniel Gherasim]

Attachment: v5.patch

Still needs to wait for "_property" parameter and "getProperty" method. (bug 1088007 then bug 1084333 - Part 2). But patch to review after those get landed is ready.

So thanks Henrik for review, all you requests were addressed.

Comment 11

[Andrei Eftimie]

Comment on attachment 8512564 [details] [diff] [review]
v5.patch

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

lgtm.
With the mentioned item addressed ask a review from Henrik please

::: firefox/lib/ui/page-info.js
@@ +71,5 @@
> + *        Parent of the element
> + *
> + * @returns {ElemBase} Element which has been found
> + */
> +PageInfoWindow.prototype.getElement = function PIW_getElement(aSpec) {

Ah, we can now remove this as we have it in the base class.

Comment 12

[Daniel Gherasim]

Attachment: v6.patch

Also made the updates as we agreed on bug 1081047.

Comment 13

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

Comment on attachment 8513418 [details] [diff] [review]
v6.patch

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

We are close. There are some unclear lines of code and other changes to do. Please see my inline comments.

::: firefox/lib/ui/browser.js
@@ +114,5 @@
> + * @param {object} [aSpec]
> + *        Information for opening the window
> + * @param {function} [aSpec.callback]
> + *        Callback function that triggers the opening
> + * @param {string} [aSpec.method="shortcut"|"menu"|"contextMenu"|"callback"]

Please sort the possible values alphabetically.

@@ +122,5 @@
> + *
> + * @returns {PageInfoWindow} New instance of the PageInfoWindow class
> + */
> +BrowserWindow.prototype.openPageInfoWindow = function BW_openPageInfoWindow(aSpec={}) {
> +  var method = aSpec.method || "shortcut";

Didn't we default to menu for other classes? I think that this is the method we wanted to use as default where-ever possible.

@@ +128,5 @@
> +  var callback = () => {
> +    switch (method) {
> +      case "callback":
> +        assert.equal(typeof aSpec.callback, "function",
> +                     "Callback to open the page info window has been defined");

"Callback function has been defined" is enough.

::: firefox/lib/ui/page-info.js
@@ +32,5 @@
> + * Get the current category
> + *
> + * @returns {string} The current selected category
> + */
> +PageInfoWindow.prototype.__defineGetter__("category", function() {

nit: please use a blank after function in all the cases in this file.

@@ +40,5 @@
> +    if (element.getNode().selected) {
> +      category = aCategory;
> +      return true;
> +    }
> +  });

Why do we have to go through each of the categories? Instead return the value of .selectedPanel from the mainDeck node.

@@ +52,5 @@
> + *        Category to swith to
> + */
> +PageInfoWindow.prototype.__defineSetter__("category", function(aCategory) {
> +  var tab = this.getElement({type: "tab_" + aCategory});
> +  tab.click();

We switch the category here, so we should have an event listener for when the new pane has been shown.

@@ +110,5 @@
> +    case "security_viewSavedPasswordsButton":
> +      elems = [findElement.ID(root, "security-view-password")];
> +      break;
> +    case "tab_feed":
> +      elems = [findElement.ID(root, "feedTab")];

Are those the categories? Then please use the category_ prefix for the names.

Comment 14

[Teodor Druta]

Attachment: v7.patch

(In reply to Henrik Skupin (:whimboo) from comment #13)

> 
> ::: firefox/lib/ui/browser.js
> @@ +114,5 @@
> > + * @param {string} [aSpec.method="shortcut"|"menu"|"contextMenu"|"callback"]
> 
> Please sort the possible values alphabetically.

Sorted alphabetically.

> @@ +122,5 @@
> > +  var method = aSpec.method || "shortcut";
> 
> Didn't we default to menu for other classes? I think that this is the method
> we wanted to use as default where-ever possible.
> 

Changed default to "menu"

> @@ +128,5 @@
> > +  var callback = () => {
> > +    switch (method) {
> > +      case "callback":
> > +        assert.equal(typeof aSpec.callback, "function",
> > +                     "Callback to open the page info window has been defined");
> 
> "Callback function has been defined" is enough.
> 

> nit: please use a blank after function in all the cases in this file.
> 

Added blank space after all anonymous functions without parameters.

> @@ +40,5 @@
> > +    if (element.getNode().selected) {
> > +      category = aCategory;
> > +      return true;
> > +    }
> > +  });
> 

Now returns the value of selectedPanel from mainDeck

> 
> @@ +52,5 @@
> > + *        Category to swith to
> > + */
> > +PageInfoWindow.prototype.__defineSetter__("category", function(aCategory) {
> > +  var tab = this.getElement({type: "tab_" + aCategory});
> > +  tab.click();
> 
> We switch the category here, so we should have an event listener for when
> the new pane has been shown.
> 

Added an waitFor for panel switch.

> @@ +110,5 @@
> > +    case "tab_feed":
> > +      elems = [findElement.ID(root, "feedTab")];
> 
> Are those the categories? Then please use the category_ prefix for the names.

Changed the "tab_" prefix to "_category".

Comment 15

[Andrei Eftimie]

Comment on attachment 8514985 [details] [diff] [review]
v7.patch

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

::: firefox/lib/ui/page-info.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> + // Include required modules

nit: there's an extra whitespace at the beginning of the line

@@ +21,5 @@
> +  baseWindow.BaseWindow.call(this, aController);
> +
> +  this._dtds = ["chrome://browser/locale/pageInfo.dtd"];
> +  this._properties = ["chrome://browser/locale/pageInfo.properties"];
> +  this._categories = ["general", "media", "feed", "permissions", "security"];

Please leave every category on a new line (helps with the source control when we need to edit only _some_ of them later on).

Why not name these "_panels" and have the id's directly as they are in the page "generalPanel", "mediaPanel" ...
Maybe better if we also want "nice" names, let them be a key/value pair

@@ +23,5 @@
> +  this._dtds = ["chrome://browser/locale/pageInfo.dtd"];
> +  this._properties = ["chrome://browser/locale/pageInfo.properties"];
> +  this._categories = ["general", "media", "feed", "permissions", "security"];
> +
> +  // dump(this.category);

I think you wanted to remove this in the final version ;)

@@ +37,5 @@
> + */
> +PageInfoWindow.prototype.__defineGetter__("category", function () {
> +  // Return category name, remove "Panel" from selected panel id
> +  let category = this.getElement({type: "mainDeck"}).getNode()
> +                                                    .selectedPanel.id.replace(/\Panel\b/g, "");

Then we can also remove the need for replace.

@@ +39,5 @@
> +  // Return category name, remove "Panel" from selected panel id
> +  let category = this.getElement({type: "mainDeck"}).getNode()
> +                                                    .selectedPanel.id.replace(/\Panel\b/g, "");
> +  // Permissions panel is named permPanel in DOM
> +  return (category === "perm") ? "permissions" : category;

Shouldn't it be better to name it directly "perm" in the _categories array?

@@ +118,5 @@
> +      break;
> +    case "security_viewSavedPasswordsButton":
> +      elems = [findElement.ID(root, "security-view-password")];
> +      break;
> +    case "category_feed":

Also name these "tab_*"

Comment 16

[Teodor Druta]

Attachment: v8.patch

Updated PageInfoWindow class category getter.

Comment 17

[Andrei Eftimie]

Comment on attachment 8518852 [details] [diff] [review]
v8.patch

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

An even better idea with indexes.
I would just group relevant items in getElements together (and a couple nits).


Ask a review from Henrik with these changes.

::: firefox/lib/ui/page-info.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> + // Include required modules

nit: theres an extra space at the beginning of the line

@@ +21,5 @@
> +  baseWindow.BaseWindow.call(this, aController);
> +
> +  this._dtds = ["chrome://browser/locale/pageInfo.dtd"];
> +  this._properties = ["chrome://browser/locale/pageInfo.properties"];
> +  this._categories = ["general", "media", "feed", "permissions", "security"];

keep each category on a separate line please.

@@ +45,5 @@
> + *        Category to swith to
> +  * @param {function} aCallback
> + *        Callback that triggers the opening
> + */
> +PageInfoWindow.prototype.__defineSetter__("category", function(aCategory) {

nit: leave a space between function and open parenthesis

@@ +93,5 @@
> +    case "panel_permissions":
> +      elems = [findElement.ID(root, "permPanel")];
> +      break;
> +    case "panel_security":
> +      elems = [findElement.ID(root, "securityPanel")];

We should group both "panels" and "tabs" (or categories, whatever), into 1 type with subtypes:
> getElements{type: "panel", subtype: "media"}

Comment 18

[Teodor Druta]

Attachment: v9.patch

>> I would just group relevant items in getElements together (and a couple nits).

Actually this change implied a lot of modifications in page-info.js and testPageInfoWindow.js, also changed a few other things in getElements() to keep everything consisted. I added Andrei and Andreea to review again because of this.

Comment 19

[Andrei Eftimie]

Comment on attachment 8518956 [details] [diff] [review]
v9.patch

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

Some bitrot:
> patching file firefox/lib/ui/browser.js
> Hunk #1 FAILED at 1
> Hunk #3 succeeded at 105 with fuzz 2 (offset 2 lines).
> 1 out of 3 hunks FAILED -- saving rejects to file firefox/lib/ui/browser.js.rej

::: firefox/lib/ui/browser.js
@@ +142,5 @@
> +      case "menu":
> +        this._controller.mainMenu.click("#menu_pageInfo");
> +        break;
> +      case "shortcut":
> +        var cmdKey = utils.getEntity(this.dtds, "pageInfoCmd.commandkey");

With bug 1084333 landed, you should be able to use `this.getEntity`

Comment 20

[Teodor Druta]

Attachment: v10.patch

Fixed the patch it should apply cleanly now.

Testruns all green:
http://mozmill-crowd.blargon7.com/#/functional/report/b67428b6e502c230ff85b6a4c42c9e66
http://mozmill-crowd.blargon7.com/#/remote/report/b67428b6e502c230ff85b6a4c42cb5c8
http://mozmill-crowd.blargon7.com/#/endurance/report/b67428b6e502c230ff85b6a4c42cc83d

Comment 21

[Andrei Eftimie]

Comment on attachment 8521481 [details] [diff] [review]
v10.patch

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

Need to be updated as there's a conflict in firefox/lib/tests/manifest.ini

For the updated patch, please ask a review from Henrik.

::: firefox/lib/tests/testPageInfoWindow.js
@@ +55,5 @@
> +  var contentElement = findElement.ID(browserWindow.controller.tabs.activeTab,
> +                                      "content");
> +  var win1 = browserWindow.openPageInfoWindow({method: "contextMenu",
> +                                               target: contentElement});
> +

nit: remove this empty line

Comment 22

[Teodor Druta]

Attachment: v10.1.patch

Removed the blank line. Fixed the rejection in manifest.ini.

Comment 23

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

Comment on attachment 8522216 [details] [diff] [review]
v10.1.patch

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

::: firefox/lib/ui/browser.js
@@ +115,5 @@
> + * @param {object} [aSpec]
> + *        Information for opening the window
> + * @param {function} [aSpec.callback]
> + *        Callback function that triggers the opening
> + * @param {string} [aSpec.method="callback"|"contextMenu"|"menu"|"shortcut"]

Not all of them can be default values at the same time. The list needs to be inside the description. Only put the default value in here.

::: firefox/lib/ui/page-info.js
@@ +27,5 @@
> +                      "media",
> +                      "feed",
> +                      "perm",
> +                      "security"
> +                     ];

This indentation does not conform to the coding-style. Also I do not like this hard-coded list. How would our test behave if a tab is not present? Wouldn't we return the wrong category? IMO we should retrieve the categories in real-time when needed.

@@ +40,5 @@
> + * @returns {string} The current selected category
> + */
> +PageInfoWindow.prototype.__defineGetter__("category", function () {
> +  let index = this.getElement({type: "mainDeck"}).getNode().selectedIndex;
> +  return this._categories[index];

Why not selectedPanel as I have suggested the last time? Then we wouldn't need this hard-coded list.

@@ +47,5 @@
> +/**
> + * Set the current selected category
> + *
> + * @param {string} aCategory
> + *        Category to swith to

nit: switch

@@ +93,5 @@
> +    case "panel":
> +      elems = [findElement.ID(root, aSpec.subtype + "Panel")];
> +      break;
> +    case "security":
> +      switch(aSpec.subtype) {

nit: missing space after switch

@@ +123,5 @@
> + * @returns {PageInfoWindow} New instance of the PageInfoWindow class
> + */
> +function open(aCallback) {
> +  assert.equal(typeof aCallback, "function",
> +               "Callback function has been defined");

"Callback has been defined"

Comment 24

[Teodor Druta]

Attachment: v10.2.patch

> This indentation does not conform to the coding-style. Also I do not like
> this hard-coded list. How would our test behave if a tab is not present?
> Wouldn't we return the wrong category? IMO we should retrieve the categories
> in real-time when needed.
> 
> @@ +40,5 @@
> > + * @returns {string} The current selected category
> > + */
> > +PageInfoWindow.prototype.__defineGetter__("category", function () {
> > +  let index = this.getElement({type: "mainDeck"}).getNode().selectedIndex;
> > +  return this._categories[index];
> 
> Why not selectedPanel as I have suggested the last time? Then we wouldn't
> need this hard-coded list.
> 

I don't think we have an easy way to retrieve the categories in real time.
The page info window always has these five categories. The feed category being hidden, displayed only for rss pages.

> let index = this.getElement({type: "mainDeck"}).getNode().selectedIndex;

Will always return the correct index even if the feed tab & panel is hidden.
Even if the feed tab & panel is not available it will return the index in this order:

> this._categories = ["general", "media", "feed", "perm", "security"];

So Perm will always be index=> 3 and security index=>4.

We choose the selectedIndex solution because we had to use regular expressions in case of returning the selectedPanel value.

Comment 25

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

Comment on attachment 8523802 [details] [diff] [review]
v10.2.patch

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

I still see an ongoing discussion, so a feedback request should not have been put up yet. This is only time we miss later. Also I do not see anything in your comment which lists what has been changed. Please remember to always add this content.

(In reply to Teodor Druta from comment #24)
> I don't think we have an easy way to retrieve the categories in real time.
> The page info window always has these five categories. The feed category
> being hidden, displayed only for rss pages.

Why don't we have that possibility? We do a similar thing for the add-on manager and preferences. So it's not different here. I don't want to have to update the test and libs if a new category gets added. As mentioned earlier the #mainDeck has all the information.

> We choose the selectedIndex solution because we had to use regular
> expressions in case of returning the selectedPanel value.

A simple split would have been enough.

::: firefox/lib/ui/browser.js
@@ +116,5 @@
> + *        Information for opening the window
> + * @param {function} [aSpec.callback]
> + *        Callback function that triggers the opening
> + * @param {string} [aSpec.method="menu"]
> + *        How to open the Page Info window

Why have you removed all the other possible options?

Comment 26

[Teodor Druta]

(In reply to Henrik Skupin (:whimboo) from comment #25)

> Why don't we have that possibility? We do a similar thing for the add-on
> manager and preferences. So it's not different here. I don't want to have to
> update the test and libs if a new category gets added. As mentioned earlier
> the #mainDeck has all the information.
> 

If a category will be changed/added/removed we will have to modifiy our tests/libs anyway because of the getElements() method.

Comment 27

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

(In reply to Teodor Druta from comment #26)
> If a category will be changed/added/removed we will have to modifiy our
> tests/libs anyway because of the getElements() method.

We would have to modify the modules to ADD a new feature, but not to fix broken tests.

Comment 28

[Teodor Druta]

(In reply to Henrik Skupin (:whimboo) from comment #27)
> We would have to modify the modules to ADD a new feature, but not to fix
> broken tests.

ok, I will look over the addons and prefs libs and will try to implement a similar approach for getting the panels and tabs.

Comment 29

[Teodor Druta]

Attachment: v11.patch

Henrik, can you please look over my attached patch and provide feedback for the changes, before I'll add it as a review.

I have removed the hardcoded categories parameter, removed the category getter, added two new getters for tab and panel and a setter for tab which will set the tab & panel, also I have added two new methods getTabs and getPanels which will return the list of available tabs and panels ids of the current Page Info Window.

I maintained the current getElements() structure, so I removed all the hardcoded panels and tabs names from the lib.

Comment 30

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

Comment on attachment 8524518 [details] [diff] [review]
v11.patch

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

It goes into the right direction but needs some more updates. Thanks Teodor.

::: firefox/lib/ui/page-info.js
@@ +63,5 @@
> +  tab.click();
> +
> +  // Wait for panel to be selected
> +  assert.waitFor(() => (this.panel.replace(/\Panel\b/g, "") === aTab),
> +                 "Panel " + aTab + " has been selected");

There is no event we can wait for?

@@ +130,5 @@
> +  let panels = [];
> +  let mainDeckChildNodes = this.getElement({type: "mainDeck"}).getNode().childNodes;
> +
> +  for(var i = 0; i < mainDeckChildNodes.length; i++) {
> +    panels.push(mainDeckChildNodes[i].id);

When would we need this method? I don't think we are interested in all the panels but only the one which is currently displayed.

@@ +141,5 @@
> + * Get current PIW tabs
> + *
> + * @returns {array} Available PIW tabs
> + */
> +PageInfoWindow.prototype.getTabs = function PIW_getTabs() {

Why have you renamed category with tabs? As I said earlier we should keep the naming convention across different ui modules, at least when it looks the same. Here this does apply, even the category list is at the top.

@@ +147,5 @@
> +  let viewGroupChildNodes = this.getElement({type: "viewGroup"}).getNode().childNodes;
> +
> +  for(var i = 0; i < viewGroupChildNodes.length; i++) {
> +    tabs.push(viewGroupChildNodes[i].id);
> +  }

Too much overhead. Please use nodeCollector here, which can directly return you all the elements via a query all selector. Maybe you can add a new type to getElements like 'categories'. That way we can also be sure to have the correct elements. In your case childNodes could become something else, e.g. if other nodes are getting injected in between. We should avoid those constructs.

Comment 31

[Teodor Druta]

Attachment: v12.patch

(In reply to Henrik Skupin (:whimboo) from comment #30)
> ::: firefox/lib/ui/page-info.js
> @@ +63,5 @@
> > +  tab.click();
> > +
> > +  // Wait for panel to be selected
> > +  assert.waitFor(() => (this.panel.replace(/\Panel\b/g, "") === aTab),
> > +                 "Panel " + aTab + " has been selected");
> 
> There is no event we can wait for?
> 

I search the mozilla-centra code and couldn't find an event for this. I think it uses css to display/hide the panels.

I removed the getters and setter for tab and panel, added an unified getter and setter for tabs & panels named category. Added a new type for the getElements() method, "categories" which will return all available tabs elements for the opened page info window.

Also I updated the filterByJSProperty() method in dom-utils:

> ::: /lib/dom-utils.js
> @@ 659,8 @@
> if (aProperty && aValue)

this line didn't worked correctly for boolean values in case the value was false changed it to:

> if (aProperty && typeof aValue !== "undefined")

This should be a more correct way to handle boolean values.

Comment 32

[Andrei Eftimie]

Comment on attachment 8525284 [details] [diff] [review]
v12.patch

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

Looks pretty good to me.

::: firefox/lib/ui/page-info.js
@@ +87,5 @@
> +      elems = [findElement.ID(root, "viewGroup")];
> +      break;
> +    case "categories":
> +      var nodeCollector = new domUtils.nodeCollector(root);
> +      nodeCollector.root = this.getElement({type: "viewGroup"}).getNode();

You can instantiate nodeCollector directly with the root you want to use.

Comment 33

[Teodor Druta]

Attachment: v12.1.patch

> ::: firefox/lib/ui/page-info.js
> @@ +87,5 @@
> > +      elems = [findElement.ID(root, "viewGroup")];
> > +      break;
> > +    case "categories":
> > +      var nodeCollector = new domUtils.nodeCollector(root);
> > +      nodeCollector.root = this.getElement({type: "viewGroup"}).getNode();
> 
> You can instantiate nodeCollector directly with the root you want to use.

Fixed.

Also updated the category getter to use mainDeck and selectedPanel, it should be better this way, because I think there could be a small delay between the selection of the viewGroup radio item and mainDeck selectedPanel.

Comment 34

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

Comment on attachment 8525953 [details] [diff] [review]
v12.1.patch

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

::: firefox/lib/tests/testPageInfoWindow.js
@@ +10,5 @@
> +
> +const BASE_URL = collector.addHttpResource("../../../data/");
> +const TEST_DATA = {
> +  url: BASE_URL + "layout/mozilla.html",
> +  tabs: [

categories

::: firefox/lib/ui/browser.js
@@ +116,5 @@
> + *        Information for opening the window
> + * @param {function} [aSpec.callback]
> + *        Callback function that triggers the opening
> + * @param {string} [aSpec.method="menu"]
> + *        How to open the Page Info window ["callback"|"contextMenu"|"menu"|"shortcut"]

Can we please stay consistent in how other methods in this file have been implemented? You might want to check that first. See:

http://hg.mozilla.org/qa/mozmill-tests/file/0d763ff26b27/firefox/lib/ui/browser.js#l53

::: firefox/lib/ui/page-info.js
@@ +33,5 @@
> + *
> + * @returns {string} The selected category
> + */
> +PageInfoWindow.prototype.__defineGetter__("category", function () {
> +  return this.getElement({type: "mainDeck"}).getNode().selectedPanel.id.replace(/\Panel\b/g, "");

Why such a complex regex, and not simply using split("Panel")[0] as mentioned earlier? Also get the element first and afterward operate on it as what we have listed on the coding style since the very beginning. Please read it again.

@@ +51,5 @@
> +    return;
> +  }
> +
> +  let tab = this.getElement({type: "tab", subtype: aCategory});
> +  tab.click();

We use 'var' in mostly all cases, so please stick to it. Also call this variable 'category' to stay consistent with the element you retrieve here.

@@ +84,5 @@
> +      elems = [findElement.ID(root, "mainDeck")];
> +      break;
> +    case "viewGroup":
> +      elems = [findElement.ID(root, "viewGroup")];
> +      break;

Please find better names for both of those elements, and do not simply copy the names. Our id's should be clear to understand by test authors.

Also where do we need viewGroup? Only for categories? We might want to move this entry to categories then.

@@ +86,5 @@
> +    case "viewGroup":
> +      elems = [findElement.ID(root, "viewGroup")];
> +      break;
> +    case "categories":
> +      var nodeCollector = new domUtils.nodeCollector(this.getElement({type: "viewGroup"}).getNode());

Same here as above for declaration and usage. This might also change given my other comment above.

@@ +88,5 @@
> +      break;
> +    case "categories":
> +      var nodeCollector = new domUtils.nodeCollector(this.getElement({type: "viewGroup"}).getNode());
> +      nodeCollector.queryNodes("radio");
> +      nodeCollector.filterByJSProperty("hidden", false);

Combine those two lines into a single one by chaining those actions.

@@ +91,5 @@
> +      nodeCollector.queryNodes("radio");
> +      nodeCollector.filterByJSProperty("hidden", false);
> +      elems = nodeCollector.elements;
> +      break;
> +    case "tab":

This is "category"

::: lib/dom-utils.js
@@ +655,5 @@
>     * @type {object}
>     */
>    filterByJSProperty : function nodeCollector_filterByJSProperty(aProperty, aValue) {
>      return this.filter(function (aNode) {
> +      if (aProperty && typeof aValue !== "undefined")

Good catch!

Comment 35

[Teodor Druta]

Attachment: v13.patch

(In reply to Henrik Skupin (:whimboo) from comment #34)

> ::: firefox/lib/ui/page-info.js
> @@ +33,5 @@
> > + *
> > + * @returns {string} The selected category
> > + */
> > +PageInfoWindow.prototype.__defineGetter__("category", function () {
> > +  return this.getElement({type: "mainDeck"}).getNode().selectedPanel.id.replace(/\Panel\b/g, "");

Changed the regex to a simple split

> @@ +51,5 @@
> > +    return;
> > +  }
> > +
> > +  let tab = this.getElement({type: "tab", subtype: aCategory});
> > +  tab.click();
> 
> We use 'var' in mostly all cases, so please stick to it. Also call this
> variable 'category' to stay consistent with the element you retrieve here.
> 
> @@ +84,5 @@
> > +      elems = [findElement.ID(root, "mainDeck")];
> > +      break;

renamed mainDeck to panelsContainer.

> @@ +88,5 @@
> > +      break;
> > +    case "categories":
> > +      var nodeCollector = new domUtils.nodeCollector(this.getElement({type: "viewGroup"}).getNode());
> > +      nodeCollector.queryNodes("radio");
> > +      nodeCollector.filterByJSProperty("hidden", false);
> 
> Combine those two lines into a single one by chaining those actions.
> 

Fixed. Also removed viewGroup from getElements using findElement on viewGroup id directly now.

> @@ +91,5 @@
> > +      nodeCollector.queryNodes("radio");
> > +      nodeCollector.filterByJSProperty("hidden", false);
> > +      elems = nodeCollector.elements;
> > +      break;
> > +    case "tab":
> 
> This is "category"
> 

Renamed to category everywhere it applied both in lib and test.

Fixed the jsdoc for the openPageInfoWindow method in browser lib.

Comment 36

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8526055 [details] [diff] [review]
v13.patch

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

I see all changes were implemented. But fails on OSX intermittently with:
    "message": "Category media has been selected", 

Once you fix that please ask from Henrik.

Comment 37

[Teodor Druta]

(In reply to Andreea Matei [:AndreeaMatei] from comment #36)
> Comment on attachment 8526055 [details] [diff] [review]
> v13.patch
> 
> Review of attachment 8526055 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I see all changes were implemented. But fails on OSX intermittently with:
>     "message": "Category media has been selected", 
> 
> Once you fix that please ask from Henrik.

There is an issue/difference on OSX which cannot be observer on win/linux, the page info opens, but the DOM is loaded with a small delay, I think I'll have to use a new method instead of waitForWindowState to listen for another event type ""page-info-dialog-loaded" rather than "toplevel-window-ready".

Comment 38

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

Interesting. Maybe we should enhance the waitForWindowState method so we can pass in a custom observer notification type? By default we could still use toplevel-window-ready.

Comment 39

[Teodor Druta]

Attachment: v15.patch

I added a new optional parameter (eventType) to the waitForWindowState method in windows lib.
Also I found an event which is fired when the category (tab) is changed in page info window, called "select", modified the category setter to listen for this event.

Comment 40

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

Comment on attachment 8528277 [details] [diff] [review]
v15.patch

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

::: firefox/lib/ui/page-info.js
@@ +34,5 @@
> + * @returns {string} The selected category
> + */
> +PageInfoWindow.prototype.__defineGetter__("category", function () {
> +  var category = this.getElement({type: "panelsContainer"})
> +                     .getNode().selectedPanel.id.split("Panel")[0];

Please get the element first, and then operate on it.

@@ +58,5 @@
> +  var selectFlag = false;
> +  var onSel = function() { selectFlag = true; };
> +
> +  try {
> +    doc.addEventListener("select", onSel);

Please attach event listeners to the window and not the document. Or why does the window not work? You would like to comment this if its handled differently than usual. Also why is that part of the try block?

@@ +63,5 @@
> +  }
> +  finally {
> +    cat.waitThenClick();
> +
> +    assert.waitFor(() => selectFlag, "Select event has been fired");

The above should all be in the try block, right?

@@ +67,5 @@
> +    assert.waitFor(() => selectFlag, "Select event has been fired");
> +
> +    doc.removeEventListener("select", onSel);
> +  }
> +  assert.waitFor(() => (this.category === aCategory),

nit: please add a blank line above.

::: lib/windows.js
@@ +188,5 @@
>   *        The expected state of the window to wait for
>   * @param {string} [aSpec.type]
>   *        Type of the window
> + * @param {boolean} [aSpec.eventType]
> + *        Event type to listen for

This is optional with a default. Also it is an observer and not an event. Please adjust that, including 'observer notification to wait for'.

Comment 41

[Teodor Druta]

Attachment: v16.patch

(In reply to Henrik Skupin (:whimboo) from comment #40)
> -----------------------------------------------------------------
> ::: firefox/lib/ui/page-info.js
> @@ +34,5 @@
> > + * @returns {string} The selected category
> > + */
> > +PageInfoWindow.prototype.__defineGetter__("category", function () {
> > +  var category = this.getElement({type: "panelsContainer"})
> > +                     .getNode().selectedPanel.id.split("Panel")[0];
> 
> Please get the element first, and then operate on it.
> 

Fixed.

> @@ +58,5 @@
> > +  var selectFlag = false;
> > +  var onSel = function() { selectFlag = true; };
> > +
> > +  try {
> > +    doc.addEventListener("select", onSel);
> 
> Please attach event listeners to the window and not the document. Or why
> does the window not work? You would like to comment this if its handled
> differently than usual. Also why is that part of the try block?
> 

Changed to attach the event on window, moved the add event listener outside the try block.

> @@ +63,5 @@
> > +  }
> > +  finally {
> > +    cat.waitThenClick();
> > +
> > +    assert.waitFor(() => selectFlag, "Select event has been fired");
> 
> The above should all be in the try block, right?
> 

Moved inside the try block

> 
> ::: lib/windows.js
> @@ +188,5 @@
> >   *        The expected state of the window to wait for
> >   * @param {string} [aSpec.type]
> >   *        Type of the window
> > + * @param {boolean} [aSpec.eventType]
> > + *        Event type to listen for
> 
> This is optional with a default. Also it is an observer and not an event.
> Please adjust that, including 'observer notification to wait for'.

Changed eventType to observer.

Comment 42

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8530146 [details] [diff] [review]
v16.patch

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

Looks good. Just a comment.

::: lib/windows.js
@@ +188,5 @@
>   *        The expected state of the window to wait for
>   * @param {string} [aSpec.type]
>   *        Type of the window
> + * @param {string} [aSpec.observer]
> + *        (Optional) Observer notification to wait for

The [] brackets on the name already are stating this is optional.

Comment 43

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

Comment on attachment 8530146 [details] [diff] [review]
v16.patch

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

::: firefox/lib/ui/page-info.js
@@ +35,5 @@
> + */
> +PageInfoWindow.prototype.__defineGetter__("category", function () {
> +  var panel = this.getElement({type: "panelsContainer"});
> +  var category = panel.getNode().selectedPanel.id.split("Panel")[0];
> +  return category;

Please remove the unnecessary declaration of category and return directly.

@@ +47,5 @@
> + */
> +PageInfoWindow.prototype.__defineSetter__("category", function (aCategory) {
> +  assert.equal(typeof aCategory, "string", "tab has been defined");
> +  var category = this.getElement({type:  "category", subtype: aCategory});
> +  var win = this._controller.window;

I would suggest we make use of it directly below in the two cases.

@@ +55,5 @@
> +    return;
> +  }
> +
> +  var selectFlag = false;
> +  var onSelect = function() { selectFlag = true; };

nit: missing blank

@@ +68,5 @@
> +  }
> +
> +  assert.waitFor(() => (this.category === aCategory),
> +                 "Category has been selected expected: " + aCategory +
> +                 ", received: " + this.category);

As we know there is no way to check for the final value. You can only reference the expected value.

@@ +96,5 @@
> +    case "panelsContainer":
> +      elems = [findElement.ID(root, "mainDeck")];
> +      break;
> +    case "categories":
> +      var nodeCollector = new domUtils.nodeCollector(findElement.ID(root, "viewGroup").getNode());

Same as in an earlier review. Declare the element first, it's too hard to read otherwise.

::: lib/windows.js
@@ +188,5 @@
>   *        The expected state of the window to wait for
>   * @param {string} [aSpec.type]
>   *        Type of the window
> + * @param {string} [aSpec.observer]
> + *        (Optional) Observer notification to wait for

Yes, please update as Andreea said.

Comment 44

[Teodor Druta]

Attachment: v17.patch

(In reply to Henrik Skupin (:whimboo) from comment #43)
> Comment on attachment 8530146 [details] [diff] [review]
> v16.patch
> 
> Review of attachment 8530146 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: firefox/lib/ui/page-info.js
> @@ +35,5 @@
> > + */
> > +PageInfoWindow.prototype.__defineGetter__("category", function () {
> > +  var panel = this.getElement({type: "panelsContainer"});
> > +  var category = panel.getNode().selectedPanel.id.split("Panel")[0];
> > +  return category;
> 
> Please remove the unnecessary declaration of category and return directly.
> 

Fixed.

> @@ +47,5 @@
> > + */
> > +PageInfoWindow.prototype.__defineSetter__("category", function (aCategory) {
> > +  assert.equal(typeof aCategory, "string", "tab has been defined");
> > +  var category = this.getElement({type:  "category", subtype: aCategory});
> > +  var win = this._controller.window;
> 
> I would suggest we make use of it directly below in the two cases.
> 

Fixed.

> 
> @@ +68,5 @@
> > +  }
> > +
> > +  assert.waitFor(() => (this.category === aCategory),
> > +                 "Category has been selected expected: " + aCategory +
> > +                 ", received: " + this.category);
> 
> As we know there is no way to check for the final value. You can only
> reference the expected value.
> 

Removed 'received: " + this.category'.

> @@ +96,5 @@
> > +    case "panelsContainer":
> > +      elems = [findElement.ID(root, "mainDeck")];
> > +      break;
> > +    case "categories":
> > +      var nodeCollector = new domUtils.nodeCollector(findElement.ID(root, "viewGroup").getNode());
> 
> Same as in an earlier review. Declare the element first, it's too hard to
> read otherwise.
> 

Fixed.

Fixed all nits, added empty lines where it applied.
Removed the unnecessary "(Optional)"

Comment 45

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8533765 [details] [diff] [review]
v17.patch

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

Just a thing missing. With that please request from Henrik again.

::: firefox/lib/ui/page-info.js
@@ +53,5 @@
> +    return;
> +  }
> +
> +  var selectFlag = false;
> +  var onSelect = function() { selectFlag = true; };

Still missed the blank space Henrik requested here, "function () {"

Comment 46

[Teodor Druta]

Attachment: v17.1.patch

Sorry I missed that one, added the blank space.

Comment 47

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

Comment on attachment 8535473 [details] [diff] [review]
v17.1.patch

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

By fixing the last two minor things as pointed out inline, you get my r+! Thanks a lot for working on that patch.

::: firefox/lib/ui/page-info.js
@@ +55,5 @@
> +
> +  var selectFlag = false;
> +  var onSelect = function () { selectFlag = true; };
> +
> +  this._controller.window.addEventListener("select", onSelect);

When we have a getter please make use of it instead of the internal variable.

@@ +66,5 @@
> +    this._controller.window.removeEventListener("select", onSelect);
> +  }
> +
> +  assert.waitFor(() => (this.category === aCategory),
> +                 "Category has been selected, expected: " + aCategory);

What about: Category 'XYZ' has been selected?

Comment 48

[Teodor Druta]

Attachment: v18.patch

Thanks for the review, Henrik !

Changed this._controller to this.controller in category getter method and in getElements method.

Changed the message on line 70 in page-info.js to "Category 'XYZ' has been selected".

Comment 49

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8536490 [details] [diff] [review]
v18.patch

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

http://hg.mozilla.org/qa/mozmill-tests/rev/0cd48a09b140 (default)

Comment 50

[Teodor Druta]

Comment on attachment 8536490 [details] [diff] [review]
v18.patch

The patch applies cleanly and runs correctly on mozilla-beta and mozilla-aurora.

Comment 51

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8536490 [details] [diff] [review]
v18.patch

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

Please update the patch.

Comment 52

[Teodor Druta]

Attachment: pageinfoaurora.patch

The previous patch applies on mozilla-beta, this is for mozilla-aurora.

Comment 53

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8537154 [details] [diff] [review]
pageinfoaurora.patch

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

http://hg.mozilla.org/qa/mozmill-tests/rev/fb2499ccb881 (aurora)

Comment 54

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8536490 [details] [diff] [review]
v18.patch

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

I'll land this tomorrow on beta.

Comment 55

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8536490 [details] [diff] [review]
v18.patch

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

http://hg.mozilla.org/qa/mozmill-tests/rev/69c78d8d2bed (beta)