Bug 142189 – The InspectorTimelineAgent should gracefully handle attempts to start more than once.

• Component: Web Inspector
• Status: RESOLVED FIXED

[Mark Lam]

InspectorTimelineAgent::internalStop() already checks for redundant calls to it in case the InspectorTimelineAgent is already disabled.  Similarly, InspectorTimelineAgent::internalStart() should check if the InspectorTimelineAgent is already enabled before proceeding to do work to enable it.   Though wasteful, it is legal for clients of the InspectorTimelineAgent to invoke start on it more than once.  Hence, this check is needed.

This check fixes the debug assertion failure when running the inspector/timeline/debugger-paused-while-recording.html test.  The test can now be unskipped.

[Radar WebKit Bug Importer]

<rdar://problem/20012601>

[Joseph Pecoraro]

"should not be allowed to start twice" => "should gracefully handling attempting to start twice".

[Mark Lam]

Created attachment 247705 [details]
the patch.

[Joseph Pecoraro]

Comment on attachment 247705 [details]
the patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=247705&action=review

r=me

> Source/WebCore/ChangeLog:13
> +        is already enabled before proceeding to do work to enable it.   Though wasteful,

Nit: No need for more than one space after ".".

> LayoutTests/TestExpectations:-100
> -webkit.org/b/137131 inspector/timeline [ Skip ]

I hope this wasn't flakey for other reasons.

[Mark Lam]

(In reply to comment #4)
> Comment on attachment 247705 [details]
> the patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247705&action=review
> 
> r=me
> 
> > Source/WebCore/ChangeLog:13
> > +        is already enabled before proceeding to do work to enable it.   Though wasteful,
> 
> Nit: No need for more than one space after ".".

=)  I always preferred 2 spaces for readability, but I can remove it.

> > LayoutTests/TestExpectations:-100
> > -webkit.org/b/137131 inspector/timeline [ Skip ]
> 
> I hope this wasn't flakey for other reasons.

I didn't see any, but if the bots complain, we can skip this again.

[Brian Burg]

Comment on attachment 247705 [details]
the patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=247705&action=review

I wonder why some clients call start and stop multiple times. Is it because they assume some initial on/off state?

> Source/WebCore/ChangeLog:8
> +        No new tests.  Unskipped an existing test that already asserts this.

Test line should be below main description.

> Source/WebCore/ChangeLog:10
> +        InspectorTimelineAgent::internalStop() already checks for redundant calls to it in

This paragraph is longer than necessary. You can simply state that you added a missing guard that prevents the timeline agent from starting if it has already started, matching the logic that guards against duplicated calls to internalStop().

[Brian Burg]

Comment on attachment 247705 [details]
the patch.

Accidentally cleared Joe's r+

[Mark Lam]

Thanks for the review.  Landed in r180901: <http://trac.webkit.org/r180901>.