Sören Hentzschel Reporter
	Description • 7 years ago

There are a few issues with Favicons in Firefox Nightly 55. For one page in my history Firefox suddenly shows a wrong Favicon in the url bar dropdown, in other cases there are sometimes HiDPI favicons and sometimes not. Also the default globe icon is no longer an HiDPI icon.

cc @Marco Bonardo because he is the places/favicon expert. ;)

	Marco Bonardo [:mak] Assignee
	Comment 3 • 7 years ago

(In reply to Sören Hentzschel from comment #0)
> Created attachment 8863182 [details]
> screen-1.png
> 
> There are a few issues with Favicons in Firefox Nightly 55. For one page in
> my history Firefox suddenly shows a wrong Favicon in the url bar dropdown,

The favicons may differ now, the behavior changed. bug 1347532 and bug 1352459 may improve things, or not, depending on the situation.

Let's see the cases one by one.

Screen1.png
in this case looks like the page has an hi-res root domain favicon and a lo-res local favicon. The browser picks the last defined icon that is likely the lo-res one. the favicons service picks the hi-res one. The fix is trivial for the website, either update the wrong favicon.ico with the right one, or provide an hi-res icon, that will be preferred over the other one.
To be clear, here we COULD use the right icon, but we'd then ignore the possible existence of a better one. The service cannot know if the webmaster forgot to replace some icons on the server.
This is a WONTFIX, for now.

Screen2.png
The website has
https://www.soeren-hentzschel.at/favicon.ico
that looks like being hi-res, but then the pages code defines a lot of other icons through <link rel="icon"...
The first link rel points to a 16x16 icon, and since firefox (until we fix bug 1352459) picks the first defined icon, it will pick up this 16px version.
This is easily fixable by pointing first to the ico file rather than single pngs, then Firefox would likely pick the ico since it's the first icon. bug 1352459 may change the bogus picking and solve this better.
Note that icons are expired after 7 days, so it may take that time before things start looking correct in the ui.
This is basically a dupe of bug 1352459.

screen-3.png
This is the only real new bug reported here, and good catch!
Before, when a page didn't have an icon, we were leaving the default image defined in the theme, that has hi-res support.
But now it's the page-icon protocol that fetches the favicon, and the default Favicon is hardcoded in Places as a 16x16 png here:
http://searchfox.org/mozilla-central/rev/ae8c2e2354db652950fe0ec16983360c21857f2a/toolkit/components/places/nsIFaviconService.idl#155

This is wrong, it has always been, there has been a discussion in bug 1074937:
https://bugzilla.mozilla.org/show_bug.cgi?id=1074937#c13
https://bugzilla.mozilla.org/show_bug.cgi?id=1074937#c19
https://bugzilla.mozilla.org/show_bug.cgi?id=1074937#c20

TL;DR we could use an svg, but that has defects:
1. the svg file is much bigger than simple pngs, since we show this often that may cause performance and memory concerns.
2. some tests are not expecting a text response (the comment is unclear about which ones and the failure rate)

A simpler alternative may be to pack the 2 current png favicons (defaultFavicon.png and defaultFavicon@2x.png).
There is a problem yet, we basically are unable to use defaultFavicon-inverted.png since the page-icon protocol doesn't know anything about the UI needs. This is worth a bug apart to evaluate what to do about it.

	Marco Bonardo [:mak] Assignee
	Comment 4 • 7 years ago

To sum up, the short term plan is to pack the 2 pngs into an ico file, and hardcode the ico file as default in Places.
This may allow more easily in the future to replace it with another format, like svg, if we should ever decide to, since I'll have to also provide a default mime through the API and fix code expecting an hardcoded png.

The inverted icon problem has been moved to bug 1361291.

	Sören Hentzschel Reporter
	Comment 5 • 7 years ago

Thanks for the detailled analysis!

re 1: But should there not be the same favicon in the tab and in the url bar? I understand if there is in both cases the one favicon or in both cases the other favicon (even if it's the "wrong" favicon), but I think it should be consistent within Firefox.

Hm… I tested again in a new Firefox profile and I can't reproduce the issue in the new profile. Does this mean something has gone wrong with the update from one Nightly version to another?

re 2: Thank you, I will change the order!

	Marco Bonardo [:mak] Assignee
	Comment 6 • 7 years ago

(In reply to Sören Hentzschel from comment #5)
> Thanks for the detailled analysis!
> 
> re 1: But should there not be the same favicon in the tab and in the url
> bar? I understand if there is in both cases the one favicon or in both cases
> the other favicon (even if it's the "wrong" favicon), but I think it should
> be consistent within Firefox.

ideally yes, but the code paths are different for various reasons. bug 1352459 may change things.

> Hm… I tested again in a new Firefox profile and I can't reproduce the issue
> in the new profile. Does this mean something has gone wrong with the update
> from one Nightly version to another?

It's possible your old database has entries the new one doesn't have yet.

	Dão Gottwald [:dao]
	Comment 8 • 7 years ago

(In reply to Marco Bonardo [::mak] from comment #3)
> TL;DR we could use an svg, but that has defects:
> 1. the svg file is much bigger than simple pngs, since we show this often
> that may cause performance and memory concerns.

Has Stephen or somebody else looked into optimizing the SVG, e.g. dropping unnecessarily detailed paths etc.? We're already using SVG in many places and it's soon going to replace even more PNGs to the point where most images in the primary UI will be SVGs. I don't think it makes sense to avoid SVG here for abstract performance and memory usage concerns.

	Marco Bonardo [:mak] Assignee
	Comment 9 • 7 years ago

Well, the difference is that we could show this image thousands of times, rather than a few times like most of the other icons, for example in Places history views. There's not doubt a simple 16+32 ico file is more efficient than svg, both perf and memory-wise. Even if the benefit would be minimal, multiplied by thousands it will be interesting.

Let me ni? Stephen and see what he thinks.

	Dão Gottwald [:dao]
	Comment 10 • 7 years ago

(In reply to Marco Bonardo [::mak] from comment #9)
> Well, the difference is that we could show this image thousands of times,
> rather than a few times like most of the other icons, for example in Places
> history views. There's not doubt a simple 16+32 ico file is more efficient
> than svg, both perf and memory-wise. Even if the benefit would be minimal,
> multiplied by thousands it will be interesting.

It should be rasterized once and then reused like other images. I don't think there should be a difference in efficiency that's worth optimizing for.

	Marco Bonardo [:mak] Assignee
	Comment 11 • 7 years ago

Ok, the current ico is 3KiB (4KiB on disk), if we can obtain an SVG not much larger than that, I'd be happy to try what out tests think of it. In the meanwhile, I'm still fighting with the moz-anno test that has some failures. Ideally once tests are happy with .ico, the step to svg *should* not be crazy, so this is not wasted work.

	Marco Bonardo [:mak] Assignee
	Comment 12 • 7 years ago

(In reply to Dão Gottwald [::dao] from comment #10)
> It should be rasterized once and then reused like other images. I don't
> think there should be a difference in efficiency that's worth optimizing for.

As a side note, each favicon in the view will have a different uri (page-icon:page_url), so the image cache won't help us here.

	Dão Gottwald [:dao]
	Comment 14 • 7 years ago

(In reply to Marco Bonardo [::mak] from comment #12)
> (In reply to Dão Gottwald [::dao] from comment #10)
> > It should be rasterized once and then reused like other images. I don't
> > think there should be a difference in efficiency that's worth optimizing for.
> 
> As a side note, each favicon in the view will have a different uri
> (page-icon:page_url), so the image cache won't help us here.

Hmm, are those really the URIs imagelib will use? Don't they resolve to the same URI internally?

	Marco Bonardo [:mak] Assignee
	Comment 15 • 7 years ago

(In reply to Dão Gottwald [::dao] from comment #14)
> Hmm, are those really the URIs imagelib will use? Don't they resolve to the
> same URI internally?

I don't think so, the protocol is custom, it fetches the data through its own channels. It's possible the network has a cache for the internal channel pointing to the chrome icon, but I'm not sure.
The image cache uses the image url as key, afaik.

	Stephen Horlander [:shorlander]
	Comment 16 • 7 years ago

(In reply to Marco Bonardo [::mak] from comment #9)
> Well, the difference is that we could show this image thousands of times,
> rather than a few times like most of the other icons, for example in Places
> history views. There's not doubt a simple 16+32 ico file is more efficient
> than svg, both perf and memory-wise. Even if the benefit would be minimal,
> multiplied by thousands it will be interesting.
> 
> Let me ni? Stephen and see what he thinks.

I can try and create an optimized favicon SVG for testing.

	Marco Bonardo [:mak] Assignee
	Comment 17 • 7 years ago

note, there is also bug 1361291 that I'm not yet sure how to solve, that means while redesigning the default favicons, maybe there could be a way to make one that works decently also on a dark background?
That bug only affects the awesomebar, since it's the only place where the inverted icon was used.

	Dão Gottwald [:dao]
	Comment 18 • 7 years ago

(In reply to Marco Bonardo [::mak] from comment #17)
> note, there is also bug 1361291 that I'm not yet sure how to solve, that
> means while redesigning the default favicons, maybe there could be a way to
> make one that works decently also on a dark background?

SVG + context-fill would make that possible

	Marco Bonardo [:mak] Assignee
	Comment 19 • 7 years ago

(In reply to Dão Gottwald [::dao] from comment #18)
> SVG + context-fill would make that possible

How? the ui doesn't know if page-icon is returning a valid favicon or the default favicon.

	Dão Gottwald [:dao]
	Comment 20 • 7 years ago

(In reply to Marco Bonardo [::mak] from comment #19)
> (In reply to Dão Gottwald [::dao] from comment #18)
> > SVG + context-fill would make that possible
> 
> How? the ui doesn't know if page-icon is returning a valid favicon or the
> default favicon.

We could set fill / stroke colors regardless and if the icon doesn't support context-fill/stroke (if I remember correctly, that's the case for all images loaded remotely), or if the image just doesn't use context-fill/stroke, that's not going to do any harm.

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

(In reply to Dão Gottwald [::dao] from comment #20)
> (In reply to Marco Bonardo [::mak] from comment #19)
> > (In reply to Dão Gottwald [::dao] from comment #18)
> > > SVG + context-fill would make that possible
> > 
> > How? the ui doesn't know if page-icon is returning a valid favicon or the
> > default favicon.
> 
> We could set fill / stroke colors regardless and if the icon doesn't support
> context-fill/stroke (if I remember correctly, that's the case for all images
> loaded remotely), or if the image just doesn't use context-fill/stroke,
> that's not going to do any harm.

Of course, it would be simpler if the icon had dark and light portions such that it works on different platforms.

	Dão Gottwald [:dao]
	Comment 22 • 7 years ago

Comment on attachment 8864025 [details]
Bug 1360891 - Places default favicon should supports hi-dpi.

cancelling review request until we know what we want to do here

	Marco Bonardo [:mak] Assignee
	Comment 23 • 7 years ago

Stephen, any news? Do you have an ETA for a possible icon replacement, or should we go with the ico?

	Stephen Horlander [:shorlander]
	Comment 24 • 7 years ago

(In reply to Marco Bonardo [::mak] from comment #23)
> Stephen, any news? Do you have an ETA for a possible icon replacement, or
> should we go with the ico?

Debating if we want to change this icon completely.

In the meantime we can try this one.

	Marco Bonardo [:mak] Assignee
	Comment 25 • 7 years ago

ah um, it's a little bit large (13KB).
As I said this is loaded everywhere and repeatedly, my current ico is 3KB. Now I don't pretend to get a 3KB svg, but I don't think we should use an image 4 times larger in size :(
Is there a way to reduce the number of paths and sort-of optimize it?

	Stephen Horlander [:shorlander]
	Comment 26 • 7 years ago

That is the optimized version :-\

	Dão Gottwald [:dao]
	Comment 27 • 7 years ago

Hmm, it looks super detailed if I zoom in to 300%. This seems like way more detail than we need.

	Stephen Horlander [:shorlander]
	Comment 28 • 7 years ago

More optimized.

	Marco Bonardo [:mak] Assignee
	Comment 29 • 7 years ago

Stephen, have you ever tried https://jakearchibald.github.io/svgomg/ ?
I just tried it on this icon, and with precision 3 it can save a 13,5% maintaining prettyfied code, 14% without it.
the differences are barely noticeable.
Maybe we could use a tool like this one to optimize our svgs?

	Marco Bonardo [:mak] Assignee
	Comment 31 • 7 years ago

NOTE: I still have to test this on Try for tests AND talos. I left the review flagged just in case both of those are fine, but I am not stating this is ready yet.

	Stephen Horlander [:shorlander]
	Comment 32 • 7 years ago

(In reply to Marco Bonardo [::mak] from comment #29)
> Stephen, have you ever tried https://jakearchibald.github.io/svgomg/ ?
> I just tried it on this icon, and with precision 3 it can save a 13,5%
> maintaining prettyfied code, 14% without it.
> the differences are barely noticeable.
> Maybe we could use a tool like this one to optimize our svgs?

We run our SVGs through SVGO (https://github.com/svg/svgo) with the same level of precision. I would have to dig into why there is a difference.

	Dão Gottwald [:dao]
	Comment 33 • 7 years ago
mozreview-review

Comment on attachment 8864025 [details]
Bug 1360891 - Places default favicon should supports hi-dpi.

https://reviewboard.mozilla.org/r/135736/#review144588

I'm not sure I'm the right reviewer for the toolkit/components/places/ changes...

::: browser/themes/osx/browser.css:1396
(Diff revision 3)
> -    list-style-image: url("chrome://mozapps/skin/places/defaultFavicon@2x.png");
> -  }
> -
>    image.bookmark-item {
>      width: 16px;
>    }

o_O

no idea what image.bookmark-item is supposed to do

	Marco Bonardo [:mak] Assignee
	Comment 35 • 7 years ago

Mark, please have a look at the changes in toolkit/components/places

	Marco Bonardo [:mak] Assignee
	Comment 36 • 7 years ago

New results and good news! The regressions are gone (it was all fault of bug 1274018):
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=979ab95fd1f5&newProject=try&newRevision=884387134bc2&framework=1&showOnlyImportant=0

The only one sticking seems to be tp5o Modified Page List Bytes, but we are indeed using a larger icon, so a small memory hit is something we could expect. I don't think it's a blocking issue, otherwise we'd just keep the whole browser graphics lo-res just to save memory.

	Marco Bonardo [:mak] Assignee
	Comment 38 • 7 years ago

(In reply to Dão Gottwald [::dao] from comment #33)
> ::: browser/themes/osx/browser.css:1396
> (Diff revision 3)
> > -    list-style-image: url("chrome://mozapps/skin/places/defaultFavicon@2x.png");
> > -  }
> > -
> >    image.bookmark-item {
> >      width: 16px;
> >    }
> 
> o_O
> 
> no idea what image.bookmark-item is supposed to do

I removed that, it was added long time ago without any reason from what I could get through blame.

	Mark Banner (:standard8)
	Comment 39 • 7 years ago
mozreview-review

Comment on attachment 8864025 [details]
Bug 1360891 - Places default favicon should supports hi-dpi.

https://reviewboard.mozilla.org/r/135736/#review145126

Looks good, just a couple of minor comments for the test. r=Standard8

::: toolkit/components/places/tests/favicons/test_moz-anno_favicon_mime_type.js:33
(Diff revision 4)
>      aRequest.cancel(Cr.NS_ERROR_ABORT);
>      throw Cr.NS_ERROR_ABORT;
>    }
>  };
>  
> -// Test Runner
> +add_task(function* () {

Maybe make this `async function` & use await as you're here?

::: toolkit/components/places/tests/favicons/test_moz-anno_favicon_mime_type.js:55
(Diff revision 4)
>    });
> -  channel.asyncOpen2(new streamListener("image/png"));
> -  do_test_pending();
> +  listener = new streamListener(PlacesUtils.favicons.defaultFaviconMimeType);
> +  channel.asyncOpen2(listener);
> +  yield listener.done.promise;
>  
> -  // Test that the content type of a favicon we add ends up being image/png.
> +  do_print("Test that the content type of a favicon we add is correct.");

This feels like the start of another test - so might be nice for a separate add_task and give both of them function names.

	Mark Banner (:standard8)
	Comment 40 • 7 years ago
mozreview-review

Comment on attachment 8864025 [details]
Bug 1360891 - Places default favicon should supports hi-dpi.

https://reviewboard.mozilla.org/r/135736/#review145138

	Marco Bonardo [:mak] Assignee
	Comment 41 • 7 years ago
mozreview-review-reply

Comment on attachment 8864025 [details]
Bug 1360891 - Places default favicon should supports hi-dpi.

https://reviewboard.mozilla.org/r/135736/#review145126

> Maybe make this `async function` & use await as you're here?

Yeah, I had done this part before the giant Task->async rewrite :(

	Marco Bonardo [:mak] Assignee
	Comment 43 • 7 years ago

let's mark this as addon-compat, in case some theme/addon refers directly to the old png.

	Pulsebot
	Comment 44 • 7 years ago

Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/73276effe1f6
Places default favicon should supports hi-dpi. r=dao,standard8

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 45 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/73276effe1f6