Let nsAppShell manage JNI/AndroidBridge lifetime

RESOLVED FIXED in Firefox 43

Status

()

Core
Widget: Android
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jchen, Assigned: jchen)

Tracking

unspecified
mozilla43
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(6 attachments)

(Assignee)

Description

2 years ago
I think nsAppShell should manage initializing and deinitializing JNI and AndroidBridge. A lot of AndroidBridge will likely be merged into nsAppShell anyways.
(Assignee)

Comment 1

2 years ago
Created attachment 8644697 [details] [diff] [review]
Expose GeckoThread states to C++; r=me
Attachment #8644697 - Flags: review+
(Assignee)

Comment 2

2 years ago
Created attachment 8644698 [details] [diff] [review]
Get rid of GeckoAppShell.nativeInit (v1)

Instead of letting AndroidBridge be constructed separately, we'll let
nsAppShell construct AndroidBridge. We'll also make AndroidBridge fetch
the init arguments from GeckoThread.
Attachment #8644698 - Flags: review?(snorp)
(Assignee)

Comment 3

2 years ago
Created attachment 8644699 [details] [diff] [review]
Expose AndroidBridge arguments through GeckoThread (v1)

AndroidBridge needed some arguments during its initialization. We'll
provide those arguments in GeckoThread, which AndroidBridge will access.
Attachment #8644699 - Flags: review?(snorp)
(Assignee)

Comment 4

2 years ago
Created attachment 8644700 [details] [diff] [review]
De-clutter AndroidBridge init/deinit (v1)

Merge all the init code into AndroidBridge constructor and
AndroidBridge::ConstructBridge; merge all the deinit code into
AndroidBridge destructor and AndroidBridge::DeconstructBridge.

In particular, the SetMainThread call is obsolete and removed.
Attachment #8644700 - Flags: review?(snorp)
(Assignee)

Comment 5

2 years ago
Created attachment 8644701 [details] [diff] [review]
Iniialize/deinitialize JNI in nsAppShell (v1)

First we need to set the Gecko thread JNIEnv* in nsAndroidStartup, but
after that we can initialize and deinitialize the rest of JNI, including
AndroidBridge, in GeckoAppShell. This makes nsAppShell control the
AndroidBridge lifetime. Over time, parts of the AndroidBridge
functionality will be migrated to nsAppShell.
Attachment #8644701 - Flags: review?(snorp)
Comment on attachment 8644698 [details] [diff] [review]
Get rid of GeckoAppShell.nativeInit (v1)

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

Yay! Are we sure we can construct it early enough?
Attachment #8644698 - Flags: review?(snorp) → review+
Comment on attachment 8644699 [details] [diff] [review]
Expose AndroidBridge arguments through GeckoThread (v1)

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

::: widget/android/GeneratedJNIWrappers.cpp
@@ +775,5 @@
>  
> +constexpr char GeckoThread::ClsLoader_t::name[];
> +constexpr char GeckoThread::ClsLoader_t::signature[];
> +
> +auto GeckoThread::ClsLoader() -> mozilla::jni::Object::LocalRef

My understanding is that the 'auto foo() -> return_type_t' syntax is only better if you declare the return type within the function. Here that's not the case, and it seems harder to read. *shrug*
Attachment #8644699 - Flags: review?(snorp) → review+
Attachment #8644700 - Flags: review?(snorp) → review+
Comment on attachment 8644701 [details] [diff] [review]
Iniialize/deinitialize JNI in nsAppShell (v1)

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

::: widget/android/nsAppShell.cpp
@@ +152,5 @@
> +
> +    if (!jni::IsAvailable()) {
> +        return;
> +    }
> +    AndroidBridge::ConstructBridge();

We should probably construct the bridge first thing, in case the PowerManagerService above needs it.
Attachment #8644701 - Flags: review?(snorp) → review+

Comment 9

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e04ced85c87f
https://hg.mozilla.org/integration/mozilla-inbound/rev/361c14b70ac2
https://hg.mozilla.org/integration/mozilla-inbound/rev/489fd24eea7b
https://hg.mozilla.org/integration/mozilla-inbound/rev/250e5f0da247
https://hg.mozilla.org/integration/mozilla-inbound/rev/2302cc77b400
https://hg.mozilla.org/mozilla-central/rev/e04ced85c87f
https://hg.mozilla.org/mozilla-central/rev/361c14b70ac2
https://hg.mozilla.org/mozilla-central/rev/489fd24eea7b
https://hg.mozilla.org/mozilla-central/rev/250e5f0da247
https://hg.mozilla.org/mozilla-central/rev/2302cc77b400
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
(Assignee)

Comment 11

2 years ago
Created attachment 8647629 [details] [diff] [review]
Followup to iniialize/deinitialize JNI in nsAppShell

Forgot to include this patch which was meant to address comment 8.
I will push it as a follow up.
Attachment #8647629 - Flags: review+

Comment 12

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebf81c68ff44
https://hg.mozilla.org/mozilla-central/rev/ebf81c68ff44
You need to log in before you can comment on or make changes to this bug.