| Summary: | Web Inspector: [Chromium] Landing detailed heap snapshots, part 1 | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Mikhail Naganov <mnaganov> | ||||||||
| Component: | Web Inspector (Deprecated) | Assignee: | Mikhail Naganov <mnaganov> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | abarth, apavlov, bweinstein, eric, 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: | 50510 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Mikhail Naganov
2011-01-26 08:43:21 PST
Created attachment 80198 [details]
patch
Created attachment 80645 [details]
rebased
Comment on attachment 80645 [details] rebased View in context: https://bugs.webkit.org/attachment.cgi?id=80645&action=review > Source/WebCore/ChangeLog:5 > + Web Inspector: [Chromium] Landing detailed heap snapshots, part 1. ChangeLog structure should be: Web Inspector: <foo> http:// bug Detailed description. ... > Source/WebCore/ChangeLog:19 > + (WebInspector.HeapSnapshotEdgeWrapper): No need to enumerate all the new methods. > Source/WebCore/inspector/front-end/HeapSnapshot.js:50 > +WebInspector.RetainersSlice = function(snapshot, start, end) NodesSlive and RetainersSlice can inherit from single class. > Source/WebCore/inspector/front-end/HeapSnapshot.js:76 > +WebInspector.HeapSnapshotEdgeWrapper.prototype = { I like iterator name better. > Source/WebCore/inspector/front-end/HeapSnapshot.js:77 > + becomeFirst: function() reset: function > Source/WebCore/inspector/front-end/HeapSnapshot.js:82 > + becomeNext: function() next: function() > Source/WebCore/inspector/front-end/HeapSnapshot.js:92 > + get count() length > Source/WebCore/inspector/front-end/HeapSnapshot.js:102 > + get hasNext() hasNext: function() > Source/WebCore/inspector/front-end/HeapSnapshot.js:184 > + return "???"; Should this string be localized? > Source/WebCore/inspector/front-end/HeapSnapshot.js:221 > + becomeDominator: function() ditto to the naming. Comment on attachment 80645 [details] rebased View in context: https://bugs.webkit.org/attachment.cgi?id=80645&action=review >> Source/WebCore/ChangeLog:5 >> + Web Inspector: [Chromium] Landing detailed heap snapshots, part 1. > > ChangeLog structure should be: > Web Inspector: <foo> > http:// bug > > Detailed description. > > ... Changed. >> Source/WebCore/ChangeLog:19 >> + (WebInspector.HeapSnapshotEdgeWrapper): > > No need to enumerate all the new methods. Fixed. >> Source/WebCore/inspector/front-end/HeapSnapshot.js:50 >> +WebInspector.RetainersSlice = function(snapshot, start, end) > > NodesSlive and RetainersSlice can inherit from single class. Merged into a single class that receives array name as a constructor argument. >> Source/WebCore/inspector/front-end/HeapSnapshot.js:76 >> +WebInspector.HeapSnapshotEdgeWrapper.prototype = { > > I like iterator name better. Changed. >> Source/WebCore/inspector/front-end/HeapSnapshot.js:77 >> + becomeFirst: function() > > reset: function Changed. >> Source/WebCore/inspector/front-end/HeapSnapshot.js:82 >> + becomeNext: function() > > next: function() Changed. >> Source/WebCore/inspector/front-end/HeapSnapshot.js:92 >> + get count() > > length Changed. >> Source/WebCore/inspector/front-end/HeapSnapshot.js:102 >> + get hasNext() > > hasNext: function() Changed. >> Source/WebCore/inspector/front-end/HeapSnapshot.js:184 >> + return "???"; > > Should this string be localized? Changed string to eliminate the need to localize. >> Source/WebCore/inspector/front-end/HeapSnapshot.js:221 >> + becomeDominator: function() > > ditto to the naming. Fixed. Created attachment 80752 [details]
comments addressed
Comment on attachment 80752 [details] comments addressed View in context: https://bugs.webkit.org/attachment.cgi?id=80752&action=review > Source/WebCore/inspector/front-end/HeapSnapshot.js:683 > + if (fieldName1 === "!edgeName") Please surround with {} > Source/WebCore/inspector/front-end/HeapSnapshot.js:746 > + function(indexA, indexB) { please declare named local function above (as sortByNodeField). Comment on attachment 80752 [details] comments addressed View in context: https://bugs.webkit.org/attachment.cgi?id=80752&action=review >> Source/WebCore/inspector/front-end/HeapSnapshot.js:683 >> + if (fieldName1 === "!edgeName") > > Please surround with {} After declaring order functions separately above, this no longer applies. >> Source/WebCore/inspector/front-end/HeapSnapshot.js:746 > > please declare named local function above (as sortByNodeField). Done. Manually committed http://trac.webkit.org/changeset/77252 2011-02-01 Mikhail Naganov <mnaganov@chromium.org> Reviewed by Pavel Feldman. Web Inspector: [Chromium] Landing detailed heap snapshots, part 1. https://bugs.webkit.org/show_bug.cgi?id=53173 Adding code for accessing heap snapshot data and performing graph calculations. * English.lproj/localizedStrings.js: * inspector/front-end/HeapSnapshot.js: (WebInspector.HeapSnapshotArraySlice): Helper class to avoid array contents copying. (WebInspector.HeapSnapshotEdge): Wrapper for accessing graph edge properties. (WebInspector.HeapSnapshotEdgeIterator): (WebInspector.HeapSnapshotNode): Wrapper for accessing graph node properties. (WebInspector.HeapSnapshotNodeIterator): (WebInspector.HeapSnapshot): Wrapper for the heap snapshot. (WebInspector.HeapSnapshotFilteredOrderedIterator): (WebInspector.HeapSnapshotEdgesProvider): (WebInspector.HeapSnapshotNodesProvider): (WebInspector.HeapSnapshotPathFinder): * inspector/front-end/HeapSnapshotView.js: (WebInspector.HeapSnapshotView.prototype._convertSnapshot): http://trac.webkit.org/changeset/77252 might have broken GTK Linux 32-bit Debug |