Move android mediaplayer from render process to browser process.

Move android mediaplayer from render process to browser process.

Due to UID isolation for security reasons, the render process can no longer have permissions to access internet.
Since Android mediaplayer requires internet permission to work, it has to be moved to the browser process to resolve this. 
Here are the changes included in this patch:
1. Make WebMediaPlayerAndroid a common base class for WebMediaPlayerImplAndroid and WebMediaPlayerInProcessAndroid.
WebMediaPlayerImplAndroid places the android mediaplayer in the brower process, this will be used for future chrome on android releases. 
WebMediaPlayerInProcessAndroid still places the android mediaplayer in the render process, this is being used for Layout tests. We will deprecate this later.

2.Added a commandline flag kMediaPlayerInRenderProcess to
allow switching between these 2 modes

3.MediaPlayerBridge now takes over all the logics originally in WebMediaPlayerAndroid. This is to shield WMPA from knowing the internal state of the mediaplayer.

BUG=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157596

[qinmin, 2012-09-04 18:15:25]

PTAL

[Yaron, 2012-09-04 20:23:09]

On 2012/09/04 18:15:25, qinmin wrote:
> PTAL

Great to see this.. thanks for sending it out. Can you identify which parts of
the patch you want each of the reviewers to look at?

[qinmin, 2012-09-04 20:33:05]

I think scherkus can help review all the webkit/ and media/ changes, you can
help review all the content/browser/android/ changes, and jam/brettw can review
the remaining changes under chrome/renderer/ and content/

On 2012/09/04 20:23:09, Yaron wrote:
> On 2012/09/04 18:15:25, qinmin wrote:
> > PTAL
> 
> Great to see this.. thanks for sending it out. Can you identify which parts of
> the patch you want each of the reviewers to look at?

[Yaron, 2012-09-05 07:03:56]

Took a pass at the content/browser/android stuff which seems reasonable assuming
the rest is. Is this sufficient to enable video? Please add the relevant crbugs
to this.

https://chromiumcodereview.appspot.com/10919075/diff/2001/content/browser/and...
File content/browser/android/cookies_retriever_impl.cc (right):

https://chromiumcodereview.appspot.com/10919075/diff/2001/content/browser/and...
content/browser/android/cookies_retriever_impl.cc:56: cookies_ = cookies;
Hrmm. Could you you have multiple calls to ReturnCookies such that one races
with the previous instance and both are accessing |cookies_|? Could you not pass
cookies through the callback instead?

https://chromiumcodereview.appspot.com/10919075/diff/2001/content/browser/and...
File content/browser/android/cookies_retriever_impl.h (right):

https://chromiumcodereview.appspot.com/10919075/diff/2001/content/browser/and...
content/browser/android/cookies_retriever_impl.h:18: // This class implements
net::CookiesRetriever to retrive cookies
media

https://chromiumcodereview.appspot.com/10919075/diff/2001/content/browser/and...
File content/browser/android/surface_texture_peer_browser_impl.cc (right):

https://chromiumcodereview.appspot.com/10919075/diff/2001/content/browser/and...
content/browser/android/surface_texture_peer_browser_impl.cc:81: jobject surface
= env->NewGlobalRef(tmp.obj());
Use ScopedJavaGlobalRef?

https://chromiumcodereview.appspot.com/10919075/diff/2001/content/browser/and...
File content/browser/android/surface_texture_peer_browser_impl.h (right):

https://chromiumcodereview.appspot.com/10919075/diff/2001/content/browser/and...
content/browser/android/surface_texture_peer_browser_impl.h:37: bool
RegisterBrowserProcessSurfaceTexture(JNIEnv* env);
Nit: move to static method of above class

https://chromiumcodereview.appspot.com/10919075/diff/2001/content/browser/bro...
File content/browser/browser_main_runner.cc (right):

