Bug 200554 – Web Inspector: Elements: Computed: show shorthand properties in addition to longhand ones

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Myles C. Maxfield]

It would be useful, since I haven't memorized exactly which properties are shorthands and which are longhands.

I was recently looking for "border-radius" in the sidebar but didn't find anything.

[Devin Rousso]

Created attachment 377195 [details]
Patch

[Devin Rousso]

Created attachment 377196 [details]
[Image] After Patch is applied

[Devin Rousso]

Created attachment 380784 [details]
Patch

[Devin Rousso]

Created attachment 380785 [details]
[Image] After Patch is applied

[Matt Baker]

(In reply to Devin Rousso from comment #4)
> Created attachment 380785 [details]
> [Image] After Patch is applied

Nice, I like having this as a toggle.

[Matt Baker]

Comment on attachment 380784 [details]
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

[Devin Rousso]

Comment on attachment 380784 [details]
Patch

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

>> 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;

I think this pattern is far more common for lazy-initialization.  I also prefer this approach as it keeps the function "cleaner" in that there's only one `return`.

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:41
>> +        this._showsShorthandsInsteadOfLonghands = false;
> 
> Since this is a concept, it should be singular. Reads better too:
> 
> this._showsShorthandInsteadOfLonghand

If there was only one property per `WI.ComputedStyleSection`, I'd agree (show the shorthand instead of the longhand), but if there are multiple it makes more sense as a plural (show all shorthands instead of all longhands).

[Devin Rousso]

Created attachment 380809 [details]
Patch

Slimmed down this patch to avoid a bunch of unnecessary churn

[WebKit Commit Bot]

Comment on attachment 380809 [details]
Patch

Clearing flags on attachment: 380809

Committed r251038: <https://trac.webkit.org/changeset/251038>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56308811>