* SUMMARY Timeline current time marker does not start moving when starting recording after just opening inspector. * STEPS TO REPRODUCE 1. Load webkit.org 2. Open Inspector 3. Show Timelines Tab 4. Start Recording => expected current time marker to start moving, it did not until the first event.
<rdar://problem/23130818>
Created attachment 263177 [details] [PATCH] Proposed Fix
Comment on attachment 263177 [details] [PATCH] Proposed Fix This is an annoying bug, but I'm not sure this is the right way to fix it. My reservation is that we are potentially updating the TimelineRecording model object based on when the TimelineRecordingContentView is shown (and triggers startUpdatingCurrentTime). Why can't TimelineManager set the recording's startTime if it's not already set, when we get the Timeline.capturingStarted event? This way there's no special fixup that needs to be done in the view code.
(In reply to comment #3) > Comment on attachment 263177 [details] > [PATCH] Proposed Fix > > This is an annoying bug, but I'm not sure this is the right way to fix it. > My reservation is that we are potentially updating the TimelineRecording > model object based on when the TimelineRecordingContentView is shown (and > triggers startUpdatingCurrentTime). In this path, startUpdatingCurrentTime would not have a startTime. > Why can't TimelineManager set the recording's startTime if it's not already > set, when we get the Timeline.capturingStarted event? This way there's no > special fixup that needs to be done in the view code. That is exactly what we are doing here. On handling the WebInspector.TimelineManager.Event.CapturingStarted event, TimelineRecordingContentView calls startUpdatingCurrentTime(startTime) and we set the recording's startTime. Do you think this should be moved somewhere else?
(In reply to comment #4) > (In reply to comment #3) > > Comment on attachment 263177 [details] > > [PATCH] Proposed Fix > > > > This is an annoying bug, but I'm not sure this is the right way to fix it. > > My reservation is that we are potentially updating the TimelineRecording > > model object based on when the TimelineRecordingContentView is shown (and > > triggers startUpdatingCurrentTime). > > In this path, startUpdatingCurrentTime would not have a startTime. > > > > Why can't TimelineManager set the recording's startTime if it's not already > > set, when we get the Timeline.capturingStarted event? This way there's no > > special fixup that needs to be done in the view code. > > That is exactly what we are doing here. On handling the > WebInspector.TimelineManager.Event.CapturingStarted event, > TimelineRecordingContentView calls startUpdatingCurrentTime(startTime) and > we set the recording's startTime. > > Do you think this should be moved somewhere else? I suggest that TimelineManager itself should set the recording's startTime before dispatching CapturingStarted. Then the views can assume that an actively capturing recording has a valid start time.
Created attachment 264075 [details] [PATCH] Proposed Fix Better fix taking Brian's suggestion. I like it much better.
Comment on attachment 264075 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=264075&action=review > Source/WebInspectorUI/UserInterface/Models/TimelineRecording.js:234 > + this._startTime = timestamp; Should this fire? this.dispatchEventToListeners(WebInspector.TimelineRecording.Event.TimesUpdated)
Comment on attachment 264075 [details] [PATCH] Proposed Fix He beat me to it!
Created attachment 264156 [details] [PATCH] Proposed Fix Dispatch the event, and also set endTime because that is what happens when new records come in, so we might as well do it here.
Comment on attachment 264156 [details] [PATCH] Proposed Fix r=me
Comment on attachment 264156 [details] [PATCH] Proposed Fix Clearing flags on attachment: 264156 Committed r191639: <http://trac.webkit.org/changeset/191639>
All reviewed patches have been landed. Closing bug.