Bug 54385

Summary: Web Inspector: [Chromium] Landing detailed heap snapshots, part 3
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 Flags
patch
pfeldman: review-, mnaganov: commit-queue-
patch pfeldman: review+, mnaganov: commit-queue-

Description Mikhail Naganov 2011-02-14 03:25:32 PST
Provide node classes for data grid views:
 - summary -- list of objects by constuctor;
 - diff -- summaries btw two snapshots;
 - explorer -- explore heap graph;
 - dominator tree;
Comment 1 Mikhail Naganov 2011-02-14 03:28:47 PST
Created attachment 82301 [details]
patch
Comment 2 Pavel Feldman 2011-02-14 09:22:44 PST
Comment on attachment 82301 [details]
patch

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

> Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:34
> +    this.defaultPopulateCount = populateCount;

_defaultPopulateCount

> Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:35
> +    this.provider = null;

this._provider

> Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:53
> +            this.provider.sort(this.comparator());

{ on the next line.

> Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:325
> +    this.isDeletedNode = !!baseSnapshot;

Should be private

> Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:399
> +    this.name = constructor;

Make thing private.

> Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:476
> +    this.it1 = it1;

Make things private please.
Comment 3 Mikhail Naganov 2011-02-14 10:26:00 PST
Comment on attachment 82301 [details]
patch

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

>> Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:34
>> +    this.defaultPopulateCount = populateCount;
> 
> _defaultPopulateCount

Done.

>> Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:35
>> +    this.provider = null;
> 
> this._provider

Done.

>> Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:53
>> +            this.provider.sort(this.comparator());
> 
> { on the next line.

Done.

>> Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:325
>> +    this.isDeletedNode = !!baseSnapshot;
> 
> Should be private

Done.

>> Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:399
>> +    this.name = constructor;
> 
> Make thing private.

Done.

>> Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:476
>> +    this.it1 = it1;
> 
> Make things private please.

Done.
Comment 4 Mikhail Naganov 2011-02-14 10:27:52 PST
Created attachment 82332 [details]
patch
Comment 5 Pavel Feldman 2011-02-16 05:42:00 PST
Comment on attachment 82332 [details]
patch

I am concerned with the amount of the code you are adding. We already show up for too long. Could you please make your code for profiler panel reside in an iframe and load on demand?
Comment 6 Mikhail Naganov 2011-02-16 05:51:43 PST
@pfeldman: OK. I will move it to an iframe.

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


2011-02-14  Mikhail Naganov  <mnaganov@chromium.org>

        Reviewed by Pavel Feldman.

        Web Inspector: [Chromium] Landing detailed heap snapshots, part 3.
        https://bugs.webkit.org/show_bug.cgi?id=54385

        Provide node classes for data grid views:
         - summary -- list of objects by constuctor;
         - diff -- summaries btw two snapshots;
         - explorer -- explore heap graph;
         - dominators tree;

        * WebCore.gypi:
        * WebCore.vcproj/WebCore.vcproj:
        * inspector/front-end/DetailedHeapshotGridNodes.js: Added.
        * inspector/front-end/HeapSnapshot.js:
        (WebInspector.HeapSnapshotFilteredOrderedIterator):
        (WebInspector.HeapSnapshotFilteredOrderedIterator.prototype.incInstancesCount):
        (WebInspector.HeapSnapshotFilteredOrderedIterator.prototype.get instancesCount):
        (WebInspector.HeapSnapshotFilteredOrderedIterator.prototype.next):
        (WebInspector.HeapSnapshotFilteredOrderedIterator.prototype.resetInstancesCount):
        (WebInspector.HeapSnapshotEdgesProvider):
        (WebInspector.HeapSnapshotNodesProvider):
        * inspector/front-end/PleaseWaitMessage.js:
        (WebInspector.PleaseWaitMessage.prototype.get instance):
        * inspector/front-end/WebKit.qrc:
        * inspector/front-end/inspector.html:
Comment 7 WebKit Review Bot 2011-02-16 10:20:29 PST
http://trac.webkit.org/changeset/78700 might have broken GTK Linux 32-bit Release