1052174 - Use HiDPI ICO favicons throughout the UI

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Bernardo Rittmeyer [:rittme]]

There are still many favicons throughout the UI that do not support HiDPI resolutions. We should append the #-moz-resolution fragment to this favicons URIs to get the right sized images, when available.

Comment 1

[Matthew N. [:MattN]]

Bernardo completed the other work for the iteration so this this is the next step in his project: doing the work for bug 951396 throughout more of the UI.

Comment 2

[Marco Mucci [:MarcoM]]

Added to iteration 34.2  Bernardo, can you mark this bug as [qa+] or [qa-] for verification.

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

Juan, is this something you might be able to take as QA contact to verify once it's fixed? Thanks!

Comment 4

[Bernardo Rittmeyer [:rittme]]

Attachment: rev 1 - #-moz-resolution fragment appended to favicon URIs throughout the UI

Favicons now are shown at the right resolution, when available, throughout the UI.

The places affected by this patch are:
 - back/forward navigation dropdown menu
 - awesomebar dropdown
 - bookmark toolbar (moved helper from PlacesUIUtils to PlacesUtils)
 - panel menu history subview
 - sidebars (bookmark/history)
 - places windows (view all history/bookmarks)
 - Preferences > General > Use Bookmark...
 - Tabs Groups window

The test page in this comment can be used for testing: https://bugzilla.mozilla.org/show_bug.cgi?id=951396#c23

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8473179 [details] [diff] [review]
rev 1 - #-moz-resolution fragment appended to favicon URIs throughout the UI

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

r=me with the autocomplete.xml issue resolved

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +158,5 @@
>      onViewShowing: function(aEvent) {
>        // Populate our list of history
>        const kMaxResults = 15;
>        let doc = aEvent.detail.ownerDocument;
> +      let window = doc.defaultView;

Nit: Is "win" or "window" more common in this file?

@@ +203,5 @@
>                  onHistoryVisit(uri, aEvent, item);
>                });
> +              if (icon) {
> +                let iconURL = PlacesUtils.getImageURLForResolution(window, "moz-anno:favicon:" + icon);
> +                item.setAttribute("image",iconURL);

Nit: missing space after the comma

::: browser/components/places/content/treeView.js
@@ +1395,5 @@
>      // Only the title column has an image.
>      if (this._getColumnType(aColumn) != this.COLUMN_TYPE_TITLE)
>        return "";
>  
> +    // If the node is an URI we should get the right resolution favicon

I don't think this comment is necessary and it doesn't add value above what I get from ready the if condition and looking at the function being called.

@@ +1397,5 @@
>        return "";
>  
> +    // If the node is an URI we should get the right resolution favicon
> +    let node = this._getNodeForRow(aRow);
> +    if(PlacesUtils.nodeIsURI(node) && node.icon)

Nit: add a space after |if|

::: browser/components/tabview/favicons.js
@@ +1,4 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +Components.utils.import('resource://gre/modules/PlacesUtils.jsm');

Nit: Add a new line above this

::: toolkit/components/places/PlacesUtils.jsm
@@ +1519,5 @@
>      return deferred.promise;
>    },
>  
>    /**
> +   * Returns the passed URL with a #moz-resolution fragment

missing hyphen: |#-moz-resolution|

@@ +1539,5 @@
> +   * @return The URL with the fragment at the end
> +   */
> +  getImageURLForResolution:
> +  function PU_getImageURLForResolution(aWindow, aURL, aWidth = 16, aHeight = 16) {
> +    if(aURL == "")

Missing space before |(|. We generally would prefer checking if aURL is falsy like |if (!aURL)|. Do any of the callers you're adding actually hit this case? If not, I would rather remove this check so it's more obvious when someone accidentally passes an empty string.

::: toolkit/content/widgets/autocomplete.xml
@@ +1130,5 @@
>  
>              // set these attributes before we set the class
>              // so that we can use them from the constructor
> +            item.setAttribute("image", this.PlacesUtils.getImageURLForResolution(window,
> +              controller.getImageAt(this._currentIndex)));

I think this might break applications which don't build with Places. e.g. Thunderbird. I can think of three ways to handle this:
1) Handle the failed import and don't try calling the function if that happens. This makes the proper HiDPI display depends on Places which seems wrong to me.
2) Create a copy of the helper in this file.
3) Fix this in the controller or lower level data source that is feeding the images and is related to Places.

Unless there is a trivial solution, we may want to do this in a new bug.

Comment 6

[Bernardo Rittmeyer [:rittme]]

Attachment: rev 2 - #-moz-resolution fragment appended to favicon URIs throughout the UI. r=MattN

Thank you for the review, MattN.

I made all the requested fixes.
I created a copy of the helper in the autocomplete.xml file.
Although duplicating code may not seem the best solution for the problem, it's better than the first option for the reasons already mentioned by MattN. Also, moving the logic to a lower level (option 3) would make it harder to have access to the display resolution and could possibly break other parts of the code that are not directly related to rendering.

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8474448 [details] [diff] [review]
rev 2 - #-moz-resolution fragment appended to favicon URIs throughout the UI. r=MattN

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

The commit message is missing the Bug #

::: toolkit/content/widgets/autocomplete.xml
@@ +1139,5 @@
>  
>              // set these attributes before we set the class
>              // so that we can use them from the constructor
> +            item.setAttribute("image", this._getImageURLForResolution(window,
> +              controller.getImageAt(this._currentIndex), 16, 16));

Can you assign the result of _getImageURLForResolution to a temporary variable to fix the indentation?

Comment 8

[Bernardo Rittmeyer [:rittme]]

Attachment: rev 3 - #-moz-resolution fragment appended to favicon URIs throughout the UI. r=MattN

Comment 9

[Matthew N. [:MattN]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=4f79ccac3ed4

https://hg.mozilla.org/integration/fx-team/rev/5e615c0e6b30

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5e615c0e6b30

Comment 11

[juan becerra [:juanb]]

I tested this on 10.9.4 using a 13" MB with Retina display. I checked the items in comment #4, and favicons seem to be displayed consistently.

However, if you compare the list of favicons in the search engine list vs the list of favicons in the "manage search engines" modal dialog they are not the same. The latter ones look a little blurry.

These may have been beyond the scope of this patch, as they are not explicitly called out in comment #4, but these are probably worth fixing.

Comment 12

[Bernardo Rittmeyer [:rittme]]

Thank you Juan. I have not noticed this dialog. I filed Bug 1055879 as a follow-up to fix that.

Comment 13

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Seems like there is no more QA work needed here. Marking as Verified.