Bug 54634 – media/video-controls-in-media-document.html has image+text diffs

• Component: Media
• Status: RESOLVED FIXED

[Mikhail Naganov]

See build logs, all bots are agree on this:

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Win/builds/1585
http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux/builds/2434
http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.5/builds/1302

[Mihai Parparita]

Looks like this test still references media-file.js from its old location, probably because it was added the patch for r78778 was created (with http://trac.webkit.org/changeset/78680)

We are woking on the fix right now.

[Anna Cavender]

Looks like this has been caught and is being handled:
https://bugs.webkit.org/show_bug.cgi?id=54631

[Mihai Parparita]

For some reason, the test was skipped, once I unskipped it with http://trac.webkit.org/changeset/80293 it started to failed with text+image diffs:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20chromium.org&tests=media%2Fvideo-controls-in-media-document.html&showExpectations=true

It was skipped because of this change
http://trac.webkit.org/changeset/78811/trunk/LayoutTests/platform/chromium/test_expectations.txt

[Mihai Parparita]

(In reply to comment #5)
> It was skipped because of this change
> http://trac.webkit.org/changeset/78811/trunk/LayoutTests/platform/chromium/test_expectations.txt

I think you mean
http://trac.webkit.org/changeset/78809/trunk/LayoutTests/platform/chromium/test_expectations.txt, but yes.

Mikhail, I don't think we normally want to skip tests that we want to eventually fix (unless they're impact tests that run after them), since otherwise it's hard to track changes in behavior (e.g. this test used to time out, but now it has a text/image diff).

[Ami Fischman]

Created attachment 85258 [details]
Patch

[Mihai Parparita]

> LayoutTests/media/video-controls-in-media-document.html:9
> +  layoutTestController.waitUntilDone();

WebKit JavaScript generally uses 4 space indents, like the rest of WebKit code.

> LayoutTests/media/video-controls-in-media-document.html:26
> +  // the rendered video+controls but the controls are part of the shadow DOM and

Dimitri might have ideas about getting at these dimensions in a test without having to use pixel output.

[Ami Fischman]

Created attachment 85334 [details]
Patch

[Ami Fischman]

(In reply to comment #8)
> > LayoutTests/media/video-controls-in-media-document.html:9
> > +  layoutTestController.waitUntilDone();
> WebKit JavaScript generally uses 4 space indents, like the rest of WebKit code.

Done.

> > LayoutTests/media/video-controls-in-media-document.html:26
> > +  // the rendered video+controls but the controls are part of the shadow DOM and
> Dimitri might have ideas about getting at these dimensions in a test without having to use pixel output.

That'd be awesome.  I hate that this is a pixel test.

[Mihai Parparita]

Comment on attachment 85334 [details]
Patch

Will r+/cq+ for now. The commit queue is backed up enough that if Dimitri has any ideas there'll be time to upload a new patch.

[WebKit Commit Bot]

Comment on attachment 85334 [details]
Patch

Clearing flags on attachment: 85334

Committed r80820: <http://trac.webkit.org/changeset/80820>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ami Fischman]

Even though my patch landed, this bug would need to stay open at the very least to track rebaselining chromium win & mac.
Sadly, even chromium linux is only passing intermittently, because the controls fade-in effect is not reliably completing in under 100ms.  

At this point I'm inclined to simply delete the test and eat the resulting coverage loss unless we can come up with up with a way to test the total height of the video+controls or the placement of the controls in the shadow DOM.  

Anybody want to suggest / lobby for an alternative?

[David Levin]

Reopening per previous comment.

[Eric Carlson]

(In reply to comment #14)
> Even though my patch landed, this bug would need to stay open at the very least to track rebaselining chromium win & mac.
> Sadly, even chromium linux is only passing intermittently, because the controls fade-in effect is not reliably completing in under 100ms.  
> 
> At this point I'm inclined to simply delete the test and eat the resulting coverage loss unless we can come up with up with a way to test the total height of the video+controls or the placement of the controls in the shadow DOM.  
> 
> Anybody want to suggest / lobby for an alternative?

Being able to examine the shadow DOM from DRT seems like the best way to go.

[Eric Carlson]

(In reply to comment #16)
> 
> Being able to examine the shadow DOM from DRT seems like the best way to go.

This would also be useful for the (currently platform specific) tests that post mouse events to test the default media controls.

[Ami Fischman]

shadowRoot is now exposed to chromium DRT (in 56573).
This bug (for chromium) is now blocked on media elements being converted to setShadowRoot (from setShadowHost), tracked by 53020.
For non-chromium DRT, this bug is also blocked on 57415.

[Ami Fischman]

Created attachment 89028 [details]
Test rewritten to use shadow DOM explicitly.

[Ami Fischman]

*** Bug 54757 has been marked as a duplicate of this bug. ***

[Eric Carlson]

Comment on attachment 89028 [details]
Test rewritten to use shadow DOM explicitly.

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

> LayoutTests/media/video-controls-in-media-document.html:15
> +    var controls = layoutTestController.shadowRoot(video).firstChild.firstChild;
> +    testExpected(controls.offsetTop, 208);  // == 240(video height) minus 32 (controls height).
> +    layoutTestController.notifyDone();

Hard coding the size of the controls doesn't seem like a great idea as ports are free to use different sizes. Because this tests is to make sure controls are rendered on top of the <video> element, why not just test that the top of the controls element is above the bottom of the video element?

> LayoutTests/platform/mac/Skipped:2
> -# Copyright (C) 2008 Apple Inc. All rights reserved.
> +# Copyright (C) 2011 Apple Inc. All rights reserved.

An Apple notice copyright should include all years, eg. "2008, 2009, 2010, 2011".

[Ami Fischman]

Created attachment 89057 [details]
Use shadow DOM and avoid hardcoding controls height.

[Ami Fischman]

(In reply to comment #21)
> (From update of attachment 89028 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89028&action=review
> 
> > LayoutTests/media/video-controls-in-media-document.html:15
> > +    var controls = layoutTestController.shadowRoot(video).firstChild.firstChild;
> > +    testExpected(controls.offsetTop, 208);  // == 240(video height) minus 32 (controls height).
> > +    layoutTestController.notifyDone();
> 
> Hard coding the size of the controls doesn't seem like a great idea as ports are free to use different sizes. Because this tests is to make sure controls are rendered on top of the <video> element, why not just test that the top of the controls element is above the bottom of the video element?

Done.

> 
> > LayoutTests/platform/mac/Skipped:2
> > -# Copyright (C) 2008 Apple Inc. All rights reserved.
> > +# Copyright (C) 2011 Apple Inc. All rights reserved.
> 
> An Apple notice copyright should include all years, eg. "2008, 2009, 2010, 2011".

Left alone (2008) to avoid rocking the boat (this was an auto-edit; will config emacs to leave webkit copyrights alone).

[Eric Carlson]

Comment on attachment 89057 [details]
Use shadow DOM and avoid hardcoding controls height.

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

> LayoutTests/media/video-controls-in-media-document.html:15
> +    var controls = layoutTestController.shadowRoot(video).firstChild.firstChild;
> +    test(controls.offsetTop < 240);
> +    layoutTestController.notifyDone();

This doesn't hard code the control height, so why use "240" for the video height? Maybe I am being pedantic because we *do* know the video dimensions, but is there any reason to not calculate the height?

Also I think it is good to structure a "test(...)" so the output shows what is being tested instead of just the result. 

For example:

    testExpected("controls.offsetTop", video.offsetHeight, "<");

will generate:

    EXPECTED (video.offsetTop < '240') OK

This can be very helpful to someone looking at a test's results later, as they don't have to view source to figure out what failed.

[Ami Fischman]

Created attachment 89069 [details]
Use shadow DOM and avoid hardcoding controls height.

[Ami Fischman]

(In reply to comment #24)
> This doesn't hard code the control height, so why use "240" for the video height? Maybe I am being pedantic because we *do* know the video dimensions, but is there any reason to not calculate the height?

My motivation is that I was worried that video.offsetHeight might end up including the height of the controls and I wanted to avoid having to worry about finding the canvas part of the element, so chose readability.  But it seems like that worry was unfounded, so changed.

> Also I think it is good to structure a "test(...)" so the output shows what is being tested instead of just the result. 

Agreed.

> For example:
>     testExpected("controls.offsetTop", video.offsetHeight, "<");
> will generate: 
>     EXPECTED (video.offsetTop < '240') OK

In fact it generates: ReferenceError: controls is not defined :)
I didn't think about it too much before and just changed to test(), but I can regain the kind of message you want by dropping the "var" to bleed |controls| out of function scope.  Done.

[Eric Carlson]

Comment on attachment 89069 [details]
Use shadow DOM and avoid hardcoding controls height.

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

Thanks!

> LayoutTests/media/video-controls-in-media-document.html:14
> +    testExpected("controls.offsetTop + controls.offsetHeight", video.offsetHeight, "<=");

Much better than what I suggested!

[WebKit Commit Bot]

Comment on attachment 89069 [details]
Use shadow DOM and avoid hardcoding controls height.

Rejecting attachment 89069 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'bu..." exit_code: 2

Last 500 characters of output:
va/lc3/JavaObject .....................................
java/lc3/StringMethods .
java/lc3/forin .
java/lc3/instanceof .
jquery ..........
loader ..
mathml ........
mathml/presentation ..................
media ................................................
media/video-controls-in-media-document.html -> failed

Exiting early after 1 failures. 13389 tests run.
386.18s total testing time

13388 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
10 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/8390101

[Ami Fischman]

Help?  I don't see what the failure is.  I suspect the test runner didn't process the expectation rm's correctly but I can't see any of the actual/expected/diff from the URL given (http://queues.webkit.org/results/8390101).
Eric - do you see what went wrong?

[Eric Seidel (no email)]

Unfortunately the cq doesn't know how to upload failure results yet. :(  I expect the test simply fails on Mac Snow Leopard.

[Ami Fischman]

Created attachment 89283 [details]
Now passes on OS/X webkit, too.

[Ami Fischman]

As Eric S. guessed, CQ failure was legit.  Updated patch fixes that, by removing the bogus bottom padding from mediaControls{,Efl,QuickTime}.css.
I say "bogus" b/c it makes the media experience difference in media documents and in <video>-using HTML, and because it makes difficult to reason about the rendered height of the media document (which can be relevant e.g. when using iframes).  This patch makes the media document case consistent with the non-media-document case.
Hopefully it doesn't make a whole bunch of (unrelated) layouttests fail.

[Jer Noble]

(In reply to comment #32)
> As Eric S. guessed, CQ failure was legit.  Updated patch fixes that, by removing the bogus bottom padding from mediaControls{,Efl,QuickTime}.css.
> I say "bogus" b/c it makes the media experience difference in media documents and in <video>-using HTML, and because it makes difficult to reason about the rendered height of the media document (which can be relevant e.g. when using iframes).  This patch makes the media document case consistent with the non-media-document case.
> Hopefully it doesn't make a whole bunch of (unrelated) layouttests fail.

Avi, there isn't a Sources/WebCore/ChangeLog entry explaining the CSS changes.  Those changes are also unrelated to this bug, so if you feel they are necessary, please file a new bug to that effect, and make the CSS changes in that new bug.

[Ami Fischman]

(In reply to comment #33)
> there isn't a Sources/WebCore/ChangeLog entry explaining the CSS changes.  Those changes are also unrelated to this bug, so if you feel they are necessary, please file a new bug to that effect, and make the CSS changes in that new bug.

ChangeLog was missing b/c I assumed webkit-patch would complain about needed updates (it seems it stays quiet as long as there's *some* ChangeLog involved in the diff, even if not all dirs are covered).

Just-css & changelog patch uploaded to new bug 58442 which this one is now blocked on.  Will upload updated test-only patch (without css changes) to this bug in a moment.

[Ami Fischman]

Created attachment 89379 [details]
CSS changes removed.

[Eric Carlson]

Cleared cq+ at Ami's request, this won't work until https://bugs.webkit.org/show_bug.cgi?id=58442 has landed.

[Ami Fischman]

(In reply to comment #36)
> Cleared cq+ at Ami's request, this won't work until https://bugs.webkit.org/show_bug.cgi?id=58442 has landed.

58442 has landed, so this patch is now open for cq+ business again.

[WebKit Commit Bot]

Comment on attachment 89379 [details]
CSS changes removed.

Clearing flags on attachment: 89379

Committed r83947: <http://trac.webkit.org/changeset/83947>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Roben (:aroben)]

This test is timing out on Windows: http://build.webkit.org/results/Windows%20XP%20Debug%20%28Tests%29/r83947%20(27652)/media/video-controls-in-media-document-pretty-diff.html