Bug 225597

Summary: Make WebCore::HitTestRequest::RequestType an enum class
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: WebCore Misc.Assignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, cdumez, cfleizach, changseok, darin, dino, dmazzoni, eric.carlson, esprehn+autocc, ews-watchlist, fmalita, glenn, gyuyoung.kim, jcraig, jdiggs, jer.noble, kangil.han, kondapallykalyan, pdr, philipj, sabouhallawa, samuel_white, sam, schenney, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

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.