[webkit-reviews] review denied: [Bug 45188] Setting innerHTML to a video element does not respect autoplay : [Attachment 66987] Patch

Tue Sep 14 21:50:15 PDT 2010

Eric Carlson <eric.carlson at apple.com> has denied Victoria <vrk at google.com>'s
request for review:
Bug 45188: Setting innerHTML to a video element does not respect autoplay
https://bugs.webkit.org/show_bug.cgi?id=45188

Attachment 66987: Patch
https://bugs.webkit.org/attachment.cgi?id=66987&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
> +	   * media/video-dom-autoplay2-expected.txt: Added.
> +	   * media/video-dom-autoplay2.html: Added.

These check to make sure WebKit behaves the same in two slightly different
situations, and have exactly
the same event handler test so I would prefer if they were combined into a
single test.

> +<div id="container"></div>
> +<script src=media-file.js></script>
> +<script src=video-test.js></script>
> +
> +<script>
> +function isPlaying() {
> +    consoleWrite("EVENT(play)");
> +    findMediaElement();
> +    testExpected("video.paused", false);
> +    endTest();
> +}
> +    var src = findMediaFile("video", "content/test");
> +    var container = document.getElementById("container");

The function should be indented to match the rest of the <scrip> content.

In general I *greatly* pefer fully formed test files, eg. please add an HTML5
doctype, <html>, <body>, etc.

>  void HTMLMediaElement::removedFromDocument()
>  {
> -    if (m_networkState > NETWORK_EMPTY)
> +    if (m_networkState != NETWORK_EMPTY && m_networkState !=
NETWORK_NO_SOURCE)
>	   pause(processingUserGesture());
>      if (m_isFullscreen)
>	   exitFullscreen();

This is incorrect according to the spec, which in 4.8.9.8 says:

  When a media element is removed from a Document, if the media element's 
  networkState attribute has a value other than NETWORK_EMPTYthen the user 
  agent must act as if the pause() method had been invoked.

What actually happens to prevent autoplay from happening? Is the problem that
m_player is NULL so we call scheduleLoad?