1453143 - Add GeckoRuntimeSettings builder to support mutable settings

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P1)

Description

[Eugen Sawin [:esawin]]

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.

Comment 1

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1453143-1.0-Add-GeckoRuntimeSettings-builder-for.patch

Add settings builder to allow separation between mutable settings and "static" settings that can only be set during initialization.

Comment 2

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1453143-2.0-Use-GeckoRuntimeSettings-builder-in-.patch

Use the settings builder in the example app.

Comment 3

[Eugen Sawin [:esawin]]

Attachment: 0003-Bug-1453143-3.0-Add-support-for-pref-based-mutable-a.patch

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.

Comment 4

[Eugen Sawin [:esawin]]

Attachment: 0004-Bug-1440708-1.0-Add-GeckoRuntime-JavaScript-control-.patch

Example pref-based settings (will move review to its bug).

Comment 5

[Eugen Sawin [:esawin]]

Remaining patch that applies the new settings API is pending the review of patch 1.

Comment 6

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

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.

Comment 7

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

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 */

Comment 8

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

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.

Comment 9

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1453143-1.1-Add-GeckoRuntimeSettings-builder-for.patch

Comment 10

[Eugen Sawin [:esawin]]

Attachment: 0003-Bug-1453143-3.1-Add-support-for-pref-based-mutable-a.patch

(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.

Comment 11

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

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.

Comment 12

[Eugen Sawin [:esawin]]

(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.

Comment 13

[Eugen Sawin [:esawin]]

Attachment: 0003-Bug-1453143-3.2-Add-support-for-pref-based-mutable-a.patch

Comment 14

[Eugen Sawin [:esawin]]

Attachment: 0004-Bug-1453143-4.0-Use-GeckoRuntimeSettings-builder-in-.patch

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/5d38501e55dc
https://hg.mozilla.org/mozilla-central/rev/4d56446fd614
https://hg.mozilla.org/mozilla-central/rev/a3b3732bb802
https://hg.mozilla.org/mozilla-central/rev/88aab44edada