[webkit-reviews] review denied: [Bug 30332] Refactor ProfilesPanel to support multiple profile types : [Attachment 41100] patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 30332: Refactor ProfilesPanel to support multiple profile types
https://bugs.webkit.org/show_bug.cgi?id=30332

Attachment 41100: patch
https://bugs.webkit.org/attachment.cgi?id=41100&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
I'd suggest that you organize classes related to profile types a bit
differently. I would rename ProfileView.js to CPUProfile.js and put all
CPUProfile-related classes there:
- WebInspector.CPUProfileType (profile type)
- WebInspector.CPUProfile (profile wrapper around the native object)
- WebInspector.CPUProfileView (profile view)

>>     createViewForProfile: function(profile)

Define createViewForProfile on profile itself - it anyway requires profile
instance

>>     buttonClicked: function()

Given that you only expose buttonClicked to the profile implementor, how do you
treat changing the style for toggle buttons? One of the solutions would be to
re-read buttonTooltip, buttonStyle and buttonCaption upon button click.

>> this.registerProfileType(new WebInspector.ProfilesPanel.CPUProfileType());

and

>>     this.registerProfileType(new
WebInspector.ProfilesPanel.HeapSnapshotProfileType());

I think these should go into inspector.js instead. profiles panel should not
really be aware of profile types.

>>  _forEachProfile: function(func)

I don't think you need this - it just iterates over array. Several diffs in
this patch are driven by this change and are not necessary.

>>     this.showProfile(this._profilesIdMap[this._makeKey(uid,
WebInspector.ProfilesPanel.CPUProfileType.TYPE_ID)]);

Can you change signature of showProfileById to showProfile(type, uid) or
showProfile(url) or something to remove the hardcoded CPUProfilerType?

>>    setRecordingProfile: function(isProfiling)

This method should be defined on CPUProfileType only. Make a call straight into
it from inspector.js. You will need to care about updating button caption I was
talking above.

>>    var cpuButton =
this._getButton(WebInspector.ProfilesPanel.CPUProfileType.TYPE_ID);

Why not to iterate over all buttons and disable them?