<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Add kinetic scrolling"
href="https://bugs.webkit.org/show_bug.cgi?id=155750#c5">Comment # 5</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Add kinetic scrolling"
href="https://bugs.webkit.org/show_bug.cgi?id=155750">bug 155750</a>
from <span class="vcard"><a class="email" href="mailto:cgarcia@igalia.com" title="Carlos Garcia Campos <cgarcia@igalia.com>"> <span class="fn">Carlos Garcia Campos</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=274652&action=diff" name="attach_274652" title="WIP patch">attachment 274652</a> <a href="attachment.cgi?id=274652&action=edit" title="WIP patch">[details]</a></span>
WIP patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=274652&action=review">https://bugs.webkit.org/attachment.cgi?id=274652&action=review</a>
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.cpp:35
> +static const double overshootFriction = 20;</span >
This is only about kinetic scrolling for now, this patch doesn't really support overshot animations, so let's keep that out for now.
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.cpp:50
> + , m_dx(orientation == HorizontalScrollbar ? step * multiplier : 0)
> + , m_dy(orientation == VerticalScrollbar ? step * multiplier : 0)</span >
Do not use abbreviations for variables, use m_delatX, m_deltaY for example instead of m_dx, m_dy. Or even better FloatSize m_delta
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.cpp:54
> +ScrollAnimationKinetic::KineticScrolling::KineticScrolling ()</span >
Space between KineticScrolling and ()
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.cpp:59
> +ScrollAnimationKinetic::KineticScrolling::KineticScrolling (double lower, double upper, double initialPosition, double initialVelocity)</span >
Space between KineticScrolling and (
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.cpp:60
> + : m_phase(Phase::Decelerating)</span >
This should be the only phase for now, since we only implement deceleration in this patch.
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.cpp:63
> + , m_overshootWidth(0)</span >
Initialize constant values in class declaration.
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.cpp:64
> + , m_decelFriction(decelFriction)</span >
Why do we need a member for a constant value?
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.cpp:67
> + , m_c1(0)
> + , m_c2(0)</span >
Do not use abbreviations, and move initialization to the class declaration.
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.cpp:69
> + , m_t(0)</span >
Ditto.
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.cpp:73
> + if(initialPosition < lower)</span >
Missing space between if and (
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.cpp:81
> + m_t = 0;</span >
This is already 0
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.cpp:83
> + m_position = initialPosition;
> + m_velocity = initialVelocity;</span >
And these also have their own initial values already.
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.cpp:87
> +bool ScrollAnimationKinetic::KineticScrolling::tick (double time_delta)</span >
tick (-> tick(
time_delta -> timeDelta
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.cpp:93
> + double exp_part = exp (-m_decelFriction * m_t);</span >
exp_part -> whatever it stands for without _
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.cpp:150
> +#if USE(REQUEST_ANIMATION_FRAME_TIMER)</span >
Since this will be supported only by GTK+ for now, and you are not going to test the !REQUEST_ANIMATION_FRAME_TIMER case, let's get rid of this, and use always a timer.
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.cpp:167
> + m_scrollHistory.append(ScrollEvent(orientation, step, multiplier, monotonicallyIncreasingTime()));</span >
This can grow indefinitely, it seems GTK+ removes events older than a constant threshold (150ms). ScrollEvent doesn't really need both step and multiplier, so you can pass the delta directly, and the current time can also be obtained by the ScrollEvent constructor instead of passing it. But I don't think it's the current time what we want, but the event time.
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.cpp:169
> + FloatPoint currentPosition = m_position;</span >
This is not actually used, you can use m_position below, since you are not changing it.
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.cpp:200
> +void ScrollAnimationKinetic::setCurrentPosition(const FloatPoint& p)</span >
p -> position
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.cpp:207
> +void ScrollAnimationKinetic::startDeceleration() {</span >
the { should be in the next line.
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.cpp:214
> +void ScrollAnimationKinetic::cancelDeceleration() {
> + stop();
> +}</span >
Do we need this?
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.cpp:230
> + double accum_dx = 0;
> + double accum_dy = 0;</span >
Don't use abbreviations nor _
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.cpp:231
> + for (auto i = m_scrollHistory.begin(); i != m_scrollHistory.end(); i++) {</span >
for (const auto& scrollEvent : m_scrollHistory)
scrollEvent.dx();
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.cpp:236
> + double first = m_scrollHistory.begin()->time();</span >
You can use m_scrollHistory[0].time()
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.cpp:260
> + if (!m_scrollHistory.isEmpty())
> + initDeceleration();</span >
initDeceleration already checks if scroll history is empty. I don't understand why the animation fire is starting the deceleration, I think it should be the other way around, startDeceleration should schedule the timer animation.
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.cpp:266
> + if (m_horizontalData.tick(deltaToNextFrame))</span >
Use animateScroll or decelerate instead of tick
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.h:31
> +#if ENABLE(SMOOTH_SCROLLING)</span >
This should not depend on SMOOTH_SCROLLING
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.h:35
> +#if !ENABLE(REQUEST_ANIMATION_FRAME)
> +#error "SMOOTH_SCROLLING requires REQUEST_ANIMATION_FRAME to be enabled."
> +#endif</span >
Remove this also.
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.h:58
> + class ScrollEvent {
> + public:
> + ScrollEvent(ScrollbarOrientation, float step, float multiplier, double time);
> + double time() const { return m_time; }
> + double dx() const { return m_dx; }
> + double dy() const { return m_dy; }
> +
> + private:
> + double m_time;
> + double m_dx;
> + double m_dy;
> +
> + };</span >
This could probably be a struct
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.h:60
> + // KineticScrolling is a port of GtkKineticScrolling as of GTK+ 3.20, mimicking its API and its behavior.</span >
We can mention in the cpp that the whole implementation is based on GTK+ code.
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.h:97
> + Phase m_phase;
> + double m_lower;
> + double m_upper;
> + double m_overshootWidth;
> + double m_decelFriction;
> + double m_overshootFriction;
> +
> + double m_c1;
> + double m_c2;
> + double m_equilibriumPosition;
> +
> + double m_t;
> + double m_position;
> + double m_velocity;
> + };</span >
And we don't probably need a class for this either.
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.h:109
> + enum class Curve {
> + Linear,
> + Quadratic,
> + Cubic,
> + Quartic,
> + Bounce
> + };</span >
Copy paste leftover, I guess.
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.h:112
> + void updateVisibleLengths() override;</span >
Since this is pure virtual, but unimplemented, you can add { } here instead of adding the empty implementation in the cpp file.
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.h:115
> + void startDeceleration();
> + void cancelDeceleration();</span >
These could be just start() and stop()
<span class="quote">> Source/WebCore/platform/ScrollAnimator.cpp:129
> +#elif PLATFORM(GTK)
> + // GDK scroll stop events are used to notify that the scrolling came to an end and that deceleration can be triggered.
> + if (e.isStop()) {
> + startDeceleration();
> + return true;
> + }
> + else
> + cancelDeceleration();</span >
We should override handleWheelEvent in ScrollAnimatorGtk and handle this there instead.
<span class="quote">> Source/WebCore/platform/ScrollAnimator.h:94
> + virtual void startDeceleration() { }
> + virtual void cancelDeceleration() { }</span >
I don't think this belongs here, ScrollAnimatorGtk can start/stop its animations internally
<span class="quote">> Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp:90
> +#if GTK_CHECK_VERSION(3, 19, 7)
> + m_isStop = event->is_stop;
> +#else
> + m_isStop = false;
> +#endif</span >
Hmm, I'm surprised this depends on GTK+ 3.19, kinetic scrolling was supported before, there should be another way.
<span class="quote">> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:80
> + if (m_kinetic_animation)</span >
m_kinetic_animation -> m_kineticAnimation
<span class="quote">> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:93
> + ensureKineticScrollingAnimation();</span >
We don't need the ensure method here, since this is created unconditionally in the constructor.
<span class="quote">> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:97
> + m_kinetic_animation->scroll(orientation, granularity, step, multiplier);
> +
> if (!m_scrollableArea.scrollAnimatorEnabled() || granularity == ScrollByPrecisePixel)
> return ScrollAnimator::scroll(orientation, granularity, step, multiplier);</span >
How does smooth and kinetic work together? I guess we should check if there's a deceleration in progress and only do the smooth when there isn't (if enabled, of course)
<span class="quote">> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:109
> if (m_animation)</span >
We should rename this now to m_smoothAnimation
<span class="quote">> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:185
> +#if GTK_CHECK_VERSION(3, 19, 7)
> + bool isStop = gdk_event_is_scroll_stop_event (event);
> +#else
> + bool isStop = false;
> +#endif</span >
Maybe we can guess this, if direction is smooth and delta x = delta y = 0 then it's stop. Not sure that's always true, though.
<span class="quote">> Source/WebKit2/UIProcess/gtk/GestureController.cpp:92
> + scrollEvent->scroll.is_stop = false;</span >
false -> FALSE
<span class="quote">> Source/WebKit2/UIProcess/gtk/GestureController.cpp:111
> + scrollEvent->scroll.is_stop = false;</span >
Ditto.
<span class="quote">> Source/WebKit2/UIProcess/gtk/GestureController.cpp:130
> + scrollEvent->scroll.is_stop = true;</span >
Ditto.
<span class="quote">> Source/WebKit2/UIProcess/gtk/GestureController.h:74
> + void stopDrag(const GdkEvent*);</span >
Shouldn't we use a swipe gesture instead?</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>