Bug 163407 – Web Inspector: Call RuntimeAgent.disableControlFlowProfiler when Code Coverage Profiler is turned off

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Disabling Code Coverage profiler by clicking [C] button should signal the backend to stop collecting the data by calling RuntimeAgent.disableControlFlowProfiler method.

RuntimeAgent.disableControlFlowProfiler is currently unused.

[Radar WebKit Bug Importer]

<rdar://problem/28764230>

[Nikita Vasilyev]

Created attachment 294390 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 294390 [details]
Patch

It is unfortunate that we don't have any tests for these methods.

Given the Type Profiler chance is probably going to be identical, is there any reason this is split into two bugs?

[Nikita Vasilyev]

> Given the Type Profiler chance is probably going to be identical, is there
> any reason this is split into two bugs?

Yes, I have concerns about Type Profiler https://bugs.webkit.org/show_bug.cgi?id=163408#c2

[Nikita Vasilyev]

(In reply to comment #3)
> Comment on attachment 294390 [details]
> Patch
> 
> It is unfortunate that we don't have any tests for these methods.

Should we test if RuntimeAgent.enableControlFlowProfiler() and RuntimeAgent.disableControlFlowProfiler() gets called on clicking the [C] icon?

I haven't written any UI tests that use view wrappers, but I expect it to be feasible.

[Joseph Pecoraro]

(In reply to comment #5)
> (In reply to comment #3)
> > Comment on attachment 294390 [details]
> > Patch
> > 
> > It is unfortunate that we don't have any tests for these methods.
> 
> Should we test if RuntimeAgent.enableControlFlowProfiler() and
> RuntimeAgent.disableControlFlowProfiler() gets called on clicking the [C]
> icon?

We have no UI tests right now.

I mean we don't even have tests for these protocol methods to ensure the backend does what we expect.

[WebKit Commit Bot]

Comment on attachment 294390 [details]
Patch

Clearing flags on attachment: 294390

Committed r208561: <http://trac.webkit.org/changeset/208561>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

(In reply to comment #0)
> Disabling Code Coverage profiler by clicking [C] button should signal the
> backend to stop collecting the data by calling
> RuntimeAgent.disableControlFlowProfiler method.
> 
> RuntimeAgent.disableControlFlowProfiler is currently unused.

The downside of doing this is that if you ever want to toggle the button, you will loose information. What I did with the type profiler is ensure that the button was just a local toggle for the UI, not the actual JSC type profiler. That way, if a user wants to show and then hide and then show the UI, the user wouldn't loose type information.

What's the motivation for not following this design here?

(Note that I still think we should turn the profilers off when recording a timeline)

[Nikita Vasilyev]

(In reply to comment #9)
> (In reply to comment #0)
> > Disabling Code Coverage profiler by clicking [C] button should signal the
> > backend to stop collecting the data by calling
> > RuntimeAgent.disableControlFlowProfiler method.
> > 
> > RuntimeAgent.disableControlFlowProfiler is currently unused.
> 
> The downside of doing this is that if you ever want to toggle the button,
> you will loose information. What I did with the type profiler is ensure that
> the button was just a local toggle for the UI, not the actual JSC type
> profiler. That way, if a user wants to show and then hide and then show the
> UI, the user wouldn't loose type information.
> 
> What's the motivation for not following this design here?
> 
> (Note that I still think we should turn the profilers off when recording a
> timeline)

Here's a use case:

1. Turn Code Coverage off and on.
2. Perform some action on a web page.
3. See what got executed.

Previously, we wouldn't be able to tell if the code was executed before performing that action or not. Now, turning off and on allows us to start with a clean slate.