Bug 34421 - Web Inspector: Elements Panel: Limit the number of initially loaded element children
: Web Inspector: Elements Panel: Limit the number of initially loaded element c...
Status: RESOLVED FIXED
: WebKit
Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-02-01 05:23 PST by
Modified: 2010-03-09 09:34 PST (History)


Attachments
[PATCH] Suggested solution (20.46 KB, patch)
2010-02-01 09:55 PST, Alexander Pavlov (apavlov)
no flags Review Patch | Details | Formatted Diff | Diff
[PATCH] Added limit checks for insertChild and removeChild (21.70 KB, patch)
2010-02-01 10:17 PST, Alexander Pavlov (apavlov)
pfeldman: review-
Review Patch | Details | Formatted Diff | Diff
[PATCH] Only tree nodes are filtered, as discussed with pfeldman (12.46 KB, patch)
2010-02-05 10:41 PST, Alexander Pavlov (apavlov)
no flags Review Patch | Details | Formatted Diff | Diff
[IMAGE] Looks with the patch. (150.21 KB, image/png)
2010-02-05 13:04 PST, Pavel Feldman
no flags Details
[PATCH] Comments addressed (18.66 KB, patch)
2010-02-08 10:14 PST, Alexander Pavlov (apavlov)
pfeldman: review+
Review Patch | Details | Formatted Diff | Diff
[IMAGE] (Windows) Updated button text/size (104.58 KB, image/png)
2010-02-09 02:44 PST, Alexander Pavlov (apavlov)
no flags Details
[PATCH] Test extended, driveby DOMAgent bug fixed (20.11 KB, patch)
2010-02-10 04:39 PST, Alexander Pavlov (apavlov)
pfeldman: review+
Review Patch | Details | Formatted Diff | Diff
[PATCH] Fixed subtle DOMNode-related bug that was letting inspector tests down (20.98 KB, patch)
2010-02-11 10:07 PST, Alexander Pavlov (apavlov)
no flags Review Patch | Details | Formatted Diff | Diff
[PATCH] Made sure the test passes both on pure WebKit and Chromium (22.28 KB, patch)
2010-02-15 03:40 PST, Alexander Pavlov (apavlov)
pfeldman: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-02-01 05:23:18 PST
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.
------- Comment #1 From 2010-02-01 09:55:25 PST -------
Created an attachment (id=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?)
------- Comment #2 From 2010-02-01 10:00:32 PST -------
Can you attach a screenshot?
------- Comment #3 From 2010-02-01 10:04:02 PST -------
How does this work if the DOM is mutated and we go to update an element that does not exist in the tree?
------- Comment #4 From 2010-02-01 10:17:30 PST -------
Created an attachment (id=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.
------- Comment #5 From 2010-02-01 11:05:01 PST -------
(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.
- 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.
------- Comment #6 From 2010-02-02 02:52:38 PST -------
(In reply to comment #5)
> (From update of attachment 47851 [details] [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...
------- Comment #7 From 2010-02-05 10:41:00 PST -------
Created an attachment (id=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).
------- Comment #8 From 2010-02-05 13:04:15 PST -------
Created an attachment (id=48252) [details]
[IMAGE] Looks with the patch.
------- Comment #9 From 2010-02-05 13:16:06 PST -------
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)"
------- Comment #10 From 2010-02-08 05:13:58 PST -------
(From update of attachment 48240 [details])
> +    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.
------- Comment #11 From 2010-02-08 10:14:34 PST -------
Created an attachment (id=48341) [details]
[PATCH] Comments addressed
------- Comment #12 From 2010-02-08 10:15:20 PST -------
(In reply to comment #10)
> (From update of attachment 48240 [details] [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
------- Comment #13 From 2010-02-09 02:44:53 PST -------
Created an attachment (id=48396) [details]
[IMAGE] (Windows) Updated button text/size
------- Comment #14 From 2010-02-09 05:14:44 PST -------
(From update of attachment 48341 [details])
It would be great to test subsequent calls to unexpanded nodes. Also button is still created using outerHTML.
------- Comment #15 From 2010-02-10 04:39:35 PST -------
Created an attachment (id=48486) [details]
[PATCH] Test extended, driveby DOMAgent bug fixed
------- Comment #16 From 2010-02-10 04:41:50 PST -------
(In reply to comment #14)
> (From update of attachment 48341 [details] [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?
------- Comment #17 From 2010-02-10 07:47:08 PST -------
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
------- Comment #18 From 2010-02-11 09:49:05 PST -------
Rolled back due to broken tests by jorlow@chromium.org
------- Comment #19 From 2010-02-11 10:07:20 PST -------
Created an attachment (id=48579) [details]
[PATCH] Fixed subtle DOMNode-related bug that was letting inspector tests down
------- Comment #20 From 2010-02-15 03:40:28 PST -------
Created an attachment (id=48746) [details]
[PATCH] Made sure teh test passes both on pure WebKit and Chromium
------- Comment #21 From 2010-02-15 09:50:47 PST -------
(From update of attachment 48746 [details])
r=me
------- Comment #22 From 2010-02-16 03:01:08 PST -------
(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.
------- Comment #23 From 2010-02-16 03:02:43 PST -------
(In reply to comment #22)
> (From update of attachment 48746 [details] [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.
------- Comment #24 From 2010-02-16 05:27:52 PST -------
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
------- Comment #25 From 2010-03-09 09:34:49 PST -------
*** Bug 28452 has been marked as a duplicate of this bug. ***