[webkit-reviews] review granted: [Bug 200554] Web Inspector: Elements: Computed: show shorthand properties in addition to longhand ones : [Attachment 380784] Patch

[bugzilla-daemon at webkit.org]

Matt Baker <mattbaker at apple.com> has granted Devin Rousso <drousso at apple.com>'s
request for review:
Bug 200554: Web Inspector: Elements: Computed: show shorthand properties in
addition to longhand ones
https://bugs.webkit.org/show_bug.cgi?id=200554

Attachment 380784: Patch

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




--- Comment #6 from Matt Baker <mattbaker at apple.com> ---
Comment on attachment 380784
  --> https://bugs.webkit.org/attachment.cgi?id=380784
Patch

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

r=me, nice work!

> Source/WebCore/inspector/InspectorStyleSheet.cpp:544
> +    for (auto property : collectProperties(true)) {

auto& will prevent copy constructing InspectorStyleProperty objects.

> Source/WebCore/inspector/InspectorStyleSheet.cpp:581
> +

IMO, separating the return value from the local with a blank line doesn't add
anything.

> Source/WebCore/inspector/InspectorStyleSheet.cpp:616
> +		   continue;

if (m_style->getPropertyValue(name).isEmpty())
    continue;

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:330
> +    get isVariable() { return this._isVariable; }

Nice.

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:391
> +	   if (this._isShorthand === undefined) {

Our usual style is to reduce arrowing with an early return:

if (this._isShorthand !== undefined)
    return this._isShorthand;

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:90
> +	   this._propertiesComputedStyleSection = new
WI.ComputedStyleSection(this);

I'd prefer to leave this as `_computedStyleSection`. I feel like it mostly adds
churn.

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:41
> +	   this._showsShorthandsInsteadOfLonghands = false;

Since this is a concept, it should be singular. Reads better too:

this._showsShorthandInsteadOfLonghand