1200343 - Convert pref events to native calls

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Comment 1

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

Attachment: Add JNI wrapper for object arrays (v1)

This patch adds a specialization for jni::Ref<jni::ObjectArray>, which
includes members for getting the length of the array and accessing
array elements.

Comment 2

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

Attachment: Add native calls for pref events (v1)

This patch adds two native calls to PrefsHelper to avoid using
GeckoEvent for getting prefs.

Comment 3

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

Attachment: Remove pref events from GeckoEvent (v1)

Now that we use native calls, we can remove the pref-related events from
GeckoEvent.

Comment 4

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

Attachment: Implment PrefsHelper native calls (v1)

This patch implements the PrefsHelper native calls for getting prefs;

Comment 5

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

Comment on attachment 8663011 [details] [diff] [review]
Add JNI wrapper for object arrays (v1)

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

nice

Comment 6

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

Comment on attachment 8663012 [details] [diff] [review]
Add native calls for pref events (v1)

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

Shouldn't the implementations of the new native calls be here somewhere?

Comment 7

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

Comment on attachment 8663014 [details] [diff] [review]
Implment PrefsHelper native calls (v1)

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

Looks good if you fix the one issue

::: widget/android/PrefsHelper.h
@@ +50,5 @@
> +        nsIAndroidBrowserApp* browserApp = nullptr;
> +        nsAppShell::gAppShell->GetBrowserApp(&browserApp);
> +        MOZ_ASSERT(browserApp);
> +
> +        if (observe) {

Looks like you have the logic or the bodies reversed here

Comment 8

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

Comment on attachment 8663012 [details] [diff] [review]
Add native calls for pref events (v1)

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

I see you added the impl in a later patch, but I would just put it here, otherwise this patch is just busted on its own.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/905e1fb54fd4
https://hg.mozilla.org/integration/mozilla-inbound/rev/e115ca114601
https://hg.mozilla.org/integration/mozilla-inbound/rev/b077acafcebb
https://hg.mozilla.org/integration/mozilla-inbound/rev/67dc42cbf02b

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I've been seeing failures like https://treeherder.mozilla.org/logviewer.html#?job_id=14729097&repo=mozilla-inbound ever since this landed. 

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/80494cf1eaba

Comment 11

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

Attachment: Add native calls for pref events (v2)

The problem was BaseTest.setPreferenceAndWaitForChange was calling the new
JNI pref methods before Gecko was ready. Blocking for Gecko ready should
fix the issue.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa9159ef5db5
https://hg.mozilla.org/integration/mozilla-inbound/rev/77b79a961f7e
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fc693d5b495
https://hg.mozilla.org/integration/mozilla-inbound/rev/51cc57918bf5

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fa9159ef5db5
https://hg.mozilla.org/mozilla-central/rev/77b79a961f7e
https://hg.mozilla.org/mozilla-central/rev/0fc693d5b495
https://hg.mozilla.org/mozilla-central/rev/51cc57918bf5