1363059 - Investigate icons that load during startup but aren't displayed

Status: RESOLVED FIXED

(Firefox :: Theme, enhancement, P1)

Description

[Jonathan Watt [:jwatt]]

In addition to the icon covered by bug 1363040, when I load Firefox and just display the start page I see various other icons loading that are not actually displayed. We should investigate whether we can and should avoid loading these icons and spin off bugs as appropriate.


chrome://browser/skin/toolbarbutton-dropdown-arrow.png

  I thought this was part of the magnifying glass in the search
  field, but that doesn't seem to be the case since the arrow
  there is baked into the magnifying glass icon in
  chrome://browser/skin/[e-mail]


chrome://browser/skin/places/[e-mail]

  This doesn't seem to be the icon we use, even after clicking
  on the bookmark icon (this has rounded corners on the right
  hand side, but the one we use seems to be joined to the
  bookmarks menu button without the corners at the join
  being rounded.


chrome://browser/skin/places/toolbarDropMarker.png
chrome://browser/skin/places/unfiledBookmarks.png
chrome://mozapps/skin/places/[e-mail]
chrome://global/skin/icons/[e-mail]
chrome://global/skin/icons/autoscroll.png
chrome://global/skin/tree/[e-mail]
chrome://branding/content/identity-icons-brand.svg
resource://gre-resources/loading-image.png
resource://gre-resources/broken-image.png

  These don't seem to be displayed


chrome://pocket/content/panels/img/[e-mail]

  Seems like this should only be displayed once the
  Pocket button is actually clicked. (And it seems like we
  should be using chrome://pocket-shared/skin/pocket.svg
  with an appropriate -moz-context-properties fill rather
  than this PNG icon.)


chrome://browser/skin/search-arrow-go.svg#search-arrow-go-inverted

  The default them uses the non-inverted icon, so it seems
  like we shouldn't be loading this inverted one before
  then going on to load the non-inverted variant
  chrome://browser/skin/search-arrow-go.svg#search-arrow-go

Comment 1

[:Gijs (out for now; he/him)]

Florian, is this qf/photon-perf fodder?

Comment 2

[Florian Quèze [:florian]]

(In reply to :Gijs from comment #1)
> Florian, is this qf/photon-perf fodder?

Could be :-). Although it could also be taken care of by the photon team cleaning up our CSS.

Jonathan, how did you build this list? I'm asking because I'm wondering if there's any way we could automate that process in a test, so that new unused images don't get added.

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: patch that will log-image-creation to the console

I used this patch.

Comment 4

[Johann Hofmann [:johannh]]

Do you have the bookmark toolbar hidden when opening the browser?

Comment 5

[Johann Hofmann [:johannh]]

> chrome://branding/content/identity-icons-brand.svg

This one is displayed in the identity block on about:home

Comment 6

[Jonathan Watt [:jwatt]]

(In reply to Johann Hofmann [:johannh] from comment #4)
> Do you have the bookmark toolbar hidden when opening the browser?

Yes. (That's the default for a new profile.)

(In reply to Johann Hofmann [:johannh] from comment #5)
> > chrome://branding/content/identity-icons-brand.svg
> 
> This one is displayed in the identity block on about:home

Ah, so it is.

Comment 7

[Florian Quèze [:florian]]

(In reply to Jonathan Watt [:jwatt] from comment #3)
> Created attachment 8869054 [details] [diff] [review]
> patch that will log-image-creation to the console
> 
> I used this patch.

This seems to cover the first half of the problem (building the list of images we load), but what about the other half (deciding if an image has been displayed)?

Comment 8

[Johann Hofmann [:johannh]]

(In reply to Jonathan Watt [:jwatt] from comment #6)
> (In reply to Johann Hofmann [:johannh] from comment #4)
> > Do you have the bookmark toolbar hidden when opening the browser?
> 
> Yes. (That's the default for a new profile.)

Huh, that's bad then. I'm not very familiar with the bookmarks toolbar, does anyone know why so many bookmark toolbar icons have to load without the toolbar even showing? It's at least:

chrome://browser/skin/places/toolbarDropMarker.png
chrome://browser/skin/places/[e-mail] (which is just the placeholder for customize mode afaik)
chrome://browser/skin/places/unfiledBookmarks.png
chrome://global/skin/icons/[e-mail]
chrome://global/skin/tree/[e-mail]

Comment 9

[Jonathan Watt [:jwatt]]

(In reply to Florian Quèze [:florian] [:flo] from comment #7)
> This seems to cover the first half of the problem (building the list of
> images we load), but what about the other half (deciding if an image has
> been displayed)?

I did that manually via visual inspection which is most directly what we care about. To automate that it may be possible to hack into VectorImage::Show for SVG images and perhaps RasterImage::LookupFrameInternal/RasterImage::GetFrameInternal/RasterImage::DrawInternal for raster images (I'm less familiar with the latter).

Comment 10

[:Gijs (out for now; he/him)]

(In reply to Johann Hofmann [:johannh] from comment #8)
> (In reply to Jonathan Watt [:jwatt] from comment #6)
> > (In reply to Johann Hofmann [:johannh] from comment #4)
> > > Do you have the bookmark toolbar hidden when opening the browser?
> > 
> > Yes. (That's the default for a new profile.)
> 
> Huh, that's bad then. I'm not very familiar with the bookmarks toolbar, does
> anyone know why so many bookmark toolbar icons have to load without the
> toolbar even showing? It's at least:
> 
> chrome://browser/skin/places/toolbarDropMarker.png
> chrome://browser/skin/places/[e-mail] (which is just the
> placeholder for customize mode afaik)
> chrome://browser/skin/places/unfiledBookmarks.png
> chrome://global/skin/icons/[e-mail]
> chrome://global/skin/tree/[e-mail]

I suspect it's because the toolbar starts off collapsed rather than hidden? Which I think means layout still runs on it and presumably that's why we load all the images, too... assuming we don't load images for elements with display: none, anyway. You could test this hypothesis fairly easily by adding hidden=true to the toolbar, running ./mach build faster, and seeing if those files still show up as being loaded immediately.

If so, it's possible we could try to come up with some way of lazily initializing the toolbar and keeping it hidden=true initially.

Comment 11

[Johann Hofmann [:johannh]]

I'm looking into making a test for this.

Comment 12

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b28fd3d7deef708429206df9ba793b4ca0455300

Comment 13

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=25ed7766adc56df0540f2da4696efaed59dcec01

Comment 14

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f136e7e91a477e3a61bf9c1d98fe6ba49aaa312a

Comment 15

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd5517250e739fa59a4d9e42c67eb893bb974095

Comment 16

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=963ef2d6ac6e79794a5dcb4e6054dbf3762b48cf

Comment 17

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd48a87a97e7448df0b7385ef767ee768909ffd2

Comment 18

[Johann Hofmann [:johannh]]

Attachment: Bug 1363059 - Add a test for images loaded at startup vs. images shown at startup.

This patch enables startupRecorder.js to collect data on
loaded and shown raster and SVG images on startup via events
from native code. It also adds a test that uses this data
to find images that are unnecessarily loaded.

I've not fixed any of the affected images yet, there's a
fairly comprehensive whitelist that I want to gradually
decrease by opening bugs in the respective components.

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

Comment 19

[Johann Hofmann [:johannh]]

I haven't fully fine-tuned the whitelist yet but it's far enough to get review, I think.

Comment 20

[Jonathan Watt [:jwatt]]

Comment on attachment 8878561 [details]
Bug 1363059 - Add a test for images loaded at startup vs. images shown at startup.

https://reviewboard.mozilla.org/r/149890/#review154560

r+ for the platform changes. I assume florian will review the browser changes.

::: image/ImageFactory.cpp:100
(Diff revision 1)
> +#ifdef DEBUG
> +  // Record the image load for performance testing.
> +  if (NS_IsMainThread()) {
> +    nsAutoCString spec;
> +    aURI->GetSpec(spec);
> +    nsCOMPtr<nsIObserverService> obs = services::GetObserverService();

It would be cleaner if you put the spec fetching code in the |if|.

::: image/ImageFactory.cpp:101
(Diff revision 1)
> +  // Record the image load for performance testing.
> +  if (NS_IsMainThread()) {
> +    nsAutoCString spec;
> +    aURI->GetSpec(spec);
> +    nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> +    if (obs) {

Make that:

  if (NS_WARN_IF(obs)) {

::: image/ImageFactory.cpp:102
(Diff revision 1)
> +  if (NS_IsMainThread()) {
> +    nsAutoCString spec;
> +    aURI->GetSpec(spec);
> +    nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> +    if (obs) {
> +      obs->NotifyObservers(nullptr, "image-loaded", NS_ConvertUTF8toUTF16(spec).get());

I think this should be "image-loading" or "image-load-started". "loaded" is past tense, which implies that the image has finished loading whereas we're only just kicking off the load here.

::: image/RasterImage.cpp:1409
(Diff revision 1)
> +  if (NS_IsMainThread()) {
> +    nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> +    nsCOMPtr<nsIURI> imageURI = mURI->ToIURI();
> +    nsAutoCString spec;
> +    imageURI->GetSpec(spec);
> +    if (obs) {

My 'spec' and NS_WARN_IF comment above apply here too.

::: image/RasterImage.cpp:1410
(Diff revision 1)
> +    nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> +    nsCOMPtr<nsIURI> imageURI = mURI->ToIURI();
> +    nsAutoCString spec;
> +    imageURI->GetSpec(spec);
> +    if (obs) {
> +      obs->NotifyObservers(nullptr, "image-shown", NS_ConvertUTF8toUTF16(spec).get());

And "shown" is past tense, which again is wrong since we're just starting the draw here. Maybe call it "image-drawing" or "image-draw-started".

::: image/VectorImage.cpp:1035
(Diff revision 1)
> +  if (NS_IsMainThread()) {
> +    nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> +    nsCOMPtr<nsIURI> imageURI = mURI->ToIURI();
> +    nsAutoCString spec;
> +    imageURI->GetSpec(spec);
> +    if (obs) {

My 'spec', NS_WARN_IF and "shown" comments above apply here too.

Comment 21

[Florian Quèze [:florian]]

Comment on attachment 8878561 [details]
Bug 1363059 - Add a test for images loaded at startup vs. images shown at startup.

https://reviewboard.mozilla.org/r/149890/#review155020

r=me, but feel free to rerequest review (with a needinfo I guess) if the changes turn out to be more significant than you are comfortable carrying forward a review for.

::: browser/base/content/test/performance/browser_startup_images.js:12
(Diff revision 1)
> + * List items support the following attributes:
> + *  - file: The location of the loaded image file.
> + *  - hidpi: An alternative hidpi file location for retina screens, if one exists.
> + *           May be the magic string <not loaded> in strange cases where
> + *           only the low-resolution image is loaded but not shown.
> + *  - platforms: An array of the platforms where the issue is occurring.

You may want to document the supported platforms, as win could be windows and macosx could be mac...

Should we make the platform field optional and assume that a whitelist entry applies to all platforms when the platform isn't specified?

::: browser/base/content/test/performance/browser_startup_images.js:157
(Diff revision 1)
> +  },
> +];
> +
> +function test() {
> +  let data = Cc["@mozilla.org/test/startuprecorder;1"].getService().wrappedJSObject.data.images;
> +  let platformWhitelist = whitelist.filter((el) => el.platforms.includes(AppConstants.platform));

nit: we don't need the parens around 'el'

::: browser/base/content/test/performance/browser_startup_images.js:183
(Diff revision 1)
> +
> +  for (let item of platformWhitelist) {
> +    if (!item.intermittentNotLoaded ||
> +        !item.intermittentNotLoaded.includes(AppConstants.platform)) {
> +      if (window.devicePixelRatio >= 2 && item.hidpi) {
> +        ok(loadedImages.has(item.hidpi) || item.hidpi == "<not loaded>", `Whitelisted image ${item.hidpi} was loaded.`);

"Whitelisted image <not loaded> was loaded." will be a confusing test output.

::: browser/base/content/test/performance/browser_startup_images.js:185
(Diff revision 1)
> +    if (!item.intermittentNotLoaded ||
> +        !item.intermittentNotLoaded.includes(AppConstants.platform)) {
> +      if (window.devicePixelRatio >= 2 && item.hidpi) {
> +        ok(loadedImages.has(item.hidpi) || item.hidpi == "<not loaded>", `Whitelisted image ${item.hidpi} was loaded.`);
> +      } else {
> +        ok(loadedImages.has(item.file), `Whitelisted image ${item.file} was loaded.`);

If the point of this check is to prevent stale entries in the whitelist, should we also verify that the item was not shown?

::: browser/components/tests/startupRecorder.js:34
(Diff revision 1)
> -  this.data = {};
> +  this.data = {
> +    images: {
> +      "image-shown": new Set(),
> +      "image-loaded": new Set(),
> +    },
> +    js: { }

I don't think 'js' is a correct name here, as this will contain info about services implemented in C++.. Maybe 'code'?

nit: I usually write {} rather than { } for an empty object, but I'm not sure we have any consistent coding style on this point.

Comment 22

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb118db362259a1ff32488868e8238b03638f40f

Comment 23

[Johann Hofmann [:johannh]]

Comment on attachment 8878561 [details]
Bug 1363059 - Add a test for images loaded at startup vs. images shown at startup.

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

Comment 24

[Johann Hofmann [:johannh]]

Comment on attachment 8878561 [details]
Bug 1363059 - Add a test for images loaded at startup vs. images shown at startup.

https://reviewboard.mozilla.org/r/149890/#review155020

> You may want to document the supported platforms, as win could be windows and macosx could be mac...
> 
> Should we make the platform field optional and assume that a whitelist entry applies to all platforms when the platform isn't specified?

I'd rather specify the platforms explicitly, it's not so much overhead right now.

Comment 25

[Johann Hofmann [:johannh]]

The try push is looking green, even after retriggers. I'm a bit worried that there's no way to run try for hidpi screens afaik, so I would want to open a follow-up for looking into automating this test for hidpi screens.

I think we can also have this patch resolve this bug and open follow-up bugs for the individual whitelist items.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7e137dfb0bff
Add a test for images loaded at startup vs. images shown at startup. r=florian,jwatt

Comment 27

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=109214906&repo=autoland

Comment 28

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/023b96f48df8
Backed out changeset 7e137dfb0bff for failures like browser_startup_images.js | Loaded image chrome://browser/skin/sidebars.svg was shown

Comment 29

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=90fa8bab86b14d3990ed0fdc8bbba61bba419fde

Comment 30

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Johann Hofmann [:johannh] from comment #25)
> The try push is looking green, even after retriggers. I'm a bit worried that
> there's no way to run try for hidpi screens afaik, so I would want to open a
> follow-up for looking into automating this test for hidpi screens.
> 
> I think we can also have this patch resolve this bug and open follow-up bugs
> for the individual whitelist items.

For HiDPI screens all we will need to do is set layout.css.devPixelsPerPx to 2 before starting up Firefox. You could run the test once with the default value, change the pref before shutting down, then run the test a second time using the changed value, then reset the pref and finish the test. We won't actually need HiDPI screens/hardware to simulate the environment.

Comment 31

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=345066e07855bc49975393ec401553981e1138b3

Comment 32

[Johann Hofmann [:johannh]]

Try is green, I'll try to land this. (Let's see if something changed under our feet again)

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #30)
> (In reply to Johann Hofmann [:johannh] from comment #25)
> > The try push is looking green, even after retriggers. I'm a bit worried that
> > there's no way to run try for hidpi screens afaik, so I would want to open a
> > follow-up for looking into automating this test for hidpi screens.
> > 
> > I think we can also have this patch resolve this bug and open follow-up bugs
> > for the individual whitelist items.
> 
> For HiDPI screens all we will need to do is set layout.css.devPixelsPerPx to
> 2 before starting up Firefox. You could run the test once with the default
> value, change the pref before shutting down, then run the test a second time
> using the changed value, then reset the pref and finish the test. We won't
> actually need HiDPI screens/hardware to simulate the environment.

Thanks for the hint! After looking into this I'd rather open a new bug for high dpi support. It is complicated enough already right now.

Comment 33

[Johann Hofmann [:johannh]]

Comment on attachment 8878561 [details]
Bug 1363059 - Add a test for images loaded at startup vs. images shown at startup.

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

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d2f4da0ae681
Add a test for images loaded at startup vs. images shown at startup. r=florian,jwatt

Comment 35

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

Backed out for failing own test browser_startup_images.js, at least on Windows 7 VM debug with e10s:

https://hg.mozilla.org/integration/autoland/rev/6b0f55d8a2d24606dff719d52a3e6598ce92664e

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d2f4da0ae6819be3b3ced71947f66585df58e145&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=109854752&repo=autoland

 634 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_startup_images.js | Whitelisted image chrome://browser/skin/urlbar-history-dropmarker.png was not shown. -
638 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_startup_images.js | Whitelisted image chrome://browser/skin/reload-stop-go.png was not shown. -

Comment 36

[Johann Hofmann [:johannh]]

Comment on attachment 8878561 [details]
Bug 1363059 - Add a test for images loaded at startup vs. images shown at startup.

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

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2cb9f9ba6b91
Add a test for images loaded at startup vs. images shown at startup. r=florian,jwatt

Comment 38

[Carsten Book [:Tomcat]]

sorry backed out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=110328296&repo=autoland

Comment 39

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3532c51a0635
Backed out changeset 2cb9f9ba6b91 on a CLOSED TREE for failures in browser_startup_images.js | Loaded image chrome://browser/skin/bookmark-hollow.svg was shown

Comment 40

[Johann Hofmann [:johannh]]

Comment on attachment 8878561 [details]
Bug 1363059 - Add a test for images loaded at startup vs. images shown at startup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149890/diff/4-5/

Comment 41

[Johann Hofmann [:johannh]]

Comment on attachment 8878561 [details]
Bug 1363059 - Add a test for images loaded at startup vs. images shown at startup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149890/diff/5-6/

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/31f204fd0550
Add a test for images loaded at startup vs. images shown at startup. r=florian,jwatt

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/31f204fd0550

Comment 44

[Timothy Nikkel (:tnikkel)]

There are other ways that a RasterImage gets used beside DrawInternal. GetImageContainer and GetFrame/GetFrameAtSize specifically.

Comment 45

[Jonathan Watt [:jwatt]]

(In reply to Timothy Nikkel (:tnikkel) from comment #44)
> There are other ways that a RasterImage gets used beside DrawInternal.
> GetImageContainer and GetFrame/GetFrameAtSize specifically.

I should have referred the RasterImage changes to you. It seems a bit weird that the code for decoding images would be duplicated in multiple places though. Isn't there a single, central place we can hook into for raster images?

Johann, I guess you're not hitting any of these paths otherwise you'd have complained about the images not being reported correctly, but it would still be good to have this code correctly cover these bases. Once tnikkel replies to my question above can you create a follow-up patch to address his comments and get him to review (maybe in a new bug)?

Comment 46

[Johann Hofmann [:johannh]]

(In reply to Jonathan Watt [:jwatt] from comment #45)
> (In reply to Timothy Nikkel (:tnikkel) from comment #44)
> > There are other ways that a RasterImage gets used beside DrawInternal.
> > GetImageContainer and GetFrame/GetFrameAtSize specifically.
> 
> I should have referred the RasterImage changes to you. It seems a bit weird
> that the code for decoding images would be duplicated in multiple places
> though. Isn't there a single, central place we can hook into for raster
> images?
> 
> Johann, I guess you're not hitting any of these paths otherwise you'd have
> complained about the images not being reported correctly, 

At least not in an obvious way, but there might be a false positive I'm not recognizing.

> but it would still
> be good to have this code correctly cover these bases. Once tnikkel replies
> to my question above can you create a follow-up patch to address his
> comments and get him to review (maybe in a new bug)?

Absolutely!

Comment 47

[Johann Hofmann [:johannh]]

Needinfo for comment 45. Otherwise I'll make a bug for hooking into the function mentioned in comment 44.

Comment 48

[Timothy Nikkel (:tnikkel)]

Bug 1386439 handled this ni. Clearing.