[Webkit-unassigned] [Bug 88070] [Gtk] Process Gtk 3.4 smooth scroll events properly

Wed Jun 6 10:51:15 PDT 2012

https://bugs.webkit.org/show_bug.cgi?id=88070

Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #146054|review?                     |review-
               Flag|                            |

--- Comment #9 from Martin Robinson <mrobinson at webkit.org>  2012-06-06 10:51:14 PST ---
(From update of attachment 146054)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.