Bug 24487 – [Gtk] Implement LayoutTestControllerGtk::setPrivateBrowsingEnabled

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Jan Alonzo]

Enable private browsing adding a 'enable-private-browsing' setting in WebKitWebSettings and enabling it in DRT. 

Doing this passes http/tests/security/cross-frame-access-private-browsing.html.

[Jan Alonzo]

Created attachment 28433 [details]
Implement private browsing in DRT and add option in WebKit/Gtk

[Holger Freyther]

Comment on attachment 28433 [details]
Implement private browsing in DRT and add option in WebKit/Gtk


> +    resetWebViewToConsistentStateBeforeTesting();
> +

you might want to copy more there... e.g. the loading of about:blank (which we should kill at some point)


ate after certain tests.
>      webkit_web_view_open(webView, "about:blank");
>  
> -    gLayoutTestController->setJavaScriptProfilingEnabled(false);

NO! The g_object_set in resetWebView... is no good replacement! setJSProfilingEnabled will disable both Developer Extras and JS profiling. Is there a reason you want to keep Developer Extras enabled?

[Jan Alonzo]

(In reply to comment #2)
> (From update of attachment 28433 [details] [review])
> 
> > +    resetWebViewToConsistentStateBeforeTesting();
> > +
> 
> you might want to copy more there... e.g. the loading of about:blank (which we
> should kill at some point)

I'll do that in a different patch.

> 
> 
> ate after certain tests.
> >      webkit_web_view_open(webView, "about:blank");
> >  
> > -    gLayoutTestController->setJavaScriptProfilingEnabled(false);
> 
> NO! The g_object_set in resetWebView... is no good replacement!
> setJSProfilingEnabled will disable both Developer Extras and JS profiling. Is
> there a reason you want to keep Developer Extras enabled?

Why not? Why is the resetWebView... not a good replacement? developer extras is also disabled in resetWebView...

Thanks

[Holger Freyther]

(In reply to comment #3)

> 
> Why not? Why is the resetWebView... not a good replacement? developer extras is
> also disabled in resetWebView...

right, didn't see it/looked for it in the g_object_set. Please describe this change in the ChangeLog and why it is semantically equivalent.

[Jan Alonzo]

Landed in r41583. Thanks a lot dude.

[Christian Dywan]

(In reply to comment #5)
> Landed in r41583. Thanks a lot dude.

This new setting badly needs documentation, neither Xan, Gustavo or me could grasp what it does without digging inside WebCore. So I'm listing behaviour which may be affected, and we need to verify and unit test those points.

- "Follow up to <rdar://problem/5394877> Safari should not log unsafe JavaScript attempts when in private browsing mode (only an issue if Log JavaScript Exceptions is turned on)"
- No page navigation ends up in history?
- Does it affect cookie storage?
- Cache is not used?
- Form completion is discarded?

The last two probably don't apply simply because we don't have these features.

[Gustavo Noronha (kov)]

Comment on attachment 28433 [details]
Implement private browsing in DRT and add option in WebKit/Gtk

Clearing review flag so that it stops showing up in the pending commit queue.

[Jan Alonzo]

Created attachment 28820 [details]
updated documentation

[Jan Alonzo]

I uploaded a new patch to fix these. I appreciate any feedback if any. I tagged that feature as experimental for WebKitGtk since some features like on-disk caching are, i think, not yet implemented for Gtk.

(In reply to comment #6)
> grasp what it does without digging inside WebCore. So I'm listing behaviour
> which may be affected, and we need to verify and unit test those points.
> 
> - "Follow up to <rdar://problem/5394877> Safari should not log unsafe
> JavaScript attempts when in private browsing mode (only an issue if Log
> JavaScript Exceptions is turned on)"
> - No page navigation ends up in history?
> - Does it affect cookie storage?
> - Cache is not used?
> - Form completion is discarded?
> 
> The last two probably don't apply simply because we don't have these features.

That's right. I think the only thing to test here is the cookie storage. Is there a way to test that?

[Holger Freyther]

Cookies are added by WebCore to the session, caching data to secondary storage is something that soup would do, so there might be a need to synchronize this flag with the soup session...

[Holger Freyther]

Comment on attachment 28820 [details]
updated documentation 


> +    * This is currently experimental for WebKitGtk.

Okay, change that to WebKitGTK+ (as this seems to be our name now) :)

[Jan Alonzo]

Landed in r42231. Thanks!