[webkit-reviews] review granted: [Bug 205174] Web Inspector: add InspectedTargetTypes diagnostic event and related hooks : [Attachment 386247] Rebased patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Brian Burg <bburg at apple.com>'s
request for review:
Bug 205174: Web Inspector: add InspectedTargetTypes diagnostic event and
related hooks
https://bugs.webkit.org/show_bug.cgi?id=205174

Attachment 386247: Rebased patch

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




--- Comment #7 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 386247
  --> https://bugs.webkit.org/attachment.cgi?id=386247
Rebased patch

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

r=me, this looks fine.	I'm slightly concerned with other ports/platforms, so
please make sure that they don't get screwed by this change.  I'm going to cq-
for now as there are still build issues, but feel free to land once they are
resolved.

> Source/WebKit/ChangeLog:15
> +	   For remote Web Inspector, WebKit clients can populate an instance of

> +	   _WKInspectorDebuggableInfo and use it when calling into
> +	   -[_WKRemoteWebInspectorViewController
loadForDebuggable:backendCommandsURL:].

Do we need to get ITML to do anything for this?  Or would that only be required
if/when <https://webkit.org/b/203300> lands?

> Source/WebKit/ChangeLog:18
> +	   For local Web Inspector, WebInspectorProxy fills in information for
the local
> +	   debuggable by consulting SystemVersion.plist (on Mac port).

Can you create/link bugs for the other ports so that they know if they need to
make changes?

> Source/WebCore/inspector/InspectorFrontendClient.h:40
> +enum class DebuggableType : uint8_t;

With something this small/specific, is it worth the forward declare?  Can we
not just include it?

> Source/WebCore/inspector/InspectorFrontendHost.cpp:299
> +    }

Should we add a `ASSERT_NOT_REACHED();` with a fallback return value, like
`return "javascript"_s;`?

> Source/WebCore/inspector/InspectorFrontendHost.cpp:305
> +	   return {debuggableTypeToString(DebuggableType::Page), "Unknown"_s,
"Unknown"_s, "Unknown"_s, false};

We normally fall back to `DebuggableType::JavaScript`, as those domains are
guaranteed to exist.

> Source/WebCore/testing/Internals.cpp:346
> +    String targetPlatformName() const { return "Unknown"_s; }
> +    String targetBuildVersion() const { return "Unknown"_s; }
> +    String targetProductVersion() const { return "Unknown"_s; }

Why not call this "test", or something more specific?

> Source/WebInspectorUI/UserInterface/Base/DebuggableType.js:43
> +    }

Please add:
```
    console.assert(false, "Unknown debuggable type", type);
    return null;
```

>
Source/WebInspectorUI/UserInterface/Controllers/InspectedTargetTypesDiagnosticE
ventRecorder.js:37
> +    // In milliseconds.

NIT: I'd put this comment inline in the function's body after the `;`

>
Source/WebInspectorUI/UserInterface/Controllers/InspectedTargetTypesDiagnosticE
ventRecorder.js:86
> +	   let payload = {

Can we skip the variable and just write the multi-line object as part of the
function call?

>
Source/WebInspectorUI/UserInterface/Controllers/InspectedTargetTypesDiagnosticE
ventRecorder.js:100
> +	   return this._cachedDebuggableInfo.debuggableType || "Unknown";

Wouldn't it be better to have these fallbacks inside
`_ensureCachedDebuggableInfo` so that they don't have to be computed each time
these functions are called?

> Source/WebKit/Shared/DebuggableInfoData.cpp:82
> +	   *debuggableType,

NIT: I usually prefer using `.value()` for optionals, so I know that it's not a
pointer.

> Source/WebKit/Shared/WebCoreArgumentCoders.h:925
> +template<> struct EnumTraits<Inspector::DebuggableType> {

Why not put this in the file so that if a new `Inspector::DebuggableType` is
added it's in the same place?

> Source/WebKit/UIProcess/API/APIDebuggableInfo.h:39
> +    DebuggableInfo() = default;

NIT: I'd either make this private or put it above the destructor.

> Source/WebKit/UIProcess/API/APIDebuggableInfo.h:42
> +    void setDebuggableType(const Inspector::DebuggableType debuggableType) {
m_data.debuggableType = debuggableType; }

Why bother with the `const` if it's a pass-by-copy?

> Source/WebKit/UIProcess/API/APIDebuggableInfo.h:44
> +    WTF::String targetPlatformName() const { return
m_data.targetPlatformName; }

Should we return a `const WTF::String&` instead?

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorDebuggableInfoInternal.h:59
> +    return Inspector::DebuggableType::Page;

Ditto (InspectorFrontendHost.cpp:305)

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorDebuggableInfoInternal.h:76
> +    return _WKInspectorDebuggableTypeWebPage;

Ditto (InspectorFrontendHost.cpp:305)

At the very least, I think this should be `Page`, not `WebPage`.

> Source/WebKit/UIProcess/API/Cocoa/_WKRemoteWebInspectorViewController.mm:122
> +    debuggableInfo.targetPlatformName = @"macOS";

Why is the target "macOS", and not "iOS"?  This is for remote inspection, so
it's probably very unlikely to be on macOS.

> Source/WebKit/WebProcess/WebPage/RemoteWebInspectorUI.cpp:57
> +    m_debuggableInfo = debuggableInfo;

Don't you want to `WTFMove(debuggableInfo)` then?

> Source/WebKit/WebProcess/WebPage/WebInspectorUI.h:157
> +    DebuggableInfoData m_debuggableInfo { DebuggableInfoData::empty() };

Can we do this in the constructor in WebInspectorUI.cpp instead?  Perhaps we
can even make it `const`?

> Source/WebKitLegacy/mac/WebCoreSupport/WebInspectorClient.h:125
> +    String targetBuildVersion() const final { return "Unknown"_s; };
> +    String targetProductVersion() const final { return "Unknown"_s; };

Why can't this also look at SystemVersion.plist?