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 to webidl2.js@22.0.0 #16864

Merged
merged 9 commits into from Jun 24, 2019
Merged

Update to webidl2.js@22.0.0 #16864

merged 9 commits into from Jun 24, 2019

Conversation

saschanaz
Copy link
Member

@saschanaz saschanaz commented May 16, 2019

Closes #16804, closes #16799

@wpt-pr-bot wpt-pr-bot requested a review from tobie May 17, 2019 06:09
@saschanaz saschanaz marked this pull request as ready for review May 17, 2019 07:20
@saschanaz saschanaz changed the title Migrate to the latest bits of webidl2.js Migrate to webidl2.js@21.0.0 May 17, 2019
@saschanaz
Copy link
Member Author

saschanaz commented May 17, 2019

Can anyone restart Azure Pipelines test? Clicking re-run does not work for me.

@saschanaz saschanaz closed this May 18, 2019
@saschanaz saschanaz reopened this May 18, 2019
@saschanaz saschanaz changed the title Migrate to webidl2.js@21.0.0 Update to webidl2.js@21.0.0 May 18, 2019
@gsnedders
Copy link
Member

@jgraham @lukebjerring can one/both of you do a full run against this PR?

@lukebjerring
Copy link
Contributor

I'm away - I think @foolip knows how to do this too?

@foolip
Copy link
Member

foolip commented May 27, 2019

I've submitted https://chromium-review.googlesource.com/c/chromium/src/+/1630483

There were the presubmit warnings:

* Presubmit Warnings **
Found lines longer than 800 characters (first 5 shown).
  third_party/blink/web_tests/external/wpt/resources/webidl2/dist/webidl2.js, line 1, 25411 chars

Found line ending with white spaces in:

***************
third_party/blink/web_tests/external/wpt/resources/webidl2/lib/productions/helpers.js:2
***************

The trailing newline is easy to fix: w3c/webidl2.js#329

The single long line I don't know what to do about. @Hexcles WDYT?

If we're only using the build webidl2.js I wonder if there's any use in us still pulling in the whole history of the repo. It still won't be possible to blame code that is actually run, since it's a single line.

@gsnedders
Copy link
Member

We also don't have the sourcemap working, which is potentially annoying when debugging things, due to the path rewriting… I don't know what the right solution there is.

@gsnedders
Copy link
Member

If we're only using the build webidl2.js I wonder if there's any use in us still pulling in the whole history of the repo. It still won't be possible to blame code that is actually run, since it's a single line.

To be fair, we were only ever pulling in a single file.

I think if anything there's a stronger argument now, because we could commit a non-minified built webidl2.js.

@foolip
Copy link
Member

foolip commented May 28, 2019

@gsnedders
Copy link
Member

https://treeherder.mozilla.org/#/jobs?repo=try&revision=211118fed274371548638f3d362e94774d71a19c is it run through Mozilla try; lots of promise_test: Unhandled rejection with value: object "TypeError: obj.toJSON is not a function" and some:

TEST-UNEXPECTED-FAIL | /workers/interfaces.worker.html | interfaces - Error: Syntax error at line 82, since `interface WindowOrWorkerGlobalScope`:
WorkerGlobalScope implements WindowOrWorkerGlobalScope;
^ Unrecognised tokens

@saschanaz
Copy link
Member Author

saschanaz commented May 28, 2019

I think I fixed TypeError: obj.toJSON is not a function but no idea about unexpected token {. implements is expected to fail, maybe we should remove it first.

PS:

Try https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec6c5777bec884afd30cabfceced9ceaa000c8c6

The failing tests are from the removal of implements support and some error message changes in xfails.

@Hexcles
Copy link
Member

Hexcles commented May 29, 2019

The single long line I don't know what to do about. @Hexcles WDYT?

I believe that's just a warning and won't block the CL from landing. Nonetheless, I'll look into if I can exclude these third-party files from the check.

@foolip
Copy link
Member

foolip commented Jun 3, 2019

https://chromium-review.googlesource.com/c/chromium/src/+/1636390 updated our lint, thanks @Hexcles!

@saschanaz is Unexpected token {an error that webidl2.js might throw, so that we should look for IDL files with some old syntax?

@saschanaz
Copy link
Member Author

@foolip webidl2.js throws with the format Syntax error at line N:\n[[actual IDL lines]], so I don't think it's a webidl2 parser error.

@saschanaz
Copy link
Member Author

external/wpt/IndexedDB/idlharness.any.serviceworker.html from #16864 (comment) explicitly says "Uncaught SyntaxError: Unexpected token {", source: https://web-platform.test:8444/resources/WebIDLParser.js (3). Not sure the cause though...

marcoscaceres
marcoscaceres previously approved these changes Jun 3, 2019
@marcoscaceres marcoscaceres dismissed their stale review June 3, 2019 15:21

Dismissing my review, as there are some deeper things to look into.

@marcoscaceres
Copy link
Contributor

Probably needs a rebase now.

@saschanaz saschanaz closed this Jun 4, 2019
@saschanaz saschanaz reopened this Jun 4, 2019
@saschanaz
Copy link
Member Author

saschanaz commented Jun 5, 2019

@foolip Could you run the tests again? Now it should at least show the problematic line number.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e08a32229ac391b7a3b9dad5d3c23dda48dc2c87&selectedJob=250164391

@foolip
Copy link
Member

foolip commented Jun 6, 2019

@saschanaz I won't be at my workstation until Monday. Perhaps @lukebjerring can give it a try, otherwise I can next week.

@gsnedders
Copy link
Member

I've gone through all the regressions on that try run (all fixed by IDL syntax fixes that have landed here but not been synced into m-c, @jgraham), and I'm happy to land this now. I'll give this a day in case anyone disagrees.

@saschanaz saschanaz changed the title Update to webidl2.js@21.0.0 Update to webidl2.js@22.0.0 Jun 7, 2019
@gsnedders gsnedders merged commit 3f55557 into web-platform-tests:master Jun 24, 2019
@saschanaz saschanaz deleted the webidl2-latest branch June 24, 2019 14:42
Ms2ger added a commit that referenced this pull request Jul 1, 2019
It was added in #6685 and removed accidentally in #16864.
Ms2ger added a commit that referenced this pull request Jul 8, 2019
It was added in #6685 and removed accidentally in #16864.
@Ms2ger
Copy link
Contributor

Ms2ger commented Jul 8, 2019

@saschanaz can you explain how you generated the webidl2.js file?

@saschanaz
Copy link
Member Author

@Ms2ger npx webpack --mode none on the webidl2.js directory should do the job.

@saschanaz
Copy link
Member Author

Filed w3c/webidl2.js#348 to make it straightforward.

marcoscaceres pushed a commit that referenced this pull request Jul 23, 2019
natechapin pushed a commit to natechapin/wpt that referenced this pull request Aug 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants