Bug 86230

Summary: Web Inspector: allow showing arbitrary range of nodes in heap snapshot view
Product: WebKit Reporter: Yury Semikhatsky <yurys>
Component: Web Inspector (Deprecated)Assignee: Yury Semikhatsky <yurys>
Status: RESOLVED FIXED    
Severity: Normal CC: alph, apavlov, bweinstein, dglazkov, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 86204    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ec2-cr-linux-03
none
Patch
none
Patch pfeldman: review+

Description Alexei Filippov 2000-12-31 16:05:59 PST
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?
Comment 1 Alexei Filippov 2000-12-31 16:07:23 PST
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.
Comment 2 Yury Semikhatsky 2012-05-11 10:35:53 PDT
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.
Comment 3 Yury Semikhatsky 2012-05-11 10:40:46 PDT
Created attachment 141444 [details]
Patch
Comment 4 Pavel Feldman 2012-05-11 10:47:02 PDT
@loislo, @alexeif, could you look at this?
Comment 5 WebKit Review Bot 2012-05-11 17:21:26 PDT
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
Comment 6 WebKit Review Bot 2012-05-11 17:21:30 PDT
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
Comment 7 Yury Semikhatsky 2012-05-12 00:11:42 PDT
Created attachment 141554 [details]
Patch
Comment 8 Yury Semikhatsky 2012-05-12 00:12:52 PDT
(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.
Comment 9 Ilya Tikhonovsky 2012-05-12 01:51:38 PDT
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>
Comment 10 Yury Semikhatsky 2012-05-12 02:59:42 PDT
Created attachment 141565 [details]
Patch
Comment 11 Yury Semikhatsky 2012-05-12 03:01:32 PDT
(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.
Comment 12 Ilya Tikhonovsky 2012-05-12 03:10:34 PDT
Comment on attachment 141565 [details]
Patch

lgtm
Comment 13 Yury Semikhatsky 2012-05-12 03:16:43 PDT
Committed r116847: <http://trac.webkit.org/changeset/116847>