The root domain icon is preferred if it has an higher resolution than the local page favicon

NEW
Unassigned

Status

()

Toolkit
Places
P3
major
8 months ago
a month ago

People

(Reporter: Virtual, Unassigned)

Tracking

({nightly-community, reproducible, ux-consistency})

55 Branch
x86_64
Windows 7
nightly-community, reproducible, ux-consistency
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox-esr52 unaffected, firefox53 unaffected, firefox54 unaffected, firefox55 wontfix, firefox56 wontfix, firefox57 wontfix, firefox58 affected, firefox59 affected)

Details

(Whiteboard: [fxsearch])

Attachments

(4 attachments)

STR:
1. Open this website page:
- http://amvnews.ru/
2. Bookmark it
3. See that wrong favicon was saved in bookmark



Tested with Mozilla Firefox Nightly 55.0a1 (2017-04-25) (64-bit) [Portable] with clean new fresh profile without any addons (extensions, plugins, themes, etc.)
Works fine with Mozilla Firefox 53 (64-bit) [Portable] with clean new fresh profile without any addons (extensions, plugins, themes, etc.)



"Speedy" Regression window (mozilla-central)
Good:
https://ftp.mozilla.org/pub/firefox/nightly/2017/04/2017-04-12-03-02-52-mozilla-central/

Bad:
https://ftp.mozilla.org/pub/firefox/nightly/2017/04/2017-04-13-03-02-27-mozilla-central/

Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f40e24f40b4c4556944c762d4764eace261297f5&tochange=819a666afddc804b6099ee1b3cff3a0fdf35ec15

Probably caused by:
d73a295b3652	Marco Bonardo — Bug 977177 - Invalidate the page-icon image cache when necessary. r=adw
42df4b3da073	Marco Bonardo — Bug 977177 - Don't expire root domain icons with history, and don't associate pages to them. r=adw
5311e5ac3b4b	Marco Bonardo — Bug 977177 - Add favicons.sqlite to profile related lists. r=adw,jmaher
864e72c60156	Marco Bonardo — Bug 977177 - Split ico files into native frames. r=adw
62f3fc3cb351	Marco Bonardo — bug 977177 - Fallback to the root domain icon. r=adw
60002894a42b	Marco Bonardo — Bug 977177 - Expire old page to icon relations to avoid serving deprecated icons. r=adw
4a0770d810dc	Marco Bonardo — Bug 977177 - Add size ref fragment to icon protocols. r=adw
90d755bcbb92	Marco Bonardo — Bug 977177 - Update favicons API consumers. r=adw
942aa1533e08	Marco Bonardo — Bug 977177 - Move favicons to a separate store. r=adw
Flags: needinfo?(mak77)
Summary: Wrong Favicons are saved in bookmarks on some website pages after landing patches from bug #977177 → Wrong favicons are saved in bookmarks on some website pages after landing patches from bug #977177
Has Regression Range: --- → yes
Has STR: --- → yes
Whiteboard: [fxsearch]
Hm, I cannot reproduce this, I see a blue gradient with AMV text on it. both on the tab and in the bookmark.

How did you add the bookmark? Because IIRC there's an old bug where a new bookmark gets the wrong icon when created through the "New bookmark" contextual menu, but it's only visual since opening a new window shows the right icon.
Flags: needinfo?(mak77) → needinfo?(Virtual)
Created attachment 8862335 [details]
1.png
Flags: needinfo?(Virtual)
Created attachment 8862336 [details]
2.png
Created attachment 8862337 [details]
3.png
Looks like I missed some steps.