https://chromiumcodereview.appspot.com/10919075/diff/2001/content/browser/bro...
content/browser/browser_main_runner.cc:91:
content::SurfaceTexturePeer::InitInstance(
Couldn't this move into ChromeBrowserMainPartsAndroid?

https://chromiumcodereview.appspot.com/10919075/diff/2001/content/public/andr...
File
content/public/android/java/src/org/chromium/content/browser/BrowserProcessSurfaceTexture.java
(right):

https://chromiumcodereview.appspot.com/10919075/diff/2001/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/BrowserProcessSurfaceTexture.java:12:
public class BrowserProcessSurfaceTexture {
@JNINamespace("content"). Also doesn't appear to need to be public.

https://chromiumcodereview.appspot.com/10919075/diff/2001/media/base/android/...
File media/base/android/cookies_retriever.h (right):

https://chromiumcodereview.appspot.com/10919075/diff/2001/media/base/android/...
media/base/android/cookies_retriever.h:28: }  // namespace net
media

[qinmin, 2012-09-05 19:34:42]

https://chromiumcodereview.appspot.com/10919075/diff/2001/content/browser/and...
File content/browser/android/cookies_retriever_impl.cc (right):

https://chromiumcodereview.appspot.com/10919075/diff/2001/content/browser/and...
content/browser/android/cookies_retriever_impl.cc:56: cookies_ = cookies;
That should not happen in the current code logic. But I have changed the code to
pass cookies through callback.

On 2012/09/05 07:03:56, Yaron wrote:
> Hrmm. Could you you have multiple calls to ReturnCookies such that one races
> with the previous instance and both are accessing |cookies_|? Could you not
pass
> cookies through the callback instead?

https://chromiumcodereview.appspot.com/10919075/diff/2001/content/browser/and...
File content/browser/android/cookies_retriever_impl.h (right):

https://chromiumcodereview.appspot.com/10919075/diff/2001/content/browser/and...
content/browser/android/cookies_retriever_impl.h:18: // This class implements
net::CookiesRetriever to retrive cookies
On 2012/09/05 07:03:56, Yaron wrote:
> media

Done.

https://chromiumcodereview.appspot.com/10919075/diff/2001/content/browser/and...
File content/browser/android/surface_texture_peer_browser_impl.cc (right):

https://chromiumcodereview.appspot.com/10919075/diff/2001/content/browser/and...
content/browser/android/surface_texture_peer_browser_impl.cc:81: jobject surface
= env->NewGlobalRef(tmp.obj());
On 2012/09/05 07:03:56, Yaron wrote:
> Use ScopedJavaGlobalRef?

This is problematic as we cannot pass a ScopedJavaGlobalRef to setSurfacePeer
using bind. If we use another callback from SetSurfacePeer() to notify surface
texture has been set and release the stored global ref in this class, then we
face the problem that this function cannot be called simultaneously by more than
2 callers.

https://chromiumcodereview.appspot.com/10919075/diff/2001/content/browser/and...
File content/browser/android/surface_texture_peer_browser_impl.h (right):

https://chromiumcodereview.appspot.com/10919075/diff/2001/content/browser/and...
content/browser/android/surface_texture_peer_browser_impl.h:37: bool
RegisterBrowserProcessSurfaceTexture(JNIEnv* env);
On 2012/09/05 07:03:56, Yaron wrote:
> Nit: move to static method of above class

Done.

https://chromiumcodereview.appspot.com/10919075/diff/2001/content/browser/bro...
File content/browser/browser_main_runner.cc (right):

https://chromiumcodereview.appspot.com/10919075/diff/2001/content/browser/bro...
content/browser/browser_main_runner.cc:91:
content::SurfaceTexturePeer::InitInstance(
SurfaceTexturePeerBrowserImpl is not under content/public/, I am wondering
whether we should do that as SetSurfacePeer() call needs to access
content/browser/

On 2012/09/05 07:03:56, Yaron wrote:
> Couldn't this move into ChromeBrowserMainPartsAndroid?

https://chromiumcodereview.appspot.com/10919075/diff/2001/content/public/andr...
File
content/public/android/java/src/org/chromium/content/browser/BrowserProcessSurfaceTexture.java
(right):

https://chromiumcodereview.appspot.com/10919075/diff/2001/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/BrowserProcessSurfaceTexture.java:12:
public class BrowserProcessSurfaceTexture {
On 2012/09/05 07:03:56, Yaron wrote:
> @JNINamespace("content"). Also doesn't appear to need to be public.

Done.

https://chromiumcodereview.appspot.com/10919075/diff/2001/media/base/android/...
File media/base/android/cookies_retriever.h (right):

https://chromiumcodereview.appspot.com/10919075/diff/2001/media/base/android/...
media/base/android/cookies_retriever.h:28: }  // namespace net
On 2012/09/05 07:03:56, Yaron wrote:
> media

Done.

[scherkus (not reviewing), 2012-09-07 13:17:35]

initial set of comments

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/browser/an...
File content/browser/android/cookies_retriever_impl.cc (right):

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/browser/an...
content/browser/android/cookies_retriever_impl.cc:29: url_ = url;
what happens if I call this multiple times with different URLs?

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/browser/an...
content/browser/android/cookies_retriever_impl.cc:35: this));
instead of storing url_ and get_cookies_cb_ in the class you can bind it to this
closure (see comments below)

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/browser/an...
content/browser/android/cookies_retriever_impl.cc:38: void
CookiesRetrieverImpl::RequestCookies() {
this function should be:
RequestCookies(const std::string& url, const GetCookieCB& get_cookie_cb)

...so that you can bind the params in line 34

Then on line 46 you can bind the get_cookies_cb_ again to the CB passed into
GetCookiesWithOptionsAsync()

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/browser/an...
content/browser/android/cookies_retriever_impl.cc:46: GURL(url_), options,
base::Bind(&CookiesRetrieverImpl::ReturnCookies,
you're accessing url_ on different threads

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/browser/an...
File content/browser/android/cookies_retriever_impl.h (right):

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/browser/an...
content/browser/android/cookies_retriever_impl.h:25: // Need to be called on the
UI thread.
this should note that this is "media::CookiesRetriever implementation."

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/browser/an...
content/browser/android/cookies_retriever_impl.h:26: virtual void
GetCookiesAsync(std::string url,
nit: incorrect indent style -- param should go on next line

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/browser/an...
content/browser/android/cookies_retriever_impl.h:30: void RequestCookies();
move all these to private

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/browser/an...
File content/browser/android/media_player_manager_android.cc (right):

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/browser/an...
content/browser/android/media_player_manager_android.cc:157: void
MediaPlayerManagerAndroid::OnVideoSizeChanged(int player_id,
indent is wrong: drop player_id to next line

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/browser/an...
File content/browser/android/media_player_manager_android.h (right):

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/browser/an...
content/browser/android/media_player_manager_android.h:20: // This class manages
all the android mediaplayer objects. It receives the
nit: replace "mediaplayer" with "media player"

even better if there is a concrete class name you can refer to -- i.e.,
"MediaPlayerBridge objects"

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/browser/an...
content/browser/android/media_player_manager_android.h:21: // control operations
from the the renderer process, and forward
s/the control/control/

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/browser/an...
content/browser/android/media_player_manager_android.h:21: // control operations
from the the renderer process, and forward
s/, and forward/ and forwards/

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/browser/an...
content/browser/android/media_player_manager_android.h:23: // this class first,
before it forwards them to the renderer process.
Are media notification messages callbacks from MediaPlayerBridge objects?

If so, perhaps reword. "sent" is usually used in the context of IPC messages so
this reads a little funny.

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/browser/an...
content/browser/android/media_player_manager_android.h:24: class
MediaPlayerManagerAndroid : public content::RenderViewHostObserver,
use initializer list style

class A
    : public B,
      public C {
};

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/browser/an...
File content/browser/android/surface_texture_peer_browser_impl.h (right):

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/browser/an...
content/browser/android/surface_texture_peer_browser_impl.h:18: 
nit: remove blank line

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/browser/an...
content/browser/android/surface_texture_peer_browser_impl.h:21: // Methods
inherited from SurfaceTexturePeer.
nit: SurfaceTexturePeer implementation.

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/browser/an...
content/browser/android/surface_texture_peer_browser_impl.h:31: // Whether the
mediaplayer is in render process
s/mediaplayer/media player/

add period to end of commends

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/browser/re...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/browser/re...
content/browser/renderer_host/render_view_host_impl.h:675:
MediaPlayerManagerAndroid* media_player_manager_;
who owns this object? do we leak it?

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/common/vie...
File content/common/view_messages.h (right):

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/common/vie...
content/common/view_messages.h:2307: // Mediaplayer related messages
----------------------------------------------
nit: "mediaplayer" is not a word and you should elaborate what these are used
for

for example, mentioning that this is for controlling media element playback in
the browser process

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/common/vie...
content/common/view_messages.h:2349:
IPC_MESSAGE_ROUTED0(ViewHostMsg_CleanUpAllPlayers)
You have DestroyMediaPlayer but CleanUpAllPlayers?

How about DestroyAllMediaPlayers?

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/public/com...
File content/public/common/content_switches.cc (right):

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/public/com...
content/public/common/content_switches.cc:666: // Disable history logging in
mediaplayer
s/in mediaplayer/for media elements./ (don't forget period!)

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/public/com...
content/public/common/content_switches.cc:669: // Running mediaplayer in
renderer process.
reword suggestion: Whether to run the media elements in the renderer process.

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/renderer/m...
File content/renderer/media/webmediaplayer_proxy_impl_android.h (right):

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/renderer/m...
content/renderer/media/webmediaplayer_proxy_impl_android.h:25: public
RenderViewObserver,
initializer list style

class A
    : public B,
      public C {
};

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/renderer/r...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/renderer/r...
content/renderer/render_view_impl.cc:2483: } else {
early return -- no need for else

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/renderer/r...
File content/renderer/render_view_impl.h (right):

https://chromiumcodereview.appspot.com/10919075/diff/10002/content/renderer/r...
content/renderer/render_view_impl.h:1412:
content::WebMediaPlayerProxyImplAndroid* media_player_proxy_;
who owns this object? are we leaking it?

https://chromiumcodereview.appspot.com/10919075/diff/10002/media/base/android...
File media/base/android/cookies_retriever.h (right):

https://chromiumcodereview.appspot.com/10919075/diff/10002/media/base/android...
media/base/android/cookies_retriever.h:14: // Class for asynchronously
retrieving the cookies for a given url.
s/url/URL

https://chromiumcodereview.appspot.com/10919075/diff/10002/media/base/android...
media/base/android/cookies_retriever.h:15: class CookiesRetriever :
naming suggestions: you have a mismatch between the class name and the method
(retrieve vs. get)

how about CookieGetter with a function called GetCookies? (don't worry about
having plural Cookies -- most chrome code using the singular for the class name
but the plural form for the method; see src/net/cookie/)

https://chromiumcodereview.appspot.com/10919075/diff/10002/media/base/android...
media/base/android/cookies_retriever.h:16: public
base::RefCountedThreadSafe<CookiesRetriever>{
why does this need to be refcounted? would weak pointers suffice?

https://chromiumcodereview.appspot.com/10919075/diff/10002/media/base/android...
media/base/android/cookies_retriever.h:20: virtual ~CookiesRetriever() {};
please move dtor impl into separate .cc

https://chromiumcodereview.appspot.com/10919075/diff/10002/media/base/android...
media/base/android/cookies_retriever.h:22: // Method for getting the cookies for
a given url. The callback takes place
s/url/URL

https://chromiumcodereview.appspot.com/10919075/diff/10002/media/base/android...
media/base/android/cookies_retriever.h:23: // on the same thread where this
function gets called.
reword suggestion -- how about:

"The callback is executed on the caller's thread."

https://chromiumcodereview.appspot.com/10919075/diff/10002/media/base/android...
media/base/android/cookies_retriever.h:24: virtual void
GetCookiesAsync(std::string url,
nit: fix indent

https://chromiumcodereview.appspot.com/10919075/diff/10002/media/base/android...
media/base/android/cookies_retriever.h:24: virtual void
GetCookiesAsync(std::string url,
s/Async// -- there's a callback involved, no need to document in function name

[qinmin, 2012-09-07 22:48:26]

renamed cookies_retriever.h and cookies_retriever_impl.h to cookie_getter.h and
cookie_getter_impl.h, same for corresponding the cc files

http://codereview.chromium.org/10919075/diff/10002/content/browser/android/co...
File content/browser/android/cookies_retriever_impl.cc (right):

http://codereview.chromium.org/10919075/diff/10002/content/browser/android/co...
content/browser/android/cookies_retriever_impl.cc:29: url_ = url;
passed url to the RequestCookies closure.

On 2012/09/07 13:17:35, scherkus wrote:
> what happens if I call this multiple times with different URLs?

http://codereview.chromium.org/10919075/diff/10002/content/browser/android/co...
content/browser/android/cookies_retriever_impl.cc:35: this));
On 2012/09/07 13:17:35, scherkus wrote:
> instead of storing url_ and get_cookies_cb_ in the class you can bind it to
this
> closure (see comments below)

Done.

http://codereview.chromium.org/10919075/diff/10002/content/browser/android/co...
content/browser/android/cookies_retriever_impl.cc:38: void
CookiesRetrieverImpl::RequestCookies() {
On 2012/09/07 13:17:35, scherkus wrote:
> this function should be:
> RequestCookies(const std::string& url, const GetCookieCB& get_cookie_cb)
> 
> ...so that you can bind the params in line 34
> 
> Then on line 46 you can bind the get_cookies_cb_ again to the CB passed into
> GetCookiesWithOptionsAsync()

Done. Use partial binding to pass GetCookiesCB

http://codereview.chromium.org/10919075/diff/10002/content/browser/android/co...
content/browser/android/cookies_retriever_impl.cc:46: GURL(url_), options,
base::Bind(&CookiesRetrieverImpl::ReturnCookies,
On 2012/09/07 13:17:35, scherkus wrote:
> you're accessing url_ on different threads

Done. passing url to this function.

http://codereview.chromium.org/10919075/diff/10002/content/browser/android/co...
File content/browser/android/cookies_retriever_impl.h (right):

http://codereview.chromium.org/10919075/diff/10002/content/browser/android/co...
content/browser/android/cookies_retriever_impl.h:25: // Need to be called on the
UI thread.
On 2012/09/07 13:17:35, scherkus wrote:
> this should note that this is "media::CookiesRetriever implementation."

Done.

http://codereview.chromium.org/10919075/diff/10002/content/browser/android/co...
content/browser/android/cookies_retriever_impl.h:26: virtual void
GetCookiesAsync(std::string url,
On 2012/09/07 13:17:35, scherkus wrote:
> nit: incorrect indent style -- param should go on next line

Done.

http://codereview.chromium.org/10919075/diff/10002/content/browser/android/co...
content/browser/android/cookies_retriever_impl.h:30: void RequestCookies();
On 2012/09/07 13:17:35, scherkus wrote:
> move all these to private

Done.

http://codereview.chromium.org/10919075/diff/10002/content/browser/android/me...
File content/browser/android/media_player_manager_android.cc (right):

http://codereview.chromium.org/10919075/diff/10002/content/browser/android/me...
content/browser/android/media_player_manager_android.cc:157: void
MediaPlayerManagerAndroid::OnVideoSizeChanged(int player_id,
On 2012/09/07 13:17:35, scherkus wrote:
> indent is wrong: drop player_id to next line

Done.

http://codereview.chromium.org/10919075/diff/10002/content/browser/android/me...
File content/browser/android/media_player_manager_android.h (right):

http://codereview.chromium.org/10919075/diff/10002/content/browser/android/me...
content/browser/android/media_player_manager_android.h:20: // This class manages
all the android mediaplayer objects. It receives the
On 2012/09/07 13:17:35, scherkus wrote:
> nit: replace "mediaplayer" with "media player"
> 
> even better if there is a concrete class name you can refer to -- i.e.,
> "MediaPlayerBridge objects"

Done.

http://codereview.chromium.org/10919075/diff/10002/content/browser/android/me...
content/browser/android/media_player_manager_android.h:21: // control operations
from the the renderer process, and forward
On 2012/09/07 13:17:35, scherkus wrote:
> s/the control/control/

Done.

http://codereview.chromium.org/10919075/diff/10002/content/browser/android/me...
content/browser/android/media_player_manager_android.h:21: // control operations
from the the renderer process, and forward
On 2012/09/07 13:17:35, scherkus wrote:
> s/, and forward/ and forwards/

Done.

http://codereview.chromium.org/10919075/diff/10002/content/browser/android/me...
content/browser/android/media_player_manager_android.h:23: // this class first,
before it forwards them to the renderer process.
Yes, they are callbacks from MPB. Rephrased the sentence here. 
On 2012/09/07 13:17:35, scherkus wrote:
> Are media notification messages callbacks from MediaPlayerBridge objects?
> 
> If so, perhaps reword. "sent" is usually used in the context of IPC messages
so
> this reads a little funny.

http://codereview.chromium.org/10919075/diff/10002/content/browser/android/me...
content/browser/android/media_player_manager_android.h:24: class
MediaPlayerManagerAndroid : public content::RenderViewHostObserver,
On 2012/09/07 13:17:35, scherkus wrote:
> use initializer list style
> 
> class A
>     : public B,
>       public C {
> };

Done.

http://codereview.chromium.org/10919075/diff/10002/content/browser/android/su...
File content/browser/android/surface_texture_peer_browser_impl.h (right):

http://codereview.chromium.org/10919075/diff/10002/content/browser/android/su...
content/browser/android/surface_texture_peer_browser_impl.h:18: 
On 2012/09/07 13:17:35, scherkus wrote:
> nit: remove blank line

Done.

http://codereview.chromium.org/10919075/diff/10002/content/browser/android/su...
content/browser/android/surface_texture_peer_browser_impl.h:21: // Methods
inherited from SurfaceTexturePeer.
On 2012/09/07 13:17:35, scherkus wrote:
> nit: SurfaceTexturePeer implementation.

Done.

http://codereview.chromium.org/10919075/diff/10002/content/browser/android/su...
content/browser/android/surface_texture_peer_browser_impl.h:31: // Whether the
mediaplayer is in render process
On 2012/09/07 13:17:35, scherkus wrote:
> s/mediaplayer/media player/
> 
> add period to end of commends

Done.

http://codereview.chromium.org/10919075/diff/10002/content/browser/renderer_h...
File content/browser/renderer_host/render_view_host_impl.h (right):

http://codereview.chromium.org/10919075/diff/10002/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_impl.h:675:
MediaPlayerManagerAndroid* media_player_manager_;
This object is inherited from RenderViewHostObserver, similar to
RenderViewObserver, it will get destroyed in the dtor of this class. Added a
comment here to mention this.

On 2012/09/07 13:17:35, scherkus wrote:
> who owns this object? do we leak it?

http://codereview.chromium.org/10919075/diff/10002/content/common/view_messag...
File content/common/view_messages.h (right):

http://codereview.chromium.org/10919075/diff/10002/content/common/view_messag...
content/common/view_messages.h:2307: // Mediaplayer related messages
----------------------------------------------
Done. Split the description into 2, one for controlling the media playback, one
for notifying the render process of status change.

On 2012/09/07 13:17:35, scherkus wrote:
> nit: "mediaplayer" is not a word and you should elaborate what these are used
> for
> 
> for example, mentioning that this is for controlling media element playback in
> the browser process

http://codereview.chromium.org/10919075/diff/10002/content/common/view_messag...
content/common/view_messages.h:2349:
IPC_MESSAGE_ROUTED0(ViewHostMsg_CleanUpAllPlayers)
On 2012/09/07 13:17:35, scherkus wrote:
> You have DestroyMediaPlayer but CleanUpAllPlayers?
> 
> How about DestroyAllMediaPlayers?

Done.

http://codereview.chromium.org/10919075/diff/10002/content/public/common/cont...
File content/public/common/content_switches.cc (right):

http://codereview.chromium.org/10919075/diff/10002/content/public/common/cont...
content/public/common/content_switches.cc:666: // Disable history logging in
mediaplayer
On 2012/09/07 13:17:35, scherkus wrote:
> s/in mediaplayer/for media elements./ (don't forget period!)

Done.

http://codereview.chromium.org/10919075/diff/10002/content/public/common/cont...
content/public/common/content_switches.cc:669: // Running mediaplayer in
renderer process.
On 2012/09/07 13:17:35, scherkus wrote:
> reword suggestion: Whether to run the media elements in the renderer process.

Done.

http://codereview.chromium.org/10919075/diff/10002/content/renderer/media/web...
File content/renderer/media/webmediaplayer_proxy_impl_android.h (right):

http://codereview.chromium.org/10919075/diff/10002/content/renderer/media/web...
content/renderer/media/webmediaplayer_proxy_impl_android.h:25: public
RenderViewObserver,
On 2012/09/07 13:17:35, scherkus wrote:
> initializer list style
> 
> class A
>     : public B,
>       public C {
> };

Done.

http://codereview.chromium.org/10919075/diff/10002/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10919075/diff/10002/content/renderer/render_vi...
content/renderer/render_view_impl.cc:2483: } else {
On 2012/09/07 13:17:35, scherkus wrote:
> early return -- no need for else

Done.

http://codereview.chromium.org/10919075/diff/10002/content/renderer/render_vi...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/10919075/diff/10002/content/renderer/render_vi...
content/renderer/render_view_impl.h:1412:
content::WebMediaPlayerProxyImplAndroid* media_player_proxy_;
This object is inherited from RenderViewObserver. In
RenderViewImpl::~RenderViewImpl(), all the dtors of RenderViewObserver will get
called. Added a comment here to mention this.
So we are fine here.

On 2012/09/07 13:17:35, scherkus wrote:
> who owns this object? are we leaking it?

http://codereview.chromium.org/10919075/diff/10002/media/base/android/cookies...
File media/base/android/cookies_retriever.h (right):

http://codereview.chromium.org/10919075/diff/10002/media/base/android/cookies...
media/base/android/cookies_retriever.h:14: // Class for asynchronously
retrieving the cookies for a given url.
On 2012/09/07 13:17:35, scherkus wrote:
> s/url/URL

Done.

http://codereview.chromium.org/10919075/diff/10002/media/base/android/cookies...
media/base/android/cookies_retriever.h:15: class CookiesRetriever :
On 2012/09/07 13:17:35, scherkus wrote:
> naming suggestions: you have a mismatch between the class name and the method
> (retrieve vs. get)
> 
> how about CookieGetter with a function called GetCookies? (don't worry about
> having plural Cookies -- most chrome code using the singular for the class
name
> but the plural form for the method; see src/net/cookie/)

Done.

http://codereview.chromium.org/10919075/diff/10002/media/base/android/cookies...
media/base/android/cookies_retriever.h:16: public
base::RefCountedThreadSafe<CookiesRetriever>{
This class can run on multiple threads, and it is not getting called by others
(except the GetCookies() call), so refcountedthreadsafe suits better here.

On 2012/09/07 13:17:35, scherkus wrote:
> why does this need to be refcounted? would weak pointers suffice?

http://codereview.chromium.org/10919075/diff/10002/media/base/android/cookies...
media/base/android/cookies_retriever.h:20: virtual ~CookiesRetriever() {};
On 2012/09/07 13:17:35, scherkus wrote:
> please move dtor impl into separate .cc

Done.

http://codereview.chromium.org/10919075/diff/10002/media/base/android/cookies...
media/base/android/cookies_retriever.h:22: // Method for getting the cookies for
a given url. The callback takes place
On 2012/09/07 13:17:35, scherkus wrote:
> s/url/URL

Done.

http://codereview.chromium.org/10919075/diff/10002/media/base/android/cookies...
media/base/android/cookies_retriever.h:23: // on the same thread where this
function gets called.
On 2012/09/07 13:17:35, scherkus wrote:
> reword suggestion -- how about:
> 
> "The callback is executed on the caller's thread."

Done.

http://codereview.chromium.org/10919075/diff/10002/media/base/android/cookies...
media/base/android/cookies_retriever.h:24: virtual void
GetCookiesAsync(std::string url,
On 2012/09/07 13:17:35, scherkus wrote:
> nit: fix indent

Done.

http://codereview.chromium.org/10919075/diff/10002/media/base/android/cookies...
media/base/android/cookies_retriever.h:24: virtual void
GetCookiesAsync(std::string url,
On 2012/09/07 13:17:35, scherkus wrote:
> s/Async// -- there's a callback involved, no need to document in function name

Done.

[scherkus (not reviewing), 2012-09-10 12:19:08]

http://codereview.chromium.org/10919075/diff/141/content/browser/android/cook...
File content/browser/android/cookie_getter_impl.h (right):

http://codereview.chromium.org/10919075/diff/141/content/browser/android/cook...
content/browser/android/cookie_getter_impl.h:20: class CookieGetterImpl : public
media::CookieGetter {
This class should have a WeakPtrFactory object that it uses when posting the
completion callback to the IO thread

One possibility includes binding the original callback to GetCookiesCallback()
w/ a weak pointer then posting that as the callback to execute to
RequestCookies() on the IO thread. Inside ReturnCookies() you would simply post
the callback to the UI thread.

Should CookieGetterImpl get deleted the task will no-op itself on the UI thread
instead of executing and requiring clients worry about use-after-free due to
CookieGetterImpl being alive due to references maintained on the IO thread.

http://codereview.chromium.org/10919075/diff/141/content/browser/android/cook...
content/browser/android/cookie_getter_impl.h:26: // Need to be called on the UI
thread.
s/Need to/Must/

http://codereview.chromium.org/10919075/diff/141/content/browser/android/cook...
content/browser/android/cookie_getter_impl.h:31: // Helper Methods for posting
tasks.
nit: remove comment (not adding much value)

http://codereview.chromium.org/10919075/diff/141/content/browser/android/medi...
File content/browser/android/media_player_manager_android.cc (right):

http://codereview.chromium.org/10919075/diff/141/content/browser/android/medi...
content/browser/android/media_player_manager_android.cc:194:
(*it)->player_id()));
indenting here is wrong -- does this not fit on previous line?

if not, this should either be aligned w/ routing_id() OR routing_id() gets
dropped to this line

http://codereview.chromium.org/10919075/diff/141/content/browser/android/medi...
File content/browser/android/media_player_manager_android.h (right):

http://codereview.chromium.org/10919075/diff/141/content/browser/android/medi...
content/browser/android/media_player_manager_android.h:26: public
content::RenderViewHostObserver,
colon (:) goes on next line

http://codereview.chromium.org/10919075/diff/141/content/browser/android/surf...
File content/browser/android/surface_texture_peer_browser_impl.cc (right):

http://codereview.chromium.org/10919075/diff/141/content/browser/android/surf...
content/browser/android/surface_texture_peer_browser_impl.cc:13: 
remove blank line

http://codereview.chromium.org/10919075/diff/141/content/browser/android/surf...
content/browser/android/surface_texture_peer_browser_impl.cc:18: void
SetSurfacePeer(jobject j_surface,
static function?

docs?

http://codereview.chromium.org/10919075/diff/141/content/browser/android/surf...
content/browser/android/surface_texture_peer_browser_impl.cc:84:
base::Bind(&SetSurfacePeer, surface,
indent is wrong -- move base::Bind( to previous line then have the args indented
by 4

http://codereview.chromium.org/10919075/diff/141/content/renderer/media/webme...
File content/renderer/media/webmediaplayer_proxy_impl_android.h (right):

http://codereview.chromium.org/10919075/diff/141/content/renderer/media/webme...
content/renderer/media/webmediaplayer_proxy_impl_android.h:25: public
RenderViewObserver,
: goes on this line -- see my style example in PS#3

http://codereview.chromium.org/10919075/diff/141/media/base/android/cookie_ge...
File media/base/android/cookie_getter.cc (right):

http://codereview.chromium.org/10919075/diff/141/media/base/android/cookie_ge...
media/base/android/cookie_getter.cc:9: CookieGetter::~CookieGetter() {
nit: {} on one line

http://codereview.chromium.org/10919075/diff/141/media/base/android/cookie_ge...
File media/base/android/cookie_getter.h (right):

http://codereview.chromium.org/10919075/diff/141/media/base/android/cookie_ge...
media/base/android/cookie_getter.h:6: #define
MEDIA_BASE_ANDROID_COOKIES_GETTER_H_
s/COOKIES/COOKIE (remove extra "S")

http://codereview.chromium.org/10919075/diff/141/media/base/android/cookie_ge...
media/base/android/cookie_getter.h:16: public
base::RefCountedThreadSafe<CookieGetter>{
please don't proliferate reference counted interfaces when it isn't needed

see my comments in CookieGetterImpl on using weak pointer

http://codereview.chromium.org/10919075/diff/141/media/base/android/cookie_ge...
media/base/android/cookie_getter.h:29: #endif  //
MEDIA_BASE_ANDROID_COOKIES_GETTER_H_
ditto

http://codereview.chromium.org/10919075/diff/141/media/base/android/media_pla...
File media/base/android/media_player_bridge_manager.h (right):

http://codereview.chromium.org/10919075/diff/141/media/base/android/media_pla...
media/base/android/media_player_bridge_manager.h:17: virtual
~MediaPlayerBridgeManager() {};
can you add a .cc w/ impl there?

http://codereview.chromium.org/10919075/diff/141/media/base/android/media_pla...
media/base/android/media_player_bridge_manager.h:19: virtual void
RequestMediaResources(MediaPlayerBridge* player) = 0;
docs?

http://codereview.chromium.org/10919075/diff/141/media/base/android/media_pla...
File media/base/android/media_player_listener.cc (right):

http://codereview.chromium.org/10919075/diff/141/media/base/android/media_pla...
media/base/android/media_player_listener.cc:32:
MediaPlayerListener::~MediaPlayerListener() {
{}

http://codereview.chromium.org/10919075/diff/141/media/base/android/media_pla...
media/base/android/media_player_listener.cc:44: 
remove extra blank line

http://codereview.chromium.org/10919075/diff/141/media/base/android/media_pla...
File media/base/android/media_player_listener.h (right):

http://codereview.chromium.org/10919075/diff/141/media/base/android/media_pla...
media/base/android/media_player_listener.h:25: : public
base::RefCountedThreadSafe<MediaPlayerListener> {
does this need to be refcounted?

who holds references to this object? it looks like you pass this object as
reinterpret_cast<intptr_t>(this) to java w/o modifying the refcount -- is that
safe?

http://codereview.chromium.org/10919075/diff/141/media/base/android/media_pla...
media/base/android/media_player_listener.h:55: base::WeakPtr<MediaPlayerBridge>
mediaplayer_;
s/mediaplayer_/media_player_/

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/media_p...
File webkit/media/android/media_player_bridge_manager_impl.cc (right):

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/media_p...
webkit/media/android/media_player_bridge_manager_impl.cc:21: if (player == NULL)
s/player == NULL/!player/

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/media_p...
File webkit/media/android/media_player_bridge_manager_impl.h (right):

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/media_p...
webkit/media/android/media_player_bridge_manager_impl.h:20: explicit
MediaPlayerBridgeManagerImpl(int threshold);
doc ctor please

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/media_p...
webkit/media/android/media_player_bridge_manager_impl.h:31: // threashold before
we start to reclaim unused media resources.
s/threashold/Threshold

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/webmedi...
File webkit/media/android/webmediaplayer_android.h (right):

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/webmedi...
webkit/media/android/webmediaplayer_android.h:28: public WebKit::WebMediaPlayer,
fix style -- colon should go on this line

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/webmedi...
webkit/media/android/webmediaplayer_android.h:108: virtual void
OnTimeUpdate(base::TimeDelta current_time) {};
virtual methods shouldn't be inlined -- can you de-inline?

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/webmedi...
webkit/media/android/webmediaplayer_android.h:119: virtual void
SetVideoSurface(jobject j_surface) {}
ditto

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/webmedi...
webkit/media/android/webmediaplayer_android.h:161: virtual void Destroy() {};
ditto

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/webmedi...
webkit/media/android/webmediaplayer_android.h:163: virtual
WebKit::WebMediaPlayerClient* client() { return client_; }
ditto

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/webmedi...
File webkit/media/android/webmediaplayer_impl_android.cc (right):

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/webmedi...
webkit/media/android/webmediaplayer_impl_android.cc:33:
WebMediaPlayerImplAndroid::~WebMediaPlayerImplAndroid() {
{}

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/webmedi...
File webkit/media/android/webmediaplayer_in_process_android.cc (right):

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/webmedi...
webkit/media/android/webmediaplayer_in_process_android.cc:32:
InProcessCookieGetter::~InProcessCookieGetter() {
{}

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/webmedi...
File webkit/media/android/webmediaplayer_in_process_android.h (right):

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/webmedi...
webkit/media/android/webmediaplayer_in_process_android.h:39: explicit
InProcessCookieGetter(WebKit::WebFrame* frame,
remove explicit

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/webmedi...
webkit/media/android/webmediaplayer_in_process_android.h:45: const
media::CookieGetter::GetCookieCB& callback) OVERRIDE;
fix indent here -- perhaps url param should go on this line

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/webmedi...
File webkit/media/android/webmediaplayer_proxy_android.h (right):

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/webmedi...
webkit/media/android/webmediaplayer_proxy_android.h:18: virtual
~WebMediaPlayerProxyAndroid() {};
move to .cc

[qinmin, 2012-09-11 04:50:23]

http://codereview.chromium.org/10919075/diff/141/content/browser/android/cook...
File content/browser/android/cookie_getter_impl.h (right):

http://codereview.chromium.org/10919075/diff/141/content/browser/android/cook...
content/browser/android/cookie_getter_impl.h:20: class CookieGetterImpl : public
media::CookieGetter {
Done. The callback passed from MediaPlayerBridge to this includes a weakptr, so
we are safe here. But you are right that we should not require the clients to
worry about use-after-free case. Change the code to pass weakptr between
threads.


On 2012/09/10 12:19:08, scherkus wrote:
> This class should have a WeakPtrFactory object that it uses when posting the
> completion callback to the IO thread
> 
> One possibility includes binding the original callback to GetCookiesCallback()
> w/ a weak pointer then posting that as the callback to execute to
> RequestCookies() on the IO thread. Inside ReturnCookies() you would simply
post
> the callback to the UI thread.
> 
> Should CookieGetterImpl get deleted the task will no-op itself on the UI
thread
> instead of executing and requiring clients worry about use-after-free due to
> CookieGetterImpl being alive due to references maintained on the IO thread.

http://codereview.chromium.org/10919075/diff/141/content/browser/android/cook...
content/browser/android/cookie_getter_impl.h:26: // Need to be called on the UI
thread.
On 2012/09/10 12:19:08, scherkus wrote:
> s/Need to/Must/

Done.

http://codereview.chromium.org/10919075/diff/141/content/browser/android/cook...
content/browser/android/cookie_getter_impl.h:31: // Helper Methods for posting
tasks.
On 2012/09/10 12:19:08, scherkus wrote:
> nit: remove comment (not adding much value)

Done.

http://codereview.chromium.org/10919075/diff/141/content/browser/android/medi...
File content/browser/android/media_player_manager_android.cc (right):

http://codereview.chromium.org/10919075/diff/141/content/browser/android/medi...
content/browser/android/media_player_manager_android.cc:194:
(*it)->player_id()));
Done, move it to the previous line.

On 2012/09/10 12:19:08, scherkus wrote:
> indenting here is wrong -- does this not fit on previous line?
> 
> if not, this should either be aligned w/ routing_id() OR routing_id() gets
> dropped to this line

http://codereview.chromium.org/10919075/diff/141/content/browser/android/medi...
File content/browser/android/media_player_manager_android.h (right):

http://codereview.chromium.org/10919075/diff/141/content/browser/android/medi...
content/browser/android/media_player_manager_android.h:26: public
content::RenderViewHostObserver,
On 2012/09/10 12:19:08, scherkus wrote:
> colon (:) goes on next line

Done.

http://codereview.chromium.org/10919075/diff/141/content/browser/android/surf...
File content/browser/android/surface_texture_peer_browser_impl.cc (right):

http://codereview.chromium.org/10919075/diff/141/content/browser/android/surf...
content/browser/android/surface_texture_peer_browser_impl.cc:13: 
On 2012/09/10 12:19:08, scherkus wrote:
> remove blank line

Done.

http://codereview.chromium.org/10919075/diff/141/content/browser/android/surf...
content/browser/android/surface_texture_peer_browser_impl.cc:18: void
SetSurfacePeer(jobject j_surface,
On 2012/09/10 12:19:08, scherkus wrote:
> static function?
> 
> docs?

Done. Also changed the args name to make it more understandable

http://codereview.chromium.org/10919075/diff/141/content/browser/android/surf...
content/browser/android/surface_texture_peer_browser_impl.cc:84:
base::Bind(&SetSurfacePeer, surface,
On 2012/09/10 12:19:08, scherkus wrote:
> indent is wrong -- move base::Bind( to previous line then have the args
indented
> by 4

Done.

http://codereview.chromium.org/10919075/diff/141/content/renderer/media/webme...
File content/renderer/media/webmediaplayer_proxy_impl_android.h (right):

http://codereview.chromium.org/10919075/diff/141/content/renderer/media/webme...
content/renderer/media/webmediaplayer_proxy_impl_android.h:25: public
RenderViewObserver,
On 2012/09/10 12:19:08, scherkus wrote:
> : goes on this line -- see my style example in PS#3

Done.

http://codereview.chromium.org/10919075/diff/141/media/base/android/cookie_ge...
File media/base/android/cookie_getter.cc (right):

http://codereview.chromium.org/10919075/diff/141/media/base/android/cookie_ge...
media/base/android/cookie_getter.cc:9: CookieGetter::~CookieGetter() {
On 2012/09/10 12:19:08, scherkus wrote:
> nit: {} on one line

Done.

http://codereview.chromium.org/10919075/diff/141/media/base/android/cookie_ge...
File media/base/android/cookie_getter.h (right):

http://codereview.chromium.org/10919075/diff/141/media/base/android/cookie_ge...
media/base/android/cookie_getter.h:6: #define
MEDIA_BASE_ANDROID_COOKIES_GETTER_H_
On 2012/09/10 12:19:08, scherkus wrote:
> s/COOKIES/COOKIE (remove extra "S")

Done.

http://codereview.chromium.org/10919075/diff/141/media/base/android/cookie_ge...
media/base/android/cookie_getter.h:16: public
base::RefCountedThreadSafe<CookieGetter>{
On 2012/09/10 12:19:08, scherkus wrote:
> please don't proliferate reference counted interfaces when it isn't needed
> 
> see my comments in CookieGetterImpl on using weak pointer

Done.

http://codereview.chromium.org/10919075/diff/141/media/base/android/cookie_ge...
media/base/android/cookie_getter.h:29: #endif  //
MEDIA_BASE_ANDROID_COOKIES_GETTER_H_
On 2012/09/10 12:19:08, scherkus wrote:
> ditto

Done.

http://codereview.chromium.org/10919075/diff/141/media/base/android/media_pla...
File media/base/android/media_player_bridge_manager.h (right):

http://codereview.chromium.org/10919075/diff/141/media/base/android/media_pla...
media/base/android/media_player_bridge_manager.h:17: virtual
~MediaPlayerBridgeManager() {};
On 2012/09/10 12:19:08, scherkus wrote:
> can you add a .cc w/ impl there?

Done.

http://codereview.chromium.org/10919075/diff/141/media/base/android/media_pla...
media/base/android/media_player_bridge_manager.h:19: virtual void
RequestMediaResources(MediaPlayerBridge* player) = 0;
On 2012/09/10 12:19:08, scherkus wrote:
> docs?

Done.

http://codereview.chromium.org/10919075/diff/141/media/base/android/media_pla...
File media/base/android/media_player_listener.cc (right):

http://codereview.chromium.org/10919075/diff/141/media/base/android/media_pla...
media/base/android/media_player_listener.cc:32:
MediaPlayerListener::~MediaPlayerListener() {
On 2012/09/10 12:19:08, scherkus wrote:
> {}

Done.

http://codereview.chromium.org/10919075/diff/141/media/base/android/media_pla...
media/base/android/media_player_listener.cc:44: 
On 2012/09/10 12:19:08, scherkus wrote:
> remove extra blank line

Done.

http://codereview.chromium.org/10919075/diff/141/media/base/android/media_pla...
File media/base/android/media_player_listener.h (right):

http://codereview.chromium.org/10919075/diff/141/media/base/android/media_pla...
media/base/android/media_player_listener.h:25: : public
base::RefCountedThreadSafe<MediaPlayerListener> {
On 2nd thought, I don't think this class needs to be refcounted. 
MediaPlayerBridge holds a reference to this object. And when MediaplayerBridge
is about to go away, it will release the android mediaplayer. This makes the
java MediaPlayerListener no longer make callbacks to this class. 
If there are any posted tasks, it will be a no-op since media_player_ is a
weakptr. So we are safe here.
Changes the class definition and the scoped_refptr in MPB

On 2012/09/10 12:19:08, scherkus wrote:
> does this need to be refcounted?
> 
> who holds references to this object? it looks like you pass this object as
> reinterpret_cast<intptr_t>(this) to java w/o modifying the refcount -- is that
> safe?

http://codereview.chromium.org/10919075/diff/141/media/base/android/media_pla...
media/base/android/media_player_listener.h:55: base::WeakPtr<MediaPlayerBridge>
mediaplayer_;
On 2012/09/10 12:19:08, scherkus wrote:
> s/mediaplayer_/media_player_/

Done.

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/media_p...
File webkit/media/android/media_player_bridge_manager_impl.cc (right):

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/media_p...
webkit/media/android/media_player_bridge_manager_impl.cc:21: if (player == NULL)
On 2012/09/10 12:19:08, scherkus wrote:
> s/player == NULL/!player/

Done.

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/media_p...
File webkit/media/android/media_player_bridge_manager_impl.h (right):

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/media_p...
webkit/media/android/media_player_bridge_manager_impl.h:20: explicit
MediaPlayerBridgeManagerImpl(int threshold);
On 2012/09/10 12:19:08, scherkus wrote:
> doc ctor please

Done.

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/media_p...
webkit/media/android/media_player_bridge_manager_impl.h:31: // threashold before
we start to reclaim unused media resources.
On 2012/09/10 12:19:08, scherkus wrote:
> s/threashold/Threshold

Done.

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/webmedi...
File webkit/media/android/webmediaplayer_android.h (right):

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/webmedi...
webkit/media/android/webmediaplayer_android.h:28: public WebKit::WebMediaPlayer,
On 2012/09/10 12:19:08, scherkus wrote:
> fix style -- colon should go on this line

Done.

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/webmedi...
webkit/media/android/webmediaplayer_android.h:108: virtual void
OnTimeUpdate(base::TimeDelta current_time) {};
On 2012/09/10 12:19:08, scherkus wrote:
> virtual methods shouldn't be inlined -- can you de-inline?

Done.

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/webmedi...
webkit/media/android/webmediaplayer_android.h:119: virtual void
SetVideoSurface(jobject j_surface) {}
On 2012/09/10 12:19:08, scherkus wrote:
> ditto

Done.

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/webmedi...
webkit/media/android/webmediaplayer_android.h:161: virtual void Destroy() {};
On 2012/09/10 12:19:08, scherkus wrote:
> ditto

Done.

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/webmedi...
webkit/media/android/webmediaplayer_android.h:163: virtual
WebKit::WebMediaPlayerClient* client() { return client_; }
On 2012/09/10 12:19:08, scherkus wrote:
> ditto

Done. This doens't need to be virtual as no child class overrides it

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/webmedi...
File webkit/media/android/webmediaplayer_impl_android.cc (right):

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/webmedi...
webkit/media/android/webmediaplayer_impl_android.cc:33:
WebMediaPlayerImplAndroid::~WebMediaPlayerImplAndroid() {
On 2012/09/10 12:19:08, scherkus wrote:
> {}

Done.

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/webmedi...
File webkit/media/android/webmediaplayer_in_process_android.cc (right):

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/webmedi...
webkit/media/android/webmediaplayer_in_process_android.cc:32:
InProcessCookieGetter::~InProcessCookieGetter() {
On 2012/09/10 12:19:08, scherkus wrote:
> {}

Done.

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/webmedi...
File webkit/media/android/webmediaplayer_in_process_android.h (right):

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/webmedi...
webkit/media/android/webmediaplayer_in_process_android.h:39: explicit
InProcessCookieGetter(WebKit::WebFrame* frame,
On 2012/09/10 12:19:08, scherkus wrote:
> remove explicit

Done.

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/webmedi...
webkit/media/android/webmediaplayer_in_process_android.h:45: const
media::CookieGetter::GetCookieCB& callback) OVERRIDE;
On 2012/09/10 12:19:08, scherkus wrote:
> fix indent here -- perhaps url param should go on this line

Done.

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/webmedi...
File webkit/media/android/webmediaplayer_proxy_android.h (right):

http://codereview.chromium.org/10919075/diff/141/webkit/media/android/webmedi...
webkit/media/android/webmediaplayer_proxy_android.h:18: virtual
~WebMediaPlayerProxyAndroid() {};
On 2012/09/10 12:19:08, scherkus wrote:
> move to .cc

Done.

[nilesh, 2012-09-11 05:48:48]

https://chromiumcodereview.appspot.com/10919075/diff/8053/content/browser/and...
File content/browser/android/cookie_getter_impl.cc (right):

https://chromiumcodereview.appspot.com/10919075/diff/8053/content/browser/and...
content/browser/android/cookie_getter_impl.cc:42:
context_getter_->GetURLRequestContext()->cookie_store();
I think you are supposed to ask the request context' network delegate whether
you can get cookies
(http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/net/chrome_...)
 

Please see
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/browser/android/do...
for similar code to get cookies for a URL. A lot of code is duplicated and it
would be great if we can have a cookie getter helper class  to share the
functionality and make this class independent (not an implementation of
media::CookieGetter).

[scherkus (not reviewing), 2012-09-11 11:56:25]

alllllllllmost there!!!

http://codereview.chromium.org/10919075/diff/8053/content/browser/android/med...
File content/browser/android/media_player_manager_android.h (right):

http://codereview.chromium.org/10919075/diff/8053/content/browser/android/med...
content/browser/android/media_player_manager_android.h:29: // Create a
MediaPlayerManagerAndroid object for the |renver_view_host|.
s/renver/render/

http://codereview.chromium.org/10919075/diff/8053/content/renderer/media/webm...
File content/renderer/media/webmediaplayer_proxy_impl_android.h (right):

http://codereview.chromium.org/10919075/diff/8053/content/renderer/media/webm...
content/renderer/media/webmediaplayer_proxy_impl_android.h:28: // Construct a
WebMediaPlayerProxyImplAndroid object for the |renver_view|.
s/renver/render/

http://codereview.chromium.org/10919075/diff/8053/content/renderer/render_vie...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/10919075/diff/8053/content/renderer/render_vie...
content/renderer/render_view_impl.h:1410: // Proxy class for WebMediaPlayer to
communicate with the real mediaplayer
s/mediaplayer/media player/

http://codereview.chromium.org/10919075/diff/8053/content/renderer/render_vie...
content/renderer/render_view_impl.h:1418: // Resource manager for all the
android mediaplayer objects if they are
s/mediaplayer/media player/

http://codereview.chromium.org/10919075/diff/8053/media/base/android/media_pl...
File media/base/android/media_player_bridge.h (right):

http://codereview.chromium.org/10919075/diff/8053/media/base/android/media_pl...
media/base/android/media_player_bridge.h:35: class MediaPlayerBridge : public
base::SupportsWeakPtr<MediaPlayerBridge> {
can you use WeakPtrFactory weak_this_ instead of inheritance?

http://codereview.chromium.org/10919075/diff/8053/media/base/android/media_pl...
media/base/android/media_player_bridge.h:152: // Create the actual android
mediaplayer.
s/mediaplayer/media player/

http://codereview.chromium.org/10919075/diff/8053/media/base/android/media_pl...
media/base/android/media_player_bridge.h:205: // Resource manager for all the
mediaplayers.
s/mediaplayers/media players/

http://codereview.chromium.org/10919075/diff/8053/media/base/android/media_pl...
media/base/android/media_player_bridge.h:218: scoped_ptr<MediaPlayerListener>
listener_;
nit: this doesn't need to be a scoped_ptr<> anymore -- you can initialize in the
ctor initializer list

http://codereview.chromium.org/10919075/diff/8053/media/base/android/media_pl...
File media/base/android/media_player_listener.cc (right):

http://codereview.chromium.org/10919075/diff/8053/media/base/android/media_pl...
media/base/android/media_player_listener.cc:25: base::WeakPtr<MediaPlayerBridge>
mediaplayer)
s/mediaplayer/media_player/ here and below

http://codereview.chromium.org/10919075/diff/8053/media/base/android/media_pl...
File media/base/android/media_player_listener.h (right):

http://codereview.chromium.org/10919075/diff/8053/media/base/android/media_pl...
media/base/android/media_player_listener.h:27: // side object will be forwarded
to |mediaplayer| by posting a task on the
s/mediaplayer/media_player/ here + below in arg list

http://codereview.chromium.org/10919075/diff/8053/webkit/media/android/media_...
File webkit/media/android/media_player_bridge_manager_impl.h (right):

http://codereview.chromium.org/10919075/diff/8053/webkit/media/android/media_...
webkit/media/android/media_player_bridge_manager_impl.h:33: // Threashold before
we start to reclaim unused media resources.
s/Threashold/Threshold/

http://codereview.chromium.org/10919075/diff/8053/webkit/media/android/webmed...
File webkit/media/android/webmediaplayer_impl_android.h (right):

http://codereview.chromium.org/10919075/diff/8053/webkit/media/android/webmed...
webkit/media/android/webmediaplayer_impl_android.h:38: // |pxoxy|.
s/pxoxy/proxy/

http://codereview.chromium.org/10919075/diff/8053/webkit/media/android/webmed...
webkit/media/android/webmediaplayer_impl_android.h:50: virtual void
SetVideoSurface(jobject j_surface) OVERRIDE {}
de-inline virtual methods

http://codereview.chromium.org/10919075/diff/8053/webkit/media/android/webmed...
File webkit/media/android/webmediaplayer_in_process_android.h (right):

http://codereview.chromium.org/10919075/diff/8053/webkit/media/android/webmed...
webkit/media/android/webmediaplayer_in_process_android.h:39: // Construct a
InProcessCookieGetter object from a WebCookieJar.
s/a/an/

http://codereview.chromium.org/10919075/diff/8053/webkit/media/android/webmed...
webkit/media/android/webmediaplayer_in_process_android.h:86: virtual void
OnTimeUpdate(base::TimeDelta current_time) OVERRIDE {}
de-inline

http://codereview.chromium.org/10919075/diff/8053/webkit/media/android/webmed...
webkit/media/android/webmediaplayer_in_process_android.h:96: virtual void
Destroy() OVERRIDE {}
de-inline

[qinmin, 2012-09-12 23:12:52]

Sorry for taking a little longer to address all the comments. 
Made some modifications to CookieGetterImpl, added the policy check for cookies.
Also fixed a mistake in previous code as WeakPtr cannot run on another thread.
Introduced a refcounted CookieGetterTask to solve this.

http://codereview.chromium.org/10919075/diff/8053/content/browser/android/coo...
File content/browser/android/cookie_getter_impl.cc (right):

http://codereview.chromium.org/10919075/diff/8053/content/browser/android/coo...
content/browser/android/cookie_getter_impl.cc:42:
context_getter_->GetURLRequestContext()->cookie_store();
This is getting a little more complicated. Added a new class CookieGetterTask to
check the policy and getting the cookie on the io thread.
This implementation is a little different from the DownloadManager because we
don't have the UrlRequest object.
However, the code shares a lot of functionalities with render_message_filter.
I will refactor the code later to make CookieGetterTask shared. Currently the
change is already big enough. Don't want to put more burden on the review
process.


On 2012/09/11 05:48:48, nileshagrawal1 wrote:
> I think you are supposed to ask the request context' network delegate whether
> you can get cookies
>
(http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/net/chrome_...)
>  
> 
> Please see
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/browser/android/do...
> for similar code to get cookies for a URL. A lot of code is duplicated and it
> would be great if we can have a cookie getter helper class  to share the
> functionality and make this class independent (not an implementation of
> media::CookieGetter).

http://codereview.chromium.org/10919075/diff/8053/content/browser/android/med...
File content/browser/android/media_player_manager_android.h (right):

http://codereview.chromium.org/10919075/diff/8053/content/browser/android/med...
content/browser/android/media_player_manager_android.h:29: // Create a
MediaPlayerManagerAndroid object for the |renver_view_host|.
On 2012/09/11 11:56:25, scherkus wrote:
> s/renver/render/

Done.

http://codereview.chromium.org/10919075/diff/8053/content/renderer/media/webm...
File content/renderer/media/webmediaplayer_proxy_impl_android.h (right):

http://codereview.chromium.org/10919075/diff/8053/content/renderer/media/webm...
content/renderer/media/webmediaplayer_proxy_impl_android.h:28: // Construct a
WebMediaPlayerProxyImplAndroid object for the |renver_view|.
On 2012/09/11 11:56:25, scherkus wrote:
> s/renver/render/

Done.

http://codereview.chromium.org/10919075/diff/8053/content/renderer/render_vie...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/10919075/diff/8053/content/renderer/render_vie...
content/renderer/render_view_impl.h:1410: // Proxy class for WebMediaPlayer to
communicate with the real mediaplayer
On 2012/09/11 11:56:25, scherkus wrote:
> s/mediaplayer/media player/

Done.

http://codereview.chromium.org/10919075/diff/8053/content/renderer/render_vie...
content/renderer/render_view_impl.h:1418: // Resource manager for all the
android mediaplayer objects if they are
On 2012/09/11 11:56:25, scherkus wrote:
> s/mediaplayer/media player/

Done.

http://codereview.chromium.org/10919075/diff/8053/media/base/android/media_pl...
File media/base/android/media_player_bridge.h (right):

http://codereview.chromium.org/10919075/diff/8053/media/base/android/media_pl...
media/base/android/media_player_bridge.h:35: class MediaPlayerBridge : public
base::SupportsWeakPtr<MediaPlayerBridge> {
On 2012/09/11 11:56:25, scherkus wrote:
> can you use WeakPtrFactory weak_this_ instead of inheritance?

Done.

http://codereview.chromium.org/10919075/diff/8053/media/base/android/media_pl...
media/base/android/media_player_bridge.h:152: // Create the actual android
mediaplayer.
On 2012/09/11 11:56:25, scherkus wrote:
> s/mediaplayer/media player/

Done.

http://codereview.chromium.org/10919075/diff/8053/media/base/android/media_pl...
media/base/android/media_player_bridge.h:205: // Resource manager for all the
mediaplayers.
On 2012/09/11 11:56:25, scherkus wrote:
> s/mediaplayers/media players/

Done.

http://codereview.chromium.org/10919075/diff/8053/media/base/android/media_pl...
media/base/android/media_player_bridge.h:218: scoped_ptr<MediaPlayerListener>
listener_;
The native MediaPlayerListener object's should still owned by this class as the
java object will not delete the native object. The java object will go away when
GC happens while the native object can go away when MPB goes away.
The listener_ is already in the initializer list.

On 2012/09/11 11:56:25, scherkus wrote:
> nit: this doesn't need to be a scoped_ptr<> anymore -- you can initialize in
the
> ctor initializer list

http://codereview.chromium.org/10919075/diff/8053/media/base/android/media_pl...
File media/base/android/media_player_listener.cc (right):

http://codereview.chromium.org/10919075/diff/8053/media/base/android/media_pl...
media/base/android/media_player_listener.cc:25: base::WeakPtr<MediaPlayerBridge>
mediaplayer)
On 2012/09/11 11:56:25, scherkus wrote:
> s/mediaplayer/media_player/ here and below

Done.

http://codereview.chromium.org/10919075/diff/8053/media/base/android/media_pl...
File media/base/android/media_player_listener.h (right):

http://codereview.chromium.org/10919075/diff/8053/media/base/android/media_pl...
media/base/android/media_player_listener.h:27: // side object will be forwarded
to |mediaplayer| by posting a task on the
On 2012/09/11 11:56:25, scherkus wrote:
> s/mediaplayer/media_player/ here + below in arg list

Done.

http://codereview.chromium.org/10919075/diff/8053/webkit/media/android/media_...
File webkit/media/android/media_player_bridge_manager_impl.h (right):

http://codereview.chromium.org/10919075/diff/8053/webkit/media/android/media_...
webkit/media/android/media_player_bridge_manager_impl.h:33: // Threashold before
we start to reclaim unused media resources.
On 2012/09/11 11:56:25, scherkus wrote:
> s/Threashold/Threshold/

Done.

http://codereview.chromium.org/10919075/diff/8053/webkit/media/android/webmed...
File webkit/media/android/webmediaplayer_impl_android.h (right):

http://codereview.chromium.org/10919075/diff/8053/webkit/media/android/webmed...
webkit/media/android/webmediaplayer_impl_android.h:38: // |pxoxy|.
On 2012/09/11 11:56:25, scherkus wrote:
> s/pxoxy/proxy/

Done.

http://codereview.chromium.org/10919075/diff/8053/webkit/media/android/webmed...
webkit/media/android/webmediaplayer_impl_android.h:50: virtual void
SetVideoSurface(jobject j_surface) OVERRIDE {}
On 2012/09/11 11:56:25, scherkus wrote:
> de-inline virtual methods

Done.

http://codereview.chromium.org/10919075/diff/8053/webkit/media/android/webmed...
File webkit/media/android/webmediaplayer_in_process_android.h (right):

http://codereview.chromium.org/10919075/diff/8053/webkit/media/android/webmed...
webkit/media/android/webmediaplayer_in_process_android.h:39: // Construct a
InProcessCookieGetter object from a WebCookieJar.
On 2012/09/11 11:56:25, scherkus wrote:
> s/a/an/

Done.

http://codereview.chromium.org/10919075/diff/8053/webkit/media/android/webmed...
webkit/media/android/webmediaplayer_in_process_android.h:86: virtual void
OnTimeUpdate(base::TimeDelta current_time) OVERRIDE {}
On 2012/09/11 11:56:25, scherkus wrote:
> de-inline

Done.

http://codereview.chromium.org/10919075/diff/8053/webkit/media/android/webmed...
webkit/media/android/webmediaplayer_in_process_android.h:96: virtual void
Destroy() OVERRIDE {}
On 2012/09/11 11:56:25, scherkus wrote:
> de-inline

Done.

[Yaron, 2012-09-13 00:14:48]

content/browser/android lgtm

http://codereview.chromium.org/10919075/diff/15002/content/renderer/media/web...
File content/renderer/media/webmediaplayer_proxy_impl_android.h (right):

http://codereview.chromium.org/10919075/diff/15002/content/renderer/media/web...
content/renderer/media/webmediaplayer_proxy_impl_android.h:22: //
WebMediaPlayerImplAndroid and the MediaplayerManagerAndroid in the browser
Nit: MediaPlayerManagerAndroid

[scherkus (not reviewing), 2012-09-13 10:40:13]

http://codereview.chromium.org/10919075/diff/15002/content/browser/android/co...
File content/browser/android/cookie_getter_impl.cc (right):

http://codereview.chromium.org/10919075/diff/15002/content/browser/android/co...
content/browser/android/cookie_getter_impl.cc:50: finish_event_.Wait();
why the blocking wait on the IO thread?

is there really no way to accomplish the same goal in an async / cb / message
posting fashion?

http://codereview.chromium.org/10919075/diff/15002/content/browser/android/co...
File content/browser/android/cookie_getter_impl.h (right):

http://codereview.chromium.org/10919075/diff/15002/content/browser/android/co...
content/browser/android/cookie_getter_impl.h:29: class CookieGetterTask :
nit: you can fwd declare this + move to .cc as CookieGetterImpl clients
shouldn't have to deal w/ this class

http://codereview.chromium.org/10919075/diff/15002/content/browser/android/co...
content/browser/android/cookie_getter_impl.h:30: public
base::RefCountedThreadSafe<CookieGetterTask> {
: goes on this line

http://codereview.chromium.org/10919075/diff/15002/media/base/android/media_p...
File media/base/android/media_player_bridge.cc (right):

http://codereview.chromium.org/10919075/diff/15002/media/base/android/media_p...
media/base/android/media_player_bridge.cc:175: ConvertUTF8ToJavaString(env,
url_);
why the code change? looks like it should fit on previous line

http://codereview.chromium.org/10919075/diff/15002/media/base/android/media_p...
File media/base/android/media_player_bridge.h (right):

http://codereview.chromium.org/10919075/diff/15002/media/base/android/media_p...
media/base/android/media_player_bridge.h:225: scoped_ptr<MediaPlayerListener>
listener_;
I was referring to replacing this line of code with:
MediaPlayerListener listener_;

...in other words you don't need to heap-allocate the object

[brettw, 2012-09-13 18:30:04]

Removing myself as a reviewer. This is getting long and I don't know what I need
to review. When regular reviews are done & if you still need an owner review,
please re-add me with what I should look at.

[qinmin, 2012-09-13 18:51:16]

http://codereview.chromium.org/10919075/diff/15002/content/browser/android/co...
File content/browser/android/cookie_getter_impl.cc (right):

http://codereview.chromium.org/10919075/diff/15002/content/browser/android/co...
content/browser/android/cookie_getter_impl.cc:50: finish_event_.Wait();
If this function finishes here, CookieGetterImpl::GetCookiesCallback will start
to run because of PostTaskAndReply. Using a waitable event here is to make sure
all the async tasks are finished before GetCookiesCallback.

Changed the code to use postTask() instead of postTaskAndReply(). The benefit of
using postTaskAndReply() is that we don't need to pass a weakPtr and the
callback object to CookieGetterTask, and also no need to make GetCookiesCallback
public. However, the drawback is that we have to rely on a waitableEvent to wait
for the async tasks to finish
On 2012/09/13 10:40:14, scherkus wrote:
> why the blocking wait on the IO thread?
> 
> is there really no way to accomplish the same goal in an async / cb / message
> posting fashion?

http://codereview.chromium.org/10919075/diff/15002/content/browser/android/co...
File content/browser/android/cookie_getter_impl.h (right):

http://codereview.chromium.org/10919075/diff/15002/content/browser/android/co...
content/browser/android/cookie_getter_impl.h:29: class CookieGetterTask :
On 2012/09/13 10:40:14, scherkus wrote:
> nit: you can fwd declare this + move to .cc as CookieGetterImpl clients
> shouldn't have to deal w/ this class

Done.

http://codereview.chromium.org/10919075/diff/15002/content/browser/android/co...
content/browser/android/cookie_getter_impl.h:30: public
base::RefCountedThreadSafe<CookieGetterTask> {
On 2012/09/13 10:40:14, scherkus wrote:
> : goes on this line

Done.

http://codereview.chromium.org/10919075/diff/15002/content/renderer/media/web...
File content/renderer/media/webmediaplayer_proxy_impl_android.h (right):

http://codereview.chromium.org/10919075/diff/15002/content/renderer/media/web...
content/renderer/media/webmediaplayer_proxy_impl_android.h:22: //
WebMediaPlayerImplAndroid and the MediaplayerManagerAndroid in the browser
On 2012/09/13 00:14:48, Yaron wrote:
> Nit: MediaPlayerManagerAndroid

Done.

http://codereview.chromium.org/10919075/diff/15002/media/base/android/media_p...
File media/base/android/media_player_bridge.cc (right):

http://codereview.chromium.org/10919075/diff/15002/media/base/android/media_p...
media/base/android/media_player_bridge.cc:175: ConvertUTF8ToJavaString(env,
url_);
looks like the url->url_ just make this line 80 characters. I must be looking at
the end of the line where eclipse shows 81. Fixed.

On 2012/09/13 10:40:14, scherkus wrote:
> why the code change? looks like it should fit on previous line

http://codereview.chromium.org/10919075/diff/15002/media/base/android/media_p...
File media/base/android/media_player_bridge.h (right):

http://codereview.chromium.org/10919075/diff/15002/media/base/android/media_p...
media/base/android/media_player_bridge.h:225: scoped_ptr<MediaPlayerListener>
listener_;
Sorry, done.

On 2012/09/13 10:40:14, scherkus wrote:
> I was referring to replacing this line of code with:
> MediaPlayerListener listener_;
> 
> ...in other words you don't need to heap-allocate the object

[scherkus (not reviewing), 2012-09-14 08:09:40]

http://codereview.chromium.org/10919075/diff/15002/content/browser/android/co...
File content/browser/android/cookie_getter_impl.cc (right):

http://codereview.chromium.org/10919075/diff/15002/content/browser/android/co...
content/browser/android/cookie_getter_impl.cc:50: finish_event_.Wait();
PostTaskAndReply() is handy but it only works for synchronously executing
functions. While it'll work as intended for your error cases it won't work where
you rely on an async function to complete.

On 2012/09/13 18:51:16, qinmin wrote:
> If this function finishes here, CookieGetterImpl::GetCookiesCallback will
start
> to run because of PostTaskAndReply. Using a waitable event here is to make
sure
> all the async tasks are finished before GetCookiesCallback.
> 
> Changed the code to use postTask() instead of postTaskAndReply(). The benefit
of
> using postTaskAndReply() is that we don't need to pass a weakPtr and the
> callback object to CookieGetterTask, and also no need to make
GetCookiesCallback
> public. However, the drawback is that we have to rely on a waitableEvent to
wait
> for the async tasks to finish
> On 2012/09/13 10:40:14, scherkus wrote:
> > why the blocking wait on the IO thread?
> > 
> > is there really no way to accomplish the same goal in an async / cb /
message
> > posting fashion?
>

http://codereview.chromium.org/10919075/diff/14013/content/browser/android/co...
File content/browser/android/cookie_getter_impl.cc (right):

http://codereview.chromium.org/10919075/diff/14013/content/browser/android/co...
content/browser/android/cookie_getter_impl.cc:153: task, GURL(url),
GURL(first_party_for_cookies), callback));
I think you can simplify this even more :)

  * Define a static helper that posts a GetCookieCB from IO to UI thread
static void RunOnUIThread(
    const GetCookieCB& callback, const std::string& cookies) { 
  DCHECK(on IO thread)
  BrowserThread::PostTask(UI, FROM_HERE, base::Bind(callback, cookies));
}

  * base::Bind() a new GetCookieCB to weak_this_->GetCookiesCallback + bind
|callback| into it (requires swapping parameter order of of
GetCookiesCallback()) + bind again to above helper:
BrowserThread::PostTask(
    IO, FROM_HERE, base::Bind(
    &CookieGetterTask::RequestCookies, task, GURL(url), GURL(f_p_f_c),
base::bind(
    &RunOnUIThread, base::Bind(
    &CookieGetterImpl::GetCookiesCallback, weak_this_.GetWeakPtr(), callback))))

What this means is that you don't have to pass a WeakPtr of CookieGetterImpl to
CookieGetterTask as the weak reference is maintained inside the closure. Since
we bind the RunOnUIThread() method you can remove the ReturnCookies() method and
inside CookieGetterTask simply execute the callback w/ the cookies.

Should CookieGetterImpl get deleted, by time the final callback to
GetCookiesCallback() is executed it'll no-op due to the weak pointer.

In a way this is like an async version of PostTaskAndReply() as the the callee
can replying via a callback and not have to concern itself with which thread it
should post the reply to.

WDYT?

[qinmin, 2012-09-14 16:50:47]

http://codereview.chromium.org/10919075/diff/14013/content/browser/android/co...
File content/browser/android/cookie_getter_impl.cc (right):

http://codereview.chromium.org/10919075/diff/14013/content/browser/android/co...
content/browser/android/cookie_getter_impl.cc:153: task, GURL(url),
GURL(first_party_for_cookies), callback));
Yes, I was thinking of something similar last night by binding the weakptr and
the callback and pass them into requestCookies().

Done.

On 2012/09/14 08:09:41, scherkus wrote:
> I think you can simplify this even more :)
> 
>   * Define a static helper that posts a GetCookieCB from IO to UI thread
> static void RunOnUIThread(
>     const GetCookieCB& callback, const std::string& cookies) { 
>   DCHECK(on IO thread)
>   BrowserThread::PostTask(UI, FROM_HERE, base::Bind(callback, cookies));
> }
> 
>   * base::Bind() a new GetCookieCB to weak_this_->GetCookiesCallback + bind
> |callback| into it (requires swapping parameter order of of
> GetCookiesCallback()) + bind again to above helper:
> BrowserThread::PostTask(
>     IO, FROM_HERE, base::Bind(
>     &CookieGetterTask::RequestCookies, task, GURL(url), GURL(f_p_f_c),
> base::bind(
>     &RunOnUIThread, base::Bind(
>     &CookieGetterImpl::GetCookiesCallback, weak_this_.GetWeakPtr(),
callback))))
> 
> What this means is that you don't have to pass a WeakPtr of CookieGetterImpl
to
> CookieGetterTask as the weak reference is maintained inside the closure. Since
> we bind the RunOnUIThread() method you can remove the ReturnCookies() method
and
> inside CookieGetterTask simply execute the callback w/ the cookies.
> 
> Should CookieGetterImpl get deleted, by time the final callback to
> GetCookiesCallback() is executed it'll no-op due to the weak pointer.
> 
> In a way this is like an async version of PostTaskAndReply() as the the callee
> can replying via a callback and not have to concern itself with which thread
it
> should post the reply to.
> 
> WDYT?

[scherkus (not reviewing), 2012-09-17 14:28:44]

LGTM w/ nit

final thing: don't forget to update the CL description to reflect changes made
-- also do you have a bug # for the BUG= field?

http://codereview.chromium.org/10919075/diff/7010/content/browser/android/coo...
File content/browser/android/cookie_getter_impl.cc (right):

http://codereview.chromium.org/10919075/diff/7010/content/browser/android/coo...
content/browser/android/cookie_getter_impl.cc:138: GetCookieCB cb = base::Bind(
nit: if you bind this cb into a bind of ReturnCookieOnUIThread() you can
simplify RequestCookies() by replacing all the calls with simply
callback.Run(cookies);

that way the contract of CookieGetterTask is that it  operates solely on IO
thread and doesn't assume anything about the caller's thread of execution (i.e.,
its the caller's responsibility to handle posting back to the correct thread if
needed)

http://codereview.chromium.org/10919075/diff/7010/content/browser/android/coo...
File content/browser/android/cookie_getter_impl.h (right):

http://codereview.chromium.org/10919075/diff/7010/content/browser/android/coo...
content/browser/android/cookie_getter_impl.h:42: // Called when GetCookies
finishes.
nit: GetCookies()

[qinmin, 2012-09-17 18:16:22]

Thanks, scherkus

http://codereview.chromium.org/10919075/diff/7010/content/browser/android/coo...
File content/browser/android/cookie_getter_impl.cc (right):

http://codereview.chromium.org/10919075/diff/7010/content/browser/android/coo...
content/browser/android/cookie_getter_impl.cc:138: GetCookieCB cb = base::Bind(
On 2012/09/17 14:28:44, scherkus wrote:
> nit: if you bind this cb into a bind of ReturnCookieOnUIThread() you can
> simplify RequestCookies() by replacing all the calls with simply
> callback.Run(cookies);
> 
> that way the contract of CookieGetterTask is that it  operates solely on IO
> thread and doesn't assume anything about the caller's thread of execution
(i.e.,
> its the caller's responsibility to handle posting back to the correct thread
if
> needed)

Done.

http://codereview.chromium.org/10919075/diff/7010/content/browser/android/coo...
File content/browser/android/cookie_getter_impl.h (right):

http://codereview.chromium.org/10919075/diff/7010/content/browser/android/coo...
content/browser/android/cookie_getter_impl.h:42: // Called when GetCookies
finishes.
On 2012/09/17 14:28:44, scherkus wrote:
> nit: GetCookies()

Done.

[qinmin, 2012-09-17 18:20:24]

@tkent: Hi, Kent, can you please help take a look for files under
webkit/support? I need OWNERS approval there.

@jam: Hi, John, would you please help me review files under content/, excluding
all the android/ dirs? there should not be too many lines there

[tkent, 2012-09-18 06:09:46]

http://codereview.chromium.org/10919075/diff/2065/webkit/support/webkit_suppo...
File webkit/support/webkit_support.cc (right):

http://codereview.chromium.org/10919075/diff/2065/webkit/support/webkit_suppo...
webkit/support/webkit_support.cc:171: new
webkit_media::MediaPlayerBridgeManagerImpl(8));
Why is it 8?

We need a comment or a const symbol to explain 8.

[qinmin, 2012-09-18 16:26:05]

http://codereview.chromium.org/10919075/diff/2065/webkit/support/webkit_suppo...
File webkit/support/webkit_support.cc (right):

http://codereview.chromium.org/10919075/diff/2065/webkit/support/webkit_suppo...
webkit/support/webkit_support.cc:171: new
webkit_media::MediaPlayerBridgeManagerImpl(8));
Comment added.
8 is the current maximum number of media elements in a layout tests. 
This is for us to manage the decoding resources. When we have more than 8
decoders allocated, we will start freeing them as the resource is limited on
android.

On 2012/09/18 06:09:47, Kent Tamura wrote:
> Why is it 8?
> 
> We need a comment or a const symbol to explain 8.

[tkent, 2012-09-19 00:43:44]

LGTM for webkit/support/

[jam, 2012-09-19 01:47:14]

lgtm

http://codereview.chromium.org/10919075/diff/20001/content/browser/renderer_h...
File content/browser/renderer_host/render_view_host_impl.h (right):

http://codereview.chromium.org/10919075/diff/20001/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_impl.h:675: // This class
inherits from RenderViewHostObserver, it will get deleted when
skip this last sentence (see my other comments on comments)

http://codereview.chromium.org/10919075/diff/20001/content/renderer/media/web...
File content/renderer/media/webmediaplayer_proxy_impl_android.cc (right):

http://codereview.chromium.org/10919075/diff/20001/content/renderer/media/web...
content/renderer/media/webmediaplayer_proxy_impl_android.cc:42:
IPC_MESSAGE_HANDLER(ViewMsg_MediaTimeUpdate,
nit: it looks like most of these can fit on one line

http://codereview.chromium.org/10919075/diff/20001/content/renderer/media/web...
File content/renderer/media/webmediaplayer_proxy_impl_android.h (right):

http://codereview.chromium.org/10919075/diff/20001/content/renderer/media/web...
content/renderer/media/webmediaplayer_proxy_impl_android.h:11: #include
"ipc/ipc_channel_proxy.h"
nit: not needed

http://codereview.chromium.org/10919075/diff/20001/content/renderer/render_vi...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/10919075/diff/20001/content/renderer/render_vi...
content/renderer/render_view_impl.h:1412: // deleted when RenderView is gone as
it inherits from RenderViewObserver.
nit: skip the last sentence since it's just describing what someone can see by
looking at the code (see the style guide section on comments)

http://codereview.chromium.org/10919075/diff/20001/content/renderer/render_vi...
content/renderer/render_view_impl.h:1419: // created in the renderer process.
This object is lazily initialized.
ditto

[qinmin, 2012-09-19 17:36:20]

http://codereview.chromium.org/10919075/diff/20001/content/browser/renderer_h...
File content/browser/renderer_host/render_view_host_impl.h (right):

http://codereview.chromium.org/10919075/diff/20001/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_impl.h:675: // This class
inherits from RenderViewHostObserver, it will get deleted when
On 2012/09/19 01:47:14, John Abd-El-Malek wrote:
> skip this last sentence (see my other comments on comments)

Done.

http://codereview.chromium.org/10919075/diff/20001/content/renderer/media/web...
File content/renderer/media/webmediaplayer_proxy_impl_android.cc (right):

http://codereview.chromium.org/10919075/diff/20001/content/renderer/media/web...
content/renderer/media/webmediaplayer_proxy_impl_android.cc:42:
IPC_MESSAGE_HANDLER(ViewMsg_MediaTimeUpdate,
On 2012/09/19 01:47:14, John Abd-El-Malek wrote:
> nit: it looks like most of these can fit on one line

Done.

http://codereview.chromium.org/10919075/diff/20001/content/renderer/media/web...
File content/renderer/media/webmediaplayer_proxy_impl_android.h (right):

http://codereview.chromium.org/10919075/diff/20001/content/renderer/media/web...
content/renderer/media/webmediaplayer_proxy_impl_android.h:11: #include
"ipc/ipc_channel_proxy.h"
On 2012/09/19 01:47:14, John Abd-El-Malek wrote:
> nit: not needed

Done.

http://codereview.chromium.org/10919075/diff/20001/content/renderer/render_vi...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/10919075/diff/20001/content/renderer/render_vi...
content/renderer/render_view_impl.h:1412: // deleted when RenderView is gone as
it inherits from RenderViewObserver.
On 2012/09/19 01:47:14, John Abd-El-Malek wrote:
> nit: skip the last sentence since it's just describing what someone can see by
> looking at the code (see the style guide section on comments)

Done.

http://codereview.chromium.org/10919075/diff/20001/content/renderer/render_vi...
content/renderer/render_view_impl.h:1419: // created in the renderer process.
This object is lazily initialized.
On 2012/09/19 01:47:14, John Abd-El-Malek wrote:
> ditto

Done.

[commit-bot: I haz the power, 2012-09-19 17:37:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qinmin@chromium.org/10919075/25001

[commit-bot: I haz the power, 2012-09-19 20:29:06]

Change committed as 157596