Bug 222296 – [Cocoa] Web Inspector: Add support for reloading the inspected page via _WKInspectorExtension

• Component: Web Inspector
• Status: RESOLVED FIXED

[Patrick Angle]

<rdar://71208668>

[Patrick Angle]

Created attachment 421268 [details]
Patch v1.0

[Blaze Burg]

Comment on attachment 421268 [details]
Patch v1.0

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

r-, need to remove unnecessary argument from API method's completion handler.

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:149
> +        for (let target of WI.targets) {

I'm pretty sure it would be enough to reload the main target only? Devin?

If you keep this code, please remove unnecessary braces.

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:151
> +        }

This should return a promise in the happy path so that the originating call to -[WKInspectorExtension reloadWithOptions:] does not complete prior to the command being handled (i.e., it could return an error that should be propagated).

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorExtension.h:67
> + * @param userAgent If specified, sets a custom user agent for the page to be sent in the `User-Agent` header and returned by calls to `navigator.userAgent` made by scripts running in the page.

I think these options are applicable to the next navigation only, right? I would mention that in the @discussion.

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorExtension.h:72
> +- (void)reloadIgnoringCache:(BOOL)ignoreCache userAgent:(NSString *)userAgent injectedScript:(NSString *)injectedScript completionHandler:(void(^)(NSError * _Nullable, NSDictionary * _Nullable result))completionHandler;

There is no return value for this operation, you can drop the 2nd parameter of the completion handler and associated comments/code (throughout).

[Devin Rousso]

Comment on attachment 421268 [details]
Patch v1.0

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

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:138
> +        // FIXME: <rdar://problem/74180355> implement execution context selection options

NIT: i usually prefer to create and include a bugzilla bug so that if an open source contributor comes along they can get the details and try to implement it themselves :)

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:149
>> +        for (let target of WI.targets) {
> 
> I'm pretty sure it would be enough to reload the main target only? Devin?
> 
> If you keep this code, please remove unnecessary braces.

In the case of `Page.reload`, yeah I think this needs to be `WI.mainTarget` only since we want to reload the entire page (i.e. if we're already reloading the main frame, there's no point in _also_ reloading subframes), but I applaud you for trying to do future-proof multi-target support :)

For future reference, you'll almost always want a `target.hasCommand("Page.reload")` check before almost all commands because not all targets are the same (e.g. a JS `Worker` target doesn't have a `Page` domain/agent).  You can verify this for yourself by looking at the value of `targetTypes` in the protocol JSON files (if there isn't one then it's assumed that the domain/command/event is supported on all targets).

[Patrick Angle]

Created attachment 421340 [details]
Patch v1.1 - Review notes

[Blaze Burg]

Comment on attachment 421340 [details]
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.

[Patrick Angle]

Created attachment 421359 [details]
Patch v1.2 - Review notes

[Devin Rousso]

Comment on attachment 421359 [details]
Patch v1.2 - Review notes

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

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:150
> +        if (target.hasCommand("Page.reload"))

Style: i'd flip this around and have it early-return with `WI.WebInspectorExtension.ErrorCode.InvalidRequest` to match the pattern above

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorExtension.mm:93
> +            capturedBlock([NSError errorWithDomain:WKErrorDomain code:WKErrorUnknown userInfo:@{ NSLocalizedFailureReasonErrorKey: WebKit::inspectorExtensionErrorToString(result.error())}]);

Style: there should either be spaces before both `{`  and `}` or neither

[EWS]

ChangeLog entry in Source/WebInspectorUI/ChangeLog contains OOPS!.

[Patrick Angle]

Created attachment 421365 [details]
Patch v1.2.1 - Review notes, now with reviewer in both changelogs

[Patrick Angle]

Created attachment 421368 [details]
Patch v1.3 - Review notes

[EWS]

Committed r273365: <https://commits.webkit.org/r273365>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421368 [details].