Closed Bug 1166868 Opened 6 years ago Closed 6 years ago

Change default custom search engine icon in search engine bar

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WORKSFORME
Firefox 41

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

Attachments

(5 files, 3 obsolete files)

Antlam requested a magnifying glass rather than the current globe. NI for assets.
Flags: needinfo?(alam)
Note that this patch may also fix the issue where the default icon gets clipped/scaled/butchered.
Attached file default_searchicon.zip (obsolete) —
These should sit well next to the higher res icons in bug 1156917!
Flags: needinfo?(alam)
The clipping will be fixed in bug 1158282.
Depends on: 1158282
Attached image New assets (obsolete) —
Anthony, these assets look a little smaller than I'd expect.
Flags: needinfo?(alam)
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?
Flags: needinfo?(alam) → needinfo?(michael.l.comella)
Default icon (with white container)
Attachment #8608367 - Attachment is obsolete: true
Flags: needinfo?(michael.l.comella)
Attached image New assets v2
Antlam, what say you?

Looks maybe small to me, but this is using the old search icon assets.
Attachment #8609082 - Attachment is obsolete: true
Attachment #8609544 - Flags: feedback?(alam)
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
Attachment #8609544 - Flags: feedback?(alam) → feedback+
^ :)
Flags: needinfo?(michael.l.comella)
Attached image New assets v2 32dp
Flags: needinfo?(michael.l.comella)
Attachment #8609580 - Flags: feedback?(alam)
Comment on attachment 8609580 [details]
New assets v2 32dp

NICE!
Attachment #8609580 - Flags: feedback?(alam) → feedback+
/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/
Attachment #8609594 - Flags: review?(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.
Attachment #8609594 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8609594 [details]
MozReview Request: bz://1166868/mcomella

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

Ship It!
NI self to comment on this bug when this is backed out (see bug 1170289 comment 1).
Flags: needinfo?(michael.l.comella)
Backed out (and fixed more simply) by bug 1170289 comment 12.
Flags: needinfo?(michael.l.comella)
Resolution: FIXED → WORKSFORME
Attachment #8609594 - Attachment is obsolete: true
Attachment #8620341 - Flags: review+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.