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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Oct 14 01:15:13 PDT 2009


Pavel Feldman <pfeldman at chromium.org> changed:

           What    |Removed                     |Added
  Attachment #41100|review?                     |review-
               Flag|                            |

--- Comment #2 from Pavel Feldman <pfeldman at chromium.org>  2009-10-14 01:15:13 PDT ---
(From update of attachment 41100)
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

>>     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());


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

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

More information about the webkit-unassigned mailing list