[webkit-reviews] review denied: [Bug 138941] Web Inspector: create canvas content view and details sidebar panel : [Attachment 313186] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jun 19 17:08:54 PDT 2017
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.
More information about the webkit-reviews
mailing list