Bug 166969 – [SOUP] Fix handling of accept language property

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 298672 [details]
Patch

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Created attachment 298752 [details]
Updated patch

[Michael Catanzaro]

(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...?

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

Committed r210785: <http://trac.webkit.org/changeset/210785>

[Carlos Garcia Campos]

(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.

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

(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