Closed Bug 740190 Opened 8 years ago Closed 8 years ago

Screen Orientation API: implement locking in Android


(Core :: Widget: Android, defect)

Not set





(Reporter: mounir, Assigned: mounir)




(2 files)

Attached patch Patch v1Splinter Review
No description provided.
Attachment #610338 - Flags: review?(doug.turner)
Whiteboard: [needs review]
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)

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+
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)
(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 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+
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [needs review]
Target Milestone: --- → mozilla14
Depends on: 757791
Depends on: 764753
No longer depends on: 764753
You need to log in before you can comment on or make changes to this bug.