1078301 - Search engine color can be slow to appear

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: Search Activity, defect)

Description

[:Margaret Leibovic]

Sometimes when I start the search activity, I see a white search bar underline appear before the dominant color filter is applied.

Since users switch their search engine so infrequently, it could be a win to just cache the computed dominant color.

To do this, I would probably move the getDomninantColor call out of SearchBar, and put it in a getColor method in SearchEngine. Then that method could pull the color out of a cache, or compute it and cache it if necessary. We could also move this computation/caching logic to where we parse the image URL if we want to move this to a background thread.

One complication with all this is that we need a Bitmap to compute the dominant color, so maybe we should refactor SearchEngine to hold onto a Bitmap for the icon instead of just the URL, but we'll need to be careful if we try to access this Bitmap from multiple threads.
 
http://mxr.mozilla.org/mozilla-central/source/mobile/android/search/java/org/mozilla/search/autocomplete/SearchBar.java#138
http://mxr.mozilla.org/mozilla-central/source/mobile/android/search/java/org/mozilla/search/providers/SearchEngine.java

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Crazy idea: Put the color in the plugin XML and just read it. Otherwise, fallback to dominant color.

Comment 2

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #1)
> Crazy idea: Put the color in the plugin XML and just read it. Otherwise,
> fallback to dominant color.

I don't love this approach, since it would require work if we want to support this in localized plugins (especially ones that aren't in en-US, where we would need to come up with a color), and it wouldn't help the performance of third-party plugins at all. Right now I like the simplicity of one code path to get the dominant color, and I don't think it would be too hard to just cache it.

I also see ckitching is working on using the favicon cahce for search engine icons in bug 961600, I wonder if we can take advantage of the favicon cache in the search activity. The favicon cache also has logic for caching the dominant color, so that would let us avoid re-implementing a solution.

ckitching, do you know if we could plug the favicon cache into the search activity? Could we just import Favicons.java and use that directly?

Comment 3

[Chris Kitching [:ckitching]]

(In reply to :Margaret Leibovic from comment #2)
> (In reply to Mark Finkle (:mfinkle) from comment #1)
> > Crazy idea: Put the color in the plugin XML and just read it. Otherwise,
> > fallback to dominant color.
> 
> I don't love this approach, since it would require work if we want to
> support this in localized plugins (especially ones that aren't in en-US,
> where we would need to come up with a color), and it wouldn't help the
> performance of third-party plugins at all. Right now I like the simplicity
> of one code path to get the dominant color, and I don't think it would be
> too hard to just cache it.
> 
> I also see ckitching is working on using the favicon cahce for search engine
> icons in bug 961600, I wonder if we can take advantage of the favicon cache
> in the search activity. The favicon cache also has logic for caching the
> dominant color, so that would let us avoid re-implementing a solution.
> 
> ckitching, do you know if we could plug the favicon cache into the search
> activity? Could we just import Favicons.java and use that directly?

You absolutely can. It's all designed to be thread safe, so should "just work".

The patch from Bug 1070092 (which has landed) made LoadFaviconTask capable of understanding data: URIs, so if you pass your data URI to the API in Favicons it should magically work, with all the caching and other bells and whistles you might expect.

You don't need to wait for Bug 961600 to land before you can do this, but the patch there might prove educational. (Depending on if you want to go to the network or not, you'll probably want either Favicons.getSizedFavicon or getSizedFaviconForPageFromLocal. Hopefully the javadoc comments in Favicons make this clear, if not feel free to poke me some more).

Comment 4

[:Margaret Leibovic]

(In reply to Chris Kitching [:ckitching] from comment #3)
> (In reply to :Margaret Leibovic from comment #2)
> > (In reply to Mark Finkle (:mfinkle) from comment #1)
> > > Crazy idea: Put the color in the plugin XML and just read it. Otherwise,
> > > fallback to dominant color.
> > 
> > I don't love this approach, since it would require work if we want to
> > support this in localized plugins (especially ones that aren't in en-US,
> > where we would need to come up with a color), and it wouldn't help the
> > performance of third-party plugins at all. Right now I like the simplicity
> > of one code path to get the dominant color, and I don't think it would be
> > too hard to just cache it.
> > 
> > I also see ckitching is working on using the favicon cahce for search engine
> > icons in bug 961600, I wonder if we can take advantage of the favicon cache
> > in the search activity. The favicon cache also has logic for caching the
> > dominant color, so that would let us avoid re-implementing a solution.
> > 
> > ckitching, do you know if we could plug the favicon cache into the search
> > activity? Could we just import Favicons.java and use that directly?
> 
> You absolutely can. It's all designed to be thread safe, so should "just
> work".
> 
> The patch from Bug 1070092 (which has landed) made LoadFaviconTask capable
> of understanding data: URIs, so if you pass your data URI to the API in
> Favicons it should magically work, with all the caching and other bells and
> whistles you might expect.
> 
> You don't need to wait for Bug 961600 to land before you can do this, but
> the patch there might prove educational. (Depending on if you want to go to
> the network or not, you'll probably want either Favicons.getSizedFavicon or
> getSizedFaviconForPageFromLocal. Hopefully the javadoc comments in Favicons
> make this clear, if not feel free to poke me some more).

I tried to do this last week, but unfortunately it didn't work as expected, since it Favicons isn't initalized if GeckoApp isn't around to initalize it. If we were to use Favicons in the search activity, I think we'd need to refactor it to make it less dependent on a Context.

But, for this bug, we could still do something simple to cache the color, since that's what this bug is about.

Comment 5

[Jan Henning [:JanH]]

This is an ex-Search Activity now (bug 1221344).