Bug 163345 – Web Inspector: Disable Type and Code Coverage profilers while timeline is being recorded

• Status: ASSIGNED
• Component: Web Inspector

[Nikita Vasilyev]

Steps:
1. Start recording a timeline.
2. Go to Resources or Debugger tab and select any JS resource.

Expected:
Type Coverage and Code Coverage profiler icons are disabled.

Actual:
Type Coverage and Code Coverage profiler icons aren't disabled.

[Radar WebKit Bug Importer]

<rdar://problem/28738310>

[Nikita Vasilyev]

Summary of the email discussion:

Starting a timeline recording should call RuntimeAgent.disableTypeProfiler() and RuntimeAgent.disableControlFlowProfiler().

Type and Code Coverage profilers should remain disabled until page reload (i.e. until WebInspector._mainResourceDidChange is called).

On page reload, Type and Code Coverage profilers should be re-enabled if they were previously disabled by a timeline recording.

Two examples:

Case 1:
1. Enable Type Profiler.
2. Record a timeline.
Type Profiler should be disabled.

Case 2:
1. Enable Type Profiler.
2. Record a timeline.
3. Reload the page.
Type Profiler should be enabled.

[Nikita Vasilyev]

*** Bug 165172 has been marked as a duplicate of this bug. ***

[Nikita Vasilyev]

Created attachment 296172 [details]
Patch

[Nikita Vasilyev]

Created attachment 296174 [details]
[Animated GIF] With patch applied

[Joseph Pecoraro]

Comment on attachment 296172 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Base/Main.js:178
> +    this.disableTypeProfilerUntilReloadSetting = new WebInspector.Setting("disable-type-profiler-until-reload", false);
> +    this.disableCodeCoverageUntilReloadSetting = new WebInspector.Setting("disable-code-coverage-until-reload", false);

Why are these a setting and not just a global boolean? Is there value in these starting off potentially initialized to true, we don't seem to be handling that if that is the case.

