<rdar://71208668>
Created attachment 421268 [details] Patch v1.0
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).
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).
Created attachment 421340 [details] Patch v1.1 - Review notes
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.
Created attachment 421359 [details] Patch v1.2 - Review notes
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
ChangeLog entry in Source/WebInspectorUI/ChangeLog contains OOPS!.
Created attachment 421365 [details] Patch v1.2.1 - Review notes, now with reviewer in both changelogs
Created attachment 421368 [details] Patch v1.3 - Review notes
Committed r273365: <https://commits.webkit.org/r273365> All reviewed patches have been landed. Closing bug and clearing flags on attachment 421368 [details].