Bug 86235 - [BlackBerry] Allow the platform media player to determine the media element's paused/playing status
Summary: [BlackBerry] Allow the platform media player to determine the media element's...
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Max Feil
URL:
Keywords:
Depends on: 86310
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-11 10:57 PDT by Max Feil
Modified: 2014-01-28 19:58 PST (History)
9 users (show)

See Also:


Attachments
Patch (7.82 KB, patch)
2012-05-11 11:06 PDT, Max Feil
no flags Details | Formatted Diff | Diff
Patch (10.60 KB, patch)
2012-05-11 12:23 PDT, Max Feil
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (741.11 KB, application/zip)
2012-05-11 13:16 PDT, WebKit Review Bot
no flags Details
Patch (8.35 KB, patch)
2012-05-11 14:02 PDT, Max Feil
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (538.68 KB, application/zip)
2012-05-11 20:02 PDT, WebKit Review Bot
no flags Details
Patch (9.05 KB, patch)
2012-05-12 07:47 PDT, Max Feil
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Max Feil 2012-05-11 10:57:37 PDT
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
Comment 1 Max Feil 2012-05-11 11:06:19 PDT
Created attachment 141451 [details]
Patch
Comment 2 Max Feil 2012-05-11 12:23:48 PDT
Created attachment 141468 [details]
Patch
Comment 3 WebKit Review Bot 2012-05-11 13:16:14 PDT
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
Comment 4 WebKit Review Bot 2012-05-11 13:16:18 PDT
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
Comment 5 Max Feil 2012-05-11 14:02:49 PDT
Created attachment 141492 [details]
Patch
Comment 6 WebKit Review Bot 2012-05-11 20:02:44 PDT
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
Comment 7 WebKit Review Bot 2012-05-11 20:02:49 PDT
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
Comment 8 Max Feil 2012-05-12 07:47:14 PDT
Created attachment 141575 [details]
Patch
Comment 9 George Staikos 2012-05-12 09:39:54 PDT
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();
Comment 10 WebKit Review Bot 2012-05-12 10:09:13 PDT
Comment on attachment 141575 [details]
Patch

Clearing flags on attachment: 141575

Committed r116858: <http://trac.webkit.org/changeset/116858>
Comment 11 WebKit Review Bot 2012-05-12 10:09:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Eric Carlson 2012-05-12 13:51:22 PDT
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).
Comment 13 Antonio Gomes 2012-05-12 18:46:18 PDT
Reopening to get the comments addressed.
Comment 14 Raphael Kubo da Costa (:rakuco) 2012-05-12 20:23:15 PDT
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.
Comment 15 Max Feil 2012-05-14 11:10:45 PDT
(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.
Comment 16 Eric Carlson 2012-07-25 09:33:42 PDT
@Max - have you addressed any of my comments?
Comment 17 Eric Carlson 2012-08-08 10:34:08 PDT
(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?
Comment 18 Max Feil 2012-08-11 10:58:26 PDT
(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.
Comment 19 Max Feil 2012-11-02 12:40:28 PDT
Assign to myself so it shows up in my list.