Bug 64880 - Controls not reset when media element url changed
Summary: Controls not reset when media element url changed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-20 11:19 PDT by Eric Carlson
Modified: 2011-09-07 13:13 PDT (History)
5 users (show)

See Also:


Attachments
test case (865 bytes, text/html)
2011-07-20 11:19 PDT, Eric Carlson
no flags Details
Fix for the bug and added a sample test page. (2.51 KB, patch)
2011-08-19 06:44 PDT, Arko Saha
no flags Details | Formatted Diff | Diff
Patch (5.95 KB, patch)
2011-08-23 06:35 PDT, Arko Saha
no flags Details | Formatted Diff | Diff
Patch (5.89 KB, patch)
2011-08-24 09:11 PDT, Arko Saha
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2011-07-20 11:19:52 PDT
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.
Comment 1 Arko Saha 2011-08-19 06:44:11 PDT
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.
Comment 2 WebKit Review Bot 2011-08-19 06:46:58 PDT
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.
Comment 3 Eric Carlson 2011-08-19 07:05:18 PDT
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).
Comment 4 Arko Saha 2011-08-23 06:35:30 PDT
Created attachment 104836 [details]
Patch
Comment 5 WebKit Review Bot 2011-08-23 07:02:04 PDT
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
Comment 6 Arko Saha 2011-08-24 09:11:48 PDT
Created attachment 105007 [details]
Patch
Comment 7 Arko Saha 2011-09-02 07:30:18 PDT
Hi Eric,
Thanks for the review+. Can you please help me to commit/land this patch?
Thanks a lot :)
Comment 8 WebKit Review Bot 2011-09-02 08:34:50 PDT
Comment on attachment 105007 [details]
Patch

Clearing flags on attachment: 105007

Committed r94418: <http://trac.webkit.org/changeset/94418>
Comment 9 WebKit Review Bot 2011-09-02 08:34:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Eric Carlson 2011-09-07 13:13:53 PDT
(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.