STR:
1. Open this URL - http://amvnews.ru/ (See Attachments in Comment #2)
2. Bookmark it (See Attachments in Comment #2)
3. Open HTTPS version of that website page - https://amvnews.ru/ [unfortunately there's certificate error and site isn't configured in HTTPS] (See Attachments in Comment #3)
you can bookmark it or not, but it's not needed to reproduce it, only visit is needed
4. go back to http://amvnews.ru/ and see that wrong favicon (from HTTPS) is saved in bookmark (in HTTP) (See Attachments in Comment #4)
Ah, I see. The problem is that the invalid https site defines a root icon (at https://amvnews.ru/favicon.ico) and the new store considers that valid also for other pages under amvnews.ru. Moreover the root icon has better resolution than the local one.
Fixing the website should be trivial (and the new icon would appear a week later, when the previous one is expired), while knowing if an icon is valid or something that a webmaster forgot to update/remove is not, we just can't know.

It's possible bug 1347532 will help here, since given a size, we'll prefer the site defined icon to a same-size root icon. What we do currently, without a size ref, is just picking the biggest icon out of the bunch.

Another possibility would be to change FetchIconPerSpec so that it always prefers local icons. There is one disadvantage to that though, if a page has a nice hi-res root domain icon, and a lo-res local icon, we'd pick the lo-res one, just because it's local to that specific url.

I'm calling this a P3, since both approach have pro and cons and it's unclear which of the 2 would work better.
We'll collect more feedback about that and see. This is something we can change at any time, we'll still store the icons, the only thing to touch is the selection logic.

I'm also not calling this a regression, since the new logic was deliberate, we could change it, but it would still be a conscious decision, not a mistake or an overlooked case.
Depends on: 1347532
Keywords: regression
Priority: -- → P3
[Tracking Requested - why for this release]: Regression



Hmm, it's the regression for sure from user point of view (at last mine ;p ),
as proper HTTP favicon is only fetched and downloaded, but not saved to bookmarked HTTP website page, only after visiting the HTTPS version, if you won't visit HTTPS version, everything is fine.

Please see Attachment in Comment #8.
tracking-firefox55: --- → ?
Keywords: regression
Created attachment 8862357 [details]
bug.png
Please don't override the decisions of the module owner. You're clearly free to confute and discuss them.
We took a "precision hit" to get a tangible performance and resolution advantage. The situation will improve with further work, as I said, but it was a deliberate choice and that work is not planned for 55 at the moment.
An expected behavioral change is not a regression, it's a change. For example moving the sidebar to the right is a choice, not a regression, then it could be a right or a wrong choice, no doubt.
Thank you for your help.
tracking-firefox55: ? → ---
Keywords: regression
(In reply to Marco Bonardo [::mak] from comment #9)
> We took a "precision hit" to get a tangible performance and resolution
> advantage. The situation will improve with further work, as I said, but it
> was a deliberate choice and that work is not planned for 55 at the moment.

Awesome!
Thank you very much for your very hard work!
Especially on bugs dependent on bug #1356525, which fixed loosing favicons after migrating to Firefox 55 from Firefox 54.
Changing blocking bugs, this is not an indicator of a bug, but a deliberate behavioral change, we'll evaluate it in the more global hi res favicons context, than a single regression bug about broken favicons storage.
Blocks: 492172
No longer blocks: 1356525
status-firefox56: --- → affected
QA Contact: Virtual
status-firefox57: --- → affected
status-firefox58: --- → affected
No longer blocks: 492172
Duplicate of this bug: 1414517
Summary: Wrong favicons are saved in bookmarks on some website pages after landing patches from bug #977177 → The root domain icon is preferred if it has an higher resolution than the local page favicon
Duplicate of this bug: 1394115
status-firefox55: affected → wontfix
status-firefox56: affected → wontfix
status-firefox57: affected → wontfix
status-firefox59: --- → affected

Comment 14

a month ago
Problem I am having is with localhost XAMPP icons getting preference over custom icons used for sites.

If the C:\xampp\htdocs\favicon.ico is higher resolution than the custom icon the custom one is ignored.
One winds up with all localhost bookmarks being the Xampp icon rather than the icon specified by the site.

The icon on the tab remains correct.
You need to log in before you can comment on or make changes to this bug.