Bug 234928 – Don't dispatch "focusin" / "focusout" events if there are no listeners

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Alexey Shvayka]

Don't dispatch "focusin" / "focusout" events if there are no listeners, and drop their obsolete variants

[Alexey Shvayka]

Created attachment 448517 [details]
Patch

[Alexey Shvayka]

Created attachment 448518 [details]
Microbenchmark

r287686: 599.7ms
patch: 563.7ms (6% faster)
(50 runs)

[Geoffrey Garen]

Comment on attachment 448517 [details]
Patch

r=me

[Darin Adler]

Comment on attachment 448517 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +          a) Avoids creating and dispatching "focusin" / "focusout" events if it's
> +             guaranteed there are no registered listeners for them. These events are
> +             rather new and not widely popular: according to Chrome stats, only 16%
> +             page loads use them [1], which is even less than `document.all`.

More than 1 out of 7 webpages listens for these events?

That sounds pretty popular to me!

But I guess a small optimization for those other pages is still good.

> Source/WebCore/dom/Element.h:569
> +    void dispatchFocusInEventIfNeeded(RefPtr<Element>&& oldFocusedElement);
> +    void dispatchFocusOutEventIfNeeded(RefPtr<Element>&& newFocusedElement);

I don’t understand why these functions take RefPtr<Element>&&, given that all callers always call copyRef; we’re not using the rvalue reference to optimize anything. The arguments should just be Element*, and the RefPtr can be inside the function.

For some reason that pattern is popular here, but I don’t get it.

[Alexey Shvayka]

(In reply to Geoffrey Garen from comment #3)
> Comment on attachment 448517 [details]
> Patch
> 
> r=me

Thanks Geoff!

(In reply to Darin Adler from comment #4)
> More than 1 out of 7 webpages listens for these events?
> 
> That sounds pretty popular to me!
> 
> But I guess a small optimization for those other pages is still good.

Yeah, 17% is kinda a lot in comparison with most other events in ListenerType, yet focus() currently is relatively slow while being heavily used in some Speedometer2 subtests (but not this path that emits events), hence this patch.

> > Source/WebCore/dom/Element.h:569
> > +    void dispatchFocusInEventIfNeeded(RefPtr<Element>&& oldFocusedElement);
> > +    void dispatchFocusOutEventIfNeeded(RefPtr<Element>&& newFocusedElement);
> 
> I don’t understand why these functions take RefPtr<Element>&&, given that
> all callers always call copyRef; we’re not using the rvalue reference to
> optimize anything. The arguments should just be Element*, and the RefPtr can
> be inside the function.

Since dispatchFocusEvent() / dispatchBlurEvent() have the same signature and their callers also call copyRef(), I will leave a FIXME for this to avoid bloating this patch.

Also, recalling your feedback on another change, I'm landing this patch in two changes: https://bugs.webkit.org/show_bug.cgi?id=234978 removes the obsolete events, while this one adds the optimisation for "focusin" / "focusout" events.

While this adds a bit of churn (especially if we would have to revert, which I doubt), it makes obvious what LayoutTests changes are related to, and makes the conditional dispatch optimization easier to follow as a separate commit.

[Alexey Shvayka]

Created attachment 448649 [details]
Patch for landing

[EWS]

Committed r287802 (245855@main): <https://commits.webkit.org/245855@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/87282027>