All users were logged out of Bugzilla on October 13th, 2018

Use mozIColorAnalyzer for autocomplete search result tiles

VERIFIED FIXED in Firefox 30

Status

P1
normal
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: bbondy, Assigned: ally)

Tracking

(Depends on: 1 bug)

Trunk
Firefox 30
x86_64
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: p=5 s=it-30c-29a-28b.3 r=ff30)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

Updated

6 years ago
Depends on: 747789

Updated

6 years ago
Blocks: 747789
No longer depends on: 747789

Updated

6 years ago
Component: General → General
Product: Firefox → Firefox for Metro
Version: unspecified → Trunk
Blocks: 747790
No longer blocks: 747789

Updated

6 years ago
Whiteboard: [metro-mvp?] [LOE:?]

Updated

6 years ago
Whiteboard: [metro-mvp?] [LOE:?] → [metro-mvp] [LOE:1]

Updated

6 years ago
Blocks: 831903, 831910

Updated

6 years ago
Whiteboard: [metro-mvp] [LOE:1] → [metro-mvp] [LOE:1] feature=work

Updated

6 years ago
Summary: Use mozIColorAnalyzer for autocomplete search result tiles → Work - Use mozIColorAnalyzer for autocomplete search result tiles
Whiteboard: [metro-mvp] [LOE:1] feature=work → feature=work

Updated

6 years ago
No longer blocks: 831903

Updated

6 years ago
Blocks: 849380

Updated

6 years ago
No longer blocks: 831910

Updated

6 years ago
Component: General → App Bar

Updated

5 years ago
Blocks: 861680
Summary: Work - Use mozIColorAnalyzer for autocomplete search result tiles → Use mozIColorAnalyzer for autocomplete search result tiles
Whiteboard: feature=work → [feature] p=0
(Assignee)

Updated

5 years ago
Assignee: nobody → ally
No longer blocks: 849380
Duplicate of this bug: 849380
(Assignee)

Comment 2

5 years ago
MarcoM, since I know you're going to ask. Doing this right without perf impact: p=8
(Assignee)

Updated

5 years ago
Whiteboard: [feature] p=0 → [feature] p=8
(Assignee)

Comment 3

5 years ago
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?
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.
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.
Depends on: 779027
(Assignee)

Updated

5 years ago
Whiteboard: [feature] p=8 → [feature] p=5
(Assignee)

Comment 6

5 years ago
Created attachment 8383372 [details] [diff] [review]
colorSearchResultTiles v0

I'd like to keep the error handler in the patch.
Attachment #8383372 - Flags: review?(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.
Attachment #8383372 - Flags: review?(mbrubeck) → review+
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;
  }

Updated

5 years ago
Priority: -- → P1

Updated

5 years ago
QA Contact: kamiljoz
Whiteboard: [feature] p=5 → p=5 s=it-30c-29a-28b.2 r=ff30
(Assignee)

Comment 9

5 years ago
Created attachment 8383926 [details] [diff] [review]
colorSearchResultTiles w/race check

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

Updated

5 years ago
Keywords: checkin-needed

Updated

5 years ago
Status: NEW → ASSIGNED
Attachment #8383372 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/ebcbf7155214
Keywords: checkin-needed
Whiteboard: p=5 s=it-30c-29a-28b.2 r=ff30 → p=5 s=it-30c-29a-28b.2 r=ff30[fixed-in-fx-team]
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
Flags: needinfo?(ally)
Whiteboard: p=5 s=it-30c-29a-28b.2 r=ff30[fixed-in-fx-team] → p=5 s=it-30c-29a-28b.2 r=ff30

Comment 12

5 years ago
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.
(Assignee)

Comment 13

5 years ago
Created attachment 8384039 [details] [diff] [review]
colorSearchResultTiles v2

Once more with feeling!

:jimm, your ability to stay on top of bugmail is impressive. :)
Attachment #8383926 - Attachment is obsolete: true
Attachment #8384039 - Flags: review?(mbrubeck)
Flags: needinfo?(ally)
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?
Attachment #8384039 - Flags: feedback+
Attachment #8384039 - Flags: review?(mbrubeck) → review+

Updated

5 years ago
Whiteboard: p=5 s=it-30c-29a-28b.2 r=ff30 → p=5 s=it-30c-29a-28b.3 r=ff30
(Assignee)

Comment 15

5 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/435378ccdcbf
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
For testing and verification.  Reopen if any defects found.
Flags: needinfo?(kamiljoz)
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
Status: RESOLVED → VERIFIED
Flags: needinfo?(kamiljoz)
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.