Brendan Dahl [:bdahl] Assignee
	Description • 7 years ago

It appears downloadsOverlay.xul is only used in one place and could be inlined. However, going back to bug 726444 where it was implemented shows it was moved to an overlay for performance reasons. As we want to remove XUL overlays, we should re-visit this decision.

	Marco Bonardo [:mak]
	Comment 1 • 7 years ago

Many panels like the star panel and the downloads panel indeed use XUL lazy overlay loading to avoid having to initialize the whole contents when the window is opened. We likely need some alternative mechanism to remove that.

	:Paolo Amadini
	Comment 2 • 7 years ago

I'll also note that maybe the performance impact today may not be as visible as when the overlay was first added. The structure of the button is simpler, and setting hidden="true" may improve the performance of the panel even if the markup is present in the document, like we do for other panels.

	Brendan Dahl [:bdahl] Assignee
	Comment 5 • 6 years ago

I've run two talos tests[1][2] and each has one or two "regressions" but neither are the same so I don't think there's really any performance impact here.

I've attempted to simplify some of the logic in downloads.js since things are no longer async, but we may want to do another review of this at some point.

Also, I'm going to lump the indicator overlay removal in here too, since it is somewhat intertwined with the downloads overlay removal.


Try: run
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6622aa9188a85d3a13e9c7ed615e6c931bbaada7 

Talos:
[1] https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=da5a105ffa3f14c7bd09dc7ad2db8b61ef94517d&newProject=try&newRevision=f8d9d0e61bf843d72b41e7648a3e311696549545&framework=1&showOnlyImportant=1

[2] https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=01eeae59fa46244576d52ef103489cbbfcbd88e7&newProject=try&newRevision=eec97fb2dd5d5c381189248122d83696cc38a26a&framework=1&showOnlyImportant=1

	:Paolo Amadini
	Comment 6 • 6 years ago
mozreview-review

Comment on attachment 8952547 [details]
Bug 1434077 - Inline downloads overlay.

https://reviewboard.mozilla.org/r/221782/#review227782

Thanks for working on this!

::: browser/base/content/browser.xul:497
(Diff revision 1)
>  #else
>        <label class="tooltip-label" value="&backForwardButtonMenu.tooltip;"/>
>  #endif
>      </tooltip>
>  
> +

nit: unneeded whitespace change

::: browser/components/downloads/content/downloads.js:184
(Diff revision 1)
> -   * hasn't been loaded yet.
>     */
>    get panel() {
> -    // If the downloads panel overlay hasn't loaded yet, just return null
> -    // without resetting this.panel.
>      let downloadsPanel = document.getElementById("downloadsPanel");

You can inline "let downloadsPanel", or even call defineLazyGetter after the object declaration.

::: browser/components/downloads/content/panel.inc.xul:1
(Diff revision 1)
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - 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/. -->

Call this file "downloadsPanel.inc.xul", and use "hg mv" from "downloadsOverlay.xul" so we can track history.

	:Paolo Amadini
	Comment 7 • 6 years ago
mozreview-review

Comment on attachment 8952548 [details]
Bug 1434077 - Inline indicators overlay.

https://reviewboard.mozilla.org/r/221784/#review227784

::: browser/base/content/browser.xul:932
(Diff revision 1)
>                         removable="true"
>                         overflows="false"
>                         cui-areatype="toolbar"
>                         hidden="true"
> -                       tooltip="dynamic-shortcut-tooltip"/>
> +                       tooltip="dynamic-shortcut-tooltip"
> +                       indicator="true">

Please file a separate bug to investigate removing the "indicator" attribute and any other workaround we introduced because of the overlay. A separate bug is useful so we can track any regressions with that part separately.

::: browser/components/downloads/content/downloads.js:508
(Diff revision 1)
>      // still loading data.
>      if (this._state != this.kStateWaitingData || DownloadsView.loading) {
>        return;
>      }
>  
>      this._state = this.kStateWaitingAnchor;

I think you can safely remove the kStateWaitingAnchor state already, and move the check for the minimized window before we obtain the anchor.

::: browser/components/downloads/content/indicator.js:298
(Diff revision 1)
>      // If we don't have a _placeholder, there's no chance that the overlay
>      // will load correctly: bail (and don't set _operational to true!)
>      if (!DownloadsButton._placeholder) {
>        return;
>      }

nit: update the comment

::: browser/components/downloads/content/indicator.js:310
(Diff revision 1)
>  
> -        // If the view is initialized, we need to update the elements now that
> +    // If the view is initialized, we need to update the elements now that
> -        // they are finally available in the document.
> +    // they are finally available in the document.
> -        // We need to re-check for the placeholder because it might have
> +    // We need to re-check for the placeholder because it might have
> -        // disappeared since then.
> +    // disappeared since then.
> -        if (this._initialized && DownloadsButton._placeholder) {
> +    if (this._initialized && DownloadsButton._placeholder) {

The second check for DownloadsButton._placeholder can be removed.

	Brendan Dahl [:bdahl] Assignee
	Comment 8 • 6 years ago
mozreview-review

Comment on attachment 8952547 [details]
Bug 1434077 - Inline downloads overlay.

https://reviewboard.mozilla.org/r/221782/#review227930

::: browser/components/downloads/content/panel.inc.xul:1
(Diff revision 1)
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - 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/. -->

git doesn't pick this up as a move because of all the whitespace changes. Looks like I'll need to get a mercurial repo setup again.

	:Paolo Amadini
	Comment 11 • 6 years ago
mozreview-review

Comment on attachment 8952547 [details]
Bug 1434077 - Inline downloads overlay.

https://reviewboard.mozilla.org/r/221782/#review228172

	:Paolo Amadini
	Comment 12 • 6 years ago
mozreview-review

Comment on attachment 8952548 [details]
Bug 1434077 - Inline indicators overlay.

https://reviewboard.mozilla.org/r/221784/#review228174

Thank you! Just one small fix needed.

::: browser/components/downloads/content/downloads.js:506
(Diff revision 2)
> -      // At this point, if the window is minimized, opening the panel could fail
> +    // At this point, if the window is minimized, opening the panel could fail
> -      // without any notification, and there would be no way to either open or
> +    // without any notification, and there would be no way to either open or
> -      // close the panel any more.  To prevent this, check if the window is
> +    // close the panel any more.  To prevent this, check if the window is
> -      // minimized and in that case force the panel to the closed state.
> +    // minimized and in that case force the panel to the closed state.
> -      if (window.windowState == window.STATE_MINIMIZED) {
> +    if (window.windowState == window.STATE_MINIMIZED) {
> -        DownloadsButton.releaseAnchor();
> +      DownloadsButton.releaseAnchor();

No need to call releaseAnchor here.

	Pulsebot
	Comment 14 • 6 years ago

Pushed by bdahl@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f92273643f7a
Inline downloads overlay. r=Paolo
https://hg.mozilla.org/integration/autoland/rev/5facafd6e4a6
Inline indicators overlay. r=Paolo

	Andrei Ciure[:aciure]
	Comment 15 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/f92273643f7a
https://hg.mozilla.org/mozilla-central/rev/5facafd6e4a6