Last Comment Bug 740190 - Screen Orientation API: implement locking in Android
: Screen Orientation API: implement locking in Android
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Android (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Mounir Lamouri (:mounir)
:
:
Mentors:
Depends on: 757791
Blocks: 740188
  Show dependency treegraph
 
Reported: 2012-03-28 16:00 PDT by Mounir Lamouri (:mounir)
Modified: 2012-06-21 03:22 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (9.70 KB, patch)
2012-03-28 16:00 PDT, Mounir Lamouri (:mounir)
doug.turner: review+
Details | Diff | Splinter Review
Part 2 - unlock function (7.29 KB, patch)
2012-03-28 21:09 PDT, Mounir Lamouri (:mounir)
doug.turner: review+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2012-03-28 16:00:55 PDT
Created attachment 610338 [details] [diff] [review]
Patch v1
Comment 1 Doug Turner (:dougt) 2012-03-28 17:14:41 PDT
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) 2012-03-28 21:09:33 PDT
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.
Comment 3 Mounir Lamouri (:mounir) 2012-03-28 21:18:28 PDT
(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) 2012-03-28 22:05:11 PDT
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) 2012-03-30 09:39:05 PDT
https://hg.mozilla.org/mozilla-central/rev/8bf3120ed0f8

Note You need to log in before you can comment on or make changes to this bug.