Bug 135295 - Web Replay: dispatch timing information should be stored out-of-line in a replay segment
Summary: Web Replay: dispatch timing information should be stored out-of-line in a rep...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brian Burg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-07-25 11:03 PDT by Brian Burg
Modified: 2014-08-06 14:53 PDT (History)
5 users (show)

See Also:


Attachments
Patch (26.12 KB, patch)
2014-07-25 11:21 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
Remove unused m_page member (25.99 KB, patch)
2014-08-06 11:47 PDT, Brian Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.