Bug 208334 – Change HitTestRequestType to an OptionSet

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Daniel Bates]

Change HitTestRequestType to an OptionSet for type safety and debugging goodness.

[Daniel Bates]

Created attachment 391899 [details]
For the bots

🧨

[Daniel Bates]

Created attachment 391900 [details]
For the bots

[Daniel Bates]

Created attachment 391902 [details]
For the bots

[Daniel Bates]

Created attachment 391903 [details]
For the bots

[Daniel Bates]

Created attachment 391906 [details]
For the bots

[Daniel Bates]

Created attachment 391908 [details]
bots

[Daniel Bates]

Created attachment 391909 [details]
Bots

[Daniel Bates]

Created attachment 391913 [details]
Bots

[Daniel Bates]

Created attachment 391923 [details]
Patch

[Wenson Hsieh]

Comment on attachment 391923 [details]
Patch

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

> Source/WebCore/rendering/HitTestRequest.h:55
> +        ASSERT(!requestType.contains(IncludeAllElementsUnderPoint) || requestType.contains(CollectMultipleElements));

Nit - IMO, this assertion could be made clearer using ASSERT_IMPLIES.

[Daniel Bates]

(In reply to Wenson Hsieh from comment #10)
> Comment on attachment 391923 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391923&action=review
> 
> > Source/WebCore/rendering/HitTestRequest.h:55
> > +        ASSERT(!requestType.contains(IncludeAllElementsUnderPoint) || requestType.contains(CollectMultipleElements));
> 
> Nit - IMO, this assertion could be made clearer using ASSERT_IMPLIES.

how?

[Daniel Bates]

Comment on attachment 391923 [details]
Patch

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

>>> Source/WebCore/rendering/HitTestRequest.h:55
>>> +        ASSERT(!requestType.contains(IncludeAllElementsUnderPoint) || requestType.contains(CollectMultipleElements));
>> 
>> Nit - IMO, this assertion could be made clearer using ASSERT_IMPLIES.
> 
> how?

oh, I got it:

ASSERT_IMPLIES(requestType.contains(IncludeAllElementsUnderPoint), requestType.contains(CollectMultipleElements));

[Daniel Bates]

Created attachment 391928 [details]
To land

[Daniel Bates]

Comment on attachment 391928 [details]
To land

Clearing flags on attachment: 391928

Committed r257592: <https://trac.webkit.org/changeset/257592>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59865444>

[Carlos Garcia Campos]

Comment on attachment 391928 [details]
To land

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

> Source/WebCore/page/EventHandler.cpp:1986
>      if (m_touchPressed)
> -        hitType |= HitTestRequest::Active | HitTestRequest::ReadOnly;
> +        hitType.add(HitTestRequest::Active);
> +        hitType.add(HitTestRequest::ReadOnly);

This if needs braces now that is has two lines in the body.

[Carlos Garcia Campos]

Committed r257625: <https://trac.webkit.org/changeset/257625>

[Said Abou-Hallawa]

Comment on attachment 391928 [details]
To land

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

>> Source/WebCore/page/EventHandler.cpp:1986
>> +        hitType.add(HitTestRequest::ReadOnly);
> 
> This if needs braces now that is has two lines in the body.

Or it can be written in one line like this:

     hitType.add({ HitTestRequest::Active, HitTestRequest::ReadOnly });

[Daniel Bates]

(In reply to Carlos Garcia Campos from comment #17)
> Comment on attachment 391928 [details]
> To land
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391928&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:1986
> >      if (m_touchPressed)
> > -        hitType |= HitTestRequest::Active | HitTestRequest::ReadOnly;
> > +        hitType.add(HitTestRequest::Active);
> > +        hitType.add(HitTestRequest::ReadOnly);
> 
> This if needs braces now that is has two lines in the body.
Yeah, I know, but it was fixed when I noticed. Thanks for fixign

[Daniel Bates]

(In reply to Said Abou-Hallawa from comment #19)
> Comment on attachment 391928 [details]
> To land
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391928&action=review
> 
> >> Source/WebCore/page/EventHandler.cpp:1986
> >> +        hitType.add(HitTestRequest::ReadOnly);
> > 
> > This if needs braces now that is has two lines in the body.
> 
> Or it can be written in one line like this:
> 
>      hitType.add({ HitTestRequest::Active, HitTestRequest::ReadOnly });

Yes, if I thought about doing that and wavered at the last minute...too bad could have avoided the missing brace. Anyway I would do the one line thing today