[webkit-reviews] review denied: [Bug 88070] [Gtk] Process Gtk 3.4 smooth scroll events properly : [Attachment 146054] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied José Dapena Paz
<jdapena at igalia.com>'s request for review:
Bug 88070: [Gtk] Process Gtk 3.4 smooth scroll events properly
https://bugs.webkit.org/show_bug.cgi?id=88070

Attachment 146054: Patch
https://bugs.webkit.org/attachment.cgi?id=146054&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=146054&action=review


Apologies as I didn't see that you added support for WebKit2 Mac. I think that
part belongs in another patch. Thanks for extending this fix to WebKit2 as
well. Can we unskip some tests for WebKit2? 

By the way, you might wish to take a look at the WebKit style guidelines
(http://www.webkit.org/coding/coding-style.html) if you haven't already.

> Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp:83
> +	   case GDK_SCROLL_SMOOTH:
> +	       {
> +		   gdouble deltaX, deltaY;
> +		   gdk_event_get_scroll_deltas((GdkEvent *) event,
> +					       &deltaX,
> +					       &deltaY);
> +		   m_deltaX = -deltaX;
> +		   m_deltaY = -deltaY;
> +	       }
> +	       break;

Looks like you forgot this bit. :)

> Tools/ChangeLog:8
> +	   WTR needs an implementation for eventSender.continuousMouseScrollBy
> +	   https://bugs.webkit.org/show_bug.cgi?id=69417
> +
> +	   Added continousMouseScrollBy support in WebKitTestRunner, and added
> +	   implementation for gtk and mac, and stub for Qt.
> +

You'll need to combine these two changelog entries for this patch to land
properly. It's probably easiest just to run generate-ChangeLogs again with just
this bug number.

> Tools/DumpRenderTree/gtk/EventSender.cpp:84
> +// WebCore and layout tests assume this value
> +static const float pixelsPerScrollTick = 40.0f;

WebKit style guidelines suggest that you can just write:

// WebCore and layout tests assume this value.
 84static const float pixelsPerScrollTick = 40;

Note that comments that are complete sentences should have punctuation.

> Tools/DumpRenderTree/gtk/EventSender.cpp:441
> +

This is a bit of a nit, but I think this newline could be removed.

> Tools/DumpRenderTree/gtk/EventSender.cpp:443
> +    if ((horizontal && vertical) || horizontal > 1 || horizontal < -1 ||
vertical > 1 || vertical < -1) {

This code now handles horizontal and vertical scrolls that are great than 1 in
either direction. Perhaps the comment above should mention that as well.

> Tools/DumpRenderTree/gtk/EventSender.cpp:452
> +#endif
> +    g_return_val_if_fail((!vertical || !horizontal),
JSValueMakeUndefined(context));

Perhaps this should go into an #else block?

> Tools/DumpRenderTree/gtk/EventSender.cpp:471
> +    // GTK support continuous scroll events from 3.3.18

I think you can just remove this comment for now.

> Tools/DumpRenderTree/gtk/EventSender.cpp:480
> +    int horizontal = (int)JSValueToNumber(context, arguments[0], exception);


If this cast is really necessary it should be a static_cast. I'm not sure that
it is necessary though.

> Tools/DumpRenderTree/gtk/EventSender.cpp:482
> +    int vertical = (int)JSValueToNumber(context, arguments[1], exception);

Ditto.

> Tools/DumpRenderTree/gtk/EventSender.cpp:485
> +    gboolean paged = false;

JSValueToBoolean returns a bool, so it's appropriate to use that type here.

> Tools/DumpRenderTree/gtk/EventSender.cpp:486
> +    if (argumentCount == 3) {

Perhaps this should be argumentCount >= 3 for future-proofing.

> Tools/WebKitTestRunner/InjectedBundle/Bindings/EventSendingController.idl:33
> +	   void continuousMouseScrollBy (in long x, in long y, in [Optional]
boolean paged);

You have an extra space after continuousMouseScrollBy here.

> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:48
> +// WebCore and layout tests assume this value
> +static const float pixelsPerScrollTick = 40.0f;

Same comment as above.

> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:414
> +    // We need smooth scroll events to implement this in Gtk+. In any case
> +    // we don't support paged scroll events.

Probably can just say "GTK+ does not support paged scroll events."

> Tools/WebKitTestRunner/mac/EventSenderProxy.mm:415
> +{
> +    RetainPtr<CGEventRef> cgScrollEvent(AdoptCF,
CGEventCreateScrollWheelEvent(0, kCGScrollEventUnitPixel, 2, y, x));
> +
> +    // CGEvent locations are in global display coordinates.
> +    CGPoint lastGlobalMousePosition = {
> +	   m_position.x,
> +	   [[NSScreen mainScreen] frame].size.height - m_position.y
> +    };
> +    CGEventSetLocation(cgScrollEvent.get(), lastGlobalMousePosition);
> +
> +    NSEvent *event = [NSEvent eventWithCGEvent:cgScrollEvent.get()];
> +    NSView *targetView = [m_testController->mainWebView()->platformView()
hitTest:[event locationInWindow]];
> +    if (targetView)
> +	   [targetView scrollWheel:event];
> +}

This can probably be saved for another patch.

> LayoutTests/platform/gtk/TestExpectations:734
>  // mouseScrollBy() and continuousMouseScrollBy() are not yet implemented in
the GTK EventSender API.
> -BUGWK36003 : fast/events/remove-child-onscroll.html = FAIL
> -BUGWK36003 : fast/events/platform-wheelevent-in-scrolling-div.html = FAIL
> -BUGWK36003 :
fast/events/continuous-platform-wheelevent-in-scrolling-div.html = FAIL
> -BUGWK36003 : fast/events/wheelevent-direction-inverted-from-device.html =
FAIL
>  BUGWK36003 : fast/events/wheelevent-in-horizontal-scrollbar-in-rtl.html =
FAIL
>  BUGWK36003 : fast/events/wheelevent-in-vertical-scrollbar-in-rtl.html = FAIL

> -BUGWK36003 : fast/events/scroll-in-scaled-page-with-overflow-hidden.html =
FAIL
>  BUGWK36003 :
fast/events/platform-wheelevent-paging-x-in-non-scrolling-div.html = FAIL
>  BUGWK36003 :
fast/events/platform-wheelevent-paging-x-in-non-scrolling-page.html = FAIL
>  BUGWK36003 : fast/events/platform-wheelevent-paging-x-in-scrolling-div.html
= FAIL

Why are the rest of the tests failing? It would be good to update the comment
and potentially open bugs for the remaining failures. Particularly
continuousMouseScrollBy is implemented now, so the comment is inaccurate.