Skip to content
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

Update the dom IDL file #9779

Merged

Conversation

@lukebjerring
Copy link
Contributor

lukebjerring commented Mar 2, 2018

Hello reviewer(s),

This PR is intended to consolidate the spec’s IDL definition with the WPT test suite’s copy, and any idlharness tests.

The up-to-date copy of the IDL file was automatically extracted from the reffy-reports repo (https://github.com/tidoust/reffy-reports/tree/master/whatwg/idl) which scrapes known specs automatically + regulary.

This PR is part of a migration project which will eventually be automatically updating and creating PRs for changes in spec IDL.

Please check that:
The Spec (and its source) is correct and up-to-date
All tests which cover the IDL in the spec have been migrated to fetch + use the idl in the interfaces/ directory (instead of inline copies in the test files).

@wpt-pr-bot wpt-pr-bot requested review from domenic, jensl and yuki3 Mar 2, 2018

[Exposed=Window]
interface NodeList {
getter Node? item(unsigned long index);
readonly attribute unsigned long length;
// iterable<Node>;
iterable<Node>;

This comment has been minimized.

Copy link
@foolip

foolip Mar 2, 2018

Contributor

This and the DOMTokenList change causes new failing tests due to this:
https://github.com/w3c/web-platform-tests/blob/9192f4b6c633f7f00ad0c1b983cbed33fff6be82/resources/idlharness.js#L2047

It fails for entries(), keys() and values() for each of NodeList and DOMTokenList, so 6 failing tests.

The per-spec behavior is defined by https://heycam.github.io/webidl/#es-iterable and since this is a "value iterator" these assertions could be made:

  • Array.prototype.entries===NodeList.prototype.entries
  • Array.prototype.keys===NodeList.prototype.keys
  • Array.prototype.values===NodeList.prototype.values

That does in fact hold true in Chrome and Safari (maybe more) and explains why NodeList.prototype.entries.call({}) should't throw TypeError but actually return something.

So, to merge this we need to at least make idlharness.js not assert that this should throw.

This comment has been minimized.

Copy link
@lukebjerring

lukebjerring May 23, 2018

Author Contributor

Sounds reasonable - Can you write a PR that makes said change?

This comment has been minimized.

Copy link
@foolip

foolip May 23, 2018

Contributor

I wrote #9790 back then, but then wasn't sure it worked at all. Needs tests I guess.

This comment has been minimized.

Copy link
@lukebjerring

lukebjerring Jul 26, 2018

Author Contributor

As you described, Chromium shows a bunch of regressions due to the issue. Are you able to rebase/get #9790 up to date?

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 2, 2018

OK, sent #9790 to uncomment the iterable bits. (In the same PR to demonstrate that it works.)

@lukebjerring lukebjerring force-pushed the lukebjerring:idl-file-updates-dom branch from f92cfd5 to 037bb2f Mar 7, 2018

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 8, 2018

Build PASSED

Started: 2018-03-08 01:09:55
Finished: 2018-03-08 01:36:19

Failing Jobs

  • safari:11.0
  • MicrosoftEdge:16.16299

View more information about this build on:

@lukebjerring

This comment has been minimized.

Copy link
Contributor Author

lukebjerring commented Jul 26, 2018

As dom is now a very common dependency for other IDL tests, I've thrown this as a CL in Chromium to see if we're causing any unexpected regressions:
https://chromium-review.googlesource.com/c/chromium/src/+/1151707

@annevk
Copy link
Member

annevk left a comment

This looks great, modulo @foolip's comments. It's a little weird to see all the newlines disappear though. Is that a result of the scraping from the specification?

@lukebjerring

This comment has been minimized.

Copy link
Contributor Author

lukebjerring commented Aug 1, 2018

Yeah, it's from a scrape (see the PR description for details)

@lukebjerring

This comment has been minimized.

Copy link
Contributor Author

lukebjerring commented Aug 1, 2018

As discussed offline with @foolip, I'll just re-comment iterable for now and we'll follow up when #9790 is resolved :)

lukebjerring added some commits Aug 1, 2018

@annevk

annevk approved these changes Aug 1, 2018

@@ -1,9 +1,14 @@
// GENERATED CONTENT - DO NOT EDIT

This comment has been minimized.

Copy link
@annevk

annevk Aug 1, 2018

Member

So I should no longer ask for updates to this file whenever we update DOM?

This comment has been minimized.

Copy link
@lukebjerring

lukebjerring Aug 1, 2018

Author Contributor

Yep, but they're not yet regular (semi-manual) so feel free to ping me or @foolip if you'd like a more hasty update.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Aug 1, 2018

(Please use squash & merge btw.)

@lukebjerring lukebjerring merged commit 04e4fda into web-platform-tests:master Aug 1, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Auto-import IDL files automation moved this from In progress to Done Aug 1, 2018

@lukebjerring lukebjerring deleted the lukebjerring:idl-file-updates-dom branch Oct 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.