Closed Bug 931115 Opened 6 years ago Closed 6 years ago

Defect - Cache dominant color data for start tiles

Categories

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

x86
Windows 8
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 28

People

(Reporter: sfoster, Assigned: sfoster)

References

Details

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

Attachments

(1 file, 2 obsolete files)

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.
Whiteboard: [triage]
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.
Assignee: nobody → sfoster
Attachment #822619 - Flags: review?(mbrubeck)
Blocks: metrov1it17
No longer blocks: metrov1backlog
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Summary: Cache dominant color data for start tiles → Defect - Cache dominant color data for start tiles
Whiteboard: [triage] → feature=defect c=tbd u=tbd p=1
Whiteboard: feature=defect c=tbd u=tbd p=1 → [block28] feature=defect c=tbd u=tbd p=1
Attachment #822619 - Flags: review?(mbrubeck) → review?(jmathies)
Blocks: metrov1it18
No longer blocks: metrov1it17
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?
Attachment #822619 - Flags: review?(jmathies) → review+
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.
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).
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.)
Attachment #822619 - Attachment is obsolete: true
Attachment #824820 - Flags: review?(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

?
Attachment #824820 - Flags: review?(mbrubeck) → review-
* 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
Attachment #824820 - Attachment is obsolete: true
Attachment #825467 - Flags: review?(mbrubeck)
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.
Attachment #825467 - Flags: review?(mbrubeck) → review+
https://hg.mozilla.org/mozilla-central/rev/2dfbe7c5c54e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
Could anyone please give guidelines that can help the QA in verifying this issue? Thanks!
Flags: needinfo?(sfoster)
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.
Flags: needinfo?(sfoster)
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.
You need to log in before you can comment on or make changes to this bug.