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

[bugzilla-daemon at webkit.org]

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

--- Comment #5 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to comment #2)
> Comment on attachment 269692 [details]
> 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-.

Sure, thanks for looking at it!.

> 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.

I agree, I just followed the 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.

Oops, that was not intentional.

> > 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 has always been confusing, even when it only implements smooth scrolling.

> > 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?

My initial idea was to add ScrollAnimationGtk derived from None and add there the scrollbars code, but then I realized that the code is not actually GTK specific, and that EFL port could benefit from it eventually. Since this is only used by EFL and GTK+, I'll ask the EFL guys.

> In new code we are trying to use std::chrono for time, rather than plain
> doubles.

Yes, here it was very convenient to use doubles, because m_overlayScrollbarAnimationCurrent is used directly as the scrollbar opacity value.

> > 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 right, float should be enough and it's what we use in GraphicsContext for example.

-- 
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/50bee1e2/attachment.html>