Add DownloadsPanel class to toolbars.js

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Daniel Gherasim, Assigned: Daniel Gherasim)

Tracking

unspecified

Firefox Tracking Flags

(firefox32 wontfix, firefox33 wontfix, firefox34 fixed, firefox35 fixed, firefox36 fixed, firefox-esr31 wontfix)

Details

(Whiteboard: [lib][lang=js][sprint])

Attachments

(2 attachments, 11 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8503043 [details] [diff] [review]
v1.patch

Class added.
Attachment #8503043 - Flags: review?(andreea.matei)
(Assignee)

Updated

3 years ago
Attachment #8503043 - Flags: review?(andreea.matei) → review?(andrei.eftimie)
(Assignee)

Comment 2

3 years ago
Created attachment 8503095 [details] [diff] [review]
v1.patch

Thanks Andreea. Asking Henrik for a final look.
Attachment #8503043 - Attachment is obsolete: true
Attachment #8503043 - Flags: review?(andrei.eftimie)
Attachment #8503043 - Flags: review?(andreea.matei)
Attachment #8503095 - Flags: review?(hskupin)
(Assignee)

Comment 3

3 years ago
Created attachment 8503097 [details] [diff] [review]
v1.patch

Oups, wrong bug, sorry!
Attachment #8503095 - Attachment is obsolete: true
Attachment #8503095 - Flags: review?(hskupin)
Attachment #8503097 - Flags: review?(andrei.eftimie)
Attachment #8503097 - Flags: review?(andreea.matei)
(Assignee)

Comment 4

3 years ago
Created attachment 8503942 [details] [diff] [review]
v1.1.patch

Updated with some changes which are similar and were requested in bug 1079710.
Attachment #8503097 - Attachment is obsolete: true
Attachment #8503097 - Flags: review?(andrei.eftimie)
Attachment #8503097 - Flags: review?(andreea.matei)
Attachment #8503942 - Flags: review?(andrei.eftimie)
Attachment #8503942 - Flags: review?(andreea.matei)

Comment 5

3 years ago
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 ;
Attachment #8503942 - Flags: review?(andrei.eftimie)
Attachment #8503942 - Flags: review?(andreea.matei)
Attachment #8503942 - Flags: review-
(Assignee)

Comment 6

3 years ago
(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)
(Assignee)

Comment 7

3 years ago
Created attachment 8504082 [details] [diff] [review]
v1.2.patch
Attachment #8503942 - Attachment is obsolete: true
Attachment #8504082 - Flags: review?(andrei.eftimie)
Attachment #8504082 - Flags: review?(andreea.matei)

Comment 8

3 years ago
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
Attachment #8504082 - Flags: review?(andrei.eftimie)
Attachment #8504082 - Flags: review?(andreea.matei)
Attachment #8504082 - Flags: review-
Whiteboard: [lib][lang=js] → [lib][lang=js][sprint]
(Assignee)

Comment 9

3 years ago
(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 ...).
Whiteboard: [lib][lang=js][sprint] → [lib][lang=js]
(Assignee)

Updated

3 years ago
Whiteboard: [lib][lang=js] → [lib][lang=js][sprint]

Comment 10

3 years ago
(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.
(Assignee)

Comment 11

3 years ago
Created attachment 8504704 [details] [diff] [review]
v2.patch

Thanks!
Passed for a final review.
Attachment #8504082 - Attachment is obsolete: true
Attachment #8504704 - Flags: review?(hskupin)
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.
Attachment #8504704 - Flags: review?(hskupin) → review-
(Assignee)

Comment 13

3 years ago
Created attachment 8510296 [details] [diff] [review]
v3.patch

Discussed with Henrik about the final implementation on IRC.
Here is the patch.
Attachment #8504704 - Attachment is obsolete: true
Attachment #8510296 - Flags: review?(andrei.eftimie)
Attachment #8510296 - Flags: review?(andreea.matei)
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.
Attachment #8510296 - Flags: review?(andrei.eftimie)
Attachment #8510296 - Flags: review?(andreea.matei)
Attachment #8510296 - Flags: review+
(Assignee)

Comment 15

3 years ago
Created attachment 8510326 [details] [diff] [review]
v3.1.patch
Attachment #8510296 - Attachment is obsolete: true
Attachment #8510326 - Flags: review?(hskupin)
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.
Attachment #8510326 - Flags: review?(hskupin) → review-
(Assignee)

Comment 17

3 years ago
(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.
(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.
(Assignee)

Comment 19

3 years ago
(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.
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?
(Assignee)

Comment 21

3 years ago
(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 ?
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.
(Assignee)

Comment 23

3 years ago
(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).
We should know the window given that the MozElement has a reference to it.
(Assignee)

Comment 25

3 years ago
Created attachment 8511066 [details] [diff] [review]
v4.patch

Thanks a lot Henrik. Updated as we discussed.
Hope now all is fine.
Attachment #8510326 - Attachment is obsolete: true
Attachment #8511066 - Flags: review?(hskupin)
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.
Attachment #8511066 - Flags: review?(hskupin) → review-
(Assignee)

Comment 27

3 years ago
So with all those 3 changes we're ok ? Or why r- ?
(Assignee)

Comment 28

3 years ago
Created attachment 8512593 [details] [diff] [review]
v4.1.patch

Anyway all review notes addressed now.
Attachment #8511066 - Attachment is obsolete: true
Attachment #8512593 - Flags: review?(andrei.eftimie)
Attachment #8512593 - Flags: review?(andreea.matei)

Comment 29

3 years ago
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.
Attachment #8512593 - Flags: review?(andrei.eftimie)
Attachment #8512593 - Flags: review?(andreea.matei)
Attachment #8512593 - Flags: review+
(Assignee)

Comment 30

3 years ago
Created attachment 8513415 [details] [diff] [review]
v5.patch
Attachment #8512593 - Attachment is obsolete: true
Attachment #8513415 - Flags: review?(hskupin)
Attachment #8513415 - Flags: review?(hskupin) → review+
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?
Attachment #8513415 - Flags: checkin+

Comment 34

3 years ago
Transplanted to Aurora:
https://hg.mozilla.org/qa/mozmill-tests/rev/db9c8bda3471 (mozilla-aurora)
status-firefox32: affected → wontfix
status-firefox35: affected → fixed
status-firefox36: --- → fixed

Comment 35

3 years ago
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.
status-firefox34: affected → fixed

Comment 36

3 years ago
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)
Attachment #8514221 - Flags: review?(andrei.eftimie)
Attachment #8514221 - Flags: review?(andreea.matei)
Attachment #8514221 - Flags: review+
Attachment #8514221 - Flags: checkin+

Updated

3 years ago
status-firefox33: affected → fixed

Comment 37

3 years ago
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)
status-firefox33: fixed → wontfix
status-firefox-esr31: affected → wontfix

Comment 38

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

We can't use this on ESR31 as we're missing the BaseWindow class (34+)
Attachment #8514253 - Attachment is obsolete: true
Attachment #8514253 - Flags: review?(andrei.eftimie)
Attachment #8514253 - Flags: review?(andreea.matei)

Comment 39

3 years ago
We're done here.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.