Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2121613002:
Convert video-controls-[captions, overlay, track]* tests to testharness.js (Closed)
|
Created:
4 years, 5 months ago by Srirama Modified:
4 years, 5 months ago CC:
blink-reviews, chromium-reviews, eric.carlson_apple.com, feature-media-reviews_chromium.org, mlamouri+watch-blink_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert video-controls-[captions, overlay, track]* tests to testharness.js
Cleaning up video-controls-[captions, overlay, track]* tests
in media/ to use testharness.js instead of video-test.js.
This will enable to upstream these tests to web-platform-tests.
BUG=588956
Committed: https://crrev.com/bb48314aaf3f1e4353cf70411dd969383f2f5c0b
Cr-Commit-Position: refs/heads/master@{#403890}
Patch Set 1 : fix #
Total comments: 16
Patch Set 2 : address comments #
Total comments: 6
Patch Set 3 : address philip's comments #
Total comments: 15
Patch Set 4 : Use EventWatcher for better readability and address other comments #
Total comments: 2
Patch Set 5 : address nit #Messages
Total messages: 27 (9 generated)
Description was changed from ========== fix BUG= ========== to ========== fix BUG= ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== fix BUG= ========== to ========== Convert video-controls-[captions, overlay, track]* tests to testharness.js Cleaning up video-controls-[captions, overlay, track]* tests in media/ to use testharness.js instead of video-test.js. This will enable to upstream these tests to web-platform-tests. BUG=588956 ==========
Patchset #1 (id:20001) has been deleted
srirama.m@samsung.com changed reviewers: + foolip@chromium.org, fs@opera.com
PTAL https://codereview.chromium.org/2121613002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/video-controls-captions.html (right): https://codereview.chromium.org/2121613002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-controls-captions.html:21: assert_throws(null, function() { This is for "text track container" error and the one below is for "cue" error. So should i compare the error message to differentiate or is there a better way?
lgtm https://codereview.chromium.org/2121613002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/video-controls-captions-on-off.html (right): https://codereview.chromium.org/2121613002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-controls-captions-on-off.html:40: function checkCaptionsDisplay(captionsVisible) { Could probably also be split into two functions, eliminating the argument. I.e checkCaptionsVisible and checkCaptionsHidden or so. Up to you. https://codereview.chromium.org/2121613002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/video-controls-captions.html (right): https://codereview.chromium.org/2121613002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-controls-captions.html:13: var track = document.querySelector("track"); We could move this into the relevant scope, right? (And make 'track' a local elsewhere.) https://codereview.chromium.org/2121613002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-controls-captions.html:21: assert_throws(null, function() { On 2016/07/04 at 07:56:56, Srirama wrote: > This is for "text track container" error and the one below is for "cue" error. So should i compare the error message to differentiate or is there a better way? I think the easiest way forward is just to add an "annotation" (description) to the assertions, and describe what they are for. Better would probably be to break up textTrackDisplayElement into several usable pieces whose result could be asserted (it's very "intelligent" as it is...) https://codereview.chromium.org/2121613002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-controls-captions.html:50: track = document.createElement("track"); Make 'track' local (per above.)
https://codereview.chromium.org/2121613002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/video-controls-captions.html (right): https://codereview.chromium.org/2121613002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-controls-captions.html:21: assert_throws(null, function() { On 2016/07/04 07:56:56, Srirama wrote: > This is for "text track container" error and the one below is for "cue" error. > So should i compare the error message to differentiate or is there a better way? It's not always clear what checkCaptionsDisplay is hoping to check, but without the error messages in -expected.txt this is indeed testing less than before. textTrackDisplayElement is doing a lot, this might all look simpler if it's split into helpers: textTrackContainerElement textTrackCueElementById textTrackCueElementAtIndex Then here just use assert_equals(textTrackContainerElement(video), null) and below maybe textTrackCueElementAtIndex(0) if that returns null if out of range? Or if the only interesting this is to check if something is rendered or not, which seems to be the case, write a helper that just checks if the bounding client rect of the container is zero or not. (Not tested.) https://codereview.chromium.org/2121613002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-controls-captions.html:35: video.removeChild(track); Just track.remove() works too. https://codereview.chromium.org/2121613002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-controls-captions.html:41: selectTextTrackAtIndex(video, 0); Since you're renaming it anyway, I think clickTextTrackAtIndex would be even better, so that it's clear this isn't using the DOM-exposed APIs to enable the track. https://codereview.chromium.org/2121613002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-controls-captions.html:60: newTrack.addCue(new VTTCue(0, 10, "Some random caption text")); Is this line really needed for the test to pass? The number of cues aren't part of the logic for text track UI I think.
https://codereview.chromium.org/2121613002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/video-controls-captions-on-off.html (right): https://codereview.chromium.org/2121613002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-controls-captions-on-off.html:40: function checkCaptionsDisplay(captionsVisible) { On 2016/07/04 08:46:55, fs wrote: > Could probably also be split into two functions, eliminating the argument. I.e > checkCaptionsVisible and checkCaptionsHidden or so. Up to you. Done. https://codereview.chromium.org/2121613002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/video-controls-captions.html (right): https://codereview.chromium.org/2121613002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-controls-captions.html:13: var track = document.querySelector("track"); On 2016/07/04 08:46:55, fs wrote: > We could move this into the relevant scope, right? (And make 'track' a local > elsewhere.) Done. https://codereview.chromium.org/2121613002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-controls-captions.html:21: assert_throws(null, function() { On 2016/07/04 08:49:31, Philip Jägenstedt wrote: > On 2016/07/04 07:56:56, Srirama wrote: > > This is for "text track container" error and the one below is for "cue" error. > > So should i compare the error message to differentiate or is there a better > way? > > It's not always clear what checkCaptionsDisplay is hoping to check, but without > the error messages in -expected.txt this is indeed testing less than before. > textTrackDisplayElement is doing a lot, this might all look simpler if it's > split into helpers: > > textTrackContainerElement > textTrackCueElementById > textTrackCueElementAtIndex > > Then here just use assert_equals(textTrackContainerElement(video), null) and > below maybe textTrackCueElementAtIndex(0) if that returns null if out of range? > > Or if the only interesting this is to check if something is rendered or not, > which seems to be the case, write a helper that just checks if the bounding > client rect of the container is zero or not. (Not tested.) Attempted the first approach. The helpers are a bit interdependent right now, or should i make them independent with each taking "video" as parameter? https://codereview.chromium.org/2121613002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-controls-captions.html:35: video.removeChild(track); On 2016/07/04 08:49:32, Philip Jägenstedt wrote: > Just track.remove() works too. Done. https://codereview.chromium.org/2121613002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-controls-captions.html:41: selectTextTrackAtIndex(video, 0); On 2016/07/04 08:49:31, Philip Jägenstedt wrote: > Since you're renaming it anyway, I think clickTextTrackAtIndex would be even > better, so that it's clear this isn't using the DOM-exposed APIs to enable the > track. Done. https://codereview.chromium.org/2121613002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-controls-captions.html:50: track = document.createElement("track"); On 2016/07/04 08:46:55, fs wrote: > Make 'track' local (per above.) Done. https://codereview.chromium.org/2121613002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-controls-captions.html:60: newTrack.addCue(new VTTCue(0, 10, "Some random caption text")); On 2016/07/04 08:49:31, Philip Jägenstedt wrote: > Is this line really needed for the test to pass? The number of cues aren't part > of the logic for text track UI I think. Done.
On 2016/07/04 13:00:23, Srirama wrote: > https://codereview.chromium.org/2121613002/diff/40001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/media/video-controls-captions-on-off.html > (right): > > https://codereview.chromium.org/2121613002/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/media/video-controls-captions-on-off.html:40: > function checkCaptionsDisplay(captionsVisible) { > On 2016/07/04 08:46:55, fs wrote: > > Could probably also be split into two functions, eliminating the argument. I.e > > checkCaptionsVisible and checkCaptionsHidden or so. Up to you. > > Done. > > https://codereview.chromium.org/2121613002/diff/40001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/media/video-controls-captions.html (right): > > https://codereview.chromium.org/2121613002/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/media/video-controls-captions.html:13: var track > = document.querySelector("track"); > On 2016/07/04 08:46:55, fs wrote: > > We could move this into the relevant scope, right? (And make 'track' a local > > elsewhere.) > > Done. > > https://codereview.chromium.org/2121613002/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/media/video-controls-captions.html:21: > assert_throws(null, function() { > On 2016/07/04 08:49:31, Philip Jägenstedt wrote: > > On 2016/07/04 07:56:56, Srirama wrote: > > > This is for "text track container" error and the one below is for "cue" > error. > > > So should i compare the error message to differentiate or is there a better > > way? > > > > It's not always clear what checkCaptionsDisplay is hoping to check, but > without > > the error messages in -expected.txt this is indeed testing less than before. > > textTrackDisplayElement is doing a lot, this might all look simpler if it's > > split into helpers: > > > > textTrackContainerElement > > textTrackCueElementById > > textTrackCueElementAtIndex > > > > Then here just use assert_equals(textTrackContainerElement(video), null) and > > below maybe textTrackCueElementAtIndex(0) if that returns null if out of > range? > > > > Or if the only interesting this is to check if something is rendered or not, > > which seems to be the case, write a helper that just checks if the bounding > > client rect of the container is zero or not. (Not tested.) > > Attempted the first approach. The helpers are a bit interdependent right now, or > should i make them independent with each taking "video" as parameter? > > https://codereview.chromium.org/2121613002/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/media/video-controls-captions.html:35: > video.removeChild(track); > On 2016/07/04 08:49:32, Philip Jägenstedt wrote: > > Just track.remove() works too. > > Done. > > https://codereview.chromium.org/2121613002/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/media/video-controls-captions.html:41: > selectTextTrackAtIndex(video, 0); > On 2016/07/04 08:49:31, Philip Jägenstedt wrote: > > Since you're renaming it anyway, I think clickTextTrackAtIndex would be even > > better, so that it's clear this isn't using the DOM-exposed APIs to enable the > > track. > > Done. > > https://codereview.chromium.org/2121613002/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/media/video-controls-captions.html:50: track = > document.createElement("track"); > On 2016/07/04 08:46:55, fs wrote: > > Make 'track' local (per above.) > > Done. > > https://codereview.chromium.org/2121613002/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/media/video-controls-captions.html:60: > newTrack.addCue(new VTTCue(0, 10, "Some random caption text")); > On 2016/07/04 08:49:31, Philip Jägenstedt wrote: > > Is this line really needed for the test to pass? The number of cues aren't > part > > of the logic for text track UI I think. > > Done. @foolip, one more pass through it please.
https://codereview.chromium.org/2121613002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/media-controls.js (right): https://codereview.chromium.org/2121613002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/media-controls.js:76: function textTrackCueElementById(parentElement, id) { It looks like you're only using this in one place and there you're using "display" for the id, isn't that the same as textTrackDisplayElement(video, 'display')? https://codereview.chromium.org/2121613002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/media-controls.js:95: function textTrackCueElement(containerElement, controlID){ This helper doesn't seem to do much, going back to using .firstchild where used seems OK. https://codereview.chromium.org/2121613002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/media-controls.js:110: if (controlID != 'cue') This special casing still seems silly. "cue" is never used together with cueNumber, it would seem better to just have a separate function for getting to that element. Overall, I think it'd be an improvement if existing uses of textTrackDisplayElement() were replaced with more specific functions and this were removed. Maybe: textTrackDisplayElement(video); => textTrackContainerElement(video) textTrackDisplayElement(video, 'cue') => textTrackCueElementByIndex(video, 0) // I think? textTrackDisplayElement(video, 'display') => textTrackDisplayElement(video) // old name, new meaning textTrackDisplayElement(video, 'display', i) => textTrackCueElementByIndex(video, i) textTrackDisplayElement(video, 'region') => textTrackRegionElement(video) textTrackDisplayElement(video, 'region-container') => textTrackRegionContainerElement(video) Would that work?
https://codereview.chromium.org/2121613002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/media-controls.js (right): https://codereview.chromium.org/2121613002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/media-controls.js:76: function textTrackCueElementById(parentElement, id) { On 2016/07/05 13:37:00, Philip Jägenstedt wrote: > It looks like you're only using this in one place and there you're using > "display" for the id, isn't that the same as textTrackDisplayElement(video, > 'display')? Just to return null and check for null instead of checking for exception. Anyway, i have changed it as per below suggestion. Right now i have named it "textTrackCueDisplayElement", once we remove all the uses of existing "textTrackDisplayElement", i will change it. https://codereview.chromium.org/2121613002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/media-controls.js:95: function textTrackCueElement(containerElement, controlID){ On 2016/07/05 13:37:00, Philip Jägenstedt wrote: > This helper doesn't seem to do much, going back to using .firstchild where used > seems OK. Done. https://codereview.chromium.org/2121613002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/media-controls.js:110: if (controlID != 'cue') On 2016/07/05 13:37:00, Philip Jägenstedt wrote: > This special casing still seems silly. "cue" is never used together with > cueNumber, it would seem better to just have a separate function for getting to > that element. Overall, I think it'd be an improvement if existing uses of > textTrackDisplayElement() were replaced with more specific functions and this > were removed. > > Maybe: > > textTrackDisplayElement(video); => textTrackContainerElement(video) > > textTrackDisplayElement(video, 'cue') => textTrackCueElementByIndex(video, 0) // > I think? > > textTrackDisplayElement(video, 'display') => textTrackDisplayElement(video) // > old name, new meaning > > textTrackDisplayElement(video, 'display', i) => > textTrackCueElementByIndex(video, i) > > textTrackDisplayElement(video, 'region') => textTrackRegionElement(video) > > textTrackDisplayElement(video, 'region-container') => > textTrackRegionContainerElement(video) > > Would that work? Yes, i have added the required functions for this CL, and a TODO to implement remaining later.
https://codereview.chromium.org/2121613002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/media-controls.js (right): https://codereview.chromium.org/2121613002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/media-controls.js:83: var displayElement = mediaControlsElement(containerElement.firstChild, "-webkit-media-text-track-display"); Can this just call textTrackCueDisplayElement(parentElement), and remove previous line? https://codereview.chromium.org/2121613002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/video-controls-captions-on-off.html (right): https://codereview.chromium.org/2121613002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-controls-captions-on-off.html:16: for (var i = 0; i < 3; i++) { The hard-coded 3 is a bit unsightly, but oh well. https://codereview.chromium.org/2121613002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-controls-captions-on-off.html:47: assert_equals(textTrackCueElementByIndex(video, i), null); Simplify this to just assert_equals(textTrackCueElementByIndex(video, 0), null) as in the other test? https://codereview.chromium.org/2121613002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/video-controls-overlay-play-button.html (right): https://codereview.chromium.org/2121613002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-controls-overlay-play-button.html:14: document.documentElement.appendChild(video); That's a weird place to have the video element, can you put it in document.body assuming this isn't intended to test something funny? https://codereview.chromium.org/2121613002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-controls-overlay-play-button.html:21: video.onloadeddata = null; The structure of this test is honestly quite hard to follow, can you try using EventWatcher like in LayoutTests/media/autoplay-muted.html? You might try using that in other places where I suggested following another style as well. https://codereview.chromium.org/2121613002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/video-controls-track-selection-menu.html (right): https://codereview.chromium.org/2121613002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-controls-track-selection-menu.html:44: assert_equals(textTrackCueElementByIndex(video, 0).innerText, "first caption"); It's a bit crazy that the layout is updated synchronously when clicking the button like this, but oh well, it's testing what the behavior actually is :)
https://codereview.chromium.org/2121613002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/video-controls-overlay-play-button.html (right): https://codereview.chromium.org/2121613002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-controls-overlay-play-button.html:21: video.onloadeddata = null; On 2016/07/06 10:00:47, Philip Jägenstedt wrote: > The structure of this test is honestly quite hard to follow, can you try using > EventWatcher like in LayoutTests/media/autoplay-muted.html? You might try using > that in other places where I suggested following another style as well. I'm afraid i understood how we can apply eventwatcher here. Just had a look at the eventwatcher interface, and it is good to test a series of events but how can we execute a specific function on each event, like this current test case? Or do you want me to use multiple eventwatchers?
https://codereview.chromium.org/2121613002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/video-controls-overlay-play-button.html (right): https://codereview.chromium.org/2121613002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-controls-overlay-play-button.html:21: video.onloadeddata = null; On 2016/07/06 10:53:18, Srirama wrote: > On 2016/07/06 10:00:47, Philip Jägenstedt wrote: > > The structure of this test is honestly quite hard to follow, can you try using > > EventWatcher like in LayoutTests/media/autoplay-muted.html? You might try > using > > that in other places where I suggested following another style as well. > > I'm afraid i understood how we can apply eventwatcher here. Just had a look at > the eventwatcher interface, and it is good to test a series of events but how > can we execute a specific function on each event, like this current test case? > Or do you want me to use multiple eventwatchers? I haven't done it myself, but https://github.com/w3c/web-platform-tests/blob/master/html/semantics/embedded... looks like an OK example. So maybe something like this: var video = document.createElement("video"); video.controls = true; var button = mediaControlsButton(video, "overlay-play-button"); assert_equals(getComputedStyle(button).display, "flex"); var watcher = new EventWatcher(t, video, ['loadeddata', 'play', 'pause']); video.src = findMediaFile("video", "content/test"); watcher.wait_for('loadeddata').then(t.step_func(function() { video.play(); return watcher.wait_for('play'); }).then(t.step_func(function() { assert_equals(getComputedStyle(button).display, "none"); video.pause(); return watcher.wait_for('pause'); }).... And so on.
https://codereview.chromium.org/2121613002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/video-controls-overlay-play-button.html (right): https://codereview.chromium.org/2121613002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-controls-overlay-play-button.html:21: video.onloadeddata = null; On 2016/07/06 at 10:53:18, Srirama wrote: > On 2016/07/06 10:00:47, Philip Jägenstedt wrote: > > The structure of this test is honestly quite hard to follow, can you try using > > EventWatcher like in LayoutTests/media/autoplay-muted.html? You might try using > > that in other places where I suggested following another style as well. > > I'm afraid i understood how we can apply eventwatcher here. Just had a look at the eventwatcher interface, and it is good to test a series of events but how can we execute a specific function on each event, like this current test case? Or do you want me to use multiple eventwatchers? I think the mechanism for that is something like: var watcher = new EventWatcher(...); watcher.wait_for(something).then(function() { // got something });
https://codereview.chromium.org/2121613002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/media-controls.js (right): https://codereview.chromium.org/2121613002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/media-controls.js:83: var displayElement = mediaControlsElement(containerElement.firstChild, "-webkit-media-text-track-display"); On 2016/07/06 10:00:47, Philip Jägenstedt wrote: > Can this just call textTrackCueDisplayElement(parentElement), and remove > previous line? Done. https://codereview.chromium.org/2121613002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/video-controls-captions-on-off.html (right): https://codereview.chromium.org/2121613002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-controls-captions-on-off.html:16: for (var i = 0; i < 3; i++) { On 2016/07/06 10:00:47, Philip Jägenstedt wrote: > The hard-coded 3 is a bit unsightly, but oh well. Replaced with text.length. https://codereview.chromium.org/2121613002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-controls-captions-on-off.html:47: assert_equals(textTrackCueElementByIndex(video, i), null); On 2016/07/06 10:00:47, Philip Jägenstedt wrote: > Simplify this to just assert_equals(textTrackCueElementByIndex(video, 0), null) > as in the other test? Done. https://codereview.chromium.org/2121613002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/video-controls-overlay-play-button.html (right): https://codereview.chromium.org/2121613002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-controls-overlay-play-button.html:14: document.documentElement.appendChild(video); On 2016/07/06 10:00:47, Philip Jägenstedt wrote: > That's a weird place to have the video element, can you put it in document.body > assuming this isn't intended to test something funny? To append to body, we need a body tag in the test, so added body tag as well. https://codereview.chromium.org/2121613002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-controls-overlay-play-button.html:21: video.onloadeddata = null; On 2016/07/06 11:11:52, fs wrote: > On 2016/07/06 at 10:53:18, Srirama wrote: > > On 2016/07/06 10:00:47, Philip Jägenstedt wrote: > > > The structure of this test is honestly quite hard to follow, can you try > using > > > EventWatcher like in LayoutTests/media/autoplay-muted.html? You might try > using > > > that in other places where I suggested following another style as well. > > > > I'm afraid i understood how we can apply eventwatcher here. Just had a look at > the eventwatcher interface, and it is good to test a series of events but how > can we execute a specific function on each event, like this current test case? > Or do you want me to use multiple eventwatchers? > > I think the mechanism for that is something like: > > var watcher = new EventWatcher(...); > watcher.wait_for(something).then(function() { > // got something > }); Done. Thanks @both for the info. https://codereview.chromium.org/2121613002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/video-controls-track-selection-menu.html (right): https://codereview.chromium.org/2121613002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-controls-track-selection-menu.html:44: assert_equals(textTrackCueElementByIndex(video, 0).innerText, "first caption"); On 2016/07/06 10:00:47, Philip Jägenstedt wrote: > It's a bit crazy that the layout is updated synchronously when clicking the > button like this, but oh well, it's testing what the behavior actually is :) Acknowledged.
lgtm % </body> https://codereview.chromium.org/2121613002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/video-controls-overlay-play-button.html (right): https://codereview.chromium.org/2121613002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/video-controls-overlay-play-button.html:7: <body></body> Closing the body has no effect, just <body> here will suffice.
https://codereview.chromium.org/2121613002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/video-controls-overlay-play-button.html (right): https://codereview.chromium.org/2121613002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/video-controls-overlay-play-button.html:7: <body></body> On 2016/07/06 11:39:41, Philip Jägenstedt wrote: > Closing the body has no effect, just <body> here will suffice. Done.
The CQ bit was checked by srirama.m@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com, foolip@chromium.org Link to the patchset: https://codereview.chromium.org/2121613002/#ps120001 (title: "address nit")
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 ========== Convert video-controls-[captions, overlay, track]* tests to testharness.js Cleaning up video-controls-[captions, overlay, track]* tests in media/ to use testharness.js instead of video-test.js. This will enable to upstream these tests to web-platform-tests. BUG=588956 ========== to ========== Convert video-controls-[captions, overlay, track]* tests to testharness.js Cleaning up video-controls-[captions, overlay, track]* tests in media/ to use testharness.js instead of video-test.js. This will enable to upstream these tests to web-platform-tests. BUG=588956 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Convert video-controls-[captions, overlay, track]* tests to testharness.js Cleaning up video-controls-[captions, overlay, track]* tests in media/ to use testharness.js instead of video-test.js. This will enable to upstream these tests to web-platform-tests. BUG=588956 ========== to ========== Convert video-controls-[captions, overlay, track]* tests to testharness.js Cleaning up video-controls-[captions, overlay, track]* tests in media/ to use testharness.js instead of video-test.js. This will enable to upstream these tests to web-platform-tests. BUG=588956 Committed: https://crrev.com/bb48314aaf3f1e4353cf70411dd969383f2f5c0b Cr-Commit-Position: refs/heads/master@{#403890} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/bb48314aaf3f1e4353cf70411dd969383f2f5c0b Cr-Commit-Position: refs/heads/master@{#403890} |
