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 webusb IDL file #9854

Merged

Conversation

6 participants
@lukebjerring
Copy link
Contributor

lukebjerring commented Mar 5, 2018

No description provided.

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

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 6, 2018

Build BROKEN

Started: 2018-03-09 02:32:51
Finished: 2018-03-09 02:36:15

Failing Jobs

  • firefox:nightly
  • safari:11.0
  • MicrosoftEdge:16.16299

View more information about this build on:

Luke Bjerring

@lukebjerring lukebjerring force-pushed the lukebjerring:idl-file-updates-webusb branch from d5b093b to 4804e36 Mar 7, 2018

Luke Bjerring
@foolip
Copy link
Contributor

foolip left a comment

Question for Reilly.

sequence<AllowedUSBDevice> allowedDevices = [];
};

interface USBPermissionResult : PermissionStatus {

This comment has been minimized.

Copy link
@foolip

foolip Mar 9, 2018

Contributor

I was expecting this to break the test, because PermissionStatus isn't known, but I think what I've seen is that if both interfaces are implemented, then one will reach some code path in idlharness.js where if we don't have any information about the right-hand side, the failure is pretty mysterious.

So... I added what I think will be needed for anyone who does implement these bits.

@@ -1,5 +1,6 @@
// Copied from https://wicg.github.io/webusb/#idl-index minus the

This comment has been minimized.

Copy link
@foolip

foolip Mar 9, 2018

Contributor

@reillyeon, you wrote this bit. What do you think about our auto-updating scheme here? The resulting tests will fail, is that OK? Should it be removed from the spec until it's ready to test and implement, perhaps?

This comment has been minimized.

Copy link
@reillyeon

reillyeon Mar 9, 2018

Contributor

I think the spec should be annotated to mark that section as unstable and if we have an automatic update mechanism it should be possible to mark IDL sections as "unstable and untested" so they aren't included.

This comment has been minimized.

Copy link
@foolip

foolip Mar 9, 2018

Contributor

That would work for us I think. @tabatkins, you have some output from Bikeshed that's used to scrape definitions and things, what you suggest as the "API" for a spec saying that pieces are not to be implemented/tested yet?

This comment has been minimized.

Copy link
@lukebjerring

lukebjerring Jun 5, 2018

Author Contributor

@tabatkins - Ping. Is there an annotation for ignoring an IDL chunk?

This comment has been minimized.

Copy link
@tabatkins

tabatkins Jun 7, 2018

Contributor

Yes, if it's in a non-normative section (.note, .example, .non-normative or .informative on an ancestor) it gets ignored for definition purposes in Bikeshed, and if it's non-normative or has .extract on the pre (I dunno where this practice came from but it's common on the WHATWG side) it gets omitted from the IDL index at the end of the spec.

This comment has been minimized.

Copy link
@reillyeon

reillyeon Jun 8, 2018

Contributor

The reason these definitions are there is partially because of how the Permissions API defines the permissions storage data model. What would the best way of defining IDL for objects that are only included in internal slots?

This comment has been minimized.

Copy link
@tabatkins

tabatkins Jun 8, 2018

Contributor

Internal objects don't have IDL definitions; the only purpose of those is to mediate JS<=>internal type conversions. Just define internal types, using Infra types when possible. Use <dfn dfn for=TheThing> to define slot names/etc if necessary.

@wpt-pr-bot wpt-pr-bot added the webusb label Mar 9, 2018

@wpt-pr-bot wpt-pr-bot requested a review from reillyeon Mar 9, 2018

@reillyeon
Copy link
Contributor

reillyeon left a comment

It looks like this change is pulling IDL definitions from https://github.com/WICG/webusb/blob/gh-pages/index.html rather than https://github.com/WICG/webusb/blob/master/index.bs. The former is out of date because a recent Bikeshed change broke the build.

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented May 23, 2018

@lukebjerring is this now getting the IDL from reffy or webidl-diff?

@lukebjerring

This comment has been minimized.

Copy link
Contributor Author

lukebjerring commented May 28, 2018

webidl-diff. Will update the specs set to redirect.

@lukebjerring

This comment has been minimized.

Copy link
Contributor Author

lukebjerring commented May 28, 2018

Uh, scratch that. Its URL is https://wicg.github.io/webusb/

@reillyeon - I might need to wait until you can push the .bs changes?

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented May 30, 2018

Does this PR match what is in https://wicg.github.io/webusb/ now? If so, is there still a problem? I guess WICG/webusb#135 is to fix the build error, but have there been IDL changes since the last successful build that this PR is reverting?

@reillyeon

This comment has been minimized.

Copy link
Contributor

reillyeon commented May 30, 2018

@foolip @odejesush is working on fixing WICG/webusb#135. This patch is still reverting changes that are in index.bs but have not been published to https://wicg.github.io/webusb/.

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented May 30, 2018

Got it, thanks!

@reillyeon

This comment has been minimized.

Copy link
Contributor

reillyeon commented May 30, 2018

The issue above has been fixed so the next update based on the published spec should contain the correct definitions.

@lukebjerring lukebjerring merged commit f110c50 into web-platform-tests:master Jun 6, 2018

1 check passed

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

This comment has been minimized.

Copy link
Contributor

reillyeon commented Jun 6, 2018

This change has broken these tests in Chromium: https://test-results.appspot.com/data/layout_results/linux-xenial-rel/669/site_per_process_webkit_layout_tests/layout-test-results/results.html

When I looked at this change earlier it seemed that the permission descriptor bits had been filtered out but it looks like the version that was merged still contains them.

Please revert this change (or remove the untestable definitions) as soon as possible.

lukebjerring added a commit to lukebjerring/wpt that referenced this pull request Jun 6, 2018

plehegar added a commit that referenced this pull request Jun 6, 2018

@Hexcles Hexcles referenced this pull request Jun 6, 2018

Merged

Revert #9854 #11393

chromium-wpt-export-bot pushed a commit that referenced this pull request Jun 6, 2018

Luke Bjerring Chrome-bot
Update webusb test
Upstream re-attempt at #9854.
Adds more untested IDL deps.

Change-Id: Icf7bda0f264ec66f4980e0742485eb406e334c19

dhdavvie pushed a commit to dhdavvie/wpt that referenced this pull request Jun 7, 2018

dhdavvie pushed a commit to dhdavvie/wpt that referenced this pull request Jun 7, 2018

chromium-wpt-export-bot pushed a commit that referenced this pull request Jun 7, 2018

Luke Bjerring Chrome-bot
Update webusb test
Upstream re-attempt at #9854.
Adds more untested IDL deps.

Change-Id: Icf7bda0f264ec66f4980e0742485eb406e334c19

chromium-wpt-export-bot pushed a commit that referenced this pull request Jun 7, 2018

Luke Bjerring Chrome-bot
Update webusb test
Upstream re-attempt at #9854.
Adds more untested IDL deps.

Change-Id: Icf7bda0f264ec66f4980e0742485eb406e334c19

chromium-wpt-export-bot pushed a commit that referenced this pull request Jun 11, 2018

Luke Bjerring Chrome-bot
Update webusb test
Upstream re-attempt at #9854.
Adds more untested IDL deps.

Change-Id: Icf7bda0f264ec66f4980e0742485eb406e334c19

chromium-wpt-export-bot pushed a commit that referenced this pull request Jun 11, 2018

Luke Bjerring Chrome-bot
Update webusb test
Upstream re-attempt at #9854.
Adds more untested IDL deps.

Change-Id: Icf7bda0f264ec66f4980e0742485eb406e334c19

aarongable pushed a commit to chromium/chromium that referenced this pull request Jun 14, 2018

Luke Bjerring Commit Bot
Update webusb test
Upstream re-attempt at web-platform-tests/wpt#9854.
Adds more untested IDL deps.

Change-Id: Icf7bda0f264ec66f4980e0742485eb406e334c19
Reviewed-on: https://chromium-review.googlesource.com/1089862
Commit-Queue: Luke Bj <lukebjerring@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567118}

chromium-wpt-export-bot pushed a commit that referenced this pull request Jun 14, 2018

Luke Bjerring Chrome-bot
Update webusb test
Upstream re-attempt at #9854.
Adds more untested IDL deps.

Change-Id: Icf7bda0f264ec66f4980e0742485eb406e334c19
Reviewed-on: https://chromium-review.googlesource.com/1089862
Commit-Queue: Luke Bj <lukebjerring@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567118}

