[webkit-reviews] review granted: [Bug 222296] [Cocoa] Web Inspector: Add support for reloading the inspected page via _WKInspectorExtension : [Attachment 421340] Patch v1.1 - Review notes

[bugzilla-daemon at webkit.org]

BJ Burg <bburg at apple.com> has granted Patrick Angle <pangle at apple.com>'s
request for review:
Bug 222296: [Cocoa] Web Inspector: Add support for reloading the inspected page
via _WKInspectorExtension
https://bugs.webkit.org/show_bug.cgi?id=222296

Attachment 421340: Patch v1.1 - Review notes

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




--- Comment #5 from BJ Burg <bburg at apple.com> ---
Comment on attachment 421340
  --> https://bugs.webkit.org/attachment.cgi?id=421340
Patch v1.1 - Review notes

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

r=me with removal of unnecessary ExceptionDetails parameter.

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorExtension.h:68
> + * @param injectedScript If specified, injects the given JavaScript
experssion into all frames on the page before any other scripts.

Nit: expression

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorExtension.h:70
> + * @discussion The completionHandler is passed an
NSJSONSerialization-compatible NSObject representing an error or null.

Nit: I would just remove the @discussion

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:284
> +void WebInspectorUIExtensionController::reloadForExtension(const
InspectorExtensionID& extensionID, const Optional<bool>& ignoreCache, const
Optional<String>& userAgent, const Optional<String>& injectedScript,
CompletionHandler<void(const Optional<WebCore::ExceptionDetails>&, const
Optional<InspectorExtensionError>&)>&& completionHandler)

The WebCore::ExceptionDetails return parameter is never filled in or used. So,
it can be removed here and from the .messages.in file. We only need to return
Optional<InspectorExtensionError> here.