908649 - Refactor lib/downloads.js replacing the nsiDownloadManager backend with the new Downloads.jsm implementation

Status: RESOLVED FIXED

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

Description

[Andrei Eftimie]

nsiDownloadManager is deprecated and will be removed shortly
We rely on it in lib/downloads.js

The new Downloads.jsm implements a new and different API.

Some references
===============
[old] nsiDownloadManager:
https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIDownloadManager

[new] Downloads.jsm:
https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Downloads.jsm

Comment 1

[u279076]

Since Mihaela is our QA Contact for the Downloads.jsm refactor I'd like her to be heavily involved in this work. Please involve her in any Mozmill Tests work that needs to happen here.

Comment 2

[Andrei Eftimie]

I have initially tried to map our current downloads lib to the new API's but the difference is significant enough that this approach will not work. Most of the old lib does not make sense anymore.

To be able to ascertain what the library needs I might refactor affected download tests at the same time.

Comment 3

[Andrei Eftimie]

Attachment: api.txt

Current draft for the refactor.
Feedback appreciated, also send via the mailing list do dev-automation.

Comment 4

[Andrei Eftimie]

Well, the documentation is partly obsolete.
Downloads.jsm has been refactored, at least some of the available methods have been changed in bug 913118, but the changes are not yet refelected in the docs here:
https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Downloads.jsm

The methods `getPublicDownloadList()` and `getPrivateDownloadList()` have been replaced by `getList(type)` and we need to specify what type of downloads we want (public, private, all)

Comment 5

[Andrei Eftimie]

A small update:
- I have the download panel test working and checking downloads with the new Promis based API.
- I have moved the Panel itself in the toolbars lib
- as per bug 930509 I will spin off part of the old download lib into a library lib to handle the general Library window

I aim to have a patch uploaded at least by Monday (even if its not complete).

Comment 6

[Andrei Eftimie]

Attachment: a1.patch [WIP]

I have enough here so I can reenable the test /firefox/tests/functional/testPrivateBrowsing/testPrivateDownloadPanel.js

So lets get this out to gather some feedback.
This is a WIP patch.

- Download Panel is part of the toolbars lib
- Library lib which handles the Library Window
- test for Library lib // tests opening and closing the Library to all locations, and existance of some UI elements
- Downloads lib uses the new Downloads.jsm service
- test Downloads lib // this fails atm as the same download appears in the UI multiple times

There's still work needed here.
In regards to scope I think the Library should be covered only for our download needs here.
We will improve it once we need to write tests covering the other parts of the Library (History, Tags, Bookmarks)
The Download Manager still needs a few methods ported.
And overall comments

Comment 7

[Andrei Eftimie]

Attachment: a2.patch

Hi guys,
another update here.

I've cleaned it up, added comments where they were missing.
Lots of nit fixes. Properly arranged all methods.

I'm not sure about yet is how to properly separate Download API vs UI calls.
At the moment using the download lib this is not clear.

The testDownload.js test module ATM fails because of bug 959103

The Library is not complete in functionality. Is has the basics, some low hanging fruit, and what was needed for downloads. Most of History and Bookmarks is not treated. We should either have another bug for implementing this, or maybe better these should be implemented as part of testing those areas together with the required tests.

I would be happy to land this in a state close to what we have now so we can at least reenable 1 download test (after we fix any issues we have of course). But as said earlier without requiring us to fully implement the Library.

Feedback greatly appreciated since this patch now runs at 1.2K LOC

Comment 8

[Cosmin Malutan, [:cosmin-malutan]]

Comment on attachment 8359672 [details] [diff] [review]
a2.patch

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

I looks good to me. 
If this can get the test re-enabled I would suggest other enhancements to the library to be done in splited bugs.

