Bug 225597 - Make WebCore::HitTestRequest::RequestType an enum class
Summary: Make WebCore::HitTestRequest::RequestType an enum class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-10 09:33 PDT by Wenson Hsieh
Modified: 2021-05-10 15:06 PDT (History)
27 users (show)

See Also:


Attachments
Patch (87.56 KB, patch)
2021-05-10 09:54 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2021-05-10 09:33:54 PDT
(...for better type safety, and also because we prefer `enum class` over `enum` in modern code)
Comment 1 Wenson Hsieh 2021-05-10 09:54:42 PDT
Created attachment 428178 [details]
Patch
Comment 2 Wenson Hsieh 2021-05-10 14:08:51 PDT
Comment on attachment 428178 [details]
Patch

Thank you for the review!
Comment 3 EWS 2021-05-10 14:31:50 PDT
Committed r277295 (237556@main): <https://commits.webkit.org/237556@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 428178 [details].
Comment 4 Radar WebKit Bug Importer 2021-05-10 14:32:18 PDT
<rdar://problem/77788970>
Comment 5 Darin Adler 2021-05-10 15:00:47 PDT
Comment on attachment 428178 [details]
Patch

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

> Source/WebCore/rendering/HitTestRequest.h:32
> +    enum class Type {

enum class Type : uint16_t?
enum class Type : uint32_t?
Is it better to not specify an underlying type?
Comment 6 Wenson Hsieh 2021-05-10 15:06:02 PDT
(In reply to Darin Adler from comment #5)
> Comment on attachment 428178 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=428178&action=review
> 
> > Source/WebCore/rendering/HitTestRequest.h:32
> > +    enum class Type {
> 
> enum class Type : uint16_t?
> enum class Type : uint32_t?
> Is it better to not specify an underlying type?

Potentially! I don't have strong preferences as to whether or not we should specify an explicit bit width here.

I did this refactoring because I was planning to add another hit test request type in the near future. Currently, there are exactly 16 types, and this new type would require a `uint32_t`.

That said, AFAICT this type flag only represents transient state (usually just stored on the stack when performing a hit test), so I don't think we'll get much of a win from shrinking this enum width.