Defect - History tiles don't show colors on the ribbon

VERIFIED FIXED

Status

Firefox for Metro
Firefox Start
P2
normal
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: Yuan, Assigned: sfoster)

Tracking

Trunk
x86
Windows 8.1

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: feature=defect c=tbd u=tbd p=1)

Attachments

(2 attachments, 1 obsolete attachment)

Noticed that the new ribbon of history tiles doesn't display colors. 

I tested the same link for top sites, bookmarks, and history. The tile is displayed with site color on Top Sites and Bookmarks, but on History the tile has no color on the ribbon.

See a screenshot
https://www.evernote.com/shard/s153/sh/aead6f2d-62d5-4f53-b9f8-d91580df6ed9/d67bbf738b064f07f7231409b6c8990d

Updated

4 years ago
Blocks: 859003
Whiteboard: feature=defect c=tbd u=tbd p=0

Updated

4 years ago
Priority: -- → P2

Updated

4 years ago
Assignee: nobody → sfoster
Blocks: 886790
No longer blocks: 859003
Status: NEW → ASSIGNED
QA Contact: jbecerra
Whiteboard: feature=defect c=tbd u=tbd p=0 → feature=defect c=tbd u=tbd p=1
(Assignee)

Comment 1

4 years ago
Created attachment 777500 [details] [diff] [review]
Hook up tile ribbon color in History tiles via new shared View module; refactor TopSitesView and BookmarksView to use same

First pass at the shared methods for our *Views in a Views.jsm module, and how it falls out with bookmarks, history and topsites. Along the way its a 1 line fix to hookup history tiles with the colored ribbon.
Attachment #777500 - Flags: review?(mbrubeck)
(Assignee)

Comment 2

4 years ago
Created attachment 777504 [details] [diff] [review]
Hook up tile ribbon color in History tiles via new shared View module; refactor TopSitesView and BookmarksView to use same

(previous was missing the View.jsm)
Attachment #777500 - Attachment is obsolete: true
Attachment #777500 - Flags: review?(mbrubeck)
Attachment #777504 - Flags: review?(mbrubeck)
(Assignee)

Comment 3

4 years ago
Yes, we could do e.g. 

  BookmarksView.prototype = new View;
  Util.extend(BookmarksView.prototype, {
    // all the stuff for bookmarks
  });

.. which is perhaps simpler and more idiomatic? I chose not to be clear we're not currently using the View's constructor, just building on its prototype. Should we need to share setup logic we can change this pattern.
Comment on attachment 777504 [details] [diff] [review]
Hook up tile ribbon color in History tiles via new shared View module; refactor TopSitesView and BookmarksView to use same

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

Nice!

::: browser/metro/base/content/history.js
@@ +98,5 @@
>    addItemToSet: function addItemToSet(aURI, aTitle, aIcon, aPos) {
>      let item = this._set.insertItemAt(aPos || 0, aTitle, aURI, this._inBatch);
>      item.setAttribute("iconURI", aIcon);
>      this._setContextActions(item);
> +    this._updateFavicon(item, aURI);

Since we already have an icon URI from the database query, it seems like wasted work to query the favicon service.  Should we just call _gotIcon instead?

::: browser/metro/modules/View.jsm
@@ +9,5 @@
> +Components.utils.import("resource:///modules/colorUtils.jsm");
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +
> +// const ColorAnalyzer = Components.classes["@mozilla.org/places/colorAnalyzer;1"]
> +//                 .getService(Components.interfaces.mozIColorAnalyzer);

looks like this can be removed?

@@ +17,5 @@
> +// module helpers
> +//
> +
> +function makeURI(aURL, aOriginCharset, aBaseURI) {
> +return Services.io.newURI(aURL, aOriginCharset, aBaseURI);

nit: indentation
Attachment #777504 - Flags: review?(mbrubeck) → review+
(Assignee)

Comment 5

4 years ago
I spent some time trying to make use of the icon URI we get from the database. And I've not successfully pinned down why it appears not to work. After digging around in the favicon service, I'm realizing I was probably looking in the wrong place and that the problem may lie in the ImageAnalyzer, which simply creates an image in a hidden document, assigns the url to the .src and waits for the load event. This is known to be flaky (out in the wild at least) when dealing with possibly-cached images. 

So, while this should totally be doable, and in fact we should avoid loading the favicon off the web from the tile whenever possible (might cause auth challenges and other side-effects) I want to punt to a follow-up ticket. Nice thing is that logic is all encapsulated in the View shared methods now. 

The other comments are addressed, so meanwhile, on inbound: 
https://hg.mozilla.org/integration/mozilla-inbound/rev/d08889089d68
(Assignee)

Comment 6

4 years ago
Follow-up filed on the caching issue: https://bugzilla.mozilla.org/show_bug.cgi?id=896135
.. This one is good to go when it merges to m-c

Comment 7

4 years ago
https://hg.mozilla.org/mozilla-central/rev/d08889089d68
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Created attachment 782577 [details]
screenshot_Nightly_2013-07-28

Mozilla/5.0 (Windows NT 6.2; rv:25.0) Gecko/20130728 Firefox/25.0

Verified as fixed on the latest Nightly build: Top Sites, Bookmarks, and History all have the same color on the ribbon.

Please see the attached screenshot for details.

Updated

4 years ago
Status: RESOLVED → VERIFIED
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.