Closed Bug 911636 Opened 8 years ago Closed 7 years ago

Webapp runtime migration to Download.jsm

Categories

(Firefox Graveyard :: Web Apps, defect, P1)

x86_64
Linux
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 34

People

(Reporter: marco, Assigned: marco)

References

Details

Attachments

(1 file, 9 obsolete files)

The webapp runtime is using the toolkit module to handle downloads.
The webapp runtime is build together with Firefox, so we can't use one of the solutions proposed for Thunderbird (moving the code to comm-central). So I guess we'll need to migrate the toolkit module to use Downloads.jsm.
The new code is much better, so everyone should use it. "Moving the code" was only suggested for Thunderbird because they have development resource constraints.
What are the steps necessary to port the toolkit module to use Downloads.jsm or, if it's simpler, to create a new webapprt module that uses Downloads.jsm?
Paolo, could you give me some pointers?

Also, is the old download module still working? Is it going to stop working soon? If so, when?
(In reply to Marco Castelluccio [:marco] from comment #2)
> What are the steps necessary to port the toolkit module to use Downloads.jsm
> or, if it's simpler, to create a new webapprt module that uses Downloads.jsm?
> Paolo, could you give me some pointers?

This depends on the needs of the Webapp Runtime, in general any user interface to interact with downloads after they've started should be rewritten. We'll be removing all the legacy code from the Downloads Panel, and that could become a good example of how a user interface for Downloads.jsm could look like.

> Also, is the old download module still working? Is it going to stop working
> soon? If so, when?

nsIDownloadManager still works for all products except Firefox for Desktop. We don't have an exact date for the total decommissioning of nsIDownloadManager yet, this will depend on the effort required by the various migrations.
(In reply to :Paolo Amadini from comment #3)
> (In reply to Marco Castelluccio [:marco] from comment #2)
> > What are the steps necessary to port the toolkit module to use Downloads.jsm
> > or, if it's simpler, to create a new webapprt module that uses Downloads.jsm?
> > Paolo, could you give me some pointers?
> 
> This depends on the needs of the Webapp Runtime, in general any user
> interface to interact with downloads after they've started should be
> rewritten. We'll be removing all the legacy code from the Downloads Panel,
> and that could become a good example of how a user interface for
> Downloads.jsm could look like.

I guess we'll just need a simple dialog window with progress, etc..
(In reply to Marco Castelluccio [:marco] from comment #4)
> I guess we'll just need a simple dialog window with progress, etc..

It looks like it could have a structure similar to the Downloads Panel. Since it doesn't have an anchor, it should be in a separate window and without a count limit, similar to the old Downloads window.
Blocks: 851471
No longer blocks: 847863
Depends on: 847863
Another option (if that's feasible) is to add a small button (i. e. does not extend the size of) to the title bar (if it's going to stay like that) that opens up the same panel Firefox has. The button should only have an active and inactive state and should only appear if there was a download in the current session.
(In reply to Florian Bender from comment #6)
> Another option (if that's feasible) is to add a small button (i. e. does not
> extend the size of) to the title bar (if it's going to stay like that) that
> opens up the same panel Firefox has. The button should only have an active
> and inactive state and should only appear if there was a download in the
> current session.

If you have the possibility of adding an anchor, that seems like the best solution, though more complex. There is still the need for an overflow window (or scrollable panel) when many downloads are started.
Flags: firefox-backlog?
Component: Download Manager → Web Apps
Flags: firefox-backlog? → firefox-backlog-
Product: Toolkit → Firefox
Myk, Marco, any chance either of you would be able to pick this bug up, or know of someone who could?

We'd like to make progress on bug 851471 (particularly the part of it that prevents us from not building the old code for Firefox builds), and this is one of the only remaining blockers.

As we understand it, the work involved would be copying code from Fennec or Firefox's use of the new API, and combining it with some of the UI code from the old toolkit download manager interface (from toolkit/mozapps/downloads/content/).
Flags: needinfo?(myk)
Flags: needinfo?(mar.castelluccio)
IIRC, Paolo said it was easier to re-write a new implementation from scratch than transitioning the old toolkit UI, do I recall correctly?

I guess I can take this bug, I can't give you an ETA though.
Flags: needinfo?(mar.castelluccio) → needinfo?(paolo.mozmail)
The "backend" will need to be rewritten, yes, but assuming you want a UI similar to the old one, there are front-end pieces from it that you can repurpose.
(In reply to Marco Castelluccio [:marco] from comment #9)
> IIRC, Paolo said it was easier to re-write a new implementation from scratch
> than transitioning the old toolkit UI, do I recall correctly?

Yes, as Gavin said you might want to reuse some XUL portions though the associated JavaScript implementation should be rewritten from scratch. I think the recent Android implementation from bug 901360 might work as a starting point.

Let me know if you have any question!
Flags: needinfo?(paolo.mozmail)
(In reply to :Paolo Amadini from comment #11)
> (In reply to Marco Castelluccio [:marco] from comment #9)
> > IIRC, Paolo said it was easier to re-write a new implementation from scratch
> > than transitioning the old toolkit UI, do I recall correctly?
> 
> Yes, as Gavin said you might want to reuse some XUL portions though the
> associated JavaScript implementation should be rewritten from scratch. I
> think the recent Android implementation from bug 901360 might work as a
> starting point.
> 
> Let me know if you have any question!

For the time being, I've a single question. Should I modify the code under toolkit/mozapps/downloads/content/ to support Downloads.jsm (so that Thunderbird will benefit too) or should I write a new implementation under webapprt/?
(In reply to Marco Castelluccio [:marco] from comment #12)
> For the time being, I've a single question. Should I modify the code under
> toolkit/mozapps/downloads/content/ to support Downloads.jsm (so that
> Thunderbird will benefit too) or should I write a new implementation under
> webapprt/?

I would start with a webapprt-specific implementation.

Probably, it will be quite easy to copy it to Thunderbird or share it in Toolkit later, but for the moment I'd focus on creating a single starting point for that work that is easier to test and cater to your needs.
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #8)
> Myk, Marco, any chance either of you would be able to pick this bug up, or
> know of someone who could?

It sounds like Marco can take this bug.  But he isn't sure when he can do the work, so let us know if it's urgent to get it done within a particular timeframe/cycle!  Then Marco and I can look for someone else to take it on if he can't get to it by then.
Flags: needinfo?(myk)
In the meantime I'll assign the bug to myself (because I've started to work on it).
Assignee: nobody → mar.castelluccio
Status: NEW → ASSIGNED
Attached patch WIP (obsolete) — Splinter Review
Attachment #8428287 - Flags: feedback?(paolo.mozmail)
Summary: Evaluate webapp runtime migration to Download.jsm → Webapp runtime migration to Download.jsm
Attached patch WIP (obsolete) — Splinter Review
I've updated the download test too.
Attachment #8428287 - Attachment is obsolete: true
Attachment #8428287 - Flags: feedback?(paolo.mozmail)
Attachment #8428389 - Flags: feedback?(paolo.mozmail)
Comment on attachment 8428389 [details] [diff] [review]
WIP

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

Thank you so much for working on this! I only took a quick look for now, let me know if there is something specific on which you'd like feedback.

In general, I think this looks good if you want to use most of the old Downloads window styles and strings. The amount of code could be reduced by using different state handling, similar to what Android will do in bug 901360, but if you don't have a lot of time to spend on this, this solution also works.

::: webapprt/DownloadManager.jsm
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +this.EXPORTED_SYMBOLS = ["DownloadManager"];

I'd call this DownloadFrontend.jsm or something similar, to clarify the role of the module.

It seems you could simplify things by using a second view in the downloads window instead of registering an observer here. If you only leave the part that opens the window when a new download starts, there is no need to keep a list of downloads here.

@@ +95,5 @@
> +// nsIDownloadManager will not be available anymore (bug 851471). The
> +// old code in this module will be removed in bug 899110.
> +Components.manager.QueryInterface(Ci.nsIComponentRegistrar)
> +          .registerFactory(kTransferCid, "",
> +                           kTransferContractId, null);

Since downloads may be initiated by webpages, this operation should be done before any web page can load. I don't know enough of webapprt initialization to say whether this is already true, but it is something to take into account.
Attachment #8428389 - Flags: feedback?(paolo.mozmail) → feedback+
(In reply to :Paolo Amadini from comment #18)
> In general, I think this looks good if you want to use most of the old
> Downloads window styles and strings. The amount of code could be reduced by
> using different state handling, similar to what Android will do in bug
> 901360, but if you don't have a lot of time to spend on this, this solution
> also works.

Yes, the idea was to start with moving the code from toolkit to webapprt,
"translating" it to use Downloads.jsm.
There's a lot of CSS/locale files that depend on that old code, I'd like to avoid
rewriting it just yet (on Android there's just a simple notification,
no complex UI).

> It seems you could simplify things by using a second view in the downloads
> window instead of registering an observer here.
> If you only leave the part that opens the window when a new download starts,
> there is no need to keep a list of downloads here.

I thought I had to keep track of the finished downloads, instead I've noticed
DownloadList will call onDownloadAdded also for finished downloads.

> 
> @@ +95,5 @@
> > +// nsIDownloadManager will not be available anymore (bug 851471). The
> > +// old code in this module will be removed in bug 899110.
> > +Components.manager.QueryInterface(Ci.nsIComponentRegistrar)
> > +          .registerFactory(kTransferCid, "",
> > +                           kTransferContractId, null);
> 
> Since downloads may be initiated by webpages, this operation should be done
> before any web page can load. I don't know enough of webapprt initialization
> to say whether this is already true, but it is something to take into
> account.

I've moved this operation in Startup.jsm, before the app page is loaded.
Attached patch Patch (obsolete) — Splinter Review
The patch should be almost ready.

I've added a "downloads" subdirectory under webapprt/test/chrome,
where the tests of the download UI will be placed.

I've added some code to make writing UI tests easy (mocking the
Downloads.jsm module). I've added two simple tests that use these
helpers.
I'll add more tests either in another version of the patch or in
another bug.
Attachment #8428389 - Attachment is obsolete: true
Attachment #8436523 - Flags: review?(paolo.mozmail)
Comment on attachment 8436523 [details] [diff] [review]
Patch

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

The approach and modularization looks good! I haven't looked in detail at the implementation, as you mentioned many refactorings and potential fixes can still be done after this initial patch lands.

If you have any specific questions in mind related to the Downloads API, just let me know!

You may want to request feedback from a WebAppRT peer too.

::: toolkit/components/downloads/nsDownloadManager.cpp
@@ +950,1 @@
>  #if !defined(XP_WIN)

nit: Can probably become an #elseif now (and maybe put the Metro case first).

::: webapprt/DownloadFrontend.jsm
@@ +12,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "Downloads",
> +  "resource://gre/modules/Downloads.jsm");
> + 

nit: I suggest getting rid of spaces at end-of-line throughout all the files.

@@ +18,5 @@
> +  init: function() {
> +    if (!this._viewAdded) {
> +      Downloads.getList(Downloads.ALL)
> +               .then(list => list.addView(this))
> +               .then(null, Cu.reportError);

nit: The ".catch(Cu.reportError);" syntax is now available.

::: webapprt/Startup.jsm
@@ +144,5 @@
> +             
> +    // Override Toolkit's nsITransfer implementation with the one from the
> +    // JavaScript API for downloads. This will eventually be removed when
> +    // nsIDownloadManager will not be available anymore (bug 851471). The
> +    // old code in this module will be removed in bug 899110.

Hm, bug 899110 is actually fixed but it seems I forgot to remove this part of the comment :-)

(Feel free to fix the original comment if you want!)

::: webapprt/content/downloads/downloads.js
@@ +37,5 @@
> +  stateDirty: "stateDirty",
> +  downloadsTitleFiles: "downloadsTitleFiles",
> +  downloadsTitlePercent: "downloadsTitlePercent",
> +  fileExecutableSecurityWarningTitle: "fileExecutableSecurityWarningTitle",
> +  fileExecutableSecurityWarningDontAsk: "fileExecutableSecurityWarningDontAsk"

Some of these strings may be actually unused, it may be better to remove them now to avoid re-translating them.
Attachment #8436523 - Flags: review?(paolo.mozmail) → review+
(In reply to :Paolo Amadini from comment #21)
> ::: webapprt/content/downloads/downloads.js
> @@ +37,5 @@
> > +  stateDirty: "stateDirty",
> > +  downloadsTitleFiles: "downloadsTitleFiles",
> > +  downloadsTitlePercent: "downloadsTitlePercent",
> > +  fileExecutableSecurityWarningTitle: "fileExecutableSecurityWarningTitle",
> > +  fileExecutableSecurityWarningDontAsk: "fileExecutableSecurityWarningDontAsk"
> 
> Some of these strings may be actually unused, it may be better to remove
> them now to avoid re-translating them.

I've removed these from gStr. I've noticed the downloads.properties file is still used in the JS downloads implementation, so it's better to share it (so, I've also removed the downloads.properties file from the patch).
Given that the downloads.dtd is just copied over from toolkit, I guess we'll need to tell localizers to just copy their translated strings (so they won't need to re-translate everything all over again).
Attached patch Patch (obsolete) — Splinter Review
Asking Myk's review for the webapprt/ bits.
Attachment #8436523 - Attachment is obsolete: true
Attachment #8439625 - Flags: review?(myk)
Comment on attachment 8439625 [details] [diff] [review]
Patch

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

::: webapprt/DownloadFrontend.jsm
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +this.EXPORTED_SYMBOLS = ["DownloadFrontend"];

Nit: since DownloadFrontend implements a "view" in the new architecture, I'd call it DownloadView.

@@ +20,5 @@
> +      Downloads.getList(Downloads.ALL)
> +               .then(list => list.addView(this))
> +               .catch(Cu.reportError);
> +
> +      this._viewAdded = true;

It looks like this sets _viewAdded to true even if there was an error calling list.addView, while *uninit* sets it to false even if there was an error calling list.removeView, which means _viewAdded will be wrong after an error.

On the other hand, _viewAdded is only used to control the behavior of init/uninit, which doesn't seem necessary, since the implementation inits DownloadFrontend unconditionally and never uninits it.

Ok, tests uninit it.  But I'd rather not implement a public method just to support tests that can just as easily access DownloadFrontend's scope to do that work.

And in any case DownloadList.addView/removeView are implemented in a way that suggests they will always succeed, regardless of whether the view in question has already been added.

So I would initialize DownloadFrontend unconditionally (and internally) and have tests manipulate its scope as needed.

@@ +36,5 @@
> +  },
> +
> +  get recentWindow() {
> +    return Services.wm.getMostRecentWindow("Download:Manager");
> +  },

Nit: this is more like "openWindow" or "existingWindow" than "recentWindow" (which sounds like a window that isn't open anymore).

@@ +38,5 @@
> +  get recentWindow() {
> +    return Services.wm.getMostRecentWindow("Download:Manager");
> +  },
> +
> +  onDownloadAdded: function(aDownload) {

If there are downloads when DownloadFrontend is added to the list, this method will get called for each download.  Is that what we want?  If not, we should wait until after initialization to define the method.

(Perhaps it doesn't matter, because DownloadFrontend is initialized on startup, before any downloads have a chance to begin.)

::: webapprt/Startup.jsm
@@ +160,5 @@
>            documentElement.mozRequestFullScreen();
>        }, true);
>      }
>  
> +    DownloadFrontend.init();

If initialization takes a noticeable amount of time, then it's wise to delay doing it until this point; but I would do it by making initialization implicit (as recommended above) and simply importing the module (which isn't otherwise used in Startup.jsm).

::: webapprt/content/downloads/download.xml
@@ +4,5 @@
>     - 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/. -->
>  
>  <!DOCTYPE bindings [
> +  <!ENTITY % downloadDTD SYSTEM "chrome://webapprt/locale/downloads/downloads.dtd" >

I understand the desire for expediency, but it seems like a shame to copy so many files that we barely modify.  Do we really expect to customize this dialog significantly?  As far as I can tell, the only substantive changes we're making are to downloads.js, and everything else is just changing paths or attaching functions to the gDownloadUI object.

::: webapprt/content/downloads/downloads.js
@@ +19,5 @@
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "PluralForm",
> +  "resource://gre/modules/PluralForm.jsm");
> +
> +let gDownloadUI;

This too seems like a "view", given that we pass it to DownloadList.addView.

@@ +54,5 @@
> +    trans.getAnyTransferData({}, data, {});
> +    let [url, name] = data.value.QueryInterface(Ci.nsISupportsString).data.split("\n");
> +
> +    if (!url)
> +      return;

Nit: enclose block in braces.

@@ +74,5 @@
> + *        New string to put in place of placeholder
> + * @return The string with placeholder replaced with the new string
> + */
> +function replaceInsert(aText, aIndex, aValue)
> +{

Nit: put the brace at the end of the preceding line.

@@ +92,5 @@
> +
> +  this.updateData();
> +
> +  this.createItem();
> +}

Nit: end this statement with a semi-colon.

@@ +282,5 @@
> +      // Assume the file URL we obtained from the downloads database or from the
> +      // "spec" property of the target has the UTF-8 charset.
> +      let fileUrl = NetUtil.newURI(aFilename).QueryInterface(Ci.nsIFileURL);
> +      return fileUrl.file.clone();
> +    } else {

Nit: don't use an else block after an if block that returns.

@@ +292,5 @@
> +    }
> +  },
> +
> +  /**
> +   * Show a donwloaded file in the system file manager.

Nit: donwloaded -> downloaded

@@ +676,5 @@
> +  searchBox: null,
> +  searchTerms: [],
> +  searchAttributes: [ "target", "status", "dateTime", ],
> +  contextMenus: [
> +    // DOWNLOAD_DOWNLOADING

This seems like it'd be better as a hash of arrays, but I'm beginning to wonder if a bunch of this code is copied from elsewhere, where it's already been reviewed, and picking nits in it is not the best use of our time.

@@ +835,5 @@
> +      this.searchBox.doCommand();
> +      this.downloadsView.focus();
> +    } catch (e) {
> +      Cu.reportError(e);
> +    }

Instead of surrounding the entire body of the function with a try/catch block that merely reports an error, we should let callers catch the exception and report it (or let it get reported as an uncaught exception).

@@ +1153,5 @@
> +  // convert strings to those in the string bundle
> +  let (sb = document.getElementById("downloadStrings")) {
> +    let getStr = function(string) sb.getString(string);
> +    for (let [name, value] in Iterator(gStr))
> +      gStr[name] = typeof value == "string" ? getStr(value) : value.map(getStr);

Nit: surround for block with braces.

Also, all of the values in gStr are strings, and their names and values are all identical (with the exception of "stateBlockedParentalControls": "stateBlocked").  Do we really need the complexity that comes from letting gStr names map to multiple and/or differently-named values?

::: webapprt/test/chrome/browser_download.js
@@ +112,5 @@
>  function test() {
>    waitForExplicitFinish();
>  
>    loadWebapp("download.webapp", undefined, function onLoad() {
> +    Task.spawn(function() {

Nit: make this a function*().
Attachment #8439625 - Flags: review?(myk) → review-
(In reply to Myk Melez [:myk] [@mykmelez] from comment #24)
> > +this.EXPORTED_SYMBOLS = ["DownloadFrontend"];
> 
> Nit: since DownloadFrontend implements a "view" in the new architecture, I'd
> call it DownloadView.

There are multiple views that can be added with addView for different purposes, even in the back-end, thus I suggest using DownloadFrontendView or something with similar specificity.

> I understand the desire for expediency, but it seems like a shame to copy so
> many files that we barely modify.  Do we really expect to customize this
> dialog significantly?  As far as I can tell, the only substantive changes
> we're making are to downloads.js, and everything else is just changing paths
> or attaching functions to the gDownloadUI object.

I don't think there is an easy way to share the front-end XUL with different JavaScript, without going through hoops, so it is better to copy all the files. The reason for copying the files in the first place rather than modifying the Toolkit code is that we'll be able to change the UI and fix bugs in the future without the risk of breaking SeaMonkey and Thunderbird at each step.

I also think there are many opportunities for simplifying the code further, though this is better done in follow-up bugs.
Priority: -- → P1
See Also: → 907732
(In reply to Myk Melez [:myk] [@mykmelez] from comment #24)
> 
> @@ +38,5 @@
> > +  get recentWindow() {
> > +    return Services.wm.getMostRecentWindow("Download:Manager");
> > +  },
> > +
> > +  onDownloadAdded: function(aDownload) {
> 
> If there are downloads when DownloadFrontend is added to the list, this
> method will get called for each download.  Is that what we want?  If not, we
> should wait until after initialization to define the method.
> 
> (Perhaps it doesn't matter, because DownloadFrontend is initialized on
> startup, before any downloads have a chance to begin.)

Yeah, I think it doesn't matter because even if it's called multiple times, it won't
open more than one window.

> 
> ::: webapprt/Startup.jsm
> @@ +160,5 @@
> >            documentElement.mozRequestFullScreen();
> >        }, true);
> >      }
> >  
> > +    DownloadFrontend.init();
> 
> If initialization takes a noticeable amount of time, then it's wise to delay
> doing it until this point; but I would do it by making initialization
> implicit (as recommended above) and simply importing the module (which isn't
> otherwise used in Startup.jsm).

For some reason this doesn't work. I'm not sure why.
I've just moved the DownloadFrontend.init() line in DownloadFrontendView.jsm,
replacing it with Cu.import(".../DownloadFrontendView.jsm").

> 
> ::: webapprt/content/downloads/download.xml
> @@ +4,5 @@
> >     - 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/. -->
> >  
> >  <!DOCTYPE bindings [
> > +  <!ENTITY % downloadDTD SYSTEM "chrome://webapprt/locale/downloads/downloads.dtd" >
> 
> I understand the desire for expediency, but it seems like a shame to copy so
> many files that we barely modify.  Do we really expect to customize this
> dialog significantly?  As far as I can tell, the only substantive changes
> we're making are to downloads.js, and everything else is just changing paths
> or attaching functions to the gDownloadUI object.

I guess we could avoid copying the downloads.dtd, downloads.css and skin/ files.
I'm not sure what the other users of this dialog are going to do though, IIRC
Thunderbird wanted to have a different UI to support downloads, so maybe it's
useless to share these files if we're going to be their only users.

If we wanted to share the XUL and XML file too, we'd need to either introduce
the gDownloadUI object in the old downloads.js too or ditch it in the new
downloads.js.

> 
> @@ +676,5 @@
> > +  searchBox: null,
> > +  searchTerms: [],
> > +  searchAttributes: [ "target", "status", "dateTime", ],
> > +  contextMenus: [
> > +    // DOWNLOAD_DOWNLOADING
> 
> This seems like it'd be better as a hash of arrays, but I'm beginning to
> wonder if a bunch of this code is copied from elsewhere, where it's already
> been reviewed, and picking nits in it is not the best use of our time.

Yes, many functions come from the old downloads.js. I haven't hg copied it because
there are so many modifications that would make the file quite harder to review.

> @@ +835,5 @@
> > +      this.searchBox.doCommand();
> > +      this.downloadsView.focus();
> > +    } catch (e) {
> > +      Cu.reportError(e);
> > +    }
> 
> Instead of surrounding the entire body of the function with a try/catch
> block that merely reports an error, we should let callers catch the
> exception and report it (or let it get reported as an uncaught exception).

The caller was some inline code in the XUL file, this is why I was
reporting the exception here. I'll just let it get reported as an uncaught
exception.
(In reply to :Paolo Amadini from comment #25)
> There are multiple views that can be added with addView for different
> purposes, even in the back-end, thus I suggest using DownloadFrontendView or
> something with similar specificity.

In the case of the desktop runtime, there's only a single "view" file, so it would be premature to distinguish it from other views that we haven't yet implemented.  Even if it's possible that we'll do so in the future, I'd rather delay renaming the file until we actually have other views.


> I don't think there is an easy way to share the front-end XUL with different
> JavaScript, without going through hoops, so it is better to copy all the
> files. The reason for copying the files in the first place rather than
> modifying the Toolkit code is that we'll be able to change the UI and fix
> bugs in the future without the risk of breaking SeaMonkey and Thunderbird at
> each step.

Ok, that makes sense.  We can always revisit this issue and refactor code as appropriate in the future, if it becomes clear that we should do so.


> I also think there are many opportunities for simplifying the code further,
> though this is better done in follow-up bugs.

Indeed!


(In reply to Marco Castelluccio [:marco] from comment #26)
> (In reply to Myk Melez [:myk] [@mykmelez] from comment #24)
> > If there are downloads when DownloadFrontend is added to the list, this
> > method will get called for each download.  Is that what we want?  If not, we
> > should wait until after initialization to define the method.
> > 
> > (Perhaps it doesn't matter, because DownloadFrontend is initialized on
> > startup, before any downloads have a chance to begin.)
> 
> Yeah, I think it doesn't matter because even if it's called multiple times,
> it won't
> open more than one window.

Ok.  It might still be unexpected for it to open one window on startup, but perhaps it's better to do so anyway, if there is somehow a download then.


> > If initialization takes a noticeable amount of time, then it's wise to delay
> > doing it until this point; but I would do it by making initialization
> > implicit (as recommended above) and simply importing the module (which isn't
> > otherwise used in Startup.jsm).
> 
> For some reason this doesn't work. I'm not sure why.
> I've just moved the DownloadFrontend.init() line in DownloadFrontendView.jsm,
> replacing it with Cu.import(".../DownloadFrontendView.jsm").

Hmm, that should work.  Do you have a work-in-progress patch (or a branch) I can look at?


> > I understand the desire for expediency, but it seems like a shame to copy so
> > many files that we barely modify.  Do we really expect to customize this
> > dialog significantly?  As far as I can tell, the only substantive changes
> > we're making are to downloads.js, and everything else is just changing paths
> > or attaching functions to the gDownloadUI object.
> 
> I guess we could avoid copying the downloads.dtd, downloads.css and skin/
> files.
> I'm not sure what the other users of this dialog are going to do though, IIRC
> Thunderbird wanted to have a different UI to support downloads, so maybe it's
> useless to share these files if we're going to be their only users.
> 
> If we wanted to share the XUL and XML file too, we'd need to either introduce
> the gDownloadUI object in the old downloads.js too or ditch it in the new
> downloads.js.

Ok, you and Paolo have convinced me to copy all the files for now!


> > Instead of surrounding the entire body of the function with a try/catch
> > block that merely reports an error, we should let callers catch the
> > exception and report it (or let it get reported as an uncaught exception).
> 
> The caller was some inline code in the XUL file, this is why I was
> reporting the exception here. I'll just let it get reported as an uncaught
> exception.

Sounds good, I think that's a better option.  It's what uncaught exception reporting is made for!
(In reply to Myk Melez [:myk] [@mykmelez] from comment #27)
> (In reply to :Paolo Amadini from comment #25)
> > There are multiple views that can be added with addView for different
> > purposes, even in the back-end, thus I suggest using DownloadFrontendView or
> > something with similar specificity.
> 
> In the case of the desktop runtime, there's only a single "view" file, so it
> would be premature to distinguish it from other views that we haven't yet
> implemented.  Even if it's possible that we'll do so in the future, I'd
> rather delay renaming the file until we actually have other views.

There's the gDownloadUI object that is a view too (even if it's not in its own
file).

> 
> > > If initialization takes a noticeable amount of time, then it's wise to delay
> > > doing it until this point; but I would do it by making initialization
> > > implicit (as recommended above) and simply importing the module (which isn't
> > > otherwise used in Startup.jsm).
> > 
> > For some reason this doesn't work. I'm not sure why.
> > I've just moved the DownloadFrontend.init() line in DownloadFrontendView.jsm,
> > replacing it with Cu.import(".../DownloadFrontendView.jsm").
> 
> Hmm, that should work.  Do you have a work-in-progress patch (or a branch) I
> can look at?
> 

I'm attaching the new WIP patch (I've almost completed all the requested changes, actually).
Actually, it's working as expected :/
I'll upload the complete patch tomorrow.
Attached patch Patch (obsolete) — Splinter Review
Attachment #8439625 - Attachment is obsolete: true
Attachment #8445178 - Flags: review?(myk)
Comment on attachment 8445178 [details] [diff] [review]
Patch

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

Building hits a preprocessor error:

10:19.29 mozbuild.preprocessor.Error: ('/Users/myk/Mozilla/gecko-dev/webapprt/themes/osx/downloads/downloads.css', 5, 'FILE_NOT_FOUND', '/Users/myk/Mozilla/gecko-dev/webapprt/themes/osx/downloads/../../global/shared.inc')


(In reply to Marco Castelluccio [:marco] from comment #28)
> There's the gDownloadUI object that is a view too (even if it's not in its
> own
> file).

Yes, that's true, but it's in a different namespace.

DownloadFrontendView currently has a single purpose: to open the Downloads window when a file starts downloading.  So if DownloadView feels insufficiently descriptive, then we could call it something like DownloadWindowOpenerView.

But we shouldn't call it DownloadFrontendView, because "frontend" provides no additional information in this context; it just makes the name unwieldy.

Alternately: move its functionality into the WebappRT object in WebappRT.jsm, which already has a startUpdateService function that registers handlers for update events, so it would be consistent to have a startDownloadObserver function that registers a handler for the downloadadded event.

As for gDownloadUI, it's more difficult to name, since it's a "view" in the eyes of the Downloads object, but it lives in a document that has a downloadView element, which it references and controls.  And it does other work besides simply relaying changes in the model to the downloadView element.

In fact it's the primary singleton global workhorse, which we reference often.  So we should pick a simple name for it, like DownloadView (despite downloadView) or DownloadList (with or without the "g" prefix).


Otherwise, I ended up reviewing downloads.js in its entirety, since it would have been difficult to distinguish between copied and new code, and I have a bunch of nits plus a few other issues.

::: webapprt/DownloadFrontendView.jsm
@@ +14,5 @@
> +this.DownloadFrontendView = {
> +  init: function() {
> +    Downloads.getList(Downloads.ALL)
> +             .then(list => list.addView(this))
> +             .catch(Cu.reportError);

Calls to "catch" like this one, which merely report the caught exception, are unnecessary, since the error would get reported anyway, and are a footgun, since the result of such a chain sometimes gets returned to a caller who expects to be able to append their own rejection handler.

It's fine to use "catch" when code actually needs to do something as the result of an exception, but otherwise we should not use it, here and elsewhere, and let unexpected exceptions propagate, just like we do for most synchronous code.

::: webapprt/content/downloads/downloads.js
@@ +23,5 @@
> +let gDownloadUIView;
> +
> +let gStr = {};
> +
> +function pasteHandler() {

This is the only function referenced by an event handler in downloads.xul that is not a property of gDownloadUIVIew.  Is there a specific reason for this inconsistency?  I know the function doesn't reference gDownloadUIVIew, but then neither do the ondragover and ondrop handlers on the richlistbox.

@@ +61,5 @@
> + * @return The string with placeholder replaced with the new string
> + */
> +function replaceInsert(aText, aIndex, aValue) {
> +  return aText.replace("#" + aIndex, aValue);
> +}

Nit: this replaces a single operation, like |gStr.paused.replace("#1", transfer)|, with an opaque function call that is harder to understand when reading it in-place, like |replaceInsert(gStr.paused, 1, transfer)|.  So it sacrifices readability without saving any space.  In fact, the refactored version is longer than the original:

  gStr.paused.replace("#1", transfer)
  replaceInsert(gStr.paused, 1, transfer)

Thus I would un-refactor this code.

@@ +136,5 @@
> +      // use the last known number of bytes transferred.  The final size on disk
> +      // will be available when bug 941063 is resolved.
> +      this.maxBytes = this._download.hasProgress ?
> +                             this._download.totalBytes :
> +                             this._download.currentBytes;

Nit: this would be a bit more readable as:

      this.maxBytes = this._download.hasProgress ? this._download.totalBytes
                                                 : this._download.currentBytes;

@@ +169,5 @@
> +   * This is true during the initial phases of a download, before the actual
> +   * download of data bytes starts.
> +   */
> +  get starting()
> +  {

Nit: put the opening brace on the same line as the get declaration.

@@ +266,5 @@
> +    if (aFilename.startsWith("file:")) {
> +      // Assume the file URL we obtained from the downloads database or from the
> +      // "spec" property of the target has the UTF-8 charset.
> +      let fileUrl = NetUtil.newURI(aFilename).QueryInterface(Ci.nsIFileURL);
> +      return fileUrl.file.clone();

Nit: I don't think clone() provides any benefit here.

@@ +315,5 @@
> +  },
> +
> +  /**
> +   * Resumes the download if paused, pauses it if active.
> +   * @throws if the download is not resumable or if has already done.

This comment seems incorrect, if this._download.cancel() calls <http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadCore.jsm?rev=69d61e42d5df#577>, because it doesn't toggle the download, and it doesn't throw: instead, it either cancels the download or does nothing, and either way it returns a promise that gets resolved.

Also, nit: here and elsewhere, functions like this one should return a promise, so their callers can observe the operation and its outcome.  In many cases, it's sufficient to simply return the promise that is returned by the function to which this function delegates.

@@ +324,5 @@
> +
> +  /**
> +   * Resumes the download if paused, pauses it if active.
> +   * @throws if the download is not resumable or if has already done.
> +   */

This comment is also incorrect, if this._download.start() calls <http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadCore.jsm?rev=69d61e42d5df#304>.

@@ +332,5 @@
> +
> +  /**
> +   * Attempts to retry the download.
> +   * @throws if we cannot.
> +   */

Ditto.

@@ +342,5 @@
> +   * Cancels the download.
> +   */
> +  cancel: function() {
> +    this._download.cancel();
> +    this._download.removePartialData().catch(Cu.reportError);

Nit: this synchronous sequence of asynchronous operations makes it look like removePartialData will race cancellation, although it turns out that removePartialData is smart enough to await cancellation, so this happens to work.

Nevertheless, to avoid misleading future hackers, I would explicitly wait for cancellation before removing partial data, either:

  cancel: function() {
    return this._download.cancel()
                         .then(() => this._download.removePartialData());
  },

Or:

  cancel: Task.async(function*() {
    yield this._download.cancel();
    yield this._download.removePartialData();
  }),

(Both examples also return a promise and avoid catching exceptions merely to report them, as described in review comments elsewhere.)

@@ +370,5 @@
> +    this.element.setAttribute("image", "moz-icon://" + this.file + "?size=32");
> +  },
> +
> +  /**
> +   * Updates the status for a download item depending on its state

Nit: here and elsewhere, end complete sentences with periods and use the imperative tense when describing behavior, i.e.:

    Update the status for a download item depending on its state.

@@ +416,5 @@
> +          stateSize[nsIDM.DOWNLOAD_FAILED] = function() gStr.stateFailed;
> +          stateSize[nsIDM.DOWNLOAD_CANCELED] = function() gStr.stateCanceled;
> +          stateSize[nsIDM.DOWNLOAD_BLOCKED_PARENTAL] = function() gStr.stateBlocked;
> +          stateSize[nsIDM.DOWNLOAD_BLOCKED_POLICY] = function() gStr.stateBlockedPolicy;
> +          stateSize[nsIDM.DOWNLOAD_DIRTY] = function() gStr.stateDirty;

Nit: use fat arrow functions for these lambdas too.

@@ +433,5 @@
> +        break;
> +    }
> +
> +    this.element.setAttribute("status", status);
> +    this.element.setAttribute("statusTip", statusTip != "" ? statusTip : status);

Nit: if statusType is an empty string, it'll evaluate to false in a conditional context, so this can be simply:

    this.element.setAttribute("statusTip", statusTip || status);

@@ +486,5 @@
> +    this.updateStatusText();
> +
> +    // Updates the disabled state of the buttons of a download
> +    let buttons = this.element.buttons;
> +

Nit: the comment actually describes the for(;;) loop, which isn't clear because of the blank line between the *buttons* declaration and the loop.  To clarify the comment, remove the blank line or move the comment to the loop.

@@ +527,5 @@
> +    // make it into one big string for easy combined searching
> +    let combinedSearch = "";
> +    for each (let attr in aSearchAttributes) {
> +      combinedSearch += this.element.getAttribute(attr).toLowerCase() + " ";
> +    }

Nit: this would be simpler as an array comprehension:

    let combinedSearch = [this.element.getAttribute(attr).toLowerCase()
                            for (attr of aSearchAttributes)].join(" ");

Or a map:

    let combinedSearch =
      aSearchAttributes.
      map(attr => this.element.getAttribute(attr).toLowerCase()).
      join(" ");

But it isn't clear why we're doing this at all.  The comment says it's for "easy combined searching," but this function would be simpler without combining attributes:

  matchesSearch: function(aSearchTerms, aSearchAttributes) {
    for (let term of aSearchTerms) {
      for (let attr of aSearchAttributes) {
        if (attr.indexOf(term) != -1) {
          return true;
        }
      }
    }
    return false;
  },

Or even just:

  matchesSearch: function(aTerms, aAttributes) {
    return aTerms.some(term => aAttributes.some(attr => attr.contains(term)));
  },

@@ +576,5 @@
> +    return false;
> +  },
> +
> +  doCommand: function(aCommand) {
> +    if (this.isCommandEnabled(aCommand)) {

Nit: if this function returned early when !this.isCommandEnabled(aCommand), then we would avoid having to indent the entire rest of the function an extra level.

@@ +580,5 @@
> +    if (this.isCommandEnabled(aCommand)) {
> +      switch (aCommand) {
> +        case "cmd_cancel":
> +          this.cancel();
> +        break;

Nit: here and below, indent the "break" statement an extra level relative to the "case" statement, as you've done above.

@@ +634,5 @@
> +    }
> +  },
> +};
> +
> +let DownloadUIView = function() {

Since this is only ever instantiated once, it should be a singleton object rather than a constructor function with a prototype.

@@ +635,5 @@
> +  },
> +};
> +
> +let DownloadUIView = function() {
> +  this.downloadsView = document.getElementById("downloadView");

Nit: use the singular "download" in the name of this property for consistency with the ID of the element (and also because it's more conventional to use the singular form of a noun being used as an adjective, as in this case).

@@ +648,5 @@
> +    if (e.keyCode == e.DOM_VK_ESCAPE) {
> +      // Move focus to the list instead of closing the window
> +      this.downloadsView.focus();
> +      e.preventDefault();
> +    }

Nit: I would make this handler be a method of the view object.

@@ +671,5 @@
> +      , "menuseparator"
> +      , "menuitem_openReferrer"
> +      , "menuitem_copyLocation"
> +      , "menuseparator"
> +      , "menuitem_selectAll"

Nit: this comma-delimitation format is useful when you want to avoid a trailing comma after the last item, but the rest of the code tends to use trailing commas, so it isn't clear why this doesn't.

@@ +774,5 @@
> +    let prevSearch = this.searchTerms.join(" ");
> +
> +    // Array of space-separated lower-case search terms
> +    this.searchTerms = this.searchBox.value.replace(/^\s+|\s+$/g, "")
> +                                           .toLowerCase().split(/\s+/);

Use String.trim():

  this.searchTerms = this.searchBox.value.trim().toLowerCase().split(/\s+/);

@@ +777,5 @@
> +    this.searchTerms = this.searchBox.value.replace(/^\s+|\s+$/g, "")
> +                                           .toLowerCase().split(/\s+/);
> +
> +    // Unless forced, don't rebuild the download list if the search didn't change
> +    if (!aForceBuild && this.searchTerms.join(" ") == prevSearch) {

Nit: to avoid rebuilding the download list when terms merely get reordered, also sort:

  this.searchTerms = this.searchBox.value.trim().toLowerCase().sort().split(/\s+/);

@@ +803,5 @@
> +    let searchTerms = this.searchTerms;
> +
> +    let list = yield Downloads.getList(Downloads.ALL);
> +
> +    yield list.removeFinished((aDownload) => {

Nit: I would avoid mixing promise chains with Tasks, as it's especially confusing to trace the execution flow through such mixtures.

@@ +924,5 @@
> +      // Convert the nodelist into an array to keep a copy of the download items
> +      let items = [];
> +      for (let i = this.downloadsView.selectedItems.length; --i >= 0; ) {
> +        items.unshift(this.downloadsView.selectedItems[i]);
> +      }

Nit: this'd be simpler as:

  let items = Array.prototype.slice.call(this.downloadsView.selectedItems);

@@ +933,5 @@
> +      }
> +
> +      return;
> +    } else {
> +      while (elm.nodeName != "richlistitem" ||

Nit: don't use an *else* block after an *if* block that returns.

@@ +944,5 @@
> +    downloadItem.doCommand(aCmd);
> +  },
> +
> +  onDragStart: function(aEvent) {
> +    if (!this.downloadsView.selectedItem){

Nit: space between closing parenthesis and opening brace.

@@ +1020,5 @@
> +        totalTransferred += downloadItem.currBytes;
> +      }
> +    }
> +
> +    // Use the default title and reset "last" values if there's no downloads

Nit: there's -> there are (plural conjugation)

@@ +1070,5 @@
> +  onDownloadChanged: function(aDownload) {
> +    let downloadItem = this.downloadItemsMap.get(aDownload);
> +    if (!downloadItem) {
> +      Cu.reportError("Download doesn't exist.");
> +      return;

Here and elsewhere, it seems like we should do something better when we observe an event about a download for which we don't have an item, like create an item for the download and add it to the list.  Do we know of reasons why this might happen, or is this simply out of an abundance of caution?

::: webapprt/content/downloads/downloads.xul
@@ +144,5 @@
>    <richlistbox id="downloadView" seltype="multiple" flex="1"
>                 context="downloadContextMenu"
> +               ondblclick="gDownloadUIView.onDownloadDblClick(event);"
> +               ondragstart="gDownloadUIView.onDragStart(event);"
> +               ondragover="gDownloadUIView.onDragOver(event);event.stopPropagation();"

Nit: while you're touching this code, take advantage of the opportunity to move the event.stopPropagation call into gDownloadUIView.onDragOver!

::: webapprt/test/chrome/browser_download.js
@@ +47,5 @@
> +        ok(fileDownloaded, "File downloaded");
> +
> +        executeSoon(() => {
> +          executeSoon(() => {
> +            test_downloadList(win, [download]);

Why does this code (and some other test code) chain a pair of executeSoon calls?  A single call just guarantees that we spin the event loop, which is reasonable under certain circumstances.  But multiple calls suggests that we're trying to wait "long enough," which is likely to fail on machines that are slower than our own.

If that's not why we're doing it, and there's a good reason to do it this way (f.e. we happen to know that it takes exactly two spins of the event loop for some code to execute), then at the very least we should document it.  Otherwise, we need to figure out why the tests fail if we don't do this.

::: webapprt/test/chrome/head.js
@@ +108,5 @@
> +  Downloads.getList = function(aKind) {
> +    return new Promise(function(resolve, reject) {
> +      resolve(list);
> +    });
> +  };

Nit: you should be able to use Promise.resolve here:

  Downloads.getList = () => Promise.resolve(list);
Attachment #8445178 - Flags: review?(myk) → review-
(In reply to Myk Melez [:myk] [@mykmelez] from comment #31)
> ::: webapprt/DownloadFrontendView.jsm
> @@ +14,5 @@
> > +this.DownloadFrontendView = {
> > +  init: function() {
> > +    Downloads.getList(Downloads.ALL)
> > +             .then(list => list.addView(this))
> > +             .catch(Cu.reportError);
> 
> Calls to "catch" like this one, which merely report the caught exception,
> are unnecessary, since the error would get reported anyway

That's a misconception: asynchronous error reporting ("a promise chain failed to handle a rejection") is unreliable, does not report stack traces, and is not a substitute for proper error handling. When you see such an error, it indicates that something is wrong in the code, there is a "failure" in the rejection handling.

In fact, when error handling is missing at the end of a Promise chain, it is unclear which case it is: did you forget to handle the error case, or did you forget to return the Promise to the caller?

https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Promise.jsm/Promise#Handling_errors_and_common_pitfalls

> let unexpected exceptions propagate, just like we do for most synchronous code.

This can be a way to see it: the "init" function, which currently returns void, could change its signature and return a Promise. In this case, all callers would need to do something with the returned promise, and if they don't need to do anything special, they must call ".catch(Cu.reportError);" to report errors and indicate that the Promise chain has intentionally ended.

I filed bug 1030714 to tweak our error messages and make the situation clearer.
(In reply to Myk Melez [:myk] [@mykmelez] from comment #31)
> So if DownloadView feels
> insufficiently descriptive, then we could call it something like
> DownloadWindowOpenerView.
> But we shouldn't call it DownloadFrontendView, because "frontend" provides
> no additional information in this context; it just makes the name unwieldy.

Ah yeah, I totally agree with this in this light.

> Also, nit: here and elsewhere, functions like this one should return a
> promise, so their callers can observe the operation and its outcome.

As noted, in this case the caller (doCommand) not only can, but must observe the returned promise.

> @@ +342,5 @@
> > +   * Cancels the download.
> > +   */
> > +  cancel: function() {
> > +    this._download.cancel();
> > +    this._download.removePartialData().catch(Cu.reportError);
> 
> Nit: this synchronous sequence of asynchronous operations makes it look like
> removePartialData will race cancellation, although it turns out that
> removePartialData is smart enough to await cancellation, so this happens to
> work.
> 
> Nevertheless, to avoid misleading future hackers, I would explicitly wait
> for cancellation before removing partial data, either:
> 
>   cancel: Task.async(function*() {
>     yield this._download.cancel();
>     yield this._download.removePartialData();
>   }),

I know it is counter-intuitive, and that's an issue of the API design, but waiting for "cancel" is more easily subject to race conditions, because in theory anything can happen between "cancel" and "removePartailData", for example the user could click a button (unlikely in practice) or an add-on can do something with the download, so for example we may be calling removePartialData on a running download.

>   matchesSearch: function(aTerms, aAttributes) {
>     return aTerms.some(term => aAttributes.some(attr =>
> attr.contains(term)));
>   },

I always like to see the big leaps forward that JavaScript has made, so evident when refactoring old code :-)

> @@ +1070,5 @@
> > +  onDownloadChanged: function(aDownload) {
> > +    let downloadItem = this.downloadItemsMap.get(aDownload);
> > +    if (!downloadItem) {
> > +      Cu.reportError("Download doesn't exist.");
> > +      return;
> 
> Here and elsewhere, it seems like we should do something better when we
> observe an event about a download for which we don't have an item, like
> create an item for the download and add it to the list.  Do we know of
> reasons why this might happen, or is this simply out of an abundance of
> caution?

The second, this is like an "assertion".

> ::: webapprt/test/chrome/browser_download.js
> @@ +47,5 @@
> > +        ok(fileDownloaded, "File downloaded");
> > +
> > +        executeSoon(() => {
> > +          executeSoon(() => {
> > +            test_downloadList(win, [download]);
> 
> Why does this code (and some other test code) chain a pair of executeSoon
> calls?  A single call just guarantees that we spin the event loop, which is
> reasonable under certain circumstances.  But multiple calls suggests that
> we're trying to wait "long enough," which is likely to fail on machines that
> are slower than our own.

Without looking at the code, I believe this _might_ be to wait for a DOM event or observer, and for the subsequent promise chain processing (promise callbacks are always delayed by one tick when triggered from inside an event handler, and in general in any case where we're not already processing a Promise callback). This is deterministic.

A code comment explaining this would suffice. I think fixing the tests to be truly asynchronous would be a lot of work, with unbounded ETA, and thus out of scope for this bug, whose immediate purpose is to unblock the removal of the old API from the tree.
(In reply to :Paolo Amadini from comment #32)

> That's a misconception: asynchronous error reporting ("a promise chain
> failed to handle a rejection") is unreliable, does not report stack traces,
> and is not a substitute for proper error handling.  When you see such an
> error, it indicates that something is wrong in the code, there is a
> "failure" in the rejection handling.

Hrm, if the reporting is unreliable, then the solution for that should be to fix its bugs.  As for such errors not reporting stack traces, neither does Cu.reportError.  But isn't clear that we should be doing so in any case.  Most error reports don't include stacks.

And as for "proper error handling," I'm not sure exactly what you mean by that, but I don't generally think of mere reporting as the handling of an error.

I suppose it's technically the least one can do, and perhaps I'm biased by the exception handling built into JavaScript, but I expect basic reporting to be the responsibility of the runtime, such that errors don't need to be handled unless something else needs to be done when they occur, like destroying objects, cleaning up files, notifying the user, retrying a network request, or extending the error message.


> In fact, when error handling is missing at the end of a Promise chain, it is
> unclear which case it is: did you forget to handle the error case, or did
> you forget to return the Promise to the caller?

This is a good point, as I imagine some number of these dangling chains should really be returned…


> This can be a way to see it: the "init" function, which currently returns
> void, could change its signature and return a Promise. In this case, all
> callers would need to do something with the returned promise, and if they
> don't need to do anything special, they must call ".catch(Cu.reportError);"
> to report errors and indicate that the Promise chain has intentionally ended.

…but then the number of call sites that must terminate the chain may increase quite considerably.


> I filed bug 1030714 to tweak our error messages and make the situation
> clearer.

That seems reasonable, given the current recommendation.  I'm still not convinced that it's better than auto-terminating such chains with default error reporting.  But I'll accommodate myself to it in code and reviews.


(In reply to :Paolo Amadini from comment #33)

> I know it is counter-intuitive, and that's an issue of the API design, but
> waiting for "cancel" is more easily subject to race conditions, because in
> theory anything can happen between "cancel" and "removePartailData", for
> example the user could click a button (unlikely in practice) or an add-on
> can do something with the download, so for example we may be calling
> removePartialData on a running download.

Fair enough, let's just note this in a comment so future hackers know why we do it this way!


> I always like to see the big leaps forward that JavaScript has made, so
> evident when refactoring old code :-)

Indeed! :-)


> Without looking at the code, I believe this _might_ be to wait for a DOM
> event or observer, and for the subsequent promise chain processing (promise
> callbacks are always delayed by one tick when triggered from inside an event
> handler, and in general in any case where we're not already processing a
> Promise callback). This is deterministic.
> 
> A code comment explaining this would suffice. I think fixing the tests to be
> truly asynchronous would be a lot of work, with unbounded ETA, and thus out
> of scope for this bug, whose immediate purpose is to unblock the removal of
> the old API from the tree.

As long as it's deterministic, and two ticks always succeeds, then I'm fine with a simple comment explaining this.
Attached patch Patch (obsolete) — Splinter Review
(In reply to Myk Melez [:myk] [@mykmelez] from comment #31)
> Building hits a preprocessor error:
> 
> 10:19.29 mozbuild.preprocessor.Error:
> ('/Users/myk/Mozilla/gecko-dev/webapprt/themes/osx/downloads/downloads.css',
> 5, 'FILE_NOT_FOUND',
> '/Users/myk/Mozilla/gecko-dev/webapprt/themes/osx/downloads/../../global/
> shared.inc')

Fixed, we don't really need to include shared.inc.

> As for gDownloadUI, it's more difficult to name, since it's a "view" in the
> eyes of the Downloads object, but it lives in a document that has a
> downloadView element, which it references and controls.  And it does other
> work besides simply relaying changes in the model to the downloadView
> element.
> 
> In fact it's the primary singleton global workhorse, which we reference
> often.  So we should pick a simple name for it, like DownloadView (despite
> downloadView) or DownloadList (with or without the "g" prefix).

Given that it is a collection of |DownloadUIItem|s, I think we could call
it gDownloadUIView or gDownloadUIList, is that ok with you? This name makes
it easy to understand what the object does.

> @@ +777,5 @@
> > +    this.searchTerms = this.searchBox.value.replace(/^\s+|\s+$/g, "")
> > +                                           .toLowerCase().split(/\s+/);
> > +
> > +    // Unless forced, don't rebuild the download list if the search didn't change
> > +    if (!aForceBuild && this.searchTerms.join(" ") == prevSearch) {
> 
> Nit: to avoid rebuilding the download list when terms merely get reordered,
> also sort:
> 
>   this.searchTerms =
> this.searchBox.value.trim().toLowerCase().sort().split(/\s+/);

I think it's extremely uncommon for terms to get reordered.

> 
> @@ +803,5 @@
> > +    let searchTerms = this.searchTerms;
> > +
> > +    let list = yield Downloads.getList(Downloads.ALL);
> > +
> > +    yield list.removeFinished((aDownload) => {
> 
> Nit: I would avoid mixing promise chains with Tasks, as it's especially
> confusing to trace the execution flow through such mixtures.

The function passed to removeFinished is only a filter, it's not chained to
removeFinished.

> ::: webapprt/test/chrome/head.js
> @@ +108,5 @@
> > +  Downloads.getList = function(aKind) {
> > +    return new Promise(function(resolve, reject) {
> > +      resolve(list);
> > +    });
> > +  };
> 
> Nit: you should be able to use Promise.resolve here:
> 
>   Downloads.getList = () => Promise.resolve(list);

I'm using standard DOM promises in the file to avoid importing Promise.jsm.

(In reply to Myk Melez [:myk] [@mykmelez] from comment #34)
> As long as it's deterministic, and two ticks always succeeds, then I'm fine
> with a simple comment explaining this.

The first executeSoon was needed to wait for the downloadView node to be created,
the second executeSoon was needed to wait for the Downloads.getList promise to
be resolved.

I've modified a bit the code to use a mutation observer instead of the first
executeSoon, this should make the test more robust.
Attachment #8445178 - Attachment is obsolete: true
Attachment #8450698 - Flags: review?(myk)
Attached patch Patch (obsolete) — Splinter Review
Oops, I uploaded the patch with all the webapprt chrome tests but the download ones disabled (I blame bug 996259, that makes me disable tests when I want to run just a subset of the tests in a directory).
Attachment #8450701 - Flags: review?(myk)
Comment on attachment 8450698 [details] [diff] [review]
Patch

And I forgot to mark as obsolete the first patch. I haven't got anyone/anything to blame here :(
Attachment #8450698 - Attachment is obsolete: true
Attachment #8450698 - Flags: review?(myk)
Comment on attachment 8450701 [details] [diff] [review]
Patch

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

(In reply to Marco Castelluccio [:marco] from comment #35)
> Given that it is a collection of |DownloadUIItem|s, I think we could call
> it gDownloadUIView or gDownloadUIList, is that ok with you? This name makes
> it easy to understand what the object does.

It's ok, but I'd rather these be called something like DownloadItem and gDownloadList, as "UI" adds name complexity without improving comprehensibility, IMO.


Otherwise, only one minor issue in webapprt/themes/windows/jar.mn.  The rest of my comments are just nits.  So r=myk with that fixed on commission.  But does anything need re-review from Paolo?

::: webapprt/test/chrome/browser_download.js
@@ +11,5 @@
> +    // Assume the file URL we obtained from the downloads database or from the
> +    // "spec" property of the target has the UTF-8 charset.
> +    let fileUrl = NetUtil.newURI(aFilename).QueryInterface(Ci.nsIFileURL);
> +    return fileUrl.file.clone();
> +  } else {

Nit: don't use "else" after "return".

@@ +74,5 @@
> +      fileDownloaded = true;
> +      try {
> +        downloadedFile.remove(true);
> +      } catch (ex) {
> +      }

Nit: for no-op "catch" blocks like this one and the one below, it's helpful to explain in a comment why exceptions don't matter in this case.

@@ +110,5 @@
>  function test() {
>    waitForExplicitFinish();
>  
>    loadWebapp("download.webapp", undefined, function onLoad() {
> +    Task.spawn(function*() {

Nit: instead of spawning a task inside the onLoad function, define the onLoad function via Task.async.

::: webapprt/themes/windows/jar.mn
@@ +15,5 @@
> +webapprt.jar:
> +% skin webapprt classic/1.0 %skin/classic/aero/webapprt/ os=WINNT osversion>=6
> +        skin/classic/aero/webapprt/downloads/downloadButtons.png            (downloads/downloadButtons-aero.png)
> +        skin/classic/aero/webapprt/downloads/downloadIcon.png               (downloads/downloadIcon-aero.png)
> +*       skin/classic/aero/webapprt/downloads/downloads.css                  (downloads/downloads-aero.css)

Close the "#ifdef XP_WIN" conditional directive:

#endif
Attachment #8450701 - Flags: review?(myk) → review+
Erm, I'm seeing test failures on my Mac:

 1:10.64 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/browser_download.js | Download item unexpected
 1:10.64 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/browser_download.js | All the downloads expected to be in the list were in the list - Got 1, expected 0
(In reply to Myk Melez [:myk] [@mykmelez] from comment #38)
> But does anything need re-review from Paolo?

The latest patch looks good to me!

nit: You may want to add the comment explaining why "cancel" is followed immediately by "removePartialData" (feel free to add the same comment to existing code as well).
(In reply to Myk Melez [:myk] [@mykmelez] from comment #39)
> Erm, I'm seeing test failures on my Mac:
> 
>  1:10.64 TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/
> browser_download.js | Download item unexpected
>  1:10.64 TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/
> browser_download.js | All the downloads expected to be in the list were in
> the list - Got 1, expected 0

I can't reproduce this failure, but I think we need an executeSoon after |list.removeDownload| and before the second call to |test_downloadList|. I'll post an updated patch to see if it fixes the failure you're seeing.

There's just one problem left, browser_add_download.js and browser_remove_download.js are crashing on my Windows machine (but browser_download.js isn't!).
Attached file log.txt (obsolete) —
The crash is in mozilla::layers::BasicCompositor::BeginFrame.
Attachment #8450701 - Attachment is obsolete: true
Depends on: 1037226
(In reply to Myk Melez [:myk] [@mykmelez] from comment #31)
> > +      // Convert the nodelist into an array to keep a copy of the download items
> > +      let items = [];
> > +      for (let i = this.downloadsView.selectedItems.length; --i >= 0; ) {
> > +        items.unshift(this.downloadsView.selectedItems[i]);
> > +      }
> 
> Nit: this'd be simpler as:
> 
>   let items = Array.prototype.slice.call(this.downloadsView.selectedItems);

Even simpler:

let items = Array.from(this.downloadsView.selectedItems);
Looks like the crash I'm experiencing is specific to my machine and is probably a test only crash (the test that doesn't mock Downloads.jsm works correctly). Should we land this anyway?
Flags: needinfo?(myk)
(In reply to Marco Castelluccio [:marco] from comment #44)
> Looks like the crash I'm experiencing is specific to my machine and is
> probably a test only crash (the test that doesn't mock Downloads.jsm works
> correctly). Should we land this anyway?

Hmm, if it's specific to your machine, then yes, let's land it.  But can you post an updated patch?  I'd like to test it on my own Windows machine.
Flags: needinfo?(myk)
Attached patch Patch (obsolete) — Splinter Review
Here it is.
Flags: needinfo?(myk)
I get the patch is ready to land now?
Flags: needinfo?(mar.castelluccio)
Yes, I'll land it today. Sorry for the delay, I had to re-prepare my building environment.
Flags: needinfo?(mar.castelluccio)
Attached patch PatchSplinter Review
Attachment #8454106 - Attachment is obsolete: true
Attachment #8460691 - Attachment is obsolete: true
Attachment #8475238 - Flags: review+
Keywords: checkin-needed
https://tbpl.mozilla.org/?tree=Try&rev=f07d1c3077cb for other platforms (there are desktop failures because I forgot to add a file to the installer manifest)
https://tbpl.mozilla.org/?tree=Try&rev=f4252864e741 for desktop platforms
(In reply to Marco Castelluccio [:marco] from comment #49)
> Yes, I'll land it today. Sorry for the delay, I had to re-prepare my
> building environment.

Thank you! I know how setting up environments may require a lot of time...
https://hg.mozilla.org/mozilla-central/rev/9c96b4e6c9ee
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
\o/
Blocks: 1037228
Depends on: 1057832
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.