Bug 86230 – Web Inspector: allow showing arbitrary range of nodes in heap snapshot view

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

[Alexei Filippov]

View in context: https://bugs.webkit.org/attachment.cgi?id=141565&action=review

> Source/WebCore/inspector/front-end/HeapSnapshotProxy.js:127
> +    this._worker =  /*typeof InspectorTest === "undefined" ? new WebInspector.HeapSnapshotRealWorker() :*/ new WebInspector.HeapSnapshotFakeWorker();

leftover?

[Alexei Filippov]

View in context: https://bugs.webkit.org/attachment.cgi?id=141565&action=review

> Source/WebCore/inspector/front-end/HeapSnapshotProxy.js:127
> +    this._worker =  /*typeof InspectorTest === "undefined" ? new WebInspector.HeapSnapshotRealWorker() :*/ new WebInspector.HeapSnapshotFakeWorker();

please fix this.

[Yury Semikhatsky]

Currently it is only possible to expand node children sequentially starting from the first child and then pressing either "Show next X items" or "Show all X items". To implement "reveal in heap snapshot" functionality we need to be able to expand any given range of children.

[Yury Semikhatsky]

Created attachment 141444 [details]
Patch

[Pavel Feldman]

@loislo, @alexeif, could you look at this?

[WebKit Review Bot]

Comment on attachment 141444 [details]
Patch

Attachment 141444 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12667553

New failing tests:
inspector/profiler/heap-snapshot-summary-show-ranges.html

[WebKit Review Bot]

Created attachment 141532 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Yury Semikhatsky]

Created attachment 141554 [details]
Patch

[Yury Semikhatsky]

