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

RESOLVED FIXED in Firefox 55

Status

defect
RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: snorp, Assigned: droeh)

Tracking

unspecified
mozilla55
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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

Comment 1

2 years ago
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.
Assignee: nobody → droeh
Attachment #8855829 - Flags: review?(nchen)
Attachment #8855829 - Flags: review?(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.
Attachment #8855829 - Flags: review?(esawin) → review+
Assignee

Comment 3

2 years ago
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.
Attachment #8855829 - Attachment is obsolete: true
Attachment #8855829 - Flags: review?(nchen)
Attachment #8855952 - Flags: review?(nchen)
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"
Attachment #8855952 - Flags: review?(nchen) → feedback+
Assignee

Comment 5

2 years ago
With recommended changes.
Attachment #8855952 - Attachment is obsolete: true
Attachment #8857068 - Flags: review?(nchen)
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
Attachment #8857068 - Flags: review?(nchen) → review+

Comment 7

2 years ago
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/128049103e10
Add private mode support to GeckoView. r=jchen,esawin

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/128049103e10
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

3 months ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.