Bug 54385 - Web Inspector: [Chromium] Landing detailed heap snapshots, part 3
Summary: Web Inspector: [Chromium] Landing detailed heap snapshots, part 3
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Mikhail Naganov
URL:
Keywords:
Depends on:
Blocks: 50510
  Show dependency tree / graph
 
Reported: 2011-02-14 03:25 PST by Mikhail Naganov
Modified: 2011-03-02 02:21 PST (History)
13 users (show)

See Also:

Attachments
patch (34.01 KB, patch)
2011-02-14 03:28 PST, Mikhail Naganov 		pfeldman: review-
mnaganov: commit-queue-
Details | Formatted Diff | Diff
patch (34.16 KB, patch)
2011-02-14 10:27 PST, Mikhail Naganov 		pfeldman: review+
mnaganov: commit-queue-
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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