1380377 - sync-illustration.svg is loaded but not shown at browser startup

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Johann Hofmann [:johannh]]

According to our new browser_startup_image.js test, sync-illustration.svg is loaded during browser startup but not shown. For startup performance and memory usage, we should avoid loading images that aren't going to be displayed.

Comment 1

[Johann Hofmann [:johannh]]

Mark, as the author of the patches in bug 1201331, can you please help resolve this issue?

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Ed, do you mind having a look at this?

Comment 3

[Edouard Oger [:eoger]]

I think this is because of http://searchfox.org/mozilla-central/source/browser/components/customizableui/content/panelUI.inc.xul#187, which is weird because it is under #PanelUI-popup which is hidden by default.

Comment 4

[Florian Quèze [:florian]]

There's probably a missing hidden="true" somewhere, but I don't see where. Is there a reason why this is using html:img rather than xul image tags? (I'm not aware of this being a problem, but it seems to be a difference compared to the other images here, so I wonder if it has an impact.)

Comment 5

[Edouard Oger [:eoger]]

Will investigate.

Comment 6

[Edouard Oger [:eoger]]

I can't reproduce, instead the tests are failing for me: "sync-illustration.svg should have been loaded"

Comment 7

[Johann Hofmann [:johannh]]

(In reply to Edouard Oger [:eoger] from comment #6)
> I can't reproduce, instead the tests are failing for me:
> "sync-illustration.svg should have been loaded"

Are you testing on a debug build? It doesn't work otherwise.

Comment 8

[Edouard Oger [:eoger]]

Attachment: Bug 1380377 part 2 - Make sure sync-illustration.svg doesn't get loaded on startup.

Review commit: https://reviewboard.mozilla.org/r/169270/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169270/

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8897962 [details]
Bug 1380377 part 2 - Make sure sync-illustration.svg doesn't get loaded on startup.

https://reviewboard.mozilla.org/r/169270/#review174702

thanks!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b0f76b216a19
Make sure sync-illustration.svg doesn't get loaded on startup. r=markh

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b0f76b216a19

Comment 12

[Johann Hofmann [:johannh]]

Awesome, thanks for fixing this!

Comment 13

[Mike Conley (:mconley) (:⚙️)]

This got backed out for mysteriously causing bug 1392280:

https://hg.mozilla.org/mozilla-central/rev/5f5549876ea6

Comment 14

[Edouard Oger [:eoger]]

Attachment: Bug 1380377 part 1 - Calculate FX_NEW_WINDOW_MS using MozAfterPaint.

Review commit: https://reviewboard.mozilla.org/r/173998/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/173998/

Comment 15

[Edouard Oger [:eoger]]

Comment on attachment 8897962 [details]
Bug 1380377 part 2 - Make sure sync-illustration.svg doesn't get loaded on startup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169270/diff/1-2/

Comment 16

[Edouard Oger [:eoger]]

Florian does the approach I took makes sense?
I wonder if we might miss the first MozAfterPaint even since we’re registering the event handler *after* we open the new window.

Comment 17

[Florian Quèze [:florian]]

Comment on attachment 8902419 [details]
Bug 1380377 part 1 - Calculate FX_NEW_WINDOW_MS using MozAfterPaint.

https://reviewboard.mozilla.org/r/173998/#review179570

(In reply to Edouard Oger [:eoger] from comment #16)
> Florian does the approach I took makes sense?

Yes (r- because I have some trivial comments below)

> I wonder if we might miss the first MozAfterPaint even since we’re
> registering the event handler *after* we open the new window.

I don't think so.

::: browser/base/content/browser.js:4186
(Diff revision 1)
>    var telemetryObj = {};
>    TelemetryStopwatch.start("FX_NEW_WINDOW_MS", telemetryObj);
>  
> -  function newDocumentShown(doc, topic, data) {
> -    if (topic == "document-shown" &&
> -        doc != document &&
> +  function mozAfterPaint(e) {
> +    if (e.target.document != document &&
> +        e.target.document.defaultView == win) {

I don't think we need these tests anymore with an event listener on the window instead of a global observer notification.

::: browser/base/content/browser.js:4187
(Diff revision 1)
>    TelemetryStopwatch.start("FX_NEW_WINDOW_MS", telemetryObj);
>  
> -  function newDocumentShown(doc, topic, data) {
> -    if (topic == "document-shown" &&
> -        doc != document &&
> -        doc.defaultView == win) {
> +  function mozAfterPaint(e) {
> +    if (e.target.document != document &&
> +        e.target.document.defaultView == win) {
> +      win.removeEventListener("MozAfterPaint", mozAfterPaint);

Could we use the {once: true} option of addEventListener instead?

::: browser/base/content/browser.js:4200
(Diff revision 1)
> -      Services.obs.removeObserver(newDocumentShown, "document-shown");
> +      win.removeEventListener("MozAfterPaint", mozAfterPaint);
>        Services.obs.removeObserver(windowClosed, "domwindowclosed");
>      }
>    }
>  
> -  // Make sure to remove the 'document-shown' observer in case the window
> +  // Make sure to remove the 'MozAfterPaint' observer in case the window

It's not an observer, but an event listener.

And I doubt that would still leak, but I'm not opposed to keeping the windowClosed observer if we are not sure.

Comment 18

[Edouard Oger [:eoger]]

Comment on attachment 8902419 [details]
Bug 1380377 part 1 - Calculate FX_NEW_WINDOW_MS using MozAfterPaint.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173998/diff/1-2/

Comment 19

[Edouard Oger [:eoger]]

Comment on attachment 8897962 [details]
Bug 1380377 part 2 - Make sure sync-illustration.svg doesn't get loaded on startup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169270/diff/2-3/

Comment 20

[Florian Quèze [:florian]]

Comment on attachment 8902419 [details]
Bug 1380377 part 1 - Calculate FX_NEW_WINDOW_MS using MozAfterPaint.

https://reviewboard.mozilla.org/r/173998/#review179944

Looks good to me, thanks!

Before landing, please verify:
- on try that this doesn't leak (ie. run all mochitests on debug builds on Windows/Linux/Mac)
- locally that the measured times for FX_NEW_WINDOW_MS shown on about:telemetry are similar before/after applying the sync-illustration.svg changes.

Comment 21

[Edouard Oger [:eoger]]

All good, landing!

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9d56bf3a749d
part 1 - Calculate FX_NEW_WINDOW_MS using MozAfterPaint. r=florian
https://hg.mozilla.org/integration/autoland/rev/77983a8e6ce9
part 2 - Make sure sync-illustration.svg doesn't get loaded on startup. r=markh

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9d56bf3a749d
https://hg.mozilla.org/mozilla-central/rev/77983a8e6ce9