Web Replay: capture and replay wheel events and scroll commands

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[BJ Burg]

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.

[BJ Burg]

I will roll wheel events and scroll commands into this patch since they all work together to implement common scrolling scenarios.

[BJ Burg]

Created attachment 227914 [details]
wip - no scrolling coordinator integration yet

[Timothy Hatcher]

Comment on attachment 227914 [details]
wip - no scrolling coordinator integration yet

Looks good so far.

[Brian Burg]

Created attachment 228237 [details]
the patch

[Timothy Hatcher]

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 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]

Created attachment 228347 [details]
the patch.2

[Brian Burg]

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

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 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]

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

So should ForcedDeterministicScrolling be removed from the patch?

[Brian Burg]

(In reply to comment #12)
> So should ForcedDeterministicScrolling be removed from the patch?

Yes, I'll remove it before landing.

[Brian Burg]

Created attachment 228424 [details]
Patch

[WebKit Commit Bot]

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]

EWS redness appears unrelated, as the tree was pretty red at the time (mac-wk2) and internal compiler error (efl).

[WebKit Commit Bot]

Comment on attachment 228424 [details]
Patch

Clearing flags on attachment: 228424

Committed r166811: <http://trac.webkit.org/changeset/166811>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

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]

(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)]

I'm worried that blindly using 64 bits for all enums will have bloated a lot of data structures without us noticing.