RESOLVED FIXED 129402
Web Replay: capture and replay wheel events and scroll commands 
https://bugs.webkit.org/show_bug.cgi?id=129402
Summary Web Replay: capture and replay wheel events and scroll commands
Blaze Burg
Reported 2014-02-26 15:39:07 PST
The capture and replay machinery is not thread-safe, so it would be very painful to change it to capture and replay true async scrolling. It would also make recordings unintelligible between async and !async-enabled clients. Instead, we should just force synchronous scrolling when capturing or replaying. In a branch, I implemented this by adding a flag (set by ReplayController) that contributes to ScrollingCoordinator::synchronousScrollingReasons. Let me know if that's not the right thing to do.
Attachments
wip - no scrolling coordinator integration yet (28.86 KB, patch)
2014-03-26 20:41 PDT, Blaze Burg 		no flags
DetailsFormatted DiffDiff
the patch (36.40 KB, patch)
2014-03-31 20:47 PDT, Brian Burg 		no flags
DetailsFormatted DiffDiff
the patch.2 (36.31 KB, patch)
2014-04-01 17:38 PDT, Brian Burg 		no flags
DetailsFormatted DiffDiff
Patch (35.94 KB, patch)
2014-04-02 13:36 PDT, Brian Burg 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Blaze Burg
Comment 1 2014-03-16 14:18:00 PDT
I will roll wheel events and scroll commands into this patch since they all work together to implement common scrolling scenarios.
Blaze Burg
Comment 2 2014-03-26 20:41:16 PDT
Created attachment 227914 [details] wip - no scrolling coordinator integration yet
Timothy Hatcher
Comment 3 2014-03-26 21:29:17 PDT
Comment on attachment 227914 [details] wip - no scrolling coordinator integration yet Looks good so far.
Brian Burg
Comment 4 2014-03-31 20:47:09 PDT
Created attachment 228237 [details] the patch
Timothy Hatcher
Comment 5 2014-04-01 11:58:19 PDT
Comment on attachment 228237 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=228237&action=review > Source/WebCore/replay/SerializationMethods.cpp:82 > +#define DECODE_OWNED_TYPE_WITH_KEY(_encodedValue, _type, _key) \ DECODE_UNIQUE_TYPE_WITH_KEY? DECODE_OWNED_TYPE_WITH_KEY sounds like OwnPtr.
Simon Fraser (smfr)
Comment 6 2014-04-01 15:01:32 PDT
Comment on attachment 228237 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=228237&action=review > Source/WebCore/page/scrolling/ScrollingTree.cpp:366 > +#if ENABLE(WEB_REPLAY) > +void ScrollingTree::setDeterministicScrollingEnabled(bool flag) > +{ > + m_deterministicScrollingEnabled = flag; > +} > + > +bool ScrollingTree::deterministicScrollingEnabled() > +{ > + return m_deterministicScrollingEnabled; > +} > +#endif > + I think you should add a flag to ScrollingCoordinator::MainThreadScrollingReasonFlags rather than doing this.
Brian Burg
Comment 7 2014-04-01 17:38:30 PDT
Created attachment 228347 [details] the patch.2
Brian Burg
Comment 8 2014-04-01 17:39:27 PDT
(In reply to comment #6) > (From update of attachment 228237 [details]) > > I think you should add a flag to ScrollingCoordinator::MainThreadScrollingReasonFlags rather than doing this. Oh, yes that is a bit simpler. I redid it that way in the latest patch.
WebKit Commit Bot
Comment 9 2014-04-01 17:40:41 PDT
Attachment 228347 [details] did not pass style-queue: ERROR: Source/WebCore/replay/SerializationMethods.cpp:79: _key is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/replay/SerializationMethods.cpp:83: _key is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/replay/SerializationMethods.cpp:87: _key is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/replay/SerializationMethods.cpp:95: _key is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/ScrollTypes.h:33: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 5 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Timothy Hatcher
Comment 10 2014-04-02 11:46:19 PDT
Comment on attachment 228347 [details] the patch.2 View in context: https://bugs.webkit.org/attachment.cgi?id=228347&action=review > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:310 > + synchronousScrollingReasons |= ForcedOnMainThread; Looks like this should use ForcedDeterministicScrolling.
Brian Burg
Comment 11 2014-04-02 12:02:44 PDT
(In reply to comment #10) > (From update of attachment 228347 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=228347&action=review > > > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:310 > > + synchronousScrollingReasons |= ForcedOnMainThread; > > Looks like this should use ForcedDeterministicScrolling. Simon said (lol) to just reuse ForcedOnMainThread.
Timothy Hatcher
Comment 12 2014-04-02 12:07:47 PDT
So should ForcedDeterministicScrolling be removed from the patch?
Brian Burg
Comment 13 2014-04-02 13:13:53 PDT
(In reply to comment #12) > So should ForcedDeterministicScrolling be removed from the patch? Yes, I'll remove it before landing.
Brian Burg
Comment 14 2014-04-02 13:36:00 PDT
Created attachment 228424 [details] Patch
WebKit Commit Bot
Comment 15 2014-04-02 13:38:44 PDT
Attachment 228424 [details] did not pass style-queue: ERROR: Source/WebCore/replay/SerializationMethods.cpp:79: _key is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/replay/SerializationMethods.cpp:83: _key is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/replay/SerializationMethods.cpp:87: _key is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/replay/SerializationMethods.cpp:95: _key is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/ScrollTypes.h:33: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 5 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Brian Burg
Comment 16 2014-04-04 08:52:25 PDT
EWS redness appears unrelated, as the tree was pretty red at the time (mac-wk2) and internal compiler error (efl).
WebKit Commit Bot
Comment 17 2014-04-04 16:14:42 PDT
Comment on attachment 228424 [details] Patch Clearing flags on attachment: 228424 Committed r166811: <http://trac.webkit.org/changeset/166811>
WebKit Commit Bot
Comment 18 2014-04-04 16:14:49 PDT
All reviewed patches have been landed. Closing bug.
Simon Fraser (smfr)
Comment 19 2014-07-23 14:32:23 PDT
Comment on attachment 228424 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228424&action=review > Source/WebCore/platform/ScrollTypes.h:33 > + enum ScrollDirection : uint64_t { Did these really need to take up 64 bits?
Brian Burg
Comment 20 2014-07-23 18:44:55 PDT
(In reply to comment #19) > (From update of attachment 228424 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=228424&action=review > > > Source/WebCore/platform/ScrollTypes.h:33 > > + enum ScrollDirection : uint64_t { > > Did these really need to take up 64 bits? Definitely not. Previous patches for other replay things used the smallest acceptable explicit size (usually u8), but Anders said (in another review or on IRC) that using different explicit sizes was confusing and to just use a size that fits any enums. (I'm happy to put up a patch changing it to use whatever explicit size people find acceptable... as long as it can be forward-declared)
Simon Fraser (smfr)
Comment 21 2014-07-23 19:03:56 PDT
I'm worried that blindly using 64 bits for all enums will have bloated a lot of data structures without us noticing.
Note You need to log in before you can comment on or make changes to this bug.