890253 - Change widget/android/nsWindow.cpp GetDefaultScaleInternal() to use actual device DPI

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Rather than using heuristics to determine the widget scale (currently we return 1.0, 1.5, or 2.0 depending on DPI) we should just pull the screen density from the device and use that directly.

See the patch at https://bugzilla.mozilla.org/show_bug.cgi?id=803207#c2 and comment 9/comment 11.

Comment 1

[Ming]

Hi Matt,

I would like to help with this bug. I have briefly look at the code and below is my thought on how this might be done. I am pretty new to fennec(this is my second bug), so please give me more info if I am totally wrong.

1: adding a "getDensity" function in AndroidBridge.h/cpp
2: adding "getDensity" function in GeckoAppShell.java
3: call "Bridge()->GetDensity()" from "GetDefaultScaleInternal()" and return the DisplayMetrics.density value.

Thanks,
Ming

Comment 2

[Matt Brubeck (:mbrubeck)]

(In reply to Ming from comment #1)
> 1: adding a "getDensity" function in AndroidBridge.h/cpp
> 2: adding "getDensity" function in GeckoAppShell.java
> 3: call "Bridge()->GetDensity()" from "GetDefaultScaleInternal()" and return
> the DisplayMetrics.density value.

Yes, this looks like the correct approach.

Comment 3

[Mina Almasry]

Attachment: Patch

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 806017 [details] [diff] [review]
Patch

Review of attachment 806017 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoAppShell.java
@@ +1347,5 @@
>  
>          return sDensityDpi;
>      }
>  
> +    @GeneratableAndroidBridgeTarget(stubName = "GetDensityWrapper")

I would just not bother with setting the stubName here; that stuff is mostly for backwards compatibility when the annotations code was added. Just let it generate a function with the signature GetDensity which is what it should do if you don't specify the stubName

@@ +1349,5 @@
>      }
>  
> +    @GeneratableAndroidBridgeTarget(stubName = "GetDensityWrapper")
> +    public static float getDensity() {
> +        return getContext().getResources().getDisplayMetrics().density;

We should cache the result of this function the same way getDpi does it, because this will get called a lot. Or you could even cache it in nsWindow::GetDefaultScaleInternal

::: widget/android/AndroidBridge.cpp
@@ +478,5 @@
>  
> +float
> +AndroidBridge::GetDensity()
> +{
> +  return GetDensityWrapper();

This function should not be needed if you remove the stubName. (But if for whatever reason it stays, please indent it to 4 spaces for consistency with the rest of the file)

::: widget/android/AndroidBridge.h
@@ +213,5 @@
>                                 nsIObserver *aAlertListener,
>                                 const nsAString& aAlertName);
>  
>      int GetDPI();
> +    jfloat GetDensity();

This should go away as well, but "jfloat" -> "float" otherwise

::: widget/android/GeneratedJNIWrappers.h
@@ +108,5 @@
>  
> +// GENERATED CODE
> +// Generated by the Java program at /build/annotationProcessors at compile time from
> +// annotations on Java methods. To update, change the annotations on the corresponding Java
> +// methods and rerun the build. Manually updating this file will cause your build to fail.

Was this file generated with the comment moved? If so that seems like a bug in the generator that we (or ckitching) should fix.

::: widget/android/nsWindow.cpp
@@ +340,5 @@
>  double
>  nsWindow::GetDefaultScaleInternal()
>  {
> +    if (AndroidBridge::Bridge())
> +        return AndroidBridge::Bridge()->GetDensity();

nit: please use braces for single-line ifs here.

Comment 5

[Mina Almasry]

Attachment: Patch v2

Comments addressed and try push: https://tbpl.mozilla.org/?tree=Try&rev=3ed59e799337

Comment 6

[Matt Brubeck (:mbrubeck)]

Comment on attachment 806170 [details] [diff] [review]
Patch v2

Review of attachment 806170 [details] [diff] [review]:
-----------------------------------------------------------------

Note: We don't run automated tests on Android debug builds, so I recommend pushing to Try a second time with "try: -b o -p android -u all -t all".

The patch looks good to me with one minor nit (below).  But I'm going to request a second review from kats who knows this code better than I do.

::: widget/android/nsWindow.cpp
@@ +351,2 @@
>      }
> +    else {

Could you replace "else" with "if (!density)" here?  Then it will also default to 1.0 in the case where AndroidBridge::GetDensity fails for any reason.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 806170 [details] [diff] [review]
Patch v2

Review of attachment 806170 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, with mbrubeck's nit addressed.

Comment 8

[Mina Almasry]

Attachment: Patch v3

Comments addressed. Marking checkin needed. Thanks!

Comment 9

[Mina Almasry]

Optimized try push: https://tbpl.mozilla.org/?tree=Try&rev=ea449cbea9c1

and filed bug 917608 for the moved comment in GeneratedJNIWrappers.h

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f88169309d8c

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f88169309d8c