Closed Bug 1117139 Opened 5 years ago Closed 5 years ago

Move code controlling the "download.xml" binding to a common place

Categories

(Firefox :: Downloads Panel, defect)

defect
Not set
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 38
Iteration:
38.3 - 23 Feb
Tracking Status
firefox38 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Attached patch Initial patchSplinter Review
This patch creates a "downloadsViewCommon.js" file to host a base object controlling the "download.xml" binding, shared by the Downloads Panel the Downloads View.
Attachment #8543304 - Flags: review?(mak77)
Assignee: nobody → paolo.mozmail
Flags: qe-verify-
Flags: firefox-backlog+
Attached patch On latest trunkSplinter Review
Comment on attachment 8543304 [details] [diff] [review]
Initial patch

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

the approach is reasonable, I don't like some implementation choice.

::: browser/base/content/global-scripts.inc
@@ +6,5 @@
>  <script type="application/javascript" src="chrome://global/content/printUtils.js"/>
>  <script type="application/javascript" src="chrome://global/content/viewZoomOverlay.js"/>
>  <script type="application/javascript" src="chrome://browser/content/places/browserPlacesViews.js"/>
>  <script type="application/javascript" src="chrome://browser/content/browser.js"/>
> +<script type="application/javascript" src="chrome://browser/content/downloads/downloadsViewCommon.js"/>

I think adding here is a very bad idea, see below.

::: browser/components/downloads/content/allDownloadsViewOverlay.js
@@ +2,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/. */
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "DownloadsDataItem",
> +                                  "resource:///modules/DownloadsCommon.jsm");

As I said in the other bug, the names clash is confusing

::: browser/components/downloads/content/downloadsViewCommon.js
@@ +34,5 @@
> +                                  "resource://gre/modules/Promise.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Services",
> +                                  "resource://gre/modules/Services.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Task",
> +                                  "resource://gre/modules/Task.jsm");

This file is included by global-scripts.inc, that is included by browser.xul, the mac menu overlay (and the chat window for some reason?). Anything we add here is part of every window and it's a cost that should be payed only if alternatives suck.

Honestly looks like this DownloadElementShell could go either into its own module or be expose as new DownloadsCommon.ElementShell()

@@ +102,5 @@
> +      this.__progressElement = document.getAnonymousElementByAttribute(
> +                               this.element, "anonid", "progressmeter");
> +    }
> +    return this.__progressElement;
> +  },

defineLazyGetter?

@@ +181,5 @@
> +      text = s.statusSeparatorBeforeNumber(s.statePaused, transfer);
> +    } else if (this.dataItem.state == nsIDM.DOWNLOAD_DOWNLOADING) {
> +      // By default, extended status information including the individual
> +      // download rate is displayed in the tooltip. The history view overrides
> +      // the getter and displays the detials in the main area instead.

typo: detials

Soon or later someone should update latest mockup from shorlander and put back the speed in the panel...
Attachment #8543304 - Flags: review?(mak77)
Hi Paolo, can you provide a point value.
Status: NEW → ASSIGNED
Iteration: --- → 38.2 - 9 Feb
Flags: needinfo?(paolo.mozmail)
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #2)
> ::: browser/components/downloads/content/downloadsViewCommon.js
> This file is included by global-scripts.inc, that is included by
> browser.xul, the mac menu overlay (and the chat window for some reason?).
> Anything we add here is part of every window and it's a cost that should be
> payed only if alternatives suck.
> 
> Honestly looks like this DownloadElementShell could go either into its own
> module or be expose as new DownloadsCommon.ElementShell()

The reason it's a JS file rather than a JSM is that it is a UI module, in other words it works on "window" and "document", and secondarily it is loaded in the same JavaScript global as the window. Not sure if the latter is important though, probably the operations we need would go through cross-compartment wrappers, that in the end I expect would work even if we use XBL bindings.

Basically this is the same reason why our front-end files are JS and not JSM.

Do you see an alternative?

> @@ +102,5 @@
> > +      this.__progressElement = document.getAnonymousElementByAttribute(
> > +                               this.element, "anonid", "progressmeter");
> > +    }
> > +    return this.__progressElement;
> > +  },
> 
> defineLazyGetter?

The difference is that we want to check again if we returned null, until we return non-null.
(In reply to Marco Mucci [:MarcoM] from comment #3)
> Hi Paolo, can you provide a point value.

Not sure there's a sensible value here. I'll go for a 3 for everything for the sake of having a number, taken individually might be more, but as part of the refactoring may be less.
Points: --- → 3
Flags: needinfo?(paolo.mozmail)
Blocks: 1129896
Iteration: 38.2 - 9 Feb → 38.3 - 23 Feb
Attachment #8543304 - Flags: feedback?(smacleod)
Attachment #8543304 - Flags: feedback?(jaws)
Comment on attachment 8543304 [details] [diff] [review]
Initial patch

Review comments addressed in bug 1129896.
Attachment #8543304 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/37616c2fcc8b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Attachment #8543304 - Flags: feedback?(jaws) → feedback+
Attachment #8543304 - Flags: feedback?(smacleod) → feedback+
You need to log in before you can comment on or make changes to this bug.