951396 - Bookmarks do not display retina grade .ico favicon in bookmarks toolbar

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Reto Haeberli]

Attachment: nr_bookmark.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20131217084757

Steps to reproduce:

Surf to a website that actually supports a retina grade favicon (i.e. http://www.heise.de/) and bookmark it (note, the retina icon is correctly displayed in the tab).


Actual results:

The bookmark in the bookmarks toolbar does either contain no icon or the standard resolution icon instead of the retina grade one which is obviously available as it can be shown as the icon of the tab.


Expected results:

I would expect to see the retina favicon in the bookmark in the bookmarks toolbar.

Comment 1

[Jona L]

Attachment: Comparison between bookmark and tab favicons

Comment 2

[Marco Bonardo [:mak]]

the current favicon service only supports 16x16 favicons, there's a longstanding bug about that, we should add support for larger icons.
Not duping though cause it makes sense to have a bug filed to use double sized icons for retina.

Comment 3

[Drew Willcoxon :adw]

All you have to do is append #-moz-resolution=32,32 to the image URIs we're already using, like what the search bar and tabs do.  You don't actually need larger icons.  It's magic!

http://mxr.mozilla.org/mozilla-central/source/browser/components/search/content/search.xml?rev=1061661a1119#296
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml?rev=20f9b816ebcd#834

Comment 4

[brianhtong]

(In reply to Drew Willcoxon :adw from comment #3)
> All you have to do is append #-moz-resolution=32,32 to the image URIs we're
> already using, like what the search bar and tabs do.  You don't actually
> need larger icons.  It's magic!
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/components/search/
> content/search.xml?rev=1061661a1119#296
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> tabbrowser.xml?rev=20f9b816ebcd#834

Hi Drew! I'm dealing with this bug/issue right now but I have no idea to do what you described above. I pressed the links but I couldn't edit or add text to it. Could you give me a really detailed instructions list on how to do what you did? It'd mean a lot, thanks!

Comment 5

[Drew Willcoxon :adw]

Hi Brian, the comment I posted is a way for Firefox developers to fix the bug, not a way for users to fix it themselves.  Or are you interested in writing a patch to fix it?

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Drew/Matt, do you guys know whether the -moz-resolution solution will also fix things for Windows HiDPI?

And that solution also relies on the ico files actually including sizes larger than 16x16, right?

Comment 7

[Matthew N. [:MattN]]

(In reply to :Gavin Sharp (email [e-mail]) from comment #6)
> Drew/Matt, do you guys know whether the -moz-resolution solution will also
> fix things for Windows HiDPI?

I don't see why it wouldn't.

> And that solution also relies on the ico files actually including sizes
> larger than 16x16, right?

Yes. The favicon service would need to store the correct size icons for non-ICO which it doesn't really do properly now.

Relatedly, we may want to double-check we're specifying the same image scaling option that we're using in the address bar.

Comment 8

[Matthew N. [:MattN]]

This is not as easy as it looked from the outside since the bookmark favicons use "moz-anno:favicon:" (e.g. moz-anno:favicon:http://www.example.com/favicon.ico) and the fragment would get passed to the DB query causing it to not find the icon.

Bernardo will start working from the backlog on his project regarding favicon improvements so I've set the iteration fields.

Comment 9

[Matthew N. [:MattN]]

This bug is just going to deal with the -moz-resolution .ico solution.

Comment 10

[Marco Mucci [:MarcoM]]

(In reply to Matthew N. [:MattN] (behind on bugmail) from comment #8)
> This is not as easy as it looked from the outside since the bookmark
> favicons use "moz-anno:favicon:" (e.g.
> moz-anno:favicon:http://www.example.com/favicon.ico) and the fragment would
> get passed to the DB query causing it to not find the icon.
> 
> Bernardo will start working from the backlog on his project regarding
> favicon improvements so I've set the iteration fields.

Hi Matthew, we'll add the Iteration ID when the bug is marked as resolved.

Comment 11

[Bernardo Rittmeyer [:rittme]]

Attachment: Work in progress

This is a work in progress solution for this bug.
The favicons are shown in high resolution, but I think this can be made in a smarter way, adding the #moz-resolution at a lower level and not repeating the same procedure 4 times.

Comment 12

[Bernardo Rittmeyer [:rittme]]

Attachment: .ico file with 16px and 32px resolutions for testing

This .ico was a useful resource for testing the resolutions at the bookmarks toolbar. It could also be useful for tests elsewhere.

Comment 13

[Bernardo Rittmeyer [:rittme]]

Attachment: rev 1 - Adds a helper function to PlacesUIUtils.jsm for adding the #-moz-resolution fragment to urls and uses it at the bookmarks toolbar

This patch adds the #-moz-resolution fragment to the favicon urls at the bookmarks toolbar.
I have created a new helper function at the PlacesUIUtils.jsm file, that can be reused in another bugs relative to favicon resolutions in Places.

Comment 14

[Matthew N. [:MattN]]

Comment on attachment 8463656 [details] [diff] [review]
rev 1 - Adds a helper function to PlacesUIUtils.jsm for adding the #-moz-resolution fragment to urls and uses it at the bookmarks toolbar

Review of attachment 8463656 [details] [diff] [review]:
-----------------------------------------------------------------

browserPlacesViews.js looks fine

::: browser/components/places/PlacesUIUtils.jsm
@@ +1015,5 @@
>             Weave.Service.engineManager.get("tabs").enabled;
>    },
> +
> +  /**
> +   * Returns the passed URL with a #moz-resolution fragment

…for the specified dimensions and devicePixelRatio.

@@ +1019,5 @@
> +   * Returns the passed URL with a #moz-resolution fragment
> +   *
> +   * @param aURL
> +   *        The URL where we should add the fragment
> +   *

You're missing aWindow

@@ +1022,5 @@
> +   *        The URL where we should add the fragment
> +   *
> +   * @return The URL with the fragment at the end
> +   */
> +  getURLWithResolution: function PUIU_getURLWithResolution(aURL, aWindow) {

I think the function name should refer to images/icons instead of any URL. Maybe getImageURLForResolution(aWindow, aURL, aWidth = 16, aHeight = 16)

Also, I put window first since we tend to put it first for JSMs (c.f. UITour.jsm)

@@ +1023,5 @@
> +   *
> +   * @return The URL with the fragment at the end
> +   */
> +  getURLWithResolution: function PUIU_getURLWithResolution(aURL, aWindow) {
> +    let size = Math.round(16 * aWindow.devicePixelRatio);

I think the sizes should be passed as an argument (defaulting to 16) as we may want to use this in places for 32x32 (e.g. in the awesome bar [bug 768703])

::: toolkit/components/places/Helpers.cpp
@@ +61,5 @@
>  }
>  
>  #define URI_TO_URLCSTRING(uri, spec) \
>    nsAutoCString spec; \
> +  if (NS_FAILED(aURI->GetSpecIgnoringRef(spec))) { \

Add a comment explaining why we're ignoring the ref. I just realized that some consumers of this macro may want the ref. Have you checked that?

Comment 15

[Bernardo Rittmeyer [:rittme]]

Attachment: rev 2 - Adds a helper function to PlacesUIUtils.jsm for adding the #-moz-resolution fragment to urls and uses it at the bookmarks toolbar

I fixed the problems you mentioned.
That macro was being used by other consumers that needed the ref, so that would not work. 
In this revision I do the url manipulations inside the nsAnnoProtocolHandler::ParseAnnoURI method, that is used only to load favicons.

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 8463805 [details] [diff] [review]
rev 2 - Adds a helper function to PlacesUIUtils.jsm for adding the #-moz-resolution fragment to urls and uses it at the bookmarks toolbar

Review of attachment 8463805 [details] [diff] [review]:
-----------------------------------------------------------------

Please also flag :mak for review on the next version.

::: browser/components/places/PlacesUIUtils.jsm
@@ +1033,5 @@
> +   *        The target image height
> +   *
> +   * @return The URL with the fragment at the end 
> +   */
> +  getImageURLForResolution: 

Nit: trailing whitespace

::: toolkit/components/places/nsAnnoProtocolHandler.cpp
@@ +293,5 @@
>  //    Splits an annotation URL into its URI and name parts
>  
>  nsresult
>  nsAnnoProtocolHandler::ParseAnnoURI(nsIURI* aURI,
>                                      nsIURI** aResultURI, nsCString& aName)

Will "aName" be "favicon" in the cases you care about? I think you should only ignore the ref only when it's for moz-anno:favicon and not for all moz-anno:

Comment 17

[Bernardo Rittmeyer [:rittme]]

Attachment: rev 3 - Adds a helper function to PlacesUIUtils.jsm for adding the #-moz-resolution fragment to urls and uses it at the bookmarks toolbar

Even though for now the nsAnnoProtocolHandler class only deals with favicons, I moved the "ref cleaning" to the nsAnnoProtocolHandler::NewFaviconChannel method, which deals exclusively with favicons. This should make this change more future-proof.

Comment 18

[Bernardo Rittmeyer [:rittme]]

Attachment: rev 4 - Adds a helper function to PlacesUIUtils.jsm for adding the #-moz-resolution fragment to urls and uses it at the bookmarks toolbar

Please disregard the previous patch, as it had memory leaks. This one should fix it.

Comment 19

[Bernardo Rittmeyer [:rittme]]

Attachment: rev 5 - Adds a helper function to PlacesUIUtils.jsm for adding the #-moz-resolution fragment to urls and uses it at the bookmarks toolbar

Sorry for yet another revision, but I think this should work better.
I just moved the ref cleaning to the nsFaviconService class, and I make it just before querying the database. With this approach there is no need of cloning the URI object.

Comment 20

[Matthew N. [:MattN]]

Comment on attachment 8464283 [details] [diff] [review]
rev 5 - Adds a helper function to PlacesUIUtils.jsm for adding the #-moz-resolution fragment to urls and uses it at the bookmarks toolbar

Review of attachment 8464283 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the JS

Comment 21

[Marco Bonardo [:mak]]

Comment on attachment 8464283 [details] [diff] [review]
rev 5 - Adds a helper function to PlacesUIUtils.jsm for adding the #-moz-resolution fragment to urls and uses it at the bookmarks toolbar

Review of attachment 8464283 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/nsFaviconService.cpp
@@ +579,5 @@
>    );
>    NS_ENSURE_STATE(stmt);
>  
> +  // We need to get the URI spec without the ref part,
> +  // so we can query the database for the real icon uri.

I'd be happier if the comment explained why we ignore the ref part... it should explicitly speak about our moz-resolution usage.

Comment 22

[Bernardo Rittmeyer [:rittme]]

Attachment: rev 5 - Adds a helper function to PlacesUIUtils.jsm for adding the #-moz-resolution fragment to urls and uses it at the bookmarks toolbar. r=MattN r=mak

Would this comment be better?

Comment 23

[Matthew N. [:MattN]]

Attachment: Test page for the ICO favicon

Comment 24

[Bernardo Rittmeyer [:rittme]]

looks good on try server: https://tbpl.mozilla.org/?tree=Try&rev=4c825201ca4b

Comment 25

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/8301ecf9f05f

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8301ecf9f05f

Comment 27

[Andrei Vaida [:avaida]]

Unfortunately, I currently don't have a Mac with retina display available for testing. 

Juan, could you please take a look at this instead?

Comment 28

[juan becerra [:juanb]]

Verified on latest nightly using 13" MB with Retina display using the attachment in comment #23 to add a bookmark and comparing the icon displayed the bookmarks toolbar. On a Retina display it now shows an icon with "32" and in a non-retina display it shows a "16." The submenus also show the correct icon.

Comment 29

[Hansol Kim (zvuc)]

Attachment: 36.0a2 (2015-01-07) screenshot of the bug as seen on retina display

Comment 30

[Hansol Kim (zvuc)]

Is this bug fixed? I'm on latest DevEd 36.0a2 (2015-01-07) build and the icons are still inconsistent. (see screenshot above) I've included one odd case of icon being correctly rendered 2x, which happens to be just a 32x32 px size PNG image file named as an .ico file. The left favicon.ico actually has proper standard 64x64, 32x32, 24x24, 16x16 sizes inside the ico file and is still displayed non-retina on the bookmarks bar (while correctly displayed retina-grade on the tab bar.)

Comment 31

[Marco Bonardo [:mak]]

(In reply to Hansol Kim (zvuc) from comment #30)
> Is this bug fixed? I'm on latest DevEd 36.0a2 (2015-01-07) build and the
> icons are still inconsistent.

this bug added the needed glue to use bigger icons, but some icons are still stored in a scaled version. see bug 798223.

Comment 32

[Reto Haeberli]

To my understanding this bug is still unfixed as of today 1st April 2015, Firefox V37.0
Retina Icons are still not displayed correctly in the Bookmarks Toolbar.

Comment 33

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

reto, can you report the problem in bug 798223? It looks like that is where the activity should be.

Comment 34

[Reto Haeberli]

Done, thanks.