[Webkit-unassigned] [Bug 155750] [GTK] Add kinetic scrolling

[bugzilla-daemon at webkit.org]

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

--- Comment #11 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 277468
  --> https://bugs.webkit.org/attachment.cgi?id=277468
WIP patch

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

Some more general comments

> Source/WebCore/platform/PlatformWheelEvent.h:32
> +#if PLATFORM(GTK)
> +#include "FloatPoint.h"
> +#endif

Don't use platform ifdefs to include cross-platform headers, just include the header if it's really needed.

> Source/WebCore/platform/PlatformWheelEvent.h:73
> +    enum PlatformWheelEventPhase {
> +        PlatformWheelEventPhaseNone        = 0,
> +        PlatformWheelEventPhaseStop        = 1 << 0,
> +        PlatformWheelEventPhaseSwipe       = 1 << 1,

Do not line up this enum with the previous one. I wonder if we could reuse the cocoa enum even if we don't use all the flags, or are they too different?

> Source/WebCore/platform/PlatformWheelEvent.h:95
> +            , m_phase(PlatformWheelEventPhaseNone)

Initialize this below when the member is delcared and you don't need to do this in every constructor.

> Source/WebCore/platform/PlatformWheelEvent.h:96
> +            , m_swipeVelocity(FloatPoint())

This is not needed

> Source/WebCore/platform/PlatformWheelEvent.h:120
> +            , m_phase(PlatformWheelEventPhaseNone)
> +            , m_swipeVelocity(FloatPoint())

Ditto.

> Source/WebCore/platform/PlatformWheelEvent.h:182
> +        FloatPoint swipeVelocity() const { return m_swipeVelocity; }

This should return a const reference instead of a copy.

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:112
> +    , m_horizontalData(0, 0, 0, 0)
> +    , m_verticalData(0, 0, 0, 0)

These are not needed.

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:123
> +    return true;

Why is this empty now?

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:142
> +    if (velocity == FloatPoint())

I would do if (!velocity.x() && !velocity.y()) instead of creating a zero FloatPoint just to compare it.

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:53
> +    // FIXME Should this be moved to the initializer list even if it's a bit complex?

I think it's fine here in the body.

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:55
> +        updatePosition(FloatPoint(position));

You should move the position, updatePosition(WTFMove(position));

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:59
> +#if ENABLE(SMOOTH_SCROLLING)
> +        if (m_smoothAnimation)
> +            m_smoothAnimation->setCurrentPosition(position);
> +#endif

Maybe updatePosition() should set the current position on both animations to keep everything in sync whenever the position is updated?

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:111
> +const FloatPoint ScrollAnimatorGtk::computeVelocity()

Since we are returning a new FloatPoint, why making it const?

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:131
> +bool ScrollAnimatorGtk::handleWheelEvent(const PlatformWheelEvent& e)

e -> event

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:136
> +        return e.timestamp() - event.timestamp() > 150;

Use a global static const for this 150 instead of magic number.

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.h:58
> +
> +#if ENABLE(SMOOTH_SCROLLING)
> +    void ensureSmoothScrollingAnimation();
>  #endif
> +    const FloatPoint computeVelocity();
> +
> +    void updatePosition(FloatPoint&&);

Move this below after the virtual methods

> Source/WebCore/platform/gtk/ScrollAnimatorGtk.h:93
> +    std::function<void(FloatPoint&&)> m_positionChangedCallback;

What is this?

> Source/WebKit2/Shared/WebEvent.h:223
> +    WebCore::FloatPoint swipeVelocity() const { return m_swipeVelocity; }

This should return a const reference.

> Source/WebKit2/Shared/WebEventConversion.cpp:162
> +        m_phase = static_cast<WebCore::PlatformWheelEventPhase>(webEvent.phase());

We normally try to avoid casts to convert from/to enums, because it's very fragile in case any of the enum changes. Better use to/from conversion functions.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:195
> +#if GTK_CHECK_VERSION(3, 19, 7)

Since 3.20 was already released, let's check for 3, 20, 0 better instead of using an unstable release number.

> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:199
> +#elif GTK_CHECK_VERSION(3, 3, 18)

I think 3.6 is the minimum GTK+ version we support, so we don't need to check for 3.3 version.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1835
> +#if PLATFORM(GTK)
> +#if GTK_CHECK_VERSION(3, 19, 7)
> +        bool isScrollStopEvent = gdk_event_is_scroll_stop_event(event.nativeEvent());
> +#else
> +        const GdkEventScroll& scroll = event.nativeEvent()->scroll;
> +        bool isScrollStopEvent = scroll.direction == GDK_SCROLL_SMOOTH && scroll.delta_x && scroll.delta_y;
> +#endif
> +        if (m_wheelEventQueue.size() < wheelEventQueueSizeThreshold && !isScrollStopEvent)
> +            return;
> +#else
>          if (m_wheelEventQueue.size() < wheelEventQueueSizeThreshold)
>              return;
> +#endif

We need a better way to handle this than this ifdef mess here :-) We could have a method in the page client, for example, to check if a given wheel event should never be discarded, with this implementation in the GTK+ page client and returning false for all other ports.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160427/7fce1a87/attachment.html>