Bug 194861 - API::HTTPCookieStore should expose setCookies()
Summary: API::HTTPCookieStore should expose setCookies()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-20 11:58 PST by Jiewen Tan
Modified: 2019-02-21 14:34 PST (History)
10 users (show)

See Also:


Attachments
Patch (3.50 KB, patch)
2019-02-20 23:38 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.42 MB, application/zip)
2019-02-21 01:38 PST, Build Bot
no flags Details
Patch (9.94 KB, patch)
2019-02-21 11:28 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (12.50 KB, patch)
2019-02-21 13:19 PST, Alex Christensen
bfulgham: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2019-02-20 11:58:11 PST
API::HTTPCookieStore should expose setCookies().
Comment 1 Radar WebKit Bug Importer 2019-02-20 23:06:56 PST
<rdar://problem/48266256>
Comment 2 Jiewen Tan 2019-02-20 23:38:22 PST
Created attachment 362596 [details]
Patch
Comment 3 Build Bot 2019-02-21 01:38:33 PST
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
Comment 4 Build Bot 2019-02-21 01:38:35 PST
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
Comment 5 Jiewen Tan 2019-02-21 10:36:05 PST
(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.
Comment 6 Geoffrey Garen 2019-02-21 10:55:13 PST
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.)
Comment 7 Alex Christensen 2019-02-21 11:28:48 PST
Created attachment 362622 [details]
Patch
Comment 8 Brent Fulgham 2019-02-21 11:35:12 PST
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.
Comment 9 Brent Fulgham 2019-02-21 11:36:55 PST
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)
Comment 10 Alex Christensen 2019-02-21 13:15:48 PST
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"
Comment 11 Alex Christensen 2019-02-21 13:19:40 PST
Created attachment 362637 [details]
Patch
Comment 12 Build Bot 2019-02-21 13:21:40 PST
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
Comment 13 Brent Fulgham 2019-02-21 13:24:26 PST
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?
Comment 14 Alex Christensen 2019-02-21 14:32:08 PST
(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
Comment 15 Alex Christensen 2019-02-21 14:34:33 PST
http://trac.webkit.org/r241903