917947 - Search engine icons are minuscule in search engine preference dialog on large DPI devices

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Attachment: Nightly (09/18) - Screenshot

See screenshot. 

--
Nightly (09/18)
Samsung Galaxy SIV (Android 4.3)

Comment 1

[Aaron Train [:aaronmt]]

Looks like a DPI issue on high-res devices; also noticed on this device that the icons for each engine have a coloured surrounding background not visible on a Nexus 4.

Comment 2

[Chris Kitching [:ckitching]]

Aye. The shipped images aren't very high resolution - explains the coloured background thing on the Nexus 4.

However, the minisculeness in the prompt is annoying. Not an issue to fix after the new Favicon system lands - sort of annoying to fix beforehand.

Comment 3

[:Margaret Leibovic]

This doesn't affect 25. I don't think we should worry about fixing this for 26, and instead wait for the new favicon work to land to get this fixed in 27.

Comment 4

[Lucas Rocha (:lucasr)]

Maybe untrack it from 26 then?

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Lucas Rocha (:lucasr) from comment #4)
> Maybe untrack it from 26 then?

we can certainly re-nom it

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Let's remove the icon from the popup menu in Fx26. Is that the only real problem? That's all I saw in the screenshot.

We can keep the icon in the menu for Fx27 as long as the new fixes make it work there.

Comment 7

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 816786 [details] [diff] [review]
Patch

This looks like the "least" we could do. I suppose it's enough. We might consider removing more, so we do less work, but maybe that is too risky.

r+ from me, but let's get margaret's opinion too.

Comment 9

[:Margaret Leibovic]

Comment on attachment 816786 [details] [diff] [review]
Patch

Review of attachment 816786 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/preferences/SearchEnginePreference.java
@@ -205,5 @@
>          // as the user may not ever actually tap this object.
>          if (mPromptIcon == null && mIconBitmap != null) {
>              mPromptIcon = new BitmapDrawable(mFaviconView.getBitmap());
>          }
> -        builder.setIcon(mPromptIcon);

I think this is a good way to make a low-risk patch that we can uplift, but let's file a follow-up bug to fix this icon. If we are planning to restore this functionality, instead of removing this line, I think it would be better to just disable it with a comment and include that bug number.

Comment 10

[Sriram Ramasubramanian [:sriram]]

One line change is pretty straightforward. When we plan on adding an icon, we don't need this line to remind us. I guess we don't need to comment this -- we can just remove this.

Comment 11

[:Margaret Leibovic]

(In reply to Sriram Ramasubramanian [:sriram] from comment #10)
> One line change is pretty straightforward. When we plan on adding an icon,
> we don't need this line to remind us. I guess we don't need to comment this
> -- we can just remove this.

Well, when someone looks at this code without any context, they're going to say "Why is there all this code for an icon but we don't do anything with it?" It would be nice to be able to just point them to a bug number.

I filed bug 926711 about this.

Comment 12

[Sriram Ramasubramanian [:sriram]]

Hiding and not removing: https://hg.mozilla.org/integration/fx-team/rev/effaf67d0336

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Sriram - Uplift to Fx26 request?

Comment 15

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 816786 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Small search icons.
User impact if declined: Miniscule icons are shown.
Testing completed (on m-c, etc.): Landed in m-c yesterday.
Risk to taking this patch (and alternatives if risky): None.
String or IDL/UUID changes made by this patch: None.

Note: The patch was later changed to comment the statement and not remove it. That's what was pushed.

Comment 16

[Sriram Ramasubramanian [:sriram]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b51c179370c9