1081024 - Add DownloadsPanel class to toolbars.js

Status: RESOLVED FIXED

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

Description

[Daniel Gherasim]

We added it in bug 908649 but let's remove it from there and add it as separate bug.

It's not depending on that or on where the other patch will land.

I was working on that bug, most of the part it's done, just a bit of refactoring to the lib test.

Comment 1

[Daniel Gherasim]

Attachment: v1.patch

Class added.

Comment 2

[Daniel Gherasim]

Attachment: v1.patch

Thanks Andreea. Asking Henrik for a final look.

Comment 3

[Daniel Gherasim]

Attachment: v1.patch

Oups, wrong bug, sorry!

Comment 4

[Daniel Gherasim]

Attachment: v1.1.patch

Updated with some changes which are similar and were requested in bug 1079710.

Comment 5

[Andrei Eftimie]

Comment on attachment 8503942 [details] [diff] [review]
v1.1.patch

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

::: firefox/lib/tests/testDownloadsPanel.js
@@ +7,5 @@
> +Cu.import("resource://gre/modules/Downloads.jsm");
> +
> +// Include required modules
> +var toolbars = require("../toolbars");
> +

nit: not sure we need this empty line here

@@ +39,5 @@
> +  var openButton = downloadsPanel.getElement({type: "openButton"});
> +  toolbars.waitForNotificationPanel(() => {
> +    openButton.click();
> +  }, {panel: downloadsPanel.panel,
> +      parent: downloadsPanel.browserWindow.controller.window});

I'm not a fan of this indentation, makes everything hard to follow, but I don't have a proper solution... maybe extend everything?
> toolbars.waitForNotificationPanel(
>   () => openButton.click(),
>   {
>     panel: downloadsPanel.panel,
>     parent: downloadsPanel.browserWindow.controller.window
>   }
> );
I'm open to alternatives.

@@ +51,5 @@
> +  });
> +
> +  toolbars.waitForNotificationPanel(() => {
> +    downloadsPanel.panel.keypress("VK_ESCAPE", {});
> +  }, {panel: downloadsPanel.panel, open: false});

Should we get a close method on the panel?
So instead of this we can directly call:
> downloadsPanel.close()

