[webkit-reviews] review granted: [Bug 195319] Web Inspector: TimelineOverview clicks do not always behave as expected : [Attachment 363785] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 195319: Web Inspector: TimelineOverview clicks do not always behave as
expected
https://bugs.webkit.org/show_bug.cgi?id=195319

Attachment 363785: [PATCH] Proposed Fix

https://bugs.webkit.org/attachment.cgi?id=363785&action=review




--- Comment #8 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 363785
  --> https://bugs.webkit.org/attachment.cgi?id=363785
[PATCH] Proposed Fix

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

r=me

> Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:755
> +	   let pixels = Math.abs(event.pageX - this._mouseStartX);
> +	   if (pixels <= 4)

NIT: this is obvious enough that it is a pixel value, so I'd just inline it.

> Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:780
> +	       if (newEvent.__timelineRecordBarClick)
> +		   event.stop();

I don't think we really want to be calling `stop`.  I think we should instead
always be setting `__timelineRecordBarClick` on `newEvent` before it is
dispatched if `event. __timelineRecordBarClick` had already been set.  Calling
`event.stop()` won't prevent us from iterating over `elements` and dispatching,
meaning that each `newEvent` won't have the property set.  `dispatchEvent`
doesn't propagate the event up the DOM, which is why we use this loop (which
basically emulates it).  Worst case, we set an extra property on an event that
is never used.

For what it's worth, the only place that I've even been able to get this event
to fire is to click on empty space or one of the records that is "behind" the
ruler (e.g. CPU and Memory timelines).	I know you added support for clicking
CPU "columns" in <https://webkit.org/b/195321>, so is this directly to support
that, or is it even necessary once that lands with the `z-index` change?