Closed Bug 890253 Opened 7 years ago Closed 6 years ago

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

Categories

(Core :: Widget: Android, defect)

24 Branch
All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: kats, Assigned: almasry.mina)

Details

(Whiteboard: [mentor=mbrubeck@mozilla.com][good first bug][lang=c++][lang=java])

Attachments

(1 file, 2 obsolete files)

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.
Whiteboard: [mentor=mbrubeck@mozilla.com][good first bug][lang=c++][lang=java]
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
Flags: needinfo?(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.
Flags: needinfo?(mbrubeck)
Assignee: nobody → malmasry
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #806017 - Flags: feedback+
Attached patch Patch v2 (obsolete) — Splinter Review
Comments addressed and try push: https://tbpl.mozilla.org/?tree=Try&rev=3ed59e799337
Attachment #806017 - Attachment is obsolete: true
Attachment #806170 - Flags: review?(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.
Attachment #806170 - Flags: review?(mbrubeck)
Attachment #806170 - Flags: review?(bugmail.mozilla)
Attachment #806170 - Flags: review+
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.
Attachment #806170 - Flags: review?(bugmail.mozilla) → review+
Attached patch Patch v3Splinter Review
Comments addressed. Marking checkin needed. Thanks!
Attachment #806170 - Attachment is obsolete: true
Keywords: checkin-needed
Optimized try push: https://tbpl.mozilla.org/?tree=Try&rev=ea449cbea9c1

and filed bug 917608 for the moved comment in GeneratedJNIWrappers.h
https://hg.mozilla.org/mozilla-central/rev/f88169309d8c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.