Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2862423002:
[RemotePlayback] Added IDL harness WebPlatform test for RemotePlayback API. (Closed)
Description[RemotePlayback] Added IDL harness WebPlatform test for RemotePlayback API.
BUG=666468
TEST=run webkit tests locally
Review-Url: https://codereview.chromium.org/2862423002
Cr-Commit-Position: refs/heads/master@{#470366}
Committed: https://chromium.googlesource.com/chromium/src/+/64282c8533da904acb78f1f9ef68c97619825b9c
Patch Set 1 #
Total comments: 5
Patch Set 2 : Moved the test under external/wpt/interfaces/ #Patch Set 3 : Added the harness test files back #
Total comments: 1
Patch Set 4 : Fixed the tests #
Messages
Total messages: 30 (16 generated)
avayvod@chromium.org changed reviewers: + foolip@chromium.org
Philip, PTaL.
The CQ bit was checked by avayvod@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.
lgtm % nits https://codereview.chromium.org/2862423002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/W3CImportExpectations (right): https://codereview.chromium.org/2862423002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/W3CImportExpectations:333: external/wpt/remote-playback [ Pass ] This should actually be commented out. The weirdness is documented at the top of this file, and I've filed crbug.com/713987 to get everything except the skip lines out of this file. https://codereview.chromium.org/2862423002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/external/wpt/remote-playback/idlharness-expected.txt (right): https://codereview.chromium.org/2862423002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/external/wpt/remote-playback/idlharness-expected.txt:4: FAIL HTMLMediaElement must be primary interface of [object HTMLUnknownElement] assert_equals: [object HTMLUnknownElement]'s prototype is not HTMLMediaElement.prototype expected object "[object HTMLMediaElement]" but got object "[object HTMLUnknownElement]" This is because of <media> in the test instead of <video>. Can you go over the failures and see if there's anything else that's a legitimate problem? https://codereview.chromium.org/2862423002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/external/wpt/remote-playback/idlharness.html (right): https://codereview.chromium.org/2862423002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/external/wpt/remote-playback/idlharness.html:22: enum RemotePlaybackState { Recently, some IDL files were moved into interfaces/, which is going to make maintaining these tests simpler, as the IDL files could be updated by scripts. See https://github.com/w3c/web-platform-tests/commits/master/interfaces for the upstream PRs doing that. Can you add interfaces/ to W3CImportExpectations and add an interfaces/remoteplayback.idl instead?
https://codereview.chromium.org/2862423002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/external/wpt/remote-playback/idlharness-expected.txt (right): https://codereview.chromium.org/2862423002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/external/wpt/remote-playback/idlharness-expected.txt:4: FAIL HTMLMediaElement must be primary interface of [object HTMLUnknownElement] assert_equals: [object HTMLUnknownElement]'s prototype is not HTMLMediaElement.prototype expected object "[object HTMLMediaElement]" but got object "[object HTMLUnknownElement]" On 2017/05/08 at 14:58:03, foolip wrote: > This is because of <media> in the test instead of <video>. Can you go over the failures and see if there's anything else that's a legitimate problem? TBH I'm confused about this. <media> is not a valid tag, is it? If I use <video> though the test complains that it's not an HTMLMediaElement. I copy-pasted this <media> from the mediastream capturefromelement idl test and then saw that they just have this expectation... Then all this failures when passing null as the object to the method?
https://codereview.chromium.org/2862423002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/external/wpt/remote-playback/idlharness-expected.txt (right): https://codereview.chromium.org/2862423002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/external/wpt/remote-playback/idlharness-expected.txt:4: FAIL HTMLMediaElement must be primary interface of [object HTMLUnknownElement] assert_equals: [object HTMLUnknownElement]'s prototype is not HTMLMediaElement.prototype expected object "[object HTMLMediaElement]" but got object "[object HTMLUnknownElement]" On 2017/05/08 17:31:45, whywhat wrote: > On 2017/05/08 at 14:58:03, foolip wrote: > > This is because of <media> in the test instead of <video>. Can you go over the > failures and see if there's anything else that's a legitimate problem? > > TBH I'm confused about this. <media> is not a valid tag, is it? If I use <video> > though the test complains that it's not an HTMLMediaElement. I copy-pasted this > <media> from the mediastream capturefromelement idl test and then saw that they > just have this expectation... > > Then all this failures when passing null as the object to the method? <media> isn't a known HTML element, so it ends up as HTMLUnknownElement, like <banana>. What you've run into might be a problem with idlharness.js, if it assumes that the object you pass in is of exactly the expected type, and not a derived type. If that's the case and can't be controlled in any way, can you file an issue at https://github.com/w3c/web-platform-tests/issues? If you can find a fix, it's OK to put it in this CL too of course.
Moved the test under external/wpt/interfaces/
IIUC, moving the test as the .idl file to interfaces removes my concerns wrt <media> etc? How do I run the test locally now though?
The CQ bit was checked by avayvod@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...
On 2017/05/08 22:08:45, whywhat wrote: > IIUC, moving the test as the .idl file to interfaces removes my concerns wrt > <media> etc? How do I run the test locally now though? Just using run-webkit-tests extern/wpt/... should start up wptserve automatically, which is what's needed to make those absolute paths work. Unless the problem is something else?
The CQ bit was unchecked by avayvod@chromium.org
Added the harness test files back
Ok, I still have some FAIL statements for: - has_stringifier, has_extended_attribute checks - EventHandler is not defined - I couldn't find its definition in our idl files to copy from?
The CQ bit was checked by avayvod@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_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
On 2017/05/08 22:43:15, whywhat wrote:
> Ok, I still have some FAIL statements for:
> - has_stringifier, has_extended_attribute checks
> - EventHandler is not defined - I couldn't find its definition in our idl
files
> to copy from?
For EventHandler you can do idlArray.add_untested_idls('interface EventHandler
{};');
The other problems are probably because of [media] and [media.remote]. If you
look at other tests, these are usually strings, and are eval()ed. I haven't
checked idlharness.js, but that'd probably fix the problem. It might be nice to
support just passing in an object directly as well, but meh.
https://codereview.chromium.org/2862423002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/interfaces/remoteplayback.idl (right): https://codereview.chromium.org/2862423002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/interfaces/remoteplayback.idl:1: interface HTMLMediaElement : HTMLElement {}; These shouldn't be in this file, they should go in add_untested_idls. That may be related to the problem you're seeing, dunno.
Fixed the tests
The CQ bit was checked by avayvod@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org Link to the patchset: https://codereview.chromium.org/2862423002/#ps60001 (title: "Fixed the tests")
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": 60001, "attempt_start_ts": 1494345669721420,
"parent_rev": "cca8845a40c0ed5c7ada964dfef71a80375c0395", "commit_rev":
"64282c8533da904acb78f1f9ef68c97619825b9c"}
Message was sent while issue was closed.
Description was changed from ========== [RemotePlayback] Added IDL harness WebPlatform test for RemotePlayback API. BUG=666468 TEST=run webkit tests locally ========== to ========== [RemotePlayback] Added IDL harness WebPlatform test for RemotePlayback API. BUG=666468 TEST=run webkit tests locally Review-Url: https://codereview.chromium.org/2862423002 Cr-Commit-Position: refs/heads/master@{#470366} Committed: https://chromium.googlesource.com/chromium/src/+/64282c8533da904acb78f1f9ef68... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/64282c8533da904acb78f1f9ef68... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
