Bug 110597 - Web Inspector: move profile type specific code out of ProfilesPanel (refactor)
Summary: Web Inspector: move profile type specific code out of ProfilesPanel (refactor)
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: Alexei Filippov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-22 05:35 PST by Alexei Filippov
Modified: 2013-02-25 02:54 PST (History)
9 users (show)

See Also:


Attachments
Patch (60.62 KB, patch)
2013-02-22 05:44 PST, Alexei Filippov
no flags Details | Formatted Diff | Diff
Patch (61.02 KB, patch)
2013-02-22 09:03 PST, Alexei Filippov
no flags Details | Formatted Diff | Diff
Patch (60.96 KB, patch)
2013-02-25 01:26 PST, Alexei Filippov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexei Filippov 2013-02-22 05:35:44 PST
Move profile type specific code out of ProfilesPanel class.
Comment 1 Alexei Filippov 2013-02-22 05:44:34 PST
Created attachment 189759 [details]
Patch
Comment 2 Yury Semikhatsky 2013-02-22 08:00:07 PST
Comment on attachment 189759 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189759&action=review

> Source/WebCore/inspector/front-end/CPUProfileView.js:678
> +        WebInspector.ProfileType.prototype.removeProfile.call(this, profile);

Do you really need this call of super class?

> Source/WebCore/inspector/front-end/CSSSelectorProfileView.js:-330
> -        profilesPanel.setRecordingProfile(WebInspector.CSSSelectorProfileType.TypeId, true);

Where is it called now?

> Source/WebCore/inspector/front-end/CSSSelectorProfileView.js:-351
> -            profilesPanel.setRecordingProfile(WebInspector.CSSSelectorProfileType.TypeId, false);

Who notifies the panel about recording state change?

> Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:356
> +    {

What if there is no temp profile?

> Source/WebCore/inspector/front-end/ProfilesPanel.js:94
> +    get profiles()

Please use function instead of getter.

> Source/WebCore/inspector/front-end/ProfilesPanel.js:484
> +        console.trace();

Remove this.

> Source/WebCore/inspector/front-end/ProfilesPanel.js:577
> +        profileType.addEventListener(WebInspector.ProfileType.Events.AddProfileHeader, (function(event) { this._addProfileHeader(event.data); }).bind(this));

Please use a method instead of anonymous function.

> Source/WebCore/inspector/front-end/ProfilesPanel.js:988
> +        return this._getProfileTypesArray().reduce(function(result, type) { return result.concat(type.profiles); }, []);

Maybe extract anonymous function and give it a name?

> Source/WebCore/inspector/front-end/ProfilesPanel.js:995
> +        const visibleView = this.visibleView;

I'd rather use a plain old loop.

> Source/WebCore/inspector/front-end/ProfilesPanel.js:-1001
> -        for (var i = 0; i < this._profiles.length; ++i) {

Revert this?

> Source/WebCore/inspector/front-end/ProfilesPanel.js:-1148
> -        this._launcherView.profileStarted();

Now that this code is removed, how does the launcher view know that profiling has started?
Comment 3 Alexei Filippov 2013-02-22 09:03:11 PST
Comment on attachment 189759 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189759&action=review

Thanks for the review. Please take another look.

>> Source/WebCore/inspector/front-end/CPUProfileView.js:678
>> +        WebInspector.ProfileType.prototype.removeProfile.call(this, profile);
> 
> Do you really need this call of super class?

Sure. Base ProfileType manages profile headers collection.

>> Source/WebCore/inspector/front-end/CSSSelectorProfileView.js:-330
>> -        profilesPanel.setRecordingProfile(WebInspector.CSSSelectorProfileType.TypeId, true);
> 
> Where is it called now?

in ProfilesPanel.toggleRecordButton

>> Source/WebCore/inspector/front-end/CSSSelectorProfileView.js:-351
>> -            profilesPanel.setRecordingProfile(WebInspector.CSSSelectorProfileType.TypeId, false);
> 
> Who notifies the panel about recording state change?

ProfilesPanel.toggleRecordButton does it itself.

>> Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:356
>> +    {
> 
> What if there is no temp profile?

This would be an error state. But I can add a check to be on the safe side.

>> Source/WebCore/inspector/front-end/ProfilesPanel.js:94
>> +    get profiles()
> 
> Please use function instead of getter.

done

>> Source/WebCore/inspector/front-end/ProfilesPanel.js:484
>> +        console.trace();
> 
> Remove this.

done

>> Source/WebCore/inspector/front-end/ProfilesPanel.js:577
>> +        profileType.addEventListener(WebInspector.ProfileType.Events.AddProfileHeader, (function(event) { this._addProfileHeader(event.data); }).bind(this));
> 
> Please use a method instead of anonymous function.

done

>> Source/WebCore/inspector/front-end/ProfilesPanel.js:988
>> +        return this._getProfileTypesArray().reduce(function(result, type) { return result.concat(type.profiles); }, []);
> 
> Maybe extract anonymous function and give it a name?

turned into a loop.

>> Source/WebCore/inspector/front-end/ProfilesPanel.js:995
>> +        const visibleView = this.visibleView;
> 
> I'd rather use a plain old loop.

ok.

>> Source/WebCore/inspector/front-end/ProfilesPanel.js:-1001
>> -        for (var i = 0; i < this._profiles.length; ++i) {
> 
> Revert this?

done

>> Source/WebCore/inspector/front-end/ProfilesPanel.js:-1148
>> -        this._launcherView.profileStarted();
> 
> Now that this code is removed, how does the launcher view know that profiling has started?

It is called from ProfilesPanel.toggleRecordButton -> ProfilesPanel.setRecordingProfile
Comment 4 Alexei Filippov 2013-02-22 09:03:35 PST
Created attachment 189778 [details]
Patch
Comment 5 Yury Semikhatsky 2013-02-25 00:48:29 PST
Comment on attachment 189778 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189778&action=review

> Source/WebCore/inspector/front-end/ProfilesPanel.js:575
> +        function onAddProfileHeader(event) {

style: { on the next line

> Source/WebCore/inspector/front-end/ProfilesPanel.js:1007
> +            searchableViews.splice(0, 0, this.visibleView);

searchableViews.unshift(this.visibleView);

BTW, why does visible view have to go first? Maybe just make a swap(0, i) then?

> Source/WebCore/inspector/front-end/ProfilesPanel.js:1042
> +//        this._getAllProfiles().forEach(function(profile) { profile._profilesTreeElement.searchMatches = 0; });

Remove commented code.
Comment 6 Alexei Filippov 2013-02-25 01:16:01 PST
Comment on attachment 189778 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189778&action=review

>> Source/WebCore/inspector/front-end/ProfilesPanel.js:575
>> +        function onAddProfileHeader(event) {
> 
> style: { on the next line

ok

>> Source/WebCore/inspector/front-end/ProfilesPanel.js:1007
>> +            searchableViews.splice(0, 0, this.visibleView);
> 
> searchableViews.unshift(this.visibleView);
> 
> BTW, why does visible view have to go first? Maybe just make a swap(0, i) then?

It was in the original implementation. Probably because it's more convenient to start searching in the opened view first. Changed to swap.

>> Source/WebCore/inspector/front-end/ProfilesPanel.js:1042
>> +//        this._getAllProfiles().forEach(function(profile) { profile._profilesTreeElement.searchMatches = 0; });
> 
> Remove commented code.

ok
Comment 7 Alexei Filippov 2013-02-25 01:26:37 PST
Created attachment 190016 [details]
Patch
Comment 8 WebKit Review Bot 2013-02-25 02:54:51 PST
Comment on attachment 190016 [details]
Patch

Clearing flags on attachment: 190016

Committed r143900: <http://trac.webkit.org/changeset/143900>
Comment 9 WebKit Review Bot 2013-02-25 02:54:54 PST
All reviewed patches have been landed.  Closing bug.