1192082 - Let nsAppShell manage JNI/AndroidBridge lifetime

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

I think nsAppShell should manage initializing and deinitializing JNI and AndroidBridge. A lot of AndroidBridge will likely be merged into nsAppShell anyways.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Expose GeckoThread states to C++; r=me

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 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.

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Expose AndroidBridge arguments through GeckoThread (v1)

AndroidBridge needed some arguments during its initialization. We'll
provide those arguments in GeckoThread, which AndroidBridge will access.

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 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.

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 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.

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

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?

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

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*

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

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.

Comment 9

[Pulsebot]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 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.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebf81c68ff44

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ebf81c68ff44