931115 - Defect - Cache dominant color data for start tiles

Status: RESOLVED FIXED

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

Description

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

We currently use ColorAnalyzer.findRepresentativeColor (via View.jsm, via colorUtils.jsm) to determine the foreground and background colors to use for the tile labels. This requires fetching the favicon url, and loading that image to examine the pixel colors. 

We should be able to cache this as favicons change infrequently, perhaps using the annotation service, against either the favicon url (many pages share the same favicon) or the page url.

Comment 1

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

Attachment: Cache color info for favicon urls on colorUtils

This patch implements a quick, partial fix to the caching problem by storing the foreground and background color info associated with each favicon on the colorUtils object. It is not persisted anywhere, so we still get that hit on startup. However, the cache is utilized when the start page is loaded or reload thereafter, including when we create a new tab.  
Also, as its common for the same favicon to be used across multiple tiles we only incur the image load and color analysis hit once there. 
I've made no provision for expiring color info if for example a favicon expired from cache during a session, or capping cache's growth. It may become necessary if we notice usage where the browser is never shut down and that colorUtils object sticks around for days.

Comment 2

[Jim Mathies [:jimm]]

Comment on attachment 822619 [details] [diff] [review]
Cache color info for favicon urls on colorUtils

This is a huge improvement on the second load of about:start. The colors are "just there". Should we keep this open though (or maybe file a follow) to address the initial load?

Comment 3

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

As you say this gets us a long way. In general I'm happy to file follow-ups for the initial load / persistence issue as necessary. I guess my only concern is the size of that map object getting out of control over a long session. I'll do a little testing but I assume that memory is frozen and thawed back out on suspend/resume. Which in the windows 8.1 world could easily mean weeks of browsing history. I think I'll just store a timestamp as well so we can easily flush stale data if that becomes necessary.

Comment 4

[Rodrigo Silveira [:rsilveira] [:rodms]]

We could cache favicons only for bookmarks and topsites as they're pretty static. That said, I don't think the size of the map will be a problem and we shouldn't worry about it (yet).

Comment 5

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

Attachment: Cache color info for favicon urls on colorUtils, observe idle-daily to purge cache

After some consideration, irc and 1:1 discussion this patch grew some View.jsm refactoring (stashing a promise in the cache map avoids overlapping duplicate requests for a given page/favicon). 
Also, StartUI now observes the idle-daily topic and triggers a cache purge on that favicon color data. 
Added a test that assures an idle-daily notification does in fact remove stale entries (> 1day) from the cache. 

Not here: The purge is not conditional on whether that favicon is currently in use on the start screen. Not sure if this is a bug or feature - it does allow favicons to be updated at least daily (getting expiry time from response headers for the favicon or db is not really practical with the current favicon/image analysis implementation.)

Comment 6

[Matt Brubeck (:mbrubeck)]

Comment on attachment 824820 [details] [diff] [review]
Cache color info for favicon urls on colorUtils, observe idle-daily to purge cache

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

The caching code looks good; some quibbles about structure:

::: browser/metro/base/content/startui/StartUI.js
@@ +35,5 @@
>  
>      this.chromeWin.addEventListener("MozPrecisePointer", this, true);
>      this.chromeWin.addEventListener("MozImprecisePointer", this, true);
>      Services.obs.addObserver(this, "metro_viewstate_changed", false);
> +    Services.obs.addObserver(this, "idle-daily", false);

I don't like doing this in StartUI.js because it will only work if about:start is open when idle-daily fires.  I think we should do this in colorUtils.jsm, either automatically when the module loads or in an explicit init() function that we call from browser-ui.js.

::: browser/metro/base/tests/mochitest/browser_startUI.js
@@ +16,5 @@
> +      yield addTab("about:start");
> +      yield waitForCondition(() => BrowserUI.isStartTabVisible);
> +
> +      Cu.import("resource:///modules/colorUtils.jsm", fixture);
> +      if (!BrowserUI.isStartTabVisible) {

In what case does isStartTabVisible become false again here?

It seems like some unit tests for colorUtils.jsm might be more useful than a browser-chrome test, especially after the change suggested above.

::: browser/metro/modules/View.jsm
@@ +85,5 @@
> +      if (!(colorInfo && colorInfo.background && colorInfo.foreground)) {
> +        return;
> +      }
> +      let background = colorInfo.background,
> +          foreground = colorInfo.foreground;

If you want to be cute: let { background, foreground } = colorInfo;

::: browser/metro/modules/colorUtils.jsm
@@ +37,5 @@
> +  getForegroundAndBackgroundIconColors: function getForegroundAndBackgroundIconColors(aIconURI) {
> +    let colorKey = aIconURI.spec;
> +    let colorInfo = this._uriColorsMap.get(colorKey);
> +    if (colorInfo) {
> +      return colorInfo;

I'd prefer to return a resolved promise here.  Returning a promise consistently is helpful for callers who aren't using Task.jsm, since they can do things like getForegroundAndBackgroundIconColors(uri).then(whatever).

@@ +144,5 @@
>      );
>    }
>  };
> +
> +// resume_process_notification

?

Comment 7

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

Attachment: Cache color info for favicon urls on colorUtils, observe idle-daily to purge cache

* All cache management now internal to colorUtils.jsm - by observing daily-idle. As View.jsm is the only direct consumer, I've removed all the other imports of colorUtils.jsm

* ColorUtils now has an init method which registers the observers, it can be safely called repeatedly so any module user can assume its the first and call ColorUtils.init()

* Tried making xpcshell unit test for colorUtils.jsm, but it barfs on loading the ColorAnalyzer dependency, so back to a simple mochitest

* Other nits dealt with

Comment 8

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

Oh and following IRC discussion, the cache now stores the promise itself, which is returned by the method. That means all callers can consistently expect a promise as a return value. Doing this means that we avoid redundant requests for identical urls that are already inflight but not yet resolved. This is a common case as its typical to have adjacent history tiles that share the same favicon.

Comment 9

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=de840873564d

Comment 10

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

Try push was green, landed on fx-team: https://hg.mozilla.org/integration/fx-team/rev/2dfbe7c5c54e

Comment 11

[Jim Mathies [:jimm]]

https://hg.mozilla.org/mozilla-central/rev/2dfbe7c5c54e

Comment 12

[Manuela Muntean [Away]]

Could anyone please give guidelines that can help the QA in verifying this issue? Thanks!

Comment 13

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

The first thing is that the colored bars on the start page tiles should appear to come in faster than without the patch applied. This is subjective and dependent on what sites are represented by tiles on the start page, network conditions etc. But the difference should be noticeable with a well-used profile. 

To confirm that caching is actually taking place, you could clear the browser's cache, reload about:start and watch the HTTP requests (e.g. in Fiddler). Each favicon should only be requested once at most, even if multiple tiles on the page use the same favicon.

Comment 14

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.2; rv:28.0) Gecko/20100101 Firefox/28.0
Build ID: 20131120062258

As mentioned in comment 13, I have noticed that the colored bars on the start page tiles appear a little bit faster.
Also, I have installed Fiddler and measured the HTTP requests for each favicon and it is only requested once.