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

Status: VERIFIED FIXED

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

Description

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

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

Comment 1

[Sam Foster [:sfoster] (he/him)]

Attachment: 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.

Comment 2

[Sam Foster [:sfoster] (he/him)]

Attachment: 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)

Comment 3

[Sam Foster [:sfoster] (he/him)]

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 4

[Matt Brubeck (:mbrubeck)]

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

Comment 5

[Sam Foster [:sfoster] (he/him)]

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

Comment 6

[Sam Foster [:sfoster] (he/him)]

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

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d08889089d68

Comment 8

[Manuela Muntean [Away]]

Attachment: 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.