[webkit-reviews] review requested: [Bug 30332] Refactor ProfilesPanel to support multiple profile types : [Attachment 41159] patch (fixed)

[bugzilla-daemon at webkit.org]

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

Attachment 41159: patch (fixed)
https://bugs.webkit.org/attachment.cgi?id=41159&action=review

------- Additional Comments from Alexander Pavlov (apavlov)
<apavlov at chromium.org>
(In reply to comment #2)
> (From update of attachment 41100 [details])
> 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)
Done.

> >>	 createViewForProfile: function(profile)
> 
> Define createViewForProfile on profile itself - it anyway requires profile
> instance
Done.

> >>	 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.
Implemented updateProfileTypeButtons() on ProfilesPanel.

> >> 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.
Moved CPU-related registration, removed Heap-related things altogether.

> >>  _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.
Removed

> >>	 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?
Done.

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

> >>	var cpuButton =
this._getButton(WebInspector.ProfilesPanel.CPUProfileType.TYPE_ID);
> 
> Why not to iterate over all buttons and disable them?
Done.