Bug 208334 - Change HitTestRequestType to an OptionSet
Summary: Change HitTestRequestType to an OptionSet
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Local Build
Hardware: All All
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-02-27 12:35 PST by Daniel Bates
Modified: 2020-02-28 18:46 PST (History)
26 users (show)

See Also:


Attachments
For the bots (66.64 KB, patch)
2020-02-27 12:36 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
For the bots (67.39 KB, patch)
2020-02-27 12:42 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
For the bots (69.91 KB, patch)
2020-02-27 13:10 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
For the bots (69.91 KB, patch)
2020-02-27 13:33 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
For the bots (71.02 KB, patch)
2020-02-27 13:45 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
bots (72.44 KB, patch)
2020-02-27 13:54 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Bots (74.24 KB, patch)
2020-02-27 13:56 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Bots (75.47 KB, patch)
2020-02-27 14:06 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (85.16 KB, patch)
2020-02-27 14:48 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
To land (85.19 KB, patch)
2020-02-27 15:25 PST, Daniel Bates
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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