Bug 86235 – [BlackBerry] Allow the platform media player to determine the media element's paused/playing status

• Component: WebKit BlackBerry
• Status: RESOLVED INVALID

[Max Feil]

The platform media player needs to know when the HTMLMediaElement is 
not paused. This is to address problems when switching source element,
which causes the destruction of the old MediaPlayerPrivate object and
construction of a new one. The new one must resume playing ASAP if the 
old one was playing.

This patch makes use of a "layer violation" that occurs frequently
in MediaPlayerPrivateBlackBerry.cpp, namely direct reference to the
HTMLMediaElement object instead of the WebCore MediaPlayer object. I had
no choice because the paused status of the MediaPlayer object does not 
give the correct information. This layer violation along with the others
will be fixed soon under: https://bugs.webkit.org/show_bug.cgi?id=84291

[Max Feil]

Created attachment 141451 [details]
Patch

[Max Feil]

Created attachment 141468 [details]
Patch

[WebKit Review Bot]

Comment on attachment 141451 [details]
Patch

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

New failing tests:
media/media-continues-playing-after-replace-source.html

[WebKit Review Bot]

Created attachment 141482 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Max Feil]

Created attachment 141492 [details]
Patch

[WebKit Review Bot]

Comment on attachment 141492 [details]
Patch

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

New failing tests:
media/media-continues-playing-after-replace-source.html

[WebKit Review Bot]

Created attachment 141547 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Max Feil]

Created attachment 141575 [details]
Patch

[George Staikos]

Comment on attachment 141575 [details]
Patch

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

Seems fine though could have been slightly simpler.  I'll r+ anyway.  Not a substantial concern.

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:728
> +    if (!element || element->paused())

This could be reduced to:

return !element || element->paused();

[WebKit Review Bot]

Comment on attachment 141575 [details]
Patch

Clearing flags on attachment: 141575

Committed r116858: <http://trac.webkit.org/changeset/116858>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Carlson]

Comment on attachment 141575 [details]
Patch

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

> LayoutTests/media/media-continues-playing-after-replace-source.html:14
> +        function swapAudio() {

A function's opening brace should be on a new line.

> LayoutTests/media/media-continues-playing-after-replace-source.html:25
> +             logResult(false, "Caught 'error' event, audio.error.code = " + this.error.code);
> +             endTest();

This could be simplified to: failTest("Caught 'error' event, audio.error.code = " + this.error.code);

> LayoutTests/media/media-continues-playing-after-replace-source.html:45
> +            if (timeupdateEventCount-skippedCount == 1) {

There should be spaces around the minus sign.

> LayoutTests/media/media-continues-playing-after-replace-source.html:46
> +                // First time update after source replacement should be 0.

I am not sure I understand this comment. Do you mean that currentTime should be 0 when the first timeUpdate event fires? It would be good to clarify the comment when this test is updated.

> LayoutTests/media/media-continues-playing-after-replace-source.html:60
> +            } else if (timeupdateEventCount-skippedCount >= 2) {

Ditto.

> LayoutTests/media/media-continues-playing-after-replace-source.html:74
> +                    // The ports are not consistent in regards to whether
> +                    // the canplaythrough and playing events are triggered
> +                    // a second time, so stop listening for them. This was
> +                    // done to help the cr-linux test pass, and does not
> +                    // necessarily indicate a problem.

The ports should *definitely* be consistent here. Please file a bug so we can figure out what the correct behavior is and fix it.

> LayoutTests/media/media-continues-playing-after-replace-source.html:91
> +            //audioElement.removeChild(audioElement.childNodes[0]);

The commented out code should be removed.

> LayoutTests/media/media-continues-playing-after-replace-source.html:103
> +        document.write("<audio controls></audio>");

Is creating the audio element with document.write necessary for the test?

> LayoutTests/media/media-continues-playing-after-replace-source.html:104
> +        testAudioElement(0);

It doesn't look like the index passed to testAudioElement is necessary any more (eg. swapAudio is hard coded to use 0).

[Antonio Gomes]

Reopening to get the comments addressed.

[Raphael Kubo da Costa (:rakuco)]

Commit 116858 has caused the EFL and GTK+ bots to fail on the tests that has been added. I have filed bug 86283 for that.

[Max Feil]

(In reply to comment #14)
> Commit 116858 has caused the EFL and GTK+ bots to fail on the tests that has been added. I have filed bug 86283 for that.

The bug filed was actually bug 86310.

[Eric Carlson]

@Max - have you addressed any of my comments?

[Eric Carlson]

(In reply to comment #16)
> @Max - have you addressed any of my comments?

Hello, is anyone home? 

Can you please say whether or not you have addressed the comments I made three months ago? If you have not, when will you?

[Max Feil]

(In reply to comment #17)
> (In reply to comment #16)
> > @Max - have you addressed any of my comments?
> 
> Hello, is anyone home? 
> 
> Can you please say whether or not you have addressed the comments I made three months ago? If you have not, when will you?

Sorry, I have not addressed your comments yet. I have been busy with higher priority work items.

[Max Feil]

Assign to myself so it shows up in my list.