[webkit-reviews] review granted: [Bug 176824] Web Inspector: Timeline should show when events preventDefault() was called on an event or not : [Attachment 320697] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Devin Rousso <webkit at devinrousso.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 176824: Web Inspector: Timeline should show when events preventDefault()
was called on an event or not
https://bugs.webkit.org/show_bug.cgi?id=176824

Attachment 320697: [PATCH] Proposed Fix

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




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

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

r=me

> LayoutTests/inspector/timeline/resources/timeline-event-utilities.js:8
> +	   InspectorTest.log("Starting Capture...");
> +	   const newRecording = true;

Style: I'd add a newline.

> LayoutTests/inspector/timeline/resources/timeline-event-utilities.js:10
> +	   InspectorTest.evaluateInPage(expression);

I would prefer you call this after adding the listener.

> LayoutTests/inspector/timeline/timeline-event-EventDispatch.html:16
> +    button.dispatchEvent(new MouseEvent("click", {bubbles: true, cancelable:
true}));

Can you just call `button.click();`?

> LayoutTests/inspector/timeline/timeline-event-EventDispatch.html:21
> +    button.dispatchEvent(new MouseEvent("click", {bubbles: true, cancelable:
true}));

Ditto.

> LayoutTests/inspector/timeline/timeline-event-TimerFire.html:20
> +	       clearTimeout(setIntervalIdentifier);

NIT: for consistency, I think `clearInterval(setIntervalIdentifier);` is
preferred.

> LayoutTests/inspector/timeline/timeline-event-TimerInstall.html:20
> +	       clearTimeout(setIntervalIdentifier);

Ditto

> LayoutTests/inspector/timeline/timeline-event-TimerRemove.html:19
> +	       clearTimeout(setIntervalIdentifier);

Ditto

> LayoutTests/inspector/timeline/timeline-event-TimerRemove.html:71
> +	   name: "SanityCheck",

lol

> Source/WebInspectorUI/UserInterface/Test/Test.js:42
> +    InspectorBackend.registerDOMDispatcher(new WI.DOMObserver);

Why did this move?