Bug 167414 – [GTK] Icon Database should be in private browsing mode for ephemeral web views

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

This is already done by WebProcessPool for the legacy private session setting, only checking the setting and not whether there are ephemeral web pages or not.

[Carlos Garcia Campos]

Created attachment 299687 [details]
Patch

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Michael Catanzaro]

Comment on attachment 299687 [details]
Patch

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

Exactly the sort of thing I was worried about with allowing a per-view ephemeral setting... will there be more?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:785
> +        // We notify the context here to ensure it's called only once. Ideally we should
> +        // call this in finalize, not dispose, but finalize is used internally and we don't
> +        // have access to the instance pointer from the private struct destructor.
> +        webkitWebContextWebViewDestroyed(webView->priv->context.get(), webView);

I don't understand this. Of course you can move the call to webkitWebContextWebViewDestroyed finalize? Why not? What do you mean it's used internally?

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitFaviconDatabase.cpp:163
> +static void testPrivateBrowsing(FaviconDatabaseTest* test)

Shouldn't you add a test for an ephemeral web context as well?

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitFaviconDatabase.cpp:173
> +    // An ephemeral web view should nto write to the database.

nto -> not

[Carlos Garcia Campos]

(In reply to comment #3)
> Comment on attachment 299687 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299687&action=review
> 
> Exactly the sort of thing I was worried about with allowing a per-view
> ephemeral setting... will there be more?

This is not because we allow per-webview ephemeral setting, but because the icon database works only with the legacy private browsing setting.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:785
> > +        // We notify the context here to ensure it's called only once. Ideally we should
> > +        // call this in finalize, not dispose, but finalize is used internally and we don't
> > +        // have access to the instance pointer from the private struct destructor.
> > +        webkitWebContextWebViewDestroyed(webView->priv->context.get(), webView);
> 
> I don't understand this. Of course you can move the call to
> webkitWebContextWebViewDestroyed finalize? Why not? What do you mean it's
> used internally?

Finalize is defined by WEBKIT_DEFINE_TYPE. So when we want to do something on finalize we use the private instance constructor, but that approach doesn't work when you also need access to the pointer of the object instance.

> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitFaviconDatabase.cpp:163
> > +static void testPrivateBrowsing(FaviconDatabaseTest* test)
> 
> Shouldn't you add a test for an ephemeral web context as well?

It's the same thing, as I said this is not happening because of per-webview setting. An ephemeral context simply creates ephemeral web views.

> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitFaviconDatabase.cpp:173
> > +    // An ephemeral web view should nto write to the database.
> 
> nto -> not

Oops.

[Michael Catanzaro]

(In reply to comment #4) 
> It's the same thing, as I said this is not happening because of per-webview
> setting. An ephemeral context simply creates ephemeral web views.

Yes, but since it's accomplished through the API in two different ways, it would be good to add a test to ensure it really is the same thing.

[Carlos Garcia Campos]

(In reply to comment #5)
> (In reply to comment #4) 
> > It's the same thing, as I said this is not happening because of per-webview
> > setting. An ephemeral context simply creates ephemeral web views.
> 
> Yes, but since it's accomplished through the API in two different ways, it
> would be good to add a test to ensure it really is the same thing.

You don't know how painful the favicon unit tests are :-)

[Carlos Garcia Campos]

Committed r211147: <http://trac.webkit.org/changeset/211147>