Closed
Bug 830935
Opened 12 years ago
Closed 12 years ago
need a way to get Fennec's Activity Context from C++
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 23
People
(Reporter: dmosedale, Assigned: dmosedale)
References
Details
(Whiteboard: [android-trunk-needed][android-gum+][qa-])
Attachments
(1 file, 8 obsolete files)
|
(deleted),
patch
|
blassey
:
review+
dmosedale
:
feedback+
|
Details | Diff | Splinter Review |
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.
|
||
Comment 1•12 years ago
|
||
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/
|
Assignee | |
Comment 2•12 years ago
|
||
kats: indeed; good catch. Will fix in next iteration.
|
|
Assignee | |
Comment 3•12 years ago
|
||
Rebased to mozilla-central & alder, kats' fix incorporated (thanks for catching that!).
Attachment #702467 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 4•12 years ago
|
||
Whoops; trying again.
Attachment #706458 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 6•12 years ago
|
||
Attachment #706469 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 7•12 years ago
|
||
Attachment #706476 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 8•12 years ago
|
||
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 | |
Updated•12 years ago
|
Assignee: dmose → nobody
|
|
Assignee | |
Comment 9•12 years ago
|
||
Has changes from GCP to make it work on any thread and also not crash.
Attachment #706617 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 10•12 years ago
|
||
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
|
|
Assignee | |
Comment 11•12 years ago
|
||
Landed on alder.
https://hg.mozilla.org/projects/alder/rev/1a98aaa40ce1
Next steps: rebase to trunk and request review.
Whiteboard: [android-trunk-needed]
|
|
Assignee | |
Updated•12 years ago
|
Attachment #712193 -
Attachment description: add GetContext to AndroidBridge, v8 → add GetContext to AndroidBridge, v8 (landed on alder)
|
|
Assignee | |
Updated•12 years ago
|
Whiteboard: [android-trunk-needed] → [android-trunk-needed][android-gum+]
|
||
Comment 12•12 years ago
|
||
Rebased to m-c.
Attachment #712193 -
Attachment is obsolete: true
Attachment #729068 -
Flags: review?(blassey.bugs)
Attachment #729068 -
Flags: feedback?(dmose)
|
|
Assignee | |
Comment 13•12 years ago
|
||
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 14•12 years ago
|
||
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+
|
|
||
Comment 15•12 years ago
|
||
|
||
Comment 16•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
|
||
Updated•12 years ago
|
Whiteboard: [android-trunk-needed][android-gum+] → [android-trunk-needed][android-gum+][qa-]
|
|
||
Updated•12 years ago
|
Blocks: android-webrtc
|
||
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•