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

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(3 attachments)

(Assignee)

Description

3 months ago
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.
(Assignee)

Comment 1

3 months ago
Need to figure out when it is ok to not flush.
(Assignee)

Updated

3 months ago
Blocks: 1339557
(Assignee)

Updated

3 months ago
Assignee: nobody → bugs
(Assignee)

Comment 2

3 months ago
Created attachment 8869439 [details] [diff] [review]
dont_try_to_refocus.diff

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

Comment 3

3 months ago
https://chromium.googlesource.com/chromium/src/+blame/6c5ad90a8e6c3d841df6b7fb01c935879aedf64e/third_party/WebKit/Source/core/dom/Element.cpp#2700
(Assignee)

Comment 4

3 months ago
FWIW, focus handling is error prone, so I'm a bit surprised if the patch doesn't break some browser chrome tests.
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.
(Assignee)

Comment 6

3 months ago
That patch was very very different in bug 792297.
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)
(Assignee)

Comment 8

3 months ago
Yes, I'm not going to add all those checks in this bug, and we already have some of those checks in tree.
(Assignee)

Comment 9

3 months ago
(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.
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.
See Also: → bug 1356034
(Assignee)

Comment 11

3 months ago
Created attachment 8869534 [details] [diff] [review]
v2

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

Comment 12

3 months ago
FlushIfNeeded in kind of vague, on purpose. If focus has changed, we've flushed already
(Assignee)

Comment 13

3 months ago
Comment on attachment 8869534 [details] [diff] [review]
v2

This is perhaps not the most beautiful patch, but rather effective in Speedometer case.
Attachment #8869534 - Flags: review?(ehsan)
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?
Attachment #8869534 - Flags: review?(ehsan) → review+
(Assignee)

Comment 15

3 months ago
Created attachment 8869585 [details] [diff] [review]
v3

Comment 16

3 months ago
Pushed by opettay@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/26b30ead4fbf
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Updated

3 months ago
Depends on: 1366517

Updated

3 months ago
Blocks: 1366777

Updated

3 months ago
No longer blocks: 1366777
You need to log in before you can comment on or make changes to this bug.