<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Implement overlay scrollbars"
href="https://bugs.webkit.org/show_bug.cgi?id=153405#c2">Comment # 2</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Implement overlay scrollbars"
href="https://bugs.webkit.org/show_bug.cgi?id=153405">bug 153405</a>
from <span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=269692&action=diff" name="attach_269692" title="Patch">attachment 269692</a> <a href="attachment.cgi?id=269692&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=269692&action=review">https://bugs.webkit.org/attachment.cgi?id=269692&action=review</a>
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.
<span class="quote">> 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;</span >
This (existing, not new) use of k prefixes doesn’t seem right for WebKit coding style.
<span class="quote">> Source/WebCore/platform/ScrollAnimatorNone.cpp:631
> + gdouble progress = 1;</span >
All the rest are using double, but this one place uses gdouble. Surprised this compiles at all on non-GTK platforms.
<span class="quote">> Source/WebCore/platform/ScrollAnimatorNone.h:53
> explicit ScrollAnimatorNone(ScrollableArea&);</span >
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.
<span class="quote">> 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 };</span >
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.
<span class="quote">> Source/WebCore/platform/Scrollbar.h:134
> + double opacity() const { return m_opacity; }
> + void setOpacity(double opacity) { m_opacity = opacity; }</span >
A little strange to use double for this. Don’t we use float for this kind of thing elsewhere?</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>