Summary: | [WK2][Soup] Add private browsing support | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kwang Yul Seo <skyul> | ||||||||||||||||
Component: | Platform | Assignee: | Kwang Yul Seo <skyul> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | ap, beidson, cdumez, commit-queue, danw, eflews.bot, gustavo, gyuyoung.kim, mrobinson, rakuco, rego+ews, svillar, xan.lopez | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
Kwang Yul Seo
2013-07-14 22:19:44 PDT
Created attachment 206639 [details]
Patch
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 on attachment 206639 [details] Patch Attachment 206639 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/1064772 (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. Created attachment 206640 [details]
Patch
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.
(In reply to comment #5) > Created an attachment (id=206640) [details] > Patch Build fix. Created attachment 206641 [details]
Patch
(In reply to comment #8) > Created an attachment (id=206641) [details] > Patch Fixed a typo in the ChangeLog. 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 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. (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. > 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.
Created attachment 206692 [details]
Patch
(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 on attachment 206692 [details] Patch Attachment 206692 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1068624 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 (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); ... Created attachment 206701 [details]
Patch
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.
(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 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. Created attachment 206733 [details]
Patch
(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 Created attachment 207420 [details]
Patch
(In reply to comment #25) > Created an attachment (id=207420) [details] > Patch Rebased to the HEAD. kov, would you review the patch again? 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 on attachment 207420 [details]
Patch
OK!
Committed r153355: <http://trac.webkit.org/changeset/153355> |