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

RESOLVED FIXED in Firefox 38

Status

()

Firefox
Downloads Panel
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 38
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

4 years ago
Created attachment 8543304 [details] [diff] [review]
Initial patch

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+
(Assignee)

Comment 1

4 years ago
Created attachment 8550363 [details] [diff] [review]
On latest trunk
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)
(Assignee)

Comment 4

4 years ago
(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.
(Assignee)

Comment 5

4 years ago
(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)
(Assignee)

Updated

4 years ago
Blocks: 1129896

Updated

4 years ago
Iteration: 38.2 - 9 Feb → 38.3 - 23 Feb
(Assignee)

Updated

4 years ago
Attachment #8543304 - Flags: feedback?(smacleod)
Attachment #8543304 - Flags: feedback?(jaws)
(Assignee)

Comment 6

4 years ago
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
Last Resolved: 4 years ago
status-firefox38: --- → fixed
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.