[webkit-reviews] review denied: [Bug 58992] Web Inspector: Metrics pane editing and visual feedback improvements : [Attachment 90344] [PATCH] Suggested solution

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 58992: Web Inspector: Metrics pane editing and visual feedback improvements
https://bugs.webkit.org/show_bug.cgi?id=58992

Attachment 90344: [PATCH] Suggested solution
https://bugs.webkit.org/attachment.cgi?id=90344&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=90344&action=review

> Source/WebCore/inspector/DOMNodeHighlighter.cpp:94
> +void drawHighlightForBox(GraphicsContext& context, const FloatQuad&
contentQuad, const FloatQuad& paddingQuad, const FloatQuad& borderQuad, const
FloatQuad& marginQuad, const String& mode)

mode -> enum

> Source/WebCore/inspector/Inspector.json:900
> +		       { "name": "mode", "type": "string", "optional": true,
"description": "The box model component(s) to highlight: \"content\",
\"padding\", \"border\", \"margin\", \"all\" (default: \"all\")." },

"enum": ["content", "padding", ...]

> Source/WebCore/inspector/InspectorDOMAgent.h:181
> +    void highlight(ErrorString*, Node*, const String& mode = "all");

Do not use default values unless necessary.

> Source/WebCore/inspector/front-end/MetricsSidebarPane.js:224
> +		   widthElement.addEventListener("click",
this.startEditing.bind(this, widthElement, "width", "width", style), false);

why did this change?

> Source/WebCore/inspector/front-end/MetricsSidebarPane.js:262
> +	   WebInspector.highlightDOMNode(0, "");

mode should be optional.

> Source/WebCore/inspector/front-end/inspector.js:320
> +	       DOMAgent.highlightDOMNode(nodeId, mode);

mode || "all"