Bug 232646 - Make scroll elasticity an enum class
Summary: Make scroll elasticity an enum class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nikos Mouchtaris
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-11-02 16:48 PDT by Nikos Mouchtaris
Modified: 2021-11-05 11:47 PDT (History)
13 users (show)

See Also:


Attachments
Patch (17.88 KB, patch)
2021-11-02 16:51 PDT, Nikos Mouchtaris
no flags Details | Formatted Diff | Diff
Patch (17.77 KB, patch)
2021-11-03 16:44 PDT, Nikos Mouchtaris
no flags Details | Formatted Diff | Diff
Patch (17.75 KB, patch)
2021-11-03 18:32 PDT, Nikos Mouchtaris
no flags Details | Formatted Diff | Diff
Patch (17.66 KB, patch)
2021-11-04 17:01 PDT, Nikos Mouchtaris
no flags Details | Formatted Diff | Diff
Patch (17.66 KB, patch)
2021-11-04 18:20 PDT, Nikos Mouchtaris
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikos Mouchtaris 2021-11-02 16:48:04 PDT
Make scroll elasticity an enum class
Comment 1 Nikos Mouchtaris 2021-11-02 16:51:57 PDT
Created attachment 443147 [details]
Patch
Comment 2 Simon Fraser (smfr) 2021-11-02 18:46:55 PDT
Comment on attachment 443147 [details]
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)
Comment 3 Nikos Mouchtaris 2021-11-03 16:44:43 PDT
Created attachment 443253 [details]
Patch
Comment 4 Nikos Mouchtaris 2021-11-03 18:32:15 PDT
Created attachment 443264 [details]
Patch
Comment 5 EWS 2021-11-04 16:36:44 PDT
Tools/Scripts/svn-apply failed to apply attachment 443264 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Comment 6 Nikos Mouchtaris 2021-11-04 17:01:08 PDT
Created attachment 443356 [details]
Patch
Comment 7 Nikos Mouchtaris 2021-11-04 18:20:58 PDT
Created attachment 443366 [details]
Patch
Comment 8 EWS 2021-11-05 11:46:29 PDT
Committed r285347 (243908@main): <https://commits.webkit.org/243908@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 443366 [details].
Comment 9 Radar WebKit Bug Importer 2021-11-05 11:47:21 PDT
<rdar://problem/85079767>