Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2463953002:
Expose RTCDataChannel and RTCDataChannelEvent (with constructor) (Closed)
DescriptionExpose RTCDataChannel and RTCDataChannelEvent (with constructor)
Intent to Implement and Ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/zp-bVSF8PpY/h_IB2O0PAQAJ
For RTCDataChannel, simply remove [NoInterfaceObject] and for
RTCDataChannelEvent, the constructor is added.
BUG=658428
Committed: https://crrev.com/9191ae53bb551191f78e94326a989f50f6c55c09
Cr-Commit-Position: refs/heads/master@{#432248}
Patch Set 1 : Intial checkin #Patch Set 2 : Partially editted tests expected results #
Total comments: 5
Patch Set 3 : Added layout test for constructor and fixed expected output for global-listing #
Total comments: 2
Patch Set 4 : Added more constructor tests for RTCDataChannelEvent and update test expects #
Total comments: 5
Patch Set 5 : Rebase update #Messages
Total messages: 81 (64 generated)
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
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 ========== Expose RTCDataChannel and RTCDataChannelEvent (with constructor) BUG=658428 ========== to ========== Expose RTCDataChannel and RTCDataChannelEvent (with constructor) For RTCDataChannel, simply remove [NoInterfaceObject] and for RTCDataChannelEvent, the constructor is added. BUG=658428 ==========
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_...)
The CQ bit was checked by lunalu@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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
foolip@chromium.org changed reviewers: + foolip@chromium.org
Code and IDL all LGTM % nits.
What's still needed here are tests for the constructor, and an Intent to Ship
for this.
For the test, you can copy
LayoutTests/imported/wpt/webrtc/rtcpeerconnection/rtcpeerconnection-constructor.html
into LayoutTests/fast/peerconnection/RTCDataChannelEvent-constructor.html and
remove almost all of it, keeping just the boilerplate. The script itself would
be:
test(function() {
assert_equals(RTCDataChannelEvent.length, 0);
assert_throws(new TypeError, new RTCDataChannelEvent('type'));
}
And then one test that actually provides the channel. If you want to do it in
upstream web-platform-tests (like
https://github.com/w3c/web-platform-tests/pull/4062) and wait for import that's
OK, but more practical would be to just do it locally and export it once the
process for doing that is in place.
For the intent, see www.chromium.org/blink#launch-process and copy-paste
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/7dTqNBRIUrg/LI...,
this is low risk so nothing fancy required.
https://codereview.chromium.org/2463953002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/peerconnection/RTCDataChannelEventInit.idl
(right):
https://codereview.chromium.org/2463953002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCDataChannelEventInit.idl:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
off-by-one error :)
https://codereview.chromium.org/2463953002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCDataChannelEventInit.idl:5:
// https://w3c.github.io/webrtc-pc/#dom-rtcdatachannelevent
This is the definition of RTCDataChannelEvent constructor. This spec doesn't
have a section for this dictionary but
https://w3c.github.io/webrtc-pc/#rtcdatachannelevent or
https://w3c.github.io/webrtc-pc/#idl-def-rtcdatachanneleventinit is OK.
https://codereview.chromium.org/2463953002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCDataChannelEventInit.idl:7:
// channel attribute needs to be required to avoid null instance of
Unlike most other specs on w3c.github.io, this one isn't automatically updated.
This was already fixed in https://github.com/w3c/webrtc-pc/pull/864 and will
eventually go live, so this comment won't be needed.
The CQ bit was checked by lunalu@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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by lunalu@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 checked by lunalu@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...
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
The CQ bit was checked by lunalu@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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Patchset #3 (id:100001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:120001) has been deleted
Please take a final look before I land the CL. https://codereview.chromium.org/2463953002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/peerconnection/RTCDataChannelEventInit.idl (right): https://codereview.chromium.org/2463953002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/peerconnection/RTCDataChannelEventInit.idl:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/11/03 15:24:16, foolip wrote: > off-by-one error :) Done. https://codereview.chromium.org/2463953002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/peerconnection/RTCDataChannelEventInit.idl:5: // https://w3c.github.io/webrtc-pc/#dom-rtcdatachannelevent On 2016/11/03 15:24:16, foolip wrote: > This is the definition of RTCDataChannelEvent constructor. This spec doesn't > have a section for this dictionary but > https://w3c.github.io/webrtc-pc/#rtcdatachannelevent or > https://w3c.github.io/webrtc-pc/#idl-def-rtcdatachanneleventinit is OK. Done.
LGTM % test nits. Also wait for 3xLGTM on your Intent to Ship on blink-dev before landing this. https://codereview.chromium.org/2463953002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/peerconnection/RTCDataChannelEvent-constructor.html (right): https://codereview.chromium.org/2463953002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/peerconnection/RTCDataChannelEvent-constructor.html:9: assert_throws(new TypeError(), function() { new RTCDataChannelEvent('type'); }); Here, can you also test that these two throw TypeError: new RTCDataChannelEvent('type', { channel: null }); new RTCDataChannelEvent('type', { channel: undefined }); With that the description wouldn't be pedantically correct, so maybe "RTCDataChannelEvent constructor without channel". (Having "Test for" or "Checks for" in the test name isn't uncommon, but I like to leave it out. Feel free to go with whatever you like.)
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_...)
The CQ bit was checked by lunalu@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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by lunalu@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...
Patchset #4 (id:160001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by lunalu@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: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lunalu@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...
Patchset #4 (id:180001) has been deleted
Patchset #4 (id:200001) has been deleted
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_...)
Patchset #4 (id:220001) has been deleted
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Done. Thanks https://codereview.chromium.org/2463953002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/peerconnection/RTCDataChannelEvent-constructor.html (right): https://codereview.chromium.org/2463953002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/peerconnection/RTCDataChannelEvent-constructor.html:9: assert_throws(new TypeError(), function() { new RTCDataChannelEvent('type'); }); On 2016/11/08 20:36:43, foolip wrote: > Here, can you also test that these two throw TypeError: > new RTCDataChannelEvent('type', { channel: null }); > new RTCDataChannelEvent('type', { channel: undefined }); > > With that the description wouldn't be pedantically correct, so maybe > "RTCDataChannelEvent constructor without channel". (Having "Test for" or "Checks > for" in the test name isn't uncommon, but I like to leave it out. Feel free to > go with whatever you like.) Done.
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 ========== Expose RTCDataChannel and RTCDataChannelEvent (with constructor) For RTCDataChannel, simply remove [NoInterfaceObject] and for RTCDataChannelEvent, the constructor is added. BUG=658428 ========== to ========== Expose RTCDataChannel and RTCDataChannelEvent (with constructor) Intent to Implement and Ship: https://groups.google.com/a/chromium.org/d/msg/blink-dev/zp-bVSF8PpY/h_IB2O0P... For RTCDataChannel, simply remove [NoInterfaceObject] and for RTCDataChannelEvent, the constructor is added. BUG=658428 ==========
foolip@chromium.org changed reviewers: + guidou@chromium.org, hbos@chromium.org
lgtm with surprise, and let's also get review from guido@ and hbos@ (my deskmates!) before landing this. Also wait for a third LGTM on the intent, I've poked chrishtr@ and dglazkov@. https://codereview.chromium.org/2463953002/diff/240001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/imported/wpt/html/browsers/origin/cross-origin-objects/cross-origin-objects-expected.txt (right): https://codereview.chromium.org/2463953002/diff/240001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/imported/wpt/html/browsers/origin/cross-origin-objects/cross-origin-objects-expected.txt:13: FAIL [[OwnPropertyKeys]] should return all properties from cross-origin objects assert_array_equals: Object.getOwnPropertyNames() gives the right answer for cross-origin Window lengths differ, expected 863 got 13 This is surprising, was this really caused by your changes?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2463953002/diff/240001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/imported/wpt/html/browsers/origin/cross-origin-objects/cross-origin-objects-expected.txt (right): https://codereview.chromium.org/2463953002/diff/240001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/imported/wpt/html/browsers/origin/cross-origin-objects/cross-origin-objects-expected.txt:13: FAIL [[OwnPropertyKeys]] should return all properties from cross-origin objects assert_array_equals: Object.getOwnPropertyNames() gives the right answer for cross-origin Window lengths differ, expected 863 got 13 On 2016/11/11 16:11:35, foolip wrote: > This is surprising, was this really caused by your changes? Yes, it was. Chrome is currently exposing all the window properties on cross-origin content windows (which is a bug).
https://codereview.chromium.org/2463953002/diff/240001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/imported/wpt/html/browsers/origin/cross-origin-objects/cross-origin-objects-expected.txt (right): https://codereview.chromium.org/2463953002/diff/240001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/imported/wpt/html/browsers/origin/cross-origin-objects/cross-origin-objects-expected.txt:13: FAIL [[OwnPropertyKeys]] should return all properties from cross-origin objects assert_array_equals: Object.getOwnPropertyNames() gives the right answer for cross-origin Window lengths differ, expected 863 got 13 On 2016/11/11 18:21:20, loonybear wrote: > On 2016/11/11 16:11:35, foolip wrote: > > This is surprising, was this really caused by your changes? > > Yes, it was. Chrome is currently exposing all the window properties on > cross-origin content windows (which is a bug). Ah, I see, thanks for checking!
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2463953002/diff/240001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/imported/wpt/html/browsers/origin/cross-origin-objects/cross-origin-objects-expected.txt (right): https://codereview.chromium.org/2463953002/diff/240001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/imported/wpt/html/browsers/origin/cross-origin-objects/cross-origin-objects-expected.txt:13: FAIL [[OwnPropertyKeys]] should return all properties from cross-origin objects assert_array_equals: Object.getOwnPropertyNames() gives the right answer for cross-origin Window lengths differ, expected 863 got 13 On 2016/11/11 18:47:10, foolip wrote: > On 2016/11/11 18:21:20, loonybear wrote: > > On 2016/11/11 16:11:35, foolip wrote: > > > This is surprising, was this really caused by your changes? > > > > Yes, it was. Chrome is currently exposing all the window properties on > > cross-origin content windows (which is a bug). > > Ah, I see, thanks for checking! Yes, we haven't yet implemented white-listed window properties correctly. yukishiino@ is working on it. That said, we're doing a cross-origin access check when the properties are accessed, so there's no security issue.
lgtm
Hi hbos and haraken, could you please take a look at the CL before I land it? Thanks https://codereview.chromium.org/2463953002/diff/240001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/imported/wpt/html/browsers/origin/cross-origin-objects/cross-origin-objects-expected.txt (right): https://codereview.chromium.org/2463953002/diff/240001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/imported/wpt/html/browsers/origin/cross-origin-objects/cross-origin-objects-expected.txt:13: FAIL [[OwnPropertyKeys]] should return all properties from cross-origin objects assert_array_equals: Object.getOwnPropertyNames() gives the right answer for cross-origin Window lengths differ, expected 863 got 13 On 2016/11/12 07:26:20, haraken wrote: > On 2016/11/11 18:47:10, foolip wrote: > > On 2016/11/11 18:21:20, loonybear wrote: > > > On 2016/11/11 16:11:35, foolip wrote: > > > > This is surprising, was this really caused by your changes? > > > > > > Yes, it was. Chrome is currently exposing all the window properties on > > > cross-origin content windows (which is a bug). > > > > Ah, I see, thanks for checking! > > Yes, we haven't yet implemented white-listed window properties correctly. > yukishiino@ is working on it. > > That said, we're doing a cross-origin access check when the properties are > accessed, so there's no security issue. Done.
On 2016/11/14 18:30:32, loonybear wrote: > Hi hbos and haraken, could you please take a look at the CL before I land it? LGTM on my side.
lgtm :3
The CQ bit was checked by lunalu@chromium.org
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by lunalu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, guidou@chromium.org, hbos@chromium.org, foolip@chromium.org Link to the patchset: https://codereview.chromium.org/2463953002/#ps260001 (title: "Rebase update")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Expose RTCDataChannel and RTCDataChannelEvent (with constructor) Intent to Implement and Ship: https://groups.google.com/a/chromium.org/d/msg/blink-dev/zp-bVSF8PpY/h_IB2O0P... For RTCDataChannel, simply remove [NoInterfaceObject] and for RTCDataChannelEvent, the constructor is added. BUG=658428 ========== to ========== Expose RTCDataChannel and RTCDataChannelEvent (with constructor) Intent to Implement and Ship: https://groups.google.com/a/chromium.org/d/msg/blink-dev/zp-bVSF8PpY/h_IB2O0P... For RTCDataChannel, simply remove [NoInterfaceObject] and for RTCDataChannelEvent, the constructor is added. BUG=658428 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Expose RTCDataChannel and RTCDataChannelEvent (with constructor) Intent to Implement and Ship: https://groups.google.com/a/chromium.org/d/msg/blink-dev/zp-bVSF8PpY/h_IB2O0P... For RTCDataChannel, simply remove [NoInterfaceObject] and for RTCDataChannelEvent, the constructor is added. BUG=658428 ========== to ========== Expose RTCDataChannel and RTCDataChannelEvent (with constructor) Intent to Implement and Ship: https://groups.google.com/a/chromium.org/d/msg/blink-dev/zp-bVSF8PpY/h_IB2O0P... For RTCDataChannel, simply remove [NoInterfaceObject] and for RTCDataChannelEvent, the constructor is added. BUG=658428 Committed: https://crrev.com/9191ae53bb551191f78e94326a989f50f6c55c09 Cr-Commit-Position: refs/heads/master@{#432248} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/9191ae53bb551191f78e94326a989f50f6c55c09 Cr-Commit-Position: refs/heads/master@{#432248} |
