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.
<rdar://problem/20012601>
"should not be allowed to start twice" => "should gracefully handling attempting to start twice".
Created attachment 247705 [details] the patch.
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.
(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.
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().
Comment on attachment 247705 [details] the patch. Accidentally cleared Joe's r+
Thanks for the review. Landed in r180901: <http://trac.webkit.org/r180901>.