(In reply to comment #5)
> Created an attachment (id=141554) [details]
> Patch

Fixed test expectations. Skipped the test on platforms that don't support heap profiling. Added more type annotations.

[Ilya Tikhonovsky]

Comment on attachment 141554 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141554&action=review

> Source/WebCore/inspector/front-end/HeapSnapshot.js:1445
> +        if (end >= this._iterationOrder.length)

could you please add a check for 'begin' param

> Source/WebCore/inspector/front-end/HeapSnapshot.js:1469
> +        if (this._sortedPrefixLength >= this._iterationOrder.length)

nit: ===

> Source/WebCore/inspector/front-end/HeapSnapshotGridNodes.js:158
> +            if (!this._positionRanges.length) {

_retrievedChildrenRanges?

> Source/WebCore/inspector/front-end/HeapSnapshotGridNodes.js:161
> +                    this._positionRanges.push({from: 0, to: 0});
> +                    insertShowMoreButton.call(this, 0, items.startPosition, insertionIndex);

comment here?

> Source/WebCore/inspector/front-end/HeapSnapshotGridNodes.js:198
> +                if (!found) {
> +                    // Update previous button.
> +                    this.children[insertionIndex - 1].setEndPosition(items.startPosition);
> +                    insertShowMoreButton.call(this, items.startPosition, items.totalLength, insertionIndex);
> +                    range = {from: items.startPosition, to: items.startPosition};
> +                    rangeIndex = this._positionRanges.length;
> +                    this._positionRanges.push(range);
> +                } else if (items.startPosition < range.from) {
> +                    // Update previous button.
> +                    this.children[insertionIndex - 1].setEndPosition(items.startPosition);
> +                    insertShowMoreButton.call(this, items.startPosition, range.from, insertionIndex);
> +                    range = {from: items.startPosition, to: items.startPosition};
> +                    this._positionRanges.splice(rangeIndex, 0, range);
> +                } else {

copy&paste: the only difference is the update for rangeIndex. splice can be used instead of push.

> Source/WebCore/inspector/front-end/HeapSnapshotGridNodes.js:216
> +                    var newEndOfRange = nextRange ? nextRange.from : items.totalLength;
> +                    if (newEndOfRange > items.endPosition)
> +                        newEndOfRange = items.endPosition;

what about inserting an empty range for the end?

> Source/WebCore/inspector/front-end/HeapSnapshotGridNodes.js:249
> +        setTimeout(serializeNextChunk.bind(this), 0);

it is not clear for me why serializeNextChunk has to be called asynchronously.

> Source/WebCore/inspector/front-end/ShowMoreDataGridNode.js:49
>  
>      this.showNext = document.createElement("button");
>      this.showNext.setAttribute("type", "button");
> -    this.showNext.textContent = WebInspector.UIString("Show next %d", nextCount);
> -    this.showNext.addEventListener("click", populate.bind(this, nextCount), false);
> +    this.showNext.addEventListener("click", this._showNextChunk.bind(this), false);

looks like you have to have three buttons here <prev><all><next>

[Yury Semikhatsky]

Created attachment 141565 [details]
Patch

[Yury Semikhatsky]

(In reply to comment #7)
> (From update of attachment 141554 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141554&action=review
> 
> > Source/WebCore/inspector/front-end/HeapSnapshot.js:1445
> > +        if (end >= this._iterationOrder.length)
> 
> could you please add a check for 'begin' param
> 
Done.

> > Source/WebCore/inspector/front-end/HeapSnapshot.js:1469
> > +        if (this._sortedPrefixLength >= this._iterationOrder.length)
> 
> nit: ===
> 
Done.

> > Source/WebCore/inspector/front-end/HeapSnapshotGridNodes.js:158
> > +            if (!this._positionRanges.length) {
> 
> _retrievedChildrenRanges?
> 
Done.

> > Source/WebCore/inspector/front-end/HeapSnapshotGridNodes.js:161
> > +                    this._positionRanges.push({from: 0, to: 0});
> > +                    insertShowMoreButton.call(this, 0, items.startPosition, insertionIndex);
> 
> comment here?
> 
Done.


> > Source/WebCore/inspector/front-end/HeapSnapshotGridNodes.js:198
> > +                if (!found) {
> > +                    // Update previous button.
> > +                    this.children[insertionIndex - 1].setEndPosition(items.startPosition);
> > +                    insertShowMoreButton.call(this, items.startPosition, items.totalLength, insertionIndex);
> > +                    range = {from: items.startPosition, to: items.startPosition};
> > +                    rangeIndex = this._positionRanges.length;
> > +                    this._positionRanges.push(range);
> > +                } else if (items.startPosition < range.from) {
> > +                    // Update previous button.
> > +                    this.children[insertionIndex - 1].setEndPosition(items.startPosition);
> > +                    insertShowMoreButton.call(this, items.startPosition, range.from, insertionIndex);
> > +                    range = {from: items.startPosition, to: items.startPosition};
> > +                    this._positionRanges.splice(rangeIndex, 0, range);
> > +                } else {
> 
> copy&paste: the only difference is the update for rangeIndex. splice can be used instead of push.
> 
Done. But I'm not sure if the new version is clearer.


> > Source/WebCore/inspector/front-end/HeapSnapshotGridNodes.js:216
> > +                    var newEndOfRange = nextRange ? nextRange.from : items.totalLength;
> > +                    if (newEndOfRange > items.endPosition)
> > +                        newEndOfRange = items.endPosition;
> 
> what about inserting an empty range for the end?
> 
> > Source/WebCore/inspector/front-end/HeapSnapshotGridNodes.js:249
> > +        setTimeout(serializeNextChunk.bind(this), 0);
> 
> it is not clear for me why serializeNextChunk has to be called asynchronously.
> 
I'm not sure as well. I'll fix this in a separate change if you don't mind.

> > Source/WebCore/inspector/front-end/ShowMoreDataGridNode.js:49
> >  
> >      this.showNext = document.createElement("button");
> >      this.showNext.setAttribute("type", "button");
> > -    this.showNext.textContent = WebInspector.UIString("Show next %d", nextCount);
> > -    this.showNext.addEventListener("click", populate.bind(this, nextCount), false);
> > +    this.showNext.addEventListener("click", this._showNextChunk.bind(this), false);
> 
> looks like you have to have three buttons here <prev><all><next>
Agree. Let's do this in a separate change.

[Ilya Tikhonovsky]

Comment on attachment 141565 [details]
Patch

lgtm

[Yury Semikhatsky]

Committed r116847: <http://trac.webkit.org/changeset/116847>