Bug 34421 – Web Inspector: Elements Panel: Limit the number of initially loaded element children

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Alexander Pavlov (apavlov)]

Loading all element children in the elements tree outline imposes a heavy load on the frontend while inspecting pages with a really huge number of sibling nodes. The initial number of element children should be constrained to, say, 500, with a control that allows to load and display the remaining children.

[Alexander Pavlov (apavlov)]

Created attachment 47850 [details]
[PATCH] Suggested solution

Slightly concerned about the "Load All Children" option presentation (the button might not look and feel well enough. Should it be a link instead? Could the text use a bit of fine-tuning?)

[Timothy Hatcher]

Can you attach a screenshot?

[Timothy Hatcher]

How does this work if the DOM is mutated and we go to update an element that does not exist in the tree?

[Alexander Pavlov (apavlov)]

Created attachment 47851 [details]
[PATCH] Added limit checks for insertChild and removeChild

Please have a look at the solution in general. The patch needs dynamic DOM update testing.

[Pavel Feldman]

Comment on attachment 47851 [details]
[PATCH] Added limit checks for insertChild and removeChild

- "ask" is a strange prefix for the query parameter. As well as "are" is not good for the push parameter.
- It might be better to use a query parameter 'maximimNodes' to return upon get child nodes request.
- We should tell user how many nodes are missing (return that info and render it).
- 500 should either be in cpp or js, not both. You either query for 500 or push 500.
- insertChild should not be muted when number of child nodes is equal to 500. Where does this logic come from? In order to manage events properly, you should extend the m_childrenRequested with the information on the number of child nodes returned upon that request. That way you would be able to send only events that make sense to the front-end.
- Imagine user is invoking 'Inspect element' on the 501-th child node. How many siblings should we push? This makes it all a bit more complex. Should we introduce 'collapsed ranges' of nodes? Or is it too challenging? I'd say for now, if N-th node is requested, at least N sibling nodes should be pushed.
- I know I should have done it, but before we do something like this we need to get elements panel tests.

[Alexander Pavlov (apavlov)]

