Closed
Bug 1366250
Opened 8 years ago
Closed 8 years ago
Flushing in nsFocusManager::CheckIfFocusable shows up significantly in Speedometer.
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: smaug, Assigned: smaug)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
Need to figure out when it is ok to not flush.
Assignee | ||
Updated•8 years ago
|
Blocks: Speedometer_V2
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → bugs
Assignee | ||
Comment 2•8 years ago
|
||
Assignee | ||
Comment 3•8 years ago
|
||
Assignee | ||
Comment 4•8 years ago
|
||
FWIW, focus handling is error prone, so I'm a bit surprised if the patch doesn't break some browser chrome tests.
Comment 5•8 years ago
|
||
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•8 years ago
|
||
That patch was very very different in bug 792297.
Comment 7•8 years ago
|
||
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•8 years 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•8 years 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.
Comment 10•8 years ago
|
||
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.
Assignee | ||
Comment 11•8 years ago
|
||
Assignee | ||
Comment 12•8 years ago
|
||
FlushIfNeeded in kind of vague, on purpose. If focus has changed, we've flushed already
Assignee | ||
Comment 13•8 years 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 14•8 years ago
|
||
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•8 years ago
|
||
Comment 16•8 years 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
Comment 17•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•8 years ago
|
Blocks: TimeToFirstPaint_FB
Updated•8 years ago
|
No longer blocks: TimeToFirstPaint_FB
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
Updated•3 years ago
|
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in
before you can comment on or make changes to this bug.
Description
•