Closed
Bug 1117139
Opened 10 years ago
Closed 10 years ago
Move code controlling the "download.xml" binding to a common place
Categories
(Firefox :: Downloads Panel, defect)
Firefox
Downloads Panel
Tracking
()
Tracking | Status | |
---|---|---|
firefox38 | --- | fixed |
People
(Reporter: Paolo, Assigned: Paolo)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
42.17 KB,
patch
|
Paolo
:
review+
jaws
:
feedback+
smacleod
:
feedback+
|
Details | Diff | Splinter Review |
42.26 KB,
patch
|
Details | Diff | Splinter 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)
Updated•10 years ago
|
Assignee: nobody → paolo.mozmail
Flags: qe-verify-
Flags: firefox-backlog+
Assignee | ||
Comment 1•10 years ago
|
||
Comment 2•10 years ago
|
||
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)
Comment 3•10 years ago
|
||
Hi Paolo, can you provide a point value.
Status: NEW → ASSIGNED
Iteration: --- → 38.2 - 9 Feb
Flags: needinfo?(paolo.mozmail)
Assignee | ||
Comment 4•10 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•10 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)
Updated•10 years ago
|
Iteration: 38.2 - 9 Feb → 38.3 - 23 Feb
Assignee | ||
Updated•10 years ago
|
Attachment #8543304 -
Flags: feedback?(smacleod)
Attachment #8543304 -
Flags: feedback?(jaws)
Assignee | ||
Comment 6•10 years ago
|
||
Comment on attachment 8543304 [details] [diff] [review] Initial patch Review comments addressed in bug 1129896.
Attachment #8543304 -
Flags: review+
Assignee | ||
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/37616c2fcc8b
Comment 8•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/37616c2fcc8b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Updated•10 years ago
|
Attachment #8543304 -
Flags: feedback?(jaws) → feedback+
Updated•9 years ago
|
Attachment #8543304 -
Flags: feedback?(smacleod) → feedback+
You need to log in
before you can comment on or make changes to this bug.
Description
•