Bug 201596 – Stop using testRunner.setPrivateBrowsingEnabled_DEPRECATED() in http/tests/adClickAttribution/conversion-disabled-in-ephemeral-session.html

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 378343 [details]
Patch

[John Wilander]

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]

(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 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 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]

Created attachment 378413 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 378413 [details]
Patch

Clearing flags on attachment: 378413

Committed r249675: <https://trac.webkit.org/changeset/249675>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55203697>

[Ryan Haddad]

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]

http://trac.webkit.org/r249705

[Chris Dumez]

(In reply to Alex Christensen from comment #11)
> http://trac.webkit.org/r249705

Thanks.