[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 bugzilla-daemon at webkit.org
Wed Mar 6 13:58:19 PST 2019

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

Attachment 363785: [PATCH] Proposed Fix


--- 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


> 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?

More information about the webkit-reviews mailing list