Convert video-controls-[captions, overlay, track]* tests to testharness.js

third_party/WebKit/LayoutTests/media/media-controls.js

Index: third_party/WebKit/LayoutTests/media/media-controls.js
diff --git a/third_party/WebKit/LayoutTests/media/media-controls.js b/third_party/WebKit/LayoutTests/media/media-controls.js
index b923623ff72fe17809c197bc339bbf72e5c5e7a2..8a4259ca54d6c7145307405044818f41b986bcd0 100644
--- a/third_party/WebKit/LayoutTests/media/media-controls.js
+++ b/third_party/WebKit/LayoutTests/media/media-controls.js
@@ -68,10 +68,37 @@ function mediaControlsButtonDimensions(element, id)
     return new Array(buttonBoundingRect.width, buttonBoundingRect.height);
 }
 
+function textTrackContainerElement(parentElement) {
+    return mediaControlsElement(internals.shadowRoot(parentElement).firstChild,
+        "-webkit-media-text-track-container");
+}
+
+function textTrackCueElementById(parentElement, id) {

[foolip, 2016/07/05 13:37:00]

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')?

[Srirama, 2016/07/05 18:33:28]

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.

+    var containerElement = textTrackContainerElement(parentElement);
+    if (!containerElement)
+        return null;
+
+    var controlID = id;
+    if (controlID != 'cue')
+        controlID = "-webkit-media-text-track-" + id;
+
+    return textTrackCueElement(containerElement, controlID);
+}
+
+function textTrackCueElementByIndex(displayElement, cueNumber) {
+    for (i = 0; i < cueNumber; i++)
+        displayElement = displayElement.nextSibling;
+
+    return displayElement;
+}
+
+function textTrackCueElement(containerElement, controlID){

[foolip, 2016/07/05 13:37:00]

This helper doesn't seem to do much, going back to using .firstchild where used
seems OK.

[Srirama, 2016/07/05 18:33:28]

Done.

+    return mediaControlsElement(containerElement.firstChild, controlID);
+}
+
 function textTrackDisplayElement(parentElement, id, cueNumber)
 {
-    var textTrackContainerID = "-webkit-media-text-track-container";
-    var containerElement = mediaControlsElement(internals.shadowRoot(parentElement).firstChild, textTrackContainerID);
+    var containerElement = textTrackContainerElement(parentElement);
 
     if (!containerElement)
         throw "Failed to find text track container element";
@@ -79,18 +106,16 @@ function textTrackDisplayElement(parentElement, id, cueNumber)
     if (!id)
         return containerElement;
 
-    if (arguments[1] != 'cue')
-        var controlID = "-webkit-media-text-track-" + arguments[1];
-    else
-        var controlID = arguments[1];
+    var controlID = id;
+    if (controlID != 'cue')

[foolip, 2016/07/05 13:37:00]

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?

[Srirama, 2016/07/05 18:33:28]

Yes, i have added the required functions for this CL, and a TODO to implement
remaining later.

+        controlID = "-webkit-media-text-track-" + id;
 
-    var displayElement = mediaControlsElement(containerElement.firstChild, controlID);
+    var displayElement = textTrackCueElement(containerElement, controlID);
     if (!displayElement)
         throw "No text track cue with display id '" + controlID + "' is currently visible";
 
     if (cueNumber) {
-        for (i = 0; i < cueNumber; i++)
-            displayElement = displayElement.nextSibling;
+        displayElement = textTrackCueElementByIndex(displayElement, cueNumber);
 
         if (!displayElement)
             throw "There are not " + cueNumber + " text track cues visible";
@@ -174,9 +199,18 @@ function selectTextTrack(video, index)
     clickAtCoordinates(trackListItemCoordinates[0], trackListItemCoordinates[1]);
 }
 
+function clickTextTrackAtIndex(video, index)
+{
+    var captionsButtonCoordinates = mediaControlsButtonCoordinates(video, "toggle-closed-captions-button");
+    clickAtCoordinates(captionsButtonCoordinates[0], captionsButtonCoordinates[1]);
+    var trackListItemElement = textTrackListItemAtIndex(video, index);
+    var trackListItemCoordinates = elementCoordinates(trackListItemElement);
+    clickAtCoordinates(trackListItemCoordinates[0], trackListItemCoordinates[1]);
+}
+
 function turnClosedCaptionsOff(video)
 {
-    selectTextTrack(video, -1);
+    clickTextTrackAtIndex(video, -1);
 }
 
 function runAfterHideMediaControlsTimerFired(func, mediaElement)