1403871 - perf: blank MarkupView for several seconds if Layout panel is opened

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P2)

Description

[Julian Descottes [:jdescottes]]

Setup:
- open the inspector
- switch to layout panel
- close inspector
- go to http://searchfox.org/mozilla-central/rev/f54c1723befe6bcc7229f005217d5c681128fcad/dom/canvas/CanvasRenderingContext2D.cpp#5489
- open the inspector

=> markup view will be blank for several seconds

The key to reproduce this is really to have the layout panel selected when switching to the inspector. If instead you open the inspector and have the rule view selected by default, the markupview will be displayed really quickly.

Perf profile: https://perf-html.io/public/3702fe27fc5098c3b75d035f64453bfa4f554a32/calltree/?hiddenThreads=&invertCallstack&thread=2&threadOrder=0-2-1

Most time seems to be spent in the content page, due to a call to getAllGrids on the Layout actor.

The call comes from http://searchfox.org/mozilla-central/rev/f54c1723befe6bcc7229f005217d5c681128fcad/devtools/client/inspector/grids/grid-inspector.js#273 . 

We are not explicitly waiting for this getAllGrids to complete before finishing the initialization of the Layout panel. I guess getAllGrids() blocks the content thread, which blocks the MarkupView _updateChildren() call.

We could either wait for the markup view to be initialized before fetching the grids or ideally find a smarter way to fetch grids that doesn't require traversing the whole tree. 

(Note that similarly, the DevTools "freeze" when trying to close them after this scenario. This is linked to another getAllGrids call made at http://searchfox.org/mozilla-central/rev/f54c1723befe6bcc7229f005217d5c681128fcad/devtools/client/inspector/grids/grid-inspector.js#415 )

Comment 1

[Patrick Brosset <:pbro>]

Brad, could we implement something in the layout engine that would be smarter at retrieving all current grids than walking through the whole DOM like we currently do?

Comment 2

[Brad Werth [:bradwerth]]

Sorry for my delay in response. There's nothing that already exists that holds a lookup table for nsGridContainerFrames. I'll discuss options with others in the Layout team on how we have handled issues like this in the past. My guess is that we'll push back on generating a "just in case" lookup table of grid-styled elements, so any solution that we would create on the platform side would look a lot like a solutions you could create on the devtools side. In other words, the platform solution would likely provide a call much like your getGrids call, where the first time you call it, it would generate the lookup table (somewhat costly) and cache it until it is invalidated by some restyle. It wouldn't be much more efficient than what you are doing already.

I'll see if we can give you something better, but my recommendation is to find a way to asynchronously call getAllGrids, draw the Layout panel immediately but delay the display of the grid panel elements until the grid info is available.

Comment 3

[Brad Werth [:bradwerth]]

gl and I just had an IRC conversation where we agreed:

1) Though a platform-side walk might not be much faster than the DOM walk, it certainly can't be slower. Let's provide a platform-side replacement for getAllGrids with something like Document.getAllElementsWithGrid().

2) We'll eventually want something similar for Flexbox containers, so let's build Document.getAllElementWithFlexbox().

3) Let's also produce a generic platform-side walker with a signature like Document.getAllElementsMatching(stringWithCSS).

4) Investigate whether we can build conditional caches of grid and flex containers to be filled during restyle when devtools is detected to be running. If so, then these caches would replace the walk done in the new methods created for Parts 1 and 2.

5) Since these methods still might not be fast enough to avoid stalls in synchronous calls, devtools may need to try different calling patterns to retrieve these caches opportunistically and asynchronously.

I'll convert this Bug into addressing Part 1 of this plan, and file new bugs for Parts 2 through 4. Devtools will decide whether or when to open a Bug corresponding to Part 5.

Comment 4

[Brad Werth [:bradwerth]]

Attachment: Bug 1403871 Part 1: Add getElementsWithFlex and getElementsWithGrid to Element.

Review commit: https://reviewboard.mozilla.org/r/195188/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195188/

Comment 5

[Brad Werth [:bradwerth]]

Attachment: Bug 1403871 Part 2: Add tests of getElementsWithFlex and getElementsWithGrid.

Review commit: https://reviewboard.mozilla.org/r/195190/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195190/

Comment 6

[Brad Werth [:bradwerth]]

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

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8924000 [details]
Bug 1403871 Part 1: Add getElementsWithFlex and getElementsWithGrid to Element.

https://reviewboard.mozilla.org/r/195188/#review200324

Don't think I am an appropriate peer to be reviewing cpp code. Clearing review for myself

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8924001 [details]
Bug 1403871 Part 2: Add tests of getElementsWithFlex and getElementsWithGrid.

https://reviewboard.mozilla.org/r/195190/#review200326

Comment 9

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

Comment on attachment 8924000 [details]
Bug 1403871 Part 1: Add getElementsWithFlex and getElementsWithGrid to Element.

https://reviewboard.mozilla.org/r/195188/#review200558

::: dom/base/Element.cpp:1601
(Diff revision 1)
> +    // Actually check the display style for something that will
> +    // resolve to flex.
> +    RefPtr<nsStyleContext> styleContext =
> +      nsComputedDOMStyle::GetStyleContext(aElement, nullptr, nullptr);
> +    if (styleContext) {
> +      const nsStyleDisplay* display = styleContext->StyleDisplay();

I'd prefer if dholbert or someone would take a look at the Style* usage here.

::: dom/base/Element.cpp:1611
(Diff revision 1)
> +    }
> +    return false;
> +  };
> +
> +  // Actually construct the list, and return it.
> +  RefPtr<nsContentList> list = new nsContentList(this,

Does this really need to be a live list (the last param to the ctor)? If so, I don't understand how the list is invalidated when the style of the elements is changed.
I assume you don't want live list.

::: dom/base/Element.cpp:1647
(Diff revision 1)
> +    }
> +    return false;
> +  };
> +
> +  // Actually construct the list, and return it.
> +  RefPtr<nsContentList> list = new nsContentList(this,

Same here.

::: dom/webidl/Element.webidl:73
(Diff revision 1)
>    [Throws, Pure]
>    HTMLCollection getElementsByTagNameNS(DOMString? namespace, DOMString localName);
>    [Pure]
>    HTMLCollection getElementsByClassName(DOMString classNames);
> -
> +  [ChromeOnly, Pure]
> +  HTMLCollection getElementsWithFlex();

Did you consider to add more extendable method, for example something like

dictionary ElementFilter
{
  boolean withFlex = false;
  boolean withGrid = false;
}

[ChromeOnly]
sequence<Element> getElements(ElementFilter aFilter);

or if sequence<Element> is annoying, returning NodeList is ok too, but don't use HTMLCollection https://dom.spec.whatwg.org/#htmlcollection see the note there.


The implementation would become simpler since you'd have just one method implementation and less duplicated code.

Comment 10

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

Comment on attachment 8924001 [details]
Bug 1403871 Part 2: Add tests of getElementsWithFlex and getElementsWithGrid.

https://reviewboard.mozilla.org/r/195190/#review200566

::: dom/base/test/chrome/test_getElementsWithFlex.html:58
(Diff revision 1)
> +    {id:"d", message:"display:flex with visibility:hidden"},
> +  ];
> +
> +  testElementsAreInCollection(initialElements, collection, 0);
> +
> +  // Now add some matching elements to the DOM. The collection should automatically add them.

So you test only that new elements get added to the collection. But if it was properly working live list, elements should be removed there too when they aren't grid or flex anymore, which is why I suggest non-live list.

But if you really want live list, explain why.
Note, live lists slow down most of other dom operations, which is why people don't like them too much.

Comment 11

[Brad Werth [:bradwerth]]

Comment on attachment 8924000 [details]
Bug 1403871 Part 1: Add getElementsWithFlex and getElementsWithGrid to Element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195188/diff/1-2/

Comment 12

[Brad Werth [:bradwerth]]

Comment on attachment 8924001 [details]
Bug 1403871 Part 2: Add tests of getElementsWithFlex and getElementsWithGrid.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195190/diff/1-2/

Comment 13

[Brad Werth [:bradwerth]]

Comment on attachment 8924000 [details]
Bug 1403871 Part 1: Add getElementsWithFlex and getElementsWithGrid to Element.

https://reviewboard.mozilla.org/r/195188/#review200558

> Does this really need to be a live list (the last param to the ctor)? If so, I don't understand how the list is invalidated when the style of the elements is changed.
> I assume you don't want live list.

Doesn't absolutely need to be live, and with your suggestion that the return type be sequence<Element>, it's impractical for it to be a live list. I've made it a one-time generated list.

> Did you consider to add more extendable method, for example something like
> 
> dictionary ElementFilter
> {
>   boolean withFlex = false;
>   boolean withGrid = false;
> }
> 
> [ChromeOnly]
> sequence<Element> getElements(ElementFilter aFilter);
> 
> or if sequence<Element> is annoying, returning NodeList is ok too, but don't use HTMLCollection https://dom.spec.whatwg.org/#htmlcollection see the note there.
> 
> 
> The implementation would become simpler since you'd have just one method implementation and less duplicated code.

I did change it to sequence<Element> as you propose. I did not unify the two methods into one method, though the implementations now share more code.

Comment 14

[Brad Werth [:bradwerth]]

Comment on attachment 8924001 [details]
Bug 1403871 Part 2: Add tests of getElementsWithFlex and getElementsWithGrid.

https://reviewboard.mozilla.org/r/195190/#review200566

> So you test only that new elements get added to the collection. But if it was properly working live list, elements should be removed there too when they aren't grid or flex anymore, which is why I suggest non-live list.
> 
> But if you really want live list, explain why.
> Note, live lists slow down most of other dom operations, which is why people don't like them too much.

No longer a live list, and so these additive sections have been removed from both tests.

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8924000 [details]
Bug 1403871 Part 1: Add getElementsWithFlex and getElementsWithGrid to Element.

https://reviewboard.mozilla.org/r/195188/#review201572

This needs review from a DOM peer, since the changes are in DOM and since it's touching a .webidl file.

Here are my notes, though:

::: dom/base/Element.h:987
(Diff revision 2)
> +  void GetElementsWithFlex(nsTArray<RefPtr<Element>>& aElements);
> +  void GetElementsWithGrid(nsTArray<RefPtr<Element>>& aElements);
> +

Two things:

(1) Please you add some documentation to indicate what these functions do? In particular:
 - what "elements" are checked (might be clearer with a rename but still worth documenting somewhere)
 - do pseudo-elements count? (looks like no)
 - do elements in display:none subtrees count? (looks like yes)
 - what does "WithFlex" actually mean (looks like it means elements with display:{inline-}flex as well as -webkit-{inline-}box)

(2) There's no need to use an outparam for these functions (and for their helper GetElementsByMatching).  These days, I think we prefer to transfer ownership of temporary nsTArrays (and other moveable objects) via direct return-values, which the caller can capture with Move(). (Move() ensures that nsTArray's buffer will just be transferred from the function to its caller, since nsTArray has a move-constructor.)   That makes the function signature a bit clearer, and it means you can simplify your implementations so that they don't have to bother clearing out the passed-in nsTArray at the beginning, and callers don't have to do any hand-wringing about what might happen if they pass in a pre-existing nsTArray (and whether they need to bother clearing it out etc)

So I think this should be declared like so:
 nsTArray<RefPtr<Element>> aElements GetElementsWithFlex();

And it'd be implemented like this roughly:
  nsTArray<RefPtr<Element>> elems =
    Move(GetElementsByMatching(...));
  return elems;

And the caller would call it like so:
  nsTArray<RefPtr<Element>> elems =
    Move(GetElementsWithFlex(...));

::: dom/base/Element.h:994
(Diff revision 2)
> +   * passed in, along with the aData provided to the call to
> +   * GetElementsByMatching().
> +   */
> +  typedef bool (*nsElementMatchFunc)(Element* aElement,
> +                                     void* aData);
> +
> +  void GetElementsByMatching(nsElementMatchFunc aFunc,
> +                             void* aData,
> +                             nsTArray<RefPtr<Element>>& aElements);

It looks to me like aData is never really used (your callers only ever pass in null, and your IsDisplayFlex/IsDisplayGrid helpers don't use/need the passed-in aData arg).

Maybe just get rid of aData entirely, for simplicity? And if some GetElementsByMatching caller/helper needs it in the future, then we can add it to the function-signatures at that point.

::: dom/base/Element.cpp:1583
(Diff revision 2)
> +  // This helper function is passed to GetElementsByMatching()
> +  // to identify elements that have display:flex styling.
> +  auto IsDisplayFlex = [](Element* aElement,

"display:flex styling" is a bit too specific here.

How about "elements that are flex containers"?

::: dom/base/Element.cpp:1588
(Diff revision 2)
> +    // Try a quick frame check first.
> +    nsIFrame* frame = aElement->GetStyledFrame();
> +    if (frame) {
> +      return frame->IsFlexContainerFrame();
> +    }

This won't do the right thing if the frame is "overflow:scroll;display:flex".

In that scenario, GetStyledFrame would return the outer scroll frame (of type nsHTMLScrollFrame I think), and that frame's IsFlexContainerFrame() will return false, so we'd end up returning false here even though aElement does really have display:flex.

So I think you'd really need to have a function that reliably finds the correct frame to check here (maybe GetContentInsertionFrame?), *or* you need to relax this return statement so that it's just a weak signal that can makes us return true but won't make us return false.

BUT, really, this might be unnecessarily complex / premature optimization.  Is there a measurable benefit to having the two-part function here rather than just a one-part function?  In the lack of measurements showing that the two-part function is usefully faster (while still being correct in cases with e.g. overflow:scroll), I'd say we should just keep this simple and only do a single check here -- probably the display->mDisplay check, since that's probably a bit more robust.

::: dom/base/Element.cpp:1600
(Diff revision 2)
> +    // resolve to flex.
> +    RefPtr<nsStyleContext> styleContext =
> +      nsComputedDOMStyle::GetStyleContext(aElement, nullptr, nullptr);
> +    if (styleContext) {
> +      const nsStyleDisplay* display = styleContext->StyleDisplay();
> +      return display && (display->mDisplay == StyleDisplay::Flex ||

No need to null-check "display" here. StyleDisplay() is guaranteed to return non-null. (Gecko convention is that, for any C++ getter which lacks the word "Get", you can assume it'll never ever return null.)

::: dom/base/Element.cpp:1647
(Diff revision 2)
> +  do {
> +    cur = cur->GetNextNode(this);
> +    if (!cur) {
> +      break;
> +    }
> +    if (cur->IsElement() && aFunc(cur->AsElement(), aData)) {
> +      aElements.AppendElement(cur->AsElement());
> +    }
> +  } while(true);

The "do {" seems unnecessarily verbose for an infinite loop here.

Can you simplify to just:
  while (true) {
   ...
  }

Or "for (;;)" if you prefer.  Or maybe better, I think you can collapse the loop-end condition into the while statement like so:

  while ((cur = cur->GetNextNode(this)) {
    if (cur->IsElement() && aFunc(cur->AsElement(), aData)) {
      aElements.AppendElement(cur->AsElement());
    }
  }

::: dom/webidl/Element.webidl:76
(Diff revision 2)
>    HTMLCollection getElementsByClassName(DOMString classNames);
> -
> +  [ChromeOnly, Pure]
> +  sequence<Element> getElementsWithFlex();
> +  [ChromeOnly, Pure]
> +  sequence<Element> getElementsWithGrid();
> +  

fix whitespace on blank line

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8924000 [details]
Bug 1403871 Part 1: Add getElementsWithFlex and getElementsWithGrid to Element.

https://reviewboard.mozilla.org/r/195188/#review201572

(Ah, I now see smaug already took a look and bounced it over to me. :)  The final version probably still needs r+ from a DOM peer [which I think is enforced automatically for any .webidl changes] -- though I'm happy to be reviewer up to that point, and this would probably eventually be r=dholbert,whoever (or r=dholbert r=whoever; I forget how mozreview structures double-review)

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 8924000 [details]
Bug 1403871 Part 1: Add getElementsWithFlex and getElementsWithGrid to Element.

https://reviewboard.mozilla.org/r/195188/#review201572

> Two things:
> 
> (1) Please you add some documentation to indicate what these functions do? In particular:
>  - what "elements" are checked (might be clearer with a rename but still worth documenting somewhere)
>  - do pseudo-elements count? (looks like no)
>  - do elements in display:none subtrees count? (looks like yes)
>  - what does "WithFlex" actually mean (looks like it means elements with display:{inline-}flex as well as -webkit-{inline-}box)
> 
> (2) There's no need to use an outparam for these functions (and for their helper GetElementsByMatching).  These days, I think we prefer to transfer ownership of temporary nsTArrays (and other moveable objects) via direct return-values, which the caller can capture with Move(). (Move() ensures that nsTArray's buffer will just be transferred from the function to its caller, since nsTArray has a move-constructor.)   That makes the function signature a bit clearer, and it means you can simplify your implementations so that they don't have to bother clearing out the passed-in nsTArray at the beginning, and callers don't have to do any hand-wringing about what might happen if they pass in a pre-existing nsTArray (and whether they need to bother clearing it out etc)
> 
> So I think this should be declared like so:
>  nsTArray<RefPtr<Element>> aElements GetElementsWithFlex();
> 
> And it'd be implemented like this roughly:
>   nsTArray<RefPtr<Element>> elems =
>     Move(GetElementsByMatching(...));
>   return elems;
> 
> And the caller would call it like so:
>   nsTArray<RefPtr<Element>> elems =
>     Move(GetElementsWithFlex(...));

Actually, RE which elements are checked -- right now it looks like you're only considering descendants of "this", since your impl calls GetNextNode() before checking anything  But that means, for example, the <html> element itself would never end up being returned via this API (even if it was display:flex)

You might consider including the "this" node (testing it with your flex/grid helper-function before calling GetNextNode on it), so that <html> (or whatever container element it's called on) would be included.

For that, I think your GetElementsByMatchin() impl could simplify to this sort of loop:
  for (nsINode* cur = this; cur; cur = cur->GetNextNode(this)) {
    // add cur to nsTArray if it satisfies conditions;
  }

Either way, though, you should document whether "this" is included in the tested elements.

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 8924001 [details]
Bug 1403871 Part 2: Add tests of getElementsWithFlex and getElementsWithGrid.

https://reviewboard.mozilla.org/r/195190/#review201604

::: dom/base/test/chrome/test_getElementsWithFlex.html:21
(Diff revision 2)
> +<script>
> +'use strict';
> +
> +SimpleTest.waitForExplicitFinish();
> +
> +function testElementsAreInCollection(elements, collection, startingAt) {

I don't think "startingAt" is useful here -- the caller passes in "0", which means the one usage (checking it for being >= length) is trivially satisfied.

::: dom/base/test/chrome/test_getElementsWithFlex.html:68
(Diff revision 2)
> +  <div id="c" class="i"></div>
> +  <div id="d" class="f" style="visibility:hidden"></div>

"visibility:hidden" is a slightly-interesting case, but not super-interesting.  That style just affects painting, but doesn't really impact the frame tree at all.  Worth keeping I suppose, but it's more important to test the following edge cases:
 * display-none flex container:
  <div style="display:none" class="f"></div>

 * flex container inside of a display:none element:
  <div style="display:none"><div class="f"></div></div>

 * overflow:scroll flex container:
  <div style="overflow:scroll" class="f"></div>

 * button as a flex container:
   <button class="f"></button>

 * fieldset as a flex container:
   <fieldset class="f"></fieldset>

 * <html> element itself as a flex container

 * All of the above with grid instead of flex (in the grid version of this test)

Also, you should test -webkit-box / -webkit-inline-box display values in at least some basic cases.

Also, you should test some elements that are *not* any variety of display:flex, and make sure they do *not* end up in the returned set.  (Because right now, your C++ implementation could pass this test by simply returning every single element, I think.)

Also, since this file is so similar to the grid version (and will need to test all the same edge cases), it might be worth merging the two somehow so that there's less duplicated test-logic and so that edge cases are consistently tested between the two APIs...

Comment 19

[Brad Werth [:bradwerth]]

Comment on attachment 8924000 [details]
Bug 1403871 Part 1: Add getElementsWithFlex and getElementsWithGrid to Element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195188/diff/2-3/

Comment 20

[Brad Werth [:bradwerth]]

Comment on attachment 8924001 [details]
Bug 1403871 Part 2: Add tests of getElementsWithFlex and getElementsWithGrid.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195190/diff/2-3/

Comment 21

[Brad Werth [:bradwerth]]

Comment on attachment 8924000 [details]
Bug 1403871 Part 1: Add getElementsWithFlex and getElementsWithGrid to Element.

https://reviewboard.mozilla.org/r/195188/#review201636

::: dom/base/Element.h:987
(Diff revision 2)
> +  void GetElementsWithFlex(nsTArray<RefPtr<Element>>& aElements);
> +  void GetElementsWithGrid(nsTArray<RefPtr<Element>>& aElements);
> +

(1) Documentation added as you propose.
(2) I like your solution, but I can't figure out how to get a webidl to generate that signature from a "sequence<Element>" type. The version I'm using is what matches the generated ElementBinding.cpp file.

Comment 22

[Brad Werth [:bradwerth]]

Comment on attachment 8924001 [details]
Bug 1403871 Part 2: Add tests of getElementsWithFlex and getElementsWithGrid.

https://reviewboard.mozilla.org/r/195190/#review201604

> "visibility:hidden" is a slightly-interesting case, but not super-interesting.  That style just affects painting, but doesn't really impact the frame tree at all.  Worth keeping I suppose, but it's more important to test the following edge cases:
>  * display-none flex container:
>   <div style="display:none" class="f"></div>
> 
>  * flex container inside of a display:none element:
>   <div style="display:none"><div class="f"></div></div>
> 
>  * overflow:scroll flex container:
>   <div style="overflow:scroll" class="f"></div>
> 
>  * button as a flex container:
>    <button class="f"></button>
> 
>  * fieldset as a flex container:
>    <fieldset class="f"></fieldset>
> 
>  * <html> element itself as a flex container
> 
>  * All of the above with grid instead of flex (in the grid version of this test)
> 
> Also, you should test -webkit-box / -webkit-inline-box display values in at least some basic cases.
> 
> Also, you should test some elements that are *not* any variety of display:flex, and make sure they do *not* end up in the returned set.  (Because right now, your C++ implementation could pass this test by simply returning every single element, I think.)
> 
> Also, since this file is so similar to the grid version (and will need to test all the same edge cases), it might be worth merging the two somehow so that there's less duplicated test-logic and so that edge cases are consistently tested between the two APIs...

Thanks for all these testcases. I added all of them except for the display:none case, since that precludes the element from being flex/grid. The container-within-a-display-none case is in. I also kept the two tests separate since I couldn't think of clean logic for transforming the styles from the different flex styles to the different grid styles, and some of the grid cases are TODO, etc.

Comment 23

[Brad Werth [:bradwerth]]

I opened Bug 1414920 to track the chrome API additions required by this bug. Also moving the existing patches over to that bug.

Comment 24

[Brad Werth [:bradwerth]]

Returning the bug to its original title, focused on the performance of the devtools experience (where the bug is classified).

Comment 25

[Julian Descottes [:jdescottes]]

Just tested the new API, retrieving grid elements on the test page mentioned in the summary is now done in a couple ms!

Comment 26

[Julian Descottes [:jdescottes]]

Attachment: bug 1403871 - use getElementsWithGrid to retrieve grid containers in devtools;

Review commit: https://reviewboard.mozilla.org/r/201630/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201630/

Comment 27

[Julian Descottes [:jdescottes]]

try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc82a868c509d93471d81ec36454b68783e16727

Comment 28

[Julian Descottes [:jdescottes]]

Comment on attachment 8930482 [details]
bug 1403871 - use getElementsWithGrid to retrieve grid containers in devtools;

The frame traversal was not behaving as explained in the JSdoc and the new implementation breaks an existing test. 

Clearing r? in the meantime

Comment 29

[Julian Descottes [:jdescottes]]

Attachment: Bug 1403871 - remove misleading traverseFrames argument in getAllGrids;

The current implementation of getAllGrids supports a traverseFrames argument
supposed to enable or not traversing nested frames under a provided root node.

First of all this argument is only used in server tests.

Also, even if this argument is false, the tree walk performed will traverse
frames anyway, meaning there is no way here to not traverse frames when calling
getAllGrids.

The only real difference when calling the method with traverseFrames = true
is that we will retrieve grid containers no longer contained in the root node
(making the root node pointless).

If we really want to implement this behavior in the future, we should split the
API in two with getGrids(node) and getAllGrids(). For now I just remove the
argument since it is not useful.

Review commit: https://reviewboard.mozilla.org/r/201682/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201682/

Comment 30

[Julian Descottes [:jdescottes]]

Attachment: Bug 1403871 - rename getAllGrids to getGrids;

Review commit: https://reviewboard.mozilla.org/r/201684/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201684/

Comment 31

[Julian Descottes [:jdescottes]]

Comment on attachment 8930482 [details]
bug 1403871 - use getElementsWithGrid to retrieve grid containers in devtools;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201630/diff/1-2/

Comment 32

[Julian Descottes [:jdescottes]]

Working on a new talos test:
- test + fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aae4c34a395aa42cc7b992c8d7f3090855274c60
- test only: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5295d2bdd7de01f6e82d4a5faa528936277c682
- baseline: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe55df7cc7cac80c427bc51045e5b231ce2e1026

Comment 33

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8930482 [details]
bug 1403871 - use getElementsWithGrid to retrieve grid containers in devtools;

https://reviewboard.mozilla.org/r/201630/#review206996

::: devtools/server/actors/layout.js:221
(Diff revision 2)
> +    }
>  
> -    while (treeWalker.nextNode()) {
> -      let currentNode = treeWalker.currentNode;
> +    let gridElements = node.getElementsWithGrid();
> +    let gridActors = gridElements.map(n => new GridActor(this, n));
>  
> -      if (currentNode.getGridFragments && currentNode.getGridFragments().length > 0) {
> +    let frames = [...node.querySelectorAll("iframe, frame")];

for..of loops will loop over a NodeList correctly. We can remove the spread of the NodeList to convert to an Array.

Comment 34

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8930550 [details]
Bug 1403871 - remove misleading traverseFrames argument in getAllGrids;

https://reviewboard.mozilla.org/r/201682/#review206998

::: devtools/server/tests/browser/browser_layout_simple.js:16
(Diff revision 1)
>      "data:text/html;charset=utf-8,<title>test</title><div></div>");
>  
>    ok(layout, "The LayoutFront was created");
>    ok(layout.getAllGrids, "The getAllGrids method exists");
>  
>    let didThrow = false;

Can you remove this test for getGrids while you are at it. getGrids is not a public method so it should method throw regardless.

Comment 35

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8930551 [details]
Bug 1403871 - rename getAllGrids to getGrids;

https://reviewboard.mozilla.org/r/201684/#review207000

::: devtools/server/actors/layout.js:204
(Diff revision 1)
>  
>    /**
>     * Returns an array of GridActor objects for all the grid elements contained in the
>     * given root node.
>     *
>     * @param  {Node} node

s/{Node}/{Node|NodeActor}

::: devtools/server/actors/layout.js:213
(Diff revision 1)
>    getGrids(node) {
>      if (!node) {
>        return [];
>      }
>  
> +    // node can either be a Node or a NodeActor.

s/node/Root node

::: devtools/server/tests/browser/browser_layout_simple.js:24
(Diff revision 1)
>    } catch (e) {
>      didThrow = true;
>    }
>    ok(didThrow, "An exception was thrown for a missing NodeActor in getGrids");
>  
>    didThrow = false;

Can probably remove one of the layout.getGrids(null) test here since they are repeating.

Comment 36

[Julian Descottes [:jdescottes]]

Some performance results:

- test-only vs baseline: 

  https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=fe55df7cc7cac80c427bc51045e5b231ce2e1026&newProject=try&newRevision=a5295d2bdd7de01f6e82d4a5faa528936277c682&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&framework=1

  Looks like my new test has an impact on webconsole tests.

- test + fix vs test-only 

  https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=a5295d2bdd7de01f6e82d4a5faa528936277c682&newProject=try&newRevision=aae4c34a395aa42cc7b992c8d7f3090855274c60&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&framework=1

  Reporting a ~40% win on the new test inspector.layout.open. Runtime is still around 1000ms, will try with less nodes in the test.

Comment 37

[Julian Descottes [:jdescottes]]

Thanks for the reviews Gabriel. I will move the test work to a follow-up and land this.

Comment 38

[Julian Descottes [:jdescottes]]

Comment on attachment 8930550 [details]
Bug 1403871 - remove misleading traverseFrames argument in getAllGrids;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201682/diff/1-2/

Comment 39

[Julian Descottes [:jdescottes]]

Comment on attachment 8930482 [details]
bug 1403871 - use getElementsWithGrid to retrieve grid containers in devtools;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201630/diff/2-3/

Comment 40

[Julian Descottes [:jdescottes]]

Comment on attachment 8930551 [details]
Bug 1403871 - rename getAllGrids to getGrids;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201684/diff/1-2/

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/731a141e8c31
remove misleading traverseFrames argument in getAllGrids;r=gl
https://hg.mozilla.org/integration/autoland/rev/c9a12455c869
use getElementsWithGrid to retrieve grid containers in devtools;r=gl
https://hg.mozilla.org/integration/autoland/rev/fb858a306b2f
rename getAllGrids to getGrids;r=gl

Comment 42

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/731a141e8c31
https://hg.mozilla.org/mozilla-central/rev/c9a12455c869
https://hg.mozilla.org/mozilla-central/rev/fb858a306b2f

Comment 43

[Sajedul Islam]

I have reproduced this bug with Nightly 58.0a1 (2017-09-28) on Windows 8.1 , 64 Bit ! 

This bug's fix is Verified with latest Nightly !

Build   ID    20171201100115
User Agent    Mozilla/5.0 (Windows NT 6.3; WOW64; rv:59.0) Gecko/20100101 Firefox/59.0

[bugday-20171129]