Closed Bug 1243049 Opened 4 years ago Closed 4 years ago

Move prefs handling to outside of browser.js

Categories

(Core :: Widget: Android, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(6 files)

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.
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.
Attachment #8712280 - Flags: review?(snorp)
Update auto-generated native bindings for PrefsHelper. r=me
Attachment #8712281 - Flags: review+
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.
Attachment #8712283 - Flags: review?(snorp)
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.
Attachment #8712286 - Flags: review?(margaret.leibovic)
Use the new PrefHelper.addObserver method for adding pref observers.
Attachment #8712288 - Flags: review?(snorp)
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.
Attachment #8712289 - Flags: review?(gbrown)
Comment on attachment 8712280 [details] [diff] [review]
Introduce new PrefsHelper implementation (v1)

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

Wow, nice!
Attachment #8712280 - Flags: review?(snorp) → review+
Attachment #8712283 - Flags: review?(snorp) → review+
Attachment #8712288 - Flags: review?(snorp) → review+
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?
Attachment #8712289 - Flags: review?(gbrown) → review+
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?
Attachment #8712286 - Flags: review?(margaret.leibovic) → review+
(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.
Depends on: 1247293
Depends on: 1248175
Depends on: 1255461
You need to log in before you can comment on or make changes to this bug.