Closed Bug 1443901 Opened 6 years ago Closed 6 years ago

Remove downloadsViewOverlay.xul

Categories

(Firefox :: Downloads Panel, enhancement)

Product:
Firefox
Component:
Downloads Panel
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: bdahl, Assigned: bdahl)

References

Details

Attachments

(1 file)

Bug 1443901 - Remove downloadsViewOverlay.xul.
59 bytes, text/x-review-board-request
Paolo
: review+
Details 
This overlay appears to only be used once by places.xul and can be inlined.
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)
Attachment #8956942 - Flags: review?(mak77)
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?
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)
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 = '';
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?
Blocks: 1443971
(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 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 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)
(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 on attachment 8956942 [details]
Bug 1443901 - Remove downloadsViewOverlay.xul.

https://reviewboard.mozilla.org/r/225882/#review233858
Attachment #8956942 - Flags: review?(paolo.mozmail) → review+
Pushed by bdahl@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9d7410af234c
Remove downloadsViewOverlay.xul. r=Paolo
https://hg.mozilla.org/mozilla-central/rev/9d7410af234c
Status: NEW → RESOLVED
Closed: 6 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.

Attachment

General

Creator:
Created:
Updated:
Size: