Eliminate MediaPlayer & MediaPlayerClient abstractions(rate, setRate and other APIs)

Eliminate MediaPlayer & MediaPlayerClient abstractions(rate, setRate and other APIs)

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

[Srirama, 2014-05-30 12:33:44]

Removed rate, setRate, maxTimeSeekable, buffered and didLoadingProgress APIs
from WebMediaPlayerClientImpl.

I have moved validation of rate value from HTMLMediaElement to
Webmediaplayer_impl.
The chromium change is 
https://codereview.chromium.org/303403002/

PTAL.

[acolwell GONE FROM CHROMIUM, 2014-06-02 23:01:13]

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

https://codereview.chromium.org/304353005/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:1977: if (m_player && potentiallyPlaying()
&& !m_mediaController && webMediaPlayer())
I believe the webMediaPlayer() check is not necessary because the conditions for
potentiallyPlaying() to be true ensure the pointer is valid.

https://codereview.chromium.org/304353005/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:1984: if (m_player && potentiallyPlaying()
&& webMediaPlayer())
ditto

https://codereview.chromium.org/304353005/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:2885: if (!m_player)
Remove this case since it should now be covered by thwe webMediaPlayer() case
below.

https://codereview.chromium.org/304353005/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:2913: if (m_player && webMediaPlayer()) {
m_player shouldn't be needed now since it is covered by webMediaPlayer()

https://codereview.chromium.org/304353005/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:3030: if (webMediaPlayer())
I believe this is not necessary because I don't think playerPaused can't be true
if webMediaPlayer() is null.

[Srirama, 2014-06-04 06:48:13]

Fixed review comments.
PTAL.

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

https://codereview.chromium.org/304353005/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:1977: if (m_player && potentiallyPlaying()
&& !m_mediaController && webMediaPlayer())
On 2014/06/02 23:01:14, acolwell wrote:
> I believe the webMediaPlayer() check is not necessary because the conditions
for
> potentiallyPlaying() to be true ensure the pointer is valid.

Done.

https://codereview.chromium.org/304353005/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:1984: if (m_player && potentiallyPlaying()
&& webMediaPlayer())
On 2014/06/02 23:01:14, acolwell wrote:
> ditto

Done.

https://codereview.chromium.org/304353005/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:2885: if (!m_player)
On 2014/06/02 23:01:14, acolwell wrote:
> Remove this case since it should now be covered by thwe webMediaPlayer() case
> below.

Done.

https://codereview.chromium.org/304353005/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:2913: if (m_player && webMediaPlayer()) {
On 2014/06/02 23:01:14, acolwell wrote:
> m_player shouldn't be needed now since it is covered by webMediaPlayer()

Done.

https://codereview.chromium.org/304353005/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:3030: if (webMediaPlayer())
On 2014/06/02 23:01:14, acolwell wrote:
> I believe this is not necessary because I don't think playerPaused can't be
true
> if webMediaPlayer() is null.

Done.

[Srirama, 2014-06-05 13:23:12]

Rebased with latest code.
PTAL.

[acolwell GONE FROM CHROMIUM, 2014-06-06 05:35:37]

lgtm

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

https://codereview.chromium.org/304353005/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:3034:
webMediaPlayer()->setRate(m_playbackRate);
Is this call actually still needed? Your WebMediaPlayerClientImpl::load() change
seems like it would remove the need for this based on what the comment says.

[Srirama, 2014-06-06 05:56:33]

Removed the unneeded call to setRate in WebMediaPlayerClientImpl.

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

https://codereview.chromium.org/304353005/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:3034:
webMediaPlayer()->setRate(m_playbackRate);
You are right, it is not needed. Initially i thought we may miss setRate value
if webMediaPlayer is not created by that time, but all setRate calls from
HTMLMediaElement comes after the webMediaPlayer has been created. I will remove
it.

On 2014/06/06 05:35:37, acolwell wrote:
> Is this call actually still needed? Your WebMediaPlayerClientImpl::load()
change
> seems like it would remove the need for this based on what the comment says.

[Srirama, 2014-06-06 06:03:59]

adamk@: owners review for web/ and platform/ changes please

[adamk, 2014-06-06 15:44:55]

lgtm for web/

[Srirama, 2014-06-06 16:52:26]

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

[commit-bot: I haz the power, 2014-06-06 16:52:52]

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

[commit-bot: I haz the power, 2014-06-06 17:57:46]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/7172)

[commit-bot: I haz the power, 2014-06-06 18:00:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-06 18:00:46]

Could not make sense out of svn commit message.

[Srirama, 2014-06-07 01:07:38]

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

[commit-bot: I haz the power, 2014-06-07 01:08:20]

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

[commit-bot: I haz the power, 2014-06-07 01:08:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-07 01:08:29]

Failed to apply patch for Source/platform/graphics/media/MediaPlayer.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/platform/graphics/media/MediaPlayer.h
  Hunk #1 FAILED at 112.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/platform/graphics/media/MediaPlayer.h.rej

Patch:       Source/platform/graphics/media/MediaPlayer.h
Index: Source/platform/graphics/media/MediaPlayer.h
diff --git a/Source/platform/graphics/media/MediaPlayer.h
b/Source/platform/graphics/media/MediaPlayer.h
index
6de447ce9a3cbe098224143fbcf6ad98f1ee6e03..0910af0fcc78189ada4bb08dc26de9ae3705b840
100644
--- a/Source/platform/graphics/media/MediaPlayer.h
+++ b/Source/platform/graphics/media/MediaPlayer.h
@@ -112,20 +112,12 @@ public:
 
     virtual void load(blink::WebMediaPlayer::LoadType, const String& url,
blink::WebMediaPlayer::CORSMode) = 0;
 
-    virtual double rate() const = 0;
-    virtual void setRate(double) = 0;
-
     enum NetworkState { Empty, Idle, Loading, Loaded, FormatError,
NetworkError, DecodeError };
     virtual NetworkState networkState() const = 0;
 
     enum ReadyState  { HaveNothing, HaveMetadata, HaveCurrentData,
HaveFutureData, HaveEnoughData };
     virtual ReadyState readyState() const = 0;
 
-    virtual double maxTimeSeekable() const = 0;
-    virtual PassRefPtr<TimeRanges> buffered() const = 0;
-
-    virtual bool didLoadingProgress() const = 0;
-
     virtual void paint(GraphicsContext*, const IntRect&) = 0;
     virtual bool
copyVideoTextureToPlatformTexture(blink::WebGraphicsContext3D*,
Platform3DObject, GC3Dint, GC3Denum, GC3Denum, bool, bool) = 0;

[Srirama, 2014-06-09 14:15:04]

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

[Srirama, 2014-06-09 14:15:05]

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

[philipj_slow, 2014-06-17 07:16:27]

What happened to this? It look good to land...

[Srirama, 2014-06-17 07:34:20]

On 2014/06/17 07:16:27, philipj wrote:
> What happened to this? It look good to land...

I thought i will wait till Bug #382721 is closed to avoid unexpected crashes.

[Srirama, 2014-06-27 19:13:44]

PTAL

[acolwell GONE FROM CHROMIUM, 2014-06-30 20:04:24]

lgtm

[Srirama, 2014-07-02 08:05:16]

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

[commit-bot: I haz the power, 2014-07-02 08:05:35]

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

[commit-bot: I haz the power, 2014-07-02 12:54:08]

Change committed as 177366