Bug 203579 - Web Inspector: Add diagnostic logging for frontend feature usage
Summary: Web Inspector: Add diagnostic logging for frontend feature usage
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Matt Baker
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-10-29 12:49 PDT by Matt Baker
Modified: 2019-11-02 00:36 PDT (History)
20 users (show)

See Also:


Attachments
WIP (28.77 KB, patch)
2019-10-29 12:52 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (25.83 KB, patch)
2019-10-30 15:29 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (25.91 KB, patch)
2019-10-30 15:33 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (26.49 KB, patch)
2019-10-31 16:59 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (26.55 KB, patch)
2019-10-31 17:04 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (78.25 KB, patch)
2019-11-01 17:02 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (77.66 KB, patch)
2019-11-01 17:39 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (77.64 KB, patch)
2019-11-01 23:53 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Baker 2019-10-29 12:49:57 PDT
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.
Comment 1 Radar WebKit Bug Importer 2019-10-29 12:50:27 PDT
<rdar://problem/56717410>
Comment 2 Matt Baker 2019-10-29 12:52:26 PDT
Created attachment 382207 [details]
WIP
Comment 3 Matt Baker 2019-10-30 15:29:22 PDT
Created attachment 382369 [details]
Patch
Comment 4 Matt Baker 2019-10-30 15:33:59 PDT
Created attachment 382371 [details]
Patch
Comment 5 Devin Rousso 2019-10-30 21:49:25 PDT
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
Comment 6 Brian Burg 2019-10-31 12:00:57 PDT
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.
Comment 7 Matt Baker 2019-10-31 16:22:13 PDT
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.
Comment 8 Matt Baker 2019-10-31 16:59:56 PDT
Created attachment 382521 [details]
Patch
Comment 9 Matt Baker 2019-10-31 17:04:30 PDT
Created attachment 382522 [details]
Patch
Comment 10 Matt Baker 2019-10-31 17:04:53 PDT
(In reply to Matt Baker from comment #9)
> Created attachment 382522 [details]
> Patch

I added `useCapture` to the keyboard and mouse listeners.
Comment 11 Devin Rousso 2019-11-01 17:02:20 PDT
Created attachment 382655 [details]
Patch
Comment 12 Devin Rousso 2019-11-01 17:39:01 PDT
Created attachment 382663 [details]
Patch
Comment 13 Brian Burg 2019-11-01 23:06:27 PDT
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?
Comment 14 Devin Rousso 2019-11-01 23:51:28 PDT
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.
Comment 15 Devin Rousso 2019-11-01 23:53:41 PDT
Created attachment 382670 [details]
Patch
Comment 16 WebKit Commit Bot 2019-11-02 00:36:32 PDT
Comment on attachment 382670 [details]
Patch

Clearing flags on attachment: 382670

Committed r251963: <https://trac.webkit.org/changeset/251963>
Comment 17 WebKit Commit Bot 2019-11-02 00:36:34 PDT
All reviewed patches have been landed.  Closing bug.