Bug 166969

Summary: [SOUP] Fix handling of accept language property
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, bugs-noreply, commit-queue, danw, gustavo, mcatanzaro, mrobinson
Priority: P2    
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 166967    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Updated patch mcatanzaro: review+

Description Carlos Garcia Campos 2017-01-12 05:25:48 PST
Accept languages should be applied to all network contexts, but we currently only apply it to the default one, and there's a workaround added in r206431 to also apply it to the testing one. We should do it properly and remove the workaround for the testing session.
Comment 1 Carlos Garcia Campos 2017-01-12 05:31:19 PST
Created attachment 298672 [details]
Patch
Comment 2 Michael Catanzaro 2017-01-12 12:21:18 PST
Comment on attachment 298672 [details]
Patch

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

I noticed this back before we supported network session. (That said, is it actually possible to have multiple network sessions in our port yet, or is that a future step?)

> Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp:101
> +    g_object_set(NetworkStorageSession::defaultStorageSession().soupNetworkSession().soupSession(), "accept-language", acceptLanguages.data(), nullptr);
> +    NetworkStorageSession::forEach([acceptLanguages](const WebCore::NetworkStorageSession& session) {
> +        g_object_set(session.soupNetworkSession().soupSession(), "accept-language", acceptLanguages.data(), nullptr);

Hmmmm

I think our SoupNetworkSession class should have a helper function that wraps this g_object_set(..., "accept-language", ...) so that you don't have to do this manually from outside the class. Up until this patch, that was SoupNetworkSession::setAcceptLanguages. So I really don't see the advantage to this approach. Instead of changing setAcceptLanguages to be static, I would call it here, and add a new static function SoupNetworkSession::setInitialAcceptLanguages to set it for new SoupNetworkSession objects. (I guess it's not possible to pass in via the constructor for some reason?)

This is just a matter of style; I'm willing to give r+ if you disagree, but I don't think this is the nicest way.
Comment 3 Carlos Garcia Campos 2017-01-13 00:39:17 PST
(In reply to comment #2)
> Comment on attachment 298672 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298672&action=review
> 
> I noticed this back before we supported network session. (That said, is it
> actually possible to have multiple network sessions in our port yet, or is
> that a future step?)

It is. Open MiniBrowser and enable private browsing in the settings, for example. The other case is testing, NetworkStorageSession::switchToNewTestingSession() creates a new session that is private from the cookie storage point of view, but non ephemeral from the session point of view, so it replaces the default session.

> > Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp:101
> > +    g_object_set(NetworkStorageSession::defaultStorageSession().soupNetworkSession().soupSession(), "accept-language", acceptLanguages.data(), nullptr);
> > +    NetworkStorageSession::forEach([acceptLanguages](const WebCore::NetworkStorageSession& session) {
> > +        g_object_set(session.soupNetworkSession().soupSession(), "accept-language", acceptLanguages.data(), nullptr);
> 
> Hmmmm
> 
> I think our SoupNetworkSession class should have a helper function that
> wraps this g_object_set(..., "accept-language", ...) so that you don't have
> to do this manually from outside the class. Up until this patch, that was
> SoupNetworkSession::setAcceptLanguages. So I really don't see the advantage
> to this approach. Instead of changing setAcceptLanguages to be static, I
> would call it here, and add a new static function
> SoupNetworkSession::setInitialAcceptLanguages to set it for new
> SoupNetworkSession objects. (I guess it's not possible to pass in via the
> constructor for some reason?)
> 
> This is just a matter of style; I'm willing to give r+ if you disagree, but
> I don't think this is the nicest way.

No, I think this is good idea.
Comment 4 Carlos Garcia Campos 2017-01-13 00:44:11 PST
Created attachment 298752 [details]
Updated patch
Comment 5 Michael Catanzaro 2017-01-13 10:13:37 PST
(In reply to comment #3) 
> It is. Open MiniBrowser and enable private browsing in the settings, for
> example. The other case is testing,
> NetworkStorageSession::switchToNewTestingSession() creates a new session
> that is private from the cookie storage point of view, but non ephemeral
> from the session point of view, so it replaces the default session.

If we support private browsing session now, should we be using that in Epiphany...?
Comment 6 Michael Catanzaro 2017-01-13 10:18:23 PST
Comment on attachment 298752 [details]
Updated patch

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

> Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp:100
> +    NetworkStorageSession::forEach([acceptLanguages](const WebCore::NetworkStorageSession& session) {

Should probably capture by reference instead of copying the CString object unnecessarily.
Comment 7 Carlos Garcia Campos 2017-01-16 03:25:43 PST
Committed r210785: <http://trac.webkit.org/changeset/210785>
Comment 8 Carlos Garcia Campos 2017-01-16 05:31:08 PST
(In reply to comment #5)
> (In reply to comment #3) 
> > It is. Open MiniBrowser and enable private browsing in the settings, for
> > example. The other case is testing,
> > NetworkStorageSession::switchToNewTestingSession() creates a new session
> > that is private from the cookie storage point of view, but non ephemeral
> > from the session point of view, so it replaces the default session.
> 
> If we support private browsing session now, should we be using that in
> Epiphany...?

We have always supported private browsing, another thing is that it has always been broken :-P I don't think it's ready for ephy yet, at least custom uri schemes doesn't work in private browsing mode, but it might be the only thing left to do. Then, yes, I think it's safer to enable private browsing in ephy, it might cover cases for which we are still writing outside the ephy profile dir.
Comment 9 Michael Catanzaro 2017-01-27 06:41:54 PST
So did we mess up here? Accept-Langs is a context-specific setting. Unless each web context gets its own separate network process, this is wrong.
Comment 10 Carlos Garcia Campos 2017-01-27 06:59:43 PST
(In reply to comment #9)
> So did we mess up here? Accept-Langs is a context-specific setting. Unless
> each web context gets its own separate network process, this is wrong.

Every web context has its own network process