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.
Created attachment 298672 [details] Patch
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.
(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.
Created attachment 298752 [details] Updated patch
(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 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.
Committed r210785: <http://trac.webkit.org/changeset/210785>
(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.
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.
(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