|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
1.14 MB, image/png
2.68 MB, image/png
58 bytes, text/x-review-board-request
|Details | Review|
58 bytes, text/x-review-board-request
|Details | Review|
264.07 KB, image/png
Created attachment 8797803 [details] Screenshot_2016-10-05-10-15-47.png User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2486.0 Safari/537.36 Edge/13.10586 Steps to reproduce: 1.Type Firefox 2.Input www.google.co.nz 3.Tab setting at right button->Page->Add to Home Screen. 4.Go back to home screen and check the created icon. Actual results: This page icon was created with poor quality at home screen. Expected results: This web icon was created withy high quality at home screen.
Not a security bug.
There's an unintentional behavior change introduced by bug 1290014: The previous code bypassed the memory cache and always loaded the icon from disk or performed a download of the icon before adding it to the home screen: https://dxr.mozilla.org/mozilla-beta/source/mobile/android/base/java/org/mozilla/gecko/favicons/Favicons.java#628 The new code does not perform a download and only loads an icon from disk: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#1922
Status: UNCONFIRMED → NEW
Ever confirmed: true
Created attachment 8806598 [details] device-2016-11-02-102606.png I've tried several website and it seems okay. I'll try to reproduce it
I found the issue. The downloaded favicon size for Google is 64*64. while the launcher icon size on Android is 144*144. That's why it is still blur even we've added some inset and background to it. Suggest to use the original size if the favicon is not big enough. PS. since the favicon has the same width/height I'll assume it has only two type of ratio: one is as large as launcher icon, the other is it's original size.
Thanks for the patch. I'll review it later. Did you look at comment 3 too? I think we want to skip memory here (because it may contain a small version), but not disk or network (to load the icon in the original size): https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#1874
Yes, I've read comment 3. Sorry I think I missed the "skip memory" part. I'll update later
Yes, I've read comment 3. Sorry I think I missed the "skip memory" part. I'll update later.
Comment on attachment 8807041 [details] Bug 1307621 - Skip memory because it may contain a small version. https://reviewboard.mozilla.org/r/90322/#review90334
Attachment #8807041 - Flags: review?(s.kaspari) → review+
Comment on attachment 8806627 [details] Bug 1307621 - Use the original size if the downloaded favicon is not big enough. https://reviewboard.mozilla.org/r/90018/#review90466 Looking with Web IDE at the HTML code our app gets, the actual problem seems to be that google.co.nz only serves a single icon to us: http://www.google.co.nz/images/branding/product/ico/googleg_lodp.ico And that's a 32x32 icon. No matter what we do this will always have low resolution.
Attachment #8806627 - Flags: review?(s.kaspari)
Yes. But should we keep the low resolution image as it and put it in the center of the launch image(this is my current approach)? Or we just stretch it (The original approach)? I think the first approach is better because it's the website's owner's choice to provide what resolution of their favicon.
Created attachment 8809361 [details] fallback-launcher-icons.png That's a good question for UX. Antlam what do you think is the best to do here? See screenshot: A) Keep our current fallback. Stretch the icon and draw the colored background. B) New patch: Draw the colored background and draw the icon in original size C) (I think that's what Chrome does): Ignore too small icons and just generate a default icon (colored background + letter, like we do in the UI) Everyone of them is not perfect. Maybe we want to create better icon templates/stencils for fallback icons or somehow generate 'material' launcher icons as fallback.
Comment on attachment 8809361 [details] fallback-launcher-icons.png See comment above.
Additionally, but that's something for a different bug, Chrome seems to take this high resolution image (if available) and then adds it to a a round launcher icon template: <meta content="/images/branding/googleg/1x/googleg_standard_color_128dp.png" itemprop="image">
Ahunt was working on something similar in bug 1254663 and it seems similar enough that I think we should be following the same logic in both places. Ahunt, where did we land on that bug? We can even maybe even use the tippy top pack for ideal situations as well.
Comment on attachment 8809361 [details] fallback-launcher-icons.png Spoke in person, we should be consistent here with the logic we establish in bug 1254663. So I don't want to make a call here yet. Waiting for what our logic is there.
Hi Joe Can you please help decide what icon should be shown here? Cause I think bug 1254663 is not confirmed the solution either. I prefer approach C in Attachment #8809361 [details] cause it looks better. And we should use the same logic for every site. (e.g. google.co.nz for example it'll still use approach C since their favico is 16*16)
agree with :antlam on comment #18 that we should be consistent in our approach base on the discussions in bug 1254663, approach A is out of the question, so that leaves us with B and/or C depending on the size of the favicon. The background for approach B seems a bit off to me but it's really not my expertise so let's have :pekochen to take a look as well
Flags: needinfo?(jcheng) → needinfo?(pchen)
Since there's already PWA work on this, I think we should close this bug. Please feel free to open it again if you think it's still valid.
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.