Bug 82476

Summary: Extra display logic for the media control panel element
Product: WebKit Reporter: Victor Carbune <vcarbune>
Component: MediaAssignee: Victor Carbune <vcarbune>
Status: RESOLVED FIXED    
Severity: Normal CC: annacc, eric.carlson, feature-media-reviews, fischman, silviapfeiffer1, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 82150    
Bug Blocks: 82124    
Attachments:
Description Flags
Fixed display bug and removed timer
none
Simple patch
none
Updated test expected output
none
Fixed ChangeLog entries
none
Updated test.
none
Rebased none

Description Victor Carbune 2012-03-28 10:05:10 PDT
In order to have the media control panel element toggle the display:none property when the fadeout
transition ends, we need to ensure that the property isn't removed when the default controls should not be displayed.
Comment 1 Victor Carbune 2012-04-06 07:02:33 PDT
Created attachment 136017 [details]
Fixed display bug and removed timer
Comment 2 Victor Carbune 2012-04-06 08:59:16 PDT
Comment on attachment 136017 [details]
Fixed display bug and removed timer

It seems that this method doesn't actually work, because dispatchEvent is called only when a listener exists. I got tricked by the fact that the test passes (because it has waitForEvent). The only way to do this is using a timer - I will add a simpler patch just adding the extra variable.
Comment 3 Victor Carbune 2012-04-09 13:22:06 PDT
Created attachment 136291 [details]
Simple patch
Comment 4 Victor Carbune 2012-04-09 13:26:15 PDT
Created attachment 136293 [details]
Updated test expected output
Comment 5 Daniel Bates 2012-04-09 19:00:07 PDT
Comment on attachment 136293 [details]
Updated test expected output

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

I'm r-ing this patch because it doesn't have a properly formatted change log entry. In particular, it's missing a "Reviewed by" line in the WebCore/ChangeLog file.

> Source/WebCore/ChangeLog:7
> +        This patch fixes a bug which caused the controls to be displayed
> +        when they should remain hidden. Added an extra variable to the
> +        panel elements which properly keeps the state of the panel (visible or not).
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=82476

This change log entry doesn't conform to the format we use for change log entries. In particular, it's missing a bug title and a "Reviewed by" line. The top portion of a change log entry has the following lines (in order): data/author's full name/author's email address, bug title, bug URL, Reviewed by line, and change description. See <http://www.webkit.org/coding/contributing.html#changelogs> for more details, which references an example entry. Another example of a change log entry that conforms to the aforementioned format is <http://trac.webkit.org/changeset/103692/trunk/Source/WebCore/ChangeLog>.

> LayoutTests/ChangeLog:8
> +        Added test to ensure that controls are not displayed when
> +        the controls attribute is not set.
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=82476
> +
> +        Reviewed by NOBODY (OOPS!).

Similarly, this change log entry doesn't conform to the format we use for a change log entry.
Comment 6 Victor Carbune 2012-04-10 07:43:43 PDT
Created attachment 136458 [details]
Fixed ChangeLog entries
Comment 7 Victor Carbune 2012-04-10 07:46:02 PDT
(In reply to comment #5)
> (From update of attachment 136293 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136293&action=review
> 
> I'm r-ing this patch because it doesn't have a properly formatted change log entry. In particular, it's missing a "Reviewed by" line in the WebCore/ChangeLog file.

I have changed both ChangeLogs, I wasn't aware how strict the rules are. Thank you for pointing this out.

Thanks for taking the time to review this.
Comment 8 Eric Carlson 2012-04-10 10:08:18 PDT
Comment on attachment 136458 [details]
Fixed ChangeLog entries

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

> LayoutTests/media/video-controls-toggling.html:11
> +        var fadeoutTime = 1000;

This is a very long pause for a layout test. The controls fade actually out in 0.3 seconds, can we make this somewhat smaller without making the test flakey? 

Could it work to set the timeout to 0.3 and have continueTest() set another timeout (only one time) if the panel is still visible?
Comment 9 Victor Carbune 2012-04-10 10:54:25 PDT
Created attachment 136489 [details]
Updated test.
Comment 10 Victor Carbune 2012-04-10 11:06:57 PDT
(In reply to comment #8)
> (From update of attachment 136458 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136458&action=review
> 
> > LayoutTests/media/video-controls-toggling.html:11
> > +        var fadeoutTime = 1000;
> 
> This is a very long pause for a layout test. The controls fade actually out in 0.3 seconds, can we make this somewhat smaller without making the test flakey? 
> 
> Could it work to set the timeout to 0.3 and have continueTest() set another timeout (only one time) if the panel is still visible?

Your solution works good on my machine and I hope it won't be flaky (unfortunately I can't think of any other way of testing this, besides using setTimeout)
Comment 11 Eric Carlson 2012-04-10 11:15:30 PDT
Comment on attachment 136489 [details]
Updated test.

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

> LayoutTests/media/video-controls-toggling.html:58
> +            if (panel.style['display'] != 'none') {
> +                setTimeout(continueTest, fadeoutTime);
> +                return;
> +            }

The only problem with this is that it will set the timer again and again until the test is killed, so it might be better to only do this once. On the other hand, the test will timeout eventually so this might be OK. Your call.
Comment 12 Victor Carbune 2012-04-10 14:52:03 PDT
(In reply to comment #11)
> (From update of attachment 136489 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136489&action=review
> 
> > LayoutTests/media/video-controls-toggling.html:58
> > +            if (panel.style['display'] != 'none') {
> > +                setTimeout(continueTest, fadeoutTime);
> > +                return;
> > +            }
> 
> The only problem with this is that it will set the timer again and again until the test is killed, so it might be better to only do this once. On the other hand, the test will timeout eventually so this might be OK. Your call.

I think that rather than dealing with flaky tests because of slightly different observed fadeout times on various platforms, we are better off with a timeout, meaning that the test really is failing.
Comment 13 Eric Carlson 2012-04-19 10:10:00 PDT
Victor, please mark this cq? if it is ready to land.
Comment 14 Victor Carbune 2012-04-19 14:11:14 PDT
(In reply to comment #13)
> Victor, please mark this cq? if it is ready to land.
This patch enables toggling display:none on the controls and depends on the volume slider rendering fix before it can be marked with cq.
Comment 15 Victor Carbune 2012-04-24 05:24:08 PDT
Created attachment 138540 [details]
Rebased
Comment 16 WebKit Review Bot 2012-04-24 15:50:25 PDT
Comment on attachment 138540 [details]
Rebased

Clearing flags on attachment: 138540

Committed r115125: <http://trac.webkit.org/changeset/115125>
Comment 17 WebKit Review Bot 2012-04-24 15:50:45 PDT
All reviewed patches have been landed.  Closing bug.