Eliminate MediaPlayer & MediaPlayerClient abstractions(seek, duration, poster APIs)

Eliminate MediaPlayer & MediaPlayerClient abstractions(seek, duration, poster APIs)

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

[Srirama, 2014-05-30 07:12:10]

Removed duration, currentTime, seek, seeking and setPoster APIs from
WebMediaPlayerClientImpl.
PTAL.

[acolwell GONE FROM CHROMIUM, 2014-06-02 22:34:17]

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

https://codereview.chromium.org/306123003/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:1844: if (webMediaPlayer())
You should not need this check. You shouldn't be able to get to this point w/o
having a webMediaPlayer(). The only way to get m_readyState > HAVE_NOTHING is
for webMediaPlayer() to actually exist and triggered a transition to something
else.

https://codereview.chromium.org/306123003/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:1885: m_cachedTime = webMediaPlayer() ?
webMediaPlayer()->currentTime() : 0.0;
You shouldn't need this check. This method shouldn't be called if we don't have
a webMediaPlayer().

https://codereview.chromium.org/306123003/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:1919: double playerCurrentTime =
webMediaPlayer() ? webMediaPlayer()->currentTime() : 0.0;
This check should be moved to the top with the !m_player check since it will
eventually replace it.

https://codereview.chromium.org/306123003/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:1959: return webMediaPlayer() ?
webMediaPlayer()->duration() : 0.0;
This check should not be needed. webMediaPlayer() must be valid for m_readyState
>= HAVE_METADATA.

https://codereview.chromium.org/306123003/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:2776: && !(webMediaPlayer() &&
webMediaPlayer()->seeking()))
the pointer check should not be necessary since webMediaPlayer() must be valid
if m_readyState >= HAVE_CURRENT_DATA. Also this method is actually being called
from the webMediaPlayer() instance so it better be non-null at this point.

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

PTAL.

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

https://codereview.chromium.org/306123003/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:1844: if (webMediaPlayer())
On 2014/06/02 22:34:17, acolwell wrote:
> You should not need this check. You shouldn't be able to get to this point w/o
> having a webMediaPlayer(). The only way to get m_readyState > HAVE_NOTHING is
> for webMediaPlayer() to actually exist and triggered a transition to something
> else.

Done.

https://codereview.chromium.org/306123003/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:1885: m_cachedTime = webMediaPlayer() ?
webMediaPlayer()->currentTime() : 0.0;
On 2014/06/02 22:34:17, acolwell wrote:
> You shouldn't need this check. This method shouldn't be called if we don't
have
> a webMediaPlayer().

It is null for preload:none case, when it is called from updatePlayState. So
should we have this check here, or should i add it in updatePlayState ?

https://codereview.chromium.org/306123003/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:1919: double playerCurrentTime =
webMediaPlayer() ? webMediaPlayer()->currentTime() : 0.0;
On 2014/06/02 22:34:17, acolwell wrote:
> This check should be moved to the top with the !m_player check since it will
> eventually replace it.

Done.

https://codereview.chromium.org/306123003/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:1959: return webMediaPlayer() ?
webMediaPlayer()->duration() : 0.0;
On 2014/06/02 22:34:17, acolwell wrote:
> This check should not be needed. webMediaPlayer() must be valid for
m_readyState
> >= HAVE_METADATA.

Done.

https://codereview.chromium.org/306123003/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:2776: && !(webMediaPlayer() &&
webMediaPlayer()->seeking()))
On 2014/06/02 22:34:17, acolwell wrote:
> the pointer check should not be necessary since webMediaPlayer() must be valid
> if m_readyState >= HAVE_CURRENT_DATA. Also this method is actually being
called
> from the webMediaPlayer() instance so it better be non-null at this point.

Done.

[acolwell GONE FROM CHROMIUM, 2014-06-03 17:02:37]

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

https://codereview.chromium.org/306123003/diff/20001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1884: m_cachedTime = webMediaPlayer() ?
webMediaPlayer()->currentTime() : 0.0;
Just return early w/o modifying m_cachedTime or m_cachedTimeWallClockUpdateTime
if webMediaPlayer() is null.  The webMediaPlayer() guard in currentTime()
prevents m_cachedTime from being used w/o a webMediaPlayer() now.

[acolwell GONE FROM CHROMIUM, 2014-06-03 17:02:55]

lgtm % comment

[Srirama, 2014-06-04 06:01:12]

adamk@: Owners review for web/ and platform/ changes please.

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

https://codereview.chromium.org/306123003/diff/20001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1884: m_cachedTime = webMediaPlayer() ?
webMediaPlayer()->currentTime() : 0.0;
On 2014/06/03 17:02:38, acolwell wrote:
> Just return early w/o modifying m_cachedTime or
m_cachedTimeWallClockUpdateTime
> if webMediaPlayer() is null.  The webMediaPlayer() guard in currentTime()
> prevents m_cachedTime from being used w/o a webMediaPlayer() now.

Done.

[adamk, 2014-06-04 16:57:41]

lgtm for web and platform

[Srirama, 2014-06-04 17:44:35]

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

[commit-bot: I haz the power, 2014-06-04 17:45:24]

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

[commit-bot: I haz the power, 2014-06-04 19:45:21]

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

[commit-bot: I haz the power, 2014-06-04 19:45:22]

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 117.
  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
a4d7ccdcac9f7590937e4603dd97d7403416489c..96f27bdf5273d52d9358e4e0a902f9d5b584af48
100644
--- a/Source/platform/graphics/media/MediaPlayer.h
+++ b/Source/platform/graphics/media/MediaPlayer.h
@@ -117,21 +117,11 @@ public:
 
     virtual bool supportsSave() const = 0;
 
-    virtual double duration() const = 0;
-
-    virtual double currentTime() const = 0;
-
-    virtual void seek(double) = 0;
-
-    virtual bool seeking() const = 0;
-
     virtual double rate() const = 0;
     virtual void setRate(double) = 0;
 
     virtual bool paused() const = 0;
 
-    virtual void setPoster(const KURL&) = 0;
-
     enum NetworkState { Empty, Idle, Loading, Loaded, FormatError,
NetworkError, DecodeError };
     virtual NetworkState networkState() const = 0;

[Srirama, 2014-06-05 06:23:32]

Rebased to latest code as the commit failed.

[Srirama, 2014-06-05 06:23:43]

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

[commit-bot: I haz the power, 2014-06-05 06:24:05]

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

[commit-bot: I haz the power, 2014-06-05 07:30:34]

Change committed as 175546