Bug 167414 - [GTK] Icon Database should be in private browsing mode for ephemeral web views
Summary: [GTK] Icon Database should be in private browsing mode for ephemeral web views
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2017-01-25 03:19 PST by Carlos Garcia Campos
Modified: 2017-01-25 09:04 PST (History)
6 users (show)

See Also:


Attachments
Patch (8.50 KB, patch)
2017-01-25 03:23 PST, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2017-01-25 03:19:11 PST
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.
Comment 1 Carlos Garcia Campos 2017-01-25 03:23:58 PST
Created attachment 299687 [details]
Patch
Comment 2 WebKit Commit Bot 2017-01-25 03:26:20 PST
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
Comment 3 Michael Catanzaro 2017-01-25 08:39:00 PST
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
Comment 4 Carlos Garcia Campos 2017-01-25 08:52:28 PST
(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.
Comment 5 Michael Catanzaro 2017-01-25 08:57:30 PST
(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.
Comment 6 Carlos Garcia Campos 2017-01-25 09:00:10 PST
(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 :-)
Comment 7 Carlos Garcia Campos 2017-01-25 09:04:31 PST
Committed r211147: <http://trac.webkit.org/changeset/211147>