Bug 30467 – Add DOMTimer support to InspectorTimelineAgent.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Kelly Norton]

Allows information about timers to be collected within inspector via the TimelineAgent.

[Kelly Norton]

Created attachment 41335 [details]
Adds support for timers.

[Pavel Feldman]

Comment on attachment 41335 [details]
Adds support for timers.


> +void InspectorTimelineAgent::didInstallTimer(int timerId, int timeout, bool singleShot)
> +{
> +    m_frontend->addItemToTimeline(TimerFireTimelineItem::newTimerInstallScriptObject(m_frontend, sessionTimeInMilliseconds(), timerId, timeout, singleShot));
> +}
TimerF
It seems like you are pushing script objects right to the frontend, without keeping them in the controller. That is Ok for 'profile' mode, but you don't need TimelineItem and TimerFireTimeline classes in that case. I'd suggest that you do one of the following:

1) Get rid of these classes, leave only static factories for creating corresponding events. Glue them together on the frontend side so that you did not need m_currentTimelineItem.

2) Leave TimelineItem and TimerTimelineItem classes;
   Collect them into the timeline events vector within TimelineAgent;
   Create populateScriptObjects and resetScriptObjects methods in TimelineAgent and call them from within corresponding inspector controller methods;
   Provide a way for TimelineAgent to push events to the frontend on the fly if latter is available. (Either via passing inspector controller into the timeline agent or coming up with some specific interface).

My gut is telling me that (2) is better long term, but I can't really explain why. It seems that (1) is enough at the moment. But (2) allows capturing the state in the frontend-less mode and all other inspector controller metrics (including profiles) are supporting this mode.

Drive-by question: Should you introduce some kind of a bit set (or convert TimelineItemTypes consts into one) so that you could filter events based on some preferences. I am sure you will be doing filtering, but given the granularity of the events of your interest, it might make sense to filter on the inspector controller side.

I am putting r- because I'd like to explore how InspectorController/InspectorFrontend interaction could be changed in order to allow you plugging your new agent in a more elegant manner. Please also share with me your thoughts on (1) vs (2).

[Kelly Norton]

Created attachment 41839 [details]
Updated patch after changing over to ScriptObjects (bug 30707)

[Timothy Hatcher]

Comment on attachment 41839 [details]
Updated patch after changing over to ScriptObjects (bug 30707)

I like the term "Record" over "TimelineItem". But I see the logic in the name change.

> +    if (context->isDocument())
> +        if (InspectorTimelineAgent* timelineAgent = static_cast<Document*>(context)->inspectorTimelineAgent())
> +            timelineAgent->didInstallTimer(timer->m_timeoutId, timeout, singleShot);

There should be braces around the outer if() since it is multi-line inside.

This would be better as helper inline function that returns InspectorTimelineAgent* to share with the other spots in this code.

[Kelly Norton]

(In reply to comment #4)
> (From update of attachment 41839 [details])
> I like the term "Record" over "TimelineItem". But I see the logic in the name
> change.
> 
I can't remember why I got rid of "Record" but I like the term better too. How about I do a separate renaming patch?

> > +    if (context->isDocument())
> > +        if (InspectorTimelineAgent* timelineAgent = static_cast<Document*>(context)->inspectorTimelineAgent())
> > +            timelineAgent->didInstallTimer(timer->m_timeoutId, timeout, singleShot);
> 
> There should be braces around the outer if() since it is multi-line inside.
>

Ok
 
> This would be better as helper inline function that returns
> InspectorTimelineAgent* to share with the other spots in this code.

Done ... uploading patch now.

[Kelly Norton]

Created attachment 41840 [details]
Added inline function for inspectorTimelineAgent in DOMTimer.

[Timothy Hatcher]

Comment on attachment 41840 [details]
Added inline function for inspectorTimelineAgent in DOMTimer.

> +#if ENABLE(INSPECTOR)
> +        static InspectorTimelineAgent* inspectorTimelineAgent(ScriptExecutionContext*);
> +#endif
> +
>          int m_timeoutId;
>          int m_nestingLevel;
>          OwnPtr<ScheduledAction> m_action;
> @@ -72,6 +79,15 @@ namespace WebCore {
>          static double s_minTimerInterval;
>      };
>  
> +#if ENABLE(INSPECTOR)
> +inline InspectorTimelineAgent* DOMTimer::inspectorTimelineAgent(ScriptExecutionContext* context)
> +{
> +    if (context->isDocument())
> +        return static_cast<Document*>(context)->inspectorTimelineAgent();
> +    return 0;
> +}
> +#endif
> +

I didn't realize this was static before. It would be good to have this as a static in InspectorTimelineAgent.h then.

Maybe as: InspectorTimelineAgent::inspectorTimelineAgent(ScriptExecutionContext* context)

Who else might need that same code?

Otherwise looks good to me.

[Kelly Norton]

Created attachment 41867 [details]
Moved static method to InspectorTimelineAgent.

[Kelly Norton]

Created attachment 41871 [details]
Forgot the JS constants.

[WebKit Commit Bot]

Comment on attachment 41871 [details]
Forgot the JS constants.

Clearing flags on attachment: 41871

Committed r50068: <http://trac.webkit.org/changeset/50068>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Ayers]

*** Bug 31372 has been marked as a duplicate of this bug. ***