|
|
DescriptionFix module scripts with for/event attributes
This CL ignores for/event attributes of module scripts as the spec says.
BUG=594639
Review-Url: https://codereview.chromium.org/2877223002
Cr-Commit-Position: refs/heads/master@{#471826}
Committed: https://chromium.googlesource.com/chromium/src/+/5835e0c757d511a695ec05a99881b73b0e4bcf2e
Patch Set 1 #Patch Set 2 : Title #Patch Set 3 : Title #
Total comments: 5
Depends on Patchset: Messages
Total messages: 25 (16 generated)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fix module scripts with for/event attributes BUG= ========== to ========== Fix module scripts with for/event attributes This CL ignores for/event attributes of module scripts as the spec says. BUG=594639 ==========
hiroshige@chromium.org changed reviewers: + kouhei@chromium.org, module-dev@chromium.org
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL. A minor fix. The added test was mostly copy-pasted from: third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/script-for-event.html (I'm not so sure whether there's additional rules to follow when deriving existing wpt tests though.)
lgtm https://codereview.chromium.org/2877223002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/script-for-event.html (right): https://codereview.chromium.org/2877223002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/script-for-event.html:31: <script for="wİndow" event="onload" type="module"> Optional: can we have run-expectation as a custom attr? https://codereview.chromium.org/2877223002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/script-for-event.html:87: assert_equals(run[i], expected[i], so we can query it here script.getAttribute() to get values?
domenic@chromium.org changed reviewers: + domenic@chromium.org
There are various ways to clean up the test, especially since the expected is always true, but overall lgtm; no need to spend too much time making the test pretty in my opinion. Great to have coverage for this.
https://codereview.chromium.org/2877223002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/script-for-event.html (right): https://codereview.chromium.org/2877223002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/script-for-event.html:31: <script for="wİndow" event="onload" type="module"> On 2017/05/12 21:53:17, kouhei (travelling MTV) wrote: > Optional: can we have run-expectation as a custom attr? Sounds nice, but how can we replace |run| with a custom attr, particularly how to get the script element from the script? (|currentScript| cannot be used for module scripts to get the current script element)
https://codereview.chromium.org/2877223002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/script-for-event.html (right): https://codereview.chromium.org/2877223002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/script-for-event.html:31: <script for="wİndow" event="onload" type="module"> On 2017/05/13 00:04:04, hiroshige wrote: > On 2017/05/12 21:53:17, kouhei (travelling MTV) wrote: > > Optional: can we have run-expectation as a custom attr? > > Sounds nice, but how can we replace |run| with a custom attr, particularly how > to get the script element from the script? > (|currentScript| cannot be used for module scripts to get the current > script element) I don't think we can replace |run|. only |expected|.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2877223002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/script-for-event.html (right): https://codereview.chromium.org/2877223002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/script-for-event.html:31: <script for="wİndow" event="onload" type="module"> On 2017/05/13 00:08:40, kouhei (travelling MTV) wrote: > On 2017/05/13 00:04:04, hiroshige wrote: > > On 2017/05/12 21:53:17, kouhei (travelling MTV) wrote: > > > Optional: can we have run-expectation as a custom attr? > > > > Sounds nice, but how can we replace |run| with a custom attr, particularly how > > to get the script element from the script? > > (|currentScript| cannot be used for module scripts to get the current > > script element) > > I don't think we can replace |run|. only |expected|. Hmm. I'd like to defer refactoring (if any, and it should be commonly applied to the classic and module versions). Even after removing |expected|, there seems more refactoring items, e.g. |run|, magical index numbers, and probably it's better to use async_test(), etc.
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/13 00:14:14, hiroshige wrote: > https://codereview.chromium.org/2877223002/diff/40001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/script-for-event.html > (right): > > https://codereview.chromium.org/2877223002/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/script-for-event.html:31: > <script for="wİndow" event="onload" type="module"> > On 2017/05/13 00:08:40, kouhei (travelling MTV) wrote: > > On 2017/05/13 00:04:04, hiroshige wrote: > > > On 2017/05/12 21:53:17, kouhei (travelling MTV) wrote: > > > > Optional: can we have run-expectation as a custom attr? > > > > > > Sounds nice, but how can we replace |run| with a custom attr, particularly > how > > > to get the script element from the script? > > > (|currentScript| cannot be used for module scripts to get the current > > > script element) > > > > I don't think we can replace |run|. only |expected|. > > Hmm. I'd like to defer refactoring (if any, and it should be commonly applied to > the classic and module versions). > Even after removing |expected|, there seems more refactoring items, e.g. |run|, > magical index numbers, and probably it's better to use async_test(), etc. All of this is opitonal to me. The original CL still LGTM.
The CQ bit was checked by hiroshige@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494864929627540, "parent_rev": "d6f04a76bd3b49d538eadf08e97e64b800531ba0", "commit_rev": "5835e0c757d511a695ec05a99881b73b0e4bcf2e"}
Message was sent while issue was closed.
Description was changed from ========== Fix module scripts with for/event attributes This CL ignores for/event attributes of module scripts as the spec says. BUG=594639 ========== to ========== Fix module scripts with for/event attributes This CL ignores for/event attributes of module scripts as the spec says. BUG=594639 Review-Url: https://codereview.chromium.org/2877223002 Cr-Commit-Position: refs/heads/master@{#471826} Committed: https://chromium.googlesource.com/chromium/src/+/5835e0c757d511a695ec05a99881... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/5835e0c757d511a695ec05a99881... |