1129896 - Address review comments for the Downloads front-end refactoring

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect)

Description

[:Paolo Amadini]

This bug is about completing the Downloads front-end refactoring by addressing the review comments on the latest version of the existing patches.

Comment 1

[:Paolo Amadini]

Attachment: Part 1 of 2 - Keep cached metadata for history downloads indefinitely

This addresses the changes to the cache we discussed.

Comment 2

[:Paolo Amadini]

Marco, what do you think about bug 1117139 comment 4?

Comment 3

[:Paolo Amadini]

Attachment: Part 2 - Miscellaneous changes

Comment 4

[:Paolo Amadini]

> > +    if (!aDownload.newDownloadNotified) {
> > +      aDownload.newDownloadNotified = true;
> 
> Wouldn't be better to use a Set rather than adding stuff to aDownload?

Pre-existing nit, maybe we can file a good first bug?

> > +    let command = getDefaultCommandForState(
> > +                            DownloadsCommon.stateOfDownload(this.download));
> 
> temp var :)

This didn't make it to the patch, fixed locally now.

> > +      // Stopped, paused, and failed downloads with partial data are removed.
> > +      let download = elt._shell.download;
> > +      if (download.stopped && !(download.canceled && download.hasPartialData)) {
> 
> ditto (! || !)

Hm, we use the positive version "canceled && hasPartialData" all around the place, this is why I've written this like so.

> > +  onDownloadAdded(download, newest) {
> > +    throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
> 
> nit: throwing Components.Exception should give better stacks than plain Cr, iirc

Since these are not supposed to be called at all, probably the longer version is overkill:

    throw new Components.Exception("Not implemented",
                                   Components.results.NS_ERROR_NOT_IMPLEMENTED);

On the other hand the longer version may be better to avoid inadvertently copy-and-pasting the inferior implementation in actual production code. Or we could just use a comment instead of "throw". What do you think?

> Marco, what do you think about bug 1117139 comment 4?

Repeating, since sometimes older comments get overlooked when checking "needinfo".

Comment 5

[:Paolo Amadini]

Attachment: All-in-one patch

Comment 6

[Marco Bonardo [:mak]]

(In reply to :Paolo Amadini from comment #2)
> Marco, what do you think about bug 1117139 comment 4?

Is there a way we can pass window and document as arguments?
So the real question is how much of the code relies on those, for example PlacesUIUtils was a js cause some of its methods needed window and document. But with small refactoring we were able to pass those to the few methods needing them and convert it to a module, that is thus shared by all the windows.

Comment 7

[:Paolo Amadini]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #6)
> Is there a way we can pass window and document as arguments?
> So the real question is how much of the code relies on those, for example
> PlacesUIUtils was a js cause some of its methods needed window and document.
> But with small refactoring we were able to pass those to the few methods
> needing them and convert it to a module, that is thus shared by all the
> windows.

The use case is probably a bit different, but what we can do could be to place "document" and "window" as properties of the DownloadElementShell itself when it is constructed, treating it as a sort of "component", and use "this.document" and "this.window" where required. I also believe "window" is not referenced at present, so it can be omitted for now until it's needed.

It's still odd to have front-end components in a JSM rather than JS, but possibly this is a better way to go anyways, reusing more code in memory (and crossing compartments shouldn't be a big deal).

Comment 8

[:Paolo Amadini]

One thing that will triplicate is the boilerplate JSM loading itself. We'll have to do it in the JSM as well as in each of the two windows separately, while currently it's done in one place by the shared JS file.

Comment 9

[Marco Bonardo [:mak]]

(In reply to :Paolo Amadini from comment #4)
> > > +    if (!aDownload.newDownloadNotified) {
> > > +      aDownload.newDownloadNotified = true;
> > 
> > Wouldn't be better to use a Set rather than adding stuff to aDownload?
> 
> Pre-existing nit, maybe we can file a good first bug?

Sure, I don't like much that property addition honestly, but a mentored bug sounds good if you're willing to mentor it.

> > 
> > ditto (! || !)
> 
> Hm, we use the positive version "canceled && hasPartialData" all around the
> place, this is why I've written this like so.

Yeah that was my suspect, though negating that generates a quite unreadable set...

> On the other hand the longer version may be better to avoid inadvertently
> copy-and-pasting the inferior implementation in actual production code. Or
> we could just use a comment instead of "throw". What do you think?

I think we should just use the proper version, even if long and boring, it's what we want others to do, we must provide our best examples :)

Comment 10

[Marco Bonardo [:mak]]

(In reply to :Paolo Amadini from comment #7)
> The use case is probably a bit different, but what we can do could be to
> place "document" and "window" as properties of the DownloadElementShell
> itself when it is constructed, treating it as a sort of "component", and use
> "this.document" and "this.window" where required. I also believe "window" is
> not referenced at present, so it can be omitted for now until it's needed.

Indeed I couldn't find it and that was why I ended up thinking of a jsm.

> It's still odd to have front-end components in a JSM rather than JS, but
> possibly this is a better way to go anyways, reusing more code in memory
> (and crossing compartments shouldn't be a big deal).

I think sometimes we need to put memory and perf in front of code purity.
In the worst case we could do something like http://mxr.mozilla.org/mozilla-central/source/toolkit/obsolete/content/inlineSpellCheckUI.js
but doesn't look like any window needs downloads.

To be clear, browserPlacesViews.js is the best example of what we should try to avoid :)

Comment 11

[Marco Bonardo [:mak]]

Also, I suspect we can use this.element.ownerDocument and thus we don't even need document...

Comment 12

[:Paolo Amadini]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #11)
> Also, I suspect we can use this.element.ownerDocument and thus we don't even
> need document...

That's actually a cool suggestion for this particular object!

With regard to module structure, I wonder whether it makes sense to have a single DownloadsFrontend.jsm module, or maybe one module per functional group of objects, such as for example a DownloadsShell and the DownloadsListbox in the same module, rather than one module per object.

You previously mentioned exporting one symbol per module. So, should we stick the constructors to an exported object (resulting in __proto__: DownloadsFrontend.DownloadElementShell) rather than exporting them directly (resulting in __proto__: DownloadsElementShell)?

Comment 13

[Marco Bonardo [:mak]]

both having many modules (code fragmentation makes harder to use your API) and having many objects in a module (confusing imports) have their downside.

you could even have something like mymodule.generateShellFor() that builds the object and returns it. I don't know. I don't think having a fancy __proto__ has downsides.

Comment 14

[:Paolo Amadini]

Attachment: Part 2 of 2 - Convert the shared front-end code to a JavaScript code module

This patch contains the miscellaneous changes as well.

Comment 15

[Marco Mucci [:MarcoM]]

Hi Paolo, can you assign a point value.

Comment 16

[Marco Bonardo [:mak]]

Comment on attachment 8559876 [details] [diff] [review]
Part 2 of 2 - Convert the shared front-end code to a JavaScript code module

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

it looks nicer!

::: browser/components/downloads/DownloadsViewUI.jsm
@@ +86,5 @@
>      if (!this.__progressElement) {
>        // If the element is not available now, we will try again the next time.
> +      this.__progressElement =
> +           this.element.ownerDocument.getAnonymousElementByAttribute(
> +                                      this.element, "anonid", "progressmeter");

nit: indent once more the function arguments

@@ +198,5 @@
>        if (this.download.succeeded) {
>          // For completed downloads, show the file size (e.g. "1.5 MB").
>          if (this.download.target.size !== undefined) {
> +          let [size, unit] =
> +              DownloadUtils.convertByteUnits(this.download.target.size);

nit: indent once less :)

::: browser/components/downloads/content/allDownloadsViewOverlay.js
@@ +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/. */
>  
> +let { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;

Is this really needed? doesn't looks like you added new code using these.

@@ +18,5 @@
> +                                  "resource://gre/modules/osfile.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
> +                                  "resource://gre/modules/PlacesUtils.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Promise",
> +                                  "resource://gre/modules/Promise.jsm");

Can we avoid the deprecated Promise, or are we not ready yet?

::: browser/components/downloads/content/downloads.js
@@ +63,5 @@
>   */
>  
>  "use strict";
>  
> +let { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;

Is this really needed? doesn't looks like you added new code using these.

Comment 17

[:Paolo Amadini]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #16)
> > +let { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
> 
> Is this really needed? doesn't looks like you added new code using these.

I see no obvious places where these may be defined... I think it's better to have them defined consistently rather than from some unknown place (maybe platform-specific).

> > +XPCOMUtils.defineLazyModuleGetter(this, "Promise",
> > +                                  "resource://gre/modules/Promise.jsm");
> 
> Can we avoid the deprecated Promise, or are we not ready yet?

This is not deprecated, on the converse is still recommended until we fix the dependencies of bug 939636.

Comment 18

[:Paolo Amadini]

Tryserver build of the patch queue that will land soon:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=098c1c9a8987

Comment 19

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/c41b520cb4a8
https://hg.mozilla.org/integration/fx-team/rev/79fcb3f8f12e

Comment 20

[:Paolo Amadini]

Full patch queue on fx-team:

https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=79fcb3f8f12e

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c41b520cb4a8
https://hg.mozilla.org/mozilla-central/rev/79fcb3f8f12e

Comment 22

[:Paolo Amadini]

The QA for this bug consist in using the Downloads Panel and the Download View in the Library window and see that everything generally still works correctly. There is no particular feature to be tested in detail. I think there are a few bugs on file for some edge cases that may be resolved by this refactoring.

Comment 23

[Catalin Varga [QA][:VarCat]]

Verified as fixed using the following environment:

FF 38.0b8
Build Id: 20150427090451
OS: Win 7 x64, Ubuntu 14.04 x64, Mac Os X 10.9.5