Upstream WebMediaPlayerAndroid as WebKit::WebMediaPlayer implementation on android.

webkit/media/android/webmediaplayer_android.cc

Index: webkit/media/android/webmediaplayer_android.cc
diff --git a/webkit/media/android/webmediaplayer_android.cc b/webkit/media/android/webmediaplayer_android.cc
new file mode 100644
index 0000000000000000000000000000000000000000..30a1b2c8ea2ea71094a479d05332531a8b6b8809
--- /dev/null
+++ b/webkit/media/android/webmediaplayer_android.cc
@@ -0,0 +1,520 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "webkit/media/android/webmediaplayer_android.h"
+
+#include <string>
+
+#include "base/bind.h"
+#include "base/command_line.h"
+#include "base/file_path.h"
+#include "base/logging.h"
+#include "base/utf_string_conversions.h"
+#include "media/base/android/media_player_bridge.h"
+#include "net/base/mime_util.h"
+#include "third_party/WebKit/Source/WebKit/chromium/public/WebFrame.h"
+#include "third_party/WebKit/Source/WebKit/chromium/public/WebMediaPlayerClient.h"
+#include "third_party/WebKit/Source/WebKit/chromium/public/WebView.h"
+#include "third_party/WebKit/Source/WebKit/chromium/public/platform/WebCookieJar.h"
+#include "third_party/WebKit/Source/WebKit/chromium/public/platform/WebRect.h"
+#include "webkit/media/android/webmediaplayer_proxy_android.h"
+#include "webkit/media/webvideoframe_impl.h"
+
+using WebKit::WebCanvas;
+using WebKit::WebFrame;
+using WebKit::WebMediaPlayerClient;
+using WebKit::WebMediaPlayer;
+using WebKit::WebRect;
+using WebKit::WebSize;
+using WebKit::WebTimeRanges;
+using WebKit::WebURL;
+using WebKit::WebVideoFrame;
+using media::MediaPlayerBridge;
+using media::VideoFrame;
+using webkit_media::WebVideoFrameImpl;
+
+namespace {
+
+// Platform independent method for converting and rounding floating point
+// seconds to an int64 timestamp.
+//
+// Refer to https://bugs.webkit.org/show_bug.cgi?id=52697 for details.
+base::TimeDelta ConvertSecondsToTimestamp(float seconds) {

[scherkus (not reviewing), 2012/04/17 03:57:31]

this code is duplicated

[qinmin, 2012/04/18 20:06:30]

move the function to webmediaplayer_util.h, and also changed
webmediaplayer_impl.cc

On 2012/04/17 03:57:31, scherkus wrote:

+  float microseconds = seconds * base::Time::kMicrosecondsPerSecond;
+  float integer = ceilf(microseconds);
+  float difference = integer - microseconds;
+
+  // Round down if difference is large enough.
+  if ((microseconds > 0 && difference > 0.5f) ||
+      (microseconds <= 0 && difference >= 0.5f)) {
+    integer -= 1.0f;
+  }
+
+  // Now we can safely cast to int64 microseconds.
+  return base::TimeDelta::FromMicroseconds(static_cast<int64>(integer));
+}
+
+}  // namespace
+
+namespace webkit_media {
+
+bool WebMediaPlayerAndroid::incognito_mode_ = false;
+
+WebMediaPlayerAndroid::WebMediaPlayerAndroid(
+    WebFrame* frame,
+    WebMediaPlayerClient* client,
+    WebKit::WebCookieJar* cookie_jar)
+    : frame_(frame),

[scherkus (not reviewing), 2012/04/17 03:57:31]

WMPA doesn't use this

[qinmin, 2012/04/18 20:06:30]

This will be used by the fullscreen view code to get the webview. Removed for
now, I will add it back later when upstreaming the fullscreen view code
On 2012/04/17 03:57:31, scherkus wrote:

+      client_(client),
+      render_message_loop_(base::MessageLoopProxy::current()),

[scherkus (not reviewing), 2012/04/17 03:57:31]

WMPA doesn't use this

[qinmin, 2012/04/18 20:06:30]

Done.

+      media_player_(NULL),

[scherkus (not reviewing), 2012/04/17 03:57:31]

for scoped_ptr/scoped_refptr we typically leave these out

[qinmin, 2012/04/18 20:06:30]

Done.

+      natural_size_(WebSize(0,0)),

[scherkus (not reviewing), 2012/04/17 03:57:31]

this is handles by default ctor -- no need to have

[qinmin, 2012/04/18 20:06:30]

Done.

+      proxy_(new WebMediaPlayerProxyAndroid(render_message_loop_)),
+      prepared_(false),
+      duration_(0),
+      time_to_seek_(0),
+      seeking_(false),
+      current_time_(0),
+      buffered_bytes_(0),
+      cookie_jar_(cookie_jar),
+      pending_play_event_(false),
+      skip_loading_(false),
+      network_state_(WebMediaPlayer::Empty),
+      ready_state_(WebMediaPlayer::HaveNothing) {
+  video_frame_.reset(new WebVideoFrameImpl(VideoFrame::CreateEmptyFrame()));
+}
+
+WebMediaPlayerAndroid::~WebMediaPlayerAndroid() {
+  proxy_->SetWebMediaPlayer(NULL);
+
+  if (media_player_.get()) {
+    media_player_->Stop();
+    media_player_.reset(NULL);

[scherkus (not reviewing), 2012/04/17 03:57:31]

do you need this line? the dtor should call this automatically

[qinmin, 2012/04/18 20:06:30]

Done.

+  }
+
+  video_frame_.reset(NULL);

[scherkus (not reviewing), 2012/04/17 03:57:31]

ditto

[qinmin, 2012/04/18 20:06:30]

Done.

+}
+
+void WebMediaPlayerAndroid::InitIncognito(bool incognito_mode) {
+  incognito_mode_ = incognito_mode;

[scherkus (not reviewing), 2012/04/17 03:57:31]

when is this called? is it guaranteed this will get called before we start
loading?

also, can we query for incognito mode? why does it need to be a class static?

[qinmin, 2012/04/18 20:06:30]

This will be called by chrome/browser/chrome_content_browser_client.cc. This
method is called when render process is created, so it should always get called
before we start loading.

Incognito is browser side setting, so renderer side should not know about this.
And also webkit/media should not have dependencies on chrome dir. And the
incoginto setting applies to all WMPAs created in that process, it makes sense
to make it static.


On 2012/04/17 03:57:31, scherkus wrote:

[scherkus (not reviewing), 2012/04/21 03:02:05]

What you describe are all implementation details of Chrome and not WebKit. It's
possible that some other consumer of Chromium WebKit may build a browser that
will have some media players hide their URL history while other media players do
not. The concept of separate processes isn't a concept that belongs in WebKit
code. 

I also noticed the MediaPlayerBridge code refers to this parameter as
hide_url_log and not incognito.

Is it possible to move this argument to the constructor + rename it?

+}
+
+void WebMediaPlayerAndroid::load(const WebURL& url) {
+  url_ = url;

[scherkus (not reviewing), 2012/04/17 03:57:31]

most of your url_ usage involves immediately converting it to a GURL -- why not
store a GURL instead?

[qinmin, 2012/04/18 20:06:30]

Done.

+  GURL gurl(url_);
+
+  // Calling LoadUrl will cause android mediaplayer to start buffering and
+  // decoding the data. On mobile devices, this costs a lot of data usage
+  // and could even introduce performance issues. So we don't load the url
+  // unless it is a local file. We will start loading the media only when
+  // play/seek/fullsceen button is clicked.

[scherkus (not reviewing), 2012/04/17 03:57:31]

as a web page author can't I work around this by having javascript code that
calls play() or sets currentTime, in which case what's the point?

this code would be vastly simplified if we didn't have to do such a thing...

[qinmin, 2012/04/18 20:06:30]

No, calling play() will not work as there are gesture requirement in
HTMLMediaElemet.
The major concern here is both performance and data. And also to avoid bugs in
video decoders. For example, for youtube webm videos, calling
mediaplayer->prepare will trigger a download of the whole file before start
parsing the metadata on ICS. This greatly slows down page scrolling and adds
tens of megabytes of data usage.   
For files, we want the same behavior as desktop so that DRT tests can pass.

Ultimately, we should have a separate thread on render process that calls
android MediaMetadataRetriever::extractMetadata() to retrieve the metadata here.
 But that is still in the planning.

On 2012/04/17 03:57:31, scherkus wrote:

[scherkus (not reviewing), 2012/04/21 03:02:05]

Thanks for the explanation!

On 2012/04/18 20:06:30, qinmin wrote:

+  if (gurl.SchemeIs("file"))
+    LoadUrl(url_);
+  else
+    skip_loading_ = true;
+
+  UpdateNetworkState(WebMediaPlayer::Loading);
+  UpdateReadyState(WebMediaPlayer::HaveNothing);
+
+  if (skip_loading_) {

[scherkus (not reviewing), 2012/04/17 03:57:31]

you can replace skip_loading_ with a check for media_player_.get() or even
rewrite this code to handle the file:// case first and return early

[qinmin, 2012/04/18 20:06:30]

rewrite the handle to file:// case
On 2012/04/17 03:57:31, scherkus wrote:

+    // FIXME(qinmin): we need a method to calculate the duration of the media.

[scherkus (not reviewing), 2012/04/17 03:57:31]

s/FIXME/TODO

[qinmin, 2012/04/18 20:06:30]

Done.

+    // Android does not provide any function to do that.
+    // Set the initial duration value to 100 seconds so that user can touch
+    // the seek bar to perform seek. We will scale the seek position later
+    // when we got the actual duration.
+    duration_ = 100;

[scherkus (not reviewing), 2012/04/17 03:57:31]

you shouldn't need a duration_ parameter at all

in fact all of this is way too hacky

if you want to disable seeking temporarily you should be able to implement the
following for maxTimeSeekable():

if (!prepared_)
  return 0;
return duration();

[qinmin, 2012/04/18 20:06:30]

We need the duration_ for many reasons:
1. MediaPlayer will be released when switching tabs, so when switching back, if
the user want to perform a seek, having a duration_ at hand helps as we don't
need to rescale the seek position.
2. If user hit the fullscreen button, we need some duration value (in the case
mediaplayer is not prepared) to send to the fullscreen view so that the media
controller will show up correctly.
3. we want to let users able to seek.

Presetting the duration_ to 100 is not very desirable. We should be able to get
around it when we have an additional thread on the renderer process to call
MediaMetadataRetriever::extractMetadata().
 
An alternative here is that I remove the preset value here, so duration_ will be
initialized to 0. In that case, the first time user sees the media, he cannot
seek it but he can play it(my guess, I haven't tested this though).  After he
starts playing it we should be able to get the duration_ . And he can perform
seek on the media the next time he visit the page.

On 2012/04/17 03:57:31, scherkus wrote:

+
+    // Pretend everything has been loaded so that webkit can
+    // still call play() and seek().
+    UpdateReadyState(WebMediaPlayer::HaveMetadata);
+    UpdateReadyState(WebMediaPlayer::HaveEnoughData);
+  }
+}
+
+void WebMediaPlayerAndroid::cancelLoad() {
+  NOTIMPLEMENTED();
+}
+
+void WebMediaPlayerAndroid::play() {
+  if (media_player_.get()) {
+    if (!prepared_)
+      pending_play_event_ = true;
+    else
+      PlayInternal();
+  } else {
+    pending_play_event_ = true;
+    LoadUrl(url_);
+  }
+}
+
+void WebMediaPlayerAndroid::pause() {
+  if (media_player_.get()) {
+    if (!prepared_)
+      pending_play_event_ = false;
+    else
+      PauseInternal();
+  } else {
+    // We don't need to load media if pause is called().

[scherkus (not reviewing), 2012/04/17 03:57:31]

pause() instead of called()?

[qinmin, 2012/04/18 20:06:30]

Done.

+    pending_play_event_ = false;
+  }
+}
+
+void WebMediaPlayerAndroid::seek(float seconds) {
+  // Record the time to seek when OnMediaPrepared() is called.
+  time_to_seek_ = seconds;

[scherkus (not reviewing), 2012/04/17 03:57:31]

perhaps a better variable name is "pending_seek_" to line up with the other
pending_xxx_ variables

[qinmin, 2012/04/18 20:06:30]

Done.

+  if (media_player_.get()) {
+    if (prepared_)
+      SeekInternal(seconds);
+  } else
+    LoadUrl(url_);

[scherkus (not reviewing), 2012/04/17 03:57:31]

curly braces please

[qinmin, 2012/04/18 20:06:30]

Done.

+}
+
+bool WebMediaPlayerAndroid::supportsFullscreen() const {
+  return true;
+}
+
+bool WebMediaPlayerAndroid::supportsSave() const {
+  return false;
+}
+
+void WebMediaPlayerAndroid::setEndTime(float seconds) {
+  // Deprecated.

[scherkus (not reviewing), 2012/04/17 03:57:31]

for these and others: if they're deprecated are there bugs filed? should we
remove these?

[qinmin, 2012/04/18 20:06:30]

No, there are no bugs filed. Added a todo for this. These functions are no
longer being called.
On 2012/04/17 03:57:31, scherkus wrote:

+}
+
+void WebMediaPlayerAndroid::setRate(float rate) {
+  NOTIMPLEMENTED();
+}
+
+void WebMediaPlayerAndroid::setVolume(float volume) {
+  media_player_->SetVolume(volume, volume);
+}
+
+void WebMediaPlayerAndroid::setVisible(bool visible) {
+  // Deprecated.
+}
+
+bool WebMediaPlayerAndroid::totalBytesKnown() {
+  NOTIMPLEMENTED();
+  return false;
+}
+
+bool WebMediaPlayerAndroid::hasVideo() const {
+  // FIXME(qinmin): need a better method to determine whether the current media
+  // content contains video. Android does not provide any function to do
+  // this.
+  // We don't know whether the current media content has video unless
+  // the player is prepared. If the player is not prepared, we fall back
+  // to the mime-type. There may be no mime-type on a redirect URL.
+  // In that case, we conservatively assume it contains video so that
+  // enterfullscreen call will not fail.
+  if (!prepared_) {
+    GURL gurl(url_);
+    if(!gurl.has_path())

[scherkus (not reviewing), 2012/04/17 03:57:31]

space between if and (

[qinmin, 2012/04/18 20:06:30]

Done.

+      return false;
+    std::string mime;
+    if(!net::GetMimeTypeFromFile(FilePath(gurl.path()), &mime))
+      return true;
+    return mime.find("audio") == std::string::npos;

[scherkus (not reviewing), 2012/04/17 03:57:31]

should this be "audio/"?

[qinmin, 2012/04/18 20:06:30]

Done, but should have the same effect for all current media mime type. 

On 2012/04/17 03:57:31, scherkus wrote:

+  }
+
+  return natural_size_.width != 0 && natural_size_.height != 0;

[scherkus (not reviewing), 2012/04/17 03:57:31]

how about !natural_size_.isEmpty() instead?

[qinmin, 2012/04/18 20:06:30]

Done.

+}
+
+bool WebMediaPlayerAndroid::hasAudio() const {
+  // TODO(hclam): Query status of audio and return the actual value.
+  return true;
+}
+
+bool WebMediaPlayerAndroid::paused() const {
+  if (!prepared_)
+    return !pending_play_event_;
+  return !media_player_->IsPlaying();
+}
+
+bool WebMediaPlayerAndroid::seeking() const {
+  return seeking_;
+}
+
+float WebMediaPlayerAndroid::duration() const {
+  return duration_;
+}
+
+float WebMediaPlayerAndroid::currentTime() const {
+  // If the player is pending for a seek, return the seek time.
+  if (!prepared_ || seeking())
+    return time_to_seek_;
+
+  // When playback is about to finish, android media player often stops
+  // at a time which is smaller than the duration. This makes webkit never
+  // know that the playback has finished. To solve this, we set the
+  // current_time_ to media duration when PLAYBACK_COMPLETE message is

[scherkus (not reviewing), 2012/04/17 03:57:31]

by "PLAYBACK_COMPLETE message" do you mean OnPlaybackComplete()? I'd update docs

[qinmin, 2012/04/18 20:06:30]

Done.

+  // received. And return the greater of the two values so that the current
+  // time is most updated.
+  return std::max(current_time_,

[scherkus (not reviewing), 2012/04/17 03:57:31]

current_time_ is completely misleading as it's either 0 or duration()

why not use a boolean to track when OnPlaybackComplete() has been called and
instead of confusing std::max() + documentation we have a much simpler:

if (playback_completed_)
  return duration();

[qinmin, 2012/04/18 20:06:30]

current_time was used to send the current time information to the fullscreen
view, and we have a timer object that regularly pulls the current time from the
media player. Removed for now as I can add it back when upstreaming the
fullscreen video.
One issue related to playback_completed_ is when we need to reset it to false. I
am putting that into seek() call, hope this does not cause any problems.

On 2012/04/17 03:57:31, scherkus wrote:

+                  (float) media_player_->GetCurrentTime().InSecondsF());

[scherkus (not reviewing), 2012/04/17 03:57:31]

static_cast<>

[qinmin, 2012/04/18 20:06:30]

Done.

+}
+
+int WebMediaPlayerAndroid::dataRate() const {
+  // Deprecated.
+  return 0;
+}
+
+WebSize WebMediaPlayerAndroid::naturalSize() const {
+  return natural_size_;
+}
+
+WebMediaPlayer::NetworkState WebMediaPlayerAndroid::networkState() const {
+  return network_state_;
+}
+
+WebMediaPlayer::ReadyState WebMediaPlayerAndroid::readyState() const {
+  return ready_state_;
+}
+
+const WebTimeRanges& WebMediaPlayerAndroid::buffered() {
+  return time_ranges_;
+}
+
+float WebMediaPlayerAndroid::maxTimeSeekable() const {
+  // TODO(hclam): If this stream is not seekable this should return 0.
+  return duration();
+}
+
+unsigned long long WebMediaPlayerAndroid::bytesLoaded() const {
+  return buffered_bytes_;
+}
+
+unsigned long long WebMediaPlayerAndroid::totalBytes() const {
+  // Deprecated.
+  return 0;
+}
+
+void WebMediaPlayerAndroid::setSize(const WebSize& size) {
+  texture_size_ = size;
+}
+
+void WebMediaPlayerAndroid::paint(WebCanvas* canvas, const WebRect& rect) {
+  NOTIMPLEMENTED();
+}
+
+bool WebMediaPlayerAndroid::hasSingleSecurityOrigin() const {
+  return false;
+}
+
+WebMediaPlayer::MovieLoadType
+    WebMediaPlayerAndroid::movieLoadType() const {
+  // Deprecated.
+  return WebMediaPlayer::Unknown;
+}
+
+float WebMediaPlayerAndroid::mediaTimeForTimeValue(float timeValue) const {
+  return ConvertSecondsToTimestamp(timeValue).InSecondsF();
+}
+
+unsigned WebMediaPlayerAndroid::decodedFrameCount() const {
+  NOTIMPLEMENTED();
+  return 0;
+}
+
+unsigned WebMediaPlayerAndroid::droppedFrameCount() const {
+  NOTIMPLEMENTED();
+  return 0;
+}
+
+unsigned WebMediaPlayerAndroid::audioDecodedByteCount() const {
+  NOTIMPLEMENTED();
+  return 0;
+}
+
+unsigned WebMediaPlayerAndroid::videoDecodedByteCount() const {
+  NOTIMPLEMENTED();
+  return 0;
+}
+
+void WebMediaPlayerAndroid::OnMediaPrepared() {
+  WebKit::WebTimeRanges new_buffered(static_cast<size_t>(1));
+  new_buffered[0].start = 0.0f;

[scherkus (not reviewing), 2012/04/17 03:57:31]

WebTimeRanges's default ctor initializes to 0

[qinmin, 2012/04/18 20:06:30]

Done.

+  new_buffered[0].end = 0.0f;
+  time_ranges_.swap(new_buffered);

[scherkus (not reviewing), 2012/04/17 03:57:31]

why the swap? how often is OnMediaPrepared() called? shouldn't it be
once-per-WMPA-lifetime?

if anything you could do:
time_ranges_.clear();
time_ranges_.push_back(WebTimeRanges());

...or something similar

[qinmin, 2012/04/18 20:06:30]

This could be called many times as MediaPlayer could got destroyed in many
situations (switching tabs, moving app to background, player become idle...)
WebVector does not have clear() and push_back() function, only swap()

On 2012/04/17 03:57:31, scherkus wrote:

+  prepared_ = true;
+
+  // Update the media duration first so that webkit will get the correct
+  // duration when UpdateReadyState is called.
+  float dur = duration_;
+  duration_ = media_player_->GetDuration().InSecondsF();
+
+  GURL gurl(url_);
+  if (gurl.SchemeIs("file"))
+    UpdateNetworkState(WebMediaPlayer::Loaded);
+
+  if (ready_state_ != WebMediaPlayer::HaveEnoughData) {
+    UpdateReadyState(WebMediaPlayer::HaveMetadata);
+    UpdateReadyState(WebMediaPlayer::HaveEnoughData);
+  } else {
+    // If the status is already set to HaveEnoughData, set it again to make sure
+    // that Videolayerchromium will get created.
+    UpdateReadyState(WebMediaPlayer::HaveEnoughData);
+  }
+
+  if (skip_loading_) {
+    // In we have skipped loading, the duration was preset to 100 sec.
+    // We have to update webkit about the new duration.

[scherkus (not reviewing), 2012/04/17 03:57:31]

even in the file:// case isn't the duration also unknown for a temporary amount
of time?

so shouldn't we *always* play it safe and rescale time_to_seek_?

[qinmin, 2012/04/18 20:06:30]

So here is what happens:
WebKit will call duration() when readyState is set to HaveEnoughData.
So for url case, after we set the ready state to HaveEnoughData in the above
code, webkit will immediately get a duration of 100 seconds. This allows the
users to do seek/play immediately. 
However, for the file:// case, because the ready state is not set here, so user
cannot perform any operation on the media controller (the controller is
disabled). Only until onPrepared() is called, webkit will get the duration and
the user will be able to seek/play. And since we will have the duration when
onPrepared() is called, so webkit will know the exact duration of the media.
On 2012/04/17 03:57:31, scherkus wrote:

+    if (duration_ != dur) {
+      // Scale the time_to_seek_ according to the new duration.
+      time_to_seek_ = time_to_seek_ * duration_ / 100.0f;
+      client_->durationChanged();
+    }
+  }
+
+  // If media player was recovered from a saved state, consume all the pending
+  // events.
+  seek(time_to_seek_);
+
+  if (pending_play_event_)
+    PlayInternal();
+
+  pending_play_event_ = false;
+}
+
+void WebMediaPlayerAndroid::OnPlaybackComplete() {
+  // Set the current time equal to duration to let webkit know that play back
+  // is completed.
+  current_time_ = duration();
+  client_->timeChanged();
+}
+
+void WebMediaPlayerAndroid::OnBufferingUpdate(int percentage) {
+  if (time_ranges_.size() > 0) {

[scherkus (not reviewing), 2012/04/17 03:57:31]

would OnBufferingUpdate() ever get called before OnMediaPrepared()?

if so that seems very strange -- maybe this should be a DCHECK() instead?

[qinmin, 2012/04/18 20:06:30]

Yes, that could happen as mediaplayer could get destroyed
and there is a previous time_range_.

On 2012/04/17 03:57:31, scherkus wrote:

[scherkus (not reviewing), 2012/04/21 03:02:05]

But time_ranges_ is never reset back to size 0 and that call ordering only
happens when building consecutive media players.

For the _first_ media player created it shouldn't be possible to receive a
buffering update, right?

Also, see my comments above about side-stepping this issue by initializing
time_ranges_ to size 1 in the first place

On 2012/04/18 20:06:30, qinmin wrote:

+    time_ranges_[0].end = duration() * percentage / 100;
+    // Implement a trick here to fake progress event, as WebKit checks
+    // consecutive bytesLoaded() to see if any progress made.
+    // See HTMLMediaElement::progressEventTimerFired.
+    buffered_bytes_++;

[scherkus (not reviewing), 2012/04/17 03:57:31]

FYI this is a pretty horrible trick!

can you at least add a TODO?

[qinmin, 2012/04/18 20:06:30]

TODO added, android mediaplayer does not provide us with these size related
callbacks, so no easy fix for now.

On 2012/04/17 03:57:31, scherkus wrote:

[scherkus (not reviewing), 2012/04/21 03:02:05]

You should send a patch to WebKit :)

In fact in response to this I chatted w/ some folks and we agree that asking for
bytes to check for progress isn't a good API so you can expect this to change at
some point.

On 2012/04/18 20:06:30, qinmin wrote:

+  }
+}
+
+void WebMediaPlayerAndroid::OnSeekComplete() {
+  seeking_ = false;
+
+  UpdateReadyState(WebMediaPlayer::HaveEnoughData);
+
+  client_->timeChanged();
+}
+
+void WebMediaPlayerAndroid::OnMediaError(int error_type) {
+  switch (error_type) {
+    case MediaPlayerBridge::MEDIA_ERROR_UNKNOWN:
+      // When playing an bogus URL or bad file, we fire an MEDIA_ERROR_UNKNOWN.

[scherkus (not reviewing), 2012/04/17 03:57:31]

s/an/a

s/,//

[qinmin, 2012/04/18 20:06:30]

Done.

+      // As WebKit uses FormatError to indicate an error for bogus URL or bad
+      // file, we default an MEDIA_ERROR_UNKNOWN to FormatError.

[scherkus (not reviewing), 2012/04/17 03:57:31]

s/an/a

s/,//

[qinmin, 2012/04/18 20:06:30]

Done.

+      UpdateNetworkState(WebMediaPlayer::FormatError);
+      break;
+    case MediaPlayerBridge::MEDIA_ERROR_SERVER_DIED:
+      // TODO(zhenghao): Media server died. In this case, the application must
+      // release the MediaPlayer object and instantiate a new one.
+      UpdateNetworkState(WebMediaPlayer::DecodeError);
+      break;
+    case MediaPlayerBridge::MEDIA_ERROR_NOT_VALID_FOR_PROGRESSIVE_PLAYBACK:
+      UpdateNetworkState(WebMediaPlayer::FormatError);
+      break;
+    case MediaPlayerBridge::MEDIA_ERROR_INVALID_CODE:
+      break;
+  }
+  client_->repaint();
+}
+
+void WebMediaPlayerAndroid::OnMediaInfo(int info_type) {
+  NOTIMPLEMENTED();
+}
+
+void WebMediaPlayerAndroid::OnVideoSizeChanged(int width, int height) {
+  natural_size_.width = width;
+  natural_size_.height = height;
+}
+
+void WebMediaPlayerAndroid::UpdateNetworkState(
+    WebMediaPlayer::NetworkState state) {
+  network_state_ = state;
+  client_->networkStateChanged();
+}
+
+void WebMediaPlayerAndroid::UpdateReadyState(
+    WebMediaPlayer::ReadyState state) {
+  ready_state_ = state;
+  client_->readyStateChanged();
+}
+
+void WebMediaPlayerAndroid::SetVideoSurface(

[scherkus (not reviewing), 2012/04/17 03:57:31]

nit: this can fit on single line

[qinmin, 2012/04/18 20:06:30]

Done.

+    jobject j_surface) {
+  if (media_player_.get())
+    media_player_->SetVideoSurface(j_surface);
+}
+
+void WebMediaPlayerAndroid::InitializeMediaPlayer() {
+  if (!media_player_.get()) {

[scherkus (not reviewing), 2012/04/17 03:57:31]

this check isn't needed -- you only call this method once from inside LoadUrl()
-- hence suggesting you combine the methods

if anything, it'd make more sense as a CHECK

[qinmin, 2012/04/18 20:06:30]

Changed it to a CHECK instead.

On 2012/04/17 03:57:31, scherkus wrote:

+    prepared_ = false;
+    media_player_.reset(new MediaPlayerBridge());
+    media_player_->SetStayAwakeWhilePlaying();
+    proxy_->SetWebMediaPlayer(this);
+  }
+}
+
+void WebMediaPlayerAndroid::LoadUrl(WebURL url) {

[scherkus (not reviewing), 2012/04/17 03:57:31]

AFAIK this only gets called once during the lifetime of the class (webkit
recreates the WMPA object if the URL changes)

you should have a CHECK(!media_player_.get()) to enforce it

[qinmin, 2012/04/18 20:06:30]

This could get called many times since we will release the mediaplayer when it
is inactive or when switching tabs.

On 2012/04/17 03:57:31, scherkus wrote:

+  if (!media_player_.get())
+    InitializeMediaPlayer();
+
+  media_player_->Reset();

[scherkus (not reviewing), 2012/04/17 03:57:31]

not needed since LoadUrl() should only get called to create a media_player_

[qinmin, 2012/04/18 20:06:30]

Done.

+
+  std::string cookies;
+  if (cookie_jar_ != NULL)

[scherkus (not reviewing), 2012/04/17 03:57:31]

is it valid to pass in a NULL cookie_jar to this class?

[qinmin, 2012/04/18 20:06:30]

Yes, for DRT case.

On 2012/04/17 03:57:31, scherkus wrote:

[scherkus (not reviewing), 2012/04/21 03:02:05]

There's no DRT-specific cookie jar implementation we can use?

On 2012/04/18 20:06:30, qinmin wrote:

+    cookies = UTF16ToUTF8(cookie_jar_->cookies(url, url));
+  media_player_->SetDataSource(url.spec(), cookies, incognito_mode_);
+
+  media_player_->Prepare(
+      base::Bind(&WebMediaPlayerProxyAndroid::MediaInfoCallback, proxy_),
+      base::Bind(&WebMediaPlayerProxyAndroid::MediaErrorCallback, proxy_),
+      base::Bind(&WebMediaPlayerProxyAndroid::VideoSizeChangedCallback, proxy_),
+      base::Bind(&WebMediaPlayerProxyAndroid::BufferingUpdateCallback, proxy_),
+      base::Bind(&WebMediaPlayerProxyAndroid::MediaPreparedCallback, proxy_));
+

[scherkus (not reviewing), 2012/04/17 03:57:31]

nit: remove blank line

[qinmin, 2012/04/18 20:06:30]

Done.

+}
+
+void WebMediaPlayerAndroid::PlayInternal() {
+  CHECK(prepared_);
+
+  if (paused())
+    media_player_->Start(base::Bind(
+        &WebMediaPlayerProxyAndroid::PlaybackCompleteCallback, proxy_));
+}
+
+void WebMediaPlayerAndroid::PauseInternal() {
+  CHECK(prepared_);
+  media_player_->Pause();
+}
+
+void WebMediaPlayerAndroid::SeekInternal(float seconds) {
+  CHECK(prepared_);
+  if (seconds == 0.0f && currentTime() == 0.0f) {

[scherkus (not reviewing), 2012/04/17 03:57:31]

what's this for?

[qinmin, 2012/04/18 20:06:30]

This is just to save some unnecessary calls to the android media player.
Removed as it only helps the case when the initial seek(0) is called.
On 2012/04/17 03:57:31, scherkus wrote:

+    client_->timeChanged();
+    return;
+  }
+  seeking_ = true;
+  media_player_->SeekTo(ConvertSecondsToTimestamp(seconds), base::Bind(
+      &WebMediaPlayerProxyAndroid::SeekCompleteCallback, proxy_));
+}
+
+// -------------------------------------------------------------------------
+// Methods that are called on the compositor thread if useThreadedCompositor
+// flag  is set.

[scherkus (not reviewing), 2012/04/17 03:57:31]

you've already covered this in the .h -- I'd remove the // ------------ comments

[qinmin, 2012/04/18 20:06:30]

Done.

+WebVideoFrame* WebMediaPlayerAndroid::getCurrentFrame() {
+  return video_frame_.get();
+}
+
+void WebMediaPlayerAndroid::putCurrentFrame(
+    WebVideoFrame* web_video_frame) {
+}
+
+// -------------------------------------------------------------------------
+
+}  // namespace webkit_media