Closed Bug 1403871 Opened 2 years ago Closed 2 years ago

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

Categories

(DevTools :: Inspector, enhancement, P2)

enhancement

Tracking

(firefox57 fix-optional, firefox59 fixed)

RESOLVED FIXED
Firefox 59
Tracking Status
firefox57 --- fix-optional
firefox59 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

(Blocks 1 open bug)

Details

(Whiteboard: [designer-tools])

Attachments

(3 files, 2 obsolete files)

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 )
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?
Flags: needinfo?(bwerth)
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.
Assignee: nobody → bwerth
Flags: needinfo?(bwerth)
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.
Priority: -- → P2
Whiteboard: [designer-tools]
Summary: perf: blank MarkupView for several seconds if Layout panel is opened → perf: cache list of document flex and grid containers for retrieval by devtools
Attachment #8924000 - Flags: review?(gl)
Attachment #8924001 - Flags: review?(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
Attachment #8924000 - Flags: review?(gl)
Comment on attachment 8924001 [details]
Bug 1403871 Part 2: Add tests of getElementsWithFlex and getElementsWithGrid.

https://reviewboard.mozilla.org/r/195190/#review200326
Attachment #8924001 - Flags: review?(gl)
Attachment #8924000 - Flags: review?(bugs)
Attachment #8924001 - Flags: review?(bugs)
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.
Attachment #8924000 - Flags: review?(bugs) → review-
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.
Attachment #8924001 - Flags: review?(bugs) → review-
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 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.
Attachment #8924000 - Flags: review?(bugs) → review?(dholbert)
Attachment #8924001 - Flags: review?(bugs)
Attachment #8924001 - Flags: review?(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
Attachment #8924000 - Flags: review?(dholbert) → review-
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 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 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...
Attachment #8924001 - Flags: review?(dholbert) → review-
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 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.
Depends on: 1414920
I opened Bug 1414920 to track the chrome API additions required by this bug. Also moving the existing patches over to that bug.
Attachment #8924000 - Attachment is obsolete: true
Attachment #8924000 - Flags: review?(dholbert)
Attachment #8924001 - Attachment is obsolete: true
Attachment #8924001 - Flags: review?(dholbert)
Returning the bug to its original title, focused on the performance of the devtools experience (where the bug is classified).
Assignee: bwerth → nobody
Summary: perf: cache list of document flex and grid containers for retrieval by devtools → perf: blank MarkupView for several seconds if Layout panel is opened
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Just tested the new API, retrieving grid elements on the test page mentioned in the summary is now done in a couple ms!
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
Attachment #8930482 - Flags: review?(gl)
Blocks: 1419462
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.
Attachment #8930482 - Flags: review?(gl) → review+
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.
Attachment #8930550 - Flags: review?(gl) → review+
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.
Attachment #8930551 - Flags: review?(gl) → review+
Thanks for the reviews Gabriel. I will move the test work to a follow-up and land this.
Depends on: 1419533
Pushed by jdescottes@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/731a141e8c31
https://hg.mozilla.org/mozilla-central/rev/c9a12455c869
https://hg.mozilla.org/mozilla-central/rev/fb858a306b2f
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
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]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.