Closed
Bug 1443901
Opened 7 years ago
Closed 7 years ago
Remove downloadsViewOverlay.xul
Categories
(Firefox :: Downloads Panel, enhancement)
Firefox
Downloads Panel
Tracking
()
RESOLVED
FIXED
Firefox 61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: bdahl, Assigned: bdahl)
References
Details
Attachments
(1 file)
This overlay appears to only be used once by places.xul and can be inlined.
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
Comment on attachment 8956942 [details]
Bug 1443901 - Remove downloadsViewOverlay.xul.
https://reviewboard.mozilla.org/r/225882/#review231790
::: browser/components/places/content/places.xul:58
(Diff revision 1)
> + <script type="application/javascript"><![CDATA[
> + const DOWNLOADS_QUERY = "place:transition=" +
> + Ci.nsINavHistoryService.TRANSITION_DOWNLOAD +
> + "&sort=" +
> + Ci.nsINavHistoryQueryOptions.SORT_BY_DATE_DESCENDING;
> +
> + ContentArea.setContentViewForQueryString(DOWNLOADS_QUERY,
> + () => new DownloadsPlacesView(document.getElementById("downloadsRichListBox"), false),
> + { showDetailsPane: false,
> + toolbarSet: "back-button, forward-button, organizeButton, clearDownloadsButton, libraryToolbarSpacer, searchFilter" });
> + ]]></script>
Looks good to me, just this initialization might go in one of the existing JavaScript files.
Redirecting the review to Marco who may recommend the best place.
Attachment #8956942 -
Flags: review?(paolo.mozmail)
Updated•7 years ago
|
Attachment #8956942 -
Flags: review?(mak77)
Assignee | ||
Comment 3•7 years ago
|
||
Tests seem to be passing fine, but I just noticed I'm getting "JavaScript error: chrome://browser/content/downloads/downloads.js, line 1: SyntaxError: redeclaration of const DownloadsView"
It looks like DownloadsView in downloads.js and allDownloadsViewOverlay.js now collide. I'm not sure why this wasn't a problem previously. Maybe something to do with how dynamic overlays are loaded?
Assignee | ||
Comment 4•7 years ago
|
||
The above error comes from loading allDownloadsViewOverlay.xul before macBrowserOverlay.xul, which is the opposite of how it use to be. allDownloadViewOverlay.xul loads the "const DownloadsView" and macBrowserOverlay.xul loads the "var DownloadsView".
Is it actually expected behavior that DownloadsView from allDownloadsViewOverlay clobbers the macBrowserOverlay definition?
Flags: needinfo?(paolo.mozmail)
Assignee | ||
Comment 5•7 years ago
|
||
Also, is there a different underlying bug here... I'm confused why one way is an error and the other isn't. I'd expect both of the following to fail (like regular JS):
const someVar = ''; var someVar = '';
var someVar = ''; const someVar = '';
Assignee | ||
Comment 6•7 years ago
|
||
I see what's happening now, 'DownloadsView' is defined as lazy script getter in browser.js and lazy script getter sets up a configurable/writable property. My original questions still remains if this was the intended behavior of DownloadsView in allDownloadsView?
Comment 7•7 years ago
|
||
(In reply to Brendan Dahl [:bdahl] from comment #6)
> I see what's happening now, 'DownloadsView' is defined as lazy script getter
> in browser.js and lazy script getter sets up a configurable/writable
> property. My original questions still remains if this was the intended
> behavior of DownloadsView in allDownloadsView?
I don't think it's intended, but it's probably harmless.
allDownloadsViewOverlay is used in the Library window, where this overriding is harmless because there's no Library button nor bookmarks button. And it's used in about:addons, that is the content window.
So in both cases the override is harmless.
In any case, the one in AllDownloadsViewOverlay could probably be renamed to AllDownloadsView
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8956942 [details]
Bug 1443901 - Remove downloadsViewOverlay.xul.
https://reviewboard.mozilla.org/r/225882/#review231994
::: browser/components/places/content/places.xul:58
(Diff revision 1)
> + <script type="application/javascript"><![CDATA[
> + const DOWNLOADS_QUERY = "place:transition=" +
> + Ci.nsINavHistoryService.TRANSITION_DOWNLOAD +
> + "&sort=" +
> + Ci.nsINavHistoryQueryOptions.SORT_BY_DATE_DESCENDING;
> +
> + ContentArea.setContentViewForQueryString(DOWNLOADS_QUERY,
> + () => new DownloadsPlacesView(document.getElementById("downloadsRichListBox"), false),
> + { showDetailsPane: false,
> + toolbarSet: "back-button, forward-button, organizeButton, clearDownloadsButton, libraryToolbarSpacer, searchFilter" });
> + ]]></script>
I think you can move this init code into PO_init(), BEFORE ContentArea.init(); so just before anything else, with a comment like "// Register the downloads view.".
The best thing would probably be to pass an array of views to ContentArea.init() like
[{ url, contructorFn, options }, ...]
and register them in ContentArea.init itself
But it's also ok to leave that for the future.
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8956942 [details]
Bug 1443901 - Remove downloadsViewOverlay.xul.
https://reviewboard.mozilla.org/r/225882/#review232012
The previous bug comments pretty much cover the review
Attachment #8956942 -
Flags: review?(mak77)
Comment 10•7 years ago
|
||
(In reply to Brendan Dahl [:bdahl] from comment #4)
> Is it actually expected behavior that DownloadsView from
> allDownloadsViewOverlay clobbers the macBrowserOverlay definition?
It's not intended as an override if we could avoid defining the other objects in the first place, but the implementations of DownloadsView.onDownloadCommand and DownloadsView.onDownloadClick should still be different here compared to the default implementation used for the Downloads Panel.
You should test the Downloads Panel, the Library window, and the Downloads View in private tabs separately.
Flags: needinfo?(paolo.mozmail)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8956942 [details]
Bug 1443901 - Remove downloadsViewOverlay.xul.
https://reviewboard.mozilla.org/r/225882/#review233858
Attachment #8956942 -
Flags: review?(paolo.mozmail) → review+
Comment hidden (mozreview-request) |
Comment 15•7 years ago
|
||
Pushed by bdahl@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9d7410af234c
Remove downloadsViewOverlay.xul. r=Paolo
Comment 16•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in
before you can comment on or make changes to this bug.
Description
•