[webkit-reviews] review granted: [Bug 222145] Serialize NSURLRequest (rather than CFURLRequest) in IPC : [Attachment 421486] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Brent Fulgham <bfulgham at webkit.org>'s
request for review:
Bug 222145: Serialize NSURLRequest (rather than CFURLRequest) in IPC
https://bugs.webkit.org/show_bug.cgi?id=222145

Attachment 421486: Patch

https://bugs.webkit.org/attachment.cgi?id=421486&action=review




--- Comment #46 from Darin Adler <darin at apple.com> ---
Comment on attachment 421486
  --> https://bugs.webkit.org/attachment.cgi?id=421486
Patch

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

> Source/WTF/wtf/cocoa/NSURLExtras.mm:169
> +	       // So only return early if the character after the slash is
somethihg else.

Typo in the word "something" here.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:60
> +- (instancetype _Nullable)initWithURL:(NSURL *)url;

Could use the wrappedURL name here too, to make it look nicer.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:76
> +    if (auto url = dynamic_objc_cast<NSURL>(object))

Could use the wrappedURL name here too, to make it look nicer.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:107
> +NSString * const baseURLExistsKey = @"WK.baseURLExists";

This is not used in the code below. If this was marked static, you would have
gotten a compiler warning about it being unused.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:108
> +NSString * const baseURLKey = @"WK.baseURL";

Since this is limited to the file it should have static. Could use constexpr
for this. The syntax is nicer:

    static constexpr NSString *baseURLKey = @"WK.baseURL";

Might also not even need "static" with constexpr, but I think it’s correct to
do both.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:128
> +    auto selfPtr = adoptNS([super init]);
> +    if (selfPtr) {

Can just write this instead of nesting the entire function:

    auto selfPtr = adoptNS([super init]);
    if (!selfPtr)
	return nil;

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:134
> +	       baseURL = (NSURL *)[coder decodeObjectOfClass:NSURL.class
forKey:baseURLKey];

Does this end up recursively using this method?

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:472
>	   [unarchiver finishDecoding];
> +	   unarchiver.get().delegate = nil;

Does this code *actually* run before the function returns even though we have
return inside the @try and @catch?