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

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect)

Description

[:Paolo Amadini]

Attachment: 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.

Comment 1

[:Paolo Amadini]

Attachment: On latest trunk

Comment 2

[Marco Bonardo [:mak]]

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...

Comment 3

[Marco Mucci [:MarcoM]]

Hi Paolo, can you provide a point value.

Comment 4

[:Paolo Amadini]

(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.

Comment 5

[:Paolo Amadini]

(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.

Comment 6

[:Paolo Amadini]

Comment on attachment 8543304 [details] [diff] [review]
Initial patch

Review comments addressed in bug 1129896.

Comment 7

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/37616c2fcc8b

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/37616c2fcc8b