Allow some Android media player implementation to be overridden by embedders.

Allow some Android media player implementation to be overridden by embedders.

BUG=178307
R=qinmin@chromium.org,bulach@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203080

[whywhat, 2013-05-24 19:08:42]

Please, take a look!

[qinmin, 2013-05-25 04:41:44]

https://codereview.chromium.org/15876006/diff/1/content/browser/android/media...
File content/browser/android/media_player_manager_impl.h (right):

https://codereview.chromium.org/15876006/diff/1/content/browser/android/media...
content/browser/android/media_player_manager_impl.h:41: virtual void
FullscreenPlayerPlay();
if you really need to override so many functions, why not inherit from
MediaPlayerManager?

[whywhat, 2013-05-28 15:16:40]

https://codereview.chromium.org/15876006/diff/1/content/browser/android/media...
File content/browser/android/media_player_manager_impl.h (right):

https://codereview.chromium.org/15876006/diff/1/content/browser/android/media...
content/browser/android/media_player_manager_impl.h:41: virtual void
FullscreenPlayerPlay();
On 2013/05/25 04:41:44, qinmin wrote:
> if you really need to override so many functions, why not inherit from
> MediaPlayerManager?

I only need to override a few of them but it feels wrong to selectively declare
some of these methods virtual. If I inherit from MediaPlayerManager I'll have to
duplicate lots of stuff that MediaPlayerManagerIml already does.

[qinmin, 2013-05-29 04:42:19]

https://codereview.chromium.org/15876006/diff/1/content/browser/android/media...
File content/browser/android/media_player_manager_impl.h (left):

https://codereview.chromium.org/15876006/diff/1/content/browser/android/media...
content/browser/android/media_player_manager_impl.h:95: bool is_media_source,
fix indentation

https://codereview.chromium.org/15876006/diff/1/content/browser/android/media...
File content/browser/android/media_player_manager_impl.h (right):

https://codereview.chromium.org/15876006/diff/1/content/browser/android/media...
content/browser/android/media_player_manager_impl.h:91: virtual void
OnEnterFullscreen(int player_id);
all these message handlers are now public just for 1 particular use case..
I think you can keep these methods private and declare a
RemoteMediaPlayerManagerImpl class, which inherits from MediaPlayerManagerImpl.
In the RemoteMediaPlayerManagerImpl, you override these method and declare them
public

[whywhat, 2013-05-29 15:49:18]

https://codereview.chromium.org/15876006/diff/1/content/browser/android/media...
File content/browser/android/media_player_manager_impl.h (left):

https://codereview.chromium.org/15876006/diff/1/content/browser/android/media...
content/browser/android/media_player_manager_impl.h:95: bool is_media_source,
On 2013/05/29 04:42:19, qinmin wrote:
> fix indentation

Done.

https://codereview.chromium.org/15876006/diff/1/content/browser/android/media...
File content/browser/android/media_player_manager_impl.h (right):

https://codereview.chromium.org/15876006/diff/1/content/browser/android/media...
content/browser/android/media_player_manager_impl.h:91: virtual void
OnEnterFullscreen(int player_id);
The handlers are protected, not public. Not only I need to override some of them
but also to call the base class version from the subclass to reuse the existing
implementation.

On 2013/05/29 04:42:19, qinmin wrote:
> all these message handlers are now public just for 1 particular use case..
> I think you can keep these methods private and declare a
> RemoteMediaPlayerManagerImpl class, which inherits from
MediaPlayerManagerImpl.
> In the RemoteMediaPlayerManagerImpl, you override these method and declare
them
> public

[qinmin, 2013-05-29 16:10:49]

lgtm

[commit-bot: I haz the power, 2013-05-29 16:23:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avayvod@chromium.org/15876006/8001

[commit-bot: I haz the power, 2013-05-29 16:54:26]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-05-29 17:59:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avayvod@chromium.org/15876006/8001

[commit-bot: I haz the power, 2013-05-29 18:14:30]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[bulach, 2013-05-29 18:31:23]

lgtm, thanks! just nits:

https://codereview.chromium.org/15876006/diff/8001/content/browser/android/me...
File content/browser/android/media_player_manager_impl.h (right):

https://codereview.chromium.org/15876006/diff/8001/content/browser/android/me...
content/browser/android/media_player_manager_impl.h:93: virtual void
OnEnterFullscreen(int player_id);
make clang happy, append OVERRIDE :)

https://codereview.chromium.org/15876006/diff/8001/media/base/android/media_p...
File media/base/android/media_player_android.h (right):

https://codereview.chromium.org/15876006/diff/8001/media/base/android/media_p...
media/base/android/media_player_android.h:79: virtual GURL
GetFirstPartyForCookies();
can these be pure virtuals, or at least move tho the next section?

https://codereview.chromium.org/15876006/diff/8001/media/base/android/media_p...
File media/base/android/media_player_bridge.h (right):

https://codereview.chromium.org/15876006/diff/8001/media/base/android/media_p...
media/base/android/media_player_bridge.h:89: virtual void CreateMediaPlayer();
nit: OVERRIDE

[whywhat, 2013-05-29 18:50:12]

https://codereview.chromium.org/15876006/diff/8001/content/browser/android/me...
File content/browser/android/media_player_manager_impl.h (right):

https://codereview.chromium.org/15876006/diff/8001/content/browser/android/me...
content/browser/android/media_player_manager_impl.h:93: virtual void
OnEnterFullscreen(int player_id);
On 2013/05/29 18:31:23, bulach wrote:
> make clang happy, append OVERRIDE :)

These are not overrides but virtual methods to override.

https://codereview.chromium.org/15876006/diff/8001/media/base/android/media_p...
File media/base/android/media_player_android.h (right):

https://codereview.chromium.org/15876006/diff/8001/media/base/android/media_p...
media/base/android/media_player_android.h:79: virtual GURL
GetFirstPartyForCookies();
On 2013/05/29 18:31:23, bulach wrote:
> can these be pure virtuals, or at least move tho the next section?

Nope, not every MediaPlayerAndroid will have a URL and cookies

https://codereview.chromium.org/15876006/diff/8001/media/base/android/media_p...
File media/base/android/media_player_bridge.h (right):

https://codereview.chromium.org/15876006/diff/8001/media/base/android/media_p...
media/base/android/media_player_bridge.h:89: virtual void CreateMediaPlayer();
On 2013/05/29 18:31:23, bulach wrote:
> nit: OVERRIDE

Ack

[commit-bot: I haz the power, 2013-05-29 18:50:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avayvod@chromium.org/15876006/8001

[commit-bot: I haz the power, 2013-05-30 05:10:14]

Change committed as 203080