Closed
Bug 783921
Opened 12 years ago
Closed 12 years ago
Application shortcuts wrongly scaled? (DPI)
Categories
(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P1)
Tracking
(firefox17 fixed, firefox18 verified)
VERIFIED
FIXED
Firefox 18
People
(Reporter: aaronmt, Assigned: wesj)
References
Details
Attachments
(6 files, 4 obsolete files)
349.71 KB,
image/png
|
Details | |
79.00 KB,
image/png
|
Details | |
34.16 KB,
image/png
|
Details | |
73.50 KB,
image/png
|
Details | |
9.04 KB,
patch
|
mfinkle
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
4.56 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
See screenshot. -- Nightly (08/19) Samsung Galaxy Nexus (Android 4.1.1)
Reporter | ||
Updated•12 years ago
|
Attachment #653214 -
Attachment is obsolete: true
Reporter | ||
Comment 1•12 years ago
|
||
Reporter | ||
Comment 2•12 years ago
|
||
createShortcut(manifest.name, manifest.fullLaunchPath(), manifest.iconURLForSize("64"), "webapp"); Can we use 128 if available?
Comment 3•12 years ago
|
||
(In reply to Aaron Train [:aaronmt] from comment #2) > createShortcut(manifest.name, manifest.fullLaunchPath(), > manifest.iconURLForSize("64"), "webapp"); > > Can we use 128 if available? Yeah, originally 64x64 I may have suggested originally. Although after doing the icon study that's more recent for web apps on marketplace, 128x128 might be better choice. Generally, we should go with the larger icon if it's available and scale down.
Reporter | ||
Comment 4•12 years ago
|
||
Is getBiggestIcon working correctly here? http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#6014
Assignee | ||
Comment 5•12 years ago
|
||
We could also try checking the icon size we're drawing. I saw the other day that there's an API for it on SDK >= 11: http://developer.android.com/reference/android/app/ActivityManager.html#getLauncherLargeIconSize%28%29
Updated•12 years ago
|
Priority: -- → P1
Reporter | ||
Comment 6•12 years ago
|
||
Note to test: what happens on an HDPI/MDPI device
Reporter | ||
Comment 7•12 years ago
|
||
Slightly different but still affected here on an HDPI device like the Samsung Nexus S.
Reporter | ||
Comment 8•12 years ago
|
||
Same deal with MDPI (Droid Pro)
Assignee | ||
Comment 9•12 years ago
|
||
We actually downscale these images to 64x64 in browser.js before sending them to Android. Android then upscales them to 72x72 for the icons (although on Nexus 7 it really wants 85x85). I played with sending the raw image to Android and forcing it to do all the resize work, but its really awful at scaling images. If I resize to exactly what Android wants in Gecko, things look pretty good (IMO), so I think that's what I will shoot for. We'll have to tell Gecko what size Android wants somehow.
Assignee | ||
Comment 10•12 years ago
|
||
Our resizing code seem much... prettier than Androids. This basically tries to do everything in Gecko if it can by sending the size we want from Android to Gecko and storing it in a pref. We should only do that once, on first run, but I don't think we have a signal for it, so this does it on every startup. From there the images we send from Gecko to Java should now be the correct size without any further resizing required. This won't be true for shortcuts. They'll likely be scaled. because I'm using getLauncherLargeIconSize, its a bit harder to actually figure out their "inset" size as well. 2/3 is bigger than we currently do on my Nexus 7, but I imagine on a MDPI or LDPI screen it will be better. Putting this aside for a bit to focus on more pressing stuff.
Attachment #653964 -
Flags: feedback?(mark.finkle)
Comment 11•12 years ago
|
||
Comment on attachment 653964 [details] [diff] [review] WIP Patch >diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java >+ JSONObject jsonPref = new JSONObject(); >+ try { >+ int[] sizes = GeckoAppShell.getPreferredIconSizes(); >+ jsonPref.put("name", "desktop.shortcut.iconsize"); >+ jsonPref.put("type", "int"); >+ jsonPref.put("value", sizes[0]); >+ GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Preferences:Set", jsonPref.toString())); >+ } catch(Exception ex) { >+ Log.e(LOGTAG, "Error setting iconsize pref", ex); >+ } I am not the biggest fan of needing to do this. If we must, we must. If the icons are still not perfect though, maybe we could drop tis and use a different approach (see below) >diff --git a/mobile/android/base/GeckoAppShell.java b/mobile/android/base/GeckoAppShell.java > static void createShortcut(String aTitle, String aURI, String aIconData, String aType) { >- byte[] raw = Base64.decode(aIconData.substring(22), Base64.DEFAULT); >- Bitmap bitmap = BitmapFactory.decodeByteArray(raw, 0, raw.length); >- createShortcut(aTitle, aURI, aURI, bitmap, aType); >+ createShortcut(aTitle, aURI, aURI, aIconData, aType); Good catch >+ static public int[] getPreferredIconSizes() { >+ sizes[1] = Math.round(sizes[0]*2/3); From a quick look, this should not cause a rounding problem >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js > makeBase64Icon: function loadAndMakeBase64Icon(aIconURL, aCallbackFunction) { >- // The images are 64px, but Android will resize as needed. >- // Bigger is better than too small. >- const kIconSize = 64; I assume this was a problem. What if we use the largest icon we can find and let Java scale it "down" if needed? You could make getBiggestIcon return the URI _and_ the size. We could use the size here to create the image. Then Java could take the image and scale it down as needed. Scaling down should be less of an issue than scaling up. We'd need to update all the callers of getBiggestIcon. When I say return both URI and size, I mean something like this: let [uri, size] = getBiggestIcon(...) and: function getBiggestIcon(...) { ... return [some_uri, some_size]; } f+, generally OK
Attachment #653964 -
Flags: feedback?(mark.finkle) → feedback+
Assignee | ||
Comment 13•12 years ago
|
||
(In reply to Mark Finkle (:mfinkle) from comment #11) > You could make getBiggestIcon return the URI _and_ the size. We could use > the size here to create the image. Then Java could take the image and scale > it down as needed. Scaling down should be less of an issue than scaling up. > We'd need to update all the callers of getBiggestIcon. I thought so too! But turns out, Android is really bad at scaling things in any direction. Probably why we can't have just a few images for our UI! This is a screenshot of three different methods. Left is with this patch (gecko resizes to 72x72, Android uses 72x72). Middle is what we currently have (Gecko resizes to 64x64, Android upscales to 72x72). Right is all the scaling done by Andriod (Gecko sends the raw 128x128 image, Android downsamples it to 72x72). I'm gonna clean this up and just submit it.
Assignee | ||
Comment 14•12 years ago
|
||
Actually, this is basically the same patch with the unnecessary import removed.
Assignee: nobody → wjohnston
Attachment #653964 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #655715 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 15•12 years ago
|
||
From irc, mfinkle wanted me to look at just using ctypes here. I... really hate exposing one method for this so now I'm trying to expose some JNI like functions. I think that would be a nice bonus for addon authors. So calling this method is something like: var jni = new JNI(); var cls = jni.findClass("org.mozilla.gecko.GeckoAppShell"); var method = jni.getStaticMethodID(cls, "getPreferredIconSize", "()I"); var val = jni.callStaticIntMethod(cls, method, null); jni.close(); Those actually are all we need here. However, I do want to make sure the passing in jvalues work so I'm playing with some casting tricks. Putting this up to save my place and see how people feel about it.
Attachment #655715 -
Attachment is obsolete: true
Attachment #655715 -
Flags: review?(mark.finkle)
Comment 16•12 years ago
|
||
Comment on attachment 656679 [details] [diff] [review] WIP Patch v2 This patch has a lot of different patches in it, but the JNI example code looks worthwhile to me. Can you file a separate bug to land the JNI support? Also, I wonder if making JNI.js be JNI.jsm (a module) would be a better idea.
Attachment #656679 -
Flags: feedback+
Assignee | ||
Comment 17•12 years ago
|
||
Pushed the jni stuff to 787271. Asking blassey to look at that to make sure platform is ok with it. Reworked patch + some bits from bug 786051 that somehow got mixed in and I don't want to separate out.
Attachment #656679 -
Attachment is obsolete: true
Attachment #657081 -
Flags: review?(mark.finkle)
Comment 18•12 years ago
|
||
Comment on attachment 657081 [details] [diff] [review] Patch v2 >+ int s2 = size/2; s2 -> halfSize ? and can you move it down to just above the | canvas.drawBitmap | call where it is first used? >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js >+ get iconSize() { >+ Cu.import("resource://gre/modules/JNI.jsm"); >+ let jni = new JNI(); >+ let cls = jni.findClass("org.mozilla.gecko.GeckoAppShell"); >+ let method = jni.getStaticMethodID(cls, "getPreferredIconSize", "()I"); >+ iconSize = jni.callStaticIntMethod(cls, method, null); >+ jni.close(); I don't think you can make any changes here based on your updated patch in bug 787271, but I wanted you to check to be sure.
Attachment #657081 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Comment 19•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/272eede695e4
Comment 21•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/272eede695e4
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Reporter | ||
Comment 22•12 years ago
|
||
I'm noticing not much of a difference with this patch and check-in. I tried Twitter/Lanyrd/Jauntly and all were still jagged and pixelated on m-c (09/13), Galaxy Nexus (4.1.1).
Assignee | ||
Comment 23•12 years ago
|
||
Grr.. I made a last minute change in the jni stuff and missed something (the "jsjni_" mark wanted on the front of the methods). I would guess there's an error in the logcat. I'll throw up a follow up.....
Assignee | ||
Comment 24•12 years ago
|
||
Sorry 'bout that. Tested again.
Attachment #660912 -
Flags: review?(mark.finkle)
Updated•12 years ago
|
Attachment #660912 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Comment 25•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d6adbfe2970
Reporter | ||
Updated•12 years ago
|
Assignee | ||
Comment 27•12 years ago
|
||
Comment on attachment 657081 [details] [diff] [review] Patch v2 [Approval Request Comment] Bug caused by (feature/regressing bug #): Been around forever User impact if declined: Webapp shortcuts looks bad Testing completed (on m-c, etc.): Landed on mc this week Risk to taking this patch (and alternatives if risky): Pretty low risk, but uses the new ctypes stuff from bug 787271 String or UUID changes made by this patch: None.
Attachment #657081 -
Flags: approval-mozilla-aurora?
Comment 28•12 years ago
|
||
Comment on attachment 657081 [details] [diff] [review] Patch v2 [Triage Comment] Approving given the upcoming Marketplace beta and where we are in the cycle.
Attachment #657081 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 29•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/974494c2de32 https://hg.mozilla.org/releases/mozilla-aurora/rev/1f82974dc08c
Updated•12 years ago
|
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•