Bug 110597 – Web Inspector: move profile type specific code out of ProfilesPanel (refactor)

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

[Alexei Filippov]

Move profile type specific code out of ProfilesPanel class.

[Alexei Filippov]

Created attachment 189759 [details]
Patch

[Yury Semikhatsky]

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?

[Alexei Filippov]

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

[Alexei Filippov]

Created attachment 189778 [details]
Patch

[Yury Semikhatsky]

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.

[Alexei Filippov]

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

[Alexei Filippov]

Created attachment 190016 [details]
Patch

[WebKit Review Bot]

Comment on attachment 190016 [details]
Patch

Clearing flags on attachment: 190016

Committed r143900: <http://trac.webkit.org/changeset/143900>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.