Bug 28220 - Layout tests shouldn't have to hard code media controller element locations
Summary: Layout tests shouldn't have to hard code media controller element locations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords:
: 30680 (view as bug list)
Depends on: 60034
Blocks: 28221 60055
  Show dependency treegraph
 
Reported: 2009-08-12 11:27 PDT by Eric Carlson
Modified: 2011-05-06 02:15 PDT (History)
5 users (show)

See Also:


Attachments
proposed patch (7.41 KB, patch)
2011-05-03 03:48 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
proposed patch (8.99 KB, patch)
2011-05-05 05:53 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
proposed patch (9.06 KB, patch)
2011-05-05 06:03 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
proposed patch (9.00 KB, patch)
2011-05-05 06:06 PDT, Philippe Normand
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2009-08-12 11:27:53 PDT
A number of media layout tests simulate clicks in the controller. These tests have to change every time we change the controller layout and we now have different layouts on different platforms, so we should have a way for tests to query the location of each element.
Comment 1 Philippe Normand 2011-05-03 03:44:18 PDT
*** Bug 30680 has been marked as a duplicate of this bug. ***
Comment 2 Philippe Normand 2011-05-03 03:48:01 PDT
Created attachment 92058 [details]
proposed patch
Comment 3 Eric Carlson 2011-05-03 08:57:00 PDT
Comment on attachment 92058 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92058&action=review

Did you mean to flag this for review?

> LayoutTests/media/video-controls-zoomed.html:28
> +            // Find the play button and click the middle of its bounding box.
> +            var playCoords = mediaControlsButtonCoordinates(video, "play");
> +            var clickX = playCoords[0];
> +            var clickY = playCoords[1];
> +            clickX = clickX * 1.5;
> +            clickY = clickY * 1.5;
> +            eventSender.mouseMoveTo(clickX, clickY);

Why can't you use the coordinates returned from mediaControlsButtonCoordinates() directly?

> LayoutTests/media/video-test.js:252
> +    if (!button) {
> +        failTest("Failed to find " + id + " button.");
> +    }

You don't need the braces here.

> LayoutTests/media/video-test.js:260
> +    var result = new Array();
> +    result[0] = x;
> +    result[1] = y;
> +    return result;

You could use "return new Array(x, y)" to shorten this up slightly.
Comment 4 Philippe Normand 2011-05-03 09:24:25 PDT
Thanks for the review Eric. I unmarked it previously because the bug this one depends on needs more work actually.

(In reply to comment #3)
> (From update of attachment 92058 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92058&action=review
> 
> Did you mean to flag this for review?
> 
> > LayoutTests/media/video-controls-zoomed.html:28
> > +            // Find the play button and click the middle of its bounding box.
> > +            var playCoords = mediaControlsButtonCoordinates(video, "play");
> > +            var clickX = playCoords[0];
> > +            var clickY = playCoords[1];
> > +            clickX = clickX * 1.5;
> > +            clickY = clickY * 1.5;
> > +            eventSender.mouseMoveTo(clickX, clickY);
> 
> Why can't you use the coordinates returned from mediaControlsButtonCoordinates() directly?
> 

Without this the test failed on GTK. Could it be a bug in the port then? I thought applying the zoom to the coordinates here would be ok.

> > LayoutTests/media/video-test.js:252
> > +    if (!button) {
> > +        failTest("Failed to find " + id + " button.");
> > +    }
> 
> You don't need the braces here.
> 
> > LayoutTests/media/video-test.js:260
> > +    var result = new Array();
> > +    result[0] = x;
> > +    result[1] = y;
> > +    return result;
> 
> You could use "return new Array(x, y)" to shorten this up slightly.

Right, will do for next iteration!
Comment 5 Eric Carlson 2011-05-03 09:32:42 PDT
(In reply to comment #4)
> > > LayoutTests/media/video-controls-zoomed.html:28
> > > +            // Find the play button and click the middle of its bounding box.
> > > +            var playCoords = mediaControlsButtonCoordinates(video, "play");
> > > +            var clickX = playCoords[0];
> > > +            var clickY = playCoords[1];
> > > +            clickX = clickX * 1.5;
> > > +            clickY = clickY * 1.5;
> > > +            eventSender.mouseMoveTo(clickX, clickY);
> > 
> > Why can't you use the coordinates returned from mediaControlsButtonCoordinates() directly?
> > 
> 
> Without this the test failed on GTK. Could it be a bug in the port then? I thought applying the zoom to the coordinates here would be ok.
> 

  Oh, I guess it make sense that the coordinates returned are untransformed. A comment in the test explaining why you are transforming them here would be helpful.
Comment 6 Philippe Normand 2011-05-05 05:53:03 PDT
Created attachment 92405 [details]
proposed patch
Comment 7 Philippe Normand 2011-05-05 06:03:19 PDT
Created attachment 92407 [details]
proposed patch
Comment 8 Philippe Normand 2011-05-05 06:06:18 PDT
Created attachment 92408 [details]
proposed patch
Comment 9 Martin Robinson 2011-05-05 08:30:15 PDT
Comment on attachment 92408 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92408&action=review

Excellent work!

> LayoutTests/media/video-test.js:248
> +    var button;
> +    var controlsShadow = layoutTestController.shadowRoot(element).firstChild.firstChild;
> +    for (child = controlsShadow.firstChild; child; child = child.nextSibling) {
> +        if (layoutTestController.shadowPseudoId(child) == "-webkit-media-controls-" + id) {
> +            button = child;
> +            break;
> +        }
> +    }

It's not possible to do something like getElementById on the shadow root?
Comment 10 Philippe Normand 2011-05-05 08:46:44 PDT
I can land this after patch for bug 60034 is in the tree.
Comment 11 Philippe Normand 2011-05-06 01:02:21 PDT
Committed r85934: <http://trac.webkit.org/changeset/85934>
Comment 12 WebKit Review Bot 2011-05-06 01:59:12 PDT
http://trac.webkit.org/changeset/85934 might have broken Windows 7 Release (Tests)
The following tests are not passing:
media/video-controls-visible-audio-only.html
Comment 13 Philippe Normand 2011-05-06 02:15:04 PDT
(In reply to comment #12)
> http://trac.webkit.org/changeset/85934 might have broken Windows 7 Release (Tests)
> The following tests are not passing:
> media/video-controls-visible-audio-only.html

See bug 59081
Will skip this test on win.