Bug 82476 - Extra display logic for the media control panel element
: Extra display logic for the media control panel element
Status: RESOLVED FIXED
: WebKit
Media Elements
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
: 82150
: 82124
  Show dependency treegraph
 
Reported: 2012-03-28 10:05 PST by
Modified: 2012-04-24 15:50 PST (History)


Attachments
Fixed display bug and removed timer (13.94 KB, text/plain)
2012-04-06 07:02 PST, Victor Carbune
no flags Details
Simple patch (10.77 KB, patch)
2012-04-09 13:22 PST, Victor Carbune
no flags Review Patch | Details | Formatted Diff | Diff
Updated test expected output (10.75 KB, patch)
2012-04-09 13:26 PST, Victor Carbune
no flags Review Patch | Details | Formatted Diff | Diff
Fixed ChangeLog entries (10.91 KB, patch)
2012-04-10 07:43 PST, Victor Carbune
no flags Review Patch | Details | Formatted Diff | Diff
Updated test. (11.06 KB, patch)
2012-04-10 10:54 PST, Victor Carbune
no flags Review Patch | Details | Formatted Diff | Diff
Rebased (10.99 KB, patch)
2012-04-24 05:24 PST, Victor Carbune
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-03-28 10:05:10 PST
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 From 2012-04-06 07:02:33 PST -------
Created an attachment (id=136017) [details]
Fixed display bug and removed timer
------- Comment #2 From 2012-04-06 08:59:16 PST -------
(From update of attachment 136017 [details])
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 From 2012-04-09 13:22:06 PST -------
Created an attachment (id=136291) [details]
Simple patch
------- Comment #4 From 2012-04-09 13:26:15 PST -------
Created an attachment (id=136293) [details]
Updated test expected output
------- Comment #5 From 2012-04-09 19:00:07 PST -------
(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.

> 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 From 2012-04-10 07:43:43 PST -------
Created an attachment (id=136458) [details]
Fixed ChangeLog entries
------- Comment #7 From 2012-04-10 07:46:02 PST -------
(In reply to comment #5)
> (From update of attachment 136293 [details] [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 From 2012-04-10 10:08:18 PST -------
(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?
------- Comment #9 From 2012-04-10 10:54:25 PST -------
Created an attachment (id=136489) [details]
Updated test.
------- Comment #10 From 2012-04-10 11:06:57 PST -------
(In reply to comment #8)
> (From update of attachment 136458 [details] [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 From 2012-04-10 11:15:30 PST -------
(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.
------- Comment #12 From 2012-04-10 14:52:03 PST -------
(In reply to comment #11)
> (From update of attachment 136489 [details] [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 From 2012-04-19 10:10:00 PST -------
Victor, please mark this cq? if it is ready to land.
------- Comment #14 From 2012-04-19 14:11:14 PST -------
(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 From 2012-04-24 05:24:08 PST -------
Created an attachment (id=138540) [details]
Rebased
------- Comment #16 From 2012-04-24 15:50:25 PST -------
(From update of attachment 138540 [details])
Clearing flags on attachment: 138540

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