[webkit-reviews] review granted: [Bug 232646] Make scroll elasticity an enum class : [Attachment 443147] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Nikos Mouchtaris
<nmouchtaris at apple.com>'s request for review:
Bug 232646: Make scroll elasticity an enum class
https://bugs.webkit.org/show_bug.cgi?id=232646

Attachment 443147: Patch

https://bugs.webkit.org/attachment.cgi?id=443147&action=review




--- Comment #2 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 443147
  --> https://bugs.webkit.org/attachment.cgi?id=443147
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=443147&action=review

> Source/WebCore/page/FrameView.cpp:200
> +    ScrollElasticity verticalElasticity = ScrollElasticity::None;
> +    ScrollElasticity horizontalElasticity = ScrollElasticity::None;

You could use 'auto' here since it's now very obvious from the RHS what the
values are.

> Source/WebCore/page/Page.cpp:285
> +    , m_verticalScrollElasticity(ScrollElasticity::Allowed)
> +    , m_horizontalScrollElasticity(ScrollElasticity::Allowed)

No need for this since you have the initializers.

> Source/WebCore/page/Page.h:1043
> -    unsigned m_verticalScrollElasticity : 2; // ScrollElasticity
> -    unsigned m_horizontalScrollElasticity : 2; // ScrollElasticity	 
> +    ScrollElasticity m_verticalScrollElasticity { ScrollElasticity::Allowed
};
> +    ScrollElasticity m_horizontalScrollElasticity {
ScrollElasticity::Allowed };

You've undone some memory saving bitfield stuff here; you'll now use 2 bytes
instead of one. But since there are not many Page objects allocated, that's
probably OK.

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:390
> +    ts.dumpProperty("horizontal scroll elasticity",
static_cast<int>(scrollableAreaParameters.horizontalScrollElasticity));

The ideal way to fix this would be to implement TextStream&
operator<<(TextStream& ts, ScrollElasticity)