[webkit-reviews] review granted: [Bug 119652] Bootstrap canvas profiler : [Attachment 208472] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Dean Jackson
<dino at apple.com>'s request for review:
Bug 119652: Bootstrap canvas profiler
https://bugs.webkit.org/show_bug.cgi?id=119652

Attachment 208472: Patch
https://bugs.webkit.org/attachment.cgi?id=208472&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
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 =(.