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
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(); please fix this. 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. Created attachment 141444 [details]
Patch
@loislo, @alexeif, could you look at this? 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 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
Created attachment 141554 [details]
Patch
(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 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> Created attachment 141565 [details]
Patch
(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 on attachment 141565 [details]
Patch
lgtm
Committed r116847: <http://trac.webkit.org/changeset/116847> |