Bug 176033 – Web Inspector: Styles Redesign: display "Inherited From" section headers

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

In the current styles sidebar UI, we show them as:

    Media: all
    [rules inside of @media all {...}]

    Inherited From: .article
    [.article rule itself]

Bug 175343 misses "Inherited From" and media query section headers.

[Radar WebKit Bug Importer]

<rdar://problem/34116930>

[Nikita Vasilyev]

To manually test this:

1. Open http://nv.github.io/webkit-inspector-bugs/styles-redesign/tests/inherited.html
2. Inspect h3#title

Expected:
In the styles sidebar, you should see sections in the following order:
h3 {...}
h3 🔒 {...}
Inherited From: div.in
.in {...}
div {...}
Inherited From: body.out
.out {...}

[Nikita Vasilyev]

Created attachment 319757 [details]
[Image] WIP

[Nikita Vasilyev]

Created attachment 319957 [details]
Patch

[Nikita Vasilyev]

Created attachment 319960 [details]
[Screenshot] With patch applied

[Devin Rousso]

Comment on attachment 319957 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:53
> +    static createInheritedHeader(style)

Is there a reason for this to be public static?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:108
> +                let inheritedHeader = WI.SpreadsheetRulesStyleDetailsPanel.createInheritedHeader(style);
> +                this.element.append(inheritedHeader);

I don't think this needs to be in it's own variable, especially if you drop the static:

    this.element.appendChild(this._createInheritedHeader(style));

or even better, you could just do the append inside the function and name it something like `_appendInheritedHeader` :)

[Nikita Vasilyev]

Comment on attachment 319957 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:53
>> +    static createInheritedHeader(style)
> 
> Is there a reason for this to be public static?

No reason besides that it can be static since it's a pure function (has no side effects and return value is only determined by its input values).

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:108
>> +                this.element.append(inheritedHeader);
> 
> I don't think this needs to be in it's own variable, especially if you drop the static:
> 
>     this.element.appendChild(this._createInheritedHeader(style));
> 
> or even better, you could just do the append inside the function and name it something like `_appendInheritedHeader` :)

I like

    this.element.appendChild(this._createInheritedHeader(style));

Although _appendInheritedHeader would be a bit more brief, I find it more readable when all the append/addSubview operations are in this loop's body.

[Nikita Vasilyev]

Created attachment 319970 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 319970 [details]
Patch

Clearing flags on attachment: 319970

Committed r221662: <http://trac.webkit.org/changeset/221662>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.