<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:mcatanzaro@igalia.com" title="Michael Catanzaro <mcatanzaro@igalia.com>"> <span class="fn">Michael Catanzaro</span></a>
</span> changed
<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>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">CC</td>
<td>
</td>
<td>mcatanzaro@igalia.com, yoon@igalia.com
</td>
</tr></table>
<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#c3">Comment # 3</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:mcatanzaro@igalia.com" title="Michael Catanzaro <mcatanzaro@igalia.com>"> <span class="fn">Michael Catanzaro</span></a>
</span></b>
<pre>(In reply to <a href="show_bug.cgi?id=153405#c2">comment #2</a>)
<span class="quote">> I have some comments, but I’m not sure enough about everything for either a
> review+ or a review-.</span >
Thanks Darin!
I'll give it a final review later, after actually trying it out. (Haven't tested it yet.)
<span class="quote">> This (existing, not new) use of k prefixes doesn’t seem right for WebKit
> coding style.</span >
Yuck, possibly my least-favorite element of Google's otherwise nice code style. Carlos can do a follow-up patch to remove the k prefixes. Best to match the existing style in this patch.
<span class="quote">> > 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.</span >
Good catch. I bet it doesn't compile on non-GLib platforms (EFL does use GLib), but Mac and IOS have separate ScrollAnimator classes so surely they don't use this code. I don't know why it works on Windows, though.
<span class="quote">> > 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.</span >
I was going to make the same complaint. Perhaps ScrollAnimatorDefault? Or is there a distinction in that ScrollAnimatorNone somehow does not animate the scroll (whatever that means)? I presume scrollableArea.scrollAnimatorEnabled() is returning false for GTK?
<span class="quote">> > Source/WebCore/platform/ScrollAnimatorNone.h:188
> 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?</span >
It might be best the way Carlos has it; it would be nice to avoid platform-specific compilation in this file, and I doubt it can be easily split out without causing lots of code duplication. Carlos, what do you think?</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>