Closed Bug 892632 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox for Metro Graveyard :: Firefox Start, defect, P2)

x86
Windows 8.1
defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: ywang, Assigned: sfoster)

References

Details

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

Attachments

(2 files, 1 obsolete file)

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
Whiteboard: feature=defect c=tbd u=tbd p=0
Priority: -- → P2
Assignee: nobody → sfoster
Blocks: metrov1it11
No longer blocks: metrov1defect&change
Status: NEW → ASSIGNED
QA Contact: jbecerra
Whiteboard: feature=defect c=tbd u=tbd p=0 → feature=defect c=tbd u=tbd p=1
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)
(previous was missing the View.jsm)
Attachment #777500 - Attachment is obsolete: true
Attachment #777500 - Flags: review?(mbrubeck)
Attachment #777504 - Flags: review?(mbrubeck)
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+
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
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
https://hg.mozilla.org/mozilla-central/rev/d08889089d68
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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.
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.