Bug 195085

Summary: Web Inspector: CPU Usage Timeline - Make Threads section expandable / collapsable
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: drousso, inspector-bugzilla-changes, joepeck, mattbaker, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 194455    
Attachments:
Description Flags
[PATCH] Proposed Fix
joepeck: commit-queue-
[IMAGE] Expandable / Collapsable
none
[PATCH] Proposed Fix mattbaker: review+

Description Joseph Pecoraro 2019-02-26 18:38:06 PST
CPU Usage Timeline - Make Threads section expandable / collapsable

To make space for more stuff below it
Comment 1 Joseph Pecoraro 2019-02-26 18:44:57 PST
Created attachment 363052 [details]
[PATCH] Proposed Fix
Comment 2 Joseph Pecoraro 2019-02-26 18:46:41 PST
Created attachment 363053 [details]
[IMAGE] Expandable / Collapsable
Comment 3 Joseph Pecoraro 2019-02-27 20:06:12 PST
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.
Comment 4 Joseph Pecoraro 2019-02-27 23:28:08 PST
Created attachment 363195 [details]
[PATCH] Proposed Fix
Comment 5 Matt Baker 2019-02-28 00:15:44 PST
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>.
Comment 6 Devin Rousso 2019-02-28 11:35:49 PST
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.
Comment 7 Matt Baker 2019-02-28 12:14:43 PST
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.
Comment 8 Joseph Pecoraro 2019-02-28 16:22:23 PST
(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.
Comment 9 Joseph Pecoraro 2019-02-28 16:48:49 PST
https://trac.webkit.org/r242242
Comment 10 Radar WebKit Bug Importer 2019-02-28 16:49:36 PST
<rdar://problem/48494376>