[Webkit-unassigned] [Bug 153405] [GTK] Implement overlay scrollbars

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=153405

--- Comment #7 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to comment #3)
> (In reply to comment #2)
> > I have some comments, but I’m not sure enough about everything for either a
> > review+ or a review-.
> 
> Thanks Darin!
> 
> I'll give it a final review later, after actually trying it out. (Haven't
> tested it yet.)
> 
> > This (existing, not new) use of k prefixes doesn’t seem right for WebKit
> > coding style.
> 
> 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.
> 
> > > 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.
> 
> 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.
> 
> > > 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.
> 
> 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?
> 
> > > 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?
> 
> 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?

ScrollAnimatorNone was thought as an intermediate class to inherit from, that's why it's not final and it has protected methods. So, we can just add ScrollAnimatorGtk without adding any news ifdefs

-- 
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/20160125/8fe3b6a2/attachment-0001.html>