961600 - Use the favicon cache for search engine icons

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Favicon Handling, defect)

Description

[Richard Newman [:rnewman]]

We currently grab JSON-serialized base64-encoded icons from the Gecko search system every time we want to display a list of search engines.

We should instead populate the favicon cache with these, ideally without all of the awful machinations that go on in JS-land (see discussion in Bug 961538).

This will buy us dominant color computation (though see Bug 926646), not to mention huge efficiency improvements.

Comment 2

[Chris Kitching [:ckitching]]

Attachment: Use the favicon cache for search engine icons

Comment 3

[Chris Kitching [:ckitching]]

Just call the usual favicon loading mechanism, enhanced in Bug 1070092 to support data URIs.
Caching ensues. Yay!

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 8492496 [details] [diff] [review]
Use the favicon cache for search engine icons

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

Fine by me if it works.

::: mobile/android/base/GeckoAppShell.java
@@ +855,5 @@
>      static void createShortcut(final String aTitle, final String aURI, final String aIconData) {
> +        Favicons.getSizedFavicon(getContext(), aURI, aIconData, Integer.MAX_VALUE, 0,
> +            new OnFaviconLoadedListener() {
> +                @Override
> +                public void onFaviconLoaded(String url, String faviconURL, Bitmap favicon) {

onFaviconLoaded is always called on the UI thread.

We probably don't want to call doCreateShortcut on the UI thread.

So, either:

* Rework the favicon system to not call the runnable on the UI thread in some circumstances, or

* Call createShortcut instead, which queues up a runnable for you.

::: mobile/android/base/preferences/SearchEnginePreference.java
@@ +165,5 @@
> +            Favicons.getSizedFavicon(getContext(), mIdentifier, iconURI, desiredWidth, 0,
> +                new OnFaviconLoadedListener() {
> +                    @Override
> +                    public void onFaviconLoaded(String url, String faviconURL, Bitmap favicon) {
> +                        mIconBitmap = favicon;

Note that this call now happens later, which might affect view binding (the uses of mIconBitmap elsewhere in this file).

Make sure that this actually works!

Comment 5

[Chris Kitching [:ckitching]]

Attachment: Use the favicon cache for search engine icons

Good point about the view binding. Added some duct-tape... Is there a neater way?

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 8500063 [details] [diff] [review]
Use the favicon cache for search engine icons

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

I think this is fine apart from the error in SEP.java.

::: mobile/android/base/preferences/SearchEnginePreference.java
@@ +57,5 @@
>          super.onBindView(view);
>  
> +        // We synchronise to avoid a race condition between this and the favicon loading callback in
> +        // setSearchEngineFromJSON.
> +        synchronized (mIconBitmap) {

You can't synchronize on a null reference. This will throw a NPE if mIconBitmap hasn't been set yet.

@@ +61,5 @@
> +        synchronized (mIconBitmap) {
> +            // Set the icon in the FaviconView.
> +            mFaviconView = ((FaviconView) view.findViewById(R.id.search_engine_icon));
> +
> +            if (mIconBitmap != null) {

And here you suspect that it's null, so I think I'm right to be skeptical!

Comment 7

[Chris Kitching [:ckitching]]

Attachment: Use the favicon cache for search engine icons

Fail. Looks like switching to a lock object should do the trick?

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 8503314 [details] [diff] [review]
Use the favicon cache for search engine icons

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

If this works, I'm happy. Please test that the regular search engine list works, that the list works after switching locales (if you have a multilocale build), and also that the search activity gets the correct icons (just in case).

Comment 9

[Richard Newman [:rnewman]]

Chris, this is waiting on you to test and land.

Comment 10

[Chris Kitching [:ckitching]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/80a69108ae62

Comment 12

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

Backed out in an attempt to find the cause of bug 1088669
https://hg.mozilla.org/integration/fx-team/rev/9beb868e1cd8

Comment 13

[Chris Kitching [:ckitching]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d0d710d0303e