Bug 169514 – Add full NSHTTPCookie fidelity to WebCore::Cookie

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

Add full NSHTTPCookie fidelity to WebCore::Cookie

[Brady Eidson]

Created attachment 304180 [details]
Current patch

I'm breaking this off from larger work and don't expect it to build quite yet.

Will iterate later after EWS tells me.

[Sam Weinig]

Comment on attachment 304180 [details]
Current patch

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

> Source/WebCore/platform/network/cocoa/CookieCocoa.mm:56
> +    StringBuilder builder;
> +    for (size_t i = 0; i < ports.size() - 1; ++i) {
> +        builder.append(String::number(ports[i]));
> +        builder.append(", ");
> +    }
> +    builder.append(String::number(ports.last()));
> +
> +    return builder.toString();

Is it really more efficient to build a WTF::String than convert it to an NSString vs just using an NSMutableString to build it?

> Source/WebCore/platform/network/cocoa/CookieCocoa.mm:99
> +        [properties setObject:@"YES" forKey:NSHTTPCookieSecure];

The old code was doing a boxed bool value here, not a string (@"YES" vs. @(secure)).  Is the change intentional?

> Source/WebCore/platform/network/cocoa/CookieCocoa.mm:101
> +    [properties setObject:session ? @"TRUE" : @"FALSE" forKey:NSHTTPCookieDiscard];

Again, the old code was doing a boxed bool value here? @(session).  Why is this now using @"TRUE" /  @"FALSE"?

> Source/WebCore/platform/network/mac/CookieJarMac.mm:230
>          NSHTTPCookie *cookie = (NSHTTPCookie *)[cookies objectAtIndex:i];

You could probably make cookiesForURL return an NSArray<NSHTTPCookie *> *, and then just use cookies[I] here (or actually, this should just be a for (NSHTTPCookie *cookie in cookies) loop).

Also, and unrelated, this function should return a Vector, not us an out parameter.

[Brady Eidson]

Created attachment 304210 [details]
Patch

[Brady Eidson]

Completely didn't notice that anybody had taken a look at this, sorry - Wasn't purposefully ignoring your feedback.

[Dean Jackson]

Comment on attachment 304210 [details]
Patch

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

> Source/WebCore/platform/network/cocoa/CookieCocoa.mm:56
> +    StringBuilder builder;
> +    for (size_t i = 0; i < ports.size() - 1; ++i) {
> +        builder.append(String::number(ports[i]));
> +        builder.append(", ");
> +    }
> +    builder.append(String::number(ports.last()));
> +
> +    return builder.toString();

Didn't Sam suggest using NSMutableString over StringBuilder?

He also made two other suggestions.

> Source/WebCore/platform/network/cocoa/CookieCocoa.mm:101
> +    [properties setObject:session ? @"TRUE" : @"FALSE" forKey:NSHTTPCookieDiscard];

My opinion is that it reads better as (session ? @"TRUE" : @"FALSE")

[Brady Eidson]

(In reply to comment #2)
> Comment on attachment 304180 [details]
> Current patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=304180&action=review
> 
> > Source/WebCore/platform/network/cocoa/CookieCocoa.mm:56
> > +    StringBuilder builder;
> > +    for (size_t i = 0; i < ports.size() - 1; ++i) {
> > +        builder.append(String::number(ports[i]));
> > +        builder.append(", ");
> > +    }
> > +    builder.append(String::number(ports.last()));
> > +
> > +    return builder.toString();
> 
> Is it really more efficient to build a WTF::String than convert it to an
> NSString vs just using an NSMutableString to build it?

Probably not.

> > Source/WebCore/platform/network/cocoa/CookieCocoa.mm:99
> > +        [properties setObject:@"YES" forKey:NSHTTPCookieSecure];
> 
> The old code was doing a boxed bool value here, not a string (@"YES" vs.
> @(secure)).  Is the change intentional?

Not really.

> > Source/WebCore/platform/network/cocoa/CookieCocoa.mm:101
> > +    [properties setObject:session ? @"TRUE" : @"FALSE" forKey:NSHTTPCookieDiscard];
> 
> Again, the old code was doing a boxed bool value here? @(session).  Why is
> this now using @"TRUE" /  @"FALSE"?

This one is actually correct, per the documentation for NSHTTPCookieDiscard - It expects the strings TRUE and FALSE, not YES/NO.
> 
> > Source/WebCore/platform/network/mac/CookieJarMac.mm:230
> >          NSHTTPCookie *cookie = (NSHTTPCookie *)[cookies objectAtIndex:i];
> 
> You could probably make cookiesForURL return an NSArray<NSHTTPCookie *> *,
> and then just use cookies[I] here (or actually, this should just be a for
> (NSHTTPCookie *cookie in cookies) loop).
> 
> Also, and unrelated, this function should return a Vector, not us an out
> parameter.

Probably yup.

[Brady Eidson]

Created attachment 304211 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 304211 [details]
Patch

Clearing flags on attachment: 304211

Committed r213778: <http://trac.webkit.org/changeset/213778>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.