Bug 150178 – Web Inspector: Timeline current time marker does not start moving when starting recording after just opening inspector

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

* 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.

[Radar WebKit Bug Importer]

<rdar://problem/23130818>

[Joseph Pecoraro]

Created attachment 263177 [details]
[PATCH] Proposed Fix

[Blaze Burg]

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.

[Joseph Pecoraro]

(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?

[Blaze Burg]

(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.

[Joseph Pecoraro]

Created attachment 264075 [details]
[PATCH] Proposed Fix

Better fix taking Brian's suggestion. I like it much better.

[Timothy Hatcher]

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)

[Blaze Burg]

Comment on attachment 264075 [details]
[PATCH] Proposed Fix

He beat me to it!

[Joseph Pecoraro]

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.

[Blaze Burg]

Comment on attachment 264156 [details]
[PATCH] Proposed Fix

r=me

[WebKit Commit Bot]

Comment on attachment 264156 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 264156

Committed r191639: <http://trac.webkit.org/changeset/191639>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.