[Webkit-unassigned] [Bug 153405] [GTK] Implement overlay scrollbars
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun Jan 24 12:07:19 PST 2016
https://bugs.webkit.org/show_bug.cgi?id=153405
--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 269692
--> https://bugs.webkit.org/attachment.cgi?id=269692
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=269692&action=review
I have some comments, but Iâm not sure enough about everything for either a review+ or a review-.
Generally looks good. I am a bit confused by some of the critical details, though.
> Source/WebCore/platform/ScrollAnimatorNone.cpp:54
> +static const double kFrameRate = 60;
> +static const double kTickTime = 1 / kFrameRate;
> +static const double kMinimumTimerInterval = .001;
> +static const double kOverflowScrollbarsAnimationDuration = 1;
> +static const double kOverflowScrollbarsAnimationHideDelay = 2;
This (existing, not new) use of k prefixes doesnât seem right for WebKit coding style.
> Source/WebCore/platform/ScrollAnimatorNone.cpp:631
> + gdouble progress = 1;
All the rest are using double, but this one place uses gdouble. Surprised this compiles at all on non-GTK platforms.
> Source/WebCore/platform/ScrollAnimatorNone.h:53
> explicit ScrollAnimatorNone(ScrollableArea&);
I donât understand how ScrollAnimatorNone is still the right name for this class, now that itâs implementing scrollbar appear/disappear animations. Is this the right way to factor this.
> Source/WebCore/platform/ScrollAnimatorNone.h:188
> + Scrollbar* m_horizontalOverlayScrollbar { nullptr };
> + Scrollbar* m_verticalOverlayScrollbar { nullptr };
> + bool m_overlayScrollbarsLocked { false };
> + Timer m_overlayScrollbarAnimationTimer;
> + double m_overlayScrollbarAnimationSource { 0 };
> + double m_overlayScrollbarAnimationTarget { 0 };
> + double m_overlayScrollbarAnimationCurrent { 0 };
> + double m_overlayScrollbarAnimationStartTime { 0 };
> + double m_overlayScrollbarAnimationEndTime { 0 };
That looks like a lot of unconditional overhead for ports that never use this, which currently includes EFL at least, perhaps also Mac and iOS? Or maybe not? Is there a way to make this cost conditional?
In new code we are trying to use std::chrono for time, rather than plain doubles.
> Source/WebCore/platform/Scrollbar.h:134
> + double opacity() const { return m_opacity; }
> + void setOpacity(double opacity) { m_opacity = opacity; }
A little strange to use double for this. Donât we use float for this kind of thing elsewhere?
--
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/20160124/2616ef35/attachment.html>
More information about the webkit-unassigned
mailing list