1166868 - Change default custom search engine icon in search engine bar

Status: RESOLVED WORKSFORME

(Firefox for Android Graveyard :: General, defect)

Description

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

Antlam requested a magnifying glass rather than the current globe. NI for assets.

Comment 1

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

Attachment: Clipped default custom search engine icon

Note that this patch may also fix the issue where the default icon gets clipped/scaled/butchered.

Comment 2

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

Attachment: default_searchicon.zip

These should sit well next to the higher res icons in bug 1156917!

Comment 3

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

The clipping will be fixed in bug 1158282.

Comment 4

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

Attachment: New assets

Anthony, these assets look a little smaller than I'd expect.

Comment 5

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

Hm, are we talking about the quick search bar? Those icons do look a bit too small. I'm confused though cause the container is croppng the magnifying glass in the quick search bar so doesn't it mean the asset is too big?

But I made this magnifying glass icon the same size as the high res defaults we want to change to in bug 1156917. It sits well next to those ones that we'd like to end up with.

Maybe the best course of action here is to get a screenshot up for bug 1156917 and then see how this magnifying asset sits next to those (bigger) ones?

Comment 6

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

Attachment: default_searchicon2.zip

Default icon (with white container)

Comment 7

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

Attachment: New assets v2

Antlam, what say you?

Looks maybe small to me, but this is using the old search icon assets.

Comment 8

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

Comment on attachment 8609544 [details]
New assets v2

I think it's fine. Can you post a shot with it at 32? i.e. without the "limit" you're putting on it

Comment 9

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

^ :)

Comment 10

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

Attachment: New assets v2 32dp

Comment 11

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

Comment on attachment 8609580 [details]
New assets v2 32dp

NICE!

Comment 12

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

Attachment: MozReview Request: bz://1166868/mcomella

/r/9279 - Bug 1166868 - Update default favicon for custom search engines. r=margaret"

Pull down this commit:

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

Comment 13

[:Margaret Leibovic]

https://reviewboard.mozilla.org/r/9279/#review8007

::: mobile/android/base/widget/FaviconView.java:26
(Diff revision 1)
> -    private static String DEFAULT_FAVICON_KEY = FaviconView.class.getSimpleName() + "DefaultFavicon";
> +    private static String DEFAULT_FAVICON_KEY = "DefaultFavicon";

This is really more like the DEFAULT_DEFAULT_FAVICON_KEY :)

The naming here is confusing, but a better system isn't immediately clear to me. Perhaps add some comments here explaining how these values are used?

::: mobile/android/base/widget/FaviconView.java:91
(Diff revision 1)
> +            defaultFaviconKey = a.getString(R.styleable.FaviconView_defaultFaviconKey);

I think you should add some class comments that explain how you can specify a custom default favicon using styles like this.

Comment 14

[:Margaret Leibovic]

Comment on attachment 8609594 [details]
MozReview Request: bz://1166868/mcomella

https://reviewboard.mozilla.org/r/9277/#review8009

Ship It!

Comment 15

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

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

Comment 16

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

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

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/fefa03e0fb9d
https://hg.mozilla.org/mozilla-central/rev/b73de443b580
https://hg.mozilla.org/mozilla-central/rev/621fe8106086

Comment 18

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

NI self to comment on this bug when this is backed out (see bug 1170289 comment 1).

Comment 19

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

Backed out (and fixed more simply) by bug 1170289 comment 12.

Comment 21

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

Attachment: MozReview Request: Bug 1166868 - Update default favicon for custom search engines. r=margaret"