1158282 - Custom search engine favicons are displayed inconsistently in BrowserSearch screen

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Follow-up to bug 1137483.

Custom search engines seem to be a bit wonky - let's fix it!

Comment 1

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

This is the only bug (besides regressions) that antlam said this needs to ship. If this doesn't hit 40, we need to add a build flag or backout bug 1137483 from 40.

Comment 2

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

See title. There doesn't seem to be any obvious reason for when the partial globe or nothing is shown.

The list of search engines also seems to sometimes show a globe and sometimes drop the favicon for custom search engines too, which means there may be two issues here:
  * Showing the entire globe in the search engine bar
  * Always showing a globe if no favicon is present

Anthony, are you okay with a globe if there's no favicon? Or is there another placeholder icon you'd like to use?

Comment 3

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

Attachment: No favicon in search engine bar

Comment 4

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

Attachment: Partial globe in search engine bar

Comment 5

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

I'm moving the search engine icon update to bug 1166868 and this bug will handle the existing issue (it's in the original BrowserSearch list) of the inconsistent display of custom search engine icons.

I've noticed that the custom search engine icons will change when scrolling up and down so I imagine this is a Loader and/or Adapter issue. This includes custom search engines with non-default favicons - sometimes they will go default.

Comment 6

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

Attachment: Custom search engine w/o default favicon has default favicon

This occurred after scrolling up and down a bit. The icon always updates when it's row is off-screen.

Comment 7

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

Does bug 1156917 help with this? Since we are providing the assets ourselves for the defaults in that bug (OK'd with our default providers).

Comment 8

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

(In reply to Anthony Lam (:antlam) from comment #7)
> Does bug 1156917 help with this? Since we are providing the assets ourselves
> for the defaults in that bug (OK'd with our default providers).

No, this is a pre-existing bug in the current code that got uncovered from the addition of the search engine bar.

Comment 9

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

Attachment: MozReview Request: bz://1158282/mcomella

/r/9211 - Bug 1158282 - Handle the default favicon as any other favicon. r=margaret
/r/9213 - Bug 1158282 - Clarify existing comment in FaviconView.formatImage. r=margaret

Pull down these commits:

hg pull -r 11af2c5e798866eb31072a3f52d07392b2d15ba1 https://reviewboard-hg.mozilla.org/gecko/

Comment 10

[:Margaret Leibovic]

https://reviewboard.mozilla.org/r/9211/#review7889

::: mobile/android/base/widget/FaviconView.java:226
(Diff revision 1)
> +        updateAndScaleImage(defaultFaviconBitmap, DEFAULT_FAVICON_KEY);

It feels strange to be manually scaling this image, but I agree it's good to avoid custom logic paths.

And if this fixes the view re-use issue, works for me!

Comment 11

[:Margaret Leibovic]

Comment on attachment 8608998 [details]
MozReview Request: bz://1158282/mcomella

https://reviewboard.mozilla.org/r/9209/#review7891

Ship It!

Comment 12

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

https://hg.mozilla.org/integration/fx-team/rev/7330e7687ca6
https://hg.mozilla.org/integration/fx-team/rev/8d1f3331f453
https://hg.mozilla.org/integration/fx-team/rev/02d343f01fa5

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7330e7687ca6
https://hg.mozilla.org/mozilla-central/rev/8d1f3331f453
https://hg.mozilla.org/mozilla-central/rev/02d343f01fa5

Comment 15

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

Attachment: MozReview Request: Bug 1158282 - Handle the default favicon as any other favicon. r=margaret

Comment 16

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

Attachment: MozReview Request: Bug 1158282 - Clarify existing comment in FaviconView.formatImage. r=margaret