Web Inspector: Add diagnostic logging for frontend feature usage

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

Summary:
Add diagnostic logging for frontend feature usage.

This patch adds a new functionality to InspectorFrontendHost, a new DiagnosticController class, and backend plumbing in WebKit for logging Web Inspector diagnostic events through the existing DiagnosticLoggingClient/_WKDiagnosticLoggingDelegate mechanism.

[Radar WebKit Bug Importer]

<rdar://problem/56717410>

[Matt Baker]

Created attachment 382207 [details]
WIP

[Matt Baker]

Created attachment 382369 [details]
Patch

[Matt Baker]

Created attachment 382371 [details]
Patch

[Devin Rousso]

Comment on attachment 382371 [details]
Patch

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

> Source/WebCore/inspector/InspectorFrontendClient.h:88
> +    virtual bool supportsDiagnosticLogging() = 0;

I don't think this is something we need to require all subclasses decide.  Why not give this a default implementation and then just let subclasses override it if they want to say "yes"?
```
    virtual bool supportsDiagnosticLogging() { return false; }
```
We could do the same for `logDiagnosticEvent` too.  That way, most of the stub functions can be dropped.

> Source/WebCore/inspector/InspectorFrontendClient.h:89
> +    virtual void logDiagnosticEvent(const WTF::String& eventName, const DiagnosticLoggingClient::ValueDictionary& payload) = 0;

Style: you can drop the `WTF::`

> Source/WebCore/inspector/InspectorFrontendHost.cpp:520
> +#if PLATFORM(COCOA)

Wouldn't it be really more accurate to use `#if PLATFORM(MAC)`, since we don't have a UI on iOS?

> Source/WebCore/inspector/InspectorFrontendHost.cpp:521
> +static Optional<DiagnosticLoggingClient::ValuePayload> valuePayloadFromJSONValue(RefPtr<JSON::Value> value)

Can you make `value` a reference `RefPtr<JSON::Value>&`?

