1243049 - Move prefs handling to outside of browser.js

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

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

Right now we handle prefs inside browser.js, which means we need a GeckoView with browser.js running for prefs for work. If we don't have a GeckoView, or if our GeckoView is not using browser.js, prefs won't work.

Comment 1

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

Attachment: Introduce new PrefsHelper implementation (v1)

Introduce a new implementation for PrefsHelper that does not use events
or rely on browser.js for getting and/or setting prefs. Also add an
addObserver method to better match the removeObserver method.

Comment 2

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

Attachment: Update native bindings for PrefsHelper (v1)

Update auto-generated native bindings for PrefsHelper. r=me

Comment 3

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

Attachment: Implement new PrefsHelper native methods (v1)

Implement the PrefsHelper native methods. The previous browser.js
implementation supported "pseudo-prefs" that did not exist as actual
prefs, but was accessible through PrefsHelper. In order to accommodate
these pseudo-prefs, we send observer notifications in order to
communicate with browser.js about prefs that we don't support.

Comment 4

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

Attachment: Convert browser.js prefs code to use observer (v1)

Convert the old prefs code in browser.js to use observer notifications
that are sent by the new PrefsHelper implementation, in order to handle
pseudo-prefs.

Comment 5

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

Attachment: Use PrefHelper.addObserver (v1)

Use the new PrefHelper.addObserver method for adding pref observers.

Comment 6

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

Attachment: Update robocop tests to use new prefs API (v1)

Change old robocop prefs API to the new API and add helper classes for
getting prefs. Also switch all tests that use prefs to use the new API.

Comment 7

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

Comment on attachment 8712280 [details] [diff] [review]
Introduce new PrefsHelper implementation (v1)

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

Wow, nice!

Comment 8

[Geoff Brown [:gbrown]]

Comment on attachment 8712289 [details] [diff] [review]
Update robocop tests to use new prefs API (v1)

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

This looks great.

I recommend a good try run with several retries of the robocop tests.

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/FennecNativeActions.java
@@ +254,5 @@
> +                if (System.nanoTime() - startTime
> +                        >= timeoutMillis * 1e6 /* ns per ms */) {
> +                    if (failOnTimeout) {
> +                        FennecNativeDriver.logAllStackTraces(FennecNativeDriver.LogLevel.ERROR);
> +                        target.asserter.ok(false, "Timeout waiting for pref", "");

Logging the expected and/or seen pref names here might assist debugging.

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/BaseTest.java
@@ +61,5 @@
>      private static final int MAX_WAIT_VERIFY_PAGE_TITLE_MS = 15000;
>      public static final int MAX_WAIT_MS = 4500;
>      public static final int LONG_PRESS_TIME = 6000;
>      private static final int GECKO_READY_WAIT_MS = 180000;
>      public static final int MAX_WAIT_BLOCK_FOR_EVENT_DATA_MS = 90000;

Is this unused now?

Comment 9

[:Margaret Leibovic]

Comment on attachment 8712286 [details] [diff] [review]
Convert browser.js prefs code to use observer (v1)

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

I didn't look at the other patches in the series, so assuming this works, that's awesome! :)

::: mobile/android/chrome/content/browser.js
@@ -1692,5 @@
> -    // This allows us to be confident that prefs set in Settings are persisted,
> -    // even if we crash very soon after.
> -    if (json.flush) {
> -      Services.prefs.savePrefFile(null);
> -    }

I don't see any logic for flushing the prefs to disk in the new version of this logic. Is that handled elsewhere?

Comment 10

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

(In reply to :Margaret Leibovic from comment #9)
> Comment on attachment 8712286 [details] [diff] [review]
> Convert browser.js prefs code to use observer (v1)
> 
> Review of attachment 8712286 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I didn't look at the other patches in the series, so assuming this works,
> that's awesome! :)
> 
> ::: mobile/android/chrome/content/browser.js
> @@ -1692,5 @@
> > -    // This allows us to be confident that prefs set in Settings are persisted,
> > -    // even if we crash very soon after.
> > -    if (json.flush) {
> > -      Services.prefs.savePrefFile(null);
> > -    }
> 
> I don't see any logic for flushing the prefs to disk in the new version of
> this logic. Is that handled elsewhere?

Yep! It's handled natively now.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/606bc36d35a1
https://hg.mozilla.org/integration/mozilla-inbound/rev/c98df93878f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9da983929fe
https://hg.mozilla.org/integration/mozilla-inbound/rev/6244023e793b
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4b3686d3283
https://hg.mozilla.org/integration/mozilla-inbound/rev/24209390c447

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/606bc36d35a1
https://hg.mozilla.org/mozilla-central/rev/c98df93878f2
https://hg.mozilla.org/mozilla-central/rev/a9da983929fe
https://hg.mozilla.org/mozilla-central/rev/6244023e793b
https://hg.mozilla.org/mozilla-central/rev/c4b3686d3283
https://hg.mozilla.org/mozilla-central/rev/24209390c447