Bug 129402

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

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

Comment on attachment 227914 [details] wip - no scrolling coordinator integration yet Looks good so far.

Created attachment 228237 [details] the patch

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.

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.

Created attachment 228347 [details] the patch.2

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

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.

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.

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

So should ForcedDeterministicScrolling be removed from the patch?

(In reply to comment #12) > So should ForcedDeterministicScrolling be removed from the patch? Yes, I'll remove it before landing.

Created attachment 228424 [details] Patch

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.

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

Comment on attachment 228424 [details] Patch Clearing flags on attachment: 228424 Committed r166811: <http://trac.webkit.org/changeset/166811>

All reviewed patches have been landed. Closing bug.

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?

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

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