[webkit-reviews] review denied: [Bug 137278] Web Inspector: add 2D/WebGL canvas instrumentation infrastructure : [Attachment 239375] Patch

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at apple.com> has denied Matt Baker
<mattbaker at apple.com>'s request for review:
Bug 137278: Web Inspector: add 2D/WebGL canvas instrumentation infrastructure
https://bugs.webkit.org/show_bug.cgi?id=137278

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

------- Additional Comments from Timothy Hatcher <timothy at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=239375&action=review


Looking real good. Some more tweaks and it will be ready!

> Source/WebCore/html/HTMLCanvasElement.h:36
> +#include <wtf/HashSet.h>

Not needed now?

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:49
> +    canvasesForFrame: function(frameId, callback)

Spell out frameIdentifier.

This can use promises. The callback system is on its way out. It should clean
up this function a lot!

See:
http://trac.webkit.org/wiki/WebInspectorCodingStyleGuide#UnderstandingandUsingP
romises

Some examples are DebuggerManager.pause and ReplayManager.getSession.

> Source/WebInspectorUI/UserInterface/Views/CanvasTreeElement.js:43
> +WebInspector.CanvasTreeElement.prototype = {
> +    constructor: WebInspector.CanvasTreeElement
> +};
> +
> +WebInspector.CanvasTreeElement.prototype.__proto__ =
WebInspector.GeneralTreeElement.prototype;

__proto__ should move into the block next to constructor property.

> Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:208
> +	       for (var i = 0; i < canvases.length; ++i)
> +		   this._addTreeElementForRepresentedObject(canvases[i]);

Should do:
for (var canvas of canvases)
    this._addTreeElementForRepresentedObject(canvas);

> Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:564
> +	   if (representedObject instanceof WebInspector.ShaderProgram) {
> +	       if (!this._shadersFolderTreeElement)
> +		   this._shadersFolderTreeElement =
createFolderTreeElement.call(this, "shaders",
WebInspector.UIString("Shaders"));
> +	       return this._shadersFolderTreeElement;
> +	   }

Sounds like this will change, since Shaders will go under the owning Canvas.

> Source/WebInspectorUI/UserInterface/Views/ShaderProgramTreeElement.js:41
> +WebInspector.ShaderProgramTreeElement.prototype = {
> +    constructor: WebInspector.ShaderProgramTreeElement
> +};
> +
> +WebInspector.ShaderProgramTreeElement.prototype.__proto__ =
WebInspector.GeneralTreeElement.prototype;

Ditto re: __proto__.