Bug 222957 – [Cocoa] Add tests for ArgumentCodersCocoa

• Status: NEW
• Component: WebKit Misc.

[Brent Fulgham]

We wouldn't have encountered Bug 222856 if we had better coverage for our IPC argument coders.

Let's star that effort by hitting a few of the recent cases we've encountered.

[Radar WebKit Bug Importer]

<rdar://problem/75201311>

[Brent Fulgham]

Created attachment 422664 [details]
Patch

[Brent Fulgham]

Created attachment 422788 [details]
Patch

[Brent Fulgham]

Created attachment 422790 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 422790 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivateForTesting.h:105
> ++ (Optional<RetainPtr<id>>)_roundTripIPCEncodeDecodeObject:(id)testObject ofClass:(Class)classType;

Yuck.

[Darin Adler]

Comment on attachment 422790 [details]
Patch

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

>> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivateForTesting.h:105
>> ++ (Optional<RetainPtr<id>>)_roundTripIPCEncodeDecodeObject:(id)testObject ofClass:(Class)classType;
> 
> Yuck.

Why do we need to use Optional here? RetainPtr already has a nil value. Do we need to distinguish "nil" and "not present"?

[Sam Weinig]

Comment on attachment 422790 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewTesting.mm:360
> +    auto encoder = IPC::Encoder(IPC::MessageName::WebPage_LoadRequest, 0);
> +    encoder << testObject;
> +
> +    auto decoder = IPC::Decoder::create(encoder.buffer(), encoder.bufferSize(), nullptr, { });
> +    if (!decoder)
> +        return WTF::nullopt;
> +
> +    return IPC::decodeObject(*decoder, @[classType]);

I think we should try to just write this code directly in TestWebKitAPI if we can, rather than exposing this new class method.

[Sam Weinig]

(In reply to Sam Weinig from comment #7)
> Comment on attachment 422790 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=422790&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebViewTesting.mm:360
> > +    auto encoder = IPC::Encoder(IPC::MessageName::WebPage_LoadRequest, 0);
> > +    encoder << testObject;
> > +
> > +    auto decoder = IPC::Decoder::create(encoder.buffer(), encoder.bufferSize(), nullptr, { });
> > +    if (!decoder)
> > +        return WTF::nullopt;
> > +
> > +    return IPC::decodeObject(*decoder, @[classType]);
> 
> I think we should try to just write this code directly in TestWebKitAPI if
> we can, rather than exposing this new class method.

Actually, I think that will probably require splitting the IPC code out into its own static library, which we should probably do. But not for this. 

I think the Optional<RetainPtr<>> is very odd though.

[Brent Fulgham]

Comment on attachment 422790 [details]
Patch

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

>>> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivateForTesting.h:105
>>> ++ (Optional<RetainPtr<id>>)_roundTripIPCEncodeDecodeObject:(id)testObject ofClass:(Class)classType;
>> 
>> Yuck.
> 
> Why do we need to use Optional here? RetainPtr already has a nil value. Do we need to distinguish "nil" and "not present"?

It's a pretty common idiom in our IPC serialization code. We return an Optional (null opt) on errors. Sometimes the encoded value is null (but not an error). But maybe that's not important for this case.

>>> Source/WebKit/UIProcess/API/Cocoa/WKWebViewTesting.mm:360
>>> +    return IPC::decodeObject(*decoder, @[classType]);
>> 
>> I think we should try to just write this code directly in TestWebKitAPI if we can, rather than exposing this new class method.
> 
> Actually, I think that will probably require splitting the IPC code out into its own static library, which we should probably do. But not for this. 
> 
> I think the Optional<RetainPtr<>> is very odd though.

It's just mirroring exactly what IPC::decodeObject returns. Optional (WTF::nullopt) signifies error, Optional(RefPtr<something null>) is an encoded null value.

[Darin Adler]

(In reply to Brent Fulgham from comment #9)
> It's a pretty common idiom in our IPC serialization code. We return an
> Optional (null opt) on errors. Sometimes the encoded value is null (but not
> an error). But maybe that's not important for this case.

Yes, I’m familiar with that idiom when we support encoding nil. I think in this case we don’t need the distinction. We don’t need to test the nil support. Not using Optional<> would simplify that test header.