Closed Bug 1052174 Opened 6 years ago Closed 6 years ago

Use HiDPI ICO favicons throughout the UI

Categories

(Firefox :: General, defect)

defect
Not set
Points:
8

Tracking

()

VERIFIED FIXED
Firefox 34
Iteration:
34.3
Tracking Status
firefox34 --- verified

People

(Reporter: rittme, Assigned: rittme)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
Flags: firefox-backlog?
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.
Status: NEW → ASSIGNED
Iteration: --- → 34.2
Points: --- → 8
Flags: firefox-backlog? → firefox-backlog+
Summary: Apply HiDPI favicons throughout the UI → Use HiDPI ICO favicons throughout the UI
Added to iteration 34.2  Bernardo, can you mark this bug as [qa+] or [qa-] for verification.
QA Whiteboard: [qa?]
Flags: needinfo?(bernardo)
QA Whiteboard: [qa?] → [qa+]
Flags: needinfo?(bernardo)
Juan, is this something you might be able to take as QA contact to verify once it's fixed? Thanks!
Flags: needinfo?(jbecerra)
QA Contact: jbecerra
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
Attachment #8473179 - Flags: review?(MattN+bmo)
Blocks: 1054712
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.
Attachment #8473179 - Flags: review?(MattN+bmo) → review+
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.
Attachment #8473179 - Attachment is obsolete: true
Attachment #8474448 - Flags: review+
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?
https://hg.mozilla.org/mozilla-central/rev/5e615c0e6b30
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
Iteration: 34.2 → 34.3
QA Whiteboard: [qa+]
Flags: qe-verify+
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.
Flags: needinfo?(jbecerra)
Blocks: 1055879
Thank you Juan. I have not noticed this dialog. I filed Bug 1055879 as a follow-up to fix that.
Seems like there is no more QA work needed here. Marking as Verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.