> Source/WebCore/inspector/InspectorFrontendHost.cpp:524
> +    case JSON::Value::Type::Boolean: {

NIT: do we you need the `{` and `}` around each `case`?

> Source/WebCore/inspector/InspectorFrontendHost.cpp:546
> +    default: {
> +        ASSERT_NOT_REACHED();
> +        return WTF::nullopt;

NIT: I've started trying to avoid using `default` as it makes it harder to find places where we need to have a `case` for an enum value at build time.  Since each `case` in this function does a `return`, we could move both of these outside the `switch` entirely.

> Source/WebCore/inspector/InspectorFrontendHost.cpp:554
> +#if PLATFORM(COCOA)

Alternatively, rather than have the platform code to unpack the payload be here, can't we just pass along the `eventName` and `payload` to the `m_client` and have this specific logic only exist for the `PLATFORM(COCOA)` clients?

> Source/WebInspectorUI/UserInterface/Base/Main.js:527
> +    WI.diagnosticController = new WI.DiagnosticController;

Should we only be creating this inside a `if (WI.DiagnosticController.supportsDiagnosticLogging())`?

> Source/WebInspectorUI/UserInterface/Controllers/DiagnosticController.js:35
> +        document.addEventListener("blur", this._documentBlurred.bind(this));

Should we be adding these event listeners during the capture phase, so nothing inside Web Inspector can call `stopPropagation` on it?

> Source/WebInspectorUI/UserInterface/Controllers/DiagnosticController.js:44
> +    static supportsDiagnosticLogging()

Style: this should have a "// Static" comment before it.

> Source/WebInspectorUI/UserInterface/Controllers/DiagnosticController.js:49
> +    logDiagnosticMessage(message, payload)

Style: this should have a "// Public" comment before it.

Shouldn't `message` be called `messageType`, or really `eventType` to match `InspectorFrontendHost`?

> Source/WebInspectorUI/UserInterface/Controllers/DiagnosticController.js:52
> +        console.assert(message in WI.DiagnosticController.MessageType, "Unexpected diagnostic message " + message);

Style: I realize that the value is the same as the key right now, but that may not be the case in the future, so please change this to a `Object.values(WI.DiagnosticController.MessageType).includes(messageType)`.

> Source/WebInspectorUI/UserInterface/Controllers/DiagnosticController.js:112
> +        let interval = WI.DiagnosticController.ActivityTimingResolution;

Why are we also logging the interval?  Is this needed for something?

> Source/WebInspectorUI/UserInterface/Controllers/DiagnosticController.js:113
> +        this.logDiagnosticMessage(DiagnosticController.MessageType.TabActivity, {tabType, interval});

So this means we are logging information at most once per minute?  I thought we were going to do accumulation in the frontend and log things before Web Inspector closes, with more information?

> Source/WebInspectorUI/UserInterface/Controllers/DiagnosticController.js:122
> +            this._tabActivityTimeoutidentifier = 0;

NIT: we normally reset timeout identifiers back to `undefined`

> Source/WebKit/UIProcess/mac/WKInspectorViewController.mm:123
> +    preferences._diagnosticLoggingEnabled = YES;

Doesn't this need to be added to 'Settings.yaml'?  Furthermore, why are we even adding this setting?  Do we wan't to allow toggling of these diagnostics on/off?  If so, is there UI for that somewhere?

> Source/WebKit/WebProcess/WebPage/RemoteWebInspectorUI.cpp:180
> +    m_page.corePage()->diagnosticLoggingClient().logDiagnosticMessageWithValueDictionary(eventName, String(), dictionary, ShouldSample::No);

Do we want to include some basic description at the very least, like "Web Inspector Frontend Diagnostics"?

> Source/WebKitLegacy/ios/WebCoreSupport/WebInspectorClientIOS.mm:139
> +void WebInspectorFrontendClient::logDiagnosticEvent(const String&, const WebCore::DiagnosticLoggingClient::Dictionary&) { }

Shouldn't this be `ValueDictionary`?

> Source/WebKitLegacy/mac/WebCoreSupport/WebInspectorClient.mm:330
> +    Page* page = frontendPage();

Style: `auto`

> Source/WebKitLegacy/mac/WebCoreSupport/WebInspectorClient.mm:336
> +    if (Page* page = frontendPage())

Ditto

[BJ Burg]

Comment on attachment 382371 [details]
Patch

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

>> Source/WebCore/inspector/InspectorFrontendClient.h:88
>> +    virtual bool supportsDiagnosticLogging() = 0;
> 
> I don't think this is something we need to require all subclasses decide.  Why not give this a default implementation and then just let subclasses override it if they want to say "yes"?
> ```
>     virtual bool supportsDiagnosticLogging() { return false; }
> ```
> We could do the same for `logDiagnosticEvent` too.  That way, most of the stub functions can be dropped.

DItto.

>> Source/WebCore/inspector/InspectorFrontendHost.cpp:520
>> +#if PLATFORM(COCOA)
> 
> Wouldn't it be really more accurate to use `#if PLATFORM(MAC)`, since we don't have a UI on iOS?

If we aren't calling ObjC, is there any point using a platform guard? Why not use ENABLE(INSPECTOR_TELEMETRY) or whatever, and define it to be PLATFORM(MAC).

>> Source/WebCore/inspector/InspectorFrontendHost.cpp:521
>> +static Optional<DiagnosticLoggingClient::ValuePayload> valuePayloadFromJSONValue(RefPtr<JSON::Value> value)
> 
> Can you make `value` a reference `RefPtr<JSON::Value>&`?

Yeah this needs to be RefPtr<>& to avoid refcount churn.

>> Source/WebCore/inspector/InspectorFrontendHost.cpp:554
>> +#if PLATFORM(COCOA)
> 
> Alternatively, rather than have the platform code to unpack the payload be here, can't we just pass along the `eventName` and `payload` to the `m_client` and have this specific logic only exist for the `PLATFORM(COCOA)` clients?

It would be nice to not do this in the InspectorFrontendHost implementation, since it is something provided by the port and may not be available. Seems better to do it in WebInspectorUI::logDiagnosticEvent or in a helper that's also usable by RemoteInspectorUI.

[Matt Baker]

Comment on attachment 382371 [details]
Patch

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

>>> Source/WebCore/inspector/InspectorFrontendClient.h:88
>>> +    virtual bool supportsDiagnosticLogging() = 0;
>> 
>> I don't think this is something we need to require all subclasses decide.  Why not give this a default implementation and then just let subclasses override it if they want to say "yes"?
>> ```
>>     virtual bool supportsDiagnosticLogging() { return false; }
>> ```
>> We could do the same for `logDiagnosticEvent` too.  That way, most of the stub functions can be dropped.
> 
> DItto.

Done!

>>> Source/WebCore/inspector/InspectorFrontendHost.cpp:521
>>> +static Optional<DiagnosticLoggingClient::ValuePayload> valuePayloadFromJSONValue(RefPtr<JSON::Value> value)
>> 
>> Can you make `value` a reference `RefPtr<JSON::Value>&`?
> 
> Yeah this needs to be RefPtr<>& to avoid refcount churn.

Needs to be a `const RefPtr<>&`, but yes I can't believe I missed this. :P

>> Source/WebCore/inspector/InspectorFrontendHost.cpp:524
>> +    case JSON::Value::Type::Boolean: {
> 
> NIT: do we you need the `{` and `}` around each `case`?

Removed. Not sure why I had those to begin with, must have had a weird compile error the way this was written previously.

>> Source/WebCore/inspector/InspectorFrontendHost.cpp:546
>> +        return WTF::nullopt;
> 
> NIT: I've started trying to avoid using `default` as it makes it harder to find places where we need to have a `case` for an enum value at build time.  Since each `case` in this function does a `return`, we could move both of these outside the `switch` entirely.

I did this to avoid having explicit cases for Array, Null, and Object, but I can just add those and also put a second ASSERT_NOT_REACHED() outside the switch.

>>> Source/WebCore/inspector/InspectorFrontendHost.cpp:554
>>> +#if PLATFORM(COCOA)
>> 
>> Alternatively, rather than have the platform code to unpack the payload be here, can't we just pass along the `eventName` and `payload` to the `m_client` and have this specific logic only exist for the `PLATFORM(COCOA)` clients?
> 
> It would be nice to not do this in the InspectorFrontendHost implementation, since it is something provided by the port and may not be available. Seems better to do it in WebInspectorUI::logDiagnosticEvent or in a helper that's also usable by RemoteInspectorUI.

I'll look into another approach. This was just the simplest.

>> Source/WebInspectorUI/UserInterface/Base/Main.js:527
>> +    WI.diagnosticController = new WI.DiagnosticController;
> 
> Should we only be creating this inside a `if (WI.DiagnosticController.supportsDiagnosticLogging())`?

Good suggestion. I'll also change the property to `_diagnosticController`, since we just need to keep an instance around. At least for now, it shouldn't be used by the rest of the UI.

>> Source/WebInspectorUI/UserInterface/Controllers/DiagnosticController.js:49
>> +    logDiagnosticMessage(message, payload)
> 
> Style: this should have a "// Public" comment before it.
> 
> Shouldn't `message` be called `messageType`, or really `eventType` to match `InspectorFrontendHost`?

I'll rename to `eventName` and `EventName` to match InspectorFrontendHost.

>> Source/WebInspectorUI/UserInterface/Controllers/DiagnosticController.js:112
>> +        let interval = WI.DiagnosticController.ActivityTimingResolution;
> 
> Why are we also logging the interval?  Is this needed for something?

I added this in case we ever decide to change the interval. This provides some context as to what 'activity' means.

>> Source/WebInspectorUI/UserInterface/Controllers/DiagnosticController.js:122
>> +            this._tabActivityTimeoutidentifier = 0;
> 
> NIT: we normally reset timeout identifiers back to `undefined`

Since the spec states that setTimeout/setInterval returns a non-zero ID, I prefer having zero indicate that the ID is unset.

>> Source/WebKit/UIProcess/mac/WKInspectorViewController.mm:123
>> +    preferences._diagnosticLoggingEnabled = YES;
> 
> Doesn't this need to be added to 'Settings.yaml'?  Furthermore, why are we even adding this setting?  Do we wan't to allow toggling of these diagnostics on/off?  If so, is there UI for that somewhere?

We could eliminate the setting altogether, but I did this since it goes through the same diagnostic logging client used by the rest of WebKit. I think we should keep it this way for now.

>> Source/WebKit/WebProcess/WebPage/RemoteWebInspectorUI.cpp:180
>> +    m_page.corePage()->diagnosticLoggingClient().logDiagnosticMessageWithValueDictionary(eventName, String(), dictionary, ShouldSample::No);
> 
> Do we want to include some basic description at the very least, like "Web Inspector Frontend Diagnostics"?

This isn't used.

>> Source/WebKitLegacy/ios/WebCoreSupport/WebInspectorClientIOS.mm:139
>> +void WebInspectorFrontendClient::logDiagnosticEvent(const String&, const WebCore::DiagnosticLoggingClient::Dictionary&) { }
> 
> Shouldn't this be `ValueDictionary`?

Oops.

[Matt Baker]

Created attachment 382521 [details]
Patch

[Matt Baker]

Created attachment 382522 [details]
Patch

[Matt Baker]

(In reply to Matt Baker from comment #9)
> Created attachment 382522 [details]
> Patch

I added `useCapture` to the keyboard and mouse listeners.

[Devin Rousso]

Created attachment 382655 [details]
Patch

[Devin Rousso]

Created attachment 382663 [details]
Patch

[BJ Burg]

Comment on attachment 382663 [details]
Patch

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

r=me

> Source/WebKit/ChangeLog:9
> +        This patch enable diagnostic logging for the Web Inspector web process

Nit: enables

> Source/WebInspectorUI/UserInterface/Controllers/DiagnosticController.js:92
> +        this._tabActivityTimeoutidentifier = setTimeout(() => {

Nit: this._tabActivityTimeoutIdentifier

> Source/WebKit/UIProcess/mac/WKInspectorViewController.mm:123
> +    preferences._diagnosticLoggingEnabled = YES;

Are you aware of any existing diagnostic logging that would be bad to enable for normal Web Inspector usage?

[Devin Rousso]

Comment on attachment 382663 [details]
Patch

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

>> Source/WebKit/UIProcess/mac/WKInspectorViewController.mm:123
>> +    preferences._diagnosticLoggingEnabled = YES;
> 
> Are you aware of any existing diagnostic logging that would be bad to enable for normal Web Inspector usage?

I am not aware of any existing diagnostic logging.  This setting doesn't seem to control anything of substance, so I think we're probably fine.

[Devin Rousso]

Created attachment 382670 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 382670 [details]
Patch

Clearing flags on attachment: 382670

Committed r251963: <https://trac.webkit.org/changeset/251963>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.