[webkit-reviews] review granted: [Bug 217783] [Cocoa] Inspector Extensions: Add _WKInspectorExtension and related plumbing : [Attachment 413110] Patch v2.0

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Brian Burg <bburg at apple.com>'s
request for review:
Bug 217783: [Cocoa] Inspector Extensions: Add _WKInspectorExtension and related
plumbing
https://bugs.webkit.org/show_bug.cgi?id=217783

Attachment 413110: Patch v2.0

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




--- Comment #9 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 413110
  --> https://bugs.webkit.org/attachment.cgi?id=413110
Patch v2.0

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

r=me, nice work!

>
Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController
.js:34
> +

missing `// Public`

>
Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController
.js:45
> +	   console.log("Registered extension: ", extensionID, displayName);

please remove all `console` other than `console.assert` as they are not
stripped from production builds

>
Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController
.js:51
> +	   if (!this._extensionIDMap.has(extensionID)) {

you could rework this to avoid the double (triple) lookup
```
    let extension = this._extensionIDMap.take(extensionID);
    if (!extension) {
	WI.reportInternalError("Unable to unregister extension with ID: ",
extensionID);
	return WI.WebInspectorExtension.ErrorCode.InvalidRequest;
    }

   
this.dispatchEventToListeners(WI.WebInspectorExtensionController.Event.Extensio
nRemoved, {extension});
```

>
Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController
.js:57
> +	   let extension = this._extensionIDMap.get(extensionID);
> +	   this._extensionIDMap.delete(extensionID);

`this._extensionIDMap.take(extensionID)`

>
Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController
.js:59
> +	   console.log("Unregistered extension: ", extensionID);

ditto (:45)

> Source/WebInspectorUI/UserInterface/Models/WebInspectorExtension.js:26
> +WI.WebInspectorExtension = class WebInspectorExtension extends WI.Object

this doesn't need to extend `WI.Object` if it's not dispatching any events

> Source/WebInspectorUI/UserInterface/Protocol/InspectorFrontendAPI.js:211
>      }

Style: missing comma

> Source/WebKit/Shared/InspectorExtensionTypes.cpp:35
> +WTF::String inspectorExtensionErrorToString(InspectorExtensionError error)

NIT: is the `WTF::` necessary?

> Source/WebKit/Shared/InspectorExtensionTypes.cpp:53
> +    return "InternalError";

`_s`

> Source/WebKit/Shared/InspectorExtensionTypes.h:35
> +using InspectorExtensionTabID = WTF::String;
> +using InspectorExtensionID = WTF::String;

NIT: I think the extension ID should go before the tab ID (both alphabetically
and conceptually)

> Source/WebKit/Shared/InspectorExtensionTypes.h:38
> +    InternalError,

This doesn't appear to be used anywhere.  Can we defer adding it until it's
actually needed?

> Source/WebKit/Shared/InspectorExtensionTypes.h:40
> +    InvalidState,

ditto (:38)

> Source/WebKit/Shared/InspectorExtensionTypes.h:43
> +    ResourceLoadFailed,

ditto (:38)

> Source/WebKit/UIProcess/Inspector/WebInspectorUIExtensionControllerProxy.h:42
> +    , public CanMakeWeakPtr<WebInspectorUIExtensionControllerProxy> {

i dunno what WebKit style is, but I'd personally put the `{` on a newline
(unindented) when there are multiple lines for inheritance

> Source/WebKit/UIProcess/Inspector/WebInspectorUIExtensionControllerProxy.h:60
> +    bool m_frontendLoaded { false };

NIT: can we move this below `m_frontendLoadedCallbackQueue` for (potentially)
better packing?

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:44
> +    Page* page = inspectorFrontend.frontendPage();
> +    ASSERT(page);

If we expect this to always be valid, can we use `Page&` instead?

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:63
> +	   if (resultString == "InternalError")

`_s` (below too)

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:75
> +	   // If the evaluation result is a string, the frontend returned an
error string.
> +	   // Anything else (falsy values, objects, arrays, DOM, etc.) is
interpreted as success.

NIT: I'd move this right before the `if (result.isString()) {`

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:107
> +	   completionHandler(true);

If you're only ever sending back `true`, then the `bool` is kinda pointless. 
Could you `void(Optional<InspectorExtensionError>)` instead (i.e.
`WTF::nullopt` is "no problem")?

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:130
> +	   completionHandler(true);

ditto (:107)

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.h:49
> +    , public CanMakeWeakPtr<WebInspectorUIExtensionController> {

ditto (WebInspectorUIExtensionControllerProxy.h:42)

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.h:64
> +    Optional<InspectorExtensionError>
parseInspectorExtensionErrorFromResult(JSC::JSValue result);

forward declare `JSC::JSValue`?

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.h:66
> +    WeakPtr<WebCore::InspectorFrontendClient> m_inspectorFrontend;

NIT: shouldn't this really be called `m_inspectorFrontendClient` (or just
`m_frontendClient`)?