Since drawing a super large dimmed black translucent layer on top of content is costly and yields a jarring UX, we'll need to be pragmatic about when to enable this feature. The current idea is to not dim the page when it's super-sized (IOW: large enough to cause 'excessive' jank) and show borders around the rectangles of found occurrences, when 'Highlight All' is enabled. We can already differentiate the look & feel depending of the luminosity of the page, this we'll have to take of that.

Comment on attachment 8787158 [details] Bug 1290914 - support Element.animate() on AnonymousContent nodes through the AnonymousContent.setAnimationForElement() method. Requesting feedback for now, because I'm not sure about not showing the highlights at all... I can also just use the 'old' method with the magenta colored ranges when the doc is too large. That'd be easy. I think that the patch without the large document detection part is worth landing in itself. However, blocking this is a bigger issue: This patch definitely streamlines the chrome-process --> content-process traffic and makes the entire findbar experience snappier than I was able to accomplish before. So that's a big win regardless. I can now also fully control the timing of when the iterator kicks in to temporary saturate the CPU time of the content process with the `findbar.iteratorTimeout` pref. Set it to 10000 and you know for sure that the iterator won't be interfering with the rendering perf. All good so far and with the majority of websites this results in rather fantastic performance, IMHO. Enough to make me consider lowering `kModalHighlightRepaintFreqMs` to increase the frame-rate of the modal highlights. The trouble is with the HTML spec page. Or any other huge-ass website of > 1,000,000px in size. When I look at Gecko Profiler runs I see such an enormous amount of time spent to flush layout when I insert a small DIV using `document.insertAnonymousContent()`, that I can only think of that being the culprit. But to see if I'm on the right track here, can I get help from people who are familiar with render/ DOM/ layout profiling? I'm flagging Ehsan - as reviewer of the AnonymousContent API - and Markus, who I've seen doing perf work before. But please, forward a request to someone else if you think he or she might be a better fit to answer this question. Thanks!!

