[Webkit-unassigned] [Bug 155750] [GTK] Add kinetic scrolling
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Apr 11 00:55:46 PDT 2016
https://bugs.webkit.org/show_bug.cgi?id=155750
--- Comment #5 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 274652
--> https://bugs.webkit.org/attachment.cgi?id=274652
WIP patch
View in context: https://bugs.webkit.org/attachment.cgi?id=274652&action=review
> Source/WebCore/platform/ScrollAnimationKinetic.cpp:35
> +static const double overshootFriction = 20;
This is only about kinetic scrolling for now, this patch doesn't really support overshot animations, so let's keep that out for now.
> Source/WebCore/platform/ScrollAnimationKinetic.cpp:50
> + , m_dx(orientation == HorizontalScrollbar ? step * multiplier : 0)
> + , m_dy(orientation == VerticalScrollbar ? step * multiplier : 0)
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
> Source/WebCore/platform/ScrollAnimationKinetic.cpp:54
> +ScrollAnimationKinetic::KineticScrolling::KineticScrolling ()
Space between KineticScrolling and ()
> Source/WebCore/platform/ScrollAnimationKinetic.cpp:59
> +ScrollAnimationKinetic::KineticScrolling::KineticScrolling (double lower, double upper, double initialPosition, double initialVelocity)
Space between KineticScrolling and (
> Source/WebCore/platform/ScrollAnimationKinetic.cpp:60
> + : m_phase(Phase::Decelerating)
This should be the only phase for now, since we only implement deceleration in this patch.
> Source/WebCore/platform/ScrollAnimationKinetic.cpp:63
> + , m_overshootWidth(0)
Initialize constant values in class declaration.
> Source/WebCore/platform/ScrollAnimationKinetic.cpp:64
> + , m_decelFriction(decelFriction)
Why do we need a member for a constant value?
> Source/WebCore/platform/ScrollAnimationKinetic.cpp:67
> + , m_c1(0)
> + , m_c2(0)
Do not use abbreviations, and move initialization to the class declaration.
> Source/WebCore/platform/ScrollAnimationKinetic.cpp:69
> + , m_t(0)
Ditto.
> Source/WebCore/platform/ScrollAnimationKinetic.cpp:73
> + if(initialPosition < lower)
Missing space between if and (
> Source/WebCore/platform/ScrollAnimationKinetic.cpp:81
> + m_t = 0;
This is already 0
> Source/WebCore/platform/ScrollAnimationKinetic.cpp:83
> + m_position = initialPosition;
> + m_velocity = initialVelocity;
And these also have their own initial values already.
> Source/WebCore/platform/ScrollAnimationKinetic.cpp:87
> +bool ScrollAnimationKinetic::KineticScrolling::tick (double time_delta)
tick (-> tick(
time_delta -> timeDelta
> Source/WebCore/platform/ScrollAnimationKinetic.cpp:93
> + double exp_part = exp (-m_decelFriction * m_t);
exp_part -> whatever it stands for without _
> Source/WebCore/platform/ScrollAnimationKinetic.cpp:150
> +#if USE(REQUEST_ANIMATION_FRAME_TIMER)
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.
> Source/WebCore/platform/ScrollAnimationKinetic.cpp:167
> + m_scrollHistory.append(ScrollEvent(orientation, step, multiplier, monotonicallyIncreasingTime()));
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.
> Source/WebCore/platform/ScrollAnimationKinetic.cpp:169
> + FloatPoint currentPosition = m_position;
This is not actually used, you can use m_position below, since you are not changing it.
> Source/WebCore/platform/ScrollAnimationKinetic.cpp:200
> +void ScrollAnimationKinetic::setCurrentPosition(const FloatPoint& p)
p -> position
> Source/WebCore/platform/ScrollAnimationKinetic.cpp:207
> +void ScrollAnimationKinetic::startDeceleration() {
the { should be in the next line.
> Source/WebCore/platform/ScrollAnimationKinetic.cpp:214
> +void ScrollAnimationKinetic::cancelDeceleration() {
> + stop();
> +}
Do we need this?
> Source/WebCore/platform/ScrollAnimationKinetic.cpp:230
> + double accum_dx = 0;
> + double accum_dy = 0;
Don't use abbreviations nor _
> Source/WebCore/platform/ScrollAnimationKinetic.cpp:231
> + for (auto i = m_scrollHistory.begin(); i != m_scrollHistory.end(); i++) {
for (const auto& scrollEvent : m_scrollHistory)
scrollEvent.dx();
> Source/WebCore/platform/ScrollAnimationKinetic.cpp:236
> + double first = m_scrollHistory.begin()->time();
You can use m_scrollHistory[0].time()
> Source/WebCore/platform/ScrollAnimationKinetic.cpp:260
> + if (!m_scrollHistory.isEmpty())
> + initDeceleration();
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.
> Source/WebCore/platform/ScrollAnimationKinetic.cpp:266
> + if (m_horizontalData.tick(deltaToNextFrame))
Use animateScroll or decelerate instead of tick
> Source/WebCore/platform/ScrollAnimationKinetic.h:31
> +#if ENABLE(SMOOTH_SCROLLING)
This should not depend on SMOOTH_SCROLLING
> Source/WebCore/platform/ScrollAnimationKinetic.h:35
> +#if !ENABLE(REQUEST_ANIMATION_FRAME)
> +#error "SMOOTH_SCROLLING requires REQUEST_ANIMATION_FRAME to be enabled."
> +#endif
Remove this also.
> 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;
> +
> + };
This could probably be a struct
> Source/WebCore/platform/ScrollAnimationKinetic.h:60
> + // KineticScrolling is a port of GtkKineticScrolling as of GTK+ 3.20, mimicking its API and its behavior.
We can mention in the cpp that the whole implementation is based on GTK+ code.
> 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;
> + };
And we don't probably need a class for this either.
> Source/WebCore/platform/ScrollAnimationKinetic.h:109
> + enum class Curve {
> + Linear,
> + Quadratic,
> + Cubic,
> + Quartic,
> + Bounce
> + };
Copy paste leftover, I guess.
> Source/WebCore/platform/ScrollAnimationKinetic.h:112
> + void updateVisibleLengths() override;
Since this is pure virtual, but unimplemented, you can add { } here instead of adding the empty implementation in the cpp file.
> Source/WebCore/platform/ScrollAnimationKinetic.h:115
> + void startDeceleration();
> + void cancelDeceleration();
These could be just start() and stop()
> 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();
We should override handleWheelEvent in ScrollAnimatorGtk and handle this there instead.
> Source/WebCore/platform/ScrollAnimator.h:94
> + virtual void startDeceleration() { }
> + virtual void cancelDeceleration() { }
I don't think this belongs here, ScrollAnimatorGtk can start/stop its animations internally
> Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp:90
> +#if GTK_CHECK_VERSION(3, 19, 7)
> + m_isStop = event->is_stop;
> +#else
> + m_isStop = false;
> +#endif
Hmm, I'm surprised this depends on GTK+ 3.19, kinetic scrolling was supported before, there should be another way.
> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:80
> + if (m_kinetic_animation)
m_kinetic_animation -> m_kineticAnimation
> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:93
> + ensureKineticScrollingAnimation();
We don't need the ensure method here, since this is created unconditionally in the constructor.
> 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);
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)
> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:109
> if (m_animation)
We should rename this now to m_smoothAnimation
> 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
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.
> Source/WebKit2/UIProcess/gtk/GestureController.cpp:92
> + scrollEvent->scroll.is_stop = false;
false -> FALSE
> Source/WebKit2/UIProcess/gtk/GestureController.cpp:111
> + scrollEvent->scroll.is_stop = false;
Ditto.
> Source/WebKit2/UIProcess/gtk/GestureController.cpp:130
> + scrollEvent->scroll.is_stop = true;
Ditto.
> Source/WebKit2/UIProcess/gtk/GestureController.h:74
> + void stopDrag(const GdkEvent*);
Shouldn't we use a swipe gesture instead?
--
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/20160411/e3a94fc3/attachment-0001.html>
More information about the webkit-unassigned
mailing list