Bug 53173 – Web Inspector: [Chromium] Landing detailed heap snapshots, part 1

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

[Mikhail Naganov]

See https://bugs.webkit.org/show_bug.cgi?id=50510

Adding code for accessing heap snapshot data and performing graph calculations.

[Mikhail Naganov]

Created attachment 80198 [details]
patch

[Mikhail Naganov]

Created attachment 80645 [details]
rebased

[Pavel Feldman]

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.

[Mikhail Naganov]

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.

[Mikhail Naganov]

Created attachment 80752 [details]
comments addressed

[Pavel Feldman]

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).

[Mikhail Naganov]

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.

[Mikhail Naganov]

Manually committed http://trac.webkit.org/changeset/77252


    2011-02-01  Mikhail Naganov  <[e-mail]>
    
            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):

[WebKit Review Bot]

http://trac.webkit.org/changeset/77252 might have broken GTK Linux 32-bit Debug