Eliminate MediaPlayer abstraction(play/pause, other APIs)

Eliminate MediaPlayer abstraction(play/pause, other APIs)

BUG=350571
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177703

[Srirama, 2014-07-07 13:34:03]

As the BUG=382721 seems to be solved, landing these changes.
Removed play, pause, paused, supportsSave, hasSingleSecurityOrigin and
mediaTimeForTimeValue from WebMediaPlayerClientImpl.
PTAL.

[acolwell GONE FROM CHROMIUM, 2014-07-08 00:26:33]

https://codereview.chromium.org/370203004/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/370203004/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:3263: if (webMediaPlayer() &&
!webMediaPlayer()->paused())
It seems like there are too many webMediaPlayer() checks in here. How about
assigning this condition to a isPlaying local variable and use it here and
below.

https://codereview.chromium.org/370203004/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:3273: bool playerPaused = webMediaPlayer()
&& webMediaPlayer()->paused();
remove this in favor of isPlaying mentioned above.

https://codereview.chromium.org/370203004/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:3282: if (playerPaused) {
!isPlaying.  You shouldn't have to worry about webMediaPlayer() equalling NULL
here because shouldBePlaying should be allowed to be true if we don't have a
webMediaPlayer().

https://codereview.chromium.org/370203004/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:3296: if (!playerPaused &&
webMediaPlayer())
I think this could become "if (isPlaying)".

https://codereview.chromium.org/370203004/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/370203004/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.h:253: bool hasSingleSecurityOrigin() const {
return webMediaPlayer() && webMediaPlayer()->hasSingleSecurityOrigin(); }
Isn't this a behavior change? Shouldn't this be !m_player || (webMediaPlayer()
&& webMediaPlayer()->hasSingleSecurityOrigin()) ?

[Srirama, 2014-07-08 05:06:52]

Fixed review comments.
PTAL

https://codereview.chromium.org/370203004/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/370203004/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:3273: bool playerPaused = webMediaPlayer()
&& webMediaPlayer()->paused();
On 2014/07/08 00:26:33, acolwell wrote:
> remove this in favor of isPlaying mentioned above.

Done.

https://codereview.chromium.org/370203004/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:3282: if (playerPaused) {
On 2014/07/08 00:26:33, acolwell wrote:
> !isPlaying.  You shouldn't have to worry about webMediaPlayer() equalling NULL
> here because shouldBePlaying should be allowed to be true if we don't have a
> webMediaPlayer().

Done.

https://codereview.chromium.org/370203004/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:3296: if (!playerPaused &&
webMediaPlayer())
On 2014/07/08 00:26:33, acolwell wrote:
> I think this could become "if (isPlaying)".

Done.

https://codereview.chromium.org/370203004/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/370203004/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.h:253: bool hasSingleSecurityOrigin() const {
return webMediaPlayer() && webMediaPlayer()->hasSingleSecurityOrigin(); }
On 2014/07/08 00:26:33, acolwell wrote:
> Isn't this a behavior change? Shouldn't this be !m_player || (webMediaPlayer()
> && webMediaPlayer()->hasSingleSecurityOrigin()) ?

Done.

[acolwell GONE FROM CHROMIUM, 2014-07-08 15:01:15]

lgtm

https://codereview.chromium.org/370203004/diff/40001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/370203004/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:3275: WTF_LOG(Media,
"HTMLMediaElement::updatePlayState - shouldBePlaying = %s, playerPaused = %s",
nit: These are just reflecting local variables so ISTM that you should
s/playerPaused/isPlaying/ and remove the ! below.

[Srirama, 2014-07-08 16:47:27]

abarth@: owners review for web/ and platform/ please.

https://codereview.chromium.org/370203004/diff/40001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/370203004/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:3275: WTF_LOG(Media,
"HTMLMediaElement::updatePlayState - shouldBePlaying = %s, playerPaused = %s",
On 2014/07/08 15:01:14, acolwell wrote:
> nit: These are just reflecting local variables so ISTM that you should
> s/playerPaused/isPlaying/ and remove the ! below.

Done.

[abarth-chromium, 2014-07-08 19:34:11]

LGTM

[Srirama, 2014-07-09 01:23:45]

The CQ bit was checked by srirama.m@samsung.com

[commit-bot: I haz the power, 2014-07-09 01:24:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/srirama.m@samsung.com/370203004/60001

[commit-bot: I haz the power, 2014-07-09 01:27:00]

Change committed as 177703