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)

ARM
Android
defect

Tracking

(firefox17 fixed, firefox18 verified)

VERIFIED FIXED
Firefox 18
Tracking Status
firefox17 --- fixed
firefox18 --- verified

People

(Reporter: aaronmt, Assigned: wesj)

References

Details

Attachments

(6 files, 4 obsolete files)

Attached image Nightly (08/19) - Screenshot (obsolete) —
See screenshot.

--
Nightly (08/19)
Samsung Galaxy Nexus (Android 4.1.1)
Attachment #653214 - Attachment is obsolete: true
createShortcut(manifest.name, manifest.fullLaunchPath(), manifest.iconURLForSize("64"), "webapp");

Can we use 128 if available?
(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.
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
Priority: -- → P1
Note to test: what happens on an HDPI/MDPI device
Slightly different but still affected here on an HDPI device like the Samsung Nexus S.
Same deal with MDPI (Droid Pro)
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.
Attached patch WIP Patch (obsolete) — Splinter Review
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 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+
Attached image 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.
Attached patch Patch (obsolete) — Splinter Review
Actually, this is basically the same patch with the unnecessary import removed.
Assignee: nobody → wjohnston
Attachment #653964 - Attachment is obsolete: true
Attachment #655715 - Flags: review?(mark.finkle)
Attached patch WIP Patch v2 (obsolete) — Splinter Review
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 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+
Attached patch Patch v2Splinter Review
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 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+
https://hg.mozilla.org/mozilla-central/rev/272eede695e4
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
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).
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.....
Attached patch Follow up PatchSplinter Review
Sorry 'bout that. Tested again.
Attachment #660912 - Flags: review?(mark.finkle)
Attachment #660912 - Flags: review?(mark.finkle) → review+
Status: RESOLVED → VERIFIED
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 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+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: