1170346 - Custom search engines w/o favicons do not have a default icon in settings screen

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bugzilla, no favicon in settings

A custom search engine for Bugzilla does not have an icon.

I'm adding a patch to set a default icon in bug 1170289, but it doesn't seem to have fixed the issue outside of BrowserSearch - perhaps the SearchEngine class is not used for the Settings pane.

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Ni-ing Ricardo to think about this more

Comment 2

[Ricardo Vazquez [:ricardo]]

Attachment: Favicon - 2.png

What we can do for custom search engine that do not have an icon is to extend the Top Sites aesthetic and colours already present in Fennec Nightly. Antlam, let me know what you think.

Comment 3

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8770982 [details]
Favicon - 2.png

Great idea!

this would be a great improvement over what we have right now I think.

Mike, would this be difficult?

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1170346 - Use top sites default favicon style for search preferences.

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

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8771037 [details]
Bug 1170346 - Use top sites default favicon style for search preferences.

https://reviewboard.mozilla.org/r/64290/#review61904

::: mobile/android/base/java/org/mozilla/gecko/preferences/SearchEnginePreference.java:207
(Diff revision 1)
>      }
> +
> +    private void generateAndUseDefaultFavicon() {
> +        // HACK: favicon generator requests a url to identify its representative character but we give
> +        // it the title string. I didn't try to modify the favicon generator due to time constraints.
> +        FaviconGenerator.generate(getContext(), getTitle().toString(), new OnFaviconLoadedListener() {

Yeah, this kind of works. But internally FaviconGenerator parses the input (Uri.parse()) and uses the host. So this seems to be fragile.

We also strip common prefixes (m., www., mobile.) - It's unlike that this will be in a search engine title but possible.

We could still go ahead and ship this if we make sure that TestFaviconGenerator has some test cases covering this.

But it seems like the better option is to add a specific method to FaviconGenerator and share as much code as possible.

Comment 6

[Ricardo Vazquez [:ricardo]]

What we could also do is take the first letter of whatever the user inputs as the title of that Search Engine.

ie. User enters "Longreads", the favicon letter would be "L".

Perhaps this way we can assure ourselves that we don't hit the m. www. mobile. use cases.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Sebastian Kaspari (:sebastian) from comment #5)
> But it seems like the better option is to add a specific method to
> FaviconGenerator and share as much code as possible.

In retrospect, this is a much better idea.

I would like to focus on context graph and this bug isn't high priority so I'm going to unassign.

Comment 8

[Sorina Florean [:sflorean]]

Hi all, 

This is a screenshot https://i.imgur.com/pKYCYAt.png of custom search engines added in Nightly and Aurora. The new added engines don't have icon in quick search bar, so it's a little bit confusing. 
It's expected behavior?
Will be fixed with patches from this bug, or should I file another bug?
Thanks!

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

This bug here has been fixed by the new icon code as a side effect: We always will generate fallback icons if we can't load one.

(In reply to Sorina Florean [:sorina] from comment #8)
> This is a screenshot https://i.imgur.com/pKYCYAt.png of custom search
> engines added in Nightly and Aurora. The new added engines don't have icon
> in quick search bar, so it's a little bit confusing. 
> It's expected behavior?

This seems to be a different problem and I filed bug 1319017.