::: firefox/lib/toolbars.js
@@ +245,5 @@
> + * @constructor
> + * @param {object} aBrowserWindow
> + *        Browser window where the downloads panel is
> + */
> +function DownloadsPanel(aBrowserWindow) {

None of the other classes in this file are capitalised. For consistency I'd leave the name `downloadsPanel`
(unless there's a plan to also change the others)

@@ +276,5 @@
> +   *
> +   * @returns {MozMillElement} The downloads panel
> +   */
> +  get panel() {
> +    return this.getElement({type: "panel"})

nit: missing ;

Comment 6

[Daniel Gherasim]

(In reply to Andrei Eftimie from comment #5)
> Comment on attachment 8503942 [details] [diff] [review]
> v1.1.patch
> 
> Review of attachment 8503942 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: firefox/lib/tests/testDownloadsPanel.js
> @@ +7,5 @@
> > +Cu.import("resource://gre/modules/Downloads.jsm");
> > +
> > +// Include required modules
> > +var toolbars = require("../toolbars");
> > +
> 
> nit: not sure we need this empty line here
> 

That's something Henrik requested to separate back-end and ui modules in tests.

> @@ +39,5 @@
> > +  var openButton = downloadsPanel.getElement({type: "openButton"});
> > +  toolbars.waitForNotificationPanel(() => {
> > +    openButton.click();
> > +  }, {panel: downloadsPanel.panel,
> > +      parent: downloadsPanel.browserWindow.controller.window});
> 
> I'm not a fan of this indentation, makes everything hard to follow, but I
> don't have a proper solution... maybe extend everything?
> > toolbars.waitForNotificationPanel(
> >   () => openButton.click(),
> >   {
> >     panel: downloadsPanel.panel,
> >     parent: downloadsPanel.browserWindow.controller.window
> >   }
> > );
> I'm open to alternatives.
> 

I would keep it as it is given that that is how we actually did indentation untill now (check the other waitForNotificationPanel we have)

Comment 7

[Daniel Gherasim]

Attachment: v1.2.patch

Comment 8

[Andrei Eftimie]

Comment on attachment 8504082 [details] [diff] [review]
v1.2.patch

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

Just 2 small things, otherwise looks good to me.

::: firefox/lib/tests/testDownloadsPanel.js
@@ +13,5 @@
> +
> +const TEST_DATA = {
> +  url: "ftp://ftp.mozilla.org/pub/firefox/releases/3.6/mac/en-US/Firefox%203.6.dmg",
> +  elements: [
> +    {name: "openButton", type: "toolbarbutton"},

You could simplify this a bit by using key/values directly on an object:
> elements: {
>   openButton: "toolbarbutton",
>   [...]
> }

::: firefox/lib/toolbars.js
@@ +410,5 @@
> +
> +    if (aSpec.force && panel.getNode()) {
> +      panel.getNode().hidePopup();
> +      return;
> +    }

Since this has a closing animation, the forced close should also be included in the waitForNotificationPanel

Comment 9

[Daniel Gherasim]

(In reply to Andrei Eftimie from comment #8)
> Review of attachment 8504082 [details] [diff] [review]:
> -----------------------------------------------------------------
> You could simplify this a bit by using key/values directly on an object:
> > elements: {
> >   openButton: "toolbarbutton",
> >   [...]
> > }
> 

This is actually how we already do in all the tests.
Should we start changing this ? That will actually mean also replacing forEach with for (var i in ...).

Comment 10

[Andrei Eftimie]

(In reply to daniel.gherasim from comment #9)
> (In reply to Andrei Eftimie from comment #8)
> > Review of attachment 8504082 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > You could simplify this a bit by using key/values directly on an object:
> > > elements: {
> > >   openButton: "toolbarbutton",
> > >   [...]
> > > }
> > 
> 
> This is actually how we already do in all the tests.
> Should we start changing this ? That will actually mean also replacing
> forEach with for (var i in ...).

There was a recent comment by Henrik in one review asking for this. I am in agreement with it.
I guess it might depend on the test / context... but yeah, if the object is simple, I'd go for this route instead of an array.

Comment 11

[Daniel Gherasim]

Attachment: v2.patch

Thanks!
Passed for a final review.

Comment 12

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

Comment on attachment 8504704 [details] [diff] [review]
v2.patch

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

Looks clean and shiny. Love to see it included soon. So lets get the remaining items fixed.

::: firefox/lib/tests/testDownloadsPanel.js
@@ +17,5 @@
> +    openButton: "toolbarbutton",
> +    panel: "panel",
> +    showAllDownloads: "button"
> +  }
> +}

nit: missing semicolon.

@@ +27,5 @@
> +
> +function teardownModule(aModule) {
> +  if (downloadsPanel.panel.getNode()) {
> +    downloadsPanel.panel.getNode().hidePopup();
> +  }

Use downloadsPanel.close() with force set to true.

@@ +41,5 @@
> +    openButton.click();
> +  }, {panel: downloadsPanel.panel,
> +      parent: downloadsPanel.browserWindow.controller.window});
> +
> +  expect.ok(downloadsPanel.isOpen, "Downloads panel is open");

You should add an open() method to the class similar to close.

::: firefox/lib/toolbars.js
@@ +404,5 @@
> +   *         Force closing the Downloads Panel
> +   *
> +   */
> +  close : function downloadsPanel_close(aSpec={}) {
> +    var panel = this.getElement({type: "panel"});

You can use `this.panel` here.

@@ +416,5 @@
> +
> +      switch (method) {
> +        case "callback":
> +          assert.equal(aSpec.callback, "function",
> +                       "Callback function has been defined");

nit: A callback is a function. So no need to explicitly mention `function` here.

Comment 13

[Daniel Gherasim]

Attachment: v3.patch

Discussed with Henrik about the final implementation on IRC.
Here is the patch.

Comment 14

[Andreea Matei [:AndreeaMatei]]

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

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

::: firefox/lib/toolbars.js
@@ +455,5 @@
> +          assert.equal(aSpec.callback, "function",
> +                       "Callback has been defined");
> +          aSpec.callback();
> +          break;
> +        case "button":

Also here we should have button before callback. Otherwise I see it has all requested changes, please add review for Henrik with this fixed.

Comment 15

[Daniel Gherasim]

Attachment: v3.1.patch

Comment 16

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

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

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

::: firefox/lib/tests/testDownloadsPanel.js
@@ +30,5 @@
> + */
> +function testDownloadPanel() {
> +  downloadsPanel.open();
> +
> +  expect.ok(downloadsPanel.isOpen, "Downloads panel is open");

nit: no need for the extra blank line.

::: firefox/lib/toolbars.js
@@ +240,5 @@
>  }
>  
>  /**
> + * Downloads Panel class
> + * Implemented in bug 1081024

No need for this line. Please remove it.

@@ +407,5 @@
> +   *
> +   */
> +  close : function DownloadsPanel_close(aSpec={}) {
> +    var panel = this.panel;
> +    var method = aSpec.method || "shortcut";

Please check parameters first before declaring other variables.

@@ +440,5 @@
> +   *
> +   * @params {object} [aSpec={}]
> +   *         Information on how to open the panel
> +   * @params {string} [aSpec.method="button"]
> +   *         Method to use to open the downloads panel ("callback"|"button")

nit: please fix the sort order of the possible options.

@@ +446,5 @@
> +   *         Callback that triggeres the download panel to open
> +   */
> +  open : function DownloadsPanel_waitForState(aSpec={}) {
> +    var panel = this.panel;
> +    var method = aSpec.method || "button";

Same as above.

@@ +462,5 @@
> +        default:
> +          assert.fail("Unknown method to open the downloads panel - " + method);
> +      }
> +    }, {panel: panel, open: true,
> +        parent: this._browserWindow.controller.window});

Why the window and not this.browserWindow.controller.window.document which is used by default? Something looks wrong here.

Also we should check if the panel is already open before executing this code.

Comment 17

[Daniel Gherasim]

(In reply to Henrik Skupin (:whimboo) from comment #16)
> Comment on attachment 8510326 [details] [diff] [review]
> v3.1.patch
> 
> Review of attachment 8510326 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +462,5 @@
> > +        default:
> > +          assert.fail("Unknown method to open the downloads panel - " + method);
> > +      }
> > +    }, {panel: panel, open: true,
> > +        parent: this._browserWindow.controller.window});
> 
> Why the window and not this.browserWindow.controller.window.document which
> is used by default? Something looks wrong here.
> 

Adding the listeners to both "[...].window" and "[...].window.document" works in this case.
So I'll use the second option ok ?

> Also we should check if the panel is already open before executing this code.

I will do the same as we discussed on the Menu Panel.

Comment 18

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

(In reply to daniel.gherasim from comment #17)
> Adding the listeners to both "[...].window" and "[...].window.document"
> works in this case.
> So I'll use the second option ok ?

This is the default root in getElements. So you can remove this line.

> > Also we should check if the panel is already open before executing this code.
> 
> I will do the same as we discussed on the Menu Panel.

Sounds good.

Comment 19

[Daniel Gherasim]

(In reply to Henrik Skupin (:whimboo) from comment #18)
> (In reply to daniel.gherasim from comment #17)
> > Adding the listeners to both "[...].window" and "[...].window.document"
> > works in this case.
> > So I'll use the second option ok ?
> 
> This is the default root in getElements. So you can remove this line.
> 

Not sure what this has to do with getElements.
In waitForNotificaitonPanel we check if the panel is initialized, otherwise we assert for a parent.
So parent is needed here.

Comment 20

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

Oh! I mixed something up. Thanks for the clarification!

So I wonder if we really have to specify a parent here if the panel does not exist yet. In such cases we should always fallback to the browser window itself, because the event is bubbling up. Do you see a need for a specific parent here?

Comment 21

[Daniel Gherasim]

(In reply to Henrik Skupin (:whimboo) from comment #20)
> Oh! I mixed something up. Thanks for the clarification!
> 
> So I wonder if we really have to specify a parent here if the panel does not
> exist yet. In such cases we should always fallback to the browser window
> itself, because the event is bubbling up. Do you see a need for a specific
> parent here?

Hmm, I would offer the posibility to specify a parent if the panel is not initialized and we know it's parent. Otherwise maybe instead of failing, we should set the parent to the browser window by default in waitForNotificationPanel ?

Comment 22

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

Sure, if the panel doesn't exist yet we need another element. But given that events are bubbling up, we should always use the browser window IMO. If we would need a very specific parent we could leave it as param and use it if specified. So if panel exists, use the panel. If not use the browser window, unless parent has been specified.

Comment 23

[Daniel Gherasim]

(In reply to Henrik Skupin (:whimboo) from comment #22)
> Sure, if the panel doesn't exist yet we need another element. But given that
> events are bubbling up, we should always use the browser window IMO. If we
> would need a very specific parent we could leave it as param and use it if
> specified. So if panel exists, use the panel. If not use the browser window,
> unless parent has been specified.

Oh wait! We don't know the browser window from where we are trying to open the window in waitForNotificationPanel :(. Maybe we should add aBrowserWindow as parameter here or keep with the implementation we currently have (and require parent if panel is not initialized).

Comment 24

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

We should know the window given that the MozElement has a reference to it.

Comment 25

[Daniel Gherasim]

Attachment: v4.patch

Thanks a lot Henrik. Updated as we discussed.
Hope now all is fine.

Comment 26

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

Comment on attachment 8511066 [details] [diff] [review]
v4.patch

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

::: firefox/lib/toolbars.js
@@ +379,5 @@
> +      case "openButton":
> +        elems = [findElement.ID(root, "downloads-button")];
> +        break;
> +      case "panel":
> +        elems = [new elementslib.ID(root, "downloadsPanel")];

Why have you reverted this to elementslib.ID? This should stay with findElement.ID().

@@ +406,5 @@
> +   *
> +   */
> +  close : function DownloadsPanel_close(aSpec={}) {
> +    var method = aSpec.method || "shortcut";
> +    var panel = this.panel;

As mentioned in my last review I don't see why we need panel being defined. We can always use this.panel, because when using => the `this` object doesn't point to the closure but the surrounding object.

@@ +1103,5 @@
>        expect.equal(spec.panel.getNode().state, "closed",
>                     "Panel is in the correct state");
>      }
>      else {
> +      spec.parent = spec.parent || spec.panel._defaultView;

Please update the jsdoc for the default.

Comment 27

[Daniel Gherasim]

So with all those 3 changes we're ok ? Or why r- ?

Comment 28

[Daniel Gherasim]

Attachment: v4.1.patch

Anyway all review notes addressed now.

Comment 29

[Andrei Eftimie]

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

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

Needs an update it seems:
> patching file firefox/lib/toolbars.js
> Hunk #4 FAILED at 1126
> 1 out of 4 hunks FAILED -- saving rejects to file firefox/lib/toolbars.js.rej

Otherwise lgtm. Please ask a review from Henrik with an update so this applies cleanly.

Comment 30

[Daniel Gherasim]

Attachment: v5.patch

Comment 31

[Andreea Matei [:AndreeaMatei]]

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

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

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/20e9cdaac3d8 (default)
Thanks a lot Daniel!

Teodor, can you check backporting here please?

Comment 32

[Teodor Druta]

Attachment: v5release.patch

The patch applies well on mozilla-aurora and no failures:
http://mozmill-crowd.blargon7.com/#/functional/report/24f9dc1b051c99c8247bed4e7837fa28
http://mozmill-crowd.blargon7.com/#/endurance/report/24f9dc1b051c99c8247bed4e78385c69
http://mozmill-crowd.blargon7.com/#/remote/report/24f9dc1b051c99c8247bed4e78376586

Same on mozilla-beta:
http://mozmill-crowd.blargon7.com/#/functional/report/24f9dc1b051c99c8247bed4e78396515
http://mozmill-crowd.blargon7.com/#/endurance/report/24f9dc1b051c99c8247bed4e7839fa7a
http://mozmill-crowd.blargon7.com/#/remote/report/24f9dc1b051c99c8247bed4e78399cf4

It doesn't apply on mozilla-release branch, I have attached a patch.
It runs well:
http://mozmill-crowd.blargon7.com/#/functional/report/24f9dc1b051c99c8247bed4e7833a5d9
http://mozmill-crowd.blargon7.com/#/endurance/report/24f9dc1b051c99c8247bed4e7834adfa
http://mozmill-crowd.blargon7.com/#/remote/report/24f9dc1b051c99c8247bed4e78362820

Comment 33

[Teodor Druta]

Attachment: v5esr31.patch

Attached a patch for mozilla-esr31 branch:
http://mozmill-crowd.blargon7.com/#/functional/report/24f9dc1b051c99c8247bed4e783de2d6
http://mozmill-crowd.blargon7.com/#/endurance/report/24f9dc1b051c99c8247bed4e783ee33d
http://mozmill-crowd.blargon7.com/#/remote/report/24f9dc1b051c99c8247bed4e783d0225

Comment 34

[Andrei Eftimie]

Transplanted to Aurora:
https://hg.mozilla.org/qa/mozmill-tests/rev/db9c8bda3471 (mozilla-aurora)

Comment 35

[Andrei Eftimie]

Low risk since it doesn't change almost anything for existing tests.
Testruns all green, so let's get this in lower branches as well:

Transplanted to beta:
https://hg.mozilla.org/qa/mozmill-tests/rev/c498283072c1 (mozilla-beta)

For the release branch I'd wait for the current runs for 33.1 #2 to finish (about 300 runs in the queue ATM). Will push it later today.

Comment 36

[Andrei Eftimie]

Comment on attachment 8514221 [details] [diff] [review]
v5release.patch

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

Landed:
https://hg.mozilla.org/qa/mozmill-tests/rev/7b618c926760 (mozilla-release)

Comment 37

[Andrei Eftimie]

We actually don't have the BrowserWindow class on release and esr31, so we can't actually use this implementation.

Backed out:
https://hg.mozilla.org/qa/mozmill-tests/rev/49818e5c9184 (mozilla-release)

Comment 38

[Andrei Eftimie]

Comment on attachment 8514253 [details] [diff] [review]
v5esr31.patch

We can't use this on ESR31 as we're missing the BaseWindow class (34+)

Comment 39

[Andrei Eftimie]

We're done here.