Bug 135307 - Web Inspector: Should be a way to go directly from an event in the overview view to the specialized timeline for that event
Summary: Web Inspector: Should be a way to go directly from an event in the overview v...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-07-25 15:19 PDT by Joseph Pecoraro
Modified: 2018-10-16 12:09 PDT (History)
5 users (show)

See Also:


Attachments
Patch (20.28 KB, patch)
2018-10-11 18:57 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (20.58 KB, patch)
2018-10-16 09:40 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (17.34 KB, patch)
2018-10-16 11:44 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2014-07-25 15:19:24 PDT
* SUMMARY
In the generic timeline overview view we see events like Forced Layout, or Mouse Down Event Dispatched. In both cases, to find out more information about those individual events you must switch to the specialized timeline (e.g. Layout duration + backtrace, e.g. JavaScript duration and profiling information). Currently there is no way to go directly from the overview to the deeper information about a particular event.
Comment 1 Radar WebKit Bug Importer 2014-07-25 15:19:37 PDT
<rdar://problem/17816221>
Comment 2 Joseph Pecoraro 2016-07-22 20:02:55 PDT
<rdar://problem/17273966>
Comment 3 Joseph Pecoraro 2016-07-22 20:03:47 PDT
This might also be a good opportunity for a force click on hardware that supports it.
Comment 4 Devin Rousso 2018-10-11 18:57:18 PDT
Created attachment 352122 [details]
Patch
Comment 5 Joseph Pecoraro 2018-10-15 20:13:55 PDT
Comment on attachment 352122 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/TimelineOverview.js:734
> +            if (firstRecord.startTime < this.selectionStartTime || lastRecord.endTime > this.selectionStartTime + this.selectionDuration) {
> +                let selectionPadding = this.secondsPerPixel * 10;

Neat!

> Source/WebInspectorUI/UserInterface/Views/TimelineOverviewGraph.js:40
>          this._selectedRecord = null;
> -        this._selectedRecordChanged = false;
> +        this._selectedRecordBar = null;

It seems weird but possible that the selectedRecord and selectedRecordBar might get out of sync. Can we add an assertion somewhere (such as inside of `set selectedRecord` that the record is inside of the selectedRecordBar if that is available)?

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.css:98
> +.timeline-record-bar:not(.selected).timeline-record-type-network > .segment {

Can the selection colors above just be "!important" and avoid the :not(.selected) classes? I think that is normally how we handle selection styles.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.js:37
> +        this._element.addEventListener("click", this._handleClick.bind(this));

I wonder about the performance (CPU and Memory) different between doing a bind like this versus doing an arrow function. I mused about this once over here:
https://bugs.webkit.org/show_bug.cgi?id=158315

Compare:

    this._element.addEventListener("click", this._handleClick.bind(this));
    this._element.addEventListener("click", (e) => { this._handleClick(e); });

This is in my head because:

    • Function.prototype.bind is a function call (maybe it gets optimized away though?)
    • JSBoundFunction has extra memory (maybe this has been optimized away for no-args as well?)

This might be a fun thing to measure some time in the future. It just always jumps out at me in Timeline code for things we may be creating many many object instances (like this), how much time we are likely spending inside of Function.prototype.bind.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.js:188
> +    set selected(selected)
> +    {
> +        this._element.classList.toggle("selected", !!selected);
> +    }

We should add a getter just for consistency.
Comment 6 Devin Rousso 2018-10-16 09:28:40 PDT
Comment on attachment 352122 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.css:98
>> +.timeline-record-bar:not(.selected).timeline-record-type-network > .segment {
> 
> Can the selection colors above just be "!important" and avoid the :not(.selected) classes? I think that is normally how we handle selection styles.

Really?  Where do we do this?  I personally avoid `!Important` wherever I can as it can really be annoying to deal with if future changes are needed.
Comment 7 Devin Rousso 2018-10-16 09:40:21 PDT
Created attachment 352463 [details]
Patch
Comment 8 Devin Rousso 2018-10-16 11:44:11 PDT
Created attachment 352479 [details]
Patch
Comment 9 WebKit Commit Bot 2018-10-16 12:09:38 PDT
Comment on attachment 352479 [details]
Patch

Clearing flags on attachment: 352479

Committed r237195: <https://trac.webkit.org/changeset/237195>
Comment 10 WebKit Commit Bot 2018-10-16 12:09:40 PDT
All reviewed patches have been landed.  Closing bug.