CPU Usage Timeline - Make Threads section expandable / collapsable To make space for more stuff below it
Created attachment 363052 [details] [PATCH] Proposed Fix
Created attachment 363053 [details] [IMAGE] Expandable / Collapsable
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.
Created attachment 363195 [details] [PATCH] Proposed Fix
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 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 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.
(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.
https://trac.webkit.org/r242242
<rdar://problem/48494376>