Bug 119652 – Bootstrap canvas profiler

• Component: Web Inspector
• Status: RESOLVED FIXED

[Dean Jackson]

Copy enough of the JS and Selector profilers to get a new profile type and new view in the Timelines panel.

[Radar WebKit Bug Importer]

<rdar://problem/14703665>

[Dean Jackson]

Created attachment 208472 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 208472 [details]
Patch

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

r=me because all my comments are nits. But feel free to post another patch if you want another look.

> Source/WebInspectorUI/ChangeLog:11
> +        * Localizations/en.lproj/localizedStrings.js: Add canvas strings.

Just a heads up in case you did not know, you do not need to edit localizedStrings by hand, you can (and should) just generate it from:
Tools/Scripts/update-webkit-localizable-strings

> Source/WebInspectorUI/UserInterface/CanvasProfileType.js:28
> +/**
> + * @constructor
> + */

You should remove these comments. Since we don't use them right now they take up a lot of space (noise) and are likely to get stale.

> Source/WebInspectorUI/UserInterface/CanvasProfileType.js:33
> +    this._profileUid = 1;

Nit: How about just _profileId. "Uid" doesn't really match up with the rest of the inspector.

> Source/WebInspectorUI/UserInterface/CanvasProfileType.js:39
> +WebInspector.CanvasProfileType.prototype = {

Style: The first property inside of prototype should be the constructor:

    constructor: WebInspector.CanvasProfileType,

> Source/WebInspectorUI/UserInterface/CanvasProfileType.js:94
> +        callback(null, { data: [] });

Style: Object literal style in the Inspector is:

    var obj = {key: value, key2: [1, 2, 3]}

So here:

    {data: []}

> Source/WebInspectorUI/UserInterface/CanvasProfileView.js:26
> +WebInspector.CanvasDataGridNode = function(profileView, data)

Nit: We prefer to give each class its own file. That simplifies looking for the file that defines the class. Please make a CanvasDataGridNode.js for this class. (I realize the others don't at this point)

> Source/WebInspectorUI/UserInterface/CanvasProfileView.js:32
> +WebInspector.CanvasDataGridNode.prototype = {

Nit: constructor line

> Source/WebInspectorUI/UserInterface/CanvasProfileView.js:57
> +    _linkifyLocation: function(url, lineNumber)

Gosh it would be really, really, great if we could throw in a columnNumber here. Maybe the backend doesn't support it YET, but our goal is to always include line and column number when possible. Hopefully there already is a FIXME in the backend, but right here maybe you could pass this.rawData.columnNumber and || 0 either here or in _linkifyLocation.

This is important when working with minified source code. If all we ever know is to jump to line 1, that is not useful. Especially if we've pretty printed the source code and line 1 contains nothing. If we know the original line and column we know exactly where to jump, even in pretty printed code, and we can give the user exactly what they want.

> Source/WebInspectorUI/UserInterface/CanvasProfileView.js:83
> +        return WebInspector.UIString("Recording Canvas Profile\u2026");

Nit: Because we auto generate the strings file, you could just put the … here. I realize the super class has \u, but it is unnecessary.

> Source/WebInspectorUI/UserInterface/CanvasProfileView.js:88
> +        var columns = { "number": { title: "#", width: "5%", sortable: false },

Style: I would prefer this be updated to our style. But it is fine to leave this as is. The other files have similar formatted =(.

> Source/WebInspectorUI/UserInterface/CanvasProfileView.js:99
> +        this._sortProfile();

Currently all columns are not sorted. So there should be no need for a sortProfile here. Do you expect sort changes? If so, we should listen for the dataGrid's WebInspector.DataGrid.Event.SortChanged event. If not, you can remove this and the sort function below.

> Source/WebInspectorUI/UserInterface/CanvasProfileView.js:141
> +

Style: remove this empty line

> Source/WebInspectorUI/UserInterface/ProfileManager.js:153
> +            this._recordingCanvasProfile.recording = false;

This line should not need to be here. stopRecordingProfile sets this._recording = false. THe others do this, but I suspect they don't need to as well =(.

[Dean Jackson]

Comment on attachment 208472 [details]
Patch

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

Thanks Joe. I updated everything but have one comment.

>> Source/WebInspectorUI/UserInterface/ProfileManager.js:153
>> +            this._recordingCanvasProfile.recording = false;
> 
> This line should not need to be here. stopRecordingProfile sets this._recording = false. THe others do this, but I suspect they don't need to as well =(.

I had to leave this in. _recordingCanvasProfile is a CanvasProfileObject, not a CanvasProfileType. It's the latter that sets its _recording property to false in stopRecordingProfile.

[Dean Jackson]

Committed r153928: <http://trac.webkit.org/changeset/153928>