DevTools: implement console.timeline/timelineEnd.

DevTools: implement console.timeline/timelineEnd.

BUG=265360
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157827

[pfeldman, 2013-09-06 07:08:29]

Please take a look while I'm working on the tests.

[yurys, 2013-09-06 07:56:45]

https://codereview.chromium.org/24027002/diff/1/Source/core/inspector/Inspect...
File Source/core/inspector/InspectorConsoleAgent.h (left):

https://codereview.chromium.org/24027002/diff/1/Source/core/inspector/Inspect...
Source/core/inspector/InspectorConsoleAgent.h:83: void
startConsoleTiming(ScriptExecutionContext*, const String& title);
Can you land this part separately?

https://codereview.chromium.org/24027002/diff/1/Source/core/inspector/Inspect...
File Source/core/inspector/InspectorProfilerAgent.cpp (left):

https://codereview.chromium.org/24027002/diff/1/Source/core/inspector/Inspect...
Source/core/inspector/InspectorProfilerAgent.cpp:230:
addStartProfilingMessageToConsole(title, 0, String());
After removing this line you won't get console message if profiling was
initiated from the front-end. Is that intentional?

https://codereview.chromium.org/24027002/diff/1/Source/core/inspector/Inspect...
File Source/core/inspector/InspectorTimelineAgent.cpp (right):

https://codereview.chromium.org/24027002/diff/1/Source/core/inspector/Inspect...
Source/core/inspector/InspectorTimelineAgent.cpp:597:
m_consoleTimelines.append(title);
What is the expected behavior when same timeline is started twice?

[caseq, 2013-09-06 09:25:45]

https://codereview.chromium.org/24027002/diff/1/Source/core/inspector/Inspect...
File Source/core/inspector/InspectorConsoleAgent.h (right):

https://codereview.chromium.org/24027002/diff/1/Source/core/inspector/Inspect...
Source/core/inspector/InspectorConsoleAgent.h:85: void
consoleTimeEnd(ScriptExecutionContext*, const String& title, ScriptState*);
Do we need both ScriptState and ScriptExecutionContext?

https://codereview.chromium.org/24027002/diff/1/Source/core/inspector/Inspect...
File Source/core/inspector/InspectorController.cpp (right):

https://codereview.chromium.org/24027002/diff/1/Source/core/inspector/Inspect...
Source/core/inspector/InspectorController.cpp:120: OwnPtr<InspectorConsoleAgent>
consoleAgentPtr(PageConsoleAgent::create(m_instrumentingAgents.get(),
m_timelineAgent, m_state.get(), m_injectedScriptManager.get(), domAgent));
It would be nice to have agents grouped together in such calls.

https://codereview.chromium.org/24027002/diff/1/Source/devtools/front_end/Tim...
File Source/devtools/front_end/TimelineManager.js (right):

https://codereview.chromium.org/24027002/diff/1/Source/devtools/front_end/Tim...
Source/devtools/front_end/TimelineManager.js:108:
this._manager._enablementCount++;
So what happens if pages does console.timeline() and then navigates away?

[pfeldman, 2013-09-06 16:10:30]

https://codereview.chromium.org/24027002/diff/1/Source/core/inspector/Inspect...
File Source/core/inspector/InspectorConsoleAgent.h (left):

https://codereview.chromium.org/24027002/diff/1/Source/core/inspector/Inspect...
Source/core/inspector/InspectorConsoleAgent.h:83: void
startConsoleTiming(ScriptExecutionContext*, const String& title);
On 2013/09/06 07:56:45, Yury Semikhatsky wrote:
> Can you land this part separately?

This is a simple rename in attempt to streamline consoleTime and consoleTimeline
naming. Otherwise, start/stopConsoleTiming methods are confusing.

https://codereview.chromium.org/24027002/diff/1/Source/core/inspector/Inspect...
File Source/core/inspector/InspectorConsoleAgent.h (right):

https://codereview.chromium.org/24027002/diff/1/Source/core/inspector/Inspect...
Source/core/inspector/InspectorConsoleAgent.h:85: void
consoleTimeEnd(ScriptExecutionContext*, const String& title, ScriptState*);
Yes, those are different.

https://codereview.chromium.org/24027002/diff/1/Source/core/inspector/Inspect...
File Source/core/inspector/InspectorController.cpp (right):

https://codereview.chromium.org/24027002/diff/1/Source/core/inspector/Inspect...
Source/core/inspector/InspectorController.cpp:120: OwnPtr<InspectorConsoleAgent>
consoleAgentPtr(PageConsoleAgent::create(m_instrumentingAgents.get(),
m_timelineAgent, m_state.get(), m_injectedScriptManager.get(), domAgent));
On 2013/09/06 09:25:46, caseq wrote:
> It would be nice to have agents grouped together in such calls.