(In reply to comment #5)
> (From update of attachment 47851 [details])
> - "ask" is a strange prefix for the query parameter. As well as "are" is not
> good for the push parameter.

I'm not familiar with other normative prefixes than "is" and "did" neither of which seems to fit well without introducing an ambiguity. What's your suggestion for "we need to run a query with P holding true" and "the query result was constrained for P to hold true"?

> - It might be better to use a query parameter 'maximimNodes' to return upon get
> child nodes request.

I have tried that idea and dropped it as it complicates the entire logic. Currently, the frontend can (a) query partial children, (b) query all the rest children if the user asks. With maxNodes, the frontend needs to figure how many nodes to query every time, and it gets down to 500 in the (a) case and to -1 (which stands for "no limit") in the (b) case. Also, maxNodes doesn't make sense if we shall push "at least N nodes if the Nth node is inspected" (on the opposite, "askPartialChildren" says "do not query more children than necessary").

> - We should tell user how many nodes are missing (return that info and render
> it).

Makes sense, as long as the children are not dynamically updated. I don't think it is a good idea to track/push the number of children for all partially loaded nodes in the DOM. Or is it?

> - 500 should either be in cpp or js, not both. You either query for 500 or push
> 500.

This complicates the API, and the insertChild check was introduced in the last minute. This is debatable, though.

> - insertChild should not be muted when number of child nodes is equal to 500.
> Where does this logic come from? In order to manage events properly, you should

#webkit-inspector
[21:08]	xenon: apavlov: as long as it does not blow up, i don't think we need to add the element if it is above the soft limit

Hence, the second patch.

> extend the m_childrenRequested with the information on the number of child
> nodes returned upon that request. That way you would be able to send only
> events that make sense to the front-end.

Last time you said we should not worry about such events going into /dev/null. Are you worried about the performance? Also, there is no way to say "this node is known to the frontend" without more complex logic that filters all backend-to-frontend node pushes on the backend side.

> - Imagine user is invoking 'Inspect element' on the 501-th child node. How many
> siblings should we push? This makes it all a bit more complex. Should we

Yes, this poses a real problem that evaluates roughly to "we need this list of nodes loaded and displayed", where nodes may not be immediate siblings.

> introduce 'collapsed ranges' of nodes? Or is it too challenging? I'd say for
> now, if N-th node is requested, at least N sibling nodes should be pushed.

We should keep in mind that all inherently dynamic structures (which are due to the dynamic nature of DOM, like the collapsed ranges of nodes you mention) are going to be very hard to update (keep the ranges for every related parent node in the backend, update the ranges (which may be O(nChildren) in the worst case), push updates into the frontend), and the increasing logic complexity may not be worth it.

> - I know I should have done it, but before we do something like this we need to
> get elements panel tests.

We have some manual tests, including the live DOM updates reflected in the inspector. Guess we need some more stuff in...

[Alexander Pavlov (apavlov)]

Created attachment 48240 [details]
[PATCH] Only tree nodes are filtered, as discussed with pfeldman

This works with dynamic node insertion-removal. If a node is inserted into the expanded range, the range grows to avoid hiding the last visible node.
If "Inspect Element" is invoked on an element inside the collapsed range, all nodes down to the node requested are made visible (so that there is no more than one collapsed range).

[Pavel Feldman]

Created attachment 48252 [details]
[IMAGE] Looks with the patch.

[Pavel Feldman]

xenon: we should use the button style we have for sidebar panes
xenon: .panel-enabler-view button:not(.status-bar-item), .pane button
xenon: The button would read better as "Show All Nodes (3938 More)"

[Pavel Feldman]

Comment on attachment 48240 [details]
[PATCH] Only tree nodes are filtered, as discussed with pfeldman

> +    treeElementFor: function(node)
> +    {

createTreeElementFor ?

+ const parentElement = this.listItemElement.parentNode;
+ const nextChild = this.listItemElement.nextSibling;

comment why this is needed?

+ for (var i = this.expandedChildCount, limit = Math.min(this.expandedChildrenLimit, childNodeCount); i < limit; ++i)

ditto

I'd really like to see some tests for this one before we land. r+ otherwise.

[Alexander Pavlov (apavlov)]

Created attachment 48341 [details]
[PATCH] Comments addressed

[Alexander Pavlov (apavlov)]

(In reply to comment #10)
> (From update of attachment 48240 [details])
> > +    treeElementFor: function(node)
> > +    {
> 
> createTreeElementFor ?

Done

> + const parentElement = this.listItemElement.parentNode;
> + const nextChild = this.listItemElement.nextSibling;
> 
> comment why this is needed?

Removed code (no considerable perf improvement found)

> + for (var i = this.expandedChildCount, limit =
> Math.min(this.expandedChildrenLimit, childNodeCount); i < limit; ++i)
> 
> ditto

Done

> I'd really like to see some tests for this one before we land. r+ otherwise.

Done

[Alexander Pavlov (apavlov)]

Created attachment 48396 [details]
[IMAGE] (Windows) Updated button text/size

[Pavel Feldman]

Comment on attachment 48341 [details]
[PATCH] Comments addressed

It would be great to test subsequent calls to unexpanded nodes. Also button is still created using outerHTML.

[Alexander Pavlov (apavlov)]

Created attachment 48486 [details]
[PATCH] Test extended, driveby DOMAgent bug fixed

[Alexander Pavlov (apavlov)]

(In reply to comment #14)
> (From update of attachment 48341 [details])
> It would be great to test subsequent calls to unexpanded nodes. Also button is

Now testing DOM updates and expansion of the collapsed nodes.

> still created using outerHTML.

TreeElement only accepts HTML (string) as its title, no way to pass in an Element. Is it better to create DOM Elements and then take the topmost outerHTML?

[Alexander Pavlov (apavlov)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
        M       LayoutTests/ChangeLog
        A       LayoutTests/inspector/elements-panel-limited-children-expected.txt
        A       LayoutTests/inspector/elements-panel-limited-children.html
        M       LayoutTests/platform/qt/Skipped
        M       WebCore/ChangeLog
        M       WebCore/inspector/front-end/DOMAgent.js
        M       WebCore/inspector/front-end/ElementsPanel.js
        M       WebCore/inspector/front-end/ElementsTreeOutline.js
        M       WebCore/inspector/front-end/inspector.css
Committed r54599

[Alexander Pavlov (apavlov)]

Rolled back due to broken tests by [e-mail]

[Alexander Pavlov (apavlov)]

Created attachment 48579 [details]
[PATCH] Fixed subtle DOMNode-related bug that was letting inspector tests down

[Alexander Pavlov (apavlov)]

Created attachment 48746 [details]
[PATCH] Made sure the test passes both on pure WebKit and Chromium

[Pavel Feldman]

Comment on attachment 48746 [details]
[PATCH] Made sure the test passes both on pure WebKit and Chromium

r=me

[Pavel Feldman]

Comment on attachment 48746 [details]
[PATCH] Made sure the test passes both on pure WebKit and Chromium

There is a number of setTimeout(..., 0) calls that make me think that we don't really know what's going on and are reducing flakiness.
Lets address this before landing. Otherwise looks good.

[Pavel Feldman]

(In reply to comment #22)
> (From update of attachment 48746 [details])
> There is a number of setTimeout(..., 0) calls that make me think that we don't
> really know what's going on and are reducing flakiness.
> Lets address this before landing. Otherwise looks good.

So it looks like one of them can be removed, while another has a solid justification. Please comment on it and feel free to land.

[Alexander Pavlov (apavlov)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
        M       LayoutTests/ChangeLog
        A       LayoutTests/inspector/elements-panel-limited-children-expected.txt
        A       LayoutTests/inspector/elements-panel-limited-children.html
        M       WebCore/ChangeLog
        M       WebCore/English.lproj/localizedStrings.js
        M       WebCore/inspector/front-end/DOMAgent.js
        M       WebCore/inspector/front-end/ElementsPanel.js
        M       WebCore/inspector/front-end/ElementsTreeOutline.js
        M       WebCore/inspector/front-end/inspector.css
Committed r54819

[Alexander Pavlov (apavlov)]

*** Bug 28452 has been marked as a duplicate of this bug. ***