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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Jan 24 19:05:09 PST 2016


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

Michael Catanzaro <mcatanzaro at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mcatanzaro at igalia.com,
                   |                            |yoon at igalia.com

--- Comment #3 from Michael Catanzaro <mcatanzaro at igalia.com> ---
(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?

-- 
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/2acacc3f/attachment.html>


More information about the webkit-unassigned mailing list