Closed Bug 951396 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Bookmarks & History, defect)

29 Branch
defect
Not set
Points:
5

Tracking

()

VERIFIED FIXED
Firefox 34
Iteration:
34.3

People

(Reporter: reto.haeberli, Assigned: rittme, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 6 obsolete files)

4.45 KB, image/png
Details
4.06 KB, image/png
Details
745 bytes, image/x-icon
Details
6.45 KB, patch
rittme
: review+
Details | Diff | Splinter Review
265 bytes, text/html
Details
44.81 KB, image/png
Details
Attached image 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.
Component: Untriaged → Bookmarks & History
Hardware: x86 → x86_64
Summary: Bookmarks do not display retina grade favicon (available in tab) → Bookmarks do not display retina grade favicon (available in tab) in bookmarks toolbar
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: p=0
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
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
Whiteboard: p=0 → p=0 [good first bug]
(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!
Flags: needinfo?(reto.haeberli)
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?
Flags: needinfo?(reto.haeberli)
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?
Flags: needinfo?(adw)
Flags: needinfo?(MattN+bmo)
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) 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.
Flags: needinfo?(adw)
Flags: needinfo?(MattN+bmo)
Assignee: nobody → bernardo
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.
Status: NEW → ASSIGNED
Iteration: --- → 33.3
Points: --- → 5
QA Whiteboard: [qa+]
OS: Mac OS X → All
Hardware: x86_64 → All
Whiteboard: p=0 [good first bug]
This bug is just going to deal with the -moz-resolution .ico solution.
Summary: Bookmarks do not display retina grade favicon (available in tab) in bookmarks toolbar → Bookmarks do not display retina grade .ico favicon in bookmarks toolbar
(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.
Iteration: 33.3 → ---
Iteration: --- → 34.1
Attached patch Work in progress (obsolete) — Splinter Review
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.
This .ico was a useful resource for testing the resolutions at the bookmarks toolbar. It could also be useful for tests elsewhere.
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.
Attachment #8462705 - Attachment is obsolete: true
Attachment #8463656 - Flags: review?(MattN+bmo)
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?
Attachment #8463656 - Flags: review?(MattN+bmo) → review-
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.
Attachment #8463656 - Attachment is obsolete: true
Attachment #8463805 - Flags: review?(MattN+bmo)
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:
Attachment #8463805 - Flags: review?(MattN+bmo) → feedback+
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.
Attachment #8463805 - Attachment is obsolete: true
Attachment #8463837 - Flags: review?(mak77)
Please disregard the previous patch, as it had memory leaks. This one should fix it.
Attachment #8463837 - Attachment is obsolete: true
Attachment #8463837 - Flags: review?(mak77)
Attachment #8463841 - Flags: review?(mak77)
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.
Attachment #8463841 - Attachment is obsolete: true
Attachment #8463841 - Flags: review?(mak77)
Attachment #8464283 - Flags: review?(mak77)
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
Attachment #8464283 - Flags: review+
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.
Attachment #8464283 - Flags: review?(mak77) → review+
Attachment #8465124 - Attachment mime type: text/plain → text/html
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8301ecf9f05f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
Unfortunately, I currently don't have a Mac with retina display available for testing. 

Juan, could you please take a look at this instead?
Flags: needinfo?(jbecerra)
QA Contact: andrei.vaida
Iteration: 34.1 → 34.2
QA Contact: andrei.vaida → jbecerra
Iteration: 34.2 → 34.3
QA Whiteboard: [qa+]
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(jbecerra)
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.)
(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.
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.
reto, can you report the problem in bug 798223? It looks like that is where the activity should be.
Flags: needinfo?(reto.haeberli)
Done, thanks.
Flags: needinfo?(reto.haeberli)
You need to log in before you can comment on or make changes to this bug.