[webkit-reviews] review denied: [Bug 25503] Add instrumentation framework to WebCore : [Attachment 38382] Similar instrumentation points, but this time it goes to an agent in InspectorController.

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at hatcher.name> has denied Kelly Norton
<knorton at google.com>'s request for review:
Bug 25503: Add instrumentation framework to WebCore
https://bugs.webkit.org/show_bug.cgi?id=25503

Attachment 38382: Similar instrumentation points, but this time it goes to an
agent in InspectorController.
https://bugs.webkit.org/attachment.cgi?id=38382&action=review

------- Additional Comments from Timothy Hatcher <timothy at hatcher.name>

> +InspectorTimelineAgent* Document::inspectorTimelineAgent() const
> +{
> +    return page() ? page()->inspectorTimelineAgent() : 0;
> +}

Make these getters inline functions and it will be better. It would make the
callers simplier to keep them.

> +    if (inspectorTimelineAgent())
> +	   inspectorTimelineAgent()->willRecalculateStyle();

This should store the timeline agent into a local so the work isn't done twice
when it is enabled. Same for the other callers.

> +void InspectorTimelineAgent::willDispatchDOMEvent(const Event& event)
> +{
> +    TimelineItem* newItem = newTimelineItem(DOMDispatchTimelineItemType);
> +    newItem->addDOMEventProperties(m_frontend, event);
> +}
> +
> +void InspectorTimelineAgent::didDispatchDOMEvent()
> +{
> +    didCompleteCurrentRecord();
> +}

I worry about didCompleteCurrentRecord() ending the wrong record. As long as
all the recorded things are synchronous it is fine. We should ASSERT here that
the current TimelineItem is the right type (DOMDispatchTimelineItemType in this
case). And in all the other places didCompleteCurrentRecord() is called.

> +void InspectorTimelineAgent::TimelineItem::addItemProperties(double endTime)

> +{
> +    m_scriptObject.set("time", m_startTime);
> +    m_scriptObject.set("type", static_cast<int>(m_itemType));
> +    m_scriptObject.set("duration", endTime - m_startTime);
> +    if (m_childObjects.length())
> +	   m_scriptObject.set("children", m_childObjects);
> +}
> +
> +void
InspectorTimelineAgent::TimelineItem::addDOMEventProperties(InspectorFrontend*
frontend, const Event& event)
> +{
> +    ScriptObject data = frontend->newScriptObject();
> +    data.set("type", event.type().string());
> +    m_scriptObject.set("data", data);
> +}

These should be normal data members, and more like ConsoleMessage when the
ScriptObjects are created. Like what Pavel was saying on IRC.

> +	   // Must be kept in sync with Timeline.js

This should say TimelineAgent.js.