[webkit-reviews] review granted: [Bug 225788] Add a way to create `"wheel"` events from gesture/touch events : [Attachment 429032] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 225788: Add a way to create `"wheel"` events from gesture/touch events
https://bugs.webkit.org/show_bug.cgi?id=225788

Attachment 429032: Patch

https://bugs.webkit.org/attachment.cgi?id=429032&action=review




--- Comment #6 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 429032
  --> https://bugs.webkit.org/attachment.cgi?id=429032
Patch

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

> Source/WebCore/platform/PlatformWheelEvent.cpp:49
> +    platformWheelEvent.m_timestamp = platformGestureEvent.timestamp();
> +    platformWheelEvent.m_modifiers = platformGestureEvent.modifiers() |
PlatformEvent::Modifier::ControlKey;

Can we give PlatformWheelEvent a PlatformEvent copy ctor that initializes the
shared members? Otherwise someone will add something in future and not know to
fix this code path.

> Source/WebCore/platform/PlatformWheelEvent.cpp:61
> +	   platformWheelEvent.m_phase = deltaY ? PlatformWheelEventPhase::Began
: PlatformWheelEventPhase::MayBegin;

This only makes sense if the first event has deltaY=0 and no subsequent event
does (MayBegin should only ever be followed by Began or Canceled).

> Source/WebCore/platform/PlatformWheelEvent.cpp:62
> +	   platformWheelEvent.m_momentumPhase = deltaY ?
PlatformWheelEventPhase::Began : PlatformWheelEventPhase::None;

It doesn't make sense to have an event where both m_phase and m_momentumPhase
are PlatformWheelEventPhase::Began. m_phase and m_momentumPhase are really
mutually exclusive. There are no momentum phases in this gesture, I think.

> Tools/ChangeLog:42
> +	   Make sure to pass all feature flags when generating JS files from
IDL files.

Oh finally! But what about DumpRenderTree?

> Tools/WebKitTestRunner/DerivedSources.make:43
> +FRAMEWORK_FLAGS := $(shell echo $(BUILT_PRODUCTS_DIR)
$(FRAMEWORK_SEARCH_PATHS) $(SYSTEM_FRAMEWORK_SEARCH_PATHS) | $(PERL) -e 'print
"-F " . join(" -F ", split(" ", <>));')
> +HEADER_FLAGS := $(shell echo $(BUILT_PRODUCTS_DIR) $(HEADER_SEARCH_PATHS)
$(SYSTEM_HEADER_SEARCH_PATHS) | $(PERL) -e 'print "-I" . join(" -I", split(" ",
<>));')
> +FEATURE_AND_PLATFORM_DEFINES := $(shell $(CC) -std=gnu++1z -x c++ -E -P -dM
$(SDK_FLAGS) $(TARGET_TRIPLE_FLAGS) $(FRAMEWORK_FLAGS) $(HEADER_FLAGS) -include
"wtf/Platform.h" /dev/null | $(PERL) -ne "print if s/\#define
((HAVE_|USE_|ENABLE_|WTF_PLATFORM_)\w+) 1/\1/")

Someone else should review this.

> Tools/WebKitTestRunner/EventSenderProxy.h:94
> +    void scaleGestureStart(double scale);
> +    void scaleGestureChange(double scale);
> +    void scaleGestureEnd(double scale);

Don't these have an origin too?

> Tools/WebKitTestRunner/mac/EventSenderProxy.mm:133
> +    CGEventSetIntegerValueField(cgEvent.get(), kCGEventGestureHIDType, 8 /*
kIOHIDEventTypeZoom */);

Should we put kIOHIDEventTypeZoom in an SPI header?

> Tools/WebKitTestRunner/mac/EventSenderProxy.mm:910
> +    auto event = adoptNS([[EventSenderSyntheticEvent alloc]
initMagnifyEventAtLocation:NSMakePoint(m_position.x, m_position.y)

Maybe pull [m_testController->mainWebView()->platformView() into a local
variable.