-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LoAF: add support for scripts #124
Conversation
This adds support for reporting scripts: - The PerformanceScriptTiming IDL - Algorithms for how script entries are created - Monkey patches to HTML/DOM/WebIDL
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}
(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}
(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
Outdated
@@ -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 « ». |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the url is #dfn-identifier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
…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 <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
index.bs
Outdated
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like infra prefers "Otherwise" to "Else"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
index.bs
Outdated
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`". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lowercase "otherwise"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
index.bs
Outdated
|
||
<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|, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
index.bs
Outdated
|
||
<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=], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|target|
isn't defined here. You probably need a step before this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
index.bs
Outdated
1. Set |scriptTimingInfo|'s [=script timing info/source url=] to |promise|'s [=Promise/script url when created=]. | ||
</div> | ||
|
||
<div algorithm="Report script execution start"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think each of these four algorithms should be in a separate <div>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
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|. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also here, if frameTimingInfo
is null, what should happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the thorough review, I believed I addressed all the comments.
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|. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
index.bs
Outdated
1. Set |scriptTimingInfo|'s [=script timing info/source url=] to |promise|'s [=Promise/script url when created=]. | ||
</div> | ||
|
||
<div algorithm="Report script execution start"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
index.bs
Outdated
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|. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
index.bs
Outdated
|
||
<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=], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
index.bs
Outdated
|
||
<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|, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
index.bs
Outdated
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`". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
index.bs
Outdated
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
|
||
<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|: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's not supposed to do anything.
<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=]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks -- the last comments here are more style than anything else; this LGTM
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