1322575 - [geckoview] Add private mode (or persistent) boolean in constructor and xml

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

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

This allows someone to create a GeckoView that will use a private mode window and not store any information.

Comment 1

[Dylan Roeh (:droeh) (he/him)]

Attachment: GeckoView private mode support

Added support for private mode via GeckoViewSettings. A second patch adding and handling the custom attribute for it will follow.

snorp suggested that we'd want to be able to take a GeckoViewSettings object in the constructor as part of this, so I had to make some changes there.

Comment 2

[Eugen Sawin [:esawin]]

Comment on attachment 8855829 [details] [diff] [review]
GeckoView private mode support

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

Looks good, but I would prefer not to add specific settings parameters to open.

Patch description is wrong.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java
@@ +96,5 @@
>  
>          static native void open(Window instance, GeckoView view,
>                                  Object compositor, EventDispatcher dispatcher,
>                                  String chromeUri, GeckoBundle settings,
> +                                int screenId, boolean privateMode);

I think instead of adding settings parameters, it would be cleaner to retrieve the flags directly from the settings bundle or by exposing GeckoViewSettings's getters via JNI.

Jim should have an opinion on this.

@@ +211,5 @@
>  
>      public GeckoView(Context context) {
>          super(context);
> +
> +        GeckoViewSettings settings = new GeckoViewSettings(getEventDispatcher());

Final.

@@ +218,5 @@
>  
>      public GeckoView(Context context, AttributeSet attrs) {
>          super(context, attrs);
> +        // TODO: Convert custom attributes to GeckoViewSettings
> +        GeckoViewSettings settings = new GeckoViewSettings(getEventDispatcher());

Final.

@@ +224,3 @@
>      }
>  
> +    public GeckoView(Context context, GeckoViewSettings settings) {

Can be both final.

@@ +228,5 @@
> +        settings.setEventDispatcher(getEventDispatcher());
> +        init(context, settings);
> +    }
> +
> +    private void init(Context context, GeckoViewSettings settings) {

Final.

@@ +281,5 @@
>          }
>  
>          if (GeckoThread.isStateAtLeast(GeckoThread.State.PROFILE_READY)) {
>              Window.open(mWindow, this, getCompositor(), mEventDispatcher,
> +                        mChromeUri, mSettings.asBundle(), mScreenId, mSettings.getBoolean(GeckoViewSettings.USE_PRIVATE_MODE));

nit: Can you please move this to a new line and add indentation despite our Java style guide's opinion on this (which is a horrible decision imho).

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoViewSettings.java
@@ +52,5 @@
>          }
>          dispatchUpdate();
>      }
>  
> +    public boolean getBoolean(Key<Boolean> key) {

Oops. Also key can be final.

@@ +65,5 @@
>  
>      private void dispatchUpdate() {
> +        if (mEventDispatcher != null) {
> +            mEventDispatcher.dispatch("GeckoView:UpdateSettings", null);
> +        }

Can we log a warning for the null-case?

@@ +68,5 @@
> +            mEventDispatcher.dispatch("GeckoView:UpdateSettings", null);
> +        }
> +    }
> +
> +    /* package */ void setEventDispatcher(EventDispatcher eventDispatcher) {

Final.

::: widget/android/nsWindow.cpp
@@ +1244,5 @@
> +                       androidView, getter_AddRefs(domWindow));
> +    } else {
> +        ww->OpenWindow(nullptr, url, nullptr, "chrome,dialog=0,resizable,scrollbars=yes",
> +                       androidView, getter_AddRefs(domWindow));
> +    }

Maybe we should construct the string conditionally instead to avoid duplicating the complete call.

Comment 3

[Dylan Roeh (:droeh) (he/him)]

Attachment: GeckoView private mode support v1.1

Addressed everything Eugen brought up except for reading the private mode setting from the bundle -- it doesn't look to me at first glance like we expose a nice way to do that in C++. I'll wait for Jim's opinion before I start writing something.

Comment 4

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

Comment on attachment 8855952 [details] [diff] [review]
GeckoView private mode support v1.1

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

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java
@@ +96,5 @@
>  
>          static native void open(Window instance, GeckoView view,
>                                  Object compositor, EventDispatcher dispatcher,
>                                  String chromeUri, GeckoBundle settings,
> +                                int screenId, boolean privateMode);

I think this is okay for now. If we add one more parameter like this, we'll want to add JNI to GeckoViewSettings instead, like Eugen suggested.

