[webkit-reviews] review denied: [Bug 138941] Web Inspector: create canvas content view and details sidebar panel : [Attachment 313186] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 138941: Web Inspector: create canvas content view and details sidebar panel
https://bugs.webkit.org/show_bug.cgi?id=138941

Attachment 313186: Patch

https://bugs.webkit.org/attachment.cgi?id=313186&action=review




--- Comment #15 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 313186
  --> https://bugs.webkit.org/attachment.cgi?id=313186
Patch

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

> LayoutTests/inspector/canvas/requestContent-expected.txt:10
>
+data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAASwAAACWCAYAAABkW7XSAAAAAXNSR0IA
rs4c6QAAAylJREFUeAHt0DEBAAAAwqD1T20IX4hAYcCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMC
AAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDB
gwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAA
QMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgw
YMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQM
GDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYM
CAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGD
BgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCA
AQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBg
wYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQ
MGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwY
MCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMG
DBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMC
AAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYOAdGL/UAAEPpnR6AAAAAE
lFTkSuQmCC

Maybe create some content that isn't an empty block? What is this, just a
transparent 300x150 box? How about a green box?

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:122
> +	   ExceptionOr<String> result =
canvasEntry->element->toDataURL("image/png");

Nit: ASCIILiteral("image/png")

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:264
> -    if (!canvasEntry.cssCanvasName.isEmpty())
> +    if (canvasEntry.cssCanvasName.isEmpty()) {

The other order seems more readable:

    if ( has css name )
	set canvas name
    else {
	find node id
	set node id
    }

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:150
> +localizedStrings["Canvas #%s"] = "Canvas #%s";

I worry that Localizers will treat this "#" as a Number sign, but its actually
there for an CSS ID selector. I think we should add the "#" ourselves and just
use: "Canvas %s".

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:916
> +localizedStrings["WebGL"] = "WebGL";

Hmm, not sure we want to localize this.

> Source/WebInspectorUI/UserInterface/Base/Main.js:1135
> +	   || (representedObject instanceof WebInspector.Canvas &&
WebInspector.settings.experimentalShowCanvasContextsInResources.value))

I don't think you need the experimental check here.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:100
> +	   if (this._domNode) {
> +	       let id = this._domNode.getAttribute("id");
> +	       if (id)
> +		   return WebInspector.UIString("Canvas #%s").format(id);
> +	   }

Yeah, I think this should be:

    if (this._domNode) {
	let idSelector = this._domNode.escapedIdSelector;
	if (idSelector)
	    return WebInspector.UIString("Canvas %s").format(idSelector);
    }

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:113
> +    _toggleImageGrid()

This is very confusingly named as it doesn't toggle anything! How about
_updateImageGrid.

> Source/WebInspectorUI/UserInterface/Views/CanvasDetailsSidebarPanel.js:50
> +    get canvas() { return this._canvas; }

Nit: Don't oneline this given related code.

> Source/WebInspectorUI/UserInterface/Views/CanvasDetailsSidebarPanel.js:54
> +	   this._canvas = canvas || null;

If this._canvas is not changing we should bail.
If this._canvas is changing I think we will have to clear our this._node and
remove its listeners, otherwise we hold onto _node longer than necessary (for
example if we navigated the inspected page).

> Source/WebInspectorUI/UserInterface/Views/CanvasDetailsSidebarPanel.js:170
> +	       if (!this._canvas.cssCanvasName && !this._node.parentNode)
> +		   this._offscreenRow.value = WebInspector.UIString("Yes");

I'm not sure this will meet everyones expectations. It looks we are saying Yes
if its a CSS -webkit-canvas() or a <canvas> element not immediately in the DOM
Tree.

Someone could interpret Offscreen to mean its just scrolled offscreen.

Likewise someone could have a <canvas> that is not detached from the document
but has a parent node.

I think this could use some refinement, and maybe a tooltip.

> Source/WebInspectorUI/UserInterface/Views/CanvasTreeElement.js:40
> +	   switch (representedObject.contextType) {
> +	   case WebInspector.Canvas.ContextType.Canvas2D:
> +	       classNames.push("canvas-2d");
> +	       break;
> +	   case WebInspector.Canvas.ContextType.WebGL:
> +	       classNames.push("webgl");
> +	       break;
> +	   }

If you converted Canvas.ContextType to be a string instead of a symbol you
could just do:

    let classNames = ["canvas", representedObject.contextType];

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:60
> +	   if (representedObject instanceof WebInspector.Canvas &&
WebInspector.settings.experimentalShowCanvasContextsInResources.value)

I don't think you need the experimental check here.

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:245
> +	   if (representedObject instanceof WebInspector.Canvas &&
WebInspector.settings.experimentalShowCanvasContextsInResources.value)

I don't think you need the experimental check here.

> Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:56
> +	   if
(WebInspector.settings.experimentalShowCanvasContextsInResources.value)

I don't think you need the experimental check here.

> Source/WebInspectorUI/UserInterface/Views/ResourceSidebarPanel.js:456
> +	       || (treeElement instanceof WebInspector.CanvasTreeElement &&
WebInspector.settings.experimentalShowCanvasContextsInResources.value)) {

I don't think you need the experimental check here.

> Source/WebInspectorUI/UserInterface/Views/ResourcesTabContentView.js:72
> +	       || (representedObject instanceof WebInspector.Canvas &&
WebInspector.settings.experimentalShowCanvasContextsInResources.value)

I don't think you need the experimental check here.