DevTools: move recurring flag into AsyncTask, control cancelation from embedder only.

DevTools: move recurring flag into AsyncTask, control cancelation from embedder only.

BUG=701177
Review-Url: https://codereview.chromium.org/2746333002
Cr-Commit-Position: refs/heads/master@{#456636}
Committed: https://chromium.googlesource.com/chromium/src/+/51fad280650c0155706ba734848b1e50864893ed

[pfeldman, 2017-03-14 00:00:24]

pfeldman@chromium.org changed reviewers:
+ alph@chromium.org, dgozman@chromium.org

[alph, 2017-03-14 00:39:38]

https://codereview.chromium.org/2746333002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/InspectorInstrumentation.cpp
(right):

https://codereview.chromium.org/2746333002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorInstrumentation.cpp:77:
m_recurring(!!step) {
drop !!

https://codereview.chromium.org/2746333002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorInstrumentation.cpp:78:
TRACE_EVENT_FLOW_STEP0("devtools.timeline.async", "AsyncTask", task,
Should it be FLOW_END for non-recurring tasks?

https://codereview.chromium.org/2746333002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorInstrumentation.cpp:79: step ?
step : "");
Where the step name is going to be used?

https://codereview.chromium.org/2746333002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorInstrumentation.cpp:81:
m_debugger->asyncTaskStarted(m_task);
should it fire it from setEnabled instead?

https://codereview.chromium.org/2746333002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/perf_ui/FlameChart.js (right):

https://codereview.chromium.org/2746333002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/perf_ui/FlameChart.js:958:
context.fillStyle = '#7f5050';
Red color is reserved for something being wrong.

https://codereview.chromium.org/2746333002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/perf_ui/FlameChart.js:974: var
distanceTime = td.flowEndTimes[i] - td.flowStartTimes[i];
This file changes seem to be unrelated to the main change.

https://codereview.chromium.org/2746333002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/perf_ui/FlameChart.js:979: var p =
[];
creating arrays of objects is not a good idea for the hot code.

https://codereview.chromium.org/2746333002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChart.js
(right):

https://codereview.chromium.org/2746333002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChart.js:748:
if (event.args.name === 'Image')
What's wrong with Image?

[pfeldman, 2017-03-14 01:15:11]

https://codereview.chromium.org/2746333002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/InspectorInstrumentation.cpp
(right):

https://codereview.chromium.org/2746333002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorInstrumentation.cpp:77:
m_recurring(!!step) {
On 2017/03/14 00:39:37, alph wrote:
> drop !!

Done.

https://codereview.chromium.org/2746333002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorInstrumentation.cpp:78:
TRACE_EVENT_FLOW_STEP0("devtools.timeline.async", "AsyncTask", task,
On 2017/03/14 00:39:37, alph wrote:
> Should it be FLOW_END for non-recurring tasks?

Nope, that is fine as it is.

https://codereview.chromium.org/2746333002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorInstrumentation.cpp:79: step ?
step : "");
On 2017/03/14 00:39:37, alph wrote:
> Where the step name is going to be used?

I can surface it in the timeline UI, but it does not compile without it.

https://codereview.chromium.org/2746333002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorInstrumentation.cpp:81:
m_debugger->asyncTaskStarted(m_task);
On 2017/03/14 00:39:37, alph wrote:
> should it fire it from setEnabled instead?

setEnabled is optional, so no. it is just muting the raii.

https://codereview.chromium.org/2746333002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/perf_ui/FlameChart.js (right):

https://codereview.chromium.org/2746333002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/perf_ui/FlameChart.js:958:
context.fillStyle = '#7f5050';
On 2017/03/14 00:39:37, alph wrote:
> Red color is reserved for something being wrong.

Yeah, that's why I made it brown.

https://codereview.chromium.org/2746333002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/perf_ui/FlameChart.js:974: var
distanceTime = td.flowEndTimes[i] - td.flowStartTimes[i];
On 2017/03/14 00:39:37, alph wrote:
> This file changes seem to be unrelated to the main change.

I know, I am sorry.

https://codereview.chromium.org/2746333002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/perf_ui/FlameChart.js:979: var p =
[];
On 2017/03/14 00:39:37, alph wrote:
> creating arrays of objects is not a good idea for the hot code.

Do you think 2 bezierCurveTo-s is not making this overhead negligible?

https://codereview.chromium.org/2746333002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChart.js
(right):

https://codereview.chromium.org/2746333002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChart.js:748:
if (event.args.name === 'Image')
On 2017/03/14 00:39:37, alph wrote:
> What's wrong with Image?

I'll drop it.

[caseq, 2017-03-14 01:15:42]

caseq@chromium.org changed reviewers:
+ caseq@chromium.org

[caseq, 2017-03-14 01:15:43]

https://codereview.chromium.org/2746333002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChart.js
(right):

https://codereview.chromium.org/2746333002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChart.js:748:
if (event.args.name === 'Image')
This is supposed to work for the generic case of flow events, having some logic
depending on what data the event carries, even without the regard to the name of
the event is not lgtm.

[alph, 2017-03-14 01:40:42]

lgtm

https://codereview.chromium.org/2746333002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorInstrumentation.cpp
(right):

https://codereview.chromium.org/2746333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorInstrumentation.cpp:80: if
(m_debugger)
Isn't is always true?

https://codereview.chromium.org/2746333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorInstrumentation.cpp:81:
m_debugger->asyncTaskStarted(m_task);
On 2017/03/14 01:15:10, pfeldman wrote:
> On 2017/03/14 00:39:37, alph wrote:
> > should it fire it from setEnabled instead?
> 
> setEnabled is optional, so no. it is just muting the raii.

So you called asyncTaskStarted, but will never call asyncTaskFinished. Is it
fine?

[pfeldman, 2017-03-14 01:42:35]

> > setEnabled is optional, so no. it is just muting the raii.
> 
> So you called asyncTaskStarted, but will never call asyncTaskFinished. Is it
> fine?

Ah, I'm being stupid and you are correct. I'll fix it right away.

[caseq, 2017-03-14 02:09:44]

https://codereview.chromium.org/2746333002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorInstrumentation.cpp
(right):

https://codereview.chromium.org/2746333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorInstrumentation.cpp:78:
TRACE_EVENT_FLOW_STEP0("devtools.timeline.async", "AsyncTask", task,
I think we eventually need to emit END event.

https://codereview.chromium.org/2746333002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/perf_ui/FlameChart.js (right):

https://codereview.chromium.org/2746333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/perf_ui/FlameChart.js:979: var p =
[];
nit: this dance seems a bit unnecessary :-)

https://codereview.chromium.org/2746333002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChart.js
(right):

https://codereview.chromium.org/2746333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChart.js:759:
if (flowIndex < 0)
How can this happen?

[pfeldman, 2017-03-14 02:29:25]

https://codereview.chromium.org/2746333002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorInstrumentation.cpp
(right):

https://codereview.chromium.org/2746333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorInstrumentation.cpp:78:
TRACE_EVENT_FLOW_STEP0("devtools.timeline.async", "AsyncTask", task,
On 2017/03/14 02:09:43, caseq wrote:
> I think we eventually need to emit END event.

not really, i don't think we do. if we have to, we can do it from canceled.

https://codereview.chromium.org/2746333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorInstrumentation.cpp:80: if
(m_debugger)
that's now fixed

https://codereview.chromium.org/2746333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorInstrumentation.cpp:81:
m_debugger->asyncTaskStarted(m_task);
On 2017/03/14 01:40:42, alph wrote:
> On 2017/03/14 01:15:10, pfeldman wrote:
> > On 2017/03/14 00:39:37, alph wrote:
> > > should it fire it from setEnabled instead?
> > 
> > setEnabled is optional, so no. it is just muting the raii.
> 
> So you called asyncTaskStarted, but will never call asyncTaskFinished. Is it
> fine?

Done.

https://codereview.chromium.org/2746333002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/perf_ui/FlameChart.js (right):

https://codereview.chromium.org/2746333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/perf_ui/FlameChart.js:979: var p =
[];
On 2017/03/14 02:09:43, caseq wrote:
> nit: this dance seems a bit unnecessary :-)

It does, but the code without it is not readable.

https://codereview.chromium.org/2746333002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChart.js
(right):

https://codereview.chromium.org/2746333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChart.js:759:
if (flowIndex < 0)
On 2017/03/14 02:09:43, caseq wrote:
> How can this happen?

That's a way to filter out things. Filtering in the index-based array is tricky.

[caseq, 2017-03-14 02:44:47]

https://codereview.chromium.org/2746333002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChart.js
(right):

https://codereview.chromium.org/2746333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChart.js:770:
pushEndFlow(event, this._flowEventIndexById.get(event.id));
btw, I think you shouldn't use  raw event id for things like this -- this would
only make sense along with process id the way you use it on the back-end side. A
good way to avoid problems with id clashes would be perhaps to handle linking
flow events on the tracing model side.

[pfeldman, 2017-03-14 04:05:16]

The CQ bit was checked by pfeldman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 04:05:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[caseq, 2017-03-14 05:10:17]

lgtm

[commit-bot: I haz the power, 2017-03-14 06:24:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-14 06:24:11]

Dry run: This issue passed the CQ dry run.

[pfeldman, 2017-03-14 06:31:21]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2017-03-14 06:31:22]

The patchset sent to the CQ was uploaded after l-g-t-m from alph@chromium.org
Link to the patchset: https://codereview.chromium.org/2746333002/#ps40001
(title: "using _END now")

[commit-bot: I haz the power, 2017-03-14 06:32:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 06:36:20]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489473081402660,
"parent_rev": "05c7a3f7523ff797cf6537109549d4bc513ece64", "commit_rev":
"51fad280650c0155706ba734848b1e50864893ed"}

[commit-bot: I haz the power, 2017-03-14 06:37:16]

Description was changed from

==========
DevTools: move recurring flag into AsyncTask, control cancelation from embedder
only.

BUG=701177
==========

to

==========
DevTools: move recurring flag into AsyncTask, control cancelation from embedder
only.

BUG=701177

Review-Url: https://codereview.chromium.org/2746333002
Cr-Commit-Position: refs/heads/master@{#456636}
Committed:
https://chromium.googlesource.com/chromium/src/+/51fad280650c0155706ba734848b...
==========

[commit-bot: I haz the power, 2017-03-14 06:37:18]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/51fad280650c0155706ba734848b...