Bug 118657 – [WK2][Soup] Add private browsing support

• Component: Platform
• Status: RESOLVED FIXED

[Kwang Yul Seo]

Soup should support private browsing in WK2.

[Kwang Yul Seo]

Created attachment 206639 [details]
Patch

[WebKit Commit Bot]

Attachment 206639 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/network/NetworkStorageSession.h', u'Source/WebCore/platform/network/NetworkStorageSessionStub.cpp', u'Source/WebCore/platform/network/ResourceHandle.h', u'Source/WebCore/platform/network/soup/CookieJarSoup.cpp', u'Source/WebCore/platform/network/soup/CookieJarSoup.h', u'Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp', u'Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/soup/WebFrameNetworkingContext.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/soup/WebFrameNetworkingContext.h', u'Source/WebKit2/WebProcess/WebPage/WebPage.cpp', u'Source/WebKit2/WebProcess/WebProcess.cpp']" exit_code: 1
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1384:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1385:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1386:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1387:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1388:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1389:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1390:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1390:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 8 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EFL EWS Bot]

Comment on attachment 206639 [details]
Patch

Attachment 206639 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1064772

[Kwang Yul Seo]

(In reply to comment #1)
> Created an attachment (id=206639) [details]
> Patch

This is the first try to add private browsing support to soup in WK2. 

One problem with this patch is that it does not use the identifier base when creating a private browsing session. The Mac port uses the bundle name of the UIProcess. Similarly, we can use the process name of the UIProcess, but I am sure what to do with this identifier base in ResourceHandle::createPrivateBrowsingSession.

Any comment is welcome.

[Kwang Yul Seo]

Created attachment 206640 [details]
Patch

[WebKit Commit Bot]

Attachment 206640 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/network/NetworkStorageSession.h', u'Source/WebCore/platform/network/NetworkStorageSessionStub.cpp', u'Source/WebCore/platform/network/ResourceHandle.h', u'Source/WebCore/platform/network/soup/CookieJarSoup.cpp', u'Source/WebCore/platform/network/soup/CookieJarSoup.h', u'Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp', u'Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/soup/WebFrameNetworkingContext.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/soup/WebFrameNetworkingContext.h', u'Source/WebKit2/WebProcess/WebPage/WebPage.cpp', u'Source/WebKit2/WebProcess/WebProcess.cpp']" exit_code: 1
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1384:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1385:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1386:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1387:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1388:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1389:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1390:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1390:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 8 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kwang Yul Seo]

