Bug 82476

Created attachment 136017 [details] Fixed display bug and removed timer

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.

Created attachment 136291 [details] Simple patch

Created attachment 136293 [details] Updated test expected output

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.

Created attachment 136458 [details] Fixed ChangeLog entries

(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 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?

Created attachment 136489 [details] Updated test.

(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 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.

(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.

Victor, please mark this cq? if it is ready to land.

(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.

Created attachment 138540 [details] Rebased

Comment on attachment 138540 [details] Rebased Clearing flags on attachment: 138540 Committed r115125: <http://trac.webkit.org/changeset/115125>

All reviewed patches have been landed. Closing bug.