(In reply to Mike de Boer [:mikedeboer] from comment #0) > Since drawing a super large dimmed black translucent layer on top of content > is costly That shouldn't be the case. If it's actually too slow, seems like that is a problem that we should fix, since normal web pages can probably trigger the same underlying cause. Can you please grab a profile that shows the slow parts so that we can investigate what's going on? I don't think we should work around this code if this is a performance issue unless fixing that is not possible...

CJ (:cjku) might be able to help you debug the slowness.

Here's the link to the most recent profile I created & saved: https://cleopatra.io/#report=9138b1b081fbe9f6fb5ad2cc968d2493f06b57f0

This profile suggests that painting is not a problem on that page. The long pause is restyling (either because you modified the DOM in some expensive way or you because you added a CSS rule, maybe?), then a bit of reflow, and after that there's only 19ms of painting. The profile also shows that your range iterator JS does not unwind the stack before it processes the "task" for the next iteration, so the stack becomes pretty huge. That might not be a problem, but fixing that would make profiles easier to read. By the way, the profile only has pseudostacks, not complete C++ stacks. You'll want to check the "Stackwalk" checkbox in the profiler panel, or, if that's not available, add "ac_add_options --enable-profiling" to your mozconfig.

(In reply to Markus Stange [:mstange] from comment #6) > This profile suggests that painting is not a problem on that page. The long > pause is restyling (either because you modified the DOM in some expensive > way or you because you added a CSS rule, maybe?), then a bit of reflow, and > after that there's only 19ms of painting. > The profile also shows that your range iterator JS does not unwind the stack > before it processes the "task" for the next iteration, so the stack becomes > pretty huge. That might not be a problem, but fixing that would make > profiles easier to read. Maybe a stupid question, but how do I go about that? Try to keep references scoped to the loop only? > By the way, the profile only has pseudostacks, not complete C++ stacks. > You'll want to check the "Stackwalk" checkbox in the profiler panel, or, if > that's not available, add "ac_add_options --enable-profiling" to your > mozconfig. I just rebuilt with that flag enabled. I will post a profile link soon!

(In reply to Markus Stange [:mstange] from comment #6) > This profile suggests that painting is not a problem on that page. The long > pause is restyling (either because you modified the DOM in some expensive > way or you because you added a CSS rule, maybe?), then a bit of reflow, and > after that there's only 19ms of painting. I indeed insert a whole new stylesheet the very first time it displays the modal UI. New profile: https://cleopatra.io/#report=12a2baff5efbbbbc5f6a3c5323fa9860543527a8

(In reply to Mike de Boer [:mikedeboer] from comment #7) > (In reply to Markus Stange [:mstange] from comment #6) > > This profile suggests that painting is not a problem on that page. The long > > pause is restyling (either because you modified the DOM in some expensive > > way or you because you added a CSS rule, maybe?), then a bit of reflow, and > > after that there's only 19ms of painting. > > The profile also shows that your range iterator JS does not unwind the stack > > before it processes the "task" for the next iteration, so the stack becomes > > pretty huge. That might not be a problem, but fixing that would make > > profiles easier to read. > > Maybe a stupid question, but how do I go about that? Not sure yet, I'll need to look at the code more closely. (In reply to Mike de Boer [:mikedeboer] from comment #8) > (In reply to Markus Stange [:mstange] from comment #6) > > This profile suggests that painting is not a problem on that page. The long > > pause is restyling (either because you modified the DOM in some expensive > > way or you because you added a CSS rule, maybe?), then a bit of reflow, and > > after that there's only 19ms of painting. > > I indeed insert a whole new stylesheet the very first time it displays the > modal UI. OK, I suggest filing a new bug about the slow restyle and asking for help from heycam. Either there is a way to restrict the CSS selectors (or even the whole stylesheet, maybe using <style scoped>?) to just the inspector overlay, or such an optimization needs to be added, or you may need to use inline styles. > New profile: > https://cleopatra.io/#report=12a2baff5efbbbbc5f6a3c5323fa9860543527a8 Thanks, this confirms the earlier findings with a bit more detail. I don't see anything else notable in the profile. (Other than the fact that nsIFrame::ClearInvalidationStateBits() can take 50ms on a huge page, I didn't know that!)

(In reply to Markus Stange [:mstange] from comment #9) > OK, I suggest filing a new bug about the slow restyle and asking for help > from heycam. Either there is a way to restrict the CSS selectors (or even > the whole stylesheet, maybe using <style scoped>?) to just the inspector > overlay, or such an optimization needs to be added, or you may need to use > inline styles. Ah! This made me think of https://bugzilla.mozilla.org/show_bug.cgi?id=933125... and highly likely to offer improvement! Since we already have that bug at the ready, why not ask Cameron here? Cameron, do you think - when you read this profile - that allowing scoped stylesheets for anonymous content would result in a big performance improvement here? Thanks for your time!

But now I also see that @keyframes is not supported in scoped stylesheets (bug 830056), which is a bummer...

(In reply to Mike de Boer [:mikedeboer] from comment #10) > Cameron, do you think - when you read this profile - that allowing scoped > stylesheets for anonymous content would result in a big performance > improvement here? Thanks for your time! To make Cameron's job easier, can you link to the stylesheet that you're adding, to the code that's adding it, and to the code that's inserting the anonymous elements? (In reply to Mike de Boer [:mikedeboer] from comment #11) > But now I also see that @keyframes is not supported in scoped stylesheets > (bug 830056), which is a bummer... Element.animate might be a reasonable replacement.

(In reply to Markus Stange [:mstange] from comment #12) > Element.animate might be a reasonable replacement. Awesome! I'm going to try inline styles first and report back here with my findings!

OK, I have implemented this locally and now see a rather impressive speedup, unless my eyes are deceiving me! New profile, with all bells & whistles turned on: https://cleopatra.io/#report=1f02edf628c8467ca170d2ebd33fbe8f8a7dd172 Markus, I don't think I can speed up nsFind any more, but is there anything else you see?

Hmm, there's still a long restyle, apparently triggered by nsPresContext::MediaFeatureValuesChanged, maybe due to the viewport being resized because of the findbar. Maybe Cameron knows how to avoid the cost from that one.

And it looks like you were profiling a debug build. For example, I see nsCOMPtr<nsIDOMNode>::Assert_NoQueryNeeded() in the profile. You should always profile optimized builds.

Ehsan, I'm trying to add Animation API support to the AnonymousContent API, but this code doesn't compile. Normally I would be able to figure out what's wrong from the errors thrown by the compiler, but here I'm getting: 1:23.51 In file included from /Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin15.4.0/dom/presentation/Unified_cpp_dom_presentation1.cpp:11: 1:23.51 In file included from /Users/mikedeboer/Projects/fx-team/dom/presentation/ipc/PresentationBuilderChild.cpp:9: 1:23.51 In file included from /Users/mikedeboer/Projects/fx-team/dom/base/nsGlobalWindow.h:42: 1:23.51 In file included from /Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin15.4.0/dist/include/mozilla/dom/UnionTypes.h:4: 1:23.52 In file included from /Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin15.4.0/dist/include/mozilla/dom/AnimatableBinding.h:6: 1:23.52 /Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin15.4.0/dist/include/mozilla/dom/AnimationEffectReadOnlyBinding.h:69:36: error: field has incomplete type 'mozilla::dom::OwningUnrestrictedDoubleOrString' 1:23.52 OwningUnrestrictedDoubleOrString mDuration; 1:23.52 ^ 1:23.52 /Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin15.4.0/dist/include/mozilla/dom/AnimationEffectReadOnlyBinding.h:22:7: note: forward declaration of 'mozilla::dom::OwningUnrestrictedDoubleOrString' 1:23.52 class OwningUnrestrictedDoubleOrString; 1:23.52 ^ 1:23.55 In file included from /Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin15.4.0/dom/presentation/Unified_cpp_dom_presentation1.cpp:11: 1:23.55 In file included from /Users/mikedeboer/Projects/fx-team/dom/presentation/ipc/PresentationBuilderChild.cpp:9: 1:23.55 In file included from /Users/mikedeboer/Projects/fx-team/dom/base/nsGlobalWindow.h:42: 1:23.55 In file included from /Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin15.4.0/dist/include/mozilla/dom/UnionTypes.h:4: 1:23.55 In file included from /Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin15.4.0/dist/include/mozilla/dom/AnimatableBinding.h:8: 1:23.55 /Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin15.4.0/dist/include/mozilla/dom/KeyframeEffectBinding.h:613:12: error: no viable conversion from returned value of type 'const mozilla::dom::binding_detail::FastKeyframeEffectOptions' to function return type 'const mozilla::dom::KeyframeEffectOptions' 1:23.55 return mValue.mKeyframeEffectOptions.Value(); 1:23.55 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1:23.90 In file included from /Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin15.4.0/dom/presentation/Unified_cpp_dom_presentation0.cpp:101: 1:23.90 In file included from /Users/mikedeboer/Projects/fx-team/dom/presentation/PresentationService.cpp:12: 1:23.90 In file included from /Users/mikedeboer/Projects/fx-team/dom/base/nsGlobalWindow.h:42: 1:23.90 In file included from /Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin15.4.0/dist/include/mozilla/dom/UnionTypes.h:4: 1:23.90 In file included from /Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin15.4.0/dist/include/mozilla/dom/AnimatableBinding.h:6: 1:23.90 /Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin15.4.0/dist/include/mozilla/dom/AnimationEffectReadOnlyBinding.h:69:36: error: field has incomplete type 'mozilla::dom::OwningUnrestrictedDoubleOrString' 1:23.90 OwningUnrestrictedDoubleOrString mDuration; 1:23.90 ^ 1:23.90 /Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin15.4.0/dist/include/mozilla/dom/AnimationEffectReadOnlyBinding.h:22:7: note: forward declaration of 'mozilla::dom::OwningUnrestrictedDoubleOrString' 1:23.90 class OwningUnrestrictedDoubleOrString; 1:23.90 ^ 1:23.94 In file included from /Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin15.4.0/dom/presentation/Unified_cpp_dom_presentation0.cpp:101: 1:23.94 In file included from /Users/mikedeboer/Projects/fx-team/dom/presentation/PresentationService.cpp:12: 1:23.94 In file included from /Users/mikedeboer/Projects/fx-team/dom/base/nsGlobalWindow.h:42: 1:23.94 In file included from /Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin15.4.0/dist/include/mozilla/dom/UnionTypes.h:4: 1:23.94 In file included from /Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin15.4.0/dist/include/mozilla/dom/AnimatableBinding.h:8: 1:23.94 /Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin15.4.0/dist/include/mozilla/dom/KeyframeEffectBinding.h:613:12: error: no viable conversion from returned value of type 'const mozilla::dom::binding_detail::FastKeyframeEffectOptions' to function return type 'const mozilla::dom::KeyframeEffectOptions' 1:23.95 return mValue.mKeyframeEffectOptions.Value(); 1:23.95 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1:24.03 In file included from /Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin15.4.0/dom/presentation/Unified_cpp_dom_presentation1.cpp:11: 1:24.03 In file included from /Users/mikedeboer/Projects/fx-team/dom/presentation/ipc/PresentationBuilderChild.cpp:9: 1:24.04 In file included from /Users/mikedeboer/Projects/fx-team/dom/base/nsGlobalWindow.h:42: 1:24.04 /Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin15.4.0/dist/include/mozilla/dom/UnionTypes.h:7882:12: error: no viable conversion from returned value of type 'const mozilla::dom::binding_detail::FastKeyframeAnimationOptions' to function return type 'const mozilla::dom::KeyframeAnimationOptions' 1:24.04 return mValue.mKeyframeAnimationOptions.Value(); 1:24.04 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ...which I can't understand (yet). Can you help?

Comment on attachment 8788422 [details] [diff] [review] Patch: add element animation API to AnonymousContent This appears to be caused by bug 995352, which I found out with the awesome help from smaug and peterv! I'm now trying solutions that will compile.

The simplest solution is to put the offending union in a webidl file of its own, have a typedef for it, and use that typedef in other webidl files. That will put the union in its own header, instead of UnionTypes.h, and everything should work.

Comment on attachment 8787158 [details] Bug 1290914 - support Element.animate() on AnonymousContent nodes through the AnonymousContent.setAnimationForElement() method. https://reviewboard.mozilla.org/r/76014/#review75122 The commit message is wrong. There is no "chromeDocument.setAnimationForElement" thing. Please fix the commit message to accurately reflect where the API is being added. ::: dom/webidl/KeyFrameAnimationOptions.webidl:13 (Diff revision 3) > + * > + * Copyright © 2014 W3C® (MIT, ERCIM, Keio), All Rights Reserved. W3C > + * liability, trademark and document use rules apply. > + */ > + > +typedef (unrestricted double or KeyframeAnimationOptions) UnrestrictedDoubleOrKeyframeAnimationOptions; Please add a comment explaining why this is off in a file by itself. r=me with the above fixed.

(In reply to Markus Stange [:mstange] from comment #15) > Hmm, there's still a long restyle, apparently triggered by > nsPresContext::MediaFeatureValuesChanged, maybe due to the viewport being > resized because of the findbar. This is an existing problem and does not have much to do with the find bar, so I filed bug 1300879 about it.

Gregory, I added a .webidl file and I'm getting the following build failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=75aeada8c068&selectedJob=27039487 This is primarily Linux and TaskCluster OSX. Is this an infra problem or do I need to touch the CLOBBER file? I thought try builds always clobbered, so what's going on here? If you don't know the answer, do you know who might? Thanks!

Case sensitive filesystem. "Keyframe" versus "KeyFrame."

Comment on attachment 8788423 [details] Bug 1290914 - move the matches count and highlight-all request from the findbar binding to the JS module in the content process, so it's closer to the metal. https://reviewboard.mozilla.org/r/76930/#review75292 ::: toolkit/modules/FinderIterator.jsm:270 (Diff revision 3) > */ > _notifyListeners(callback, params, listeners = this._listeners.keys()) { > callback = "onIterator" + callback.charAt(0).toUpperCase() + callback.substr(1); > for (let listener of listeners) { > try { > - listener[callback](params); > + listener[callback](params) Please add the semicolon back, and can you check that these files are being run through eslint?

Comment on attachment 8788524 [details] Bug 1290914 - use inline styles for the modal highlighting anonymous content nodes to dramatically improve performance when find in page is used on large documents. https://reviewboard.mozilla.org/r/76988/#review75296 ::: toolkit/modules/FinderHighlighter.jsm:434 (Diff revision 2) > } > > outlineNode = dict.modalHighlightOutline; > - try { > - outlineNode.removeAttributeForElement(kModalOutlineId, "grow"); > - } catch (ex) {} > + if (dict.anim) > + dict.anim.finish(); > + dict.anim = outlineNode.setAnimationForElement(kModalOutlineId, Please use "animate" instead of "anim"

(In reply to Gregory Szorc [:gps] from comment #32) > Case sensitive filesystem. "Keyframe" versus "KeyFrame." Facepalm! Thanks Greg :)

Pushed by mdeboer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a71b7098d0c6 support Element.animate() on AnonymousContent nodes through the AnonymousContent.setAnimationForElement() method. r=bz https://hg.mozilla.org/integration/autoland/rev/7158434e674a move the matches count and highlight-all request from the findbar binding to the JS module in the content process, so it's closer to the metal. r=jaws https://hg.mozilla.org/integration/autoland/rev/e6f59b598c17 use inline styles for the modal highlighting anonymous content nodes to dramatically improve performance when find in page is used on large documents. r=jaws

Backed out for failure in test_findbar.xul: https://hg.mozilla.org/integration/autoland/rev/8bb0e9ca7a693e5ea2cfba1d5f827d5631367616 https://hg.mozilla.org/integration/autoland/rev/3e76c2fd3b99308b0c066f18342d744390bc2329 https://hg.mozilla.org/integration/autoland/rev/85e02f8c2b692c2f241783d604e67ec15a3a785f Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=e6f59b598c177e3b2080f0d33e59de86a318169e Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=3203067&repo=autoland 09:16:56 INFO - 116 INFO TEST-PASS | toolkit/content/tests/chrome/test_findbar.xul | Total amount of matches should be 10 for 't' 09:16:56 INFO - 117 INFO TEST-UNEXPECTED-FAIL | toolkit/content/tests/chrome/test_findbar.xul | Currently highlighted match should be at 3 for 'te' - got "4", expected "3" 09:16:56 INFO - SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:268:5 09:16:56 INFO - assertMatches@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/findbar_window.xul:546:9 09:16:56 INFO - testFindCountUI@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/findbar_window.xul:563:11 09:16:56 INFO - TaskImpl_run@resource://gre/modules/Task.jsm:319:40

https://hg.mozilla.org/integration/fx-team/rev/9cda5c032561859624fc4d7d2dafd5f980b7b0d4 Bug 1290914 - support Element.animate() on AnonymousContent nodes through the AnonymousContent.setAnimationForElement() method. r=bz https://hg.mozilla.org/integration/fx-team/rev/14aa33f8babc5382a23025126eb2eaac0d94d0a1 Bug 1290914 - move the matches count and highlight-all request from the findbar binding to the JS module in the content process, so it's closer to the metal. r=jaws https://hg.mozilla.org/integration/fx-team/rev/a05091172da102a71a89a6602385b2114574abfd Bug 1290914 - use inline styles for the modal highlighting anonymous content nodes to dramatically improve performance when find in page is used on large documents. r=jaws

https://hg.mozilla.org/mozilla-central/rev/9cda5c032561 https://hg.mozilla.org/mozilla-central/rev/14aa33f8babc https://hg.mozilla.org/mozilla-central/rev/a05091172da1