1319017 - Always default icon shown for custom search engines

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect, P1)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

When adding a custom search engine then we will always show:

* The default search icon in the awesomescreen
* A generated default icon in the search settings

It seems like we do not use the page's icon for search engines anymore.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: search_default_icon.png

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: settings_default_icon.png

Comment 3

[Max Liu [:maliu]]

SearchPreference receive the list of SearchEngines from "SearchEngines:Data", which queries data from view table "SearchEngines:Data" in browser.db.
The favicon of custom search engine is "null" and cause the symptom is because there's no data related to the search page in both history and favicon table.
The favicon in history list is provided by in-memory cache.

Question is, When does favicon be insert into favicon table?

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Thanks for debugging this. This seems to be a part I missed when I refactored parts of the icon code - because we access the db directly from JS without going through the Java side.

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

I'll have a look.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: Bug 1319017 - browser.js addEngine(): Use icon pipeline to load search engine icon.

I missed this part of the code when refactoring the icon code in bug 1290014 - because this code
never used our favicon loader but instead accessed the database directly. Now this code is using
the same pipeline we use from Java code and should therefore always load (or generate) an icon.

Review commit: https://reviewboard.mozilla.org/r/102668/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102668/

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

@Grisha: Btw. we discussed a similar approach for AS images. This works pretty well for the icons, but of course they are only 32x32dp. However there's no delay and it's fast. :)

Comment 8

[Max Liu [:maliu]]

Comment on attachment 8824110 [details]
Bug 1319017 - browser.js addEngine(): Use icon pipeline to load search engine icon.

https://reviewboard.mozilla.org/r/102668/#review103194

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:1911
(Diff revision 1)
>                  break;
>  
> +            case "SearchEngines:IconRequest":
> +                final String pageUrl = message.getString("url");
> +
> +                if (!TextUtils.isEmpty(pageUrl)) {

Is it possible that js side return the url with plain string "null"?

How about early return?
```
if (TextUtils.isEmpty(pageUrl)) {
    callback.sendError(null);
    break;
}
```

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:1912
(Diff revision 1)
>  
> +            case "SearchEngines:IconRequest":
> +                final String pageUrl = message.getString("url");
> +
> +                if (!TextUtils.isEmpty(pageUrl)) {
> +                    Icons.with(this)

This builder block looks very similar to BrowserApp#getFaviconFromCache except having this line:".skipNetwork()". Is it a good idea if we wrap this with extra argument(skipNetwork)?

Also, after checked all reference to Bitmap.CompressFormat.PNG in project, they all seems to encode/decode bitmap from/to Base64 strings for FavIcon. Is it a good timing to make a more generic util class in this patch? Or file another bug for follow up refactoring?

::: mobile/android/chrome/content/browser.js:6255
(Diff revision 1)
> -        },
> -        handleCompletion: function (reason) {
> -          // if there's already an engine with this name, add a number to
> +        // if there's already an engine with this name, add a number to
> -          // make the name unique (e.g., "Google" becomes "Google 2")
> +        // make the name unique (e.g., "Google" becomes "Google 2")
> -          let name = title.value;
> +        let name = title.value;
> -          for (let i = 2; Services.search.getEngineByName(name); i++)
> +        for (let i = 2; Services.search.getEngineByName(name); i++) {

When I try this patch locally, a strange symptom happen. I can't add a SearchEngine if it has been added and deleted before. I guess it's not related to this bug right?

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8824110 [details]
Bug 1319017 - browser.js addEngine(): Use icon pipeline to load search engine icon.

https://reviewboard.mozilla.org/r/102668/#review103764

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:1912
(Diff revision 1)
>  
> +            case "SearchEngines:IconRequest":
> +                final String pageUrl = message.getString("url");
> +
> +                if (!TextUtils.isEmpty(pageUrl)) {
> +                    Icons.with(this)

Yeah, let's see if I can unify the implementations.

::: mobile/android/chrome/content/browser.js:6255
(Diff revision 1)
> -        },
> -        handleCompletion: function (reason) {
> -          // if there's already an engine with this name, add a number to
> +        // if there's already an engine with this name, add a number to
> -          // make the name unique (e.g., "Google" becomes "Google 2")
> +        // make the name unique (e.g., "Google" becomes "Google 2")
> -          let name = title.value;
> +        let name = title.value;
> -          for (let i = 2; Services.search.getEngineByName(name); i++)
> +        for (let i = 2; Services.search.getEngineByName(name); i++) {

Some time ago we added additional code to not allow adding the same search engine twice (bug 1262916). However the option should re-appear after deleting the search engine - maybe there's a regression - let's file a separate bug if you can reproduce this.

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8824110 [details]
Bug 1319017 - browser.js addEngine(): Use icon pipeline to load search engine icon.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102668/diff/1-2/

Comment 11

[:Grisha Kruglov]

Comment on attachment 8824110 [details]
Bug 1319017 - browser.js addEngine(): Use icon pipeline to load search engine icon.

https://reviewboard.mozilla.org/r/102668/#review104368

Looks good. Happy to hear this approach works well in this case; let's see if it'll work for larger images, and in reverse :)

::: mobile/android/base/java/org/mozilla/gecko/icons/IconsHelper.java:159
(Diff revision 2)
> +            @Override
> +            public void onIconResponse(IconResponse response) {
> +                ByteArrayOutputStream stream = new ByteArrayOutputStream();
> +
> +                try {
> +                    response.getBitmap().compress(Bitmap.CompressFormat.PNG, 100, stream);

Might be worth noting in a comment that PNG is lossless, and so `compress` will ignore the quality setting.

Comment 12

[Max Liu [:maliu]]

Comment on attachment 8824110 [details]
Bug 1319017 - browser.js addEngine(): Use icon pipeline to load search engine icon.

https://reviewboard.mozilla.org/r/102668/#review104400

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8824110 [details]
Bug 1319017 - browser.js addEngine(): Use icon pipeline to load search engine icon.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102668/diff/2-3/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d1d89a3a187a
browser.js addEngine(): Use icon pipeline to load search engine icon. r=Grisha,maliu

Comment 15

[Carsten Book [:Tomcat]]

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

Comment 16

[Oana Horvath [:ohorvath - out of office]]

Verified as fixed on Nightly 53 (2017-01-16) with the following devices: 
- Prestigio Grace 5x (Android 4.4.2)
- Huawei Honor (Android 5.1.1)
- LG G4 (Android 6.0)
- Asus ZenPad(Android 6.0.1)