740190 - Screen Orientation API: implement locking in Android

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Comment 1

[Doug Turner (:dougt)]

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).

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - unlock function

I've added an unlock function. Pretty basic patch. I will merge it with the other one before landing.

Comment 3

[Mounir Lamouri (:mounir)]

(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

[Doug Turner (:dougt)]

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

Comment 5

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/8bf3120ed0f8