third_party/WebKit/LayoutTests/media/track/track-css-user-settings-override-author-settings.html - Issue 1882583002: Convert track tests from video-test.js to testharness.js based

Side by Side Diff: third_party/WebKit/LayoutTests/media/track/track-css-user-settings-override-author-settings.html

Issue 1882583002: Convert track tests from video-test.js to testharness.js based (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: Created 4 years, 8 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

« third_party/WebKit/LayoutTests/media/track/track-css-property-whitelist.html ('K') | « third_party/WebKit/LayoutTests/media/track/track-css-property-whitelist-expected.txt ('k') | third_party/WebKit/LayoutTests/media/track/track-css-user-settings-override-author-settings-expected.txt » ('j') | third_party/WebKit/LayoutTests/media/track/track-css-user-settings-override-internal-settings.html » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
1 <!DOCTYPE html>	1 <!DOCTYPE html>

2 <script src=../media-file.js></script>	2 <title>Test that WebVTT objects are being styled correctly based on user setting s that should override author settings.</title>

3 <!-- TODO(philipj): Convert test to testharness.js. crbug.com/588956	3 <script src="../media-file.js"></script>

4 (Please avoid writing new tests using video-test.js) -->	4 <script src="../media-controls.js"></script>

5 <script src=../video-test.js></script>	5 <script src="../../resources/testharness.js"></script>

6 <script src=../media-controls.js></script>	6 <script src="../../resources/testharnessreport.js"></script>

7 <style>	7 <style>

8 /* Author settings for the cues */	8 /* Author settings for the cues */

9 video::cue(c) {	9 video::cue(c) {

10 color: red;	10 color: red;

11 background-color: green;	11 background-color: green;

12 text-shadow: 3px 3px #00ff00;	12 text-shadow: 3px 3px #00ff00;

13 font-size: 20px;	13 font-size: 20px;

14 font-family: arial;	14 font-family: arial;

15 font-style: normal;	15 font-style: normal;

16 font-variant: normal;	16 font-variant: normal;

17 }	17 }

18 </style>	18 </style>

	19 <video>

	20 <track src="captions-webvtt/styling.vtt" kind="captions" default>

	21 </video>

19 <script>	22 <script>

	23 async_test(function(t) {

	24 var video = document.querySelector("video");

	25 video.src = findMediaFile("video", "../content/test");
	mlamouri (slow - plz ping) 2016/04/13 12:58:30 The test used to wait for the load event and set c The test used to wait for the load event and set currentTime to 0.1. I'm not sure why it used to do that. Do you know why? Srirama 2016/04/13 13:19:46 Previously we are calling findMediaElement() to ge Show quoted text On 2016/04/13 12:58:30, Mounir Lamouri wrote: > The test used to wait for the load event and set currentTime to 0.1. I'm not > sure why it used to do that. Do you know why? Previously we are calling findMediaElement() to get the "video" element and then set the "src". This requires window onload event. But now that i have moved "video" tag before this script, window would have already loaded before this script gets executed and we need not wait for onload event specifically. philipj_slow 2016/04/15 14:04:50 AFAICT, it was just because the video element was Show quoted text On 2016/04/13 13:19:46, Srirama wrote: > On 2016/04/13 12:58:30, Mounir Lamouri wrote: > > The test used to wait for the load event and set currentTime to 0.1. I'm not > > sure why it used to do that. Do you know why? > > Previously we are calling findMediaElement() to get the "video" element and then > set the "src". This requires window onload event. But now that i have moved > "video" tag before this script, window would have already loaded before this > script gets executed and we need not wait for onload event specifically. AFAICT, it was just because the video element was after the script tag, now it's before and since there's no resource block the load event here, the timing will be almost exactly the same. (In some tests with multiple media elements, the window load event can be used to make sure all have reached HAVE_CURRENT_DATA, because "Once the readyState attribute reaches HAVE_CURRENT_DATA, after the loadeddata event has been fired, set the element's delaying-the-load-event flag to false. This stops delaying the load event." Not here.)
20	26

21 function applyUserOverrideSettings() {	27 video.oncanplaythrough = t.step_func_done(function() {

22 if (window.internals) {	28 var cue = textTrackDisplayElement(video, "cue").firstElementChild;

	29 var cueStyle = getComputedStyle(cue);

	30 assert_equals(cueStyle.color, "rgb(255, 0, 0)");

	31 assert_equals(cueStyle.backgroundColor, "rgb(0, 128, 0)");

	32 assert_equals(cueStyle.textShadow, "rgb(0, 255, 0) 3px 3px 0px");

	33 assert_equals(cueStyle.fontSize, "20px");

	34 assert_equals(cueStyle.fontFamily, "arial");

	35 assert_equals(cueStyle.fontStyle, "normal");

	36 assert_equals(cueStyle.fontVariant, "normal");

	37

	38 // Apply user settings and verify they override author-specified setting s

	39 if (internals) {
	philipj_slow 2016/04/15 14:04:50 Need to say window.internals, otherwise it'll thro Need to say window.internals, otherwise it'll throw. I think just removing this check is fine, because the asserts that follow will fail if the code isn't run anyway. Srirama 2016/04/16 06:44:32 It is working without "window." prefix as well, I Show quoted text On 2016/04/15 14:04:50, philipj_slow wrote: > Need to say window.internals, otherwise it'll throw. I think just removing this > check is fine, because the asserts that follow will fail if the code isn't run > anyway. It is working without "window." prefix as well, I guess it is because window is a global object and it is same like how we use "setInterval" "alert" functions... Anyway i have removed the condition. philipj_slow 2016/04/21 11:38:09 It works as long as window.internals exists, but t Show quoted text On 2016/04/16 06:44:32, Srirama wrote: > On 2016/04/15 14:04:50, philipj_slow wrote: > > Need to say window.internals, otherwise it'll throw. I think just removing > this > > check is fine, because the asserts that follow will fail if the code isn't run > > anyway. > > It is working without "window." prefix as well, I guess it is because window is > a global object and it is same like how we use "setInterval" "alert" > functions... > Anyway i have removed the condition. It works as long as window.internals exists, but the point of the check is to avoid throwing an exception if it doesn't exist. Then this would throw "ReferenceError: internals is not defined" instead. So, I suggest not doing the check and letting an exception be thrown.
23 internals.settings.setTextTrackTextColor("cyan");	40 internals.settings.setTextTrackTextColor("cyan");

24 internals.settings.setTextTrackBackgroundColor("green");	41 internals.settings.setTextTrackBackgroundColor("green");

25 internals.settings.setTextTrackTextShadow("2px 2px #ff0000")	42 internals.settings.setTextTrackTextShadow("2px 2px #ff0000")

26 internals.settings.setTextTrackTextSize("14px");	43 internals.settings.setTextTrackTextSize("14px");

27 internals.settings.setTextTrackFontFamily("fantasy");	44 internals.settings.setTextTrackFontFamily("fantasy");

28 internals.settings.setTextTrackFontStyle("italic");	45 internals.settings.setTextTrackFontStyle("italic");

29 internals.settings.setTextTrackFontVariant("small-caps");	46 internals.settings.setTextTrackFontVariant("small-caps");

30 }	47 }

31 }

32	48

33 function verifyAuthorSettings() {	49 video.currentTime = 0.3;
	philipj_slow 2016/04/15 14:04:50 Does the test still pass without this? There's a n Does the test still pass without this? There's a new cue at 0.3 (compared to 0), but AFAICT the style above doesn't actually target any of the classes in that cue... Srirama 2016/04/16 06:44:32 It doesn't pass without this, so i kept it. It doe Show quoted text On 2016/04/15 14:04:50, philipj_slow wrote: > Does the test still pass without this? There's a new cue at 0.3 (compared to 0), > but AFAICT the style above doesn't actually target any of the classes in that > cue... It doesn't pass without this, so i kept it. It doesn't apply the user settings without this line. I did a bit of code walkthrough and found that the these settings are applied only when cue display is updated (VTTCue::getDisplayTree) which in turn is triggered from HTMLMediaElement and TextTrackContainer. So this line is updating the cue display which inturn updates user css properties. Is it a bug that it doesn't apply the properties without some display trigger point? philipj_slow 2016/04/21 11:38:09 Well it does seek to another cue, so this line doe Show quoted text On 2016/04/16 06:44:32, Srirama wrote: > On 2016/04/15 14:04:50, philipj_slow wrote: > > Does the test still pass without this? There's a new cue at 0.3 (compared to > 0), > > but AFAICT the style above doesn't actually target any of the classes in that > > cue... > > It doesn't pass without this, so i kept it. It doesn't apply the user settings > without this line. I did a bit of code walkthrough and found that the these > settings are applied only when cue display is updated (VTTCue::getDisplayTree) > which in turn is triggered from HTMLMediaElement and TextTrackContainer. > So this line is updating the cue display which inturn updates user css > properties. > Is it a bug that it doesn't apply the properties without some display trigger > point? Well it does seek to another cue, so this line does something. It was already like this, so OK to leave like this without further investigation.
34 consoleWrite("");	50

35 cue = textTrackDisplayElement(video, 'cue').firstElementChild;	51 cue = textTrackDisplayElement(video, "cue").firstElementChild;

36 testExpected("getComputedStyle(cue).color", "rgb(255, 0, 0)");	52 cueStyle = getComputedStyle(cue);

37 testExpected("getComputedStyle(cue).backgroundColor", "rgb(0, 128, 0)");	53 assert_equals(cueStyle.color, "rgb(0, 255, 255)");

38 testExpected("getComputedStyle(cue).textShadow", "rgb(0, 255, 0) 3px 3px 0px");	54 assert_equals(cueStyle.backgroundColor, "rgb(0, 128, 0)");

39 testExpected("getComputedStyle(cue).fontSize", "20px");	55 assert_equals(cueStyle.textShadow, "rgb(255, 0, 0) 2px 2px 0px");

40 testExpected("getComputedStyle(cue).fontFamily", "arial");	56 assert_equals(cueStyle.fontSize, "14px");

41 testExpected("getComputedStyle(cue).fontStyle", "normal");	57 assert_equals(cueStyle.fontFamily, "fantasy");

42 testExpected("getComputedStyle(cue).fontVariant", "normal");	58 assert_equals(cueStyle.fontStyle, "italic");

43 // Apply user settings and verify they override author-specified setting s	59 assert_equals(cueStyle.fontVariant, "small-caps");

44 applyUserOverrideSettings();	60 });

45 run("video.currentTime = 0.3");	61 });

46 verifyUserOverrideSettings();	62 </script>

47 }

48

49 function verifyUserOverrideSettings() {

50 consoleWrite("");

51 cue = textTrackDisplayElement(video, 'cue').firstElementChild;

52 testExpected("getComputedStyle(cue).color", "rgb(0, 255, 255)");

53 testExpected("getComputedStyle(cue).backgroundColor", "rgb(0, 128, 0)");

54 testExpected("getComputedStyle(cue).textShadow", "rgb(255, 0, 0) 2px 2px 0px");

55 testExpected("getComputedStyle(cue).fontSize", "14px");

56 testExpected("getComputedStyle(cue).fontFamily", "fantasy");

57 testExpected("getComputedStyle(cue).fontStyle", "italic");

58 testExpected("getComputedStyle(cue).fontVariant", "small-caps");

59 endTest();

60 }

61

62 window.onload = function() {

63 consoleWrite("Test that WebVTT objects are being styled correctly based on user settings that should override author settings.");

64 findMediaElement();

65 video.src = findMediaFile('video', '../content/test');

66 video.currentTime = 0.1;

67 waitForEvent('canplaythrough', verifyAuthorSettings);

68 }

69

70 </script>

71 <video>

72 <track src="captions-webvtt/styling.vtt" kind="captions" default>

73 </video>

OLD	NEW