Bug 30332 - Refactor ProfilesPanel to support multiple profile types
Summary: Refactor ProfilesPanel to support multiple profile types
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-13 05:46 PDT by Alexander Pavlov (apavlov)
Modified: 2009-10-15 09:50 PDT (History)
3 users (show)

See Also:


Attachments
patch (24.61 KB, patch)
2009-10-13 06:28 PDT, Alexander Pavlov (apavlov)
pfeldman: review-
Details | Formatted Diff | Diff
patch (fixed) (70.29 KB, patch)
2009-10-14 06:32 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
patch (fixed v2) (1.50 MB, patch)
2009-10-14 10:16 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
patch (fixed v3) (24.42 KB, patch)
2009-10-14 10:20 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
patch (fixed case) (24.42 KB, patch)
2009-10-14 10:32 PDT, Alexander Pavlov (apavlov)
timothy: review-
Details | Formatted Diff | Diff
patch (fixed URLRegExp case) (24.42 KB, patch)
2009-10-14 10:57 PDT, Alexander Pavlov (apavlov)
commit-queue: review-
Details | Formatted Diff | Diff
patch (r49615 merge) (24.33 KB, patch)
2009-10-15 04:23 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
patch (fixed r49615-related breakage) (24.75 KB, patch)
2009-10-15 05:15 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Pavlov (apavlov) 2009-10-13 05:46:06 PDT
Currently, only CPU profiles are supported in the Profiles panel. It should be possible to plug in other types of profiles.
Comment 1 Alexander Pavlov (apavlov) 2009-10-13 06:28:02 PDT
Created attachment 41100 [details]
patch
Comment 2 Pavel Feldman 2009-10-14 01:15:13 PDT
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?
Comment 3 Alexander Pavlov (apavlov) 2009-10-14 06:32:08 PDT
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.
Comment 4 Alexander Pavlov (apavlov) 2009-10-14 10:16:43 PDT
Created attachment 41165 [details]
patch (fixed v2)
Comment 5 Alexander Pavlov (apavlov) 2009-10-14 10:18:08 PDT
Comment on attachment 41165 [details]
patch (fixed v2)

Patchfile improperly applied
Comment 6 Alexander Pavlov (apavlov) 2009-10-14 10:20:21 PDT
Created attachment 41166 [details]
patch (fixed v3)
Comment 7 Timothy Hatcher 2009-10-14 10:22:04 PDT
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.
Comment 8 Alexander Pavlov (apavlov) 2009-10-14 10:32:30 PDT
Created attachment 41168 [details]
patch (fixed case)
Comment 9 Alexander Pavlov (apavlov) 2009-10-14 10:33:33 PDT
(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.
Comment 10 Timothy Hatcher 2009-10-14 10:46:39 PDT
Comment on attachment 41168 [details]
patch (fixed case)

You should fix the case for WebInspector.ProfileType.URL_REGEXP too.
Comment 11 Alexander Pavlov (apavlov) 2009-10-14 10:57:55 PDT
Created attachment 41169 [details]
patch (fixed URLRegExp case)
Comment 12 WebKit Commit Bot 2009-10-15 02:10:37 PDT
Comment on attachment 41169 [details]
patch (fixed URLRegExp case)

Rejecting patch 41169 from review queue.

pfeldman@chromium.org does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.
Comment 13 Pavel Feldman 2009-10-15 02:22:36 PDT
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.
Comment 14 Alexander Pavlov (apavlov) 2009-10-15 02:37:00 PDT
(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).
Comment 15 Alexander Pavlov (apavlov) 2009-10-15 04:23:17 PDT
Created attachment 41215 [details]
patch (r49615 merge)

This patch merges in the r49615 changes
Comment 16 Alexander Pavlov (apavlov) 2009-10-15 05:00:15 PDT
Comment on attachment 41215 [details]
patch (r49615 merge)

Broken due to the r49615 changes
Comment 17 Alexander Pavlov (apavlov) 2009-10-15 05:15:01 PDT
Created attachment 41221 [details]
patch (fixed r49615-related breakage)
Comment 18 WebKit Commit Bot 2009-10-15 09:50:33 PDT
Comment on attachment 41221 [details]
patch (fixed r49615-related breakage)

Clearing flags on attachment: 41221

Committed r49632: <http://trac.webkit.org/changeset/49632>
Comment 19 WebKit Commit Bot 2009-10-15 09:50:37 PDT
All reviewed patches have been landed.  Closing bug.