Bug 238187 – Add a debug overlay for interaction regions

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tim Horton]

Add a debug overlay for interaction regions

[Tim Horton]

Created attachment 455334 [details]
Patch

[Tim Horton]

Created attachment 455345 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/90964948>

[Wenson Hsieh]

Comment on attachment 455345 [details]
Patch

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

r=me (with a build fix for CMake)

> Source/WebCore/dom/Node.cpp:2519
> +    for (const auto& eventType : eventTypes()) {
> +        if (eventNames().isTouchRelatedEventType(eventType, *this))
> +            return true;
> +    }
> +    return false;

(Could consider using `containsIf` here — might be a tiny bit cleaner)

> Source/WebCore/page/DebugPageOverlays.cpp:265
> +    Vector<Setting> m_settings {

FixedVector?

> Source/WebCore/page/FrameSnapshotting.cpp:189
> +    for (; !!renderer; renderer = renderer->parent()) {

Could we use `ancestorsOfType<RenderElement>` here?

> Source/WebKit/UIProcess/API/Cocoa/WKPreferencesPrivate.h:40
> +    _WKInteractionRegion = 1 << 4,

Does this need availability information too?

[Darin Adler]

Comment on attachment 455345 [details]
Patch

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

>> Source/WebCore/page/DebugPageOverlays.cpp:265
>> +    Vector<Setting> m_settings {
> 
> FixedVector?

std::array?

[Tim Horton]

Created attachment 456285 [details]
Patch

[Tim Horton]

Created attachment 456310 [details]
Patch

[Tim Horton]

(In reply to Darin Adler from comment #5)
> Comment on attachment 455345 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455345&action=review
> 
> >> Source/WebCore/page/DebugPageOverlays.cpp:265
> >> +    Vector<Setting> m_settings {
> > 
> > FixedVector?
> 
> std::array?

FixedVector seems like std::array with less steps? (unless we have another way to construct one from an initializer list without specifying the size?)

(In reply to Wenson Hsieh from comment #4)
> Comment on attachment 455345 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455345&action=review
> 
> r=me (with a build fix for CMake)
> 
> > Source/WebCore/dom/Node.cpp:2519
> > +    for (const auto& eventType : eventTypes()) {
> > +        if (eventNames().isTouchRelatedEventType(eventType, *this))
> > +            return true;
> > +    }
> > +    return false;
> 
> (Could consider using `containsIf` here — might be a tiny bit cleaner)

Way cleaner!

> > Source/WebCore/page/FrameSnapshotting.cpp:189
> > +    for (; !!renderer; renderer = renderer->parent()) {
> 
> Could we use `ancestorsOfType<RenderElement>` here?

lineageOfType, apparently, so that we visit self.

> > Source/WebKit/UIProcess/API/Cocoa/WKPreferencesPrivate.h:40
> > +    _WKInteractionRegion = 1 << 4,
> 
> Does this need availability information too?

Of course!

[Darin Adler]

Comment on attachment 456310 [details]
Patch

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

> Source/WebCore/page/DebugPageOverlays.cpp:266
> +        { "constrain", "Constrain to Regions", true },

Need a bunch of _s here if you don’t want to mess things up for Chris.

[Tim Horton]

(In reply to Darin Adler from comment #9)
> Comment on attachment 456310 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=456310&action=review
> 
> > Source/WebCore/page/DebugPageOverlays.cpp:266
> > +        { "constrain", "Constrain to Regions", true },
> 
> Need a bunch of _s here if you don’t want to mess things up for Chris.

I don't want to mess things up for Chris! I think I see what you're referring to.

[Tim Horton]

Created attachment 456322 [details]
Patch

[EWS]

Committed r292208 (249111@main): <https://commits.webkit.org/249111@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 456322 [details].