Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Application shortcuts wrongly scaled? (DPI)

VERIFIED FIXED in Firefox 17

Status

()

Firefox for Android
Web Apps
P1
normal
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: aaronmt, Assigned: wesj)

Tracking

Trunk
Firefox 18
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox17 fixed, firefox18 verified)

Details

Attachments

(6 attachments, 4 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 653214 [details]
Nightly (08/19) - Screenshot

See screenshot.

--
Nightly (08/19)
Samsung Galaxy Nexus (Android 4.1.1)
(Reporter)

Updated

5 years ago
Attachment #653214 - Attachment is obsolete: true
(Reporter)

Comment 1

5 years ago
Created attachment 653215 [details]
Nightly (08/19) - Screenshot
(Reporter)

Comment 2

5 years ago
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.
(Reporter)

Comment 4

5 years ago
Is getBiggestIcon working correctly here?

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#6014
(Assignee)

Comment 5

5 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

5 years ago
Priority: -- → P1
(Reporter)

Comment 6

5 years ago
Note to test: what happens on an HDPI/MDPI device
(Reporter)

Comment 7

5 years ago
Created attachment 653748 [details]
Samsung Nexus S (HDPI) - Screenshot

Slightly different but still affected here on an HDPI device like the Samsung Nexus S.
(Reporter)

Comment 8

5 years ago
Created attachment 653755 [details]
Motorola Droid Pro (MDPI) - Screenshot

Same deal with MDPI (Droid Pro)
(Assignee)

Comment 9

5 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

5 years ago
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.
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+

Updated

5 years ago
Duplicate of this bug: 785450
(Assignee)

Comment 13

5 years ago
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.
(Assignee)

Comment 14

5 years ago
Created attachment 655715 [details] [diff] [review]
Patch

Actually, this is basically the same patch with the unnecessary import removed.
Assignee: nobody → wjohnston
Attachment #653964 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #655715 - Flags: review?(mark.finkle)
(Assignee)

Comment 15

5 years ago
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.
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+
(Assignee)

Comment 17

5 years ago
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.
Attachment #656679 - Attachment is obsolete: true
Attachment #657081 - Flags: review?(mark.finkle)
Depends on: 787271
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

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/272eede695e4
(Assignee)

Updated

5 years ago
Duplicate of this bug: 786051
https://hg.mozilla.org/mozilla-central/rev/272eede695e4
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
(Reporter)

Comment 22

5 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

5 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

5 years ago
Created attachment 660912 [details] [diff] [review]
Follow up Patch

Sorry 'bout that. Tested again.
Attachment #660912 - Flags: review?(mark.finkle)
Attachment #660912 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 25

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d6adbfe2970
https://hg.mozilla.org/mozilla-central/rev/1d6adbfe2970
(Reporter)

Updated

5 years ago
Status: RESOLVED → VERIFIED
status-firefox17: --- → affected
status-firefox18: --- → verified
(Assignee)

Comment 27

5 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 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

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/974494c2de32
https://hg.mozilla.org/releases/mozilla-aurora/rev/1f82974dc08c

Updated

5 years ago
status-firefox17: affected → fixed
You need to log in before you can comment on or make changes to this bug.