[webkit-reviews] review granted: [Bug 205239] Add WKWebView SPI to evaluate a function with arguments : [Attachment 386461] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has granted Brady Eidson
<beidson at apple.com>'s request for review:
Bug 205239: Add WKWebView SPI to evaluate a function with arguments
https://bugs.webkit.org/show_bug.cgi?id=205239

Attachment 386461: Patch

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




--- Comment #49 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 386461
  --> https://bugs.webkit.org/attachment.cgi?id=386461
Patch

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

r=me with lots of nits.

> Source/WebCore/bindings/js/RunJavaScriptParameters.h:40
> +    RunJavaScriptParameters(const String& source, bool runAsAsyncFunction,
Optional<ArgumentWireBytesMap> arguments, bool forceUserGesture)

I think if we remove both of these constructors and just use WTFMove in the
decoder, it should do everything Darin hoped.

> Source/WebCore/bindings/js/ScriptController.cpp:569
> +    auto result = executeScriptInWorld(world, { script, false, WTF::nullopt,
forceUserGesture });

It would probably be worth putting RunJavaScriptParameters here so we find it
easier when we add more members to RunJavaScriptParameters that have
initializers and this will continue to successfully compile.

> Source/WebCore/bindings/js/ScriptController.cpp:589
> +    default:
> +	   RELEASE_ASSERT_NOT_REACHED();

I think it would be better to not have a default here.

> Source/WebCore/bindings/js/ScriptController.cpp:614
> +	       errorMessage = "Unable to deserialize argument to execute
asynchronous JavaScript function";
> +	       break;

Should we return the exception here?

> Source/WebCore/bindings/js/ScriptController.h:99
> +    using ResolveFunction = WTF::Function<void(ValueOrException)>;

Could this be a CompletionHandler, because it needs to be called once?
WTF:: unnecessary in either case.

> Source/WebCore/xml/XPathGrammar.y:404
> +	   $$ = new XPath::Path(std::unique_ptr<Expression>($1),
std::unique_ptr<LocationPath>($3));

This could use a comment in the ChangeLog saying it's unrelated but necessary
to continue compiling for some reason.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:904
> +    _page->runJavaScriptInMainFrame({ javaScriptString,
(bool)asAsyncFunction, WTFMove(argumentsMap), (bool)forceUserGesture },
[handler](API::SerializedScriptValue* serializedScriptValue,
Optional<WebCore::ExceptionDetails> details, WebKit::ScriptValueCallback::Error
errorCode) {

I think we prefer !! or static_cast<bool> over c-style casting to bool.
Also, we should move handler into the capture list.

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:319
> +- (void)_callAsyncFunction:(NSString *)javaScriptString
withArguments:(NSDictionary *)arguments completionHandler:(void (^)(id, NSError
*error))completionHandler;

Could this be NSDictionary<NSString *, id> *?  It seems the keys are the
variable names.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3382
> +	   send(Messages::WebPageProxy::ScriptValueCallback(dataReference,
details, callbackID));

Maybe as a followup we could use sendWithAsyncReply instead of two messages and
manually generating and keeping track of this identifier.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/AsyncFunction.mm:92
> +    // Values can only be NSString, NSNumber, NSDate, NSArray, NSDictionary,
and may only contain those 5 types.

The return value seems to be able to be NSNull.  Can a parameter? Either way, 
I think that would be a great test case.
Also, what about NSMutableDictionary?  Also, what about custom classes that
inherit from these types?