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

VERIFIED FIXED in Firefox 26

Status

()

defect
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: aaronmt, Assigned: sriram)

Tracking

({regression, reproducible})

26 Branch
Firefox 27
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox25 unaffected, firefox26 fixed, firefox27 verified, fennec26+)

Details

Attachments

(2 attachments)

See screenshot. 

--
Nightly (09/18)
Samsung Galaxy SIV (Android 4.3)
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.
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.
Summary: Search engine icons are minuscule in search engine preference dialog → Search engine icons are minuscule in search engine preference dialog on large DPI devices
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.
tracking-fennec: ? → 26+
Maybe untrack it from 26 then?
(In reply to Lucas Rocha (:lucasr) from comment #4)
> Maybe untrack it from 26 then?

we can certainly re-nom it
tracking-fennec: 26+ → ?
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.
Assignee: nobody → sriram
tracking-fennec: ? → 26+
Posted patch PatchSplinter Review
Attachment #816786 - Flags: review?(mark.finkle)
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.
Attachment #816786 - Flags: review?(mark.finkle)
Attachment #816786 - Flags: review?(margaret.leibovic)
Attachment #816786 - Flags: review+
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.
Attachment #816786 - Flags: review?(margaret.leibovic) → review+
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.
Depends on: 926711
(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.
https://hg.mozilla.org/mozilla-central/rev/effaf67d0336
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Sriram - Uplift to Fx26 request?
Flags: needinfo?(sriram)
Status: RESOLVED → VERIFIED
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.
Attachment #816786 - Flags: approval-mozilla-aurora?
Attachment #816786 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.