> Source/WebInspectorUI/UserInterface/Base/Main.js:190
> +    this.timelineManager.addEventListener(WebInspector.TimelineManager.Event.CapturingStarted, (startTime) => {

Hmm, I think we should be doing this on the backend as well for Auto Capture on Reload cases.

Otherwise, in this scenario:

    1. Have Type Profiler and Code Coverage Profilers enabled
    2. Switch to Timeline tab
    3. Reload => Starts autorecord
      -> Backend starts preferred instruments but doesn't disable profilers
      -> Frontend receives auto record started, starts capturing
      -> Frontend tells backend to disable profilers
      -> Backend eventually disables profilers after a lot of code has run

So the timeline recording for reload caused a lot of code to be executed with the profilers enabled.

Same thing for console.profile/profileEnd. This is equivalent to the user clicking the Timeline start/stop buttons but can happen whenever.

See backend methods:

    InspectorTimelineAgent::mainFrameStartedLoading
    InspectorTimelineAgent::startProgrammaticCapture

We automatically disable breakpoints when starting programmatic captures. We should likewise automatically disable these profilers.

> Source/WebInspectorUI/UserInterface/Base/Main.js:202
> +        RuntimeAgent.disableTypeProfiler();
> +        RuntimeAgent.disableControlFlowProfiler();

This should be done on all targets, not just the main target.

    for (let target of WebInspector.targets) {
        target.RuntimeAgent.disableTypeProfiler();
        target.RuntimeAgent.disableControlFlowProfiler();
    }

However is this needed? Will toggling the Setting (this.showJavaScriptTypeInformationSetting.value) trigger this inside SourceCodeTextEditors? If it doesn't, it seems like the SourceCodeTextEditors would continue to have their Annotators running despite the backend being disabled. What, if anything, disables the Annotators? For example, you could be looking at a SourceCodeTextEditor and a TimelineRecording starts via console.profile.

> Source/WebInspectorUI/UserInterface/Images/NavigationItemCodeCoverageTemporaryDisabled.svg:5
> +    <path fill="none" stroke="currentColor" stroke-width="1" d="M9 5.5s0-1.5-2.5-1.5C3 4 3 10 6.5 10c2.5 0 2.5-1.5 2.5-1.5"/>

Style: We normally canonicalize paths. Capitalized characters and space around characters. I believe these dashes can just be spaces.

> Source/WebInspectorUI/UserInterface/Views/TextResourceContentView.js:245
> +            this._codeCoverageButtonNavigationItem.image = "Images/NavigationItemCodeCoverageTemporaryDisabled.svg";

What do we do for GTK which don't have this image? Can we solve this by setting an additional CSS class and having CSS to switch to the better image?

[Nikita Vasilyev]

(In reply to comment #6)
> Comment on attachment 296172 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=296172&action=review
> 
> > Source/WebInspectorUI/UserInterface/Base/Main.js:178
> > +    this.disableTypeProfilerUntilReloadSetting = new WebInspector.Setting("disable-type-profiler-until-reload", false);
> > +    this.disableCodeCoverageUntilReloadSetting = new WebInspector.Setting("disable-code-coverage-until-reload", false);
> 
> Why are these a setting and not just a global boolean?

Global booleans get lost when Web Inspector closes.

A use case:
1. Make a timeline recording.
2. Close Web Inspector.
3. Open Web Inspector.

When WebInspector.Setting is used, which is backed by localStorage, it can preserve the state.

(Most of the other comments were discussed in person.)

[Joseph Pecoraro]

(In reply to comment #7)
> (In reply to comment #6)
> > Comment on attachment 296172 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=296172&action=review
> > 
> > > Source/WebInspectorUI/UserInterface/Base/Main.js:178
> > > +    this.disableTypeProfilerUntilReloadSetting = new WebInspector.Setting("disable-type-profiler-until-reload", false);
> > > +    this.disableCodeCoverageUntilReloadSetting = new WebInspector.Setting("disable-code-coverage-until-reload", false);
> > 
> > Why are these a setting and not just a global boolean?
> 
> Global booleans get lost when Web Inspector closes.
> 
> A use case:
> 1. Make a timeline recording.
> 2. Close Web Inspector.
> 3. Open Web Inspector.
> 
> When WebInspector.Setting is used, which is backed by localStorage, it can
> preserve the state.
> 
> (Most of the other comments were discussed in person.)

But how is this different from:

1. User loads page
2. User opens Web Inspector

In which we aren't showing dashed line buttons but is basically equivalent.

[Nikita Vasilyev]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > Comment on attachment 296172 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=296172&action=review
> > > 
> > > > Source/WebInspectorUI/UserInterface/Base/Main.js:178
> > > > +    this.disableTypeProfilerUntilReloadSetting = new WebInspector.Setting("disable-type-profiler-until-reload", false);
> > > > +    this.disableCodeCoverageUntilReloadSetting = new WebInspector.Setting("disable-code-coverage-until-reload", false);
> > > 
> > > Why are these a setting and not just a global boolean?
> > 
> > Global booleans get lost when Web Inspector closes.
> > 
> > A use case:
> > 1. Make a timeline recording.
> > 2. Close Web Inspector.
> > 3. Open Web Inspector.
> > 
> > When WebInspector.Setting is used, which is backed by localStorage, it can
> > preserve the state.
> > 
> > (Most of the other comments were discussed in person.)
> 
> But how is this different from:
> 
> 1. User loads page
> 2. User opens Web Inspector
> 
> In which we aren't showing dashed line buttons but is basically equivalent.

Consider the following case:

1. Enable Type Profiler
2. Make a timeline recording.
3. Close Web Inspector.
4. Open Web Inspector.
5. Reload the page.

Expected: Type Profiler enabled.

Also, I use settings for addEventListener, which is handy.

[Joseph Pecoraro]

Comment on attachment 296172 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/ScriptContentView.js:67
> +        WebInspector.disableTypeProfilerUntilReloadSetting.addEventListener(WebInspector.Setting.Event.Changed, this._disableTypeProfilerUntilReloadSettingChanged, this);

These buttons should be respecting the initial state of the setting and not just updating buttons on changes.

[Joseph Pecoraro]

> > But how is this different from:
> > 
> > 1. User loads page
> > 2. User opens Web Inspector
> > 
> > In which we aren't showing dashed line buttons but is basically equivalent.
> 
> Consider the following case:
> 
> 1. Enable Type Profiler
> 2. Make a timeline recording.
> 3. Close Web Inspector.
> 4. Open Web Inspector.
> 5. Reload the page.
> 
> Expected: Type Profiler enabled.

If that is the case then we should be respecting the initial value of the setting, which doesn't appear to be the case.

But, again applying the exact same logic as before:

1. Enable Type Profiler
2. Make a timeline recording
3. Close Web Inspector
4. Reload / Navigate (same as my #1 above)
5. Open Web Inspector (same as my #2 above)

The closed frontend cannot update its setting for the navigation in (4), so its state will just be wrong when we open the inspector in (5). Therefore, I think we should create inspector close/open the same as if we navigated.

[Joseph Pecoraro]

> should create inspector close/open

Correction: ... should *treat* inspector close/open ...

[Nikita Vasilyev]

(In reply to comment #6)
> Comment on attachment 296172 [details]
> > Source/WebInspectorUI/UserInterface/Images/NavigationItemCodeCoverageTemporaryDisabled.svg:5
> > +    <path fill="none" stroke="currentColor" stroke-width="1" d="M9 5.5s0-1.5-2.5-1.5C3 4 3 10 6.5 10c2.5 0 2.5-1.5 2.5-1.5"/>
> 
> Style: We normally canonicalize paths. Capitalized characters and space
> around characters. I believe these dashes can just be spaces.

This is exactly the same path as in the existing NavigationItemCodeCoverage.svg.

SVG path commands are case-sensitive; an upper-case command specifies its arguments as absolute positions, while a lower-case command specified points relative to the current position.

Dashes can't just be spaces; they're minus signs. However, "s0-1.5-2.5" can be written as "s0 -1.5 -2.5".

[Joseph Pecoraro]

> However, "s0-1.5-2.5" can be written as "s0 -1.5 -2.5".

I would think: "S 0 -1.5 -2.5"

[Matt Baker]

(In reply to comment #14)
> > However, "s0-1.5-2.5" can be written as "s0 -1.5 -2.5".
> 
> I would think: "S 0 -1.5 -2.5"

Uppercase letters indicate that absolute coordinates follow the command, instead of relative.

[Nikita Vasilyev]

Created attachment 296743 [details]
Patch

[Nikita Vasilyev]

I'd prefer to convert all SVGs to a single code style in a separate patch. The newly added *TemporaryDisabled.svg icons are copy/paste of the existing ones with the exception of added stroke-dasharray="1 1" attribute.

[Blaze Burg]

Is this patch still up for review? Please clear flag if not.

[Nikita Vasilyev]

(In reply to comment #18)
> Is this patch still up for review? Please clear flag if not.

Yes, it is. Joe is reviewing it.

[Joseph Pecoraro]

Comment on attachment 296743 [details]
Patch

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

r- because there are no tests. We should always have some kind of test for new protocol methods.

> Source/JavaScriptCore/inspector/protocol/Timeline.json:77
> +            "description": "Toggle Type Profiler on navigation. If <code>true</code> the backend will start capturing Type Profiler on navigation.",

Grammar: "start capturing Type Profiler"

> Source/JavaScriptCore/inspector/protocol/Timeline.json:84
> +            "description": "Toggle Control Flow Profiler on navigation. If <code>true</code> the backend will start capturing Control Flow Profiler on navigation.",

Grammar: "start capturing Control Flow Profiler"

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:143
> +void InspectorTimelineAgent::setTypeProfilerEnabledOnPageLoad(ErrorString&, bool enabled)
> +{
> +    m_autoEnableTypeProfiler = enabled;
> +}
> +
> +void InspectorTimelineAgent::setControlFlowProfilerEnabledOnPageLoad(ErrorString&, bool enabled)
> +{
> +    m_autoEnableControlFlowProfiler = enabled;
> +}

These states should be cleared in `InspectorTimelineAgent::willDestroyFrontendAndBackend` in case the inspector closes. In such cases we should reset back to default state.

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:459
> +        }

This should be disabling the type profilers in case they were enabled.

> Source/WebInspectorUI/UserInterface/Base/Main.js:2102
> +WebInspector._timelineCapturingStarted = function(startTime) {

Style: brace on newline

> Source/WebInspectorUI/UserInterface/Base/Main.js:2120
> +        // Type Profiler is already enabled, no need to re-enable it on navigation.

Instead of "is already enabled" lets say "is now enabled".

> Source/WebInspectorUI/UserInterface/Base/Main.js:2134
> +        // Code Coverage is already enabled, no need to re-enable it on navigation.

Ditto.

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:169
> +    get enableCodeCoverageOnNavigation() { return this._enableCodeCoverageOnNavigation }
> +    get enableTypeProfilerOnNavigation() { return this._enableTypeProfilerOnNavigation }

Style: Because these have setters, let not one-line them and put them by the setters.

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:1120
> +    EnableTypeProfilerOnNavigationChanged: "enable-type-profiler-on-navigation-changed"

Nit: Lets add the trailing comma for future diff goodness.

> Source/WebInspectorUI/UserInterface/Images/gtk/NavigationItemTypesTemporaryDisabled.svg:1
> +<?xml version="1.0" encoding="UTF-8"?>

Is there an Images/gtk/NavigationItemCodeCoverageTemporaryDisabled to match this?

> Source/WebInspectorUI/UserInterface/Views/ScriptContentView.js:235
> +            WebInspector.timelineManager.addEventListener(WebInspector.TimelineManager.Event.EnableTypeProfilerOnNavigationChanged, this._renderTypesButton, this);

We need to remove this event handler on the persistent TimelineManager in close() otherwise we leak this ContentView forever on navigations.

> Source/WebInspectorUI/Versions/Inspector-iOS-10.0.json:3607
> +            "name": "setTypeProfilerEnabledOnPageLoad",

We should NOT be adding this to an older build of iOS which did not support the commands.

Just adding these to the JavaScriptCore/inspector/Timeline.json is enough.