(In reply to comment #5)
> Created an attachment (id=206640) [details]
> Patch

Build fix.

[Kwang Yul Seo]

Created attachment 206641 [details]
Patch

[Kwang Yul Seo]

(In reply to comment #8)
> Created an attachment (id=206641) [details]
> Patch

Fixed a typo in the ChangeLog.

[WebKit Commit Bot]

Attachment 206641 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/network/NetworkStorageSession.h', u'Source/WebCore/platform/network/NetworkStorageSessionStub.cpp', u'Source/WebCore/platform/network/ResourceHandle.h', u'Source/WebCore/platform/network/soup/CookieJarSoup.cpp', u'Source/WebCore/platform/network/soup/CookieJarSoup.h', u'Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp', u'Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/soup/WebFrameNetworkingContext.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/soup/WebFrameNetworkingContext.h', u'Source/WebKit2/WebProcess/WebPage/WebPage.cpp', u'Source/WebKit2/WebProcess/WebProcess.cpp']" exit_code: 1
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1384:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1385:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1386:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1387:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1388:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1389:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1390:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1390:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 8 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gustavo Noronha (kov)]

Comment on attachment 206641 [details]
Patch

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

r- because of the style issues and because I feel we should figure out the identifier base before landing this

> Source/WebCore/platform/network/NetworkStorageSession.h:53
> +    bool isPrivateBrowsingSession() const { return m_isPrivate; }

This should be kept under a PLATFORM(MAC) || USE(CFNETWORK) || USE(SOUP) check, since it's still not universally supported.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1408
> +    // FIXME: Handle identifierBase

What do you mean handle identifierBase? What do you need to do with it?

> Source/WebKit2/WebProcess/WebCoreSupport/soup/WebFrameNetworkingContext.cpp:32
>  

There should be no empty line here.

[Kwang Yul Seo]

(In reply to comment #11)
> (From update of attachment 206641 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206641&action=review
> 
> r- because of the style issues and because I feel we should figure out the identifier base before landing this

Okay, I will fix the style issues. I intentionally violated the style issues as I tried to keep the original code as-is. 

> > Source/WebCore/platform/network/NetworkStorageSession.h:53
> > +    bool isPrivateBrowsingSession() const { return m_isPrivate; }
> 
> This should be kept under a PLATFORM(MAC) || USE(CFNETWORK) || USE(SOUP) check, since it's still not universally supported.

This method is not platform-specific. Isn't it okay to make other ports return false by initializing m_isPrivate to false in the constructor?

> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1408
> > +    // FIXME: Handle identifierBase
> 
> What do you mean handle identifierBase? What do you need to do with it?

I will ask the original authors about the intention of identifierBase.

> > Source/WebKit2/WebProcess/WebCoreSupport/soup/WebFrameNetworkingContext.cpp:32
> >  
> 
> There should be no empty line here.

Okay. I will fix it.

Thanks for your review.

[Alexey Proskuryakov]

> What do you mean handle identifierBase? What do you need to do with it?

It's used by CFNetwork to identify named sessions (specifically, a private browsing session for Safari would be created with identifierBase "com.apple.Safari", and will be named "com.apple.Safari.PrivateBrowsing").

You probably don't need it unless Soup also identifies sessions by a string identifier.

[Kwang Yul Seo]

Created attachment 206692 [details]
Patch

[Kwang Yul Seo]

(In reply to comment #13)
> > What do you mean handle identifierBase? What do you need to do with it?
> 
> It's used by CFNetwork to identify named sessions (specifically, a private browsing session for Safari would be created with identifierBase "com.apple.Safari", and will be named "com.apple.Safari.PrivateBrowsing").
> 
> You probably don't need it unless Soup also identifies sessions by a string identifier.

Thanks for the information. I changed the patch to ignore the string identifier passed by createPrivateBrowsingSession() because Soup does not identify sessions by a string identifier.

[EFL EWS Bot]

Comment on attachment 206692 [details]
Patch

Attachment 206692 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1068624

[EFL EWS Bot]

Comment on attachment 206692 [details]
Patch

Attachment 206692 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1071595

[Kwang Yul Seo]

(In reply to comment #17)
> (From update of attachment 206692 [details])
> Attachment 206692 [details] did not pass efl-wk2-ews (efl-wk2):
> Output: http://webkit-queues.appspot.com/results/1071595

It seems the last argument of g_object_set should NULL instead of 0. Otherwise, EFL build fails with the following errors:

Building CXX object Source/WebCore/CMakeFiles/WebCore.dir/platform/network/soup/ResourceHandleSoup.cpp.o
/home/kseo/webkit-efl/Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp: In function 'SoupSession* WebCore::createSoupSession()':
/home/kseo/webkit-efl/Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1390:10: error: missing sentinel in function call [-Werror=format]

It seems we pass NULL to g_object_set in other places too. Should we add an exception to check-webkit-style?

kseo@kseo:WebCore (privatebrowsing)> grep -R g_object_set *
platform/graphics/gtk/FullscreenVideoControllerGtk.cpp:    g_object_set(m_exitFullscreenAction, "icon-name", EXIT_FULLSCREEN_ICON_NAME, NULL);
platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:    g_object_set(m_volumeElement.get(), "mute", muted, NULL);
platform/graphics/gstreamer/GStreamerGWorld.cpp:        g_object_set(sink, "force-aspect-ratio", TRUE, NULL);
...

[Kwang Yul Seo]

Created attachment 206701 [details]
Patch

[WebKit Commit Bot]

Attachment 206701 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/network/NetworkStorageSession.h', u'Source/WebCore/platform/network/ResourceHandle.h', u'Source/WebCore/platform/network/soup/CookieJarSoup.cpp', u'Source/WebCore/platform/network/soup/CookieJarSoup.h', u'Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp', u'Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/soup/WebFrameNetworkingContext.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/soup/WebFrameNetworkingContext.h', u'Source/WebKit2/WebProcess/WebPage/WebPage.cpp', u'Source/WebKit2/WebProcess/WebProcess.cpp']" exit_code: 1
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1390:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kwang Yul Seo]

(In reply to comment #20)
> Attachment 206701 [details] did not pass style-queue:

Bug 32858 and Bug 39372 fixed this false positive style error. But the rule works only when the entire function call is one line. If g_*() function call spans multiple lines, check-webkit-style does not recognize the pattern.

[Kwang Yul Seo]

Comment on attachment 206701 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2453
> +    bool privateBrowsingEnabled = store.getBoolValueForKey(WebPreferencesKey::privateBrowsingEnabledKey());
> +    settings->setPrivateBrowsingEnabled(privateBrowsingEnabled);
> +#if USE(SOUP)
> +    if (privateBrowsingEnabled)
> +        WebFrameNetworkingContext::ensurePrivateBrowsingSession();
> +    else
> +        WebFrameNetworkingContext::destroyPrivateBrowsingSession();
> +#endif

I realized that this is not necessary because WebPreferences keeps track of privateBrowsingPageGroupCount and sends WebProcess::EnsurePrivateBrowsingSession and WebProcess::DestroyPrivateBrowsingSession messages accordingly.

I will fix the patch.

[Kwang Yul Seo]

Created attachment 206733 [details]
Patch

[Kwang Yul Seo]

(In reply to comment #21)
> Bug 32858 and Bug 39372 fixed this false positive style error. But the rule works only when the entire function call is one line. If g_*() function call spans multiple lines, check-webkit-style does not recognize the pattern.

I filed a separate bug to fix this. See Bug 118718

[Kwang Yul Seo]

Created attachment 207420 [details]
Patch

[Kwang Yul Seo]

(In reply to comment #25)
> Created an attachment (id=207420) [details]
> Patch

Rebased to the HEAD. kov, would you review the patch again?

[WebKit Commit Bot]

Attachment 207420 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/network/NetworkStorageSession.h', u'Source/WebCore/platform/network/ResourceHandle.h', u'Source/WebCore/platform/network/soup/CookieJarSoup.cpp', u'Source/WebCore/platform/network/soup/CookieJarSoup.h', u'Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp', u'Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/soup/WebFrameNetworkingContext.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/soup/WebFrameNetworkingContext.h', u'Source/WebKit2/WebProcess/WebProcess.cpp']" exit_code: 1
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1386:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gustavo Noronha (kov)]

Comment on attachment 207420 [details]
Patch

OK!

[Kwang Yul Seo]

Committed r153355: <http://trac.webkit.org/changeset/153355>