::: firefox/lib/downloads.js
@@ +70,2 @@
>     */
> +  get library() {

You could make the library public and drop the getter.

@@ +207,3 @@
>      // If there is a Save File As radio, make sure it is selected
> +    var saveFileRadio = new elementslib.ID(aController.window.document, "save");
> +    dump('\n\n' + saveFileRadio + '\n\n');

I think you forgot this here.

::: firefox/lib/library.js
@@ +40,5 @@
> +   *
> +   * @returns {boolean}
> +   */
> +  get isOpen() {
> +    return this._isOpen;

If this is public, you could leave it public, there is no need to wrap it in a getter method.

@@ +80,5 @@
> +    var type = spec.type || "shortcut";
> +    var location = spec.location || "bookmarks";
> +    spec.accelKey = true;
> +    spec.shiftKey = false;
> +

You could check here if is already open and if so return early.

@@ +140,5 @@
> +      this.controller.window.close();
> +    }
> +    else {
> +      var cmdKey = utils.getEntity(this.dtds, "closeCmd.key");
> +      this.controller.keypress(null, cmdKey, {accelKey: true});

I would give as a first parameter controller.window instead of null. What happens if the focus is on a different window than the current modal?

@@ +213,5 @@
> +      case "downloadList":
> +        return [new elementslib.ID(this.controller.window.document, "downloadsRichListBox")];
> +      case "download":
> +        var downloadList = this.getElement({type: "downloadList"});
> +        return [downloadList.getItemAtIndex(spec.subType)];

space above the return.

Comment 9

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

Comment on attachment 8359672 [details] [diff] [review]
a2.patch

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

Good start but the class separation is not the well designed as you already mentioned. If you have problems with those things please start a discussion ahead of time, so we don't loose time in the implementation. f- mostly because of this.

::: firefox/lib/downloads.js
@@ +45,2 @@
>   */
> +function downloadManager(aController) {

This module should be the backend. Why do we need the controller passed in here?

@@ +45,4 @@
>   */
> +function downloadManager(aController) {
> +  this._controller = aController || mozmill.getBrowserController();
> +  this._library = new library.libraryManager(this._controller);

An API should never be dependent on UI code.

@@ +98,4 @@
>     */
> +  get dtds() {
> +    return ["chrome://browser/locale/browser.dtd",
> +            "chrome://mozapps/locale/downloads/downloads.dtd"];

UI code which is not related to an API.

@@ +108,4 @@
>     *        Download which state should be checked
>     */
> +  getDownloadState : function downloadManager_getDownloadState(aDownload) {
> +    return aDownload.getAttribute('state');

As best we should have a separate class for the download itself. It would better align the class structure to the underlying API classes.

@@ +128,3 @@
>     */
> +  getElement : function downloadManager_getElement(aSpec) {
> +    return this.library.getElement(aSpec);

This is an API and no UI module.

@@ +145,3 @@
>  
> +    this.library.close();
> +  },

This should be an API call to force cleaning the downloads. Otherwise a clean() method could be added to the library UI class.

@@ +162,2 @@
>     */
> +  getDownloadList : function downloadManager_getDownloadList(aType = "all") {

If we want to start using defaults, then do it like Python and kill the blanks around the equal sign.

::: firefox/lib/library.js
@@ +13,5 @@
> +
> +/**
> + * Library Window Constructor
> + */
> +function libraryManager() {

I would call it like the window type: PlacesOrganizer.

@@ +49,5 @@
> +   */
> +  get selectedItem() {
> +    var tree = this.getElement({type: "tree"}).getNode();
> +    return tree.view.nodeForTreeIndex(tree.currentIndex);
> +  },

Please see widgets.js under firefox/lib for tree operations. This mentioned module should really be moved under the top library folder and any method for trees added there.

@@ +58,5 @@
> +   * @returns URL's of external DTD files
> +   * @type {array of string}
> +   */
> +  get dtds() {
> +    return ["chrome://browser/locale/browser.dtd"];

Is there no DTD for the library window? I cannot believe that.

@@ +74,5 @@
> +   *        Controller from where to open the window
> +   */
> +  open: function libraryManager_open(aSpec, aController) {
> +    // Gather defaults
> +    var controller = aController || mozmill.getBrowserController();

I wouldn't make aController optional.

@@ +77,5 @@
> +    // Gather defaults
> +    var controller = aController || mozmill.getBrowserController();
> +    var spec = aSpec || {};
> +    var type = spec.type || "shortcut";
> +    var location = spec.location || "bookmarks";

I don't think we should force a location here. That would prevent us from opening the last selected pane.

@@ +79,5 @@
> +    var spec = aSpec || {};
> +    var type = spec.type || "shortcut";
> +    var location = spec.location || "bookmarks";
> +    spec.accelKey = true;
> +    spec.shiftKey = false;

Why that?

@@ +96,5 @@
> +        spec.cmdKey = utils.getEntity(this.dtds, "showAllHistoryCmd.commandkey");
> +        spec.menuItem = "#menu_showAllHistory";
> +        break;
> +      case "tags":
> +        throw new Error("Tags not supported yet directly");

I think all that should be better located in a browser class. We could augment e.g new methods to the MozmillController object.

@@ +116,5 @@
> +    }
> +
> +    var self = this;
> +    utils.handleWindow("type", "Places:Organizer", function (aController) {
> +      self._controller = aController;

to reduce confusion I would call this aPlacesController.

@@ +122,5 @@
> +    }, false);
> +
> +    assert.waitFor(function () {
> +      return self.isOpen;
> +    }, "Library window has been opened");

I don't see the reason for this waitFor call.

@@ +145,5 @@
> +    }
> +
> +    assert.waitFor(function () {
> +      return mozmill.utils.getWindows().length === (windowCount - 1);
> +    }, "Library Window has been closed");

Can we rely on the DOM event or an observer notification? I strongly feel so.

::: firefox/lib/tests/testDownloads.js
@@ +25,5 @@
>  var testOpenDownloadManager = function() {
>    downloads.downloadFileOfUnknownType(controller, TEST_DATA);
>  
>    // Open the download manager
> +  dm.library.open({ location: "downloads" });

I expect tests for the panel but not the library here.

::: firefox/lib/tests/testLibrary.js
@@ +12,5 @@
> +const TREE_PROPERTIES = {
> +  bookmarks: "OrganizerQueryAllBookmarks",
> +  downloads: "OrganizerQueryDownloads",
> +  history: "OrganizerQueryHistory",
> +  tags: "OrganizerQueryTags"

I'm not sure

@@ +32,5 @@
> +  // Test library elements
> +  library.open();
> +
> +  var backButton = library.getElement({ type: "backButton" });
> +  assert.ok(backButton.exists());

As for all the other libs please check for the localName. I mentioned that a couple of times in other reviews you should have seen.

@@ +61,5 @@
> + *        Where the Library Window should be open to
> + * @param {string} aTitleProperty
> + *        The title property of the requested tree item to be open to
> + */
> +function openCloseLibrary(aType, aLocation, aTitleProperty) {

Why do we need a separate method? Can't we do this in a loop in the test itself?

@@ +67,5 @@
> +  // Show All History on OSX doesn't work with a shortcut
> +  if (mozmill.isMac && aType === "shortcut" && aLocation === "history") return;
> +
> +  // Tags are currently not supported
> +  if (aLocation === "tags") return;

Please mark as todo on our side.

::: firefox/lib/toolbars.js
@@ +246,5 @@
> + *        MozMillController of the window to operate on
> + */
> +function downloadPanel(aController) {
> +  this._controller = aController;
> +  this.isOpen = false;

I don't see why we have to define this here.

@@ +275,5 @@
> +      return self._controller.window.top.DownloadsIndicatorView._operational;
> +    }, "Downloads Panel has been opened");
> +    // TODO: this is not reliable, we need another method to make sure
> +    // the Downloads Panel is loaded and shown
> +    this._controller.sleep(100);

You should kill this method and move the code into open(). I assume we have events we can wait for?

@@ +284,5 @@
> +   *
> +   * @returns {[type]} [description]
> +   */
> +  get downloads() {
> +    var itemList = this.getElement({ type: "itemList" }).element.childNodes;

You should get the childs directly and not using .childNodes. nodeCollector will work perfectly with this.

@@ +289,5 @@
> +    var downloadedFiles = [];
> +    for (var i = 0; i < itemList.length; i++) {
> +      downloadedFiles.push(itemList[i].attributes["target"].value)
> +    }
> +    return downloadedFiles;

Do the download entries match the download class from the API?

@@ +300,5 @@
> +   *
> +   * @returns {[type]} Download Object
> +   */
> +  getDownload : function panel_getDownload(aIndex) {
> +    return this.downloads[aIndex || 0];

Is 0 the first or most recently added download?

@@ +327,5 @@
> +   *        value: Value of the element or property
> +   * @returns Element which has been created
> +   * @type ElemBase
> +   */
> +  getElement: function downloadPanel_getElement(aSpec) {

getElements() please

@@ +353,5 @@
> +   */
> +  close: function() {
> +    var panel = this.getElement({ type: "panel" });
> +    panel.getNode().focus();
> +    panel.keypress("VK_ESCAPE");

What happens if is not open?

::: firefox/tests/functional/testPrivateBrowsing/testPrivateDownloadPanel.js
@@ +32,5 @@
>  
>    aModule.pbWindow = new privateBrowsing.PrivateBrowsingWindow();
>    aModule.pbWindow.open(aModule.controller);
>  
> +  aModule.dm = new downloads.downloadManager(aModule.controller);

I would expect to also see an instance of the download panel here.

@@ +71,5 @@
> +        aDownload.whenSucceeded().then(function (aSucceeded) {
> +          activeDownloads--;
> +        });
> +      });
> +    });

Any asynchronous call we have to hide in the libs. We shouldn't see any of those in the tests. With yield or waitFor it should work fine.

Comment 11

[Daniel Gherasim]

As we have discussed, I'll continue working on this.

Comment 12

[Daniel Gherasim]

So given on what Andrei & Henrik suggested, I purpose to have this structure in our code :

> lib/downloads.js //  Here we can handle the Downloads.jsm & other backend related code
* Downloads
*   getDownloadList()
*   set/get/reset download directory (the prefs)
*   clearAllDownloads() // an API force clearing downloads


Should this live under lib/ or under firefox/lib ?

> firefox/lib/ui/places-organizer.js
* PlacesOrganier
* ^ Downloads
* ^ [History] // when we will need it
* ^ [All Bookmarks] // same

Only PlacesOrganizer will be exported.

> firefox/lib/toolbars.js
* LocationBar
* ^ DownloadPanel
This will pe added to already existing locationBar class.

Comment 13

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

(In reply to daniel.gherasim from comment #12)
> > lib/downloads.js //  Here we can handle the Downloads.jsm & other backend related code
> * Downloads
> *   getDownloadList()
> *   set/get/reset download directory (the prefs)
> *   clearAllDownloads() // an API force clearing downloads
> 
> 
> Should this live under lib/ or under firefox/lib ?

When the download manager is under toolkit in bugzilla and hg, then it is shared. So put it under /lib in such a case.

> > firefox/lib/ui/places-organizer.js
> * PlacesOrganier
> * ^ Downloads
> * ^ [History] // when we will need it
> * ^ [All Bookmarks] // same

We should not differentiate between classes here. All is a huge tree. We might want to have helper methods to navigate, retrieve, and set entries.

> > firefox/lib/toolbars.js
> * LocationBar
> * ^ DownloadPanel
> This will pe added to already existing locationBar class.

No, this is not a child of the location bar. So it should get its own class.

Comment 14

[Andrei Eftimie]

What Henrik said sounds good.

(In reply to daniel.gherasim from comment #12)
> > firefox/lib/toolbars.js
> * LocationBar
> * ^ DownloadPanel
> This will pe added to already existing locationBar class.

Note that the Download Panel is currently being worked on, see the tracking bug 963745

Comment 15

[Daniel Gherasim]

Comments:

(In reply to Henrik Skupin (:whimboo) from comment #9)
> Comment on attachment 8359672 [details] [diff] [review]
> 
> @@ +96,5 @@
> > +        spec.cmdKey = utils.getEntity(this.dtds, "showAllHistoryCmd.commandkey");
> > +        spec.menuItem = "#menu_showAllHistory";
> > +        break;
> > +      case "tags":
> > +        throw new Error("Tags not supported yet directly");
> 
> I think all that should be better located in a browser class. We could
> augment e.g new methods to the MozmillController object.
> 

Should I create a new browser.js just for opening and closing this window ?
I see those 2 methods (open and close) more likely in the PlacesOrganizer class.

Comment 16

[Daniel Gherasim]

Attachment: WIP.patch

WIP patch and what I'm asking here is just structure related feedback, not specific code issues as I still have to fix previous review requested changes.

So given the latest requested changes and refactoring I have created this structure of code:

> firefox/lib/ui/places-organizer.js
* PlacesOrganizer class

> firefox/lib/ui/save-file-dialog.js
* SafeFileWindow class

> firefox/lib/browser.js
* function openLibrary(aSpec, aCallback)
* function closeLibrary(aController, aForce)

> firefox/lib/toolbars.js
* Added the DownloadPanel Class.

> lib/downloads.js
* Handling Downloads.jsm calls

With my WIP patch I can get enabled 2 tests : 
> firefox/tests/functional/testDownloading/testOpenDownloadManager.js
> firefox/tests/functional/testPrivateBrowsing/testPrivadeDownloadPanel.js

What I suggest here is that we split this patch into 2, in this bug we can continue working on the specific download parts (SaveFileWindow, DownloadPanel, downloads.js), and in another patch we can create the PlacesOrganize Patch and create new tests for it's functionality (open, close using menu or shortcuts ).

In this case this 2 tests : testDownloading/testCloseDownloadManager.js & testDownloading/testOpenDownloadManager.js would be removed or will handle 'about:downloads' as we will create testOpenPlacesOrganizer && testClosePlacesOrganizer where we can handle opening it with different locations setted too.

I will gladly like a feedback on this from Henrik & Andrei who put a big effort on this bug.

Comment 17

[Daniel Gherasim]

Comment on attachment 8406219 [details] [diff] [review]
WIP.patch

Sorry, feedback flag fixed.

Comment 18

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

(In reply to daniel.gherasim from comment #16)
> > firefox/lib/ui/places-organizer.js
> * PlacesOrganizer class
> 
> > firefox/lib/ui/save-file-dialog.js
> * SafeFileWindow class
> 
> > firefox/lib/browser.js
> * function openLibrary(aSpec, aCallback)
> * function closeLibrary(aController, aForce)

This module should contain a Browser class, which will automatically have the controller via its constructor. openLibrary() might be a method on the class, and which returns an instance of the PlaceOrganizer. closeLibrary() instead should only be a method on the PlacesOrganizer class, and called close()?

> > firefox/lib/toolbars.js
> * Added the DownloadPanel Class.

If that is not yet in firefox/lib/ui/toolbars.js we should move it. Probably file a new bug and get this done right now before we are getting started here.

> What I suggest here is that we split this patch into 2, in this bug we can
> continue working on the specific download parts (SaveFileWindow,
> DownloadPanel, downloads.js), and in another patch we can create the
> PlacesOrganize Patch and create new tests for it's functionality (open,
> close using menu or shortcuts ).

Sounds fine to me given that we even don't make use of the organizer yet.

Comment 19

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

Comment on attachment 8406219 [details] [diff] [review]
WIP.patch

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

There is still a bit of work left to do, but I think it goes into the right direction. Please see my feedback inline.

::: firefox/lib/browser.js
@@ +1,1 @@
> +  /* This Source Code Form is subject to the terms of the Mozilla Public

This module contains ui related code. So it should be placed under ui/

@@ +4,5 @@
> +
> + "use strict";
> +
> +// Include required modules
> +var { assert, expect } = require("../../lib/assertions");

No need for this anymore. This is available by default via mozmill.

@@ +7,5 @@
> +// Include required modules
> +var { assert, expect } = require("../../lib/assertions");
> +var utils = require("utils");
> +
> +const DTDS = ["chrome://browser/locale/browser.dtd"]

Make a browser class and put all methods and necessary constants in there as already done for other classes.

::: firefox/lib/library.js
@@ -1,1 @@
> -/* This Source Code Form is subject to the terms of the Mozilla Public

I don't see where this file is coming from. It's not part of our repository. Did you submit a patch based on a local reference?

::: firefox/lib/toolbars.js
@@ +297,5 @@
>     */
> +  open: function downloadsPanel_open(aCallback) {
> +    aCallback();
> +    //this._controller.sleep(5000);
> +    this.waitForPanel();

waitForPanel should get this callback. Otherwise we might fail recognizing the event.

@@ +354,5 @@
> +      return self._controller.window.top.DownloadsIndicatorView._operational;
> +    }, "Downloads Panel has been opened");
> +    // TODO: this is not reliable, we need another method to make sure
> +    // the Downloads Panel is loaded and shown
> +    this._controller.sleep(100);

Right, we have to find the proper events to wait for.

::: firefox/lib/ui/places-organizer.js
@@ +20,2 @@
>   */
> +function PlacesOrganizer(aController) {

I think we should add the 'Window' prefix, so indicate the type of class.

@@ +58,5 @@
> +  /**
> +   * Returns the currently selected item in the tree
> +   */
> +  get selectedItem() {
> +    var tree = this.getElement({type: "tree"}).getNode();

I think we should cache the tree given the high usage of it.

@@ +70,5 @@
> +  /**
> +   * Clears the downloads using the Library Window UI
> +   */
> +  clearDownloads : function downloadManager_clearDownloads() {
> +    this.open();

We should kill the usage of open() and close() here. That's not something the class should know about, but the customer is in charge of.

@@ +135,5 @@
>  
> +    var nc = new domUtils.nodeCollector(spec.parent);
> +    switch (spec.type) {
> +      case "backButton":
> +        return [new elementslib.ID(this.controller.window.document, "back-button")];

Do not return immediately but assign to to elems and retrun at the end.

@@ +142,5 @@
> +      case "organizeButton":
> +        return [new elementslib.ID(this.controller.window.document, "organizeButton")];
> +      case "viewMenu":
> +        return [new elementslib.ID(this.controller.window.document, "viewMenu")];
> +      case "maintenanceButton":

Keep the alphabetical order, and maybe add prefixes to separate different sections of the window.

::: firefox/lib/ui/save-file-dialog.js
@@ +108,5 @@
> + *
> + * @param {MozMillController} controller MozMillController of the browser window
> + * @param {String} url URL of the file which has to be downloaded
> + */
> +function downloadFileOfUnknownType(aController, aUrl) {

I think it would be better suited in the browser class?

@@ +143,5 @@
> + * @param {string} aPathOrUrl
> + *        Native path or URL of the file
> + * @see http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/content/downloads.js#1309
> + */
> +function getLocalFileFromNativePathOrUrl(aPathOrUrl) {

I don't see how this is related to this module. It's backend code.

Comment 20

[Daniel Gherasim]

(In reply to Henrik Skupin (:whimboo) from comment #18)
> (In reply to daniel.gherasim from comment #16)
> > > firefox/lib/toolbars.js
> > * Added the DownloadPanel Class.
> 
> If that is not yet in firefox/lib/ui/toolbars.js we should move it. Probably
> file a new bug and get this done right now before we are getting started
> here.
> 

DownloadPanel was handled in firefox/lib/downloads.js.
In the current toolbars.js we have API & UI code all together, so I'm ok with getting started from now to separate this better, and have an firefox/lib/ui/toolbars.js file.
That means in bug 816084 the IdentityPopup can also be to the same firefox/lib/ui/toolbars.js.

Here I'm not sure of one thing, should we export the DownloadPanel & IdentityPopup through the LocationBar class or as separate classes ?

Comment 21

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

(In reply to daniel.gherasim from comment #20)
> Here I'm not sure of one thing, should we export the DownloadPanel &
> IdentityPopup through the LocationBar class or as separate classes ?

I think a property on the location bar class should be fine. We could call open() on it and could even retrieve an already open one. I don't see why we explicitly have to export them.

Comment 22

[Daniel Gherasim]

Attachment: a4.patch

With this patch, we can get enabled firefox/tests/functional/testPrivateBrowsing/testPrivateDownloadPanel.js

Specific files & libraries according to our discussions were created.
Files not needed anymore ( like firefox/lib/downloads.js or firefox/lib/tests/testDownloads.js will be deleted in a final patch here so it can be reviewed easily.

Comment 23

[Daniel Gherasim]

Attachment: a4.1.patch

Updated to test if the panel it's opened when calling close() & also wait for propper events.

Comment 24

[Andrei Eftimie]

Comment on attachment 8408866 [details] [diff] [review]
a4.1.patch

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

This patch should also remove the old code from firefox/lib/downloads.js

::: firefox/lib/tests/testDownloadsPanel.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +

nit: remove one empty line

@@ +5,5 @@
> +"use strict";
> +
> +
> +// Include required modules
> +var {assert, expect} = require("../../../lib/assertions");

You don't need to include these anymore, they are bundled with mozmill.

@@ +19,5 @@
> +  aModule.locationBar = new toolbars.locationBar(aModule.controller);
> +  aModule.downloadsPanel = locationBar.downloadsPanel;
> +}
> +
> +function tearDownModule (aModule) {

nit: remove the whitespace between the function name and the brace

@@ +32,5 @@
> +    button.click();
> +  });
> +
> +  expect.ok(downloadsPanel.getElement({type: "panel"}).exists(),
> +            "Panel should be opened");

Panel has opened

@@ +38,5 @@
> +  expect.ok(downloadsPanel.getElement({type: "downloadsList"}).exists(),
> +            "List with elements exists");
> +
> +  var downloadItem = downloadsPanel.getElement({type: "download", value: "1"});
> +

nit: remove this extra line

::: firefox/lib/ui/browser.js
@@ +1,1 @@
> +  /* This Source Code Form is subject to the terms of the Mozilla Public

I'm not sure what this file should contain.
Only for the browse.dtd file?

The only method it has should be located in a download specific lib.

@@ +45,5 @@
> +   *
> +   * @param {MozMillController} controller MozMillController of the browser window
> +   * @param {String} url URL of the file which has to be downloaded
> +   */
> +  downloadFileOfUnknownType : function browser_downloadFileOfUnknownType(aUrl, aController) {

I'm not sold that this method should be in a `browser` class.

It should be a helper method on in download ui lib since it uses the UI rather than the API.

@@ +49,5 @@
> +  downloadFileOfUnknownType : function browser_downloadFileOfUnknownType(aUrl, aController) {
> +    this._controller.open(aUrl);
> +    // Wait until the unknown content type dialog has been opened
> +    assert.waitFor(function () {
> +      return mozmill.wm.getMostRecentWindow('').document.documentElement.id === 'unknownContentType';

Please split this across 2 lines.

@@ +64,5 @@
> +                       "Unknown Type dialog has been selected");
> +      }
> +
> +      // Wait until the OK button has been enabled and click on it
> +      var acceptButton =  saveFileW.getElement({type: "accept"});

nit: 2 spaces

@@ +65,5 @@
> +      }
> +
> +      // Wait until the OK button has been enabled and click on it
> +      var acceptButton =  saveFileW.getElement({type: "accept"});
> +      acceptButton.waitForElement();

I don't think we need this call to waitForElement. We can enhance the following waitFor call to make sure the element exists before checking its attributes (if it is needed)

@@ +66,5 @@
> +
> +      // Wait until the OK button has been enabled and click on it
> +      var acceptButton =  saveFileW.getElement({type: "accept"});
> +      acceptButton.waitForElement();
> +      assert.waitFor(function () {

Use the arrow syntax

::: firefox/lib/ui/save-file-dialog.js
@@ +87,5 @@
> +        nodeCollector.queryNodes("[dlgtype=accept]");
> +        elems = nodeCollector.elements;
> +        break;
> +      case "save":
> +        elems = [new elementslib.ID(this._controller.window.document, "save")];

Use findElement

::: firefox/lib/ui/toolbars.js
@@ +7,5 @@
> + * The ToolbarAPI adds support for accessing and interacting with toolbar elements
> + *
> + * @version 1.0.0
> + */
> +

Missing: "use strict";

@@ +29,5 @@
> +}
> +
> +DownloadsPanel.prototype = {
> +  /**
> +   * List of available download states

Missing @return {object}

@@ +62,5 @@
> +   *
> +   * @returns {Boolen} True if the panel is open
> +   */
> +  get isOpened() {
> +    return (this._panel.getNode().state === 'open');

The braces are not needed.

@@ +103,5 @@
> +   *        Parent of the to find element
> +   *
> +   * @returns {[ElemBase]} Elements which have been found
> +   */
> +  getElements: function downloadsPanel_getElements(aSpec) {

The inner name should reflect the name of the class. It should be `DownloadsPanel_[..]`

@@ +111,5 @@
> +    var nodeCollector = new domUtils.nodeCollector(parent);
> +
> +    var elems = null;
> +
> +    switch(spec.type) {

nit: should have a space between the switch statement and the opening brace.


I've looked through the rest of the repo and we have 22 instances of `switch(` and 62 instances of `switch (`.
Since there's nothing specific in our style guide, I'll add an entry and file a bug to convert all of them to `switch (`

@@ +114,5 @@
> +
> +    switch(spec.type) {
> +      case "download":
> +        assert.ok(spec.value, "Item's value has been specified");
> +        elems = [new findElement.ID(this._controller.window.document, "downloadsItem_id:" + spec.value)];

Please break this into 2 lines.

@@ +164,5 @@
> +   *
> +   * @params {Boolean} aForce
> +   *         Force closing the panel
> +   */
> +  close: function(aForce) {

Missing the internal function name.
Also be consistent with the space before the semicolon.

::: firefox/tests/functional/testPrivateBrowsing/testPrivateDownloadPanel.js
@@ +7,2 @@
>  // Include the required modules
>  var { assert } = require("../../../../lib/assertions");

You can remove this.

@@ +10,3 @@
>  var privateBrowsing = require("../../../lib/ui/private-browsing");
> +var prefs = require("../../../lib/prefs");
> +var browser = require ("../../../lib/ui/browser");

Please sort these alphabetically.

@@ +27,5 @@
>    ]
>  };
>  
> +const PREF_DOWNLOAD_DIR = "browser.download.dir";
> +const PREF_PANEL_SOWN = "browser.download.panel.shown";

typo: "SHOWN" ?

@@ +34,1 @@
>  var setupModule = function (aModule) {

Please also change the notation to `function setupModule` (do this for all function declarations).

@@ +41,2 @@
>    aModule.pbWindow = new privateBrowsing.PrivateBrowsingWindow();
> +  aModule.pbWindow.open(controller);

aModule.controller

@@ +41,3 @@
>    aModule.pbWindow = new privateBrowsing.PrivateBrowsingWindow();
> +  aModule.pbWindow.open(controller);
> +  aModule.pbBrowserUI = new browser.Browser(pbWindow.controller);

aModule.pbWindow.controller

@@ +41,4 @@
>    aModule.pbWindow = new privateBrowsing.PrivateBrowsingWindow();
> +  aModule.pbWindow.open(controller);
> +  aModule.pbBrowserUI = new browser.Browser(pbWindow.controller);
> +  aModule.pbLocationBar = new toolbars.locationBar(pbWindow.controller);

aModule.pbWindow.controller

@@ +47,5 @@
>    // Set the Download Folder to %profile%/downloads
> +  prefs.preferences.setPref(PREF_DOWNLOAD_DIR, DOWNLOAD_LOCATION);
> +  prefs.preferences.setPref(PREF_DOWNLOAD_FOLDERLIST, 2);
> +  // Prevent auto-open of the download panel
> +  prefs.preferences.setPref(PREF_PANEL_SOWN, true);

This pref should be added in the downloads lib along with a comment referencing bug 959103

::: lib/downloads.js
@@ +17,5 @@
> +
> +/**
> + * Returns a new DownloadList object.
> + *
> + * @param {String} [aType="all"]

We should also include the other available types. |public|private

@@ +22,5 @@
> + *        Type of downloads to return
> + *
> + * @returns {DownloadList} Object that handles the list of downloads
> + */
> +function getDownloadList(aType='all') {

nit: please use double quotes for consistency

@@ +30,5 @@
> +/**
> + * Remove all downloads
> + */
> +function removeAllDownloads() {
> +  getDownloadList("all").then(function (aDownloadList) {

Use the arrow syntax for all these anonymous functions.

@@ +36,5 @@
> +      aDownloads.forEach(function (aDownload) {
> +        aDownloadList.remove(aDownload);
> +      });
> +    });
> +  });

I think we should also wait for all the calls to complete and assert that all downloads have actually been removed.

@@ +42,5 @@
> +
> +/**
> + * Wait for all pending downloads to finish
> + */
> +function waitAllDownloadsFinished() {

I'm not sold on the name. But I don't have a much better proposal that would not make the name way too long.

@@ +44,5 @@
> + * Wait for all pending downloads to finish
> + */
> +function waitAllDownloadsFinished() {
> +  var activeDownloads = null;
> +  getDownloadList("all").then(function (aDownloadList) {

Arrow syntax please.

@@ +55,5 @@
> +      });
> +    });
> +  });
> +
> +  assert.waitFor(function () {

Arrow syntax.

Comment 25

[Daniel Gherasim]

> ::: firefox/lib/ui/browser.js
> @@ +1,1 @@
> > +  /* This Source Code Form is subject to the terms of the Mozilla Public
> 
> I'm not sure what this file should contain.
> Only for the browse.dtd file?
> 
> The only method it has should be located in a download specific lib.
> 
> @@ +45,5 @@
> > +   *
> > +   * @param {MozMillController} controller MozMillController of the browser window
> > +   * @param {String} url URL of the file which has to be downloaded
> > +   */
> > +  downloadFileOfUnknownType : function browser_downloadFileOfUnknownType(aUrl, aController) {
> 
> I'm not sold that this method should be in a `browser` class.
> 
> It should be a helper method on in download ui lib since it uses the UI
> rather than the API.
> 

Henrik asked if this could live in the browser class, I'm asking him for a response on your suggestion.

Comment 26

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

The download ui is handled in toolbars.js, so I don't think it makes sense to put it in there. It's not related to the toolbar at all. So given the rare usage of this method, the browser ui module is still the best place for me.

Comment 27

[Daniel Gherasim]

Attachment: a5.patch

Thanks Andrei, I've made the requested changes except moving the pref, which I believe it should live in the test, so we can clear it in teardown & we'll remove it once the bug get's fixed.

Comment 28

[Andrei Eftimie]

Comment on attachment 8417892 [details] [diff] [review]
a5.patch

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

Looks good, just a few nits.

::: firefox/lib/ui/browser.js
@@ +1,1 @@
> +  /* This Source Code Form is subject to the terms of the Mozilla Public

nit: extra whitespace

@@ +1,5 @@
> +  /* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> + "use strict";

nit: extra whitespace

@@ +33,5 @@
> +
> +  /**
> +   * Gets all the needed external DTD urls as an array
> +   *
> +   * @returns {[String]} Array of external DTD urls

The type is: string[]

@@ +43,5 @@
> +   * Download the file of unkown type from the given location by saving it
> +   * automatically to disk
> +   *
> +   * @param {MozMillController} controller MozMillController of the browser window
> +   * @param {String} url URL of the file which has to be downloaded

nit: reverse order and type should be lowercase for primitives

@@ +53,5 @@
> +      var id = mozmill.wm.getMostRecentWindow('').document.documentElement.id;
> +      return id === 'unknownContentType';
> +    }, "Unknown content type dialog has been opened");
> +
> +    utils.handleWindow("type", "", (aSaveFileController) => {

nit: you don't need parentheses around the arguments

::: firefox/lib/ui/save-file-dialog.js
@@ +37,5 @@
> +   * Retrieve list of UI elements based on the given specification
> +   *
> +   * @param {Object} aSpec
> +   *        Information of the UI elements which should be retrieved
> +   * @parma {String} aSpec.type

nit: lowercase on primitive types

@@ +46,5 @@
> +   *        Value of the attribute to filter
> +   * @param {String} [aSpec.parent=document]
> +   *        Parent of the to find element
> +   *
> +   * @returns {[ElemBase]} Elements which have been found

ElemBase[]

@@ +68,5 @@
> +   *        Value of the attribute to filter
> +   * @param {String} [aSpec.parent=document]
> +   *        Parent of the to find element
> +   *
> +   * @returns {[ElemBase]} Elements which have been found

ElemBase[]

::: firefox/lib/ui/toolbars.js
@@ +26,5 @@
> +DownloadsPanel.prototype = {
> +  /**
> +   * List of available download states
> +   *
> +   * @returns {Object} Download states dictionary

lowercase, please check all instances

@@ +59,5 @@
> +   *
> +   * @returns {Boolen} True if the panel is open
> +   */
> +  get isOpened() {
> +    return this._panel.getNode().state === 'open';

nit: double quotes

@@ +98,5 @@
> +   *        Value of the attribute to filter
> +   * @param {String} [aSpec.parent=document]
> +   *        Parent of the to find element
> +   *
> +   * @returns {[ElemBase]} Elements which have been found

ElemBase[]

@@ +187,5 @@
> +   */
> +  waitForPanel : function DownloadsPanel_waitForPanel(aCallback, aState) {
> +    assert.equal(typeof aCallback, "function", "Callback function is defined");
> +
> +    var state = aState || "open";

You can set the default value directly.
See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/default_parameters

@@ +200,5 @@
> +    try {
> +      aCallback();
> +
> +      assert.waitFor(() => {
> +        return checkPopupEvent &&

If there is only 1 expression the return statement is implied, you can remove it. The curly braces can also be removed in this case.

Comment 29

[Daniel Gherasim]

Attachment: a5.1.patch

Fixed the nits.

Comment 30

[Andrei Eftimie]

Comment on attachment 8418018 [details] [diff] [review]
a5.1.patch

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

::: firefox/lib/tests/testDownloadsPanel.js
@@ +12,5 @@
> +                  "Firefox%203.6.dmg";
> +
> +function setupModule(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.browserUI = new browser.Browser();

This should be browser.BrowserWindow()

@@ +39,5 @@
> +  var downloadItem = downloadsPanel.getElement({type: "download", value: "1"});
> +  expect.ok(downloadItem.exists(), "List element has been found");
> +
> +  expect.ok(downloadsPanel.getElement({type: "showAllDownloads"}).exists(),
> +            "Show all downloads button exists");

nit: use single quotes around 'Show all downloads'

@@ +45,5 @@
> +  // Cancel the download
> +  var cancelDownload = downloadsPanel.getElement({type: "downloadButton",
> +                                                  subtype: "1",
> +                                                  value: "cancel"});
> +  cancelDownload.click();

This fails for me. The download is not getting canceled.

::: firefox/lib/ui/browser.js
@@ +45,5 @@
> +   *
> +   * @param {string} URL of the file which has to be downloaded
> +   * @param {MozMillController} controller MozMillController of the browser window
> +   */
> +  downloadFileOfUnknownType : function browser_downloadFileOfUnknownType(aUrl, aController) {

aController is not used anymore.

@@ +66,5 @@
> +      }
> +
> +      // Wait until the OK button has been enabled and click on it
> +      var acceptButton = saveFileW.getElement({type: "accept"});
> +      assert.waitFor(() => acceptButton.exists(), "Accept button exists");

This fails for me under OSX.

::: lib/downloads.js
@@ +17,5 @@
> +
> +/**
> + * Returns a new DownloadList object.
> + *
> + * @param {String} [aType="all"|"public"|"private"]

nit: string

Comment 31

[Daniel Gherasim]

Attachment: a5.3.patch

Fixed the changes, I'll continue tu run this on different platforms.

Comment 32

[Daniel Gherasim]

Runs fine on all platforms.

Comment 33

[Andrei Eftimie]

Comment on attachment 8420955 [details] [diff] [review]
a5.3.patch

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

::: firefox/lib/tests/testDownloadsPanel.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

This test fails for me on OSX with either:
> "message": "Download has been canceled", 
> "lineNumber": 27, 

Or:
> "message": "Unknown content type dialog has been opened", 
> "lineNumber": 27,

@@ +14,5 @@
> +function setupModule(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.browserUI = new browser.BrowserWindow();
> +  aModule.locationBar = new toolbars.locationBar(aModule.controller);
> +  aModule.downloadsPanel = locationBar.downloadsPanel;

This should be `aModule.locationBar`
It doesn't fail directly because locationBar is exported in the `toolbars` module, but you are not referencing the same instance.

@@ +17,5 @@
> +  aModule.locationBar = new toolbars.locationBar(aModule.controller);
> +  aModule.downloadsPanel = locationBar.downloadsPanel;
> +}
> +
> +function tearDownModule(aModule) {

`teardownModule` (it won't get executed if it doesn't have the correct name)

@@ +18,5 @@
> +  aModule.downloadsPanel = locationBar.downloadsPanel;
> +}
> +
> +function tearDownModule(aModule) {
> +  downloadsPanel.close(true);

This call fails with:
> this._panel.getNode(...).close is not a function

@@ +45,5 @@
> +  cancelDownload.click();
> +
> +  assert.waitFor(() => {
> +    var state = downloadItem.getNode().getAttribute("state");
> +    return parseInt(state) === downloadsPanel.DOWNLOAD_STATE["canceled"] ||

We should cast `state` as a Number in the assignment statement above.

::: firefox/lib/ui/browser.js
@@ +37,5 @@
> +   * @returns {string[]]} Array of external DTD urls
> +   */
> +  get dtds() {
> +    return ["chrome://browser/locale/browser.dtd"];
> +  },

nit: missing empty line

@@ +49,5 @@
> +    this._controller.open(aUrl);
> +    // Wait until the unknown content type dialog has been opened
> +    assert.waitFor(() => {
> +      var id = mozmill.wm.getMostRecentWindow('').document.documentElement.id;
> +      return id === 'unknownContentType';

nit: double quotes

@@ +66,5 @@
> +
> +      // Wait until the OK button has been enabled and click on it
> +      var acceptButton = saveFileW.getElement({type: "accept"});
> +      assert.waitFor(() => acceptButton.exists(), "Accept button exists");
> +      assert.waitFor(() => !acceptButton.getNode().hasAttribute('disabled'),

nit: double quotes please

::: firefox/lib/ui/save-file-dialog.js
@@ +31,5 @@
> +   * @returns {MozMillController} Mozmill Controller
> +   */
> +  get controller() {
> +    return this._controller;
> +  },

nit: missing empty line

@@ +46,5 @@
> +   *        Value of the attribute to filter
> +   * @param {string} [aSpec.parent=document]
> +   *        Parent of the to find element
> +   *
> +   * @returns {ElemBase[]]} Elements which have been found

This returns a single element

@@ +47,5 @@
> +   * @param {string} [aSpec.parent=document]
> +   *        Parent of the to find element
> +   *
> +   * @returns {ElemBase[]]} Elements which have been found
> +   */

This whole comment is not properly indented

::: firefox/lib/ui/toolbars.js
@@ +59,5 @@
> +   *
> +   * @returns {Boolen} True if the panel is open
> +   */
> +  get isOpened() {
> +    return this._panel.getNode().state === 'open';

nit: double quotes please

@@ +184,5 @@
> +   *        Function that triggeres the panel to open/close
> +   * @param {boolean} [aState="open"]
> +   *        State of the panel we are waiting for
> +   */
> +  waitForPanel : function DownloadsPanel_waitForPanel(aCallback, aState = "open") {

Please remove the spaces that wrap the assignment operator.

@@ +223,5 @@
> +  /**
> +   * Returns the autocomplete object
> +   *
> +   * @returns Autocomplete object
> +   * @type {object}

@returns {object} [...]

@@ +233,5 @@
> +  /**
> +   * Returns the controller of the current window
> +   *
> +   * @returns Mozmill controller
> +   * @type {MozMillController}

@returns {MozMillController} [..]

::: firefox/tests/functional/testPrivateBrowsing/testPrivateDownloadPanel.js
@@ +69,3 @@
>    // Download files of unknown type
>    // Normal Browsing
>    DOWNLOADS.normal.forEach(function (aFile) {

Please also change these calls to the fat arrow syntax.

::: lib/downloads.js
@@ +33,5 @@
> +function removeAllDownloads() {
> +  var removed = 0;
> +  var toRemove = null;
> +  var view = {
> +    onDownloadRemoved: function(aDownload) {

Fat arrow syntax please.

@@ +38,5 @@
> +      removed++;
> +    }
> +  }
> +
> +  getDownloadList("all").then(aDownloadList => {

"all" is the default value, we can omit it from this call.

@@ +57,5 @@
> + * Wait for all pending downloads to finish
> + */
> +function waitAllDownloadsFinished() {
> +  var activeDownloads = null;
> +  getDownloadList("all").then(aDownloadList => {

Same here. We can omit "all"

Comment 34

[Daniel Gherasim]

Attachment: a6.patch

Thanks, I updated patch based on your review.

Regarding the failure, here is why:

We get the failure as we are not properly waiting for the download popup to show. Same issue as in bug 994040. So the solution here would be to deactivate animation on panel until those awkward transitions get's fixed.

For this we have to add an initialize method for the panel to load the overlay (as it's not loaded on firefox startup (resource/time consuming reasons)). This method is only triggered when we open the panel, so no other way to avoid the animation.

To avoid code duplication we can use the 'waitForNotificationPopup()' method from bug 994040.

Comment 35

[Andrei Eftimie]

Comment on attachment 8426232 [details] [diff] [review]
a6.patch

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

We'll need to land bug 994040 before we can safely test this.
In the meantime some nits and conflicts:

There's a conflict in firefox/lib/toolbars.js

::: firefox/lib/toolbars.js
@@ +256,5 @@
> +DownloadsPanel.prototype = {
> +  /**
> +   * List of available download states
> +   *
> +   * @returns {Object} Download states dictionary

nit: type should be lowercase

@@ +286,5 @@
> +
> +  /**
> +   * Check if the downloads panel is opened
> +   *
> +   * @returns {Boolen} True if the panel is open

nit: boolean

@@ +328,5 @@
> +   *        Value of the attribute to filter
> +   * @param {string} [aSpec.parent=document]
> +   *        Parent of the to find element
> +   *
> +   * @returns {ElemBase[]]} Elements which have been found

nit: there are 2 "]"

@@ +341,5 @@
> +
> +    switch (spec.type) {
> +      case "download":
> +        assert.ok(spec.value, "Item's value has been specified");
> +        elems = [new findElement.ID(this._controller.window.document,

findElement doesn't need "new". Please check all instances.

::: firefox/lib/ui/browser.js
@@ +33,5 @@
> +
> +  /**
> +   * Gets all the needed external DTD urls as an array
> +   *
> +   * @returns {string[]]} Array of external DTD urls

nit: extra "]"

Comment 36

[Daniel Gherasim]

Attachment: a6.1.patch

We are unblocked here now so this can be landed.

Comment 37

[Andrei Eftimie]

Comment on attachment 8438999 [details] [diff] [review]
a6.1.patch

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

We're in good shape with this.
Just a few minor changes needed from my perspective.

Thanks

::: firefox/lib/tests/testDownloadsPanel.js
@@ +21,5 @@
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.browserUI = new browser.BrowserWindow();
> +  aModule.locationBar = new toolbars.locationBar(aModule.controller);
> +  aModule.downloadsPanel = aModule.locationBar.downloadsPanel;
> +  downloadsPanel.initialize();

aModule.downloadsPanel

Weird thing is we do have a reference to downloadsPanel even though we shouldn't... it seems we leak these?
I've filed bug 1025850 for this.

::: firefox/lib/toolbars.js
@@ +288,5 @@
> +   * Check if the downloads panel is opened
> +   *
> +   * @returns {boolen} True if the panel is open
> +   */
> +  get isOpened() {

This should probably be `isOpen` since it checks the current state.
Also change in the description above.

@@ +340,5 @@
> +    var elems = null;
> +
> +    switch (spec.type) {
> +      case "download":
> +        assert.ok(spec.value, "Item's value has been specified");

I would amend the description a bit: "downloadsItem id has [...]"

@@ +341,5 @@
> +
> +    switch (spec.type) {
> +      case "download":
> +        assert.ok(spec.value, "Item's value has been specified");
> +        elems = [findElement.ID(this._controller.window.document,

We should use `spec.parent` throughout this method instead of `this._controller.window.document`

@@ +342,5 @@
> +    switch (spec.type) {
> +      case "download":
> +        assert.ok(spec.value, "Item's value has been specified");
> +        elems = [findElement.ID(this._controller.window.document,
> +                                    "downloadsItem_id:" + spec.value)];

nit: indentation

@@ +351,5 @@
> +        elems = nodeCollector.elements;
> +        break;
> +      case "downloadsList":
> +        elems = [findElement.ID(this._controller.window.document,
> +                                    "downloadsListBox")];

nit: indentation

@@ +398,5 @@
> +    };
> +    this._controller.window.DownloadsPanel.initialize(onInitializeOver);
> +
> +    assert.waitFor(() => downloadsPanelOverlayState,
> +                 "Downloads panel overlay has been loaded");

nit: indentation

::: firefox/lib/ui/save-file-dialog.js
@@ +71,5 @@
> +   * @returns {ElemBase[]} Elements which have been found
> +   */
> +  getElements : function ExceptionDialog_getElements(aSpec) {
> +    var spec = aSpec || { };
> +    var parent = spec.parent;

You can delete this line and test for spec.parent directly.

@@ +86,5 @@
> +        nodeCollector.queryNodes("[dlgtype=accept]");
> +        elems = nodeCollector.elements;
> +        break;
> +      case "save":
> +        elems = [new findElement.ID(this._controller.window.document, "save")];

Should use `root` instead of `this._controller.window.document`

@@ +89,5 @@
> +      case "save":
> +        elems = [new findElement.ID(this._controller.window.document, "save")];
> +        break;
> +      case "content":
> +        elems = [new findElement.ID(this._controller.window.document,

Same here.

::: firefox/tests/functional/testPrivateBrowsing/testPrivateDownloadPanel.js
@@ +37,5 @@
>    aModule.controller = mozmill.getBrowserController();
> +  aModule.browserWindow = new browser.BrowserWindow();
> +  aModule.locationBar = new toolbars.locationBar(aModule.controller);
> +  aModule.downloadsPanel = aModule.locationBar.downloadsPanel;
> +  downloadsPanel.initialize();

aModule.downloadsPanel

@@ +45,5 @@
>    aModule.pbWindow.open(aModule.controller);
> +  aModule.pbBrowserWindow = new browser.BrowserWindow(aModule.pbWindow.controller);
> +  aModule.pbLocationBar = new toolbars.locationBar(aModule.pbWindow.controller);
> +  aModule.pbDownloadsPanel = aModule.pbLocationBar.downloadsPanel;
> +  pbDownloadsPanel.initialize();

aModule.pbDownloadsPanel

Comment 38

[Daniel Gherasim]

Attachment: v6.2.patch

Patch updated based on review.

Comment 39

[Andrei Eftimie]

Comment on attachment 8445013 [details] [diff] [review]
v6.2.patch

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

With the inline mentioned nit fixed, please ask a review from Henrik.

::: firefox/tests/functional/testPrivateBrowsing/testPrivateDownloadPanel.js
@@ +30,5 @@
> +// Bug 959103
> +// Download gets duplicated and stuck with a new profile
> +// Remove this pref once bug has been fixed
> +const PREF_PANEL_SHOWN = "browser.download.panel.shown";
> +const PREF_DOWNLOAD_FOLDERLIST = "browser.download.folderList";

Please keep this prefs alphabetically sorted.

Comment 40

[Daniel Gherasim]

Attachment: v6.3.patch

Fixed the nit, thanks!

Comment 41

[Daniel Gherasim]

Henrik can you review this please ? It's indeed blocked by 1047235 but this can still be reviewed & landed safely, in which case we can just update the patch from the dependency bug.

Comment 42

[Daniel Gherasim]

So I would unblock this by the browser library given that we have just one function to add there. ( we can add it directly here ).

Comment 43

[Daniel Gherasim]

Just to mention: the function is already added in the patch above for which review is waiting.

Comment 44

[Daniel Gherasim]

Attachment: v6.4.patch

Fixed to apply cleanly as we landed the patch on bug 1036848.

Comment 45

[Daniel Gherasim]

Attachment: v6.5.patch

Add a lib test for downloading a file.

Comment 46

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

Comment on attachment 8478913 [details] [diff] [review]
v6.5.patch

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

I was checking all the code, but haven't actually run it. I will wait with that until all the remaining issues have been fixed.

::: firefox/lib/tests/testDownloadFileOfUnknownType.js
@@ +23,5 @@
> +  browserWindow.downloadFileOfUnknownType(TEST_DATA);
> +
> +  // Check that the file was downloaded
> +  var downloadList = null;
> +  Downloads.getList(Downloads.ALL).then(aDownloadList => {

We don't want to make use of promises in our Mozmill tests. You will have to synchronize those in our downloads module.

@@ +29,5 @@
> +      downloadList = aDownloads;
> +    });
> +  });
> +
> +  expect.waitFor(() => downloadList !== null,

Missing brackets around the comparison.

::: firefox/lib/tests/testDownloadsPanel.js
@@ +13,5 @@
> +                  "Firefox%203.6.dmg";
> +
> +// Bug 959103
> +// Downloads gets duplicated with a new profile
> +// Remove pref once this is fixed

Please move this comment to the actual executed code.

@@ +18,5 @@
> +const PREF_PANEL_SHOWN = "browser.download.panel.shown";
> +
> +function setupModule(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.browserUI = new browser.BrowserWindow();

I would change the variable name to browserWindow or simply browser.

@@ +20,5 @@
> +function setupModule(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.browserUI = new browser.BrowserWindow();
> +  aModule.locationBar = new toolbars.locationBar(aModule.controller);
> +  aModule.downloadsPanel = aModule.locationBar.downloadsPanel;

The download panel is not part of the locationbar, so the LocationBar class cannot hold a reference to it.

@@ +21,5 @@
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.browserUI = new browser.BrowserWindow();
> +  aModule.locationBar = new toolbars.locationBar(aModule.controller);
> +  aModule.downloadsPanel = aModule.locationBar.downloadsPanel;
> +  aModule.downloadsPanel.initialize();

Why has it to be initialized separately? Anything which cannot be done in the constructor?

@@ +49,5 @@
> +  var downloadItem = downloadsPanel.getElement({type: "download", value: "1"});
> +  expect.ok(downloadItem.exists(), "List element has been found");
> +
> +  assert.waitFor(() => {
> +    var state = parseInt(downloadItem.getNode().getAttribute("state"));

I would add a helper method to the class to retrieve the status of a download item.

@@ +50,5 @@
> +  expect.ok(downloadItem.exists(), "List element has been found");
> +
> +  assert.waitFor(() => {
> +    var state = parseInt(downloadItem.getNode().getAttribute("state"));
> +    return state === downloadsPanel.DOWNLOAD_STATE["downloading"];

nit: missing brackets around the comparison.

@@ +63,5 @@
> +  assert.waitFor(() => {
> +    var state = parseInt(downloadItem.getNode().getAttribute("state"));
> +    return state === downloadsPanel.DOWNLOAD_STATE["canceled"] ||
> +           state === downloadsPanel.DOWNLOAD_STATE["finished"];
> +  }, "Download has been canceled");

nit: what about finished?

@@ +66,5 @@
> +           state === downloadsPanel.DOWNLOAD_STATE["finished"];
> +  }, "Download has been canceled");
> +
> +  expect.ok(downloadsPanel.getElement({type: "panel"}).exists(),
> +            "Panel has opened");

So in case it is hidden the DOM entry is not present? Better check for the element type as what a lib test should do anyway.

@@ +72,5 @@
> +  expect.ok(downloadsPanel.getElement({type: "downloadsList"}).exists(),
> +            "List with elements exists");
> +
> +  expect.ok(downloadsPanel.getElement({type: "showAllDownloads"}).exists(),
> +            "'Show all downloads' button exists");

Same for the other two here. Also I think we have more elements to check for.

::: firefox/lib/toolbars.js
@@ +240,5 @@
>  }
>  
>  /**
> + * Downloads Panel (from location bar) class
> + * @constructor

nit: better an empty line above instead of below.

@@ +271,5 @@
> +      blockedParental : 6,
> +      scanning        : 7,
> +      dirty           : 8,
> +      blockedPolicy   : 9
> +    }

This is static content and you should not have to create an instance of the DownloadsPanel to access it. So define it in the download back-end module.

Also lets add a link to the original definition, so in case of failures it will be easier to investigate.

@@ +340,5 @@
> +    var elems = null;
> +
> +    switch (spec.type) {
> +      case "download":
> +        assert.ok(spec.value, "Downloads item id has been specified");

Is that an id or index?

@@ +352,5 @@
> +      case "downloadsList":
> +        elems = [findElement.ID(root, "downloadsListBox")];
> +        break;
> +      case "downloadButton":
> +        assert.ok(spec.value, "Download button has been specified");

'The type of the button'. Also why no assert for subtype?

@@ +356,5 @@
> +        assert.ok(spec.value, "Download button has been specified");
> +
> +        var item = this.getElement({type: "download", value: spec.subtype});
> +        nodeCollector.root = item.getNode();
> +        switch (spec.value) {

You mixed the order up. spec.value should be the download id while spec.subtype is the type of button.

@@ +399,5 @@
> +    this._controller.window.DownloadsPanel.initialize(onInitializeOver);
> +
> +    assert.waitFor(() => downloadsPanelOverlayState,
> +                   "Downloads panel overlay has been loaded");
> +  }

So please explain why we have to do that. Why can't we Firefox let initialize the panel?

@@ +593,5 @@
>    assert.ok(aController, "A controller has to be specified");
>  
>    this._controller = aController;
>    this._autoCompleteResults = new autoCompleteResults(aController);
> +  this._downloadsPanel = new DownloadsPanel(aController);

As said earlier, it's part of the toolbar but not the locationbar.

::: firefox/lib/ui/browser.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

I don't see why it is part of this patch. We have bug 1047235 for that. We might have to wait for the other bug.

::: firefox/lib/ui/save-file-dialog.js
@@ +15,5 @@
> + * @constructor
> + * @param {MozMillController} controller
> + *        MozMillController of the SaveFile Window
> + */
> +function SaveFileWindow(aController) {

The name of this class might have to be changed, because it adds confusion. This is not the File Save window, which is a OS level window and cannot be handled. Please check the window type in its XUL file.

@@ +88,5 @@
> +      case "save":
> +        elems = [new findElement.ID(root, "save")];
> +        break;
> +      case "content":
> +        elems = [new findElement.ID(root, "unknownContentType")];

Sorry, but what is that?

::: lib/downloads.js
@@ +6,5 @@
> +
> +Cu.import("resource://gre/modules/Downloads.jsm");
> +
> +// Include required modules
> +var { assert, expect } = require("assertions");

We don't need this anymore.

@@ +58,5 @@
> + *
> + * @param {number} aTimeout
> + *        Timeout to wait for the downloads to finish
> + */
> +function waitAllDownloadsFinished(aTimeout) {

I would add another parameter for the type of download, so that you can wait for all, public, or privatebrowsing.

Comment 47

[Daniel Gherasim]

Patch is updated but we still have to wait for the dependency bug so I will upload it once that is fixed.

About the last review here are some comments:


(In reply to Henrik Skupin (:whimboo) from comment #46)
> Comment on attachment 8478913 [details] [diff] [review]
> v6.5.patch
> 
> Review of attachment 8478913 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +399,5 @@
> > +    this._controller.window.DownloadsPanel.initialize(onInitializeOver);
> > +
> > +    assert.waitFor(() => downloadsPanelOverlayState,
> > +                   "Downloads panel overlay has been loaded");
> > +  }
> 
> So please explain why we have to do that. Why can't we Firefox let
> initialize the panel?
> 

So we currently disable animatinons on panels by setting the animate attribute to false on the panel.

With no panel loaded (initialized) we don't have a dom entry at all. I don't see how can we wait for it's opening if the .xul file is injected as the first action when panel opening is triggered.

> ::: firefox/lib/ui/browser.js
> @@ +1,1 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
>
> I don't see why it is part of this patch. We have bug 1047235 for that. We
> might have to wait for the other bug.
> 

Just added it here too so we can test all stuff here.
I would wait for that bug too.

> ::: firefox/lib/ui/save-file-dialog.js
> @@ +15,5 @@
> > + * @constructor
> > + * @param {MozMillController} controller
> > + *        MozMillController of the SaveFile Window
> > + */
> > +function SaveFileWindow(aController) {
> 
> The name of this class might have to be changed, because it adds confusion.
> This is not the File Save window, which is a OS level window and cannot be
> handled. Please check the window type in its XUL file.
> 

So I'll rename it to UnknownContentTypeDialog then.

> @@ +88,5 @@
> > +      case "save":
> > +        elems = [new findElement.ID(root, "save")];
> > +        break;
> > +      case "content":
> > +        elems = [new findElement.ID(root, "unknownContentType")];
> 
> Sorry, but what is that?
> 

That's actually the dialog:
http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/content/unknownContentType.xul

I will call the element "dialog".

> ::: lib/downloads.js
> @@ +6,5 @@
> > +
> > +Cu.import("resource://gre/modules/Downloads.jsm");
> > +
> > +// Include required modules
> > +var { assert, expect } = require("assertions");
> 
> We don't need this anymore.
> 

We currently do.

Comment 48

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

(In reply to daniel.gherasim from comment #47)
> > > +    this._controller.window.DownloadsPanel.initialize(onInitializeOver);
> > > +
> > > +    assert.waitFor(() => downloadsPanelOverlayState,
> > > +                   "Downloads panel overlay has been loaded");
> > > +  }
> > 
> > So please explain why we have to do that. Why can't we Firefox let
> > initialize the panel?
> 
> So we currently disable animatinons on panels by setting the animate
> attribute to false on the panel.
> 
> With no panel loaded (initialized) we don't have a dom entry at all. I don't
> see how can we wait for it's opening if the .xul file is injected as the
> first action when panel opening is triggered.

So for what specifically do we need that? For disabling the animations? Is there no global pref for that, and you have to do it by changing a property? Please give references here.

> > > +      case "content":
> > > +        elems = [new findElement.ID(root, "unknownContentType")];
> > 
> > Sorry, but what is that?
> 
> That's actually the dialog:
> http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/
> content/unknownContentType.xul

How can this be the dialog? That would be top-level. Maybe its a button to trigger the dialog? I don't understand without clear references.

Also make sure you update the internal function names. Lot of those still reference other classes.

Comment 49

[Daniel Gherasim]

(In reply to Henrik Skupin (:whimboo) from comment #48)
> (In reply to daniel.gherasim from comment #47)
> > > > +    this._controller.window.DownloadsPanel.initialize(onInitializeOver);
> > > > +
> > > > +    assert.waitFor(() => downloadsPanelOverlayState,
> > > > +                   "Downloads panel overlay has been loaded");
> > > > +  }
> > > 
> > > So please explain why we have to do that. Why can't we Firefox let
> > > initialize the panel?
> > 
> > So we currently disable animatinons on panels by setting the animate
> > attribute to false on the panel.
> > 
> > With no panel loaded (initialized) we don't have a dom entry at all. I don't
> > see how can we wait for it's opening if the .xul file is injected as the
> > first action when panel opening is triggered.
> 
> So for what specifically do we need that? For disabling the animations? Is
> there no global pref for that, and you have to do it by changing a property?
> Please give references here.
> 

We need that so we can wait for the panel, with or without the animation, to use any item on the downloads panel, we have to at least wait for the "popupshown" event on panel. Without the dom entry the only element in the tree on which we can wait for the event is the window. I'm not sure if that works too but I'll try.

> > > > +      case "content":
> > > > +        elems = [new findElement.ID(root, "unknownContentType")];
> > > 
> > > Sorry, but what is that?
> > 
> > That's actually the dialog:
> > http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/
> > content/unknownContentType.xul
> 
> How can this be the dialog? That would be top-level. Maybe its a button to
> trigger the dialog? I don't understand without clear references.
> 

I'm not sure what exactly it's not clear here. From the link above, on line 19 we have 
> <dialog id="unknownContentType"...
That's the element we get.

Comment 50

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

(In reply to daniel.gherasim from comment #49)
> We need that so we can wait for the panel, with or without the animation, to
> use any item on the downloads panel, we have to at least wait for the
> "popupshown" event on panel. Without the dom entry the only element in the
> tree on which we can wait for the event is the window. I'm not sure if that
> works too but I'll try.

Maybe the event bubbles up so we can make use of it. Otherwise we might have to wait for the popup DOM node first, and then for the final state.

> > > That's actually the dialog:
> > > http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/
> > > content/unknownContentType.xul
> > 
> > How can this be the dialog? That would be top-level. Maybe its a button to
> > trigger the dialog? I don't understand without clear references.
> 
> I'm not sure what exactly it's not clear here. From the link above, on line
> 19 we have 
> > <dialog id="unknownContentType"...
> That's the element we get.

But why do you need that? This is not an element but the dialog itself. I don't see for what it should be used for.

Comment 51

[Daniel Gherasim]

(In reply to Henrik Skupin (:whimboo) from comment #50)
> (In reply to daniel.gherasim from comment #49)
> > We need that so we can wait for the panel, with or without the animation, to
> > use any item on the downloads panel, we have to at least wait for the
> > "popupshown" event on panel. Without the dom entry the only element in the
> > tree on which we can wait for the event is the window. I'm not sure if that
> > works too but I'll try.
> 
> Maybe the event bubbles up so we can make use of it. Otherwise we might have
> to wait for the popup DOM node first, and then for the final state.
> 

So the "panelshown" event works on the main-window. But the problem is with disabling the animations, which we can't do for this particular element, except following your advice, about waiting for the dom node, then for final state.
That would mean to wait for a state and not the animation given that we can't interfferr thorugh this process (between the xul loading and starting to open).

But hey, Bug 994117 was fixed, so waiting for the popupshown event might be enough for now. We may even remove the workaround with disabling animations we did here, given that they reverted animations to the old ones.

> > > > That's actually the dialog:
> > > > http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/
> > > > content/unknownContentType.xul
> > > 
> > > How can this be the dialog? That would be top-level. Maybe its a button to
> > > trigger the dialog? I don't understand without clear references.
> > 
> > I'm not sure what exactly it's not clear here. From the link above, on line
> > 19 we have 
> > > <dialog id="unknownContentType"...
> > That's the element we get.
> 
> But why do you need that? This is not an element but the dialog itself. I
> don't see for what it should be used for.

I need that for getting the anonymous elements in the dialog, without this element we can't get them (e.g. : the accept button).

Comment 52

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

(In reply to daniel.gherasim from comment #51)
> But hey, Bug 994117 was fixed, so waiting for the popupshown event might be
> enough for now. We may even remove the workaround with disabling animations
> we did here, given that they reverted animations to the old ones.

Wonderful. Lets do that!

> > But why do you need that? This is not an element but the dialog itself. I
> > don't see for what it should be used for.
> 
> I need that for getting the anonymous elements in the dialog, without this
> element we can't get them (e.g. : the accept button).

Why not? Take the document of the dialog as root for the query. Why should that not work?

Comment 53

[Daniel Gherasim]

Attachment: v9.patch

Patch updated based on last review & discussions we had.

Comment 54

[Andreea Matei [:AndreeaMatei]]

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

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

::: firefox/lib/tests/testDownloadFileOfUnknownType.js
@@ +17,5 @@
> +
> +function testDownloadFileOfUnknownType() {
> +  browserWindow.downloadFileOfUnknownType(TEST_DATA);
> +
> +  // Check that the download is propperly added to the list]

nit: ] at the end

@@ +19,5 @@
> +  browserWindow.downloadFileOfUnknownType(TEST_DATA);
> +
> +  // Check that the download is propperly added to the list]
> +  var downloadList = downloads.getDownloadList();
> +

No need for this blank line, the var and the verification should be a single block, they're related.

@@ +23,5 @@
> +
> +  expect.equal(downloadList.length, 1,
> +               "One download has been added to the list");
> +
> +  // Shouldn't take more then 2 seconds to download the local file

nit: than

::: firefox/lib/tests/testDownloadsPanel.js
@@ +38,5 @@
> +    downloadsPanel.close({force: true});
> +  }
> +}
> +
> +function testDownloadPanel() {

Please add jsdoc.

@@ +60,5 @@
> +
> +  assert.waitFor(() => {
> +    var state = downloadsPanel.getDownloadStatus(downloadItem);
> +    return (state === downloads.DOWNLOAD_STATE["canceled"]) ||
> +           (state === downloads.DOWNLOAD_STATE["finished"]);

Not sure how quickly this goes (download and test), so when you tested, did we ever got it to finish?
Also, shouldn't we also add/test the retry button? Before I see we had pause and resume download, now we have cancel and retry.

::: firefox/lib/toolbars.js
@@ +248,5 @@
> + */
> +function DownloadsPanel(aController) {
> +  this._controller = aController || mozmill.getBrowserController();
> +
> +  this._panel = this.getElement({type: "panel"});

Please remove the empty line.

@@ +276,5 @@
> +   *
> +   * @param {MozMillElement} aDownload
> +   *        The element for which to get the state
> +   *
> +   * @returns {number} The number represinting the state download is in

nit: representing

@@ +439,5 @@
> +    var method = aSpec.method || "button";
> +
> +    waitForNotificationPanel(() => {
> +      switch (method) {
> +        case "button":

We also have shortcut here, don't we? command key + J.

::: firefox/lib/ui/browser.js
@@ +33,5 @@
>    return PrivateBrowsingUtils.isWindowPrivate(this._controller.window)
>  });
>  
>  /**
> + * Download the file of unkown type from the given location by saving it

nit: unknown. Please end this line at "by" to keep the flow better when reading the 2 lines.

::: firefox/lib/ui/unknown-content-type-dialog.js
@@ +98,5 @@
> +  }, {id: "unknownContentType", state: "open"});
> +}
> +
> +/**
> + * Download the file of unkown type from the given location by saving it

Same here with the comment readability

@@ +127,5 @@
> +  // Wait until the OK button has been enabled and click on it
> +  var acceptButton = unknownContentTypeDialog.getElement({type: "accept"});
> +  assert.waitFor(() => acceptButton.exists(), "Accept button exists");
> +  assert.waitFor(() => !acceptButton.getNode().hasAttribute("disabled"),
> +                 "The OK button has been enabled");

Can't we group these 2 into one waitFor?

::: firefox/tests/functional/testPrivateBrowsing/testPrivateDownloadPanel.js
@@ +45,5 @@
>  
>    // Set the Download Folder to %profile%/downloads
> +  prefs.preferences.setPref(PREF_DOWNLOAD_DIR, DOWNLOAD_LOCATION);
> +  prefs.preferences.setPref(PREF_DOWNLOAD_FOLDERLIST, 2);
> +  // Bug 959103

New line before the comment please.

::: lib/downloads.js
@@ +34,5 @@
> +
> +/**
> + * Returns a new DownloadList object.
> + *
> + * @param {string} [aType="all"|"public"|"private"]

We usually put the default value here.

Comment 55

[Daniel Gherasim]

(In reply to Andreea Matei [:AndreeaMatei] from comment #54)
> Comment on attachment 8500438 [details] [diff] [review]
> v9.patch
> 
> Review of attachment 8500438 [details] [diff] [review]:
> -----------------------------------------------------------------

> @@ +439,5 @@
> > +    var method = aSpec.method || "button";
> > +
> > +    waitForNotificationPanel(() => {
> > +      switch (method) {
> > +        case "button":
> 
> We also have shortcut here, don't we? command key + J.
> 

That's for the PlacesOrganizer window focused on downloads.

> ::: lib/downloads.js
> @@ +34,5 @@
> > +
> > +/**
> > + * Returns a new DownloadList object.
> > + *
> > + * @param {string} [aType="all"|"public"|"private"]
> 
> We usually put the default value here.

Along with the other options avaible I guess. 
That's something Andrei asked in some reviews.

Comment 56

[Teodor Druta]

Attachment: b908649v10.patch

There were a lot of rejections applying the latest patch version.
I think I have fixed all of this rejections and also rewrote/refactored bits of code as it was not working any more with the latest library changes landed.
Also I have fixed the issues from Andreea's review.

Comment 57

[Teodor Druta]

Testruns all green:

http://mozmill-crowd.blargon7.com/#/functional/report/b67428b6e502c230ff85b6a4c439ab10
http://mozmill-crowd.blargon7.com/#/remote/report/b67428b6e502c230ff85b6a4c43a1c7b (1 Failure - Bug 1097682)

Comment 58

[Andrei Eftimie]

Comment on attachment 8522223 [details] [diff] [review]
b908649v10.patch

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

::: firefox/lib/tests/testDownloadFileOfUnknownType.js
@@ +24,5 @@
> +  });
> +
> +  browserWindow.downloadFileOfUnknownType(dialog);
> +
> +  // Check that the download is propperly added to the list

typo: properly

::: firefox/lib/tests/testDownloadsPanel.js
@@ +1,1 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public

This file should be named `testDownloadPanel`

@@ +4,5 @@
>  
>  "use strict";
>  
>  // Include required modules
> +var browser = require("../ui/browser");

As done recently in other tests, separate ui libs from the rest with an empty newline.

@@ +73,5 @@
> +
> +  // Retry the download
> +  var retryDownload = downloadsPanel.getElement({type: "downloadButton",
> +                                                  subtype: "retry",
> +                                                  value: "1"});

nit: indentation

@@ +86,5 @@
> +  assert.waitFor(() => {
> +    var state = downloadsPanel.getDownloadStatus(downloadItem);
> +    return (state === downloads.DOWNLOAD_STATE["canceled"]) ||
> +           (state === downloads.DOWNLOAD_STATE["finished"]);
> +  }, "Download has been canceled or finished");

All this should be part of a regular test.

The library test only needs to test the existence of the required elements, as is done below.

::: firefox/lib/toolbars.js
@@ +449,3 @@
>     */
> +  open : function DownloadsPanel_open(aSpec={}) {
> +    this._controller = aSpec.controller || this._controller;

We don't need a controller here... Use this.browserWindow.controller if you need it. (I don't see it used anyway)

@@ +462,2 @@
>                         "Callback has been defined");
> +          aCallback();

There is no `aCallback`

@@ +467,2 @@
>        }
> +    }, {panel: this.getElement({type: "panel"}),

`this.panel` was fine here.

@@ +467,3 @@
>        }
> +    }, {panel: this.getElement({type: "panel"}),
> +        parent: this.browserWindow.controller.window});

Why do we need `parent`? And where is the `open` attribute?

::: firefox/tests/functional/testPrivateBrowsing/testPrivateDownloadPanel.js
@@ +6,5 @@
>  
>  Cu.import("resource://gre/modules/Services.jsm");
>  
>  // Include the required modules
>  var { assert } = require("../../../../lib/assertions");

Please also remove this include.

@@ +37,3 @@
>  
> +function setupModule(aModule) {
> +  aModule.browserWindow = new browser.BrowserWindow();

You are also overriding aModule.browserWindow a couple lines below.

What you want here is 1 Normal Window, with a reference to its download panel, and 1 Private window, with a reference to its download panel.

@@ +43,4 @@
>    aModule.browserWindow = new browser.BrowserWindow();
>    aModule.pbWindow = browserWindow.open({private: true, method: "shortcut"});
>  
> +  aModule.pbBrowserWindow = new browser.BrowserWindow(aModule.pbWindow.controller);

Something's not right here. There is already a Private Window open in aModule.pbWindow

@@ +48,4 @@
>  
>    // Set the Download Folder to %profile%/downloads
> +  prefs.preferences.setPref(PREF_DOWNLOAD_DIR, DOWNLOAD_LOCATION);
> +  prefs.preferences.setPref(PREF_DOWNLOAD_FOLDERLIST, 2);

The ability to set the download location should be part of the download lib.

::: lib/downloads.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Make sure that this file is a rename from the old downloads.js library, to preserve history.

@@ +6,5 @@
> +
> +Cu.import("resource://gre/modules/Downloads.jsm");
> +
> +// Include required modules
> +var { assert, expect } = require("assertions");

You can remove the inclusion of the assertion module, this is available by default.

@@ +39,5 @@
> + *        Type of downloads to return
> + *
> + * @returns {DownloadList} Object that handles the list of downloads
> + */
> +function getDownloadList(aType = "all") {

nit: remove the spaces around the assignment operator.

Comment 59

[Teodor Druta]

Attachment: b908649v11.patch

Fixed typos renamed testDownloadsPanel => testDownloadPanel, separated ui libs from libs, fixed indentation, removed unnecessary includes. 

> 
> @@ +86,5 @@
> > +  assert.waitFor(() => {
> > +    var state = downloadsPanel.getDownloadStatus(downloadItem);
> > +    return (state === downloads.DOWNLOAD_STATE["canceled"]) ||
> > +           (state === downloads.DOWNLOAD_STATE["finished"]);
> > +  }, "Download has been canceled or finished");
> 
> All this should be part of a regular test.
> 
> The library test only needs to test the existence of the required elements,
> as is done below.
> 

Updated the remote test testDownloading/testDownloadStates with this code.

> ::: firefox/lib/toolbars.js
> @@ +449,3 @@
> >     */
> > +  open : function DownloadsPanel_open(aSpec={}) {
> > +    this._controller = aSpec.controller || this._controller;
> 
> We don't need a controller here... Use this.browserWindow.controller if you
> need it. (I don't see it used anyway)
> 
> @@ +462,2 @@
> >                         "Callback has been defined");
> > +          aCallback();
> 
> There is no `aCallback`
> 
> @@ +467,2 @@
> >        }
> > +    }, {panel: this.getElement({type: "panel"}),
> 
> `this.panel` was fine here.
> 
> @@ +467,3 @@
> >        }
> > +    }, {panel: this.getElement({type: "panel"}),
> > +        parent: this.browserWindow.controller.window});
> 
> Why do we need `parent`? And where is the `open` attribute?
> 
> 
> @@ +37,3 @@
> >  
> > +function setupModule(aModule) {
> > +  aModule.browserWindow = new browser.BrowserWindow();
> 
> You are also overriding aModule.browserWindow a couple lines below.
> 
> What you want here is 1 Normal Window, with a reference to its download
> panel, and 1 Private window, with a reference to its download panel.
> 
> @@ +43,4 @@
> >    aModule.browserWindow = new browser.BrowserWindow();
> >    aModule.pbWindow = browserWindow.open({private: true, method: "shortcut"});
> >  
> > +  aModule.pbBrowserWindow = new browser.BrowserWindow(aModule.pbWindow.controller);
> 
> Something's not right here. There is already a Private Window open in
> aModule.pbWindow
> 

Fixed.

> @@ +48,4 @@
> >  
> >    // Set the Download Folder to %profile%/downloads
> > +  prefs.preferences.setPref(PREF_DOWNLOAD_DIR, DOWNLOAD_LOCATION);
> > +  prefs.preferences.setPref(PREF_DOWNLOAD_FOLDERLIST, 2);
> 
> The ability to set the download location should be part of the download lib.
> 

I think this should be a new bug ?

> ::: lib/downloads.js
> @@ +1,1 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> 
> Make sure that this file is a rename from the old downloads.js library, to
> preserve history.
> 

Moved the old firefox/lib/downloads.js file to lib/downloads.js using hg move command.

Comment 60

[Andrei Eftimie]

Comment on attachment 8523783 [details] [diff] [review]
b908649v11.patch

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

::: firefox/lib/downloads.js
@@ -1,1 @@
> -/* This Source Code Form is subject to the terms of the Mozilla Public

`firefox/lib/downloads.js` is still removed and `lib/downloads.js` is still marked as a new file.

::: firefox/lib/tests/testDownloadsPanel.js
@@ -1,1 @@
> -/* This Source Code Form is subject to the terms of the Mozilla Public

Don't remove this library test, leave it as it was. In my previous review I was talking about only the extra tests, which you now included in testDownloadStates.
But this basic test should remain here.

::: firefox/lib/toolbars.js
@@ +440,5 @@
> +   *         Information about the panel to open
> +   * @parmas {string} [aSpec.method="button"|"callback"]
> +   *         Method to use for opening the Downloads Panel
> +   * @parmas {function} [aSpec.callback]
> +   *         Callback function that triggeres the opening

typo: triggers

@@ +442,5 @@
> +   *         Method to use for opening the Downloads Panel
> +   * @parmas {function} [aSpec.callback]
> +   *         Callback function that triggeres the opening
> +   * @params {MozMillController} [aSpec.controller]
> +   *         MozMillController from where to open the page

We don't use any controller.

@@ +444,5 @@
> +   *         Callback function that triggeres the opening
> +   * @params {MozMillController} [aSpec.controller]
> +   *         MozMillController from where to open the page
> +   *
> +   * @returns {DownloadsPanel} Instance of the Downloads Panel

This method doesn't return anything.

@@ +461,5 @@
>                         "Callback has been defined");
>            aSpec.callback();
>            break;
>          default:
> +          assert.fail("Unknown method for opening downloads panel - " + method);

nity: "the downloads..."

::: firefox/lib/ui/unknown-content-type-dialog.js
@@ +94,5 @@
> +
> +  // Wait until the OK button has been enabled and click on it
> +  var acceptButton = aDialog.getElement({type: "button", subtype: "accept"});
> +  assert.waitFor(() => (acceptButton.exists() && !acceptButton.getNode().hasAttribute("disabled")),
> +                        "The OK button has been enabled");

nit: indentation should be 1 space to the left

::: firefox/tests/functional/testPrivateBrowsing/testPrivateDownloadPanel.js
@@ +31,5 @@
>  };
>  
> +const PREF_DOWNLOAD_DIR = "browser.download.dir";
> +const PREF_DOWNLOAD_FOLDERLIST = "browser.download.folderList";
> +const PREF_PANEL_SHOWN = "browser.download.panel.shown";

As said previously, please preserve these as part of the downloads lib.
We can have 2 independent functions for setting and getting the download location.

@@ +40,4 @@
>  
> +  // PB Window
> +  aModule.pbBrowserWindow = browserWindow.open({private: true, method: "shortcut"});
> +  // aModule.pbBrowserWindow = new browser.BrowserWindow(aModule.pbWindow.controller);

Please remove this altogether.

@@ +98,3 @@
>  
> +  // Open the Download Panel and check that we downloaded the correct files
> +  downloadsPanel.open()

nit: missing ;

@@ +104,3 @@
>  
> +  // Open the Private Download Panel and check that we downloaded the correct files
> +  pbDownloadsPanel.open()

nit: missing ;

Comment 61

[Teodor Druta]

Attachment: b908649v12.patch

Fixed all typos and nits. 
Moved the downloads related constants from testPrivateDownloadPanel to the downloads lib, added a two new methods for the downloads lib getDownloadLocation and setDownloadLocation.
Moved the old firefox/lib/downloads.js to lib/downloads.js to preserve history.
Fixed the mistake from the previous patch where testDownloadsPanel was removed, renamed it to testDownloadPanel.

Comment 62

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8525935 [details] [diff] [review]
b908649v12.patch

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

We're getting closer :)

::: firefox/lib/tests/testDownloadPanel.js
@@ +5,5 @@
> +"use strict";
> +
> +// Include required modules
> +var browser = require("../ui/browser");
> +var downloads = require("../../../lib/downloads");

You're not using this lib in this test.

@@ +8,5 @@
> +var browser = require("../ui/browser");
> +var downloads = require("../../../lib/downloads");
> +var prefs = require("../prefs");
> +var toolbars = require("../toolbars");
> +var utils = require("../../../lib/utils");

This is not used here.

@@ +62,5 @@
> +    expect.equal(element.getNode().localName, aElement.type,
> +                 aElement.name + " element has been found");
> +  });
> +
> +  downloadsPanel.close();

We need to wait here a little longer, like we do in the other test. Otherwise on OSX when we try to close we get an extra dialog "Cancel all downloads?" with buttons to "Don't quit" or "Cancel 1 download", and even if you don't choose for a second and the download finishes, the browser will remain in that state until you choose.

::: firefox/lib/ui/unknown-content-type-dialog.js
@@ +93,5 @@
> +  }
> +
> +  // Wait until the OK button has been enabled and click on it
> +  var acceptButton = aDialog.getElement({type: "button", subtype: "accept"});
> +  assert.waitFor(() => (acceptButton.exists() && !acceptButton.getNode().hasAttribute("disabled")),

Please move the second condition on a new line to keep the 80 chars rule.

@@ +121,5 @@
>  exports.UnknownContentTypeDialog = UnknownContentTypeDialog;
>  
>  // Export of methods
>  exports.open = open;
> +exports.downloadFileOfUnknownType = downloadFileOfUnknownType;

Alphabetically sorted, this should go above.

::: firefox/tests/functional/testPrivateBrowsing/testPrivateDownloadPanel.js
@@ +55,4 @@
>  
> +  if (aModule.downloadsPanel) {
> +    aModule.downloadsPanel.close({force: true});
> +  }

I think we already check this "if" condition in the close method, with force.

::: firefox/tests/remote/testDownloading/testDownloadStates.js
@@ +32,4 @@
>  }
>  
> +/**
> + * Test download states

Not sure why you removed the information about the states, would be good to have, even if are different ones.

Comment 63

[Teodor Druta]

Attachment: b908649v13.patch

> 
> @@ +62,5 @@
> > +    expect.equal(element.getNode().localName, aElement.type,
> > +                 aElement.name + " element has been found");
> > +  });
> > +
> > +  downloadsPanel.close();
> 
> We need to wait here a little longer, like we do in the other test.
> Otherwise on OSX when we try to close we get an extra dialog "Cancel all
> downloads?" with buttons to "Don't quit" or "Cancel 1 download", and even if
> you don't choose for a second and the download finishes, the browser will
> remain in that state until you choose.
> 

I added some code to cancel the download, before closing the download panel, so the alert dialog would not appear after test is finished.


Fixed all the nits, removed all the unnecesary libs, removed the useless ifs from tearDownModule of testPrivateDownloadPanel.

Comment 64

[Andrei Eftimie]

Comment on attachment 8528316 [details] [diff] [review]
b908649v13.patch

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

::: firefox/lib/tests/testDownloadPanel.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

I think I wrongly suggested to name this "downloadPanel" instead of "downloadsPanel". I've looked through how it's named in other places and that seems to be the consensus.
So you can leave it as it was.
Sorry for the confusion.

@@ +9,5 @@
> +var downloads = require("../../../lib/downloads");
> +var prefs = require("../prefs");
> +var toolbars = require("../toolbars");
> +
> +var browser = require("../ui/browser");

nit: leave an empty line here

::: firefox/lib/toolbars.js
@@ +1463,5 @@
>    }
>  }
>  
>  // Export of classes
> +exports.DownloadsPanel = DownloadsPanel;

Remove this. This will be used by instantiating a NavBar, and using navBar.downloadsPanel

::: firefox/tests/functional/testPrivateBrowsing/testPrivateDownloadPanel.js
@@ +40,4 @@
>  
>    // Set the Download Folder to %profile%/downloads
> +  downloads.setDownloadLocation(DOWNLOAD_LOCATION);
> +  prefs.preferences.setPref(downloads.PREF_DOWNLOAD_FOLDERLIST, 2);

both of these should be part of the download lib. You can't really separate them.

@@ +50,5 @@
>  
> +function teardownModule(aModule) {
> +  prefs.preferences.clearUserPref(downloads.PREF_DOWNLOAD_DIR);
> +  prefs.preferences.clearUserPref(downloads.PREF_DOWNLOAD_FOLDERLIST);
> +  prefs.preferences.clearUserPref(downloads.PREF_PANEL_SHOWN);

Make a method to clear the custom download dir.

::: lib/downloads.js
@@ +102,5 @@
> + * @params {string} aDownloadLocation
> + *         Location for the downloads directory.
> + */
> +function setDownloadLocation(aDownloadLocation) {
> +  prefs.preferences.setPref(PREF_DOWNLOAD_DIR, aDownloadLocation);

You need to also se the other pref here.

And have a method to reset the downloadLocation (or maybe reuse this when the argument is something like null? not sure which one is best)

Comment 65

[Teodor Druta]

Attachment: b908649v14.patch

(In reply to Andrei Eftimie from comment #64)

Renamed testDownloadPanel.js to testDownloadsPanel.js
Fixed all the nits.

> ::: firefox/lib/toolbars.js
> @@ +1463,5 @@
> >    }
> >  }
> >  
> >  // Export of classes
> > +exports.DownloadsPanel = DownloadsPanel;
> 
> Remove this. This will be used by instantiating a NavBar, and using
> navBar.downloadsPanel
> 

Removed the line also update all the tests to use navBar.downloadsPanel

> ::: firefox/tests/functional/testPrivateBrowsing/testPrivateDownloadPanel.js
> @@ +40,4 @@
> >  
> >    // Set the Download Folder to %profile%/downloads
> > +  downloads.setDownloadLocation(DOWNLOAD_LOCATION);
> > +  prefs.preferences.setPref(downloads.PREF_DOWNLOAD_FOLDERLIST, 2);
> 
> both of these should be part of the download lib. You can't really separate
> them.
> 

Added both to the setDownloadLocation method.

> @@ +50,5 @@
> >  
> > +function teardownModule(aModule) {
> > +  prefs.preferences.clearUserPref(downloads.PREF_DOWNLOAD_DIR);
> > +  prefs.preferences.clearUserPref(downloads.PREF_DOWNLOAD_FOLDERLIST);
> > +  prefs.preferences.clearUserPref(downloads.PREF_PANEL_SHOWN);
> 
> Make a method to clear the custom download dir.
> 

Added new clearDownloadLocationPrefs method in the downloads lib.

Comment 66

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8529103 [details] [diff] [review]
b908649v14.patch

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

::: firefox/lib/tests/testDownloadsPanel.js
@@ +35,5 @@
> +
> +  // Bug 959103
> +  // Downloads gets duplicated with a new profile
> +  // Remove pref once this is fixed
> +  prefs.preferences.setPref(PREF_PANEL_SHOWN, true);

prefs.setPref is used now. Patch needs updating with the latest changes.

::: firefox/lib/ui/browser.js
@@ +56,5 @@
>    return this._navBar = this._navBar || new toolbars.NavBar(this);
>  });
>  
>  /**
> + * Download the file of unkown type from the given UCTP by

nit: unknown type

::: firefox/lib/ui/unknown-content-type-dialog.js
@@ +73,5 @@
>    return elems;
>  }
>  
>  /**
> + * Download the file of unkown type from an open UCTD by

Same here.

::: lib/downloads.js
@@ +46,5 @@
>  
>  /**
> + * Returns a new DownloadList object.
> + *
> + * @param {string} [aType="all"|"public"|"private"]

Please leave here just the default option and the rest in the description.

Comment 67

[Teodor Druta]

Attachment: b908649v15.patch

(In reply to Andreea Matei [:AndreeaMatei] from comment #66)
> Comment on attachment 8529103 [details] [diff] [review]
> b908649v14.patch
> 
> Review of attachment 8529103 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: firefox/lib/tests/testDownloadsPanel.js
> @@ +35,5 @@
> > +
> > +  // Bug 959103
> > +  // Downloads gets duplicated with a new profile
> > +  // Remove pref once this is fixed
> > +  prefs.preferences.setPref(PREF_PANEL_SHOWN, true);
> 
> prefs.setPref is used now. Patch needs updating with the latest changes.
> 

Changed everywhere it applied.

> 
> ::: lib/downloads.js
> @@ +46,5 @@
> >  
> >  /**
> > + * Returns a new DownloadList object.
> > + *
> > + * @param {string} [aType="all"|"public"|"private"]
> 
> Please leave here just the default option and the rest in the description.

Fixed everywhere in the lib.

There were a lot of rejections I had to fix, mainly due to landing of the patch in Bug 1040610, I think I resolved all of them without overwriting any code from that patch. Testruns were all green.

Comment 68

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8531575 [details] [diff] [review]
b908649v15.patch

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

::: firefox/lib/tests/testDownloadFileOfUnknownType.js
@@ +13,5 @@
> +
> +function setupModule(aModule) {
> +  aModule.browserWindow = new browser.BrowserWindow();
> +}
> +

Add teardownModule to clear the downloads list

::: firefox/lib/tests/testDownloadsPanel.js
@@ +9,5 @@
> +var downloads = require("../../../lib/downloads");
> +var prefs = require("../../../lib/prefs");
> +var toolbars = require("../toolbars");
> +
> +var browser = require("../ui/browser");

You included this module twice, please remove from here

@@ +18,5 @@
> +    {name: "openButton", type: "toolbarbutton"},
> +    {name: "panel", type: "panel"},
> +    {name: "showAllDownloads", type: "button"}
> +  ]
> +}

};

@@ +23,1 @@
>  

Extra blank line, please remove

@@ +45,1 @@
>  }

Also remove the downloads from the list

::: firefox/lib/toolbars.js
@@ +405,3 @@
>     * @params {boolean} [aSpec.force=false]
>     *         Force closing the Downloads Panel
>     *

Extra line, please remove it

::: firefox/lib/ui/browser.js
@@ +59,5 @@
>  /**
> + * Download the file of unknown type from the given UCTP by
> + * saving it automatically to disk
> + *
> + * @param {UnknownContentTypeDialog} aDialog

Please add a description for the aDialog parameter.

@@ +62,5 @@
> + *
> + * @param {UnknownContentTypeDialog} aDialog
> + */
> +BrowserWindow.prototype.downloadFileOfUnknownType = function BW_downloadFileOfUT(aDialog) {
> +  return unknownContentTypeDialog.downloadFileOfUnknownType(aDialog);

unknownContentTypeDialog.downloadFileOfUnknownType function doesn't return a value, so remove the "return" here

::: firefox/tests/functional/testPrivateBrowsing/testPrivateDownloadPanel.js
@@ +13,2 @@
>  var windows = require("../../../../lib/windows");
>  

Extra blank line, please remove

@@ +13,4 @@
>  var windows = require("../../../../lib/windows");
>  
> +
> +var browser = require ("../../../lib/ui/browser");

Move this before downloads

@@ +19,1 @@
>  

No need for this blank line

@@ +57,1 @@
>  

No need for this blank line

::: firefox/tests/remote/testDownloading/testDownloadStates.js
@@ +12,1 @@
>  

Extra blank line, please remove

@@ +12,2 @@
>  
> +var browser = require("../../../lib/ui/browser");

Move this before downloads

@@ +16,1 @@
>  

No need for this blank line

@@ +33,1 @@
>  }

It would be good to also remove the downloads from the list.

::: lib/downloads.js
@@ +13,5 @@
> +const DOWNLOAD_TYPE = {
> +  all     : Downloads.ALL,
> +  public  : Downloads.PUBLIC,
> +  private : Downloads.PRIVATE
> +}

}; please

@@ +20,5 @@
>  const PREF_DOWNLOAD_FOLDERLIST = "browser.download.folderList";
>  
> +// List of available download states
> +// Original definition in nsIDownloadManager.idl
> +// which is deprecated but this information is reused through DownloadsCommon.jsm

This would look better as a multi-line comment

@@ +33,5 @@
>    blockedParental : 6,
>    scanning        : 7,
>    dirty           : 8,
>    blockedPolicy   : 9
>  }

}; please

Comment 69

[Teodor Druta]

Attachment: b908649v16.patch

(In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #68)
> Comment on attachment 8531575 [details] [diff] [review]
> b908649v15.patch
> 
> Review of attachment 8531575 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: firefox/lib/tests/testDownloadFileOfUnknownType.js
> @@ +13,5 @@
> > +
> > +function setupModule(aModule) {
> > +  aModule.browserWindow = new browser.BrowserWindow();
> > +}
> > +
> 
> Add teardownModule to clear the downloads list
> 

Fixed.

> ::: firefox/lib/tests/testDownloadsPanel.js
> @@ +9,5 @@
> > +var downloads = require("../../../lib/downloads");
> > +var prefs = require("../../../lib/prefs");
> > +var toolbars = require("../toolbars");
> > +
> > +var browser = require("../ui/browser");
> 
> You included this module twice, please remove from here
> 

Fixed.

> 
> ::: firefox/lib/ui/browser.js
> @@ +59,5 @@
> >  /**
> > + * Download the file of unknown type from the given UCTP by
> > + * saving it automatically to disk
> > + *
> > + * @param {UnknownContentTypeDialog} aDialog
> 
> Please add a description for the aDialog parameter.
> 

Fixed.

> @@ +62,5 @@
> > + *
> > + * @param {UnknownContentTypeDialog} aDialog
> > + */
> > +BrowserWindow.prototype.downloadFileOfUnknownType = function BW_downloadFileOfUT(aDialog) {
> > +  return unknownContentTypeDialog.downloadFileOfUnknownType(aDialog);
> 
> unknownContentTypeDialog.downloadFileOfUnknownType function doesn't return a
> value, so remove the "return" here
> 

Fixed.

> 
> @@ +13,4 @@
> >  var windows = require("../../../../lib/windows");
> >  
> > +
> > +var browser = require ("../../../lib/ui/browser");
> 
> Move this before downloads
> 
> @@ +12,2 @@
> >  
> > +var browser = require("../../../lib/ui/browser");
> 
> Move this before downloads
> 
> @@ +33,1 @@
> >  }
> 

I think that it was decided to separate ui libs from libs by a blank line.

Removed all extra blank spaces, added semicolons where it applied.

Comment 70

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8533615 [details] [diff] [review]
b908649v16.patch

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

::: firefox/tests/remote/testDownloading/testDownloadStates.js
@@ +39,2 @@
>   */
> +function testDownloadPanel() {

This test fails on OSX for me with 
ERROR | Test Failure | {
  "exception": {
    "message": "Expected window state has been reached - open (Window has been opened)", 
    "lineNumber": 27, 
    "name": "TimeoutError", 
    "fileName": "resource://mozmill/modules/errors.js"
  }
}

Comment 71

[Andreea Matei [:AndreeaMatei]]

Seems intermittent-rarely, it failed once, later runs passed. Might need checking a bit.

Comment 72

[Teodor Druta]

Attachment: b908649v16.1.patch

I tried to reproduce this issue both locally and on the 10.9 staging machine, with no luck. I'm pretty sure this is a networking related issue, the "Save Dialog" fails to open in time. I don't think there is something I can do with this.
Actually investigating this issue on the staging machine, I came accross another issue related to a too "speedy" network, the file was saved so fast, that the test didn't had time to check all the states, so I changed the TEST_DATA file to a bigger one.

Comment 73

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8534947 [details] [diff] [review]
b908649v16.1.patch

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

Seems fine now.

Comment 74

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

Comment on attachment 8534947 [details] [diff] [review]
b908649v16.1.patch

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

::: firefox/lib/tests/testDownloadFileOfUnknownType.js
@@ +27,5 @@
> +  var dialog = browserWindow.openUnknownContentTypeDialog(() => {
> +    browserWindow.controller.open(TEST_DATA);
> +  });
> +
> +  browserWindow.downloadFileOfUnknownType(dialog);

I don't see why we have to route this through the browser window. Isn't it just like `dialog.save()` or such?

::: firefox/lib/tests/testDownloadsPanel.js
@@ +23,5 @@
>    openButton: "toolbarbutton",
>    panel: "panel",
>    showAllDownloads: "button"
>  };
> +const PREF_PANEL_SHOWN = "browser.download.panel.shown";

nit: Please separate those const blocks.

@@ +28,4 @@
>  
>  function setupModule(aModule) {
>    aModule.browserWindow = new browser.BrowserWindow();
> +  aModule.navBar = new toolbars.NavBar(aModule.browserWindow);

Why has the current code been reverted? Also not sure why we need this separately declared. It's never used beside the line below.

@@ +51,5 @@
> +  var dialog = browserWindow.openUnknownContentTypeDialog(() => {
> +    browserWindow.controller.open(TEST_DATA.url);
> +  });
> +
> +  browserWindow.downloadFileOfUnknownType(dialog);

Same here as for the other unit test. Why that complicated?

@@ +66,5 @@
> +  // Cancel the download
> +  var cancelDownload = downloadsPanel.getElement({type: "downloadButton",
> +                                                  subtype: "cancel",
> +                                                  value: "1"});
> +  cancelDownload.click();

I don't see why we need this as part of the unit test. This is more a real test under the remote testrun.

@@ +73,5 @@
> +  assert.waitFor(() => {
> +    var state = downloadsPanel.getDownloadStatus(downloadItem);
> +    return (state === downloads.DOWNLOAD_STATE["canceled"]) ||
> +           (state === downloads.DOWNLOAD_STATE["finished"]);
> +  }, "Download has been canceled or finished");

Same here.

::: firefox/lib/toolbars.js
@@ +438,5 @@
>     *
>     * @params {object} [aSpec={}]
> +   *         Information about the panel to open
> +   * @params {string} [aSpec.method="button"|"callback"]
> +   *         Method to use for opening the Downloads Panel

As I mentioned a couple of times, possible values have to be part of the description and not the optional value part in line 1.

@@ +443,2 @@
>     * @params {function} [aSpec.callback]
> +   *         Callback function that triggers the opening

Why again 'function'? Please read review comments for your and others patches, so we can stop about complaining things like those which were done dozen of times.

::: firefox/lib/ui/browser.js
@@ +65,5 @@
> + *        Dialog object from which the download is saved
> + */
> +BrowserWindow.prototype.downloadFileOfUnknownType = function BW_downloadFileOfUT(aDialog) {
> +  unknownContentTypeDialog.downloadFileOfUnknownType(aDialog);
> +}

As mentioned earlier, I do not see why this is necessary.

::: firefox/lib/ui/unknown-content-type-dialog.js
@@ +79,5 @@
> + *
> + * @param {UnknownContentTypeDialog} aDialog
> + *        UnknownContentTypeDialog from where to download the file
> + */
> +function downloadFileOfUnknownType(aDialog) {

Why is that a global method and not part of the dialog class?

@@ +84,5 @@
> +  assert.ok(aDialog, "Dialog has been defined");
> +
> +  var saveFileRadio = aDialog.getElement({type: "save"});
> +
> +  if (saveFileRadio.getNode()) {

I don't like that. What happens in case of false? We would skip saving the file silently.

::: firefox/tests/functional/testPrivateBrowsing/testPrivateDownloadPanel.js
@@ +16,2 @@
>  
> +const PREF_PANEL_SHOWN = "browser.download.panel.shown";

Below the TEST_DATA const in its own block please.

@@ +29,5 @@
>  };
>  
> +function setupModule(aModule) {
> +  aModule.browserWindow = new browser.BrowserWindow();
> +  aModule.navBar = new toolbars.NavBar(aModule.browserWindow);

I don't see why having this variable is necessary.

@@ +35,4 @@
>  
> +  // PB Window
> +  aModule.pbBrowserWindow = browserWindow.open({private: true, method: "shortcut"});
> +  aModule.pbNavBar = new toolbars.NavBar(aModule.pbBrowserWindow);

Same here.

@@ +86,4 @@
>  
> +  // Open the Download Panel and check that we downloaded the correct files
> +  downloadsPanel.open();
> +  assert.equal(downloadsPanel.getElements({type: "downloads"}).length, 2,

As usual declare the element first.

@@ +86,5 @@
>  
> +  // Open the Download Panel and check that we downloaded the correct files
> +  downloadsPanel.open();
> +  assert.equal(downloadsPanel.getElements({type: "downloads"}).length, 2,
> +               "Normal Downloads are correctly shown in the Downloads Panel");

What are normal downloads? you mean non-privatebrowsing here.

@@ +92,2 @@
>  
> +  // Open the Private Download Panel and check that we downloaded the correct files

Same here, what is a "Private Download Panel". Please use the correct wording like 'Download panel of the private brwosing window".

::: firefox/tests/remote/testDownloading/testDownloadStates.js
@@ +14,2 @@
>  
> +const TEST_DATA = "ftp://ftp.mozilla.org/pub/firefox/releases/latest/mac/en-US/Firefox%2034.0.5.dmg";

Not sure how this test will survive the next release of Firefox, if you do it via latest.

@@ +17,5 @@
> +const PREF_PANEL_SHOWN = "browser.download.panel.shown";
> +
> +function setupModule(aModule) {
> +  aModule.browserWindow = new browser.BrowserWindow();
> +  aModule.navBar = new toolbars.NavBar(aModule.browserWindow);

Same as for the other files.

@@ +46,3 @@
>  
> +  downloadsPanel.open();
> +  expect.ok(downloadsPanel.isOpen, "Downloads panel is open");

This is a UI check, so you really want assert here.

@@ +63,5 @@
>  
> +  assert.waitFor(() => {
> +    var state = downloadsPanel.getDownloadStatus(downloadItem);
> +    return (state === downloads.DOWNLOAD_STATE["canceled"]) ||
> +           (state === downloads.DOWNLOAD_STATE["finished"]);

We should be sure to only check for canceled here. Otherwise we could miss issues.

::: lib/downloads.js
@@ +59,5 @@
> +      downloadList = aDownloads;
> +    });
> +  });
> +
> +  expect.waitFor(() => (downloadList !== null),

`() => !!downloadList`?

@@ +107,5 @@
> +/**
> + * Sets the download location
> + *
> + * @params {string} aDownloadLocation
> + *         Location for the downloads directory relative to the profile workspace.

s/workspace/directory

@@ +121,5 @@
> + *
> + * @param {string} [aType="all"]
> + *        Type of downloads to wait for ("private", "public")
> + * @param {number} aTimeout
> + *        Timeout to wait for the downloads to finish

Please do not put a mandatory parameter after an optional one.

Comment 75

[Teodor Druta]

Attachment: b908649v17.patch

(In reply to Henrik Skupin (:whimboo) from comment #74)
> 
> ::: firefox/lib/tests/testDownloadFileOfUnknownType.js
> @@ +27,5 @@
> > +  var dialog = browserWindow.openUnknownContentTypeDialog(() => {
> > +    browserWindow.controller.open(TEST_DATA);
> > +  });
> > +
> > +  browserWindow.downloadFileOfUnknownType(dialog);
> 
> I don't see why we have to route this through the browser window. Isn't it
> just like `dialog.save()` or such?
> 

Removed downloadFileOfUnknownType function from browser windows,
moved downloadFileOfUnknownType function to save method for the UnknownContentTypeDialog class.


> ::: firefox/lib/tests/testDownloadsPanel.js
> @@ +23,5 @@
> >    openButton: "toolbarbutton",
> >    panel: "panel",
> >    showAllDownloads: "button"
> >  };
> > +const PREF_PANEL_SHOWN = "browser.download.panel.shown";
> 
> nit: Please separate those const blocks.
> 

Separated by an empty line.

> @@ +28,4 @@
> >  
> >  function setupModule(aModule) {
> >    aModule.browserWindow = new browser.BrowserWindow();
> > +  aModule.navBar = new toolbars.NavBar(aModule.browserWindow);
> 
> Why has the current code been reverted? Also not sure why we need this
> separately declared. It's never used beside the line below.
> 

Updated it to "aModule.downloadsPanel = aModule.browserWindow.navBar.downloadsPanel;" everywhere it applied.

> 
> @@ +66,5 @@
> > +  // Cancel the download
> > +  var cancelDownload = downloadsPanel.getElement({type: "downloadButton",
> > +                                                  subtype: "cancel",
> > +                                                  value: "1"});
> > +  cancelDownload.click();
> 
> I don't see why we need this as part of the unit test. This is more a real
> test under the remote testrun.
> 
> @@ +73,5 @@
> > +  assert.waitFor(() => {
> > +    var state = downloadsPanel.getDownloadStatus(downloadItem);
> > +    return (state === downloads.DOWNLOAD_STATE["canceled"]) ||
> > +           (state === downloads.DOWNLOAD_STATE["finished"]);
> > +  }, "Download has been canceled or finished");
> 
> Same here.
> 

Yes, this is already tested in the remote test, removed the lines.

> ::: firefox/lib/toolbars.js
> @@ +438,5 @@
> >     *
> >     * @params {object} [aSpec={}]
> > +   *         Information about the panel to open
> > +   * @params {string} [aSpec.method="button"|"callback"]
> > +   *         Method to use for opening the Downloads Panel
> 
> As I mentioned a couple of times, possible values have to be part of the
> description and not the optional value part in line 1.
> 
> @@ +443,2 @@
> >     * @params {function} [aSpec.callback]
> > +   *         Callback function that triggers the opening
> 
> Why again 'function'? Please read review comments for your and others
> patches, so we can stop about complaining things like those which were done
> dozen of times.
> 

Fixed...

> 
> ::: firefox/lib/ui/unknown-content-type-dialog.js
> @@ +79,5 @@
> > + *
> > + * @param {UnknownContentTypeDialog} aDialog
> > + *        UnknownContentTypeDialog from where to download the file
> > + */
> > +function downloadFileOfUnknownType(aDialog) {
> 
> Why is that a global method and not part of the dialog class?
> 

Moved to a "save" method of the UnknownContentTypeDialog class

> @@ +84,5 @@
> > +  assert.ok(aDialog, "Dialog has been defined");
> > +
> > +  var saveFileRadio = aDialog.getElement({type: "save"});
> > +
> > +  if (saveFileRadio.getNode()) {
> 
> I don't like that. What happens in case of false? We would skip saving the
> file silently.
> 

Removed the if, added an assert.waitFor instead, it's more correct this way and we should wait and check if the save radio button is enabled before clicking it.

> ::: firefox/tests/functional/testPrivateBrowsing/testPrivateDownloadPanel.js
> @@ +16,2 @@
> >  
> > +const PREF_PANEL_SHOWN = "browser.download.panel.shown";
> 
> Below the TEST_DATA const in its own block please.
> 

Fixed.

> 
> @@ +86,4 @@
> >  
> > +  // Open the Download Panel and check that we downloaded the correct files
> > +  downloadsPanel.open();
> > +  assert.equal(downloadsPanel.getElements({type: "downloads"}).length, 2,
> 
> As usual declare the element first.
> 

Cached to a variable.

> @@ +86,5 @@
> >  
> > +  // Open the Download Panel and check that we downloaded the correct files
> > +  downloadsPanel.open();
> > +  assert.equal(downloadsPanel.getElements({type: "downloads"}).length, 2,
> > +               "Normal Downloads are correctly shown in the Downloads Panel");
> 
> What are normal downloads? you mean non-privatebrowsing here.
> 

Renamed to Non-Private Downloads

> @@ +92,2 @@
> >  
> > +  // Open the Private Download Panel and check that we downloaded the correct files
> 
> Same here, what is a "Private Download Panel". Please use the correct
> wording like 'Download panel of the private brwosing window".
> 

Fixed.

> ::: firefox/tests/remote/testDownloading/testDownloadStates.js
> @@ +14,2 @@
> >  
> > +const TEST_DATA = "ftp://ftp.mozilla.org/pub/firefox/releases/latest/mac/en-US/Firefox%2034.0.5.dmg";
> 
> Not sure how this test will survive the next release of Firefox, if you do
> it via latest.
> 

Changed to another firefox build.

> 
> @@ +46,3 @@
> >  
> > +  downloadsPanel.open();
> > +  expect.ok(downloadsPanel.isOpen, "Downloads panel is open");
> 
> This is a UI check, so you really want assert here.
> 

Changed to assert.

> @@ +63,5 @@
> >  
> > +  assert.waitFor(() => {
> > +    var state = downloadsPanel.getDownloadStatus(downloadItem);
> > +    return (state === downloads.DOWNLOAD_STATE["canceled"]) ||
> > +           (state === downloads.DOWNLOAD_STATE["finished"]);
> 
> We should be sure to only check for canceled here. Otherwise we could miss
> issues.
> 

Removed the check for "finished" download state

> ::: lib/downloads.js
> @@ +59,5 @@
> > +      downloadList = aDownloads;
> > +    });
> > +  });
> > +
> > +  expect.waitFor(() => (downloadList !== null),
> 
> `() => !!downloadList`?
> 

Fixed.

> @@ +107,5 @@
> > +/**
> > + * Sets the download location
> > + *
> > + * @params {string} aDownloadLocation
> > + *         Location for the downloads directory relative to the profile workspace.
> 
> s/workspace/directory
> 

Changed comment to "Location for the downloads directory relative to the s/workspace/directory"

> @@ +121,5 @@
> > + *
> > + * @param {string} [aType="all"]
> > + *        Type of downloads to wait for ("private", "public")
> > + * @param {number} aTimeout
> > + *        Timeout to wait for the downloads to finish
> 
> Please do not put a mandatory parameter after an optional one.

Fixed.

Comment 76

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8539240 [details] [diff] [review]
b908649v17.patch

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

::: firefox/lib/tests/manifest.ini
@@ +6,5 @@
>  [testAddonsManager.js]
>  [testBaseInContentPage.js]
>  [testBrowserWindow.js]
> +[testDownloadFileOfUnknownType.js]
> +[testDownloadPanel.js]

The name of the file is "testDownloadsPanel.js"

::: firefox/lib/tests/testDownloadFileOfUnknownType.js
@@ +14,5 @@
> +function setupModule(aModule) {
> +  aModule.browserWindow = new browser.BrowserWindow();
> +}
> +
> +

You have 2 blank lines here, please remove one of them

@@ +35,5 @@
> +  expect.equal(downloadList.length, 1,
> +               "One download has been added to the list");
> +
> +  // Shouldn't take more than 2 seconds to download the local file
> +  expect.waitFor(() => downloadList[0].succeeded, 2000 ,"Download finished");

You have one misplaced blank space before the last comma, it should be be after it

::: firefox/tests/functional/testPrivateBrowsing/testPrivateDownloadPanel.js
@@ +95,5 @@
> +  // and check that we downloaded the correct files
> +  pbDownloadsPanel.open();
> +  downloadsList = pbDownloadsPanel.getElements({type: "downloads"});
> +  assert.equal(downloadsList.length, 1,
> +               "Private Downloads are correctly shown in the Private Downloads Panel");

You should update the message to "Private Downloads are correctly shown in the Downloads Panel of the private window" (make sure you also split it on 2 lines as it's too long for a single line message)

::: firefox/tests/remote/testDownloading/testDownloadStates.js
@@ +33,5 @@
>  }
>  
> +/**
> + * Test download states:
> + * Downloading, Finished, Canceled, and Retry

You don't test the "Finished" state here. Please either update the comment or add a test for "Finished"

::: lib/downloads.js
@@ +60,5 @@
> +    });
> +  });
> +
> +  expect.waitFor(() => !!downloadList,
> +                 "Download list has been retrieved");

This can be single line

@@ +107,5 @@
> +/**
> + * Sets the download location
> + *
> + * @params {string} aDownloadLocation
> + *         Location for the downloads directory relative to the s/workspace/directory

This should have been "Location for the downloads directory relative to the profile directory"

@@ +139,2 @@
>  
> +  assert.waitFor(() => (activeDownloads === 0),

Put aTimeout on the previous line, leaving the message alone on a new line.

Comment 77

[Teodor Druta]

Attachment: b908649v18.patch

Fixed all nits, fixed the typo in the manifest file.

> ::: firefox/tests/remote/testDownloading/testDownloadStates.js
> @@ +33,5 @@
> >  }
> >  
> > +/**
> > + * Test download states:
> > + * Downloading, Finished, Canceled, and Retry
> 
> You don't test the "Finished" state here. Please either update the comment
> or add a test for "Finished"

I think that we should not test the finished state in our testruns, it will add useless time to the jobs. Removed the Finished from the comment.

Comment 78

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8545265 [details] [diff] [review]
b908649v18.patch

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

::: firefox/lib/tests/testDownloadsPanel.js
@@ +6,5 @@
>  
>  // Include required modules
> +var downloads = require("../../../lib/downloads");
> +var prefs = require("../../../lib/prefs");
> +var toolbars = require("../toolbars");

You don't use toolbars in the test, please remove it.

::: firefox/tests/remote/testDownloading/testDownloadStates.js
@@ +6,5 @@
>  
>  // Include required modules
> +var downloads = require("../../../../lib/downloads");
> +var prefs = require("../../../../lib/prefs");
> +var toolbars = require("../../../lib/toolbars");

You don't use toolbars, please remove it.

::: lib/downloads.js
@@ +82,4 @@
>   */
> +function removeAllDownloads(aType="all") {
> +  var removed = 0;
> +  var toRemove = null;

Please set the initial value to 0, since you are using it to store a number.

@@ +123,5 @@
> + * @param {string} [aType="all"]
> + *        Type of downloads to wait for ("private", "public")
> + */
> +function waitAllDownloadsFinished(aTimeout, aType="all") {
> +  var activeDownloads = null;

Please initialize with 0.

Comment 79

[Teodor Druta]

Attachment: b908649v19.patch

Removed unused toolbars.js module from the two tests, changed downloads.js functions counters instantiation from null to 0.

Comment 80

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8547437 [details] [diff] [review]
b908649v19.patch

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

r+ with these nits fixed.

::: firefox/tests/functional/testPrivateBrowsing/testPrivateDownloadPanel.js
@@ +88,5 @@
> +  downloadsPanel.open();
> +  var downloadsList = downloadsPanel.getElements({type: "downloads"});
> +  assert.equal(downloadsList.length, 2,
> +               "Non-private downloads are correctly shown in the Downloads" +
> +               " Panel of the non-private browser window");

Add the whitespace on the first message line.

@@ +97,5 @@
> +  pbDownloadsPanel.open();
> +  downloadsList = pbDownloadsPanel.getElements({type: "downloads"});
> +  assert.equal(downloadsList.length, 1,
> +               "Private downloads are correctly shown in the Downloads Panel" +
> +               " of the private browser window");

Same here.

::: firefox/tests/remote/testDownloading/testDownloadStates.js
@@ +62,3 @@
>  
> +  assert.waitFor(() => {
> +    var state = downloadsPanel.getDownloadStatus(downloadItem);

I would declare the state outside the waitFor and remove 'var' from all the waitFors in the test.

Comment 81

[Teodor Druta]

Attachment: b908649v20.patch

Fixed the nits reported by Andreea.
Henrik, can you please give a review ?

Comment 82

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

Comment on attachment 8548938 [details] [diff] [review]
b908649v20.patch

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

::: firefox/lib/tests/testDownloadsPanel.js
@@ +11,4 @@
>  var browser = require("../ui/browser");
>  
> +const TEST_DATA = {
> +  url: "ftp://ftp.mozilla.org/pub/firefox/releases/3.6/mac/en-US/Firefox%203.6.dmg",

Can't this be a local download? We do not specifically check behavior which is time dependent.

::: firefox/lib/ui/unknown-content-type-dialog.js
@@ +81,5 @@
> +  var saveFileRadio = this.getElement({type: "save"});
> +
> +  assert.waitFor(() => !!saveFileRadio.getNode(),
> +                 "Save File radio button on the Download " +
> +                 "Unknown Type dialog has been enabled");

There is no reason to explicitely mention unknown type dialog here. That's where we are in anyway.

@@ +86,5 @@
> +
> +  saveFileRadio.click();
> +  assert.waitFor(() => saveFileRadio.getNode().selected,
> +                 "Save File radio button on the Download " +
> +                 "Unknown Type dialog has been selected");

Why do we have to wait for here? It will immediately be selected.

@@ +93,5 @@
> +  var acceptButton = this.getElement({type: "button", subtype: "accept"});
> +  assert.waitFor(() => (acceptButton.exists() &&
> +                        !acceptButton.getNode().hasAttribute("disabled")),
> +                        "The OK button has been enabled");
> +  this.close(() => { acceptButton.click(); });

Can you please file a bug that this class needs an update once the dialog class has been landed? Thanks.

::: firefox/tests/functional/testPrivateBrowsing/testPrivateDownloadPanel.js
@@ +55,2 @@
>  
> +  downloads.removeAllDownloads();

Please move this up so it is right next to the other call into downloads.

@@ +88,5 @@
> +  downloadsPanel.open();
> +  var downloadsList = downloadsPanel.getElements({type: "downloads"});
> +  assert.equal(downloadsList.length, 2,
> +               "Non-private downloads are correctly shown in the Downloads " +
> +               "Panel of the non-private browser window");

Please be less expressive in your messages. It's clear which element is checked here. So only mention the state we expect here. This also applies to other code.

::: firefox/tests/remote/testDownloading/testDownloadStates.js
@@ +14,2 @@
>  
> +const TEST_DATA = "ftp://ftp.mozilla.org/pub/firefox/releases/34.0.5/mac/en-US/Firefox%2034.0.5.dmg";

Maybe we can use one of the video files as available on mozqa.com?

::: lib/downloads.js
@@ +20,5 @@
>  const PREF_DOWNLOAD_FOLDERLIST = "browser.download.folderList";
>  
> +// List of available download states
> +// Original definition in nsIDownloadManager.idl
> +// which is deprecated but this information is reused through DownloadsCommon.jsm

If those states are still used in DownloadsCommon.js why are they deprecated? I don't think we would have to reference the interface anytime longer.

@@ +38,5 @@
> +
> +/**
> + * Clears the download location prefs
> + */
> +function clearDownloadLocationPrefs() {

I would suggest we have setDownloadLocation and resetDownloadLocation. It doesn't matter which code is executed in that method.

@@ +47,5 @@
>  /**
> + * Returns a new DownloadList object.
> + *
> + * @param {string} [aType="all"]
> + *        Type of downloads to return ("private", "public")

You missed to list "all" here.

@@ +79,3 @@
>   *
> + * @params {string} [aType="all"]
> + *         Type of downloads to remove ("private", "public")

Same for "all"

@@ +120,5 @@
> + *
> + * @param {number} aTimeout
> + *        Timeout to wait for the downloads to finish
> + * @param {string} [aType="all"]
> + *        Type of downloads to wait for ("private", "public")

Same for "all"

@@ +138,3 @@
>  
> +  assert.waitFor(() => (activeDownloads === 0), aTimeout,
> +                 "All downloads have been finished");

This assert is wrong and is causing a race condition. You have to initialize the activeDownloads variable with null.

Comment 83

[Teodor Druta]

(In reply to Henrik Skupin (:whimboo) from comment #82)
> Comment on attachment 8548938 [details] [diff] [review]
> b908649v20.patch
> 
> Review of attachment 8548938 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: firefox/lib/tests/testDownloadsPanel.js
> @@ +11,4 @@
> >  var browser = require("../ui/browser");
> >  
> > +const TEST_DATA = {
> > +  url: "ftp://ftp.mozilla.org/pub/firefox/releases/3.6/mac/en-US/Firefox%203.6.dmg",
> 
> Can't this be a local download? We do not specifically check behavior which
> is time dependent.
> 

I'm actually against this, since this is just a unit test, I don't want to add a new file to the data/downloading folder with a "known file type", which will have a few kB, that will only add more useless space to our mercurial mozmill-tests repository.

Comment 84

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

Unit tests should never reach the network. And I don't see what speaks against adding a file with e.g. 2kB.

Comment 85

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

Btw. I checked that and we already have those files present:
http://hg.mozilla.org/qa/mozmill-tests/file/default/data/downloading

Comment 86

[Teodor Druta]

(In reply to Henrik Skupin (:whimboo) from comment #85)
> Btw. I checked that and we already have those files present:
> http://hg.mozilla.org/qa/mozmill-tests/file/default/data/downloading

Yeah, those are "unknown type" files, which are handle by a different type of dialog (unknown content type dialog). firefox/lib/tests/testDownloadsPanel.js requires a "known type" file. These file types can be viewed under Preferences -> Applications.

Comment 87

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

So add a new file then.

Comment 88

[Teodor Druta]

Attachment: b908649v21.patch

(In reply to Henrik Skupin (:whimboo) from comment #82)
> ::: firefox/lib/ui/unknown-content-type-dialog.js
> @@ +93,5 @@
> > +  var acceptButton = this.getElement({type: "button", subtype: "accept"});
> > +  assert.waitFor(() => (acceptButton.exists() &&
> > +                        !acceptButton.getNode().hasAttribute("disabled")),
> > +                        "The OK button has been enabled");
> > +  this.close(() => { acceptButton.click(); });
> 
> Can you please file a bug that this class needs an update once the dialog
> class has been landed? Thanks.

Filed bug added as blocker of this bug and the dialog one.

> ::: firefox/tests/remote/testDownloading/testDownloadStates.js
> @@ +14,2 @@
> >  
> > +const TEST_DATA = "ftp://ftp.mozilla.org/pub/firefox/releases/34.0.5/mac/en-US/Firefox%2034.0.5.dmg";
> 
> Maybe we can use one of the video files as available on mozqa.com?

I tested this on staging, so we can't use a video file from mozqa.com, the reason behind this is that the production machines have an insanely download rate ~30MB/s, so the test fails because the file is downloading too fast, we need at least a file of minimum 60 MB, in order to have time to test all the states.

Also there apparently was a regression I needed to handle in Nightly, located here:
http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/content/downloads.js#1014

Everything else was fixed, testruns green.

Comment 89

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

(In reply to Teodor Druta from comment #88)
> I tested this on staging, so we can't use a video file from mozqa.com, the
> reason behind this is that the production machines have an insanely download
> rate ~30MB/s, so the test fails because the file is downloading too fast, we
> need at least a file of minimum 60 MB, in order to have time to test all the
> states.

It still can cause race conditions, because we never know how fast the download will be. Maybe you want to use larger files from our ftp, like for the crash symbols or tests, something like http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/31.0/source/firefox-31.0.bundle

Comment 90

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8553033 [details] [diff] [review]
b908649v21.patch

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

Looks good, just a few nits.
Ask for Andreea's review after you update the patch.

::: firefox/tests/functional/testPrivateBrowsing/testPrivateDownloadPanel.js
@@ +8,5 @@
>  
>  // Include the required modules
> +var downloads = require("../../../../lib/downloads");
> +var prefs = require("../../../../lib/prefs");
> +var toolbars = require("../../../lib/toolbars");

You don't use this lib

::: firefox/tests/remote/testDownloading/testDownloadStates.js
@@ +6,5 @@
>  
>  // Include required modules
> +var downloads = require("../../../../lib/downloads");
> +var prefs = require("../../../../lib/prefs");
> +var toolbars = require("../../../lib/toolbars");

You don't use this lib

::: lib/downloads.js
@@ +141,5 @@
> +// Exports of variables
> +exports.DOWNLOAD_STATE = DOWNLOAD_STATE;
> +
> +// Exports methods
> +exports.resetDownloadLocation = resetDownloadLocation;

Please move this after the removeAllDownloads export, to have them in order.

Comment 91

[Teodor Druta]

Attachment: b908649v21.1.patch

Fixed all the nits reported by Mihaela.
Updated the testDownloadStates TEST_DATA file as suggested by Henrik.

Comment 92

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8553171 [details] [diff] [review]
b908649v21.1.patch

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

LGTM.

Comment 93

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

Comment on attachment 8553171 [details] [diff] [review]
b908649v21.1.patch

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

There are two remaining nits to be fixed. No need to request review from me again.

::: firefox/lib/tests/testDownloadFileOfUnknownType.js
@@ +34,5 @@
> +  expect.equal(downloadList.length, 1,
> +               "One download has been added to the list");
> +
> +  // Shouldn't take more than 2 seconds to download the local file
> +  expect.waitFor(() => downloadList[0].succeeded, 2000, "Download finished");

The message comes before the timeout. Also I would leave the default timeout.

::: firefox/tests/functional/testPrivateBrowsing/testPrivateDownloadPanel.js
@@ +51,4 @@
>  
> +  aModule.downloadsPanel.close({force: true});
> +  aModule.pbDownloadsPanel.close({force: true});
> +  downloads.removeAllDownloads();

It's not exactly what I meant. I wanted it right next to resetDownloadLocation() which is the call into the downloads module.

Comment 94

[Teodor Druta]

Attachment: b908649v22.patch

Fixed the reported nits.
Thanks for the review, Henrik !

Comment 95

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8555711 [details] [diff] [review]
b908649v22.patch

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

https://hg.mozilla.org/qa/mozmill-tests/rev/a7d2feac16bf (default)