Last Comment Bug 783921 - Application shortcuts wrongly scaled? (DPI)
: Application shortcuts wrongly scaled? (DPI)
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: Web Apps (show other bugs)
: Trunk
: ARM Android
: P1 normal (vote)
: Firefox 18
Assigned To: Wesley Johnston (:wesj)
: Aaron Train [:aaronmt]
:
Mentors:
: 785450 786051 (view as bug list)
Depends on: 787271
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-19 16:25 PDT by Aaron Train [:aaronmt]
Modified: 2012-09-20 14:28 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
verified


Attachments
Nightly (08/19) - Screenshot (123.43 KB, image/png)
2012-08-19 16:25 PDT, Aaron Train [:aaronmt]
no flags Details
Nightly (08/19) - Screenshot (349.71 KB, image/png)
2012-08-19 16:26 PDT, Aaron Train [:aaronmt]
no flags Details
Samsung Nexus S (HDPI) - Screenshot (79.00 KB, image/png)
2012-08-21 07:32 PDT, Aaron Train [:aaronmt]
no flags Details
Motorola Droid Pro (MDPI) - Screenshot (34.16 KB, image/png)
2012-08-21 07:55 PDT, Aaron Train [:aaronmt]
no flags Details
WIP Patch (10.93 KB, patch)
2012-08-21 14:47 PDT, Wesley Johnston (:wesj)
mark.finkle: feedback+
Details | Diff | Splinter Review
Screenshot (73.50 KB, image/png)
2012-08-27 12:08 PDT, Wesley Johnston (:wesj)
no flags Details
Patch (10.34 KB, patch)
2012-08-27 12:54 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
WIP Patch v2 (25.82 KB, patch)
2012-08-29 17:07 PDT, Wesley Johnston (:wesj)
mark.finkle: feedback+
Details | Diff | Splinter Review
Patch v2 (9.04 KB, patch)
2012-08-30 17:06 PDT, Wesley Johnston (:wesj)
mark.finkle: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Follow up Patch (4.56 KB, patch)
2012-09-13 11:22 PDT, Wesley Johnston (:wesj)
mark.finkle: review+
Details | Diff | Splinter Review

Description Aaron Train [:aaronmt] 2012-08-19 16:25:25 PDT
Created attachment 653214 [details]
Nightly (08/19) - Screenshot

See screenshot.

--
Nightly (08/19)
Samsung Galaxy Nexus (Android 4.1.1)
Comment 1 Aaron Train [:aaronmt] 2012-08-19 16:26:07 PDT
Created attachment 653215 [details]
Nightly (08/19) - Screenshot
Comment 2 Aaron Train [:aaronmt] 2012-08-19 16:30:11 PDT
createShortcut(manifest.name, manifest.fullLaunchPath(), manifest.iconURLForSize("64"), "webapp");

Can we use 128 if available?
Comment 3 Jason Smith [:jsmith] 2012-08-19 16:51:35 PDT
(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.
Comment 4 Aaron Train [:aaronmt] 2012-08-19 16:54:19 PDT
Is getBiggestIcon working correctly here?

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#6014
Comment 5 Wesley Johnston (:wesj) 2012-08-19 18:29:11 PDT
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
Comment 6 Aaron Train [:aaronmt] 2012-08-20 13:27:14 PDT
Note to test: what happens on an HDPI/MDPI device
Comment 7 Aaron Train [:aaronmt] 2012-08-21 07:32:38 PDT
Created attachment 653748 [details]
Samsung Nexus S (HDPI) - Screenshot

Slightly different but still affected here on an HDPI device like the Samsung Nexus S.
Comment 8 Aaron Train [:aaronmt] 2012-08-21 07:55:05 PDT
Created attachment 653755 [details]
Motorola Droid Pro (MDPI) - Screenshot

Same deal with MDPI (Droid Pro)
Comment 9 Wesley Johnston (:wesj) 2012-08-21 11:50:43 PDT
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.
Comment 10 Wesley Johnston (:wesj) 2012-08-21 14:47:26 PDT
Created attachment 653964 [details] [diff] [review]
WIP Patch

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.
Comment 11 Mark Finkle (:mfinkle) (use needinfo?) 2012-08-21 22:08:43 PDT
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
Comment 12 Tom Lowenthal [:StrangeCharm] 2012-08-24 14:59:54 PDT
*** Bug 785450 has been marked as a duplicate of this bug. ***
Comment 13 Wesley Johnston (:wesj) 2012-08-27 12:08:19 PDT
Created attachment 655692 [details]
Screenshot

(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.
Comment 14 Wesley Johnston (:wesj) 2012-08-27 12:54:27 PDT
Created attachment 655715 [details] [diff] [review]
Patch

Actually, this is basically the same patch with the unnecessary import removed.
Comment 15 Wesley Johnston (:wesj) 2012-08-29 17:07:55 PDT
Created attachment 656679 [details] [diff] [review]
WIP Patch v2

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.
Comment 16 Mark Finkle (:mfinkle) (use needinfo?) 2012-08-30 07:59:01 PDT
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.
Comment 17 Wesley Johnston (:wesj) 2012-08-30 17:06:22 PDT
Created attachment 657081 [details] [diff] [review]
Patch v2

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.
Comment 18 Mark Finkle (:mfinkle) (use needinfo?) 2012-09-05 08:28:27 PDT
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.
Comment 20 Wesley Johnston (:wesj) 2012-09-12 11:36:07 PDT
*** Bug 786051 has been marked as a duplicate of this bug. ***
Comment 21 Ryan VanderMeulen [:RyanVM] 2012-09-12 18:16:44 PDT
https://hg.mozilla.org/mozilla-central/rev/272eede695e4
Comment 22 Aaron Train [:aaronmt] 2012-09-13 06:59:55 PDT
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).
Comment 23 Wesley Johnston (:wesj) 2012-09-13 07:10:51 PDT
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.....
Comment 24 Wesley Johnston (:wesj) 2012-09-13 11:22:03 PDT
Created attachment 660912 [details] [diff] [review]
Follow up Patch

Sorry 'bout that. Tested again.
Comment 26 Ryan VanderMeulen [:RyanVM] 2012-09-13 18:56:59 PDT
https://hg.mozilla.org/mozilla-central/rev/1d6adbfe2970
Comment 27 Wesley Johnston (:wesj) 2012-09-18 10:30:15 PDT
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.
Comment 28 Alex Keybl [:akeybl] 2012-09-19 17:25:21 PDT
Comment on attachment 657081 [details] [diff] [review]
Patch v2

[Triage Comment]
Approving given the upcoming Marketplace beta and where we are in the cycle.

Note You need to log in before you can comment on or make changes to this bug.