Bug 64880 – Controls not reset when media element url changed

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

Created attachment 101486 [details]
test case

Media element controls are not reset to their default state when the src is changed. This means that when the src is changed from a valid url to an invalid url, the "Loading..." label for the new url is shown along with the transport controls. 

In the attached test case, let the video load and click the "Invalid url"  button.

[Arko Saha]

Created attachment 104506 [details]
Fix for the bug and added a sample test page.

Resetting media controls when the src is changed from a valid url to an invalid url.

[WebKit Review Bot]

Attachment 104506 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/manual-tests/media-controls-invalid-url.html:4:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/manual-tests/media-controls-invalid-url.html:5:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Comment on attachment 104506 [details]
Fix for the bug and added a sample test page.

Thanks for picking this up!


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

> Source/WebCore/ChangeLog:7
> +
> +

Nit: The extra blank line is not necessary.


> Source/WebCore/manual-tests/media-controls-invalid-url.html:6
> +      function setMovie(index)

Evil, evil tabs!


> Source/WebCore/manual-tests/media-controls-invalid-url.html:28
> +  <p>Test for <a href="https://bugs.webkit.org/show_bug.cgi?id=64880">https://bugs.webkit.org/show_bug.cgi?id=64880</a><br>
> +    Media element controls are not reset to their default state when the src is changed.</p>
> +  <video id="vid" height=480 type="video/mp4"
> +         src="../../../LayoutTests/media/content/test.mp4"
> +         controls>
> +  </video>
> +  <br>
> +  <div onclick="setMovie('1')" >Valid url</div>
> +  <div onclick="setMovie('0')" >Invalid url</div>

This should be made into an automated layout test. Manual tests aren't run very often so the things they test for can regress - e.g.. https://bugs.webkit.org/show_bug.cgi?id=66303. 

Making this into an automated test should be easy, I would start out with a valid url and change to an invalid one in a 'canplaythrough' event listener. 

It should be possible to be make the results platform agnostic, despite the fact that ports have different control layout, by checking for the presence of the slider with the shadow DOM API (look for other tests in media/ tests that use media-controls.js).

[Arko Saha]

Created attachment 104836 [details]
Patch

[WebKit Review Bot]

Comment on attachment 104836 [details]
Patch

Attachment 104836 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9478333

New failing tests:
media/media-controls-invalid-url.html

[Arko Saha]

Created attachment 105007 [details]
Patch

[Arko Saha]

Hi Eric,
Thanks for the review+. Can you please help me to commit/land this patch?
Thanks a lot :)

[WebKit Review Bot]

Comment on attachment 105007 [details]
Patch

Clearing flags on attachment: 105007

Committed r94418: <http://trac.webkit.org/changeset/94418>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

This caused a test to fail on Mac:
http://build.webkit.org/old-results/SnowLeopard%20Intel%20Release%20(Tests)/r94433%20(32798)/media/media-controls-invalid-url-pretty-diff.html

[Eric Carlson]

(In reply to comment #10)
> This caused a test to fail on Mac:
> http://build.webkit.org/old-results/SnowLeopard%20Intel%20Release%20(Tests)/r94433%20(32798)/media/media-controls-invalid-url-pretty-diff.html

That test was added with this patch.