Web Inspector: Add a button to navigation bar to toggle Control Flow Profiler

[Nikita Vasilyev]

The backend of Control Flow Profiler was decoupled from Type Profiler (bug 160750).

Add a button to toggle Control Flow Profiler independently from Type Profiler.
The existing Type Profiler button shouldn't affect Control Flow Profiler any longer.

[Radar WebKit Bug Importer]

<rdar://problem/27912606>

[Nikita Vasilyev]

Created attachment 286397 [details]
WIP

No new icon yet. The same [T] icon is temporary used for Control Flow Profiler.

[Joseph Pecoraro]

Comment on attachment 286397 [details]
WIP

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

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:294
> +    toggleDeadCodeHighlights()

This name doesn't sound right. This is showing code coverage, code that has executed or not executed. "Dead Code" is a term of art for unreachable code, and that isn't what we are doing here, right?

[Nikita Vasilyev]

(In reply to comment #3)
> Comment on attachment 286397 [details]
> WIP
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=286397&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:294
> > +    toggleDeadCodeHighlights()
> 
> This name doesn't sound right. This is showing code coverage, code that has
> executed or not executed. "Dead Code" is a term of art for unreachable code,
> and that isn't what we are doing here, right?

You're right. I'll change it.

[Nikita Vasilyev]

Created attachment 286472 [details]
Patch

In the code we use terms:
* Control Flow Profiler
* Code Coverage Profiler
* Basic Block Annotator
* Unexecuted Code Highlights

I'm open to unify some of them.

[BJ Burg]

Comment on attachment 286472 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:35
> +        Make Code Coverage profiler indepedent from Type Profiler.

Nit: independent

> Source/WebInspectorUI/UserInterface/Views/ScriptContentView.js:68
> +        var toolTipControlFlow = WebInspector.UIString("Gray out unexecuted code");

Nit: use 'let'

I don't like this wording. How about "Fade unexecuted code"?

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:178
> +        return !!this._basicBlockAnnotator;

This is super indirect as this is set far away in _makeBasicBlockAnnotator where it's guarded by if (RuntimeAgent.getBasicBlocks) {
If we are keying off this RuntimeAgent method, it would be better to do that here and rewrite the other guard to call this method instead.

Second, this doesn't seem to take into account the value of this.enableControlFlowProfilerSetting. Is this intentional? Some text to explain the different configurations that are now possible would make this a lot easier to review. It is aggravating that this patch introduces more uses of the term "control flow profiler" which so far has been limited to the RuntimeAgent method. I think we should stick to BasicBlockAnnotator-like method and setting names.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:356
> +                    this._setBasicBlockAnnotatorEnabledState(false);

It would be nice to fold the existence checks into _setBasicBlockAnnotatorEnabledState as early exits. It would make this code easier to follow.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1737
>              console.assert(this.visible, "Annotators should not be enabled if the TextEditor is not visible");

This patch doesn't apply anymore so I can't see what this is doing. Can you rebase?

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1761
> +    _setBasicBlockAnnotatorEnabledState(shouldActivate)

This should be a setter. Or I can't figure out why it shouldn't be.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1877
> +    _makeControlFlowScrollEventHandler()

We use 'create' as the prefix for factory methods.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1879
> +        var timeoutIdentifier = null;

Nit: let

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1881
> +        {

Don't put this on its own line.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1885
> +                if (this._basicBlockAnnotator)

Please make an else if {

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1889
> +            timeoutIdentifier = setTimeout(function() {

Use an arrow function instead of .bind(this).

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1896
> +        return scrollHandler.bind(this);

The outer function that's returned should be an arrow function too.

> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:162
> +    canShowBasicBlockAnnotator()

This name doesn't make sense. The user doesn't see a BasicBlockAnnotator. They see code coverage/altered styles. I would name it canShowCoverageHints or something.

[Nikita Vasilyev]

Comment on attachment 286472 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1761
>> +    _setBasicBlockAnnotatorEnabledState(shouldActivate)
> 
> This should be a setter. Or I can't figure out why it shouldn't be.

This was mirrored from _setTypeTokenAnnotatorEnabledState.
Should it be "_basicBlockAnnotatorEnabled" and "_typeTokenAnnotatorEnabled"?

>> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1877
>> +    _makeControlFlowScrollEventHandler()
> 
> We use 'create' as the prefix for factory methods.

This was mirrored from _makeTypeTokenAnnotator. Should I use "create" for both?

[Nikita Vasilyev]

Created attachment 287235 [details]
Patch

[Nikita Vasilyev]

Created attachment 287236 [details]
Patch

[BJ Burg]

Comment on attachment 287236 [details]
Patch

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

The patch looks fine overall, but I am worried about the race condition when clicking the button quickly. Can you post a new version with that fixed?

> Source/WebInspectorUI/UserInterface/Base/Main.js:167
> +    this.enableControlFlowProfilerSetting = new WebInspector.Setting("enable-control-flow-profiler", false);

I think this should be on by default. Isn't the performance/deopt impact negligible? We turn it off anyway while profiling.

> Source/WebInspectorUI/UserInterface/Views/ScriptContentView.js:69
> +        let activatedToolTipCodeCoverage = WebInspector.UIString("Donât fade unexecuted code");

Please do not use contractions in UIStrings.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:302
> +            this.updateFormattedState(true).then(() => {

Is it possible for this promise to race with the user clicking on the button again really quickly? What would happen? Maybe toggleUnexecutedCodeHighlights should itself return a promise in all paths, and the button can disable itself until the state update is committed (i.e., the promise resolves).

> Source/WebInspectorUI/UserInterface/Views/TextContentView.js:56
> +        let activatedToolTipCodeCoverage = WebInspector.UIString("Donât fade unexecuted code");

Same comment as above.

[Nikita Vasilyev]

Comment on attachment 287236 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:142
> +        return this._codeMirror.historySize().undo > 0;

Pretty printing code creates a CodeMirror history item. This getter always returns `true` for pretty-printed code.

[Nikita Vasilyev]

(In reply to comment #10)
> Comment on attachment 287236 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=287236&action=review
> 
> The patch looks fine overall, but I am worried about the race condition when
> clicking the button quickly. Can you post a new version with that fixed?
> 
> > Source/WebInspectorUI/UserInterface/Base/Main.js:167
> > +    this.enableControlFlowProfilerSetting = new WebInspector.Setting("enable-control-flow-profiler", false);
> 
> I think this should be on by default. Isn't the performance/deopt impact
> negligible? We turn it off anyway while profiling.

Saam and I need to measure performance impact first. I think it's better to keep it disabled by default for now.

> 
> > Source/WebInspectorUI/UserInterface/Views/ScriptContentView.js:69
> > +        let activatedToolTipCodeCoverage = WebInspector.UIString("Donât fade unexecuted code");
> 
> Please do not use contractions in UIStrings.
> 
> > Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:302
> > +            this.updateFormattedState(true).then(() => {
> 
> Is it possible for this promise to race with the user clicking on the button
> again really quickly? What would happen? Maybe
> toggleUnexecutedCodeHighlights should itself return a promise in all paths,
> and the button can disable itself until the state update is committed (i.e.,
> the promise resolves).

It is practically impossible to have a race condition here. A minified resource always gets
pretty printed on first open. `this.updateFormattedState`, although asynchronous, is very fast
on consequent calls.

I can disable the button to be make the code less race condition prone, if you want.

[Nikita Vasilyev]

Created attachment 288340 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 288340 [details]
Patch

Rejecting attachment 288340 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 288340, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
file without the binary data in line: "Binary files a/Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js and b/Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js differ".  Be sure to use the --binary flag when invoking "git diff" with diffs containing binary files. at /Volumes/Data/EWS/WebKit/Tools/Scripts/VCSUtils.pm line 820, <ARGV> line 80.

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 25 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/2037042

[Nikita Vasilyev]

Created attachment 288342 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 288342 [details]
Patch

Clearing flags on attachment: 288342

Committed r205674: <http://trac.webkit.org/changeset/205674>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.