RESOLVED FIXED 201596
Stop using testRunner.setPrivateBrowsingEnabled_DEPRECATED() in http/tests/adClickAttribution/conversion-disabled-in-ephemeral-session.html
https://bugs.webkit.org/show_bug.cgi?id=201596
Summary Stop using testRunner.setPrivateBrowsingEnabled_DEPRECATED() in http/tests/ad...
Chris Dumez
Reported 2019-09-08 17:37:19 PDT
Stop using testRunner.setPrivateBrowsingEnabled_DEPRECATED() in http/tests/adClickAttribution/conversion-disabled-in-ephemeral-session.html as it is a hack which does not work truly using a private session in WebKit2.
Attachments
Patch (29.42 KB, patch)
2019-09-08 18:49 PDT, Chris Dumez
no flags
Patch (30.49 KB, patch)
2019-09-09 15:51 PDT, Chris Dumez
no flags
Chris Dumez
Comment 1 2019-09-08 18:49:12 PDT
John Wilander
Comment 2 2019-09-09 15:00:43 PDT
Comment on attachment 378343 [details] Patch Changes to the test case look good. Do we have a suitable reviewer for the TestRunner changes?
Chris Dumez
Comment 3 2019-09-09 15:01:21 PDT
(In reply to John Wilander from comment #2) > Comment on attachment 378343 [details] > Patch > > Changes to the test case look good. Do we have a suitable reviewer for the > TestRunner changes? I asked Alex to take a look. Thanks for double checking the test.
Alex Christensen
Comment 4 2019-09-09 15:05:25 PDT
Comment on attachment 378343 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378343&action=review > Source/WebKit/UIProcess/API/C/WKFramePolicyListener.cpp:-58 > - RELEASE_ASSERT_WITH_MESSAGE(sessionID.isEphemeral() || sessionID == PAL::SessionID::defaultSessionID(), "If WebsitePolicies specifies a WebsiteDataStore, the data store's session must be default or non-persistent."); Please remove the corresponding check in the ObjC API. It's also no longer needed. > Tools/DumpRenderTree/TestRunner.cpp:2567 > + ASSERT(!(m_shouldSwapToEphemeralSessionOnNextNavigation && m_shouldSwapToDefaultSessionOnNextNavigation)); ditto with comment below. > Tools/WebKitTestRunner/TestController.cpp:2822 > + ASSERT(!(shouldSwapToEphemeralSessionOnNextNavigation && shouldSwapToDefaultSessionOnNextNavigation)); You could just assert that they are unequal. > Tools/WebKitTestRunner/TestController.cpp:2824 > + WKRetainPtr<WKWebsiteDataStoreRef> newSession = TestController::websiteDataStore(); There's no need to retain this. Just use a plain old WKWebsiteDataStoreRef
Chris Dumez
Comment 5 2019-09-09 15:36:14 PDT
Comment on attachment 378343 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378343&action=review >> Tools/WebKitTestRunner/TestController.cpp:2824 >> + WKRetainPtr<WKWebsiteDataStoreRef> newSession = TestController::websiteDataStore(); > > There's no need to retain this. Just use a plain old WKWebsiteDataStoreRef This is needed for the assignment of WKWebsiteDataStoreCreateNonPersistentDataStore() 2 lines below.
Chris Dumez
Comment 6 2019-09-09 15:51:48 PDT
WebKit Commit Bot
Comment 7 2019-09-09 16:35:18 PDT
Comment on attachment 378413 [details] Patch Clearing flags on attachment: 378413 Committed r249675: <https://trac.webkit.org/changeset/249675>
WebKit Commit Bot
Comment 8 2019-09-09 16:35:20 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 9 2019-09-09 16:36:21 PDT
Ryan Haddad
Comment 10 2019-09-09 17:32:30 PDT
It appears that this change introduced an API test failure: TestWebKitAPI.WebKit.UpdateWebsitePoliciesInvalid _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL. _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL. /Volumes/Data/slave/mojave-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsitePolicies.mm:1546 Value of: sawException Actual: false Expected: true https://build.webkit.org/builders/Apple%20Mojave%20Release%20WK2%20(Tests)/builds/6506
Alex Christensen
Comment 11 2019-09-09 21:51:15 PDT
Chris Dumez
Comment 12 2019-09-09 21:56:06 PDT
(In reply to Alex Christensen from comment #11) > http://trac.webkit.org/r249705 Thanks.
Note You need to log in before you can comment on or make changes to this bug.