Bug 208334

Summary: Change HitTestRequestType to an OptionSet
Product: WebKit Reporter: Daniel Bates <dbates>
Component: WebCore Misc.Assignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, cdumez, cfleizach, cgarcia, dino, dmazzoni, eric.carlson, esprehn+autocc, ews-watchlist, fmalita, glenn, gyuyoung.kim, jcraig, jdiggs, jer.noble, kangil.han, kondapallykalyan, pdr, philipj, sabouhallawa, samuel_white, schenney, sergio, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
For the bots
none
For the bots
none
For the bots
none
For the bots
none
For the bots
none
bots
none
Bots
none
Bots
none
Patch
none
To land none

Description Daniel Bates 2020-02-27 12:35:32 PST 
Change HitTestRequestType to an OptionSet for type safety and debugging goodness.
Comment 1 Daniel Bates 2020-02-27 12:36:48 PST 
Created attachment 391899 [details]
For the bots

🧨
Comment 2 Daniel Bates 2020-02-27 12:42:46 PST 
Created attachment 391900 [details]
For the bots
Comment 3 Daniel Bates 2020-02-27 13:10:25 PST 
Created attachment 391902 [details]
For the bots
Comment 4 Daniel Bates 2020-02-27 13:33:55 PST 
Created attachment 391903 [details]
For the bots
Comment 5 Daniel Bates 2020-02-27 13:45:25 PST 
Created attachment 391906 [details]
For the bots
Comment 6 Daniel Bates 2020-02-27 13:54:03 PST 
Created attachment 391908 [details]
bots
Comment 7 Daniel Bates 2020-02-27 13:56:43 PST 
Created attachment 391909 [details]
Bots
Comment 8 Daniel Bates 2020-02-27 14:06:42 PST 
Created attachment 391913 [details]
Bots
Comment 9 Daniel Bates 2020-02-27 14:48:28 PST 
Created attachment 391923 [details]
Patch
Comment 10 Wenson Hsieh 2020-02-27 14:55:51 PST 
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.
Comment 11 Daniel Bates 2020-02-27 15:02:40 PST 
(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?
Comment 12 Daniel Bates 2020-02-27 15:14:42 PST 
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));
Comment 13 Daniel Bates 2020-02-27 15:25:38 PST 
Created attachment 391928 [details]
To land
Comment 14 Daniel Bates 2020-02-27 15:27:05 PST 
Comment on attachment 391928 [details]
To land

Clearing flags on attachment: 391928

Committed r257592: <https://trac.webkit.org/changeset/257592>
Comment 15 Daniel Bates 2020-02-27 15:27:07 PST 
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2020-02-27 15:27:45 PST 
<rdar://problem/59865444>
Comment 17 Carlos Garcia Campos 2020-02-28 01:27:08 PST 
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.
Comment 18 Carlos Garcia Campos 2020-02-28 01:27:22 PST 
Committed r257625: <https://trac.webkit.org/changeset/257625>
Comment 19 Said Abou-Hallawa 2020-02-28 09:18:23 PST 
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 });
Comment 20 Daniel Bates 2020-02-28 18:39:21 PST 
(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
Comment 21 Daniel Bates 2020-02-28 18:46:46 PST 
(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