Bug 150178 - Web Inspector: Timeline current time marker does not start moving when starting recording after just opening inspector
Summary: Web Inspector: Timeline current time marker does not start moving when starti...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-10-15 11:50 PDT by Joseph Pecoraro
Modified: 2015-10-27 14:39 PDT (History)
8 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (5.38 KB, patch)
2015-10-15 12:08 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (6.04 KB, patch)
2015-10-26 14:22 PDT, Joseph Pecoraro
timothy: review+
Details | Formatted Diff | Diff
[PATCH] Proposed Fix (6.42 KB, patch)
2015-10-27 13:50 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2015-10-15 11:50:10 PDT
* 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.
Comment 1 Radar WebKit Bug Importer 2015-10-15 11:51:21 PDT
<rdar://problem/23130818>
Comment 2 Joseph Pecoraro 2015-10-15 12:08:15 PDT
Created attachment 263177 [details]
[PATCH] Proposed Fix
Comment 3 BJ Burg 2015-10-18 15:53:59 PDT
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.
Comment 4 Joseph Pecoraro 2015-10-20 10:58:46 PDT
(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?
Comment 5 BJ Burg 2015-10-20 11:40:27 PDT
(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.
Comment 6 Joseph Pecoraro 2015-10-26 14:22:17 PDT
Created attachment 264075 [details]
[PATCH] Proposed Fix

Better fix taking Brian's suggestion. I like it much better.
Comment 7 Timothy Hatcher 2015-10-27 09:34:22 PDT
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 8 BJ Burg 2015-10-27 09:35:45 PDT
Comment on attachment 264075 [details]
[PATCH] Proposed Fix

He beat me to it!
Comment 9 Joseph Pecoraro 2015-10-27 13:50:02 PDT
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 10 BJ Burg 2015-10-27 13:54:04 PDT
Comment on attachment 264156 [details]
[PATCH] Proposed Fix

r=me
Comment 11 WebKit Commit Bot 2015-10-27 14:39:33 PDT
Comment on attachment 264156 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 264156

Committed r191639: <http://trac.webkit.org/changeset/191639>
Comment 12 WebKit Commit Bot 2015-10-27 14:39:36 PDT
All reviewed patches have been landed.  Closing bug.