Bug 109832 - Web Inspector: Move profiler tools into separate panels
Summary: Web Inspector: Move profiler tools into separate panels
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-14 07:40 PST by Alexei Filippov
Modified: 2013-02-28 06:45 PST (History)
9 users (show)

See Also:


Attachments
Patch (39.32 KB, patch)
2013-02-14 07:42 PST, Alexei Filippov
no flags Details | Formatted Diff | Diff
Patch (39.31 KB, patch)
2013-02-14 08:39 PST, Alexei Filippov
no flags Details | Formatted Diff | Diff
Patch (23.46 KB, patch)
2013-02-26 10:05 PST, Alexei Filippov
no flags Details | Formatted Diff | Diff
Patch (24.03 KB, patch)
2013-02-28 05:05 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-14 07:40:04 PST
Put each profiler tool into a separate panel.
Comment 1 Alexei Filippov 2013-02-14 07:42:51 PST
Created attachment 188349 [details]
Patch
Comment 2 Alexei Filippov 2013-02-14 08:39:08 PST
Created attachment 188363 [details]
Patch
Comment 3 Pavel Feldman 2013-02-14 09:20:41 PST
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)
Comment 4 Alexei Filippov 2013-02-26 10:05:47 PST
Created attachment 190320 [details]
Patch
Comment 5 Yury Semikhatsky 2013-02-28 03:33:09 PST
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.
Comment 6 Alexei Filippov 2013-02-28 05:03:00 PST
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
Comment 7 Alexei Filippov 2013-02-28 05:05:24 PST
Created attachment 190709 [details]
Patch
Comment 8 WebKit Review Bot 2013-02-28 06:45:44 PST
Comment on attachment 190709 [details]
Patch

Clearing flags on attachment: 190709

Committed r144304: <http://trac.webkit.org/changeset/144304>
Comment 9 WebKit Review Bot 2013-02-28 06:45:48 PST
All reviewed patches have been landed.  Closing bug.