LoAF: add support for scripts #124

noamr · 2023-11-29T16:31:45Z

This adds support for reporting scripts:
- The PerformanceScriptTiming IDL
- Algorithms for how script entries are created
- Monkey patches to HTML/DOM/WebIDL, also for reporting rendering/tasks in general.

Preview | Diff

This adds support for reporting scripts: - The PerformanceScriptTiming IDL - Algorithms for how script entries are created - Monkey patches to HTML/DOM/WebIDL

noamr · 2023-11-30T21:56:04Z

There seems to be some CI problem... @yoavweiss do you know how I turn to with this?

(Part of the effort to properly spec LoAF, see w3c/longtasks#124) Bug: 1508308 Change-Id: I2580ac2f93bbb922193fd4ef6738a2b163f1210c

(Part of the effort to properly spec LoAF, see w3c/longtasks#124) Bug: 1508308 Change-Id: I2580ac2f93bbb922193fd4ef6738a2b163f1210c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5089128 Commit-Queue: Noam Rosenthal <nrosenthal@chromium.org> Reviewed-by: Michal Mocny <mmocny@chromium.org> Reviewed-by: Dominic Farolino <dom@chromium.org> Cr-Commit-Position: refs/heads/main@{#1234177}

index.bs

clelland · 2023-12-14T14:29:13Z

index.bs

@@ -331,7 +359,92 @@ The {{PerformanceLongAnimationFrameTiming/blockingDuration}} attribute's getter
    1. If |workDuration| + |renderDuration| is greater than 50, then return |workDuration| + |renderDuration| - 50 milliseconds.
    1. Return 0.

-</div>
+The {{PerformanceLongAnimationFrameTiming/scripts}} attribute's getter steps are:
+    1. Let |scripts| be « ».


I think this should read "Let |scripts| be the [=list=] « »", since the literal syntax can also be used for ordered sets and maps, and when empty, it's impossible to distinguish between them

index.bs

clelland · 2023-12-14T16:18:28Z

index.bs

 urlPrefix: https://tc39.github.io/ecma262/; spec: ECMASCRIPT;
    type: dfn; url: #sec-code-realms; text: JavaScript Realms;
 urlPrefix: https://dom.spec.whatwg.org/; spec: DOM;
    type: attribute; for: Element;
        text: id; url: #dom-element-id;
+urlPrefix: https://webidl.spec.whatwg.org/; spec: WEBIDL;
+    type: dfn; text: identifier; url: #identifier;


I think the url is #dfn-identifier

index.bs

…nStart, a=testonly Automatic update from web-platform-tests LoAF: for no-cors scripts, hide executionStart (Part of the effort to properly spec LoAF, see w3c/longtasks#124) Bug: 1508308 Change-Id: I2580ac2f93bbb922193fd4ef6738a2b163f1210c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5089128 Commit-Queue: Noam Rosenthal <nrosenthal@chromium.org> Reviewed-by: Michal Mocny <mmocny@chromium.org> Reviewed-by: Dominic Farolino <dom@chromium.org> Cr-Commit-Position: refs/heads/main@{#1234177} -- wpt-commits: b675788b7ffa130c95611124da4b4d964e46b23a wpt-pr: 43522

…nStart, a=testonly Automatic update from web-platform-tests LoAF: for no-cors scripts, hide executionStart (Part of the effort to properly spec LoAF, see w3c/longtasks#124) Bug: 1508308 Change-Id: I2580ac2f93bbb922193fd4ef6738a2b163f1210c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5089128 Commit-Queue: Noam Rosenthal <nrosenthalchromium.org> Reviewed-by: Michal Mocny <mmocnychromium.org> Reviewed-by: Dominic Farolino <domchromium.org> Cr-Commit-Position: refs/heads/main{#1234177} -- wpt-commits: b675788b7ffa130c95611124da4b4d964e46b23a wpt-pr: 43522 UltraBlame original commit: 4c2bc6696b4d25598c8b8971b5b8b2ae78e196ad

…nStart, a=testonly Automatic update from web-platform-tests LoAF: for no-cors scripts, hide executionStart (Part of the effort to properly spec LoAF, see w3c/longtasks#124) Bug: 1508308 Change-Id: I2580ac2f93bbb922193fd4ef6738a2b163f1210c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5089128 Commit-Queue: Noam Rosenthal <nrosenthalchromium.org> Reviewed-by: Michal Mocny <mmocnychromium.org> Reviewed-by: Dominic Farolino <domchromium.org> Cr-Commit-Position: refs/heads/main{#1234177} -- wpt-commits: b675788b7ffa130c95611124da4b4d964e46b23a wpt-pr: 43522 UltraBlame original commit: 4c2bc6696b4d25598c8b8971b5b8b2ae78e196ad

…nStart, a=testonly Automatic update from web-platform-tests LoAF: for no-cors scripts, hide executionStart (Part of the effort to properly spec LoAF, see w3c/longtasks#124) Bug: 1508308 Change-Id: I2580ac2f93bbb922193fd4ef6738a2b163f1210c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5089128 Commit-Queue: Noam Rosenthal <nrosenthal@chromium.org> Reviewed-by: Michal Mocny <mmocny@chromium.org> Reviewed-by: Dominic Farolino <dom@chromium.org> Cr-Commit-Position: refs/heads/main@{#1234177} -- wpt-commits: b675788b7ffa130c95611124da4b4d964e46b23a wpt-pr: 43522

noamr · 2024-01-05T09:54:14Z

@clelland @mmocny anything missing? Can we push this forward?

index.bs

clelland · 2024-01-05T16:34:16Z

index.bs

+            1. Let |targetName| be |this|'s [=PerformanceScriptTiming/timing info=]'s [=script timing info/invoker name=].
+            1. If |this|'s [=PerformanceScriptTiming/timing info=]'s [=script timing info/event target element id=] is not the empty string, then:
+                Set |targetName| to the [=concatenate|concatenation=] of « |targetName|, "#", |this|'s [=PerformanceScriptTiming/timing info=]'s [=script timing info/event target element id=] ».
+            1. Else, If |this|'s [=PerformanceScriptTiming/timing info=]'s [=script timing info/event target element src attribute=] is not the empty string, then:


It looks like infra prefers "Otherwise" to "Else"

clelland · 2024-01-05T16:34:43Z

index.bs

+                then:
+                    1. If |this|'s {{PerformanceScriptTiming/type}} is "`resolve-promise`", then return "`Promise.resolve`".
+                    1. Otherwise, return "`Promise.reject`".
+            1. Let |thenOrCatch| be "`then`" if {{PerformanceScriptTiming/type}} is "`resolve-promise`"; Otherwise "`reject-promise`".


lowercase "otherwise"

clelland · 2024-01-05T16:43:05Z

index.bs

+
+<div algorithm="Report pending user callback">
+    To <dfn>report user callback</dfn> given a [=callback function=] |callback| and an [=environment settings object=] |settings|:
+        [=Create script entry point=] given "`user-callback`", |settings|,


This indentation doesn't seem to do anything. ("Create" appears directly after "settings" on the previous line). I think you might need a blank line, or a list item marker here (or maybe both).

Also , the order of parameters used in these algorithms doesn't match Create script entry point's definition.

clelland · 2024-01-05T16:45:31Z

index.bs

+
+<div algorithm="Report event handler">
+    To <dfn>report event handler</dfn> given an {{Event}} |event| and an {{EventListener}} |listener|:
+        [=Create script entry point=] given "`event-listener`", |target|'s [=relevant settings object=],


|target| isn't defined here. You probably need a step before this one.

index.bs

clelland · 2024-01-05T16:53:00Z

index.bs

+        1. Set |scriptTimingInfo|'s [=script timing info/source url=] to |promise|'s [=Promise/script url when created=].
+</div>
+
+<div algorithm="Report script execution start">


I think each of these four algorithms should be in a separate <div>

index.bs

clelland · 2024-01-05T16:58:00Z

index.bs

+            whose [=script timing info/start time=] is the [=unsafe shared current time=],
+            and whose [=script timing info/type=] is |type|.
+        1. Run |steps| given |scriptTimingInfo| and |frameTimingInfo|.
+        1. Set |frameTimingInfo|'s [=frame timing info/pending script=] to |scriptTimingInfo|.


Also here, if frameTimingInfo is null, what should happen?

noamr

Thanks for the thorough review, I believed I addressed all the comments.

noamr · 2024-01-08T13:24:42Z

index.bs

+            whose [=script timing info/start time=] is the [=unsafe shared current time=],
+            and whose [=script timing info/type=] is |type|.
+        1. Run |steps| given |scriptTimingInfo| and |frameTimingInfo|.
+        1. Set |frameTimingInfo|'s [=frame timing info/pending script=] to |scriptTimingInfo|.


index.bs

noamr · 2024-01-08T13:28:17Z

index.bs

+        1. Set |scriptTimingInfo|'s [=script timing info/source url=] to |promise|'s [=Promise/script url when created=].
+</div>
+
+<div algorithm="Report script execution start">


noamr · 2024-01-08T13:28:31Z

index.bs

+            1. Set |scriptTimingInfo|'s [=script timing info/invoker name=] to |target|'s {{Node/nodeName}}.
+            1. If |target| is an {{Element}}, then:
+                1. Set |scriptTimingInfo|'s [=script timing info/event target element id=] to |target|'s [=Element/id=].
+                1. Set |scriptTimingInfo|'s [=script timing info/event target element src attribute=] to the result of [=get an attribute by name|getting an attribute value by name] "`src`" and |target|.


noamr · 2024-01-08T13:29:21Z

index.bs

+
+<div algorithm="Report event handler">
+    To <dfn>report event handler</dfn> given an {{Event}} |event| and an {{EventListener}} |listener|:
+        [=Create script entry point=] given "`event-listener`", |target|'s [=relevant settings object=],


noamr · 2024-01-08T13:31:51Z

index.bs

+
+<div algorithm="Report pending user callback">
+    To <dfn>report user callback</dfn> given a [=callback function=] |callback| and an [=environment settings object=] |settings|:
+        [=Create script entry point=] given "`user-callback`", |settings|,


noamr · 2024-01-08T13:32:09Z

index.bs

+                then:
+                    1. If |this|'s {{PerformanceScriptTiming/type}} is "`resolve-promise`", then return "`Promise.resolve`".
+                    1. Otherwise, return "`Promise.reject`".
+            1. Let |thenOrCatch| be "`then`" if {{PerformanceScriptTiming/type}} is "`resolve-promise`"; Otherwise "`reject-promise`".


noamr · 2024-01-08T13:32:25Z

index.bs

+            1. Let |targetName| be |this|'s [=PerformanceScriptTiming/timing info=]'s [=script timing info/invoker name=].
+            1. If |this|'s [=PerformanceScriptTiming/timing info=]'s [=script timing info/event target element id=] is not the empty string, then:
+                Set |targetName| to the [=concatenate|concatenation=] of « |targetName|, "#", |this|'s [=PerformanceScriptTiming/timing info=]'s [=script timing info/event target element id=] ».
+            1. Else, If |this|'s [=PerformanceScriptTiming/timing info=]'s [=script timing info/event target element src attribute=] is not the empty string, then:


clelland · 2024-01-08T14:06:01Z

index.bs

+
+<div algorithm="Report pending user callback">
+    To <dfn>report user callback</dfn> given a [=callback function=] |callback| and an [=environment settings object=] |settings|:
+    [=Create script entry point=] given |settings|, "`user-callback`", and the following steps given a [=script timing info=] |scriptTimingInfo|:


FYI, this indent still doesn't do anything (at least, in the rendered preview); it's just part of the same paragraph as the preceding line.

Yes, it's not supposed to do anything.

clelland · 2024-01-08T14:13:00Z

index.bs

+<div algorithm="Create script entry point">
+    To <dfn>create script entry point</dfn> given an [=environment settings object=] |settings|,
+        a {{ScriptTimingType}} |type|, and |steps|,
+        which is an algorithm that takes a [=script timing info=] and an optional [=frame timing info=]:


It took me a while to understand the way that "optional" is working here -- since frameTimingInfo is unconditionally passed to the callback, there's no reason for the callback to ever check that it exists, or is non-null. (So, not optional on either side of that function call).

It's more like the JS sense, where all arguments are "optional"; you don't have to name positional parameters that you aren't interested in, and the arguments on each side of the function call don't have to match at all.

clelland

Thanks -- the last comments here are more style than anything else; this LGTM

noamr added 5 commits November 27, 2023 13:43

LoAF: Add HTML monkey-patches

1f76638

Fix WebIDL overloading

61a67ee

nit

d277da3

LoAF: add support for scripts

229708d

This adds support for reporting scripts: - The PerformanceScriptTiming IDL - Algorithms for how script entries are created - Monkey patches to HTML/DOM/WebIDL

Add privsec

e8d5168

noamr mentioned this pull request Nov 29, 2023

LoAF: Add HTML monkey-patches #123

Closed

noamr added 2 commits November 29, 2023 17:31

Still report opaque script source

9191e01

Still report opaque script source

105f0fe

noamr closed this Nov 30, 2023

noamr reopened this Nov 30, 2023

noamr requested a review from mmocny December 4, 2023 14:52

noamr closed this Dec 4, 2023

noamr reopened this Dec 4, 2023

chromium-wpt-export-bot mentioned this pull request Dec 5, 2023

LoAF: for no-cors scripts, hide executionStart web-platform-tests/wpt#43522

Merged

noamr closed this Dec 13, 2023

noamr reopened this Dec 13, 2023

noamr added 4 commits December 13, 2023 16:51

Fix indent

5f48e12

Fix indent

4e38509

Fix indent

6130e87

Fix indent

57d977d

noamr requested a review from clelland December 13, 2023 19:28

clelland reviewed Dec 14, 2023

View reviewed changes

noamr added 2 commits December 14, 2023 20:25

nits

3565f5a

nits

b13e825

nits

bf8051b

clelland reviewed Jan 5, 2024

View reviewed changes

Fix multiple nits

eff4124

noamr commented Jan 8, 2024

View reviewed changes

clelland reviewed Jan 8, 2024

View reviewed changes

clelland approved these changes Jan 8, 2024

View reviewed changes

noamr merged commit 0cd3723 into main Jan 8, 2024
3 checks passed

noamr deleted the loaf-scripts branch January 8, 2024 14:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LoAF: add support for scripts #124

LoAF: add support for scripts #124

noamr commented Nov 29, 2023 •

edited by pr-preview bot

noamr commented Nov 30, 2023

clelland Dec 14, 2023

clelland Dec 14, 2023

noamr Dec 14, 2023

noamr commented Jan 5, 2024

clelland Jan 5, 2024

noamr Jan 8, 2024

clelland Jan 5, 2024

noamr Jan 8, 2024

clelland Jan 5, 2024

noamr Jan 8, 2024

clelland Jan 5, 2024

noamr Jan 8, 2024

clelland Jan 5, 2024

noamr Jan 8, 2024

clelland Jan 5, 2024

noamr Jan 8, 2024

noamr left a comment

noamr Jan 8, 2024

noamr Jan 8, 2024

noamr Jan 8, 2024

noamr Jan 8, 2024

noamr Jan 8, 2024

noamr Jan 8, 2024

noamr Jan 8, 2024

clelland Jan 8, 2024 •

edited

noamr Jan 8, 2024

clelland Jan 8, 2024

clelland left a comment

LoAF: add support for scripts #124

LoAF: add support for scripts #124

Conversation

noamr commented Nov 29, 2023 • edited by pr-preview bot

noamr commented Nov 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noamr commented Jan 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noamr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clelland Jan 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clelland left a comment

Choose a reason for hiding this comment

noamr commented Nov 29, 2023 •

edited by pr-preview bot

clelland Jan 8, 2024 •

edited