Dave Townsend [:mossop] Assignee
	Description • 6 years ago

Since bug 1453751 it's a bit more obvious that there is a delay between when the page finishes loading and the favicon becomes visible. I'm not sure if the delay is real or if something else is going on. The biggest problem to my eye is that the page finishes loading, we hide the throbber at which point the tab title jumps to the left until the favicon is loaded.

I think there are two options that would make this look nicer that we could consider.

1. Have the content process signal that it has found a favicon that it is loading and just display a favicon sized space in the tab until it completes loading. This would stop the title jump but has the downside that if the favicon fails to load for some reason we'd show the gap for a short time before hiding it again.

2. Keep the tab throbber showing until the favicon completes loading and we know it has either failed or succeeded.

I think I prefer option 2, what do you think?

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

This is a UX question, personally I'd keep the throbber, an empty space may look bogus

	Dão Gottwald [:dao]
	Comment 2 • 6 years ago

Wait... why / how has bug 1453751 made this worse or more obvious? Are we loading the favicon with a lower priority or something? Or is this IPC and transcoding overhead?

	:Gijs (he/him)
	Comment 3 • 6 years ago

(In reply to Dão Gottwald [::dao] from comment #2)
> Wait... why / how has bug 1453751 made this worse or more obvious? Are we
> loading the favicon with a lower priority or something? Or is this IPC and
> transcoding overhead?

I'm also curious about this. I also wonder if swapping from data: to blob: will improve this. Of course, the other confusing thing in my head is that AFAIK, http channels actually live in the parent, so in theory there should be some sane way of getting the result of that request in a sane way in the parent process, if indeed it succeeds, with minimal overhead. In practice, perhaps that's a laughable idea...

The other thing that I'm curious about is the perceived performance aspects of this. If we're showing the spinner for longer that presumably impacts on how quickly people think Firefox loads pages. It'd be unfortunate if we showed the spinner significantly longer than before.

My other suggestion would be that we could potentially use the page-icon that we may already have for some sites as a backup. That wouldn't require a network fetch and might allow us to show a favicon even sooner than we did before.

	Dave Townsend [:mossop] Assignee
	Comment 4 • 6 years ago

So there are a couple of things going on.

First thing to note is that in my measurements for the first load of a page I see around the same time between end of page load and favicon load for both old and new loading method. It's very noisy so I'd need to do more measurements to be sure but that's about what I would expect, there shouldn't really be any additional overhead here.

Subsequent loads though are slower, depending on the website. This is because the XUL image element uses the image cache, if it is loading an image already in the cache no network transfer happens and the image is already decoded in memory. The new method may need to touch the network or the network cache and convert to the data URI. I just experimented and it is fairly straightforward to do something similar in JS, caching the data URI for a given favicon URI giving similar performance for subsequent loads, so I've filed bug 1473601 to look into doing that.

So the first loads take the same time but the perception is better, because we show the (albeit empty) XUL image as soon as we know the favicon url to load whereas now we don't show it until we have downloaded the data. This causes the tab title to jump back and forth. In fact the old code didn't wait for the right favicon to start showing the image, as soon as load completed it showed the image and told it to load the root favicon (/favicon.ico) until that got overridden if the page specified something else. So we basically used to do what I propose as option 1 in comment 0.

	David Durst [:ddurst]
	Comment 5 • 6 years ago

Mossop -- any update on this (or bug 1473601)?

	Dave Townsend [:mossop] Assignee
	Comment 6 • 6 years ago

(In reply to David Durst [:ddurst] from comment #5)
> Mossop -- any update on this (or bug 1473601)?

Not yet.

	Dave Townsend [:mossop] Assignee
	Comment 8 • 6 years ago

Comment on attachment 8994901 [details]
Bug 1473514: Display an empty space for the tab icon while waiting for the real favicon to appear. r=dao

I'm not a big fan of this approach but it brings us back to what the situation looked like before the favicon changes. This just displays a pending (transparent) icon if we're waiting for the real favicon to load. I still need to do more checking that this doesn't break anything else but wanted to see what you thought of this first dao.

	Dão Gottwald [:dao]
	Comment 9 • 6 years ago

Comment on attachment 8994901 [details]
Bug 1473514: Display an empty space for the tab icon while waiting for the real favicon to appear. r=dao

This seems okay to me if this is the UX we want. Not quite sure about that.

	Dave Townsend [:mossop] Assignee
	Comment 10 • 6 years ago

Going to put this on hold to see if bug 1478823 works out, that gets rid of a major source of slowness for when favicons appear on sites like amazon.com.

	Dave Townsend [:mossop] Assignee
	Comment 11 • 6 years ago

Now that bug 1478823 is fixed I believe that the main source of delays in displaying favicons is gone so I think we should re-prioritise this.

	Sören Hentzschel
	Comment 13 • 6 years ago

Bug 1483492 has been marked as duplicate of this bug which has been de-prioritized to P5 a few days ago. While this ticket is about slowness the other ticket is about a visible and annoying regression of the primary UI. I guess this means that this ticket should be re-prioritised again.

	Dave Townsend [:mossop] Assignee
	Comment 14 • 6 years ago

(In reply to Sören Hentzschel from comment #13)
> Bug 1483492 has been marked as duplicate of this bug which has been
> de-prioritized to P5 a few days ago. While this ticket is about slowness the
> other ticket is about a visible and annoying regression of the primary UI. I
> guess this means that this ticket should be re-prioritised again.

The first comment explicitly calls out the behaviour you're seeing and it has been mostly mitigated by bug 1478823. The patch already here could also take care of any remaining title shifts.

	Sören Hentzschel
	Comment 15 • 6 years ago

Unfortunately the patch of bug 1478823 doesn't seem to be good enough because it still happens very often for me. :-(

	Marcia Knous [:marcia]
	Comment 16 • 6 years ago

Per Triage team - this keeps coming up in our weekly regression triage. Given the fact that Soren is still seeing an issue, is there anything we should do here? Thanks.

	David Durst [:ddurst]
	Comment 18 • 6 years ago

(In reply to Sören Hentzschel from comment #15)
> Unfortunately the patch of bug 1478823 doesn't seem to be good enough
> because it still happens very often for me. :-(

Curious why you see this very often. What's different about your setup?

	Sören Hentzschel
	Comment 19 • 6 years ago

I open a lot of Bugzilla tabs every day and Bugzilla is more affected than most other websites. ghacks.net is another example for a website where it happens A LOT.

About my setup: When I wrote comment #15 I used a MacBook Pro Late 2013, now I have a MacBook Pro 2018, both with macOS Mojave Public Beta, otherwise nothing special.

	Dave Townsend [:mossop] Assignee
	Comment 20 • 6 years ago

Comment on attachment 8994901 [details]
Bug 1473514: Display an empty space for the tab icon while waiting for the real favicon to appear. r=dao

Made this a bit less hacky. Still not great but it's simple which is probably want we want this close to the merge.

	Dão Gottwald [:dao]
	Comment 21 • 6 years ago

Comment on attachment 8994901 [details]
Bug 1473514: Display an empty space for the tab icon while waiting for the real favicon to appear. r=dao

Dão Gottwald [::dao] has approved the revision.

	Pulsebot
	Comment 22 • 6 years ago

Pushed by dtownsend@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/141d116489a7
Display an empty space for the tab icon while waiting for the real favicon to appear. r=dao

	Arthur Iakab [arthur_iakab]
	Comment 23 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/141d116489a7