API::HTTPCookieStore should expose setCookies()

• Component: WebKit API
• Status: RESOLVED FIXED

[Jiewen Tan]

API::HTTPCookieStore should expose setCookies().

[Radar WebKit Bug Importer]

<rdar://problem/48266256>

[Jiewen Tan]

Created attachment 362596 [details]
Patch

[EWS Watchlist]

Comment on attachment 362596 [details]
Patch

Attachment 362596 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11229515

New failing tests:
fast/viewport/ios/device-width-viewport-after-changing-view-scale.html

[EWS Watchlist]

Created attachment 362599 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Jiewen Tan]

(In reply to Build Bot from comment #3)
> Comment on attachment 362596 [details]
> Patch
> 
> Attachment 362596 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> Output: https://webkit-queues.webkit.org/results/11229515
> 
> New failing tests:
> fast/viewport/ios/device-width-viewport-after-changing-view-scale.html

It doesn't make sense. The API is new and not used by anyone else at this moment.

[Geoffrey Garen]

Comment on attachment 362596 [details]
Patch

The implementation of pending cookies has many edge cases that cause bugs. It's hard to tell whether this patch will work or not, without seeing the code that will use it. Ideally, a call to set cookies would happen after instantiating the network process, and would talk directly to the network process. That would avoid all the edge cases with pending cookies. (I'm not saying this approach is definitely wrong. Just that it might run into correctness problems, and it's hard to evaluate without seeing the calling code.)

[Alex Christensen]

Created attachment 362622 [details]
Patch

[Brent Fulgham]

Comment on attachment 362622 [details]
Patch

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

> Source/WebKit/NetworkProcess/Cookies/WebCookieManager.cpp:117
> +            storageSession->setCookie(cookie);

It's too bad this can't be done using 'setCookies', though perhaps there's no real performance benefit in doing so.

[Brent Fulgham]

Comment on attachment 362622 [details]
Patch

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

>> Source/WebKit/NetworkProcess/Cookies/WebCookieManager.cpp:117
>> +            storageSession->setCookie(cookie);
> 
> It's too bad this can't be done using 'setCookies', though perhaps there's no real performance benefit in doing so.

(I'm thinking of:

 - (void)setCookies:(NSArray<NSHTTPCookie *> *)cookies 
            forURL:(NSURL *)URL 
   mainDocumentURL:(NSURL *)mainDocumentURL;

... and mainDocumentURL can be nil.

See https://developer.apple.com/documentation/foundation/nshttpcookiestorage/1412510-setcookies?language=objc)

[Alex Christensen]

I think there's no real benefit in passing an array to CFNetwork, especially since we would need to allocate an NSArray just to get this "benefit"

[Alex Christensen]

Created attachment 362637 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Brent Fulgham]

Comment on attachment 362637 [details]
Patch

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

r=me

> Source/WebKit/UIProcess/API/Cocoa/WKPreferences.mm:1336
> +    WTFLogAlways("SAFE BROWSING ENABLED");

Was this meant to be included in this patch?

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:83
> +            WTFLogAlways("SAFE BROWSING FRAMEWORK COMPLETION HANDLER CALLED %@ %@", result.get(), error.get());

Was this meant to be included in this patch?

[Alex Christensen]

(In reply to Brent Fulgham from comment #13)
> Comment on attachment 362637 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362637&action=review
> 
> r=me
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKPreferences.mm:1336
> > +    WTFLogAlways("SAFE BROWSING ENABLED");
> 
> Was this meant to be included in this patch?
> 
> > Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:83
> > +            WTFLogAlways("SAFE BROWSING FRAMEWORK COMPLETION HANDLER CALLED %@ %@", result.get(), error.get());
> 
> Was this meant to be included in this patch?
No

[Alex Christensen]

http://trac.webkit.org/r241903