Change HitTestRequestType to an OptionSet for type safety and debugging goodness.
Created attachment 391899 [details] For the bots ๐งจ
Created attachment 391900 [details] For the bots
Created attachment 391902 [details] For the bots
Created attachment 391903 [details] For the bots
Created attachment 391906 [details] For the bots
Created attachment 391908 [details] bots
Created attachment 391909 [details] Bots
Created attachment 391913 [details] Bots
Created attachment 391923 [details] Patch
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.
(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 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));
Created attachment 391928 [details] To land
Comment on attachment 391928 [details] To land Clearing flags on attachment: 391928 Committed r257592: <https://trac.webkit.org/changeset/257592>
All reviewed patches have been landed. Closing bug.
<rdar://problem/59865444>
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.
Committed r257625: <https://trac.webkit.org/changeset/257625>
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 });
(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
(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