[webkit-reviews] review granted: [Bug 222145] Serialize NSURLRequest (rather than CFURLRequest) in IPC : [Attachment 421470] 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 421470: Patch

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




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

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

Looks good. I still have style comments (all optional, but most are good
ideas), and one comment that *might* be a correctness issue, the return value
of initWithCoder: when decoding fails.

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

On reflection after reading multiple iterations of this patch, it seems to me
we want to wrap the contents of this file into NS_ASSUME_NONNULL_BEGIN/END. We
then would only have to annotate nullable things rather than also having to
annotate all the non-null things.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:59
> + at property (nonatomic, readonly) NSURL *_Nonnull url;

Could avoid the lowercase url by naming this wrappedURL (or targetURL, or
underlyingURL).

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:67
> +    if (auto *font = dynamic_objc_cast<NSFont>(object)) {

I slightly prefer:

    if (auto font = xxx

Maybe we could debate the merits?

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

Ditto.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:82
> +    auto objectPtr = adoptNS(object);

Not sure objectPtr is a great name. After all "object" is already a pointer.
Maybe adoptedObject?

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:83
> +    if (auto *encodedURL = dynamic_objc_cast<WKEncodedURL>(objectPtr.get()))

Same comment about auto.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:91
> + at implementation WKEncodedURL {

I don’t think "encoded URL" is a great name for this. The URL in this object is
not "encoded". This is a proxy that facilitates encoding and decoding the URL.
It does in fact function as the "decoded URL" in the decoding process, but
during the encoding process it is not at all the "encoded" URL.

I would name this class something like "WKURLArchivalWrapper" or
"WKSecureCodingURLWrapper".

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:92
> +    RetainPtr<NSURL> m_url;

Could avoid the lowercase url by naming this m_wrappedURL (or m_targetURL or
m_underlyingURL).

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:125
> +	       m_url = retainPtr([NSURL URLWithString:@""]);

Should not need to call retainPtr here.

But also shouldn’t we return nil here, letting self get released, instead of
setting the URL to ""? We want to correctly indicate failure rather than
emptying out the URL and incorrectly indicating success.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:196
> +static inline bool isSerializableFont(CTFontRef _Nullable font)

What determines where we use "nullable" and where we use "_Nullable"?

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:441
> +    auto allowedClassSet = [NSMutableSet setWithArray:allowedClasses];

Could remove one autorelease by doing this instead:

    auto allowedClassSet = adoptNS([[NSMutableSet alloc]
initWithArray:allowedClasses]);