This method is intended to replace a JS-side DOM walk done by devtools. This has been identified as a performance bottleneck for the devtools grid inspector panel, and expected to be a bottleneck for the planned flex inspector panel.

Comment on attachment 8925629 [details] Bug 1414920 Part 1: Add chrome-only API getElementsWithGrid(), for devtools. https://reviewboard.mozilla.org/r/196734/#review202444 r=me on the code changes, with nits addressed -- though it also needs r+ from a DOM peer since these are changes in /dom (and since there are changes to WebIDL) ::: commit-message-2535b:1 (Diff revision 1) > +Bug 1414920 Part 1: Add getElementsWithFlex and getElementsWithGrid to Element. Clearer about what's happening & why: "Bug 1414920 Part 1: Add chrome-only APIs getElementsWithFlex() and getElementsWithGrid(), for devtools" ::: dom/base/Element.h:987 (Diff revision 1) > + /** > + * Return an array of all elements and descendants, starting with > + * this, that have styling which will cause them to generate > + * a nsFlexContainerFrame during layout. This covers elements > + * that are not visible, but does not include pseudo-elements. > + */ > + void GetElementsWithFlex(nsTArray<RefPtr<Element>>& aElements); Thanks for the documentation! A few nits: (1) The phrase "all elements and descendants, starting with this" isn't quite right. That sounds like it could also include "this" element's next-sibling, for example. (Not a descendant, but it's arguably in the category of "all elements...starting with this".) And also, the current phrasing sounds like it's saying some of the returned descendants might not be Elements, which is a bit confusing. How about, replace this phrase with: "all elements in the subtree rooted at this element" (2) The "styling which will cause them to generate a nsFlexContainer" isn't quite correct. It makes it sound like this API would exclude display:flex elements that have a display:none ancestor e.g.: <div style="display:none"> <div style="display:flex"> ...because such an element **does not** generate a nsFlexContainerFrame (because its whole subtree won't generate **any** frames). But in fact, the current implelemntation **does** include this element in the returned array, I think. (The "not visible" caveat kind of hand-waves past this, but it's not quite precise enough, in part because "visible" sounds more like "visibility:hidden"). How about something like: "...that are styled as flex containers (including via legacy 'box' styling that we emulate with a modern flex container). This includes elements that don't actually generate any frames (by virtue of being in a 'display:none' subtree), but this does not include pseudo-elements." ::: dom/base/Element.cpp:1583 (Diff revision 1) > + // This helper function is passed to GetElementsByMatching() > + // to identify elements with styling which will cause them to > + // generate a nsFlexContainerFrame during layout. > + auto IsDisplayFlex = [](Element* aElement) -> bool > + { > + // Check the display style for something that will resolve to flex. The phrase "something that will resolve to flex" is a bit misleading, in this one-liner comment -- it sounds like it's saying the value gets converted/replaced with a different value in the CSS engine (but it doesn't). How about "Check 'display' for values that generate a flex container"? Or alternately, just get rid of this one-liner comment, since the multiline comment above it kind of already duplicates the same information. ::: dom/base/Element.cpp:1612 (Diff revision 1) > + // This helper function is passed to GetElementsByMatching() > + // to identify elements with styling which will cause them to > + // generate a nsGridContainerFrame during layout. > + auto IsDisplayGrid = [](Element* aElement) -> bool > + { > + // Check the display style for something that will resolve to grid. (same here - "something that will resolve to grid" sounds misleading)

Comment on attachment 8925630 [details] Bug 1414920 Part 2: Add test of getElementsWithGrid. https://reviewboard.mozilla.org/r/196736/#review202482 The tests need a bit more work, I think, so I'm leaving those r- for now. ::: dom/base/test/chrome/test_getElementsWithFlex.html:30 (Diff revision 1) > +function testElementsAreInCollection(elements, collection) { > + if (collection.length > 0) { > + let c = 0; > + let foundElement = collection[c]; > + for (let expectedElement of elements) { I find it hard to keep track of the naming here. In particular: - "elements" is not actually an array of elements, despite its name (it's technically an array of id/message pairs) - It's unclear from the naming which arg is which, between "elements" and "collection" (which is the actual thing returned by the API vs. which is the expected thing that we're testing against?) - "foundElement" sounds like it's a match that we expected & found, but it's not quite that. Could you rename the args to something like the following to make it clearer: * expectedElements (or maybe even "expectedElementsInfo" to reflect that they're not actually elements) * actualElements ...and then "expectedElem"/"actualElem" for the individual entries, perhaps? (or something like that) ::: dom/base/test/chrome/test_getElementsWithFlex.html:31 (Diff revision 1) > +'use strict'; > + > +SimpleTest.waitForExplicitFinish(); > + > +function testElementsAreInCollection(elements, collection) { > + if (collection.length > 0) { Why the collection.length > 0 check here? Suppose "elements" is empty and "collection" is nonempty -- then this function would just immediately return without failing any tests. That seems wrong. Similarly: suppose "collection" has more entries than "elements" does. Right now, your test-code stops when it hits the end of "elements", so it wouldn't catch that sort of bug (having extra bogus entries at the end of the API's results). Please adjust to handle that. ::: dom/base/test/chrome/test_getElementsWithFlex.html:38 (Diff revision 1) > + let foundElement = collection[c]; > + for (let expectedElement of elements) { > + let isMatching = (expectedElement.id == foundElement.id); > + let test_function = (expectedElement.todo ? todo : ok); > + > + test_function(isMatching, "Found " + expectedElement.message + "."); s/Found/Should have found/ to make it clearer what the expectation is. (If you hit a failure here, then with the current wording, it'll print out something like "TEST_UNEXPECTED_FAIL: Found root with display:flex". It's not clear from that wording whether "Found" is the problematic thing that happened, vs. the thing that you were expecting & didn't happen. "Should have found" makes that clearer.) ::: dom/base/test/chrome/test_getElementsWithFlex.html:40 (Diff revision 1) > + // Only move to the next element in the collection if this one was a match. > + if (isMatching) { > + foundElement = collection[++c]; Can you add a line to this comment to explain why you're doing this? (advancing one specific array and not the other, in this mismatch case) It seems like you're implicitly assuming that the most likely reason for a mismatch is that an expected entry was missing from our output, right? (Rather than an *unexpected* entry being found in the output -- if that happens, the rest of the comparisons here would still fail, I think.) That assumption seems fine, but it'd be worth explicitly stating to make it clearer why you're doing this, because this arbitrary one-array advancing is non-intuitive. ::: dom/base/test/chrome/test_getElementsWithFlex.html:49 (Diff revision 1) > + let collection = document.documentElement.getElementsWithFlex(); > + let initialElementCount = collection.length; Could you also test the API on some smaller subtree than the root (e.g. maybe on the element with id "a"), just to make sure that it does what you expect (only returning a couple results) when called at that level of granularity? ::: dom/base/test/chrome/test_getElementsWithGrid.html:55 (Diff revision 1) > + {id:"b", message:"display:subgrid inside display:grid", todo:true}, > + {id:"c", message:"'plain' grid container with display:inline-grid"}, > + {id:"d", message:"display:subgrid inside display:inline-grid", todo:true}, Can you add "// bug 1240834" at the end of these lines, to tie them to an actual bug that should (hopefully) fix these todo's? That makes the reason for the failure a little clearer, and it increases the likelihood that we'll remember to update the test as part of fixing that bug.

(In reply to Brad Werth [:bradwerth] from comment #0) > This method is intended to replace a JS-side DOM walk done by devtools. This > has been identified as a performance bottleneck for the devtools grid > inspector panel, Have you verified that this implementation is noticably faster than the slow JS-side DOM walk that we're replacing? I'd guess that it'd be faster, but it'd be nice to validate that expectation before we actually take this API (as long as it's easy to swap it in for testing purposes). If it turns out that this doesn't improve things (e.g. because style resolution is the bottleneck, hypothetically), then it'd be nice to discover that before we land this, and figure out a different approach.

(In reply to Daniel Holbert [:dholbert] from comment #5) > (In reply to Brad Werth [:bradwerth] from comment #0) > > This method is intended to replace a JS-side DOM walk done by devtools. This > > has been identified as a performance bottleneck for the devtools grid > > inspector panel, > > Have you verified that this implementation is noticably faster than the slow > JS-side DOM walk that we're replacing? No, I haven't. I'll get confirmation from devtools that this is faster and that the speed matters before I land the bug. Gabe, would you please apply this patch and then use the new Element.getElementsWithGrid() method for the Grid layout panel? Does this new API speed things up in the scenario that inspired the original Bug 1403871?

Comment on attachment 8925629 [details] Bug 1414920 Part 1: Add chrome-only API getElementsWithGrid(), for devtools. https://reviewboard.mozilla.org/r/196734/#review203356 I don't know the use case here, so just a reminder that this will not capture elements in Shadow DOM.

Comment on attachment 8925630 [details] Bug 1414920 Part 2: Add test of getElementsWithGrid. https://reviewboard.mozilla.org/r/196736/#review203396 r=me

Let's go ahead with landing getElementsWithGrid(). The performance difference here is rather significant compare to having devtools walk the tree and checking for getGridFragments. The use case for getElementsWithFlex() became a lower priority because it seems unlikely we will show a list of flexbox elements like in the Grid panel. The reasoning here is that there are simply too many flex containers compare to grids, and the use case is unclear compare to what we have done with the Grid panel.

(In reply to Gabriel [:gl] (ΦωΦ) from comment #12) > Let's go ahead with landing getElementsWithGrid(). The performance > difference here is rather significant compare to having devtools walk the > tree and checking for getGridFragments. > > The use case for getElementsWithFlex() became a lower priority because it > seems unlikely we will show a list of flexbox elements like in the Grid > panel. The reasoning here is that there are simply too many flex containers > compare to grids, and the use case is unclear compare to what we have done > with the Grid panel. I've reworked the patches to only implement and test getElementsWithGrid().

Pushed by bwerth@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/49af88e4f1be Part 1: Add chrome-only API getElementsWithGrid(), for devtools. r=dholbert,smaug https://hg.mozilla.org/integration/autoland/rev/07704c3a2fde Part 2: Add test of getElementsWithGrid. r=dholbert

https://hg.mozilla.org/mozilla-central/rev/49af88e4f1be https://hg.mozilla.org/mozilla-central/rev/07704c3a2fde