[webkit-reviews] review denied: [Bug 31080] WebInspector: Handle turning TimelineAgent on/off in the middle of an event dispatch : [Attachment 42413] WebInspector: Adds test to bug 31080, Handle turning profiling on/off in the middle of event dispatch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Eric Ayers
<zundel at google.com>'s request for review:
Bug 31080: WebInspector: Handle turning TimelineAgent on/off in the middle of
an event dispatch
https://bugs.webkit.org/show_bug.cgi?id=31080

Attachment 42413: WebInspector: Adds test to bug 31080, Handle turning
profiling on/off in the middle of event dispatch
https://bugs.webkit.org/attachment.cgi?id=42413&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
Please refer to a bug from the ChangeLog (s)!


> -	       type == eventNames().clickEvent || type ==
eventNames().mousedownEvent ||
> -	       type == eventNames().mouseupEvent || type ==
eventNames().dblclickEvent ||
> +	       type == eventNames().clickEvent || type ==
eventNames().mousedownEvent 
> +	       || type == eventNames().mouseupEvent || type ==
eventNames().dblclickEvent 
>	       // keyboard events
> -	       type == eventNames().keydownEvent || type ==
eventNames().keypressEvent ||
> -	       type == eventNames().keyupEvent ||
> +	       || type == eventNames().keydownEvent || type ==
eventNames().keypressEvent
> +	       || type == eventNames().keyupEvent
>	       // other accepted events
> -	       type == eventNames().selectEvent || type ==
eventNames().changeEvent ||
> -	       type == eventNames().focusEvent || type ==
eventNames().blurEvent ||
> -	       type == eventNames().submitEvent)
> +	       || type == eventNames().selectEvent || type ==
eventNames().changeEvent
> +	       || type == eventNames().focusEvent || type ==
eventNames().blurEvent
> +	       || type == eventNames().submitEvent)
>	       return true;

Nothing changed, right?

> +    // An empty stack could merely mean that the timeline agent was turned
on in the middle of
> +    // an event.  Don't treat as an error.

We are now forgiving too much and it could be error-prone. I think it is Ok for
now though.

> +    if (!m_recordStack.isEmpty()) {
> +	   TimelineRecordEntry entry = m_recordStack.last();
> +	   m_recordStack.removeLast();
> +	   ASSERT(entry.type == type);

There is no guarantee that the top of the stack is the matching event, is
there?
Counter and a map would save the world, but will make things more complex.
Returning timelineagent "version" from will* methods and passing it back to
did* method would be Ok.