LoAF: Use document character position for inline scripts [chromium/src : main]
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/45542.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny voted and added 6 comments![]( "Open in Gerrit")
Votes added by Michal Mocny
| Code-Review | +1 |
6 comments
LGTM but with some questions. I think this will provide valuable insights and better than the current status-quo, but I wonder if the specific values are fully consistent?
current_string.NumberOfCharactersConsumed());Will this mark the end of the token where we would prefer the start of the token?
: line_(line), column_(column), offset_(offset) {}My understanding was that we expose character offset because line/column are expensive to compute needlessly. But from this context it sounds like we already have these values?
Could an alternative spec have provided line/column when available, and character offset as a fallback (or both)
assert_greater_than(script.sourceCharPosition, 0);What is the actual value? Would it point to:
- the start of <script> tag
- the first char of the script itself
- the end of one of the above
I think any insight is better than none, but curious!
assert_greater_than(script.sourceCharPosition, 0);What is the actual value? Would it point to:
- the start of <img> tag
- the first char of the script itself
- the end of one of the above
(Based on my read it will be different than classic/module script)
}).observe({ type: "long-animation-frame", buffered: true }));Was buffered needed because these (may) be reported immediately onload and before the test loads?
Related details
- Kouhei Ueno
- Noam Rosenthal
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal added 5 comments![]( "Open in Gerrit")
current_string.NumberOfCharactersConsumed());Will this mark the end of the token where we would prefer the start of the token?
It's the end of the start tag. This would be consistent with character index (or the exchangeable line/column) in dev-tools.
: line_(line), column_(column), offset_(offset) {}My understanding was that we expose character offset because line/column are expensive to compute needlessly. But from this context it sounds like we already have these values?
Could an alternative spec have provided line/column when available, and character offset as a fallback (or both)
Correct, though the current spec is constrained to character index, as one common denominator is simple for a start.
assert_greater_than(script.sourceCharPosition, 0);What is the actual value? Would it point to:
- the start of <script> tag
- the first char of the script itself
- the end of one of the above
I think any insight is better than none, but curious!
end of the <script> start tag
assert_greater_than(script.sourceCharPosition, 0);What is the actual value? Would it point to:
- the start of <img> tag
- the first char of the script itself
- the end of one of the above
(Based on my read it will be different than classic/module script)
end of the <img> tag. The attributes for an element are parsed when the element's start tag is consumed. So both in inline event listener and in inline script blocks, the character position would be at the end of the starting tag of the relevant element. I think that's consistent enough
}).observe({ type: "long-animation-frame", buffered: true }));Was buffered needed because these (may) be reported immediately onload and before the test loads?
- Kouhei Ueno
- Michal Mocny
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kouhei Ueno (Gerrit)
Kouhei Ueno added 1 comment![]( "Open in Gerrit")
unsigned offset_ = 0;What would `offset_` mean? e.g. how is it different to `columns`? I think we should be careful about changing the TextPosition class.
Related details
- Michal Mocny
- Noam Rosenthal
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal added 1 comment![]( "Open in Gerrit")
unsigned offset_ = 0;What would `offset_` mean? e.g. how is it different to `columns`? I think we should be careful about changing the TextPosition class.
It's the character offset regardless of rows/columns. We expose that in long animation frames rather than row/column for performance reasons. I can rename it to char_offset_. Alternative proposals to doing this?
Related details
- Kouhei Ueno
- Michal Mocny
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kouhei Ueno (Gerrit)
Kouhei Ueno added 1 comment![]( "Open in Gerrit")
sorry i'm still reviewing this - in particular trying to come up with TextPosision::offset_ alternative
Related details
- Michal Mocny
- Noam Rosenthal
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal added 1 comment![]( "Open in Gerrit")
sorry i'm still reviewing this - in particular trying to come up with TextPosision::offset_ alternative
- Kouhei Ueno
- Michal Mocny
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kouhei Ueno (Gerrit)
Kouhei Ueno voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kouhei Ueno
| Code-Review | +1 |
1 comment
unsigned offset_ = 0;Noam RosenthalWhat would `offset_` mean? e.g. how is it different to `columns`? I think we should be careful about changing the TextPosition class.
It's the character offset regardless of rows/columns. We expose that in long animation frames rather than row/column for performance reasons. I can rename it to char_offset_. Alternative proposals to doing this?
I searched for alternatives but I couldn't find path forward. Would you add comment that this is specifically to support the loaf spec?
Related details
- Michal Mocny
- Noam Rosenthal
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal added 5 comments![]( "Open in Gerrit")
current_string.NumberOfCharactersConsumed());Will this mark the end of the token where we would prefer the start of the token?
Noam RosenthalIt's the end of the start tag. This would be consistent with character index (or the exchangeable line/column) in dev-tools.
Done
: line_(line), column_(column), offset_(offset) {}Noam RosenthalMy understanding was that we expose character offset because line/column are expensive to compute needlessly. But from this context it sounds like we already have these values?
Could an alternative spec have provided line/column when available, and character offset as a fallback (or both)
Correct, though the current spec is constrained to character index, as one common denominator is simple for a start.
Done
assert_greater_than(script.sourceCharPosition, 0);Noam RosenthalWhat is the actual value? Would it point to:
- the start of <script> tag
- the first char of the script itself
- the end of one of the above
I think any insight is better than none, but curious!
end of the <script> start tag
Done
assert_greater_than(script.sourceCharPosition, 0);Noam RosenthalWhat is the actual value? Would it point to:
- the start of <img> tag
- the first char of the script itself
- the end of one of the above
(Based on my read it will be different than classic/module script)
end of the <img> tag. The attributes for an element are parsed when the element's start tag is consumed. So both in inline event listener and in inline script blocks, the character position would be at the end of the starting tag of the relevant element. I think that's consistent enough
Done
}).observe({ type: "long-animation-frame", buffered: true }));Noam RosenthalWas buffered needed because these (may) be reported immediately onload and before the test loads?
Yes, it's to avoid raciness
Done
Related details
- Michal Mocny
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Michal Mocny
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal added 2 comments![]( "Open in Gerrit")
What would `offset_` mean? e.g. how is it different to `columns`? I think we should be careful about changing the TextPosition class.
It's the character offset regardless of rows/columns. We expose that in long animation frames rather than row/column for performance reasons. I can rename it to char_offset_. Alternative proposals to doing this?
I searched for alternatives but I couldn't find path forward. Would you add comment that this is specifically to support the loaf spec?
Done
What would `offset_` mean? e.g. how is it different to `columns`? I think we should be careful about changing the TextPosition class.
It's the character offset regardless of rows/columns. We expose that in long animation frames rather than row/column for performance reasons. I can rename it to char_offset_. Alternative proposals to doing this?
I searched for alternatives but I couldn't find path forward. Would you add comment that this is specifically to support the loaf spec?
Done
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
4 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/platform/wtf/text/text_position.h
Insertions: 4, Deletions: 0.
The diff is too large to show. Please review the diff.
```
Change information
LoAF: Use document character position for inline scripts
This applies to classic/module script blocks and event content
attributes.
- M third_party/blink/renderer/bindings/core/v8/js_event_handler_for_content_attribute.h
- M third_party/blink/renderer/core/frame/animation_frame_timing_monitor.cc
- M third_party/blink/renderer/core/html/parser/html_document_parser.cc
- M third_party/blink/renderer/core/probe/core_probes.pidl
- M third_party/blink/renderer/core/script/classic_script.cc
- M third_party/blink/renderer/core/script/module_script.cc
- M third_party/blink/renderer/platform/wtf/text/text_position.cc
- M third_party/blink/renderer/platform/wtf/text/text_position.h
- A third_party/blink/web_tests/external/wpt/long-animation-frame/tentative/loaf-source-location-inline-classic-script.html
- A third_party/blink/web_tests/external/wpt/long-animation-frame/tentative/loaf-source-location-inline-event-listener.html
- A third_party/blink/web_tests/external/wpt/long-animation-frame/tentative/loaf-source-location-inline-module-script.html
- M third_party/blink/web_tests/external/wpt/long-animation-frame/tentative/resources/utils.js
Code-Review: +1 by Kouhei Ueno, +1 by Michal Mocny
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/45542
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin McNee (Gerrit)
Kevin McNee has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |