[webkit-reviews] review denied: [Bug 30467] Add DOMTimer support to InspectorTimelineAgent. : [Attachment 41335] Adds support for timers.

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Kelly Norton
<knorton at google.com>'s request for review:
Bug 30467: Add DOMTimer support to InspectorTimelineAgent.
https://bugs.webkit.org/show_bug.cgi?id=30467

Attachment 41335: Adds support for timers.
https://bugs.webkit.org/attachment.cgi?id=41335&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>

> +void InspectorTimelineAgent::didInstallTimer(int timerId, int timeout, bool
singleShot)
> +{
> +   
m_frontend->addItemToTimeline(TimerFireTimelineItem::newTimerInstallScriptObjec
t(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).