Closed Bug 830935 Opened 7 years ago Closed 7 years ago

need a way to get Fennec's Activity Context from C++

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 23

People

(Reporter: dmose, Assigned: dmose)

References

(Blocks 1 open bug)

Details

(Whiteboard: [android-trunk-needed][android-gum+][qa-])

Attachments

(1 file, 8 obsolete files)

The C++ WebRTC video engine calls through JNI into some of its own Java code for various dealings with the hardware.  To do this, it requires the Activity Context to be set from C++.

Wes and I put together a WIP patch yesterday to do it, which still needs some cleanup and testing.
Blocks: 830942
Comment on attachment 702467 [details] [diff] [review]
add GetContext method to AndroidBridge and friends

>diff --git a/widget/android/AndroidBridge.cpp b/widget/android/AndroidBridge.cpp
>+jobject
>+AndroidBridge::GetContext() {
>+    JNIEnv *env = GetJNIEnv();
>+    if (!env)
>+        return 0;
>+
>+    AutoLocalJNIFrame jniFrame(env, 0);
>+
>+    jobject ret = env->CallStaticObjectMethod(mGeckoAppShellClass, jGetGfxInfoData);

Copypasta error? s/jGetGfxInfoData/jGetContext/
kats: indeed; good catch.  Will fix in next iteration.
Rebased to mozilla-central & alder, kats' fix incorporated (thanks for catching that!).
Attachment #702467 - Attachment is obsolete: true
Whoops; trying again.
Attachment #706458 - Attachment is obsolete: true
One more time!
Attachment #706459 - Attachment is obsolete: true
Attachment #706469 - Attachment is obsolete: true
Attachment #706476 - Attachment is obsolete: true
Since the natural place for this to be called in the WebRTC code is from the MediaManager thread, this patch probably wants to be changed to call GetJNIForThread instead of GetJNIEnv.

Is an android context itself a thread-specific thing?  Or can that be handed off to other threads without fear?
Assignee: dmose → nobody
Has changes from GCP to make it work on any thread and also not crash.
Attachment #706617 - Attachment is obsolete: true
Removes the MOZ_WEBRTC ifdef, as this doesn't really want to be conditionalized that way.  Changes the method and name and header comments so that we're now returning a globalized reference to the Context which the callers needs to ensure is disposed of by calling DeleteGlobalRef so that it doesn't leak.
Assignee: nobody → dmose
Attachment #708775 - Attachment is obsolete: true
Landed on alder. 

https://hg.mozilla.org/projects/alder/rev/1a98aaa40ce1

Next steps: rebase to trunk and request review.
Whiteboard: [android-trunk-needed]
Attachment #712193 - Attachment description: add GetContext to AndroidBridge, v8 → add GetContext to AndroidBridge, v8 (landed on alder)
Whiteboard: [android-trunk-needed] → [android-trunk-needed][android-gum+]
Rebased to m-c.
Attachment #712193 - Attachment is obsolete: true
Attachment #729068 - Flags: review?(blassey.bugs)
Attachment #729068 - Flags: feedback?(dmose)
Comment on attachment 729068 [details] [diff] [review]
Patch 1. add GetContext to AndroidBridge

Looks good to me; feedback+.  Bonus points if you add a unit test.
Attachment #729068 - Flags: feedback?(dmose) → feedback+
Comment on attachment 729068 [details] [diff] [review]
Patch 1. add GetContext to AndroidBridge

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

::: widget/android/AndroidBridge.cpp
@@ +2074,5 @@
> +    jobject globalRef = env->NewGlobalRef(context);
> +    MOZ_ASSERT(globalRef);
> +
> +    // we've got our global reference; free the local one
> +    env->DeleteLocalRef(context);

you don't need to delete the ref explicitly since you have an AndroidJNIFrame.
Attachment #729068 - Flags: review?(blassey.bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/b76af038db2a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Whiteboard: [android-trunk-needed][android-gum+] → [android-trunk-needed][android-gum+][qa-]
You need to log in before you can comment on or make changes to this bug.