Bug 167370 – [GTK] Add API to create ephemeral web views and deprecate the private browsing setting

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

ssia

[Carlos Garcia Campos]

Created attachment 299599 [details]
Patch

[Michael Catanzaro]

Comment on attachment 299599 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299599&action=review

Looking forward to the Epiphany implementation. ;)

Nice tests.

> Source/WebKit2/ChangeLog:23
> +        (webkitWebViewConstructed): Check if the WebView is or should be ephemeral and create a non persistent website
> +        data manager if it's an ephemeral web view in a non ephemeral web context.

What is the advantage to allowing users to create ephemeral web views in non-ephemeral web contexts? Is this really well-supported and unlikely to cause problems?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:506
> + * Get whether a #WebKitWebContext is ephemeral.

It would probably be advantageous to expose this implementation detail, so the user has a better clue what is going on:

"Get whether a #WebKitWebContext uses an ephemeral #WebKitWebsiteDataManager"

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:508
> + * Returns: %TRUE if @context is ephemeral or %FALSE otherwise.

You forgot the Since tag here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1002
> +     * created with an ephemeral #WebKitWebContext will be ephemeral automatically,

Another comma splice. Last time I told you to use a semicolon. This one would work better split it into two sentences.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:322
> +     * handles all websites data as non persistent, and nothing will be written to the client

non-persistent

> Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:426
>   * Returns: the base directory for Website data, or %NULL if

Preexisting bug: it does not have (allow-none) so bindings are broken. You won't be able to null-check it from vala, for example.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:447
>   * Returns: the base directory for Website cache, or %NULL if

(allow-none)

> Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:468
> + * Returns: the directory where local storage data is stored or %NULL if @manager is ephemeral.

(allow-none)

> Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:491
> + * Returns: the directory where HTTP disk cache is stored or %NULL if @manager is ephemeral.

(allow-none)

> Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:516
> + * Returns: the directory where offline web application cache is stored or %NULL if @manager is ephemeral.

(allow-none)

> Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:539
> + * Returns: the directory where IndexedDB databases are stored or %NULL if @manager is ephemeral.

(allow-none)

> Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:562
> + * Returns: the directory where WebSQL databases are stored or %NULL if @manager is ephemeral.

(allow-none)

[Carlos Garcia Campos]

(In reply to comment #2)
> Comment on attachment 299599 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299599&action=review
> 
> Looking forward to the Epiphany implementation. ;)
> 
> Nice tests.
> 
> > Source/WebKit2/ChangeLog:23
> > +        (webkitWebViewConstructed): Check if the WebView is or should be ephemeral and create a non persistent website
> > +        data manager if it's an ephemeral web view in a non ephemeral web context.
> 
> What is the advantage to allowing users to create ephemeral web views in
> non-ephemeral web contexts? Is this really well-supported and unlikely to
> cause problems?

Yes, it's well supported. It allows to open an incognito window in your normal browser instance, for example. It's not useful for epiphany because we use a new browser process for incognito mode. Note also that private browsing, or now ephemeral sessions, are per page. Allowing to create an ephemeral context is just a convenience way to avoid having to create all webviews ephemeral.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:506
> > + * Get whether a #WebKitWebContext is ephemeral.
> 
> It would probably be advantageous to expose this implementation detail, so
> the user has a better clue what is going on:
> 
> "Get whether a #WebKitWebContext uses an ephemeral #WebKitWebsiteDataManager"

Ok, I think we can explain it, yes.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:508
> > + * Returns: %TRUE if @context is ephemeral or %FALSE otherwise.
> 
> You forgot the Since tag here.

Oops.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1002
> > +     * created with an ephemeral #WebKitWebContext will be ephemeral automatically,
> 
> Another comma splice. Last time I told you to use a semicolon. This one
> would work better split it into two sentences.

Ok.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:322
> > +     * handles all websites data as non persistent, and nothing will be written to the client
> 
> non-persistent

Sure.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:426
> >   * Returns: the base directory for Website data, or %NULL if
> 
> Preexisting bug: it does not have (allow-none) so bindings are broken. You
> won't be able to null-check it from vala, for example.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:447
> >   * Returns: the base directory for Website cache, or %NULL if
> 
> (allow-none)

Ah, ok.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:468
> > + * Returns: the directory where local storage data is stored or %NULL if @manager is ephemeral.
> 
> (allow-none)
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:491
> > + * Returns: the directory where HTTP disk cache is stored or %NULL if @manager is ephemeral.
> 
> (allow-none)
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:516
> > + * Returns: the directory where offline web application cache is stored or %NULL if @manager is ephemeral.
> 
> (allow-none)
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:539
> > + * Returns: the directory where IndexedDB databases are stored or %NULL if @manager is ephemeral.
> 
> (allow-none)
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebsiteDataManager.cpp:562
> > + * Returns: the directory where WebSQL databases are stored or %NULL if @manager is ephemeral.
> 
> (allow-none)

[Carlos Garcia Campos]

Committed r211138: <http://trac.webkit.org/changeset/211138>