784042 - Use mozIColorAnalyzer for autocomplete search result tiles

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: App Bar, defect, P1)

Description

[Brian R. Bondy [:bbondy]]

For autocomplete search result tiles, we should use mozIColorAnalyzer from bug 634139 to determine the tile color.

Comment 2

[Allison Naaktgeboren :ally]

MarcoM, since I know you're going to ask. Doing this right without perf impact: p=8

Comment 3

[Allison Naaktgeboren :ally]

Brain Dump

  Search tiles are generated & destroyed with each keystroke in the urlbar. We want tile generation to be fast and not degrade user interaction with the urlbar. Furthermore, coloranalyzer is asynchronous, so I could not reliably get colors on tile generation.
  Therefore, we should get colors in the background and save them. The colorutils.jsm has grown to include a nifty cache, but it will need to be modified. Entries get erased after a day or when metrofx closes. Since the desktop browser re-writes session store files and this is metro only data, we'll make a new file in the profile/metro folder so desktop leaves it alone. 

Current Plan:
Data source (the color cache)
  On startup
    - pull in profile/metro/faviconcolors.json if it exists
      - load into color cache object
    - spawn async task to go through bookmarks & histroy
      - find new entries & get color info
      - use dumb-but-reliable iteration algo for now
  On new history visit
    - is it possible to bookmark a page w/o visiting it?
      - if it is, I'll have to solve that edgecase
    - kick off a task to get color info and add it to cache
  On shutdown
    - serialize color cache out to session store, profile/metro/faviconcolors.json
      - other files get overwritten by desktop on its new session
      - modeled off crash monitor in bug 872206 where profile/metro was introduced

Applying the colors to the search{history, bookmarks} results grid
  - in urlbar.xml, updateResults() iconURI is supplied
    - use iconURI(.spec possibly) to query colorUtils if there are colors in cache
      - do I want to modify this to not spawn task to get the color if color is not found?
        - concern about too much work
    -if color is found, set item attribute customColor w/value, else do nothing

Modify colorcache
  - max life is 1 day. too short.
  - bookmarks live forever
  - history items live for a long time
  - Questions
    - remove time stamp entirely? modify history removal to get rid of it? 
      - wont handle desktop removal. leak memory
    - set long lived but not NaN to get icons refreshed regularly, colors from removed history items fall out naturally
  

Likely followup bugs
  - smarter algo to find new entries
  - refactor color code for views.jsm. possibly store tint values in the color cache?

Comment 4

[Matthew N. [:MattN]]

A suggestion is to store the result of mozIColorAnalyzer in Places beside the favicon data so it's shared between applications. This was the plan in bug 779027 for desktop's usage.

Comment 5

[Matt Brubeck (:mbrubeck)]

How bad is this with just the current in-memory cache?  If it's jarring seeing the colors appear after a delay, can we fix that with CSS transitions?

If the current caching situation isn't something we can work with, then I'd rather focus on a shared solution integrated with Places (bug 779027), rather than roll our our own in Metro.

Comment 6

[Allison Naaktgeboren :ally]

Attachment: colorSearchResultTiles v0

I'd like to keep the error handler in the patch.

Comment 7

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8383372 [details] [diff] [review]
colorSearchResultTiles v0

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

(In reply to :Ally Naaktgeboren from comment #6)
> I'd like to keep the error handler in the patch.

+1.  We should probably get in the habit of using similar error handlers whenever we write promise-based async code.

Let's see how this feels (especially on low-spec tablets) and file follow-up bugs on performance if needed.

Comment 8

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8383372 [details] [diff] [review]
colorSearchResultTiles v0

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

::: browser/metro/base/content/bindings/urlbar.xml
@@ +736,5 @@
>                item.setAttribute("iconURI", iconURI);
> +              let xpFaviconURI = Services.io.newURI(iconURI.replace("moz-anno:favicon:",""), null, null);
> +              Task.spawn(function() {
> +                Components.utils.import("resource:///modules/colorUtils.jsm");
> +                let colorInfo = yield ColorUtils.getForegroundAndBackgroundIconColors(xpFaviconURI);

I just noticed a subtle race condition while testing this.  Since we reuse richgriditems, it's possible that the item has changed by the time this async call succeeds, which can cause us to apply the color from one icon to a tile with a different icon:

We can fix this by checking if the icon has changed after the "yield" returns:

  if (item.getAttribute("iconURI") != iconURI) {
    return;
  }

Comment 9

[Allison Naaktgeboren :ally]

Attachment: colorSearchResultTiles w/race check

don't have my keys handy.
r+ in the v0 of the patch

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/ebcbf7155214

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/87cb292f3470 for metro-chrome bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=35451525&tree=Fx-Team

Comment 12

[Jim Mathies [:jimm]]

Comment on attachment 8383926 [details] [diff] [review]
colorSearchResultTiles w/race check

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

::: browser/metro/base/content/bindings/urlbar.xml
@@ +735,5 @@
>                item.setAttribute("value", value);
>                item.setAttribute("iconURI", iconURI);
> +              let xpFaviconURI = Services.io.newURI(iconURI.replace("moz-anno:favicon:",""), null, null);
> +              Task.spawn(function() {
> +                Components.utils.import("resource:///modules/colorUtils.jsm");

you should move this to browser-scripts. I ran into this as well with some updater jsms in settings.

Comment 13

[Allison Naaktgeboren :ally]

Attachment: colorSearchResultTiles v2

Once more with feeling!

:jimm, your ability to stay on top of bugmail is impressive. :)

Comment 14

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

Comment on attachment 8384039 [details] [diff] [review]
colorSearchResultTiles v2

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

drive-by: looks good to me. Is the plan to see how it goes and optimize later if that becomes necessary?

Comment 15

[Allison Naaktgeboren :ally]

(In reply to Sam Foster [:sfoster] from comment #14)
> Comment on attachment 8384039 [details] [diff] [review]
> colorSearchResultTiles v2
> 
> Review of attachment 8384039 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> drive-by: looks good to me. Is the plan to see how it goes and optimize
> later if that becomes necessary?

Thanks for the feedback. :)
That's the idea, in particular on low power tablets. :mbrubeck talks about it in comment 7.

Comment 16

[Allison Naaktgeboren :ally]

Once more, with feeling!

 https://hg.mozilla.org/integration/fx-team/rev/435378ccdcbf

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/435378ccdcbf

Comment 18

[Marco Mucci [:MarcoM]]

For testing and verification.  Reopen if any defects found.

Comment 19

[Kamil Jozwiak [:kjozwiak]]

Went through the verification process using the latest Nightly build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-03-06-03-02-01-mozilla-central/

Before going through the verification process, used the following build to document the old behavior:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-03-01-03-02-04-mozilla-central/

- Ensured that the correct color is being used for the tiles that are listed under the autocomplete search
- Ensured that the tile colors from the autocomplete search match the tile colors from Top Sites, Bookmarks and Recent History
- Ensured that the correct tiles are being added into the autocomplete search when typing
- Ensured that the correct tiles are being removed from the autocomplete search while using backspace
- Ensured that you can select the tiles from the autocomplete search
- Ensured that the tiles load without any jank, performance issues or other graphic problems
- Ensured that the autocomplete search field can be used under about:start and when browsing websites
- Ensured that the search buttons under the autocomplete search are still working correctly
- Went through all of the above test cases using several different variations of snapped view

Issues found when going through verification:
- Bug #980667
- Bug #980717