1366250 - Flushing in nsFocusManager::CheckIfFocusable shows up significantly in Speedometer.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Olli Pettay [:smaug][[e-mail]]]

Using using m-i, I get 90 in https://sm.duct.io/Full.html, and if I prevent flushing in http://searchfox.org/mozilla-central/rev/24c443a440104dabd9447608bd41b8766e8fc2f5/dom/base/nsFocusManager.cpp#1570
I get 114.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Need to figure out when it is ok to not flush.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: dont_try_to_refocus.diff

https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=52d28eb991cb5b4323e446332b513e9516abf386

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

https://chromium.googlesource.com/chromium/src/+blame/6c5ad90a8e6c3d841df6b7fb01c935879aedf64e/third_party/WebKit/Source/core/dom/Element.cpp#2700

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

FWIW, focus handling is error prone, so I'm a bit surprised if the patch doesn't break some browser chrome tests.

Comment 5

[(no longer active)]

FWIW in bug 792297 you didn't like avoiding flushing in the callee...  IINM the front-end folks have been running into this as well.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

That patch was very very different in bug 792297.

Comment 7

[(no longer active)]

This is what Chomium does: <https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Element.cpp?rcl=b68636d648c64b4312aa050486b74cccd6ceece4&l=2696>

As far as I can tell, their fast paths for not flushing includes:

 * When the element isn't in the DOM tree
 * When the element is already focused, as your patch is doing
 * When the element is in a document that has been unloaded
 * When the element is an iframe that is being unloaded (https://chromium.googlesource.com/chromium/src/+/b16ecc7508fb9da7626dad7708d04b1f745c54e1)

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Yes, I'm not going to add all those checks in this bug, and we already have some of those checks in tree.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Olli Pettay [:smaug] from comment #2)
> Created attachment 8869439 [details] [diff] [review]
> dont_try_to_refocus.diff
> 
> https://treeherder.mozilla.org/perf.html#/
> comparechooser?newProject=try&newRevision=52d28eb991cb5b4323e446332b513e9516a
> bf386
Oops, wrong link.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=52d28eb991cb5b4323e446332b513e9516abf386

And looks like there are quite a few failures.

Comment 10

[Florian Quèze [:florian]]

The (almost meta at this point) bug that blocks all the front-end issues that are directly related to this is bug 1356034. We were hoping Neil would find a 'magic' platform solution for us there, but unfortunately Neil isn't available these days.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e9f5a74e0a439313ee1cad7afc85fddfebaeabf

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

FlushIfNeeded in kind of vague, on purpose. If focus has changed, we've flushed already

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8869534 [details] [diff] [review]
v2

This is perhaps not the most beautiful patch, but rather effective in Speedometer case.

Comment 14

[(no longer active)]

Comment on attachment 8869534 [details] [diff] [review]
v2

Review of attachment 8869534 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below nits addressed.  Also can you please add a useful commit message before landing?  Thanks!

::: dom/base/nsFocusManager.cpp
@@ +179,5 @@
>    "focusmanager.testmode",
>    nullptr
>  };
>  
> +nsFocusManager::nsFocusManager() : mEventHandlingNeedsFlush(false)

Nit: please move this to its own line.

::: dom/base/nsFocusManager.h
@@ +105,5 @@
> +  {
> +    return mFocusedContent == aContent;
> +  }
> +
> +  void FlushIfNeeded(nsIContent* aContent)

Nit: I think FlushBeforeEventHandlingIfNeeded is a better name for this function.

@@ +584,5 @@
>    // and the caller can access the document node, the caller should succeed in
>    // moving focus.
>    nsCOMPtr<nsIDocument> mMouseButtonEventHandlingDocument;
>  
> +  bool mEventHandlingNeedsFlush;

Do you mind adding some comment about how this is used?

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v3

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/26b30ead4fbf
don't flush layout when calling element.focus() on already focused element. Ensure layout is flushed after changing input.type, r=ehsan

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/26b30ead4fbf