Screen Orientation API: implement locking in Android

RESOLVED FIXED in mozilla14

Status

()

Core
Widget: Android
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

Trunk
mozilla14
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

6 years ago
Created attachment 610338 [details] [diff] [review]
Patch v1
Attachment #610338 - Flags: review?(doug.turner)
(Assignee)

Updated

6 years ago
Whiteboard: [needs review]

Comment 1

6 years ago
Comment on attachment 610338 [details] [diff] [review]
Patch v1

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

You can go as is - but do file a follow up about the inconsistency in AndroidBridge.cpp.  Assign to me or blassey.  Or better yet, assign it to yourself and fix it ;)

::: widget/android/AndroidBridge.cpp
@@ +2108,5 @@
> +void
> +AndroidBridge::LockScreenOrientation(const dom::ScreenOrientationWrapper& aOrientation)
> +{
> +  ALOG_BRIDGE("AndroidBridge::LockScreenOrientation");
> +  mJNIEnv->CallStaticVoidMethod(mGeckoAppShellClass, jLockScreenOrientation, aOrientation.orientation);

JNIEnv *env = AndroidBridge::GetJNIEnv();
if (!env)
  return;

AutoLocalJNIFrame jniFrame(env, 1);


You want to be using ^^ to get the jni env.  The other instances of this mistake in this file should be corrected (since you are there).
Attachment #610338 - Flags: review?(doug.turner) → review+
(Assignee)

Comment 2

6 years ago
Created attachment 610419 [details] [diff] [review]
Part 2 - unlock function

I've added an unlock function. Pretty basic patch. I will merge it with the other one before landing.
Attachment #610419 - Flags: review?(doug.turner)
(Assignee)

Comment 3

6 years ago
(In reply to Doug Turner (:dougt) from comment #1)
> JNIEnv *env = AndroidBridge::GetJNIEnv();
> if (!env)
>   return;
>
> AutoLocalJNIFrame jniFrame(env, 1);

I've been able to find a bug (bug 713803) that explains quite clearly why the three first lines. However, I do not understand why we need to call AutoLocalJNIFrame every time. I would be happy to write the patch but I would prefer to understand what I'm doing ;)

Comment 4

6 years ago
Comment on attachment 610419 [details] [diff] [review]
Part 2 - unlock function

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

Fix or follow up w/ a bug

::: widget/android/AndroidBridge.cpp
@@ +2116,5 @@
> +void
> +AndroidBridge::UnlockScreenOrientation()
> +{
> +  ALOG_BRIDGE("AndroidBridge::UnlockScreenOrientation");
> +  mJNIEnv->CallStaticVoidMethod(mGeckoAppShellClass, jUnlockScreenOrientation);

same thing here.  you don't want to use mJNIEnv directly.  Also you want to add a java stack frame like the other calls to CallStatic*Method
Attachment #610419 - Flags: review?(doug.turner) → review+
(Assignee)

Comment 5

6 years ago
https://hg.mozilla.org/mozilla-central/rev/8bf3120ed0f8
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [needs review]
Target Milestone: --- → mozilla14

Updated

5 years ago
Depends on: 757791
(Assignee)

Updated

5 years ago
Depends on: 764753
(Assignee)

Updated

5 years ago
No longer depends on: 764753
You need to log in before you can comment on or make changes to this bug.