Refactor ProfilesPanel to support multiple profile types

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Alexander Pavlov (apavlov)]

Currently, only CPU profiles are supported in the Profiles panel. It should be possible to plug in other types of profiles.

[Alexander Pavlov (apavlov)]

Created attachment 41100 [details]
patch

[Pavel Feldman]

Comment on attachment 41100 [details]
patch

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?

[Alexander Pavlov (apavlov)]

Created attachment 41159 [details]
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.

[Alexander Pavlov (apavlov)]

Created attachment 41165 [details]
patch (fixed v2)

[Alexander Pavlov (apavlov)]

Comment on attachment 41165 [details]
patch (fixed v2)

Patchfile improperly applied

[Alexander Pavlov (apavlov)]

Created attachment 41166 [details]
patch (fixed v3)

[Timothy Hatcher]

Comment on attachment 41165 [details]
patch (fixed v2)

Why the huge change to WebCore.vcproj?

Also WebInspector.CPUProfileType.TYPE_ID should be fixed. TYPE_ID should be CamelCase.

[Alexander Pavlov (apavlov)]

Created attachment 41168 [details]
patch (fixed case)

[Alexander Pavlov (apavlov)]

(In reply to comment #7)
> (From update of attachment 41165 [details])
> Why the huge change to WebCore.vcproj?
Sorry, it was a result of improper patch application on my side (all CRLF's got converted into LF's)

> Also WebInspector.CPUProfileType.TYPE_ID should be fixed. TYPE_ID should be
> CamelCase.
Fixed.

[Timothy Hatcher]

Comment on attachment 41168 [details]
patch (fixed case)

You should fix the case for WebInspector.ProfileType.URL_REGEXP too.

[Alexander Pavlov (apavlov)]

Created attachment 41169 [details]
patch (fixed URLRegExp case)

[WebKit Commit Bot]

Comment on attachment 41169 [details]
patch (fixed URLRegExp case)

Rejecting patch 41169 from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.

[Pavel Feldman]

I am seeting another bug (regression?):
1. Gather several profiles
2. Close inspector
3. Start/Stop profiling from the developers menu

Actual: previously gathered profiles disappear.

[Alexander Pavlov (apavlov)]

(In reply to comment #13)
> I am seeting another bug (regression?):
> 1. Gather several profiles
> 2. Close inspector
> 3. Start/Stop profiling from the developers menu
> 
> Actual: previously gathered profiles disappear.

This is an unrelated regression (manifests itself on the tip-of-tree).

[Alexander Pavlov (apavlov)]

Created attachment 41215 [details]
patch (r49615 merge)

This patch merges in the r49615 changes

[Alexander Pavlov (apavlov)]

Comment on attachment 41215 [details]
patch (r49615 merge)

Broken due to the r49615 changes

[Alexander Pavlov (apavlov)]

Created attachment 41221 [details]
patch (fixed r49615-related breakage)

[WebKit Commit Bot]

Comment on attachment 41221 [details]
patch (fixed r49615-related breakage)

Clearing flags on attachment: 41221

Committed r49632: <http://trac.webkit.org/changeset/49632>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.