Bug 135295

Summary: Web Replay: dispatch timing information should be stored out-of-line in a replay segment
Product: WebKit Reporter: Brian Burg <burg>
Component: WebCore Misc.Assignee: Brian Burg <burg>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, graouts, joepeck, kling, timothy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Remove unused m_page member none

Description Brian Burg 2014-07-25 11:03:56 PDT
We need to save a timestamp for each event loop input so that replay can simulate the original user and network delays. Currently that timestamp is stored on each event loop input instance.

I argue that it should be stored in a separate vector attached to the segment rather than as a member variable of some NondeterministicInput subclasses. This will make the event loop input class immutable, and make it easier to modify or add auxiliary debugging data in the future (such as how many DOM events we should expect per user action).
Comment 1 Brian Burg 2014-07-25 11:21:06 PDT
Created attachment 235528 [details]
Patch
Comment 2 Brian Burg 2014-08-06 11:38:10 PDT
Comment on attachment 235528 [details]
Patch

This patch is actually ready for review, don't know why I marked it WIP.
Comment 3 Timothy Hatcher 2014-08-06 11:41:12 PDT
Comment on attachment 235528 [details]
Patch

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

> Source/WebCore/replay/CapturingInputCursor.cpp:47
> +    UNUSED_PARAM(page);

I don't think this is needed since it was used when setting m_page.

> Source/WebCore/replay/CapturingInputCursor.cpp:66
> +        // FIXME: rewrite this (and related dispatch code) to use std::chrono.

Are we using std::chrono in other places now?
Comment 4 Brian Burg 2014-08-06 11:44:24 PDT
Comment on attachment 235528 [details]
Patch

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

>> Source/WebCore/replay/CapturingInputCursor.cpp:47
>> +    UNUSED_PARAM(page);
> 
> I don't think this is needed since it was used when setting m_page.

I think clang complains about m_page never being used. On my branch, I just removed the field and the various plumbing of it through constructors. (I guess that's why it was WIP? nothing else is different)

>> Source/WebCore/replay/CapturingInputCursor.cpp:66
>> +        // FIXME: rewrite this (and related dispatch code) to use std::chrono.
> 
> Are we using std::chrono in other places now?

It is andersca's New Favorite Thing. I haven't had time to figure it out, but it's supposedly good stuff.
Comment 5 Brian Burg 2014-08-06 11:47:28 PDT
Created attachment 236122 [details]
Remove unused m_page member
Comment 6 WebKit Commit Bot 2014-08-06 14:53:39 PDT
Comment on attachment 236122 [details]
Remove unused m_page member

Clearing flags on attachment: 236122

Committed r172180: <http://trac.webkit.org/changeset/172180>
Comment 7 WebKit Commit Bot 2014-08-06 14:53:42 PDT
All reviewed patches have been landed.  Closing bug.