Use the favicon cache for search engine icons

RESOLVED FIXED in Firefox 36

Status

()

Firefox for Android
Favicon Handling
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: rnewman, Assigned: ckitching)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 36
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

Updated

5 years ago
Duplicate of this bug: 913746
(Assignee)

Updated

5 years ago
See Also: → bug 928585
(Reporter)

Updated

4 years ago
Component: General → Favicon Handling
(Reporter)

Updated

4 years ago
Blocks: 928585
See Also: bug 928585
(Assignee)

Updated

4 years ago
Blocks: 1029524
(Assignee)

Comment 2

4 years ago
Created attachment 8492496 [details] [diff] [review]
Use the favicon cache for search engine icons
Attachment #8492496 - Flags: review?(rnewman)
(Assignee)

Updated

4 years ago
Assignee: nobody → chriskitching
Status: NEW → ASSIGNED
(Assignee)

Comment 3

4 years ago
Just call the usual favicon loading mechanism, enhanced in Bug 1070092 to support data URIs.
Caching ensues. Yay!
Depends on: 1070092
(Reporter)

Comment 4

4 years ago
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!
Attachment #8492496 - Flags: review?(rnewman) → review+
(Assignee)

Comment 5

4 years ago
Created attachment 8500063 [details] [diff] [review]
Use the favicon cache for search engine icons

Good point about the view binding. Added some duct-tape... Is there a neater way?
Attachment #8500063 - Flags: review?(rnewman)
(Assignee)

Updated

4 years ago
Attachment #8492496 - Attachment is obsolete: true
(Reporter)

Comment 6

4 years ago
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!
Attachment #8500063 - Flags: review?(rnewman) → review-
(Assignee)

Comment 7

4 years ago
Created attachment 8503314 [details] [diff] [review]
Use the favicon cache for search engine icons

Fail. Looks like switching to a lock object should do the trick?
Attachment #8503314 - Flags: review?(rnewman)
(Assignee)

Updated

4 years ago
Attachment #8500063 - Attachment is obsolete: true
(Reporter)

Comment 8

4 years ago
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).
Attachment #8503314 - Flags: review?(rnewman) → review+
(Reporter)

Comment 9

4 years ago
Chris, this is waiting on you to test and land.
Flags: needinfo?(chriskitching)
(Reporter)

Updated

4 years ago
Blocks: 926646
https://hg.mozilla.org/mozilla-central/rev/80a69108ae62
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
(Assignee)

Updated

4 years ago
Flags: needinfo?(chriskitching)
Depends on: 1088669
Backed out in an attempt to find the cause of bug 1088669
https://hg.mozilla.org/integration/fx-team/rev/9beb868e1cd8
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/d0d710d0303e
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.