[Webkit-unassigned] [Bug 30332] Refactor ProfilesPanel to support multiple profile types

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=30332


Alexander Pavlov (apavlov) <apavlov at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #41159|                            |review?
               Flag|                            |
  Attachment #41100|0                           |1
        is obsolete|                            |




--- Comment #3 from Alexander Pavlov (apavlov) <apavlov at chromium.org>  2009-10-14 06:32:08 PDT ---
Created an attachment (id=41159)
 --> (https://bugs.webkit.org/attachment.cgi?id=41159)
patch (fixed)

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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.