Bug 109832 – Web Inspector: Move profiler tools into separate panels

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

[Alexei Filippov]

Put each profiler tool into a separate panel.

[Alexei Filippov]

Created attachment 188349 [details]
Patch

[Alexei Filippov]

Created attachment 188363 [details]
Patch

[Pavel Feldman]

Comment on attachment 188363 [details]
Patch

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

> Source/WebCore/inspector/front-end/CPUProfileView.js:578
> +    WebInspector.ProfileType.call(this, WebInspector.CPUProfileType.TypeId, WebInspector.UIString("Collect JavaScript CPU Profile"), panel);

So what is profiles panel now? Making a dependency from profile type descriptor to the UI component does not sound right to me.

> Source/WebCore/inspector/front-end/inspector.js:295
> +            // TODO(alph): route to the appropriate profiler

// FIXME: (with no name)

[Alexei Filippov]

Created attachment 190320 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 190320 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        This is a first part of the fix that puts each profiler tool into a separate panel.

Please provide some details on what's happening in this change.

> Source/WebCore/inspector/front-end/ProfilesPanel.js:339
> +    var singleProfileMode = typeof name !== "undefined";

Can you explain this logic? Also, move this below, no need to have it before super constructor call.

> Source/WebCore/inspector/front-end/ProfilesPanel.js:781
> +                    profile.view(this).dataGrid.highlightObjectByHeapSnapshotId(snapshotObjectId);

You should bind the callback to make sure that this is what you expect it to be.

> Source/WebCore/inspector/front-end/inspector.js:57
> +            if (WebInspector.experimentsSettings.canvasInspection.isEnabled())

You should skip it for workers.

> Source/WebCore/inspector/front-end/inspector.js:59
> +            if (WebInspector.experimentsSettings.nativeMemorySnapshots.isEnabled()) {

You should skip them for workers.

[Alexei Filippov]

Comment on attachment 190320 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        This is a first part of the fix that puts each profiler tool into a separate panel.
> 
> Please provide some details on what's happening in this change.

ok

>> Source/WebCore/inspector/front-end/ProfilesPanel.js:339
>> +    var singleProfileMode = typeof name !== "undefined";
> 
> Can you explain this logic? Also, move this below, no need to have it before super constructor call.

Added a comment. I can't move it below the next line as it changes the name variable.

>> Source/WebCore/inspector/front-end/ProfilesPanel.js:781
>> +                    profile.view(this).dataGrid.highlightObjectByHeapSnapshotId(snapshotObjectId);
> 
> You should bind the callback to make sure that this is what you expect it to be.

done

>> Source/WebCore/inspector/front-end/inspector.js:57
>> +            if (WebInspector.experimentsSettings.canvasInspection.isEnabled())
> 
> You should skip it for workers.

ok

>> Source/WebCore/inspector/front-end/inspector.js:59
>> +            if (WebInspector.experimentsSettings.nativeMemorySnapshots.isEnabled()) {
> 
> You should skip them for workers.

ok

[Alexei Filippov]

Created attachment 190709 [details]
Patch

[WebKit Review Bot]

Comment on attachment 190709 [details]
Patch

Clearing flags on attachment: 190709

Committed r144304: <http://trac.webkit.org/changeset/144304>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.