chromium-wpt-export-bot added a commit that referenced this pull request Jun 14, 2018

Update webusb test
Upstream re-attempt at #9854.
Adds more untested IDL deps.

Change-Id: Icf7bda0f264ec66f4980e0742485eb406e334c19
Reviewed-on: https://chromium-review.googlesource.com/1089862
Commit-Queue: Luke Bj <lukebjerring@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567118}

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jul 11, 2018

Bug 1467666 [wpt PR 11394] - Update webusb test Upstream re-attempt at
…web-platform-tests/wpt#9854. Adds more untested IDL deps., a=testonly

Automatic update from web-platform-testsUpdate webusb test
Upstream re-attempt at web-platform-tests/wpt#9854.
Adds more untested IDL deps.

Change-Id: Icf7bda0f264ec66f4980e0742485eb406e334c19
Reviewed-on: https://chromium-review.googlesource.com/1089862
Commit-Queue: Luke Bj <lukebjerring@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567118}

--

wpt-commits: 1110f36ff667fbfa004ae52aad161ccba2574dbe
wpt-pr: 11394

mykmelez pushed a commit to mozilla/gecko that referenced this pull request Jul 13, 2018

Bug 1467666 [wpt PR 11394] - Update webusb test Upstream re-attempt at
…web-platform-tests/wpt#9854. Adds more untested IDL deps., a=testonly

Automatic update from web-platform-testsUpdate webusb test
Upstream re-attempt at web-platform-tests/wpt#9854.
Adds more untested IDL deps.

Change-Id: Icf7bda0f264ec66f4980e0742485eb406e334c19
Reviewed-on: https://chromium-review.googlesource.com/1089862
Commit-Queue: Luke Bj <lukebjerring@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567118}

--

wpt-commits: 1110f36ff667fbfa004ae52aad161ccba2574dbe
wpt-pr: 11394
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.