Closed Bug 1129896 Opened 5 years ago Closed 5 years ago

Address review comments for the Downloads front-end refactoring

Categories

(Firefox :: Downloads Panel, defect)

defect
Not set
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 38
Iteration:
38.3 - 23 Feb
Tracking Status
firefox38 --- verified

People

(Reporter: Paolo, Assigned: Paolo)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

This bug is about completing the Downloads front-end refactoring by addressing the review comments on the latest version of the existing patches.
This addresses the changes to the cache we discussed.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #8559759 - Flags: feedback?(mak77)
Marco, what do you think about bug 1117139 comment 4?
Flags: needinfo?(mak77)
Attached patch Part 2 - Miscellaneous changes (obsolete) — Splinter Review
Attachment #8559759 - Attachment is obsolete: true
Attachment #8559759 - Flags: feedback?(mak77)
Attachment #8559779 - Flags: feedback?(mak77)
Attachment #8559759 - Attachment is obsolete: false
Attachment #8559759 - Flags: feedback?(mak77)
> > +    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".
Attached patch All-in-one patch (obsolete) — Splinter Review
(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.
Flags: needinfo?(mak77)
(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).
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.
(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 :)
(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 :)
Also, I suspect we can use this.element.ownerDocument and thus we don't even need document...
(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)?
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.
Attachment #8559759 - Attachment description: Part 1 - Keep cached metadata for history downloads indefinitely → Part 1 of 2 - Keep cached metadata for history downloads indefinitely
This patch contains the miscellaneous changes as well.
Attachment #8559779 - Attachment is obsolete: true
Attachment #8559779 - Flags: feedback?(mak77)
Attachment #8559876 - Flags: feedback?(mak77)
Attachment #8559782 - Attachment is obsolete: true
Flags: firefox-backlog+
Hi Paolo, can you assign a point value.
Iteration: --- → 38.2 - 9 Feb
Flags: qe-verify?
Flags: needinfo?(paolo.mozmail)
Points: --- → 3
Flags: needinfo?(paolo.mozmail)
Flags: qe-verify? → qe-verify+
Iteration: 38.2 - 9 Feb → 38.3 - 23 Feb
Attachment #8559759 - Flags: feedback?(smacleod)
Attachment #8559759 - Flags: feedback?(jaws)
Attachment #8559876 - Flags: feedback?(smacleod)
Attachment #8559876 - Flags: feedback?(jaws)
Attachment #8559759 - Flags: review?(mak77)
Attachment #8559759 - Flags: feedback?(mak77)
Attachment #8559759 - Flags: feedback?
Attachment #8559876 - Flags: feedback?(mak77) → review?(mak77)
Attachment #8559759 - Flags: feedback?
Attachment #8559759 - Flags: review?(mak77) → review+
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.
Attachment #8559876 - Flags: review?(mak77) → review+
(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.
Tryserver build of the patch queue that will land soon:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=098c1c9a8987
https://hg.mozilla.org/mozilla-central/rev/c41b520cb4a8
https://hg.mozilla.org/mozilla-central/rev/79fcb3f8f12e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
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.
QA Contact: catalin.varga
Depends on: 1135348
Attachment #8559759 - Flags: feedback?(jaws) → feedback+
Attachment #8559876 - Flags: feedback?(jaws) → feedback+
Attachment #8559759 - Flags: feedback?(smacleod) → feedback+
Attachment #8559876 - Flags: feedback?(smacleod) → feedback+
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.