Bug 118657 - [WK2][Soup] Add private browsing support
Summary: [WK2][Soup] Add private browsing support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kwang Yul Seo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-07-14 22:19 PDT by Kwang Yul Seo
Modified: 2013-07-25 16:23 PDT (History)
13 users (show)

See Also:


Attachments
Patch (19.30 KB, patch)
2013-07-14 22:38 PDT, Kwang Yul Seo
no flags Details | Formatted Diff | Diff
Patch (19.29 KB, patch)
2013-07-14 22:48 PDT, Kwang Yul Seo
no flags Details | Formatted Diff | Diff
Patch (19.29 KB, patch)
2013-07-14 22:56 PDT, Kwang Yul Seo
no flags Details | Formatted Diff | Diff
Patch (17.55 KB, patch)
2013-07-15 15:33 PDT, Kwang Yul Seo
no flags Details | Formatted Diff | Diff
Patch (17.55 KB, patch)
2013-07-15 16:32 PDT, Kwang Yul Seo
no flags Details | Formatted Diff | Diff
Patch (15.55 KB, patch)
2013-07-16 00:21 PDT, Kwang Yul Seo
no flags Details | Formatted Diff | Diff
Patch (15.54 KB, patch)
2013-07-24 16:48 PDT, Kwang Yul Seo
gns: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kwang Yul Seo 2013-07-14 22:19:44 PDT
Soup should support private browsing in WK2.
Comment 1 Kwang Yul Seo 2013-07-14 22:38:51 PDT
Created attachment 206639 [details]
Patch
Comment 2 WebKit Commit Bot 2013-07-14 22:40:44 PDT
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.
Comment 3 EFL EWS Bot 2013-07-14 22:44:58 PDT
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
Comment 4 Kwang Yul Seo 2013-07-14 22:45:40 PDT
(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.
Comment 5 Kwang Yul Seo 2013-07-14 22:48:26 PDT
Created attachment 206640 [details]
Patch
Comment 6 WebKit Commit Bot 2013-07-14 22:49:22 PDT
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.
Comment 7 Kwang Yul Seo 2013-07-14 22:56:26 PDT
(In reply to comment #5)
> Created an attachment (id=206640) [details]
> Patch

Build fix.
Comment 8 Kwang Yul Seo 2013-07-14 22:56:53 PDT
Created attachment 206641 [details]
Patch
Comment 9 Kwang Yul Seo 2013-07-14 22:57:09 PDT
(In reply to comment #8)
> Created an attachment (id=206641) [details]
> Patch

Fixed a typo in the ChangeLog.
Comment 10 WebKit Commit Bot 2013-07-14 22:58:11 PDT
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.
Comment 11 Gustavo Noronha (kov) 2013-07-15 08:02:30 PDT
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.
Comment 12 Kwang Yul Seo 2013-07-15 08:36:06 PDT
(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.
Comment 13 Alexey Proskuryakov 2013-07-15 10:29:44 PDT
> 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.
Comment 14 Kwang Yul Seo 2013-07-15 15:33:29 PDT
Created attachment 206692 [details]
Patch
Comment 15 Kwang Yul Seo 2013-07-15 15:35:43 PDT
(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.
Comment 16 EFL EWS Bot 2013-07-15 16:01:50 PDT
Comment on attachment 206692 [details]
Patch

Attachment 206692 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1068624
Comment 17 EFL EWS Bot 2013-07-15 16:17:54 PDT
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
Comment 18 Kwang Yul Seo 2013-07-15 16:30:48 PDT
(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);
...
Comment 19 Kwang Yul Seo 2013-07-15 16:32:36 PDT
Created attachment 206701 [details]
Patch
Comment 20 WebKit Commit Bot 2013-07-15 16:35:11 PDT
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.
Comment 21 Kwang Yul Seo 2013-07-15 21:43:31 PDT
(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.
Comment 22 Kwang Yul Seo 2013-07-16 00:17:41 PDT
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.
Comment 23 Kwang Yul Seo 2013-07-16 00:21:40 PDT
Created attachment 206733 [details]
Patch
Comment 24 Kwang Yul Seo 2013-07-16 00:39:35 PDT
(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
Comment 25 Kwang Yul Seo 2013-07-24 16:48:32 PDT
Created attachment 207420 [details]
Patch
Comment 26 Kwang Yul Seo 2013-07-24 16:49:25 PDT
(In reply to comment #25)
> Created an attachment (id=207420) [details]
> Patch

Rebased to the HEAD. kov, would you review the patch again?
Comment 27 WebKit Commit Bot 2013-07-24 16:49:36 PDT
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.
Comment 28 Gustavo Noronha (kov) 2013-07-25 06:11:00 PDT
Comment on attachment 207420 [details]
Patch

OK!
Comment 29 Kwang Yul Seo 2013-07-25 16:23:47 PDT
Committed r153355: <http://trac.webkit.org/changeset/153355>