Created attachment 344209 [details] [review]
Use an ephemeral WebKitWebContext in incognito mode

I think this is enough. This patch also removes the code that sets languages setting to the default context, I have no idea why we do this, but incognito mode should never use the default context nor any other web context than the ephemeral one.

Review of attachment 344209 [details] [review]:

I guess it needs more work before we can get rid of the profile stealing code?

It looks except for one problem:

::: embed/ephy-embed-prefs.c
@@ -372,3 @@
-  /* Set preferred languages also for the default web context, used by the sync
-   * tab in the preferences dialog. It doesn't need spellchecking. */
-  webkit_web_context_set_preferred_languages (webkit_web_context_get_default (), (const char * const *)(void *)array->data);

The reason we do this is to ensure that the user can read the contents of the Firefox Sync web view in the preferences dialog when built with Firefox Sync support. Without this, the user is going to receive an English page from Mozilla that he might not be able to read. So we do need it.

Maybe we want to use our WebKitWebsiteDataManager on the default context as well, to ensure it uses normal Ephy cache directories?

The problem of the sync web view is tricky. It really cannot be an EphyWebView and it cannot use the Epiphany web context because we don't want it running the  Epiphany web extension, but there are a few settings that need shared. I was thinking just language, but really it should use the cache and data directories as well, right? It's also pretty confusing that we use the default web context only for the Firefox Sync web view, but the layering makes it awkward to do otherwise.

(In reply to Michael Catanzaro from comment #3)
> but really it should use the
> cache and data directories as well, right?

Except when in incognito mode... then we probably want to use the normal non-incognito directories, since we surely don't want incognito mode to apply to the sync web view.

(In reply to Michael Catanzaro from comment #2)
> Review of attachment 344209 [details] [review] [review]:
> 
> I guess it needs more work before we can get rid of the profile stealing
> code?

Yes, WebKit doesn't know anything about ephy history and bookmarks.

> It looks except for one problem:
> 
> ::: embed/ephy-embed-prefs.c
> @@ -372,3 @@
> -  /* Set preferred languages also for the default web context, used by the
> sync
> -   * tab in the preferences dialog. It doesn't need spellchecking. */
> -  webkit_web_context_set_preferred_languages
> (webkit_web_context_get_default (), (const char * const *)(void
> *)array->data);
> 
> The reason we do this is to ensure that the user can read the contents of
> the Firefox Sync web view in the preferences dialog when built with Firefox
> Sync support. Without this, the user is going to receive an English page
> from Mozilla that he might not be able to read. So we do need it.

It's unacceptable that the incognito mode writes to the user default local storage like it does with this. What the the firefox sync web view does, it should be done in an ephemeral web view. With newer WebKit, the language setting is set globally and applied to all sessions, so this is not needed anyway. But still, nothing should use the default context in ephy, I don't know why the ff sycn web view is using the default web context.

(In reply to Michael Catanzaro from comment #3)
> Maybe we want to use our WebKitWebsiteDataManager on the default context as
> well, to ensure it uses normal Ephy cache directories?

Why are we using the default context at all?

> The problem of the sync web view is tricky. It really cannot be an
> EphyWebView and it cannot use the Epiphany web context because we don't want
> it running the  Epiphany web extension, but there are a few settings that
> need shared. I was thinking just language, but really it should use the
> cache and data directories as well, right? It's also pretty confusing that
> we use the default web context only for the Firefox Sync web view, but the
> layering makes it awkward to do otherwise.

Do you really want to cache that? I have no idea what the ff web view does. Maybe we can just create a new context with the same website data manager. And either use an ephemeral web view in incognito mode or disable that feature. What we can't do is create the web context always like we currently do for a feature that maybe is not even used at all.

(In reply to Carlos Garcia Campos from comment #5)
> It's unacceptable that the incognito mode writes to the user default local
> storage like it does with this. What the the firefox sync web view does, it
> should be done in an ephemeral web view.

I filed bug #777756 to make sure Firefox Sync is disabled in incognito mode, but the sync web view is really completely separate from normal Epiphany browsing. I don't think it needs to be ephemeral.

> With newer WebKit, the language
> setting is set globally and applied to all sessions, so this is not needed
> anyway.

Hm, good point, but this is actually a big problem: what happens if I have two web contexts with different language settings? Did we break our WebKitWebContext API?

> But still, nothing should use the default context in ephy, I don't
> know why the ff sycn web view is using the default web context.

We agreed on Jabber that I should make it use a different web context.

(In reply to Michael Catanzaro from comment #7) 
> We agreed on Jabber that I should make it use a different web context.

Ah, but I can't access the embed web context from prefs-dialog.c, so no way to get the right locale setting....

(In reply to Michael Catanzaro from comment #8)
> Ah, but I can't access the embed web context from prefs-dialog.c, so no way
> to get the right locale setting....

I was confused. Of course we can.

(In reply to Michael Catanzaro from comment #9)
> (In reply to Michael Catanzaro from comment #8)
> > Ah, but I can't access the embed web context from prefs-dialog.c, so no way
> > to get the right locale setting....
> 
> I was confused. Of course we can.

This is, surprisingly, nontrivial, because WebKitWebContext does not store the preferred languages. There is webkit_web_context_set_preferred_languages() but no webkit_web_context_get_preferred_languages(); they're passed directly into WebCore. What a silly problem....

Created attachment 344502 [details] [review]
prefs-dialog: Do not use the default web context

Instead, create a new web context just for the sync web view and get
preferred languages from the Epiphany embed web context. Use
g_object_get/set_data() as a workaround for the lack of a
webkit_web_context_get_preferred_languages().

(In reply to Michael Catanzaro from comment #11)
> Created attachment 344502 [details] [review] [review]
> prefs-dialog: Do not use the default web context
> 
> Instead, create a new web context just for the sync web view and get
> preferred languages from the Epiphany embed web context. Use
> g_object_get/set_data() as a workaround for the lack of a
> webkit_web_context_get_preferred_languages().

You should file a bug report to WebKit if you need a setting instead of/in addition to work around it in ephy.

Comment on attachment 344502 [details] [review]
prefs-dialog: Do not use the default web context

If this works, it's fine with me.

(In reply to Carlos Garcia Campos from comment #12)
> (In reply to Michael Catanzaro from comment #11)
> > Created attachment 344502 [details] [review] [review] [review]
> > prefs-dialog: Do not use the default web context
> > 
> > Instead, create a new web context just for the sync web view and get
> > preferred languages from the Epiphany embed web context. Use
> > g_object_get/set_data() as a workaround for the lack of a
> > webkit_web_context_get_preferred_languages().
> 
> You should file a bug report to WebKit if you need a setting instead of/in
> addition to work around it in ephy.

I meant if you need a getter.

https://bugs.webkit.org/show_bug.cgi?id=167646