Bug 205174 – Web Inspector: add InspectedTargetTypes diagnostic event and related hooks

• Component: Web Inspector
• Status: RESOLVED FIXED

[Blaze Burg]

.

[Radar WebKit Bug Importer]

<rdar://problem/57887953>

[Blaze Burg]

Created attachment 386146 [details]
Patch

[Blaze Burg]

Created attachment 386148 [details]
Patch

[Blaze Burg]

Comment on attachment 386148 [details]
Patch

Wrong bug. I have exceeded the capabilities of webkit-patch's tiny brain.

[Blaze Burg]

Created attachment 386156 [details]
Patch

[Blaze Burg]

Created attachment 386247 [details]
Rebased patch

[Devin Rousso]

Comment on attachment 386247 [details]
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/InspectedTargetTypesDiagnosticEventRecorder.js:37
> +    // In milliseconds.

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

> Source/WebInspectorUI/UserInterface/Controllers/InspectedTargetTypesDiagnosticEventRecorder.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/InspectedTargetTypesDiagnosticEventRecorder.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?

[Blaze Burg]

Comment on attachment 386247 [details]
Rebased patch

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

>> Source/WebKit/ChangeLog:15
>> +        -[_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?

That would be dependent on 203300. I think that patch will need to be reworked to use this new DebuggableInfo approach.

>> Source/WebKit/ChangeLog:18
>> +        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?

OK

>> 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?

That sounds reasonable.

>> Source/WebCore/inspector/InspectorFrontendHost.cpp:299
>> +    }
> 
> Should we add a `ASSERT_NOT_REACHED();` with a fallback return value, like `return "javascript"_s;`?

Windows seems to complain w/o, so I'll add it.

>> 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.

OK

>> Source/WebInspectorUI/UserInterface/Base/DebuggableType.js:43
>> +    }
> 
> Please add:
> ```
>     console.assert(false, "Unknown debuggable type", type);
>     return null;
> ```

OK

>> Source/WebInspectorUI/UserInterface/Controllers/InspectedTargetTypesDiagnosticEventRecorder.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?

OK

>> 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?

Oh, okay, didn't realize that approach was more modern.

>> Source/WebKit/UIProcess/API/APIDebuggableInfo.h:44
>> +    WTF::String targetPlatformName() const { return m_data.targetPlatformName; }
> 
> Should we return a `const WTF::String&` instead?

OK x 3

