Bug 195085 – Web Inspector: CPU Usage Timeline - Make Threads section expandable / collapsable

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

[Joseph Pecoraro]

CPU Usage Timeline - Make Threads section expandable / collapsable

To make space for more stuff below it

[Joseph Pecoraro]

Created attachment 363052 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 363053 [details]
[IMAGE] Expandable / Collapsable

[Joseph Pecoraro]

Comment on attachment 363052 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:534
> +                this._detailsContainerElement.insertBefore(workerView.element, this._webkitThreadUsageView.element);

This insertBefore should be: `this._threadsDetailsElement` not `this._detailsContainerElement`.

Rest of the patch should still be good to review.

[Joseph Pecoraro]

Created attachment 363195 [details]
[PATCH] Proposed Fix

[Matt Baker]

Comment on attachment 363195 [details]
[PATCH] Proposed Fix

r=me, very nice! I ran into an assertion, but it looks like it will be fixed by <https://webkit.org/b/195146>.

[Devin Rousso]

Comment on attachment 363195 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:66
> +.timeline-view.cpu > .content > .details > .subtitle,
> +.timeline-view.cpu > .content > .details > details > .subtitle {

Rather than have two `.details` in a row, could we simplify this as `.timeline-view.cpu > .content > .details .subtitle`?

If not, I think the second `.details` should actually be a tag selector `details` (no class ".").

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:71
> +.timeline-view.cpu > .content > .details > details > .subtitle.threads {

Ditto (>65).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:212
> +        this._threadsDetailsElement.addEventListener("toggle", () => {

NIT: I normally still put the `event` in the parameters even if it's not used to help signify that this function is expected to be used as an event handler.

[Matt Baker]

Comment on attachment 363195 [details]
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:212
>> +        this._threadsDetailsElement.addEventListener("toggle", () => {
> 
> NIT: I normally still put the `event` in the parameters even if it's not used to help signify that this function is expected to be used as an event handler.

I like this convention. Same with the first parameter of delegate methods.

[Joseph Pecoraro]

(In reply to Devin Rousso from comment #6)
> Comment on attachment 363195 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363195&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:66
> > +.timeline-view.cpu > .content > .details > .subtitle,
> > +.timeline-view.cpu > .content > .details > details > .subtitle {
> 
> Rather than have two `.details` in a row, could we simplify this as
> `.timeline-view.cpu > .content > .details .subtitle`?
> 
> If not, I think the second `.details` should actually be a tag selector
> `details` (no class ".").

Since subtitle is so commonly used, I want to make sure it is the direct child of these two cases.

[Joseph Pecoraro]

https://trac.webkit.org/r242242

[Radar WebKit Bug Importer]

<rdar://problem/48494376>