Move prefs handling to outside of browser.js

RESOLVED FIXED in Firefox 47

Status

()

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

People

(Reporter: jchen, Assigned: jchen)

Tracking

(Blocks: 1 bug)

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

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(6 attachments)

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.
Created attachment 8712280 [details] [diff] [review]
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.
Attachment #8712280 - Flags: review?(snorp)
Created attachment 8712281 [details] [diff] [review]
Update native bindings for PrefsHelper (v1)

Update auto-generated native bindings for PrefsHelper. r=me
Attachment #8712281 - Flags: review+
Created attachment 8712283 [details] [diff] [review]
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.
Attachment #8712283 - Flags: review?(snorp)
Created attachment 8712286 [details] [diff] [review]
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.
Attachment #8712286 - Flags: review?(margaret.leibovic)
Created attachment 8712288 [details] [diff] [review]
Use PrefHelper.addObserver (v1)

Use the new PrefHelper.addObserver method for adding pref observers.
Attachment #8712288 - Flags: review?(snorp)
Created attachment 8712289 [details] [diff] [review]
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.
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 9

2 years ago
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.

Comment 11

2 years ago
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

2 years ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47

Updated

2 years ago
Depends on: 1247293
Depends on: 1248175

Updated

2 years ago
Depends on: 1255461
You need to log in before you can comment on or make changes to this bug.