>> 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`.

OK

>> 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.

After this patch lands we'll know just how unlikely. =)

This code path will be removed within a week so I'm not going to sweat it.

>> Source/WebKit/WebProcess/WebPage/RemoteWebInspectorUI.cpp:57
>> +    m_debuggableInfo = debuggableInfo;
> 
> Don't you want to `WTFMove(debuggableInfo)` then?

oops yep.

>> Source/WebKitLegacy/mac/WebCoreSupport/WebInspectorClient.h:125
>> +    String targetProductVersion() const final { return "Unknown"_s; };
> 
> Why can't this also look at SystemVersion.plist?

Didn't bother, as we don't support Inspector on Mac WK1.

[Blaze Burg]

Created attachment 386287 [details]
Patch v1.1

[Blaze Burg]

Created attachment 386288 [details]
Patch v1.2

[Blaze Burg]

Created attachment 386289 [details]
Patch v1.3

[Blaze Burg]

Created attachment 386290 [details]
Patch v1.3

[Blaze Burg]

Created attachment 386291 [details]
Patch v1.4

[Blaze Burg]

Created attachment 386298 [details]
Patch v1.4

[Blaze Burg]

Windows EWS appears to be compiling. I'll land this today after the others settle.

[Blaze Burg]

Windows EWS appears to be compiling. I'll land this today after the others settle.

[Blaze Burg]

Created attachment 386305 [details]
Patch v1.4

[Blaze Burg]

WPE had one more little thing. Waiting for all greens.

[WebKit Commit Bot]

Comment on attachment 386305 [details]
Patch v1.4

Clearing flags on attachment: 386305

Committed r253868: <https://trac.webkit.org/changeset/253868>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ross Kirsling]

Comment on attachment 386305 [details]
Patch v1.4

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

> Source/WebKit/UIProcess/socket/RemoteInspectorClient.cpp:244
> -            || !itemObject->getString("type"_s, target.type))
> +            || !itemObject->getString("type"_s, debuggableTypeToString(target.type)))

Whoa -- apparently MSVC didn't complain about this (clang-cl does), but this is loading a string *into* an intermediate...?!

I'm really confused because if this patch also changed Target to hold a DebuggableType value instead of a String, then the only change to this line should have been getString -> getInteger, right?

[Ross Kirsling]

(In reply to Ross Kirsling from comment #21)
> Comment on attachment 386305 [details]
> Patch v1.4
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=386305&action=review
> 
> > Source/WebKit/UIProcess/socket/RemoteInspectorClient.cpp:244
> > -            || !itemObject->getString("type"_s, target.type))
> > +            || !itemObject->getString("type"_s, debuggableTypeToString(target.type)))
> 
> Whoa -- apparently MSVC didn't complain about this (clang-cl does), but this
> is loading a string *into* an intermediate...?!
> 
> I'm really confused because if this patch also changed Target to hold a
> DebuggableType value instead of a String, then the only change to this line
> should have been getString -> getInteger, right?

Er rather the message still has a string field; (In reply to Ross Kirsling from comment #21)
> Comment on attachment 386305 [details]
> Patch v1.4
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=386305&action=review
> 
> > Source/WebKit/UIProcess/socket/RemoteInspectorClient.cpp:244
> > -            || !itemObject->getString("type"_s, target.type))
> > +            || !itemObject->getString("type"_s, debuggableTypeToString(target.type)))
> 
> Whoa -- apparently MSVC didn't complain about this (clang-cl does), but this
> is loading a string *into* an intermediate...?!
> 
> I'm really confused because if this patch also changed Target to hold a
> DebuggableType value instead of a String, then the only change to this line
> should have been getString -> getInteger, right?

Sorry, I misspoke -- socket inspector's messaging protocol is still using the string name of the debuggable type, so the change here should have been to getString to a local lvalue and then call parseDebuggableTypeFromString, not debuggableTypeToString.

Immediately though, I think it's simpler to not change socket inspector's Target struct at all until we actually change its messaging protocol. As such, I'll revert this part and add a FIXME to simplify the messaging.

[Blaze Burg]

(In reply to Ross Kirsling from comment #22)
> (In reply to Ross Kirsling from comment #21)
> > Comment on attachment 386305 [details]
> > Patch v1.4
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=386305&action=review
> > 
> > > Source/WebKit/UIProcess/socket/RemoteInspectorClient.cpp:244
> > > -            || !itemObject->getString("type"_s, target.type))
> > > +            || !itemObject->getString("type"_s, debuggableTypeToString(target.type)))
> > 
> > Whoa -- apparently MSVC didn't complain about this (clang-cl does), but this
> > is loading a string *into* an intermediate...?!
> > 
> > I'm really confused because if this patch also changed Target to hold a
> > DebuggableType value instead of a String, then the only change to this line
> > should have been getString -> getInteger, right?
> 
> Er rather the message still has a string field; (In reply to Ross Kirsling
> from comment #21)
> > Comment on attachment 386305 [details]
> > Patch v1.4
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=386305&action=review
> > 
> > > Source/WebKit/UIProcess/socket/RemoteInspectorClient.cpp:244
> > > -            || !itemObject->getString("type"_s, target.type))
> > > +            || !itemObject->getString("type"_s, debuggableTypeToString(target.type)))
> > 
> > Whoa -- apparently MSVC didn't complain about this (clang-cl does), but this
> > is loading a string *into* an intermediate...?!
> > 
> > I'm really confused because if this patch also changed Target to hold a
> > DebuggableType value instead of a String, then the only change to this line
> > should have been getString -> getInteger, right?
> 
> Sorry, I misspoke -- socket inspector's messaging protocol is still using
> the string name of the debuggable type, so the change here should have been
> to getString to a local lvalue and then call parseDebuggableTypeFromString,
> not debuggableTypeToString.
> 
> Immediately though, I think it's simpler to not change socket inspector's
> Target struct at all until we actually change its messaging protocol. As
> such, I'll revert this part and add a FIXME to simplify the messaging.

Sounds fine to me, I was mainly concerned with keeping it compiling on all ports.