Bug 176824 – Web Inspector: Timeline should show when events preventDefault() was called on an event or not

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Timeline should show when events preventDefault() was called on an event or not

* Test
<button id="b1">Default Not Prevent</button>
<button id="b2">Default Prevented</button>
<button onclick="sleep(); return false">Default Prevented Two</button>
<script>
function sleep() {
    let now = Date.now();
    while ((Date.now() - now) < 100);
}
document.getElementById("b1").addEventListener("click", (event) => { sleep(); });
document.getElementById("b2").addEventListener("click", (event) => { sleep(); event.preventDefault(); });
</script>

* Steps to reproduce:
1. Inspect test page
2. Start a timeline
3. Click each button
4. Stop recording
  => Should see which events had default prevented

[Joseph Pecoraro]

<rdar://problem/34290931>

[Joseph Pecoraro]

Created attachment 320617 [details]
[PATCH] Proposed Fix - Lacking Tests

[Joseph Pecoraro]

Created attachment 320618 [details]
[IMAGE] Event Display

[Simon Fraser (smfr)]

Noice. No spaces around em dash tho.

[Joseph Pecoraro]

(In reply to Simon Fraser (smfr) from comment #4)
> Noice. No spaces around em dash tho.

I'm use the default styles for subtitles in tree elements.

For example Resources has the same styles for "Name – domain".

[Joseph Pecoraro]

Comment on attachment 320617 [details]
[PATCH] Proposed Fix - Lacking Tests

I'll shortly include a patch with tests.

[Joseph Pecoraro]

Created attachment 320696 [details]
[PATCH] Proposed Fix

Now with tests!

[Joseph Pecoraro]

Comment on attachment 320696 [details]
[PATCH] Proposed Fix

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

> LayoutTests/inspector/timeline/resources/timeline-event-utilities.js:21
> +function finishRecording(data = undefined) {

I suppose at this point the = undefined is redundant.

[Joseph Pecoraro]

Created attachment 320697 [details]
[PATCH] Proposed Fix

[Devin Rousso]

Comment on attachment 320697 [details]
[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?

[Joseph Pecoraro]

Comment on attachment 320697 [details]
[PATCH] Proposed Fix

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

>> LayoutTests/inspector/timeline/resources/timeline-event-utilities.js:10
>> +        InspectorTest.evaluateInPage(expression);
> 
> I would prefer you call this after adding the listener.

I mostly find that approach harder to read. In practice these commands trigger async operations and are serial and so the events below cannot happen before. So I like to write it this way to logically group things. If we somehow rewrote tests to use a synchronous backend we'd encounter all kinds of issues.

>> LayoutTests/inspector/timeline/timeline-event-TimerFire.html:20
>> +            clearTimeout(setIntervalIdentifier);
> 
> NIT: for consistency, I think `clearInterval(setIntervalIdentifier);` is preferred.

Hah, I can't believe I totally forgot about `clearInterval`. Done!

>> Source/WebInspectorUI/UserInterface/Test/Test.js:42
>> +    InspectorBackend.registerDOMDispatcher(new WI.DOMObserver);
> 
> Why did this move?

To match the order of operations in Main.js. It isn't important, just consistency.

[Joseph Pecoraro]

Created attachment 320724 [details]
[PATCH] For Landing

[WebKit Commit Bot]

Comment on attachment 320724 [details]
[PATCH] For Landing

Clearing flags on attachment: 320724

Committed r222015: <http://trac.webkit.org/changeset/222015>

[Ryan Haddad]

(In reply to WebKit Commit Bot from comment #13)
> Comment on attachment 320724 [details]
> [PATCH] For Landing
> 
> Clearing flags on attachment: 320724
> 
> Committed r222015: <http://trac.webkit.org/changeset/222015>
Three of the tests added with this change are failing on macOS Debug WK1 bots:

inspector/timeline/timeline-event-FireAnimationFrame.html
inspector/timeline/timeline-event-TimerFire.html
inspector/timeline/timeline-event-TimerRemove.html

https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK1%20(Tests)/r222016%20(3594)/results.html

[Ryan Haddad]

Actually, it seems like these tests are flaky failures on WK2 as well:
https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r222016%20(3074)/results.html

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Ftimeline%2Ftimeline-event-

[Ryan Haddad]

Reverted r222015 for reason:

The LayoutTests added with this change are flaky.

Committed r222030: <http://trac.webkit.org/changeset/222030>

[Joseph Pecoraro]

Okay, I will avoid adding the Timer tests to this patch (they are kind of unrelated anyways) and investigate them separately. It doesn't appear the EventDispatch test was flakey like the timer ones.

[Joseph Pecoraro]

Created attachment 320789 [details]
[PATCH] For Landing

[WebKit Commit Bot]

Comment on attachment 320789 [details]
[PATCH] For Landing

Clearing flags on attachment: 320789

Committed r222036: <http://trac.webkit.org/changeset/222036>

[Matt Lewis]

After the second patch the test inspector/timeline/timeline-event-EventDispatch.html is still very flaky.

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Ftimeline%2Ftimeline-event-EventDispatch.html

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Ftimeline%2Ftimeline-event-EventDispatch.html

[Joseph Pecoraro]

Rolled out in: https://trac.webkit.org/changeset/222064/webkit

It appears we have an existing bug in Timelines, that will have to be hunted down in order to test this.

Likewise it may be more common in Debug, since running this test 50 times in release didn't exhibit any issue on my machines before I submitted it.

[Devin Rousso]

Created attachment 364999 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 364999 [details]
Patch

Rejecting attachment 364999 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 364999, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=364999&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=176824&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Processing patch 364999 from bug 176824.
Fetching: https://bugs.webkit.org/attachment.cgi?id=364999
Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Timothy Hatcher']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Parsed 29 diffs from patch file(s).
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebInspectorUI/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/dom/EventTarget.cpp
patching file Source/WebCore/inspector/InspectorInstrumentation.cpp
Hunk #1 succeeded at 413 with fuzz 1 (offset 2 lines).
Hunk #2 succeeded at 431 (offset 2 lines).
patching file Source/WebCore/inspector/InspectorInstrumentation.h
Hunk #1 FAILED at 152.
Hunk #2 succeeded at 347 with fuzz 2.
Hunk #3 succeeded at 793 with fuzz 1.
1 out of 4 hunks FAILED -- saving rejects to file Source/WebCore/inspector/InspectorInstrumentation.h.rej
patching file Source/WebCore/inspector/agents/InspectorTimelineAgent.cpp
Hunk #1 succeeded at 306 (offset -3 lines).
patching file Source/WebCore/inspector/agents/InspectorTimelineAgent.h
Hunk #1 succeeded at 115 (offset -6 lines).
patching file Source/WebCore/page/DOMWindow.cpp
Hunk #1 succeeded at 2109 (offset 3 lines).
patching file Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js
Hunk #1 succeeded at 1178 (offset 8 lines).
patching file Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js
patching file Source/WebInspectorUI/UserInterface/Models/ScriptTimelineRecord.js
patching file Source/WebInspectorUI/UserInterface/Views/ScriptDetailsTimelineView.js
patching file Source/WebInspectorUI/UserInterface/Views/ScriptTimelineDataGridNode.js
Hunk #1 succeeded at 86 (offset -14 lines).
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/inspector/timeline/resources/timeline-event-utilities.js
patching file LayoutTests/inspector/timeline/timeline-event-CancelAnimationFrame-expected.txt
patching file LayoutTests/inspector/timeline/timeline-event-CancelAnimationFrame.html
patching file LayoutTests/inspector/timeline/timeline-event-EventDispatch-expected.txt
patching file LayoutTests/inspector/timeline/timeline-event-EventDispatch.html
patching file LayoutTests/inspector/timeline/timeline-event-FireAnimationFrame-expected.txt
patching file LayoutTests/inspector/timeline/timeline-event-FireAnimationFrame.html
patching file LayoutTests/inspector/timeline/timeline-event-RequestAnimationFrame-expected.txt
patching file LayoutTests/inspector/timeline/timeline-event-RequestAnimationFrame.html
patching file LayoutTests/inspector/timeline/timeline-event-TimerFire-expected.txt
patching file LayoutTests/inspector/timeline/timeline-event-TimerFire.html
patching file LayoutTests/inspector/timeline/timeline-event-TimerInstall-expected.txt
patching file LayoutTests/inspector/timeline/timeline-event-TimerInstall.html
patching file LayoutTests/inspector/timeline/timeline-event-TimerRemove-expected.txt
patching file LayoutTests/inspector/timeline/timeline-event-TimerRemove.html

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Timothy Hatcher']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: https://webkit-queues.webkit.org/results/11583606

[Devin Rousso]

Created attachment 365390 [details]
Patch

[Simon Fraser (smfr)]

Nice!

[WebKit Commit Bot]

Comment on attachment 365390 [details]
Patch

Clearing flags on attachment: 365390

Committed r243269: <https://trac.webkit.org/changeset/243269>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

Looks like all of the tests added in https://trac.webkit.org/changeset/243269/webkit

are flaky:
inspector/timeline/timeline-event-CancelAnimationFrame.html
inspector/timeline/timeline-event-EventDispatch.html
inspector/timeline/timeline-event-FireAnimationFrame.html
inspector/timeline/timeline-event-RequestAnimationFrame.html
inspector/timeline/timeline-event-TimerFire.html
inspector/timeline/timeline-event-TimerInstall.html
inspector/timeline/timeline-event-TimerRemove.html

Combined History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Ftimeline%2Ftimeline-event-CancelAnimationFrame.html%20inspector%2Ftimeline%2Ftimeline-event-EventDispatch.html%20inspector%2Ftimeline%2Ftimeline-event-FireAnimationFrame.html%20inspector%2Ftimeline%2Ftimeline-event-RequestAnimationFrame.html%20inspector%2Ftimeline%2Ftimeline-event-TimerFire.html%20inspector%2Ftimeline%2Ftimeline-event-TimerInstall.html%20inspector%2Ftimeline%2Ftimeline-event-TimerRemove.html

[Truitt Savell]

Looks like this also broke inspector/timeline/line-column.html again.

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Ftimeline%2Fline-column.html

[Devin Rousso]

(In reply to Truitt Savell from comment #28)
> Looks like all of the tests added in
> https://trac.webkit.org/changeset/243269/webkit
> 
> are flaky:
> inspector/timeline/timeline-event-CancelAnimationFrame.html
> inspector/timeline/timeline-event-EventDispatch.html
> inspector/timeline/timeline-event-FireAnimationFrame.html
> inspector/timeline/timeline-event-RequestAnimationFrame.html
> inspector/timeline/timeline-event-TimerFire.html
> inspector/timeline/timeline-event-TimerInstall.html
> inspector/timeline/timeline-event-TimerRemove.html
> 
> Combined History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=inspector%2Ftimeline%2Ftimeline-event-
> CancelAnimationFrame.html%20inspector%2Ftimeline%2Ftimeline-event-
> EventDispatch.html%20inspector%2Ftimeline%2Ftimeline-event-
> FireAnimationFrame.html%20inspector%2Ftimeline%2Ftimeline-event-
> RequestAnimationFrame.html%20inspector%2Ftimeline%2Ftimeline-event-TimerFire.
> html%20inspector%2Ftimeline%2Ftimeline-event-TimerInstall.
> html%20inspector%2Ftimeline%2Ftimeline-event-TimerRemove.html

(In reply to Truitt Savell from comment #29)
> Looks like this also broke inspector/timeline/line-column.html again.
> 
> History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=inspector%2Ftimeline%2Fline-column.html

(╯°□°）╯︵ ┻━┻

[Devin Rousso]

(In reply to Truitt Savell from comment #29)
> Looks like this also broke inspector/timeline/line-column.html again.
> 
> History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=inspector%2Ftimeline%2Fline-column.html
Fixed in r243317 <https://trac.webkit.org/changeset/243317>.

Still investigating the other failures.

[Devin Rousso]

(In reply to Truitt Savell from comment #28)
> Looks like all of the tests added in
> https://trac.webkit.org/changeset/243269/webkit
> 
> are flaky:
> inspector/timeline/timeline-event-CancelAnimationFrame.html
> inspector/timeline/timeline-event-EventDispatch.html
> inspector/timeline/timeline-event-FireAnimationFrame.html
> inspector/timeline/timeline-event-RequestAnimationFrame.html
> inspector/timeline/timeline-event-TimerFire.html
> inspector/timeline/timeline-event-TimerInstall.html
> inspector/timeline/timeline-event-TimerRemove.html
> 
> Combined History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=inspector%2Ftimeline%2Ftimeline-event-
> CancelAnimationFrame.html%20inspector%2Ftimeline%2Ftimeline-event-
> EventDispatch.html%20inspector%2Ftimeline%2Ftimeline-event-
> FireAnimationFrame.html%20inspector%2Ftimeline%2Ftimeline-event-
> RequestAnimationFrame.html%20inspector%2Ftimeline%2Ftimeline-event-TimerFire.
> html%20inspector%2Ftimeline%2Ftimeline-event-TimerInstall.
> html%20inspector%2Ftimeline%2Ftimeline-event-TimerRemove.html
Fixed in r243405 <http://trac.webkit.org/changeset/243405>.