@@ +212,5 @@
>      public GeckoView(Context context) {
>          super(context);
> +
> +        final GeckoViewSettings settings = new GeckoViewSettings(getEventDispatcher());
> +        init(context, settings);

Pass null as settings

@@ +219,5 @@
>      public GeckoView(Context context, AttributeSet attrs) {
>          super(context, attrs);
> +        // TODO: Convert custom attributes to GeckoViewSettings
> +        final GeckoViewSettings settings = new GeckoViewSettings(getEventDispatcher());
> +        init(context, settings);

Pass null as settings

@@ +224,5 @@
>      }
>  
> +    public GeckoView(Context context, final GeckoViewSettings settings) {
> +        super(context);
> +        settings.setEventDispatcher(getEventDispatcher());

Make a copy of settings, with new EventDispatcher

@@ +246,5 @@
>  
>          initializeView();
>          mListener.registerListeners();
>  
> +        mSettings = settings;

Create new GeckoViewSettings here if settings is null.

@@ +281,5 @@
>          }
>  
>          if (GeckoThread.isStateAtLeast(GeckoThread.State.PROFILE_READY)) {
>              Window.open(mWindow, this, getCompositor(), mEventDispatcher,
> +                        mChromeUri, mSettings.asBundle(), mScreenId, 

Extra space at the end

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoViewSettings.java
@@ +29,2 @@
>  
> +    private EventDispatcher mEventDispatcher;

Keep it final

@@ +37,1 @@
>      /* package */ GeckoViewSettings(EventDispatcher eventDispatcher) {

Add a copy constructor that also sets a new EventDispatcher.

@@ +66,5 @@
>      private void dispatchUpdate() {
> +        if (mEventDispatcher != null) {
> +            mEventDispatcher.dispatch("GeckoView:UpdateSettings", null);
> +        } else {
> +            Log.w(LOGTAG, "Cannot dispatch updates without an EventDispatcher");

Get rid of the warning because `mEventDispatcher == null` is not an error

@@ +70,5 @@
> +            Log.w(LOGTAG, "Cannot dispatch updates without an EventDispatcher");
> +        }
> +    }
> +
> +    /* package */ void setEventDispatcher(final EventDispatcher eventDispatcher) {

Don't need this with above changes.

::: widget/android/nsWindow.cpp
@@ +289,5 @@
>                       GeckoView::Param aView, jni::Object::Param aCompositor,
>                       jni::Object::Param aDispatcher,
>                       jni::String::Param aChromeURI,
>                       jni::Object::Param aSettings,
> +                     int32_t screenId,

aScreenID

@@ +290,5 @@
>                       jni::Object::Param aDispatcher,
>                       jni::String::Param aChromeURI,
>                       jni::Object::Param aSettings,
> +                     int32_t screenId,
> +                     bool privateMode);

aPrivateMode

@@ +1210,5 @@
>                                   jni::Object::Param aDispatcher,
>                                   jni::String::Param aChromeURI,
>                                   jni::Object::Param aSettings,
> +                                 int32_t aScreenId,
> +                                 bool privateMode)

aPrivateMode

@@ +1237,5 @@
>      if (aSettings) {
>          androidView->mSettings = java::GeckoBundle::Ref::From(aSettings);
>      }
>  
> +    nsCString chromeFlags;

nsAutoCString, and use the const char* constructor.

@@ +1238,5 @@
>          androidView->mSettings = java::GeckoBundle::Ref::From(aSettings);
>      }
>  
> +    nsCString chromeFlags;
> +    chromeFlags = "chrome,dialog=0,resizable,scrollbars=yes";

While you're at it, change "scrollbars=yes" to just "scrollbars"

Comment 5

[Dylan Roeh (:droeh) (he/him)]

Attachment: GeckoView private mode support v1.2

With recommended changes.

Comment 6

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

Comment on attachment 8857068 [details] [diff] [review]
GeckoView private mode support v1.2

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

LGTM with following fixes.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java
@@ +222,5 @@
> +    public GeckoView(Context context, final GeckoViewSettings settings) {
> +        super(context);
> +
> +        final GeckoViewSettings newSettings = new GeckoViewSettings(settings, getEventDispatcher());
> +        init(context, settings);

newSettings

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoViewSettings.java
@@ +43,4 @@
>      }
>  
> +    /* package */ GeckoViewSettings(GeckoViewSettings settings, EventDispatcher eventDispatcher) {
> +        mBundle = settings.mBundle;

No, make a copy of the Bundle as well.

@@ +45,5 @@
> +    /* package */ GeckoViewSettings(GeckoViewSettings settings, EventDispatcher eventDispatcher) {
> +        mBundle = settings.mBundle;
> +        mEventDispatcher = eventDispatcher;
> +
> +        dispatchUpdate();

Don't need this

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/128049103e10
Add private mode support to GeckoView. r=jchen,esawin

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/128049103e10