Bug 54634 - media/video-controls-in-media-document.html has image+text diffs
Summary: media/video-controls-in-media-document.html has image+text diffs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 54757 (view as bug list)
Depends on: 56573 58442
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-17 04:16 PST by Mikhail Naganov
Modified: 2011-04-15 05:05 PDT (History)
10 users (show)

See Also:


Attachments
Patch (45.08 KB, patch)
2011-03-09 16:42 PST, Ami Fischman
no flags Details | Formatted Diff | Diff
Patch (44.98 KB, patch)
2011-03-10 08:22 PST, Ami Fischman
no flags Details | Formatted Diff | Diff
Test rewritten to use shadow DOM explicitly. (152.07 KB, patch)
2011-04-11 10:41 PDT, Ami Fischman
no flags Details | Formatted Diff | Diff
Use shadow DOM and avoid hardcoding controls height. (151.75 KB, patch)
2011-04-11 12:43 PDT, Ami Fischman
no flags Details | Formatted Diff | Diff
Use shadow DOM and avoid hardcoding controls height. (151.84 KB, patch)
2011-04-11 13:33 PDT, Ami Fischman
no flags Details | Formatted Diff | Diff
Now passes on OS/X webkit, too. (154.38 KB, patch)
2011-04-12 15:36 PDT, Ami Fischman
no flags Details | Formatted Diff | Diff
CSS changes removed. (151.84 KB, patch)
2011-04-13 08:17 PDT, Ami Fischman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Mihai Parparita 2011-02-17 11:12:38 PST
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)
Comment 2 imasaki 2011-02-17 11:21:40 PST
We are woking on the fix right now.
Comment 3 Anna Cavender 2011-02-17 11:24:42 PST
Looks like this has been caught and is being handled:
https://bugs.webkit.org/show_bug.cgi?id=54631
Comment 4 Mihai Parparita 2011-03-03 15:59:31 PST
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
Comment 5 imasaki 2011-03-03 18:31:27 PST
It was skipped because of this change
http://trac.webkit.org/changeset/78811/trunk/LayoutTests/platform/chromium/test_expectations.txt
Comment 6 Mihai Parparita 2011-03-03 18:33:51 PST
(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).
Comment 7 Ami Fischman 2011-03-09 16:42:59 PST
Created attachment 85258 [details]
Patch
Comment 8 Mihai Parparita 2011-03-10 07:51:39 PST
> 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.
Comment 9 Ami Fischman 2011-03-10 08:22:19 PST
Created attachment 85334 [details]
Patch
Comment 10 Ami Fischman 2011-03-10 08:22:48 PST
(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.
Comment 11 Mihai Parparita 2011-03-10 10:14:57 PST
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.
Comment 12 WebKit Commit Bot 2011-03-10 23:14:06 PST
Comment on attachment 85334 [details]
Patch

Clearing flags on attachment: 85334

Committed r80820: <http://trac.webkit.org/changeset/80820>
Comment 13 WebKit Commit Bot 2011-03-10 23:14:12 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Ami Fischman 2011-03-11 08:18:55 PST
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?
Comment 15 David Levin 2011-03-11 08:22:54 PST
Reopening per previous comment.
Comment 16 Eric Carlson 2011-03-11 14:22:24 PST
(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.
Comment 17 Eric Carlson 2011-03-11 14:24:19 PST
(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.
Comment 18 Ami Fischman 2011-03-30 10:56:11 PDT
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.
Comment 19 Ami Fischman 2011-04-11 10:41:32 PDT
Created attachment 89028 [details]
Test rewritten to use shadow DOM explicitly.
Comment 20 Ami Fischman 2011-04-11 10:43:11 PDT
*** Bug 54757 has been marked as a duplicate of this bug. ***
Comment 21 Eric Carlson 2011-04-11 11:12:36 PDT
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".
Comment 22 Ami Fischman 2011-04-11 12:43:58 PDT
Created attachment 89057 [details]
Use shadow DOM and avoid hardcoding controls height.
Comment 23 Ami Fischman 2011-04-11 12:45:37 PDT
(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).
Comment 24 Eric Carlson 2011-04-11 13:20:55 PDT
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.
Comment 25 Ami Fischman 2011-04-11 13:33:38 PDT
Created attachment 89069 [details]
Use shadow DOM and avoid hardcoding controls height.
Comment 26 Ami Fischman 2011-04-11 13:34:25 PDT
(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.
Comment 27 Eric Carlson 2011-04-11 13:44:10 PDT
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!
Comment 28 WebKit Commit Bot 2011-04-11 17:03:48 PDT
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
Comment 29 Ami Fischman 2011-04-11 19:06:55 PDT
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?
Comment 30 Eric Seidel (no email) 2011-04-11 19:10:26 PDT
Unfortunately the cq doesn't know how to upload failure results yet. :(  I expect the test simply fails on Mac Snow Leopard.
Comment 31 Ami Fischman 2011-04-12 15:36:58 PDT
Created attachment 89283 [details]
Now passes on OS/X webkit, too.
Comment 32 Ami Fischman 2011-04-12 15:41:19 PDT
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.
Comment 33 Jer Noble 2011-04-12 16:22:05 PDT
(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.
Comment 34 Ami Fischman 2011-04-13 08:14:10 PDT
(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.
Comment 35 Ami Fischman 2011-04-13 08:17:03 PDT
Created attachment 89379 [details]
CSS changes removed.
Comment 36 Eric Carlson 2011-04-13 08:41:38 PDT
Cleared cq+ at Ami's request, this won't work until https://bugs.webkit.org/show_bug.cgi?id=58442 has landed.
Comment 37 Ami Fischman 2011-04-14 20:52:21 PDT
(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.
Comment 38 WebKit Commit Bot 2011-04-14 22:22:46 PDT
Comment on attachment 89379 [details]
CSS changes removed.

Clearing flags on attachment: 89379

Committed r83947: <http://trac.webkit.org/changeset/83947>
Comment 39 WebKit Commit Bot 2011-04-14 22:22:55 PDT
All reviewed patches have been landed.  Closing bug.