I am not sure I follow.

https://codereview.chromium.org/24027002/diff/1/Source/core/inspector/Inspect...
File Source/core/inspector/InspectorProfilerAgent.cpp (left):

https://codereview.chromium.org/24027002/diff/1/Source/core/inspector/Inspect...
Source/core/inspector/InspectorProfilerAgent.cpp:230:
addStartProfilingMessageToConsole(title, 0, String());
On 2013/09/06 07:56:45, Yury Semikhatsky wrote:
> After removing this line you won't get console message if profiling was
> initiated from the front-end. Is that intentional?

Correct, this method was standing in my way and I nuked it.

https://codereview.chromium.org/24027002/diff/1/Source/core/inspector/Inspect...
File Source/core/inspector/InspectorTimelineAgent.cpp (right):

https://codereview.chromium.org/24027002/diff/1/Source/core/inspector/Inspect...
Source/core/inspector/InspectorTimelineAgent.cpp:597:
m_consoleTimelines.append(title);
console.timeline("one")
console.timeline("one")
console.timelineEnd("one")
console.timelineEnd("one")

is a valid supported case.

https://codereview.chromium.org/24027002/diff/1/Source/devtools/front_end/Tim...
File Source/devtools/front_end/TimelineManager.js (right):

https://codereview.chromium.org/24027002/diff/1/Source/devtools/front_end/Tim...
Source/devtools/front_end/TimelineManager.js:108:
this._manager._enablementCount++;
On 2013/09/06 09:25:46, caseq wrote:
> So what happens if pages does console.timeline() and then navigates away?

Good catch. InspectorTimelineAgent::restore is now telling front-end that
console-initiated timeline has stopped.

[caseq, 2013-09-09 08:37:45]

On 2013/09/06 16:10:30, pfeldman wrote:
> 
>
https://codereview.chromium.org/24027002/diff/1/Source/core/inspector/Inspect...
> File Source/core/inspector/InspectorConsoleAgent.h (right):
> 
>
https://codereview.chromium.org/24027002/diff/1/Source/core/inspector/Inspect...
> Source/core/inspector/InspectorConsoleAgent.h:85: void
> consoleTimeEnd(ScriptExecutionContext*, const String& title, ScriptState*);
> Yes, those are different.

My point is that you can get scriptExecutionContext from ScriptState.

> 
>
https://codereview.chromium.org/24027002/diff/1/Source/core/inspector/Inspect...
> File Source/core/inspector/InspectorController.cpp (right):
> 
>
https://codereview.chromium.org/24027002/diff/1/Source/core/inspector/Inspect...
> Source/core/inspector/InspectorController.cpp:120:
OwnPtr<InspectorConsoleAgent>
> consoleAgentPtr(PageConsoleAgent::create(m_instrumentingAgents.get(),
> m_timelineAgent, m_state.get(), m_injectedScriptManager.get(), domAgent));
> On 2013/09/06 09:25:46, caseq wrote:
> > It would be nice to have agents grouped together in such calls.
> 
> I am not sure I follow.

Let's keep constructor arguments consistent among agents, in particular, if
agent accepts pointers to more than one other agent, let's keep them together.

[caseq, 2013-09-09 08:51:16]

https://codereview.chromium.org/24027002/diff/16001/Source/devtools/front_end...
File Source/devtools/front_end/TimelineManager.js (right):

https://codereview.chromium.org/24027002/diff/16001/Source/devtools/front_end...
Source/devtools/front_end/TimelineManager.js:76: TimelineAgent.stop(callback);
So, your implementation of InspectorTimelineAgent::stop() stops timeline
unconditionally and you have a check for other, page-initiated timelines, being
recorded, in front-end -- is that right?
So, what happens if front-end starts timeline through protocol, then page starts
timeline with console.timeline(), then front-end stops timeline before getting
and processing Timeline.started notification?

[pfeldman, 2013-09-13 13:03:33]

PTAL

https://codereview.chromium.org/24027002/diff/1/Source/core/inspector/Inspect...
File Source/core/inspector/InspectorController.cpp (right):

https://codereview.chromium.org/24027002/diff/1/Source/core/inspector/Inspect...
Source/core/inspector/InspectorController.cpp:120: OwnPtr<InspectorConsoleAgent>
consoleAgentPtr(PageConsoleAgent::create(m_instrumentingAgents.get(),
m_timelineAgent, m_state.get(), m_injectedScriptManager.get(), domAgent));
On 2013/09/06 09:25:46, caseq wrote:
> It would be nice to have agents grouped together in such calls.

Unfortunately, there is no single order of things. I fixed this one to have
agents next to each other.

[caseq, 2013-09-13 15:00:22]

https://codereview.chromium.org/24027002/diff/27001/Source/core/inspector/Ins...
File Source/core/inspector/InspectorTimelineAgent.cpp (right):

https://codereview.chromium.org/24027002/diff/27001/Source/core/inspector/Ins...
Source/core/inspector/InspectorTimelineAgent.cpp:203: innerStart(false);
This will send a duplicate notification to front-end.

https://codereview.chromium.org/24027002/diff/27001/Source/core/inspector/Ins...
Source/core/inspector/InspectorTimelineAgent.cpp:230: *errorString = "Timeline
is already started.";
nit: please no terminating punctuation in error messages.

https://codereview.chromium.org/24027002/diff/27001/Source/core/inspector/Ins...
Source/core/inspector/InspectorTimelineAgent.cpp:641:
appendRecord(TimelineRecordFactory::createTimeStampData(message),
TimelineRecordType::TimeStamp, true, frameForScriptExecutionContext(context));
I'm not sure if passing formatted message rather than user-provided title is a
good thing here. Also, why TimeStamp and not Time/TimeEnd?

https://codereview.chromium.org/24027002/diff/27001/Source/core/inspector/Ins...
Source/core/inspector/InspectorTimelineAgent.cpp:657:
appendRecord(TimelineRecordFactory::createTimeStampData(message),
TimelineRecordType::TimeStamp, true, frameForScriptExecutionContext(context));
Ditto.

https://codereview.chromium.org/24027002/diff/27001/Source/devtools/front_end...
File Source/devtools/front_end/TimelineManager.js (right):

https://codereview.chromium.org/24027002/diff/27001/Source/devtools/front_end...
Source/devtools/front_end/TimelineManager.js:71: if (this._enablementCount < 0)
{
So if we actually hit this, we now let enablemenetCount become negative and
hence it would likely be impossible to enable timeline again until devtools
restart. Is this intentional?

[pfeldman, 2013-09-13 15:21:02]

PTAL

https://codereview.chromium.org/24027002/diff/27001/Source/core/inspector/Ins...
File Source/core/inspector/InspectorTimelineAgent.cpp (right):

https://codereview.chromium.org/24027002/diff/27001/Source/core/inspector/Ins...
Source/core/inspector/InspectorTimelineAgent.cpp:203: innerStart(false);
On 2013/09/13 15:00:23, caseq wrote:
> This will send a duplicate notification to front-end.

Done.

https://codereview.chromium.org/24027002/diff/27001/Source/core/inspector/Ins...
Source/core/inspector/InspectorTimelineAgent.cpp:230: *errorString = "Timeline
is already started.";
On 2013/09/13 15:00:23, caseq wrote:
> nit: please no terminating punctuation in error messages.

Done.

https://codereview.chromium.org/24027002/diff/27001/Source/core/inspector/Ins...
Source/core/inspector/InspectorTimelineAgent.cpp:641:
appendRecord(TimelineRecordFactory::createTimeStampData(message),
TimelineRecordType::TimeStamp, true, frameForScriptExecutionContext(context));
On 2013/09/13 15:00:23, caseq wrote:
> I'm not sure if passing formatted message rather than user-provided title is a
> good thing here. Also, why TimeStamp and not Time/TimeEnd?

That's because timestamps are rendered as event markers, while time/timeEnd is
rendered as bars.

https://codereview.chromium.org/24027002/diff/27001/Source/devtools/front_end...
File Source/devtools/front_end/TimelineManager.js (right):

https://codereview.chromium.org/24027002/diff/27001/Source/devtools/front_end...
Source/devtools/front_end/TimelineManager.js:71: if (this._enablementCount < 0)
{
On 2013/09/13 15:00:23, caseq wrote:
> So if we actually hit this, we now let enablemenetCount become negative and
> hence it would likely be impossible to enable timeline again until devtools
> restart. Is this intentional?

Yes, it is anyways impossible to recover from the unbalanced state.

[caseq, 2013-09-13 15:36:17]

https://codereview.chromium.org/24027002/diff/21001/Source/core/inspector/Ins...
File Source/core/inspector/InspectorConsoleAgent.cpp (right):

https://codereview.chromium.org/24027002/diff/21001/Source/core/inspector/Ins...
Source/core/inspector/InspectorConsoleAgent.cpp:222:
addMessageToConsole(ConsoleAPIMessageSource, LogMessageType, DebugMessageLevel,
message, String(), 0, 0, state);
are we intentionally loosing call-site location here?

[caseq, 2013-09-13 15:50:31]

lgtm

Note that it may be counter-intuitive for user that timeline settings (e.g.
stack capture depth) are not applied when timeline is started from the console.
We probably need to change these to BackendSettin rather than pass them in
TimelineAgent.start()

[commit-bot: I haz the power, 2013-09-16 13:33:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pfeldman@chromium.org/24027002/21001

[commit-bot: I haz the power, 2013-09-16 15:34:34]

Change committed as 157827