Bug 221316 – Optimize PointerCaptureController::elementWasRemoved()

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Simon Fraser (smfr)]

Optimize PointerCaptureController::elementWasRemoved()

[Simon Fraser (smfr)]

Created attachment 419102 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 419102 [details]
Patch

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

> Source/WebCore/page/PointerCaptureController.cpp:178
> +void PointerCaptureController::updateHaveAnyCapturingElement()

Do we need to eagerly update this or can we compute it lazily?
e.g. use Optional<bool> for m_haveAnyCapturingElement
and then haveAnyCapturingElement() inlined like haveAnyCapturingElement() { return m_haveAnyCapturingElement ? *m_haveAnyCapturingElement : haveAnyCapturingElementSlowCase();  }

> Source/WebCore/page/PointerCaptureController.cpp:180
> +    auto haveAnyCapturingElement = [&] {

Do we really need to use lambda here?
I find that setting m_haveAnyCapturingElement = false and breaking when the condition is hit would be easier to read.
Alternatively, can we just use anyOf in Algorithms.h so that:
m_haveAnyCapturingElement = WTF::anyOf(m_activePointerIdsToCapturingData.values(), [&] { ~ })

> Source/WebCore/page/PointerCaptureController.cpp:183
> +            if (capturingData.pendingTargetOverride || capturingData.targetOverride

Can we add a helper function on CapturingData like CapturingData::hasCapturingElement()?

[Simon Fraser (smfr)]

(In reply to Ryosuke Niwa from comment #2)
> Comment on attachment 419102 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419102&action=review
> 
> > Source/WebCore/page/PointerCaptureController.cpp:178
> > +void PointerCaptureController::updateHaveAnyCapturingElement()
> 
> Do we need to eagerly update this or can we compute it lazily?
> e.g. use Optional<bool> for m_haveAnyCapturingElement
> and then haveAnyCapturingElement() inlined like haveAnyCapturingElement() {
> return m_haveAnyCapturingElement ? *m_haveAnyCapturingElement :
> haveAnyCapturingElementSlowCase();  }

I think it's better to do the work eagerly in the functions that start/stop capture, since the no-capture case is massively more common than the capturing case.

> 
> > Source/WebCore/page/PointerCaptureController.cpp:180
> > +    auto haveAnyCapturingElement = [&] {
> 
> Do we really need to use lambda here?
> I find that setting m_haveAnyCapturingElement = false and breaking when the
> condition is hit would be easier to read.
> Alternatively, can we just use anyOf in Algorithms.h so that:
> m_haveAnyCapturingElement =
> WTF::anyOf(m_activePointerIdsToCapturingData.values(), [&] { ~ })

I like the lambda. but I can change it.

> > Source/WebCore/page/PointerCaptureController.cpp:183
> > +            if (capturingData.pendingTargetOverride || capturingData.targetOverride
> 
> Can we add a helper function on CapturingData like
> CapturingData::hasCapturingElement()?

Will do.

[Ryosuke Niwa]

(In reply to Simon Fraser (smfr) from comment #3)
> (In reply to Ryosuke Niwa from comment #2)
> > Comment on attachment 419102 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=419102&action=review
> > 
> > > Source/WebCore/page/PointerCaptureController.cpp:178
> > > +void PointerCaptureController::updateHaveAnyCapturingElement()
> > 
> > Do we need to eagerly update this or can we compute it lazily?
> > e.g. use Optional<bool> for m_haveAnyCapturingElement
> > and then haveAnyCapturingElement() inlined like haveAnyCapturingElement() {
> > return m_haveAnyCapturingElement ? *m_haveAnyCapturingElement :
> > haveAnyCapturingElementSlowCase();  }
> 
> I think it's better to do the work eagerly in the functions that start/stop
> capture, since the no-capture case is massively more common than the
> capturing case.

Ok.

> > > Source/WebCore/page/PointerCaptureController.cpp:180
> > > +    auto haveAnyCapturingElement = [&] {
> > 
> > Do we really need to use lambda here?
> > I find that setting m_haveAnyCapturingElement = false and breaking when the
> > condition is hit would be easier to read.
> > Alternatively, can we just use anyOf in Algorithms.h so that:
> > m_haveAnyCapturingElement =
> > WTF::anyOf(m_activePointerIdsToCapturingData.values(), [&] { ~ })
> 
> I like the lambda. but I can change it.

Please use anyOf then.

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/272331/webkit

[Radar WebKit Bug Importer]

<rdar://problem/73939380>