[webkit-reviews] review denied: [Bug 37768] Web Inspector: differentiate element highlight colors for margin and padding : [Attachment 104493] [PATCH] Comments addressed

[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 37768: Web Inspector: differentiate element highlight colors for margin and
padding
https://bugs.webkit.org/show_bug.cgi?id=37768

Attachment 104493: [PATCH] Comments addressed
https://bugs.webkit.org/attachment.cgi?id=104493&action=review

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


> Source/WebCore/inspector/DOMNodeHighlighter.cpp:186
> +	   FontFamily* currFamily = 0;

currentFamily

> Source/WebCore/inspector/DOMNodeHighlighter.h:42
> +struct HighlightColors {

I'd rather make it a struct with non-const fields.

> Source/WebCore/inspector/DOMNodeHighlighter.h:73
> +    const bool hasContent;

Why do you need these?

> Source/WebCore/inspector/Inspector.json:1063
> +		       { "name": "frameId", "type": "string", "description":
"Identifier of the frame to highlight" },

No need to highlight frame's box model, make it similar to rect.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:101
> +Color parseColor(const RefPtr<InspectorObject>* color)

colorObject

> Source/WebCore/inspector/InspectorDOMAgent.cpp:114
> +    bool success = (*color)->getNumber(red, &r);

Just inline "r"

> Source/WebCore/inspector/InspectorDOMAgent.cpp:125
> +    return Color(r, g, b, static_cast<int>(a * 255));

what if a == 5 ?

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1050
> +void InspectorDOMAgent::highlightNode(ErrorString* error, int nodeId, const
RefPtr<InspectorObject>* contentColor, const RefPtr<InspectorObject>*
contentOutlineColor, const RefPtr<InspectorObject>* paddingColor, const
RefPtr<InspectorObject>* paddingOutlineColor, const RefPtr<InspectorObject>*
borderColor, const RefPtr<InspectorObject>* borderOutlineColor, const
RefPtr<InspectorObject>* marginColor, const RefPtr<InspectorObject>*
marginOutlineColor)

Could you format parameters as a column?

> Source/WebCore/inspector/front-end/Color.js:171
> +	   if ("_protocolRGBA" in this)

this._protocolRGBA

> Source/WebCore/inspector/front-end/Color.js:681
> +    Content: new WebInspector.Color("rgba(111, 168, 220, .66)"),

You should define these by components.

> Source/WebCore/inspector/front-end/inspector.js:420
> +	       DOMAgent.highlightNode.apply(DOMAgent, highlightArgs);

Please use new DOMAgent.highlightNode.invoke({}) notation.