Closed Bug 1453143 Opened 2 years ago Closed Last year

Add GeckoRuntimeSettings builder to support mutable settings

Categories

(GeckoView :: General, enhancement, P1)

51 Branch
All
Android
enhancement

Tracking

(firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: esawin, Assigned: esawin)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 4 obsolete files)

2.22 KB, patch
rbarker
: review+
Details | Diff | Splinter Review
5.62 KB, patch
esawin
: review+
Details | Diff | Splinter Review
5.49 KB, patch
esawin
: review+
Details | Diff | Splinter Review
3.36 KB, patch
esawin
: review+
Details | Diff | Splinter Review
Currently we only support settings that need to be set during the initialization of the GV runtime, but we want to support runtime-mutable settings that can be changed post-initialization.
Assignee: nobody → esawin
Priority: -- → P1
Add settings builder to allow separation between mutable settings and "static" settings that can only be set during initialization.
Attachment #8966781 - Flags: review?(snorp)
Attachment #8966781 - Flags: review?(nchen)
Use the settings builder in the example app.
Attachment #8966782 - Flags: review?(rbarker)
Add yet another prefs helper for pref-based settings in GeckoRuntimeSettings.
This could be used as the base for the "prefs layer" between Gecko and GV in future.
Attachment #8966783 - Flags: review?(nchen)
Example pref-based settings (will move review to its bug).
Remaining patch that applies the new settings API is pending the review of patch 1.
Attachment #8966782 - Flags: review?(rbarker) → review+
Comment on attachment 8966781 [details] [diff] [review]
0001-Bug-1453143-1.0-Add-GeckoRuntimeSettings-builder-for.patch

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

I love it.
Attachment #8966781 - Flags: review?(nchen) → review+
Comment on attachment 8966781 [details] [diff] [review]
0001-Bug-1453143-1.0-Add-GeckoRuntimeSettings-builder-for.patch

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

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java
@@ +15,5 @@
>  public final class GeckoRuntimeSettings implements Parcelable {
> +    /**
> +     * Settings builder used to construct the settings object.
> +     */
> +    public static class Builder {

final

@@ +16,5 @@
> +    /**
> +     * Settings builder used to construct the settings object.
> +     */
> +    public static class Builder {
> +        private GeckoRuntimeSettings mSettings;

final

@@ +18,5 @@
> +     */
> +    public static class Builder {
> +        private GeckoRuntimeSettings mSettings;
> +
> +        public Builder() {

Provide second constructor with `GeckoRuntimeSettings` argument

@@ +28,5 @@
> +         *
> +         * @return The constructed settings.
> +         */
> +        public GeckoRuntimeSettings build() {
> +            return mSettings;

Return a copy, so the returned instance cannot be modified anymore.

@@ +36,5 @@
> +         * Set the content process hint flag.
> +         *
> +         * @param use If true, this will reload the content process for future use.
> +         */
> +        public Builder useContentProcessHint(boolean use) {

final, for consistency

@@ +46,5 @@
> +         * Set the custom Gecko process arguments.
> +         *
> +         * @param args The Gecko process arguments.
> +         */
> +        public Builder arguments(final @NonNull String[] args) {

Annotate the return values as @NonNull

@@ +66,3 @@
>      private boolean mUseContentProcess;
>      private String[] mArgs;
>      private Bundle mExtras;

Should be /* package */
Attachment #8966781 - Flags: review?(snorp) → review+
Comment on attachment 8966783 [details] [diff] [review]
0003-Bug-1453143-3.0-Add-support-for-pref-based-mutable-a.patch

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

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java
@@ +11,5 @@
>  import android.os.Parcelable;
>  import android.support.annotation.NonNull;
>  import android.support.annotation.Nullable;
>  
> +import org.mozilla.gecko.PrefsHelper;

Hopefully the prefs layer will replace PrefsHelper. I want to move PrefsHelper out of GV because it has some Fennec crud in it.

@@ +71,5 @@
>  
> +    private class Pref<T> {
> +        public final String name;
> +        public final T defaultValue;
> +        public T value;

private. Outside of this class, `newValue` should be treated as the current pref value rather than `value`, otherwise you would have weird behavior like `set(true); get() == false`.

@@ +72,5 @@
> +    private class Pref<T> {
> +        public final String name;
> +        public final T defaultValue;
> +        public T value;
> +        public T newValue;

private with getter

@@ +77,5 @@
> +
> +        public Pref(final String name, final T defaultValue) {
> +            this.name = name;
> +            this.defaultValue = defaultValue;
> +            newValue = defaultValue;

value is uninitialized

@@ +84,5 @@
> +        public void set(T value) {
> +            this.newValue = value;
> +        }
> +
> +        public void flush() {

flush() is dependent on the target runtime (you only want to call PrefsHelper on a local runtime), so probably need to store the runtime inside GeckoRuntimeSettings, and ask the runtime to flush, similar to how we store the session inside GeckoSessionSettings.

@@ +93,5 @@
> +            PrefsHelper.setPref(name, value);
> +        }
> +    }
> +
> +    private final Pref<?>[] mPrefs = new Pref<?>[] {};

I think this is one case where enum could work better,

> enum Prefs {
>   JAVASCRIPT(...),
>   ...
> }

Then you can use Prefs.JAVASCRIPT directly or refer to all values using Prefs.values(). The only caveat is enum doesn't support generics, so you would probably want somethiing like `Prefs.JAVASCRIPT.get<Boolean>()` to access the values.

If you want to keep a separate mPrefs array, you would need a test to check the array actually includes all Pref instances, like the GeckoSessionHandler check in GeckoSession.
Attachment #8966783 - Flags: review?(nchen) → feedback+
(In reply to Jim Chen [:jchen] [:darchons] from comment #8)
> Hopefully the prefs layer will replace PrefsHelper. I want to move
> PrefsHelper out of GV because it has some Fennec crud in it.

I'm preparing the runtime prefs helper in anticipation of having some version of a prefs layer in future, at which point we would move it out of GeckoRuntimeSettings and pull in required parts from PrefsHelper.

> private. Outside of this class, `newValue` should be treated as the current
> pref value rather than `value`, otherwise you would have weird behavior like
> `set(true); get() == false`.

This can be tricky, by moving the "ground truth" to the prefs helper now, we might run into situations where we report a  value that is out of sync with the pref set in Gecko. At least that would be true as long as the prefs layer doesn't actually override the pref value for Gecko.
But maybe it's better to fake it until we actually support the layer?

> > +
> > +        public Pref(final String name, final T defaultValue) {
> > +            this.name = name;
> > +            this.defaultValue = defaultValue;
> > +            newValue = defaultValue;
> 
> value is uninitialized

This is related to the previous comment. The helper separates setting and flushing of the pref, |value| represents Gecko's pref value, which we don't know/care about at this point. But having a null value could break things, so I'll err on potentially redundant pref flushes instead.

> flush() is dependent on the target runtime (you only want to call
> PrefsHelper on a local runtime), so probably need to store the runtime
> inside GeckoRuntimeSettings, and ask the runtime to flush, similar to how we
> store the session inside GeckoSessionSettings.

Good point, albeit the indirection is rather pointless for now.
Attachment #8966783 - Attachment is obsolete: true
Attachment #8967182 - Flags: review?(nchen)
Comment on attachment 8967182 [details] [diff] [review]
0003-Bug-1453143-3.1-Add-support-for-pref-based-mutable-a.patch

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

> > private. Outside of this class, `newValue` should be treated as the current
> > pref value rather than `value`, otherwise you would have weird behavior like
> > `set(true); get() == false`.
> 
> This can be tricky, by moving the "ground truth" to the prefs helper now, we
> might run into situations where we report a  value that is out of sync with
> the pref set in Gecko. At least that would be true as long as the prefs
> layer doesn't actually override the pref value for Gecko.
> But maybe it's better to fake it until we actually support the layer?

The value we report here is never strictly in sync with Gecko because pref-setting is async. The most logical value we can report is the current value on the Java side, which is `newValue`.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java
@@ +66,5 @@
>              return this;
>          }
>      }
>  
> +    /* package */ GeckoRuntime mRuntime;

Call it `runtime` because it's used by GeckoRuntime, outside of `GeckoRuntimeSettings` and its inner classes

@@ +88,5 @@
> +            newValue = defaultValue;
> +        }
> +
> +        public void set(T value) {
> +            this.newValue = value;

Should just flush here if `runtime != null`.

@@ +92,5 @@
> +            this.newValue = value;
> +        }
> +
> +        public T get() {
> +            return value;

newValue. I don't think we really need the `newValue` and `value` distinction; just one `value` should be okay.

@@ +96,5 @@
> +            return value;
> +        }
> +
> +        public T flush(final @Nullable GeckoRuntime runtime) {
> +            if (runtime != null) {

I think null check should happen in `GeckoRuntimeSettings.flush`, not here. runtime should be @NonNull here.
Attachment #8967182 - Flags: review?(nchen) → review+
(In reply to Jim Chen [:jchen] [:darchons] from comment #11)
> Should just flush here if `runtime != null`.

I think it's better to separate flushing from setting, e.g., we set it when restoring or when copy-constructing, we only want to flush once during startup and not for each value change.

> newValue. I don't think we really need the `newValue` and `value`
> distinction; just one `value` should be okay.

We use two value states to separate flushing and setting.

> I think null check should happen in `GeckoRuntimeSettings.flush`, not here.
> runtime should be @NonNull here.

That's mostly that way to avoid null-checks in every pref-based setter.
Pushed by esawin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d38501e55dc
[1.1] Add GeckoRuntimeSettings builder for mutable settings support. r=snorp,jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d56446fd614
[2.0] Use GeckoRuntimeSettings builder in the example app. r=rbarker
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3b3732bb802
[3.2] Add support for pref-based mutable and static runtime settings. r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/88aab44edada
[4.0] Use GeckoRuntimeSettings builder in tests. r=me
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 61 → mozilla61
You need to log in before you can comment on or make changes to this bug.