[webkit-reviews] review granted: [Bug 44531] Web Inspector: Store heap snapshots in InspectorProfilerAgent : [Attachment 65286] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Aug 25 02:20:50 PDT 2010
Yury Semikhatsky <yurys at chromium.org> has granted Mikhail Naganov
<mnaganov at chromium.org>'s request for review:
Bug 44531: Web Inspector: Store heap snapshots in InspectorProfilerAgent
https://bugs.webkit.org/show_bug.cgi?id=44531
Attachment 65286: Patch
https://bugs.webkit.org/attachment.cgi?id=65286&action=review
------- Additional Comments from Yury Semikhatsky <yurys at chromium.org>
The patch is huge, could you break it down into smaller pieces? Also would be
nice to cover this functionality with at least couple of layout tests.
WebCore/bindings/v8/ScriptHeapSnapshot.cpp:70
+ entry->setString("cons", toWebCoreString(node->GetName()));
What does "cons" stand for? Is it "constructor"? If so, please use full name
for the field.
WebCore/inspector/InspectorProfilerAgent.cpp:70
+ , m_nextUserInitiatedHeapSnapshotNumber(1)
Shouldn't it be static to be unique among all instances of the
InspectorProfilerAgent in the process?
WebKit/chromium/src/js/HeapProfilerPanel.js:119
+ this._loadProfile(this.profile, profileCallback);
We usually call profileCallback.bind(this) instead of using var self = this; in
Web Inspector code.
WebKit/chromium/src/js/HeapProfilerPanel.js:297
+ var self = this;
ditto
WebKit/chromium/src/js/HeapProfilerPanel.js:333
+ var self = this;
dtto
More information about the webkit-reviews
mailing list