Bug 234928

Created attachment 448517 [details] Patch

Created attachment 448518 [details] Microbenchmark r287686: 599.7ms patch: 563.7ms (6% faster) (50 runs)

Comment on attachment 448517 [details] Patch r=me

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.

(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.

Created attachment 448649 [details] Patch for landing

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].

<rdar://problem/87282027>