Make RendererMediaPlayerManager a RenderFrameObserver.

Make RendererMediaPlayerManager a RenderFrameObserver.

Major change:
- Make BrowserMediaPlayerManager (BMPM) a per RenderFrame object.
- Introduce MediaWebContentsObserver (MWCO) which managers all BMPMs.
- MWCO forwards all IPCs to BMPM.

Other changes necessary to switch from RenderView to RenderFrame:
- Use MediaWebContentsObserver to send PauseVideo message to render.
- Use render frame routing ID to set surface peer.

VIDEO_HOLE related changes:
- Add DidCommitCompositorFrame() to RenderFrameObserver.
- Add OnFrameInfoUpdated() to MediaWebContentsObserver.

BUG=338910
TEST=Tested with test pages and Youtube purchased movies.

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

[xhwang, 2014-05-12 16:58:47]

This CL is based on https://codereview.chromium.org/276973005/.

ddorwin/qinmin: PTAL
damienv: FYI

[xhwang, 2014-05-12 17:01:57]

https://codereview.chromium.org/278353003/diff/1/content/browser/android/chil...
File content/browser/android/child_process_launcher_android.cc (right):

https://codereview.chromium.org/278353003/diff/1/content/browser/android/chil...
content/browser/android/child_process_launcher_android.cc:64: // instead of
|render_view_id|.
qinmin: let me know if this TODO makes sense ;)

https://codereview.chromium.org/278353003/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/278353003/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:3304:
GetRenderViewHostImpl()->media_player_manager()->DestroyAllMediaPlayers();
This should be taken care of by MediaWebContentsObserver::RenderFrameDeleted().

[jam, 2014-05-12 17:09:12]

I'm curious why you're changing BrowserMediaPlayerManager so that it's per
RenderFrameHost? In doing the refactorings so far, we have tried to make the
code in the browser work per-page instead of per-frame. Can you give more
background?

[xhwang, 2014-05-12 20:25:04]

On 2014/05/12 17:09:12, jam wrote:
> I'm curious why you're changing BrowserMediaPlayerManager so that it's per
> RenderFrameHost? In doing the refactorings so far, we have tried to make the
> code in the browser work per-page instead of per-frame. Can you give more
> background?

Well, in short, I thought RenderViewXxx is going away and we should move
everything to RenderFrameXxx. Is this wrong?

More details about this CL:

At the renderer side, the media player is created in RenderFrameImpl [1]. In
parallel to that, the CDM (for EME implementation) is also created in
RenderFrameImpl [2].

At the browser side, we still have a per page object MediaWebContentsObserver
which receives IPCs for all RenderFrameHosts (implements OnMessage(msg,
render_frame_host)). Instead of having MediaWebContentsObserver manage all
players in all frames, I convert the old BrowserMediaPlayerManager, which was a
per page (render view) manager, to a per frame manager, which manages all
players in that frame. I feel that this makes the code a bit cleaner, e.g. in
RenderFrameDeleted(), I can easily destroy all players that belong to that
frame.

The next step in my plan is add a per frame BrowserCdmManager which is also
managed by the per page object MediaWebContentsObserver.

Let me know if I am missing anything or am totally on a wrong path :)

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...

[xhwang, 2014-05-13 15:35:45]

On 2014/05/12 20:25:04, xhwang wrote:
> On 2014/05/12 17:09:12, jam wrote:
> > I'm curious why you're changing BrowserMediaPlayerManager so that it's per
> > RenderFrameHost? In doing the refactorings so far, we have tried to make the
> > code in the browser work per-page instead of per-frame. Can you give more
> > background?
> 
> Well, in short, I thought RenderViewXxx is going away and we should move
> everything to RenderFrameXxx. Is this wrong?
> 
> More details about this CL:
> 
> At the renderer side, the media player is created in RenderFrameImpl [1]. In
> parallel to that, the CDM (for EME implementation) is also created in
> RenderFrameImpl [2].
> 
> At the browser side, we still have a per page object MediaWebContentsObserver
> which receives IPCs for all RenderFrameHosts (implements OnMessage(msg,
> render_frame_host)). Instead of having MediaWebContentsObserver manage all
> players in all frames, I convert the old BrowserMediaPlayerManager, which was
a
> per page (render view) manager, to a per frame manager, which manages all
> players in that frame. I feel that this makes the code a bit cleaner, e.g. in
> RenderFrameDeleted(), I can easily destroy all players that belong to that
> frame.
> 
> The next step in my plan is add a per frame BrowserCdmManager which is also
> managed by the per page object MediaWebContentsObserver.
> 
> Let me know if I am missing anything or am totally on a wrong path :)
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
> [2]
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...

+jam, in case you missed my reply. Please see my reply and PTAL at the CL since
this CL might depend on how you implement IPC_MESSAGE_XXX macros to support
RenderFrameHost* binding.

[jam, 2014-05-13 20:49:42]

On 2014/05/13 15:35:45, xhwang wrote:
> On 2014/05/12 20:25:04, xhwang wrote:
> > On 2014/05/12 17:09:12, jam wrote:
> > > I'm curious why you're changing BrowserMediaPlayerManager so that it's per
> > > RenderFrameHost? In doing the refactorings so far, we have tried to make
the
> > > code in the browser work per-page instead of per-frame. Can you give more
> > > background?
> > 
> > Well, in short, I thought RenderViewXxx is going away and we should move
> > everything to RenderFrameXxx. Is this wrong?
> > 
> > More details about this CL:
> > 
> > At the renderer side, the media player is created in RenderFrameImpl [1]. In
> > parallel to that, the CDM (for EME implementation) is also created in
> > RenderFrameImpl [2].
> > 
> > At the browser side, we still have a per page object
MediaWebContentsObserver
> > which receives IPCs for all RenderFrameHosts (implements OnMessage(msg,
> > render_frame_host)). Instead of having MediaWebContentsObserver manage all
> > players in all frames, I convert the old BrowserMediaPlayerManager, which
was
> a
> > per page (render view) manager, to a per frame manager, which manages all
> > players in that frame. I feel that this makes the code a bit cleaner, e.g.
in
> > RenderFrameDeleted(), I can easily destroy all players that belong to that
> > frame.
> > 
> > The next step in my plan is add a per frame BrowserCdmManager which is also
> > managed by the per page object MediaWebContentsObserver.
> > 
> > Let me know if I am missing anything or am totally on a wrong path :)
> > 
> > [1]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
> > [2]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
> 
> +jam, in case you missed my reply. Please see my reply and PTAL at the CL
since
> this CL might depend on how you implement IPC_MESSAGE_XXX macros to support
> RenderFrameHost* binding.

yep I had seen this cl; the question I have to you: would you do things
differently if the IPC message handlers had access to the RenderFrameHost
easily? see https://codereview.chromium.org/283623002/

[jam, 2014-05-13 23:29:52]

On 2014/05/13 20:49:42, jam wrote:
> On 2014/05/13 15:35:45, xhwang wrote:
> > On 2014/05/12 20:25:04, xhwang wrote:
> > > On 2014/05/12 17:09:12, jam wrote:
> > > > I'm curious why you're changing BrowserMediaPlayerManager so that it's
per
> > > > RenderFrameHost? In doing the refactorings so far, we have tried to make
> the
> > > > code in the browser work per-page instead of per-frame. Can you give
more
> > > > background?
> > > 
> > > Well, in short, I thought RenderViewXxx is going away and we should move
> > > everything to RenderFrameXxx. Is this wrong?
> > > 
> > > More details about this CL:
> > > 
> > > At the renderer side, the media player is created in RenderFrameImpl [1].
In
> > > parallel to that, the CDM (for EME implementation) is also created in
> > > RenderFrameImpl [2].
> > > 
> > > At the browser side, we still have a per page object
> MediaWebContentsObserver
> > > which receives IPCs for all RenderFrameHosts (implements OnMessage(msg,
> > > render_frame_host)). Instead of having MediaWebContentsObserver manage all
> > > players in all frames, I convert the old BrowserMediaPlayerManager, which
> was
> > a
> > > per page (render view) manager, to a per frame manager, which manages all
> > > players in that frame. I feel that this makes the code a bit cleaner, e.g.
> in
> > > RenderFrameDeleted(), I can easily destroy all players that belong to that
> > > frame.
> > > 
> > > The next step in my plan is add a per frame BrowserCdmManager which is
also
> > > managed by the per page object MediaWebContentsObserver.
> > > 
> > > Let me know if I am missing anything or am totally on a wrong path :)
> > > 
> > > [1]
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
> > > [2]
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
> > 
> > +jam, in case you missed my reply. Please see my reply and PTAL at the CL
> since
> > this CL might depend on how you implement IPC_MESSAGE_XXX macros to support
> > RenderFrameHost* binding.
> 
> yep I had seen this cl; the question I have to you: would you do things
> differently if the IPC message handlers had access to the RenderFrameHost
> easily? see https://codereview.chromium.org/283623002/

btw this landed in r270237

[xhwang, 2014-05-13 23:59:18]

On 2014/05/13 23:29:52, jam wrote:
> On 2014/05/13 20:49:42, jam wrote:
> > On 2014/05/13 15:35:45, xhwang wrote:
> > > On 2014/05/12 20:25:04, xhwang wrote:
> > > > On 2014/05/12 17:09:12, jam wrote:
> > > > > I'm curious why you're changing BrowserMediaPlayerManager so that it's
> per
> > > > > RenderFrameHost? In doing the refactorings so far, we have tried to
make
> > the
> > > > > code in the browser work per-page instead of per-frame. Can you give
> more
> > > > > background?
> > > > 
> > > > Well, in short, I thought RenderViewXxx is going away and we should move
> > > > everything to RenderFrameXxx. Is this wrong?
> > > > 
> > > > More details about this CL:
> > > > 
> > > > At the renderer side, the media player is created in RenderFrameImpl
[1].
> In
> > > > parallel to that, the CDM (for EME implementation) is also created in
> > > > RenderFrameImpl [2].
> > > > 
> > > > At the browser side, we still have a per page object
> > MediaWebContentsObserver
> > > > which receives IPCs for all RenderFrameHosts (implements OnMessage(msg,
> > > > render_frame_host)). Instead of having MediaWebContentsObserver manage
all
> > > > players in all frames, I convert the old BrowserMediaPlayerManager,
which
> > was
> > > a
> > > > per page (render view) manager, to a per frame manager, which manages
all
> > > > players in that frame. I feel that this makes the code a bit cleaner,
e.g.
> > in
> > > > RenderFrameDeleted(), I can easily destroy all players that belong to
that
> > > > frame.
> > > > 
> > > > The next step in my plan is add a per frame BrowserCdmManager which is
> also
> > > > managed by the per page object MediaWebContentsObserver.
> > > > 
> > > > Let me know if I am missing anything or am totally on a wrong path :)
> > > > 
> > > > [1]
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
> > > > [2]
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
> > > 
> > > +jam, in case you missed my reply. Please see my reply and PTAL at the CL
> > since
> > > this CL might depend on how you implement IPC_MESSAGE_XXX macros to
support
> > > RenderFrameHost* binding.
> > 
> > yep I had seen this cl; the question I have to you: would you do things
> > differently if the IPC message handlers had access to the RenderFrameHost
> > easily? see https://codereview.chromium.org/283623002/
> 
> btw this landed in r270237

Yeah, since most views will probably only have one media player, having another
(frame) layer probably is overkill. I'll use your new macro and reduce the extra
layer. That will actually make my CL simpler.

That being said. In the next CL, I need to separate CDMs from media players.
I'll probably add another WebContentsObserver implementation (BrowserCdmManager)
which will handle CDM IPCs. Then when player->SetCdm() is called, I need to find
the correct CDM in BrowserCdmManager. This should all work and I don't see any
issues with them. Just an early heads-up :)

I'll update my CL. Thanks.

[xhwang, 2014-05-14 17:38:59]

On 2014/05/13 23:59:18, xhwang wrote:
> On 2014/05/13 23:29:52, jam wrote:
> > On 2014/05/13 20:49:42, jam wrote:
> > > On 2014/05/13 15:35:45, xhwang wrote:
> > > > On 2014/05/12 20:25:04, xhwang wrote:
> > > > > On 2014/05/12 17:09:12, jam wrote:
> > > > > > I'm curious why you're changing BrowserMediaPlayerManager so that
it's
> > per
> > > > > > RenderFrameHost? In doing the refactorings so far, we have tried to
> make
> > > the
> > > > > > code in the browser work per-page instead of per-frame. Can you give
> > more
> > > > > > background?
> > > > > 
> > > > > Well, in short, I thought RenderViewXxx is going away and we should
move
> > > > > everything to RenderFrameXxx. Is this wrong?
> > > > > 
> > > > > More details about this CL:
> > > > > 
> > > > > At the renderer side, the media player is created in RenderFrameImpl
> [1].
> > In
> > > > > parallel to that, the CDM (for EME implementation) is also created in
> > > > > RenderFrameImpl [2].
> > > > > 
> > > > > At the browser side, we still have a per page object
> > > MediaWebContentsObserver
> > > > > which receives IPCs for all RenderFrameHosts (implements
OnMessage(msg,
> > > > > render_frame_host)). Instead of having MediaWebContentsObserver manage
> all
> > > > > players in all frames, I convert the old BrowserMediaPlayerManager,
> which
> > > was
> > > > a
> > > > > per page (render view) manager, to a per frame manager, which manages
> all
> > > > > players in that frame. I feel that this makes the code a bit cleaner,
> e.g.
> > > in
> > > > > RenderFrameDeleted(), I can easily destroy all players that belong to
> that
> > > > > frame.
> > > > > 
> > > > > The next step in my plan is add a per frame BrowserCdmManager which is
> > also
> > > > > managed by the per page object MediaWebContentsObserver.
> > > > > 
> > > > > Let me know if I am missing anything or am totally on a wrong path :)
> > > > > 
> > > > > [1]
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
> > > > > [2]
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
> > > > 
> > > > +jam, in case you missed my reply. Please see my reply and PTAL at the
CL
> > > since
> > > > this CL might depend on how you implement IPC_MESSAGE_XXX macros to
> support
> > > > RenderFrameHost* binding.
> > > 
> > > yep I had seen this cl; the question I have to you: would you do things
> > > differently if the IPC message handlers had access to the RenderFrameHost
> > > easily? see https://codereview.chromium.org/283623002/
> > 
> > btw this landed in r270237
> 
> Yeah, since most views will probably only have one media player, having
another
> (frame) layer probably is overkill. I'll use your new macro and reduce the
extra
> layer. That will actually make my CL simpler.
> 
> That being said. In the next CL, I need to separate CDMs from media players.
> I'll probably add another WebContentsObserver implementation
(BrowserCdmManager)
> which will handle CDM IPCs. Then when player->SetCdm() is called, I need to
find
> the correct CDM in BrowserCdmManager. This should all work and I don't see any
> issues with them. Just an early heads-up :)
> 
> I'll update my CL. Thanks.

jam@, I tried to apply your macro and reduce one layer of redirecting, but I see
the following problems:

(1) Now each IPC handling function in BMPM has to have an extra parameter
"RenderFrameHost*". Since we have a lot of these functions, this ends up with
some extra code which makes this class kind of busy.

(2) Now we need to be able to find the correct "player" given a
"RenderFrameHost*" and a "player_id". So I need a more complicated data
structure in BMPM to hold the players, e.g. std::map<RenderFrameHost*,
std::map<int, Player*> >. This is fine. But see (3).

(3) BMPM also overrides a lot of media::MediaPlayerManager methods (e.g.
BMPM::OnTimeUpdate()) which are called by the "players". These methods also
takes "player_id" so that the correct IPC can be sent to the render side. But
now we need both "RenderFrameHost*" and a "player_id" to be able to locate a
player or send the IPC to the correct render frame. I could change the
MediaPlayerManager interface so that it provides callbacks to the players
instead of a "manager" interface. Then I can bind "RenderFrameHost*" in the
callbacks. But that'll be a pretty big change and I still feel we should NOT
spread the "RenderFrameHost*" too far.

(4) We will need to revisit the BMPM implementation later when we want to
support more platforms (not limited to Android). Currently my priority is
actually to hook up the CDM part to make the new EME spec work. So I probably
won't spend too much time refactoring the "player manager" code now.

Given these, I'd like to keep a layer to isolate "RenderFrameHost*" from the
manager and the players. So I feel I still like my current CL (using
IPC_MESSAGE_FORWARD) better.

WDYT?

[xhwang, 2014-05-15 17:56:34]

On 2014/05/14 17:38:59, xhwang wrote:
> On 2014/05/13 23:59:18, xhwang wrote:
> > On 2014/05/13 23:29:52, jam wrote:
> > > On 2014/05/13 20:49:42, jam wrote:
> > > > On 2014/05/13 15:35:45, xhwang wrote:
> > > > > On 2014/05/12 20:25:04, xhwang wrote:
> > > > > > On 2014/05/12 17:09:12, jam wrote:
> > > > > > > I'm curious why you're changing BrowserMediaPlayerManager so that
> it's
> > > per
> > > > > > > RenderFrameHost? In doing the refactorings so far, we have tried
to
> > make
> > > > the
> > > > > > > code in the browser work per-page instead of per-frame. Can you
give
> > > more
> > > > > > > background?
> > > > > > 
> > > > > > Well, in short, I thought RenderViewXxx is going away and we should
> move
> > > > > > everything to RenderFrameXxx. Is this wrong?
> > > > > > 
> > > > > > More details about this CL:
> > > > > > 
> > > > > > At the renderer side, the media player is created in RenderFrameImpl
> > [1].
> > > In
> > > > > > parallel to that, the CDM (for EME implementation) is also created
in
> > > > > > RenderFrameImpl [2].
> > > > > > 
> > > > > > At the browser side, we still have a per page object
> > > > MediaWebContentsObserver
> > > > > > which receives IPCs for all RenderFrameHosts (implements
> OnMessage(msg,
> > > > > > render_frame_host)). Instead of having MediaWebContentsObserver
manage
> > all
> > > > > > players in all frames, I convert the old BrowserMediaPlayerManager,
> > which
> > > > was
> > > > > a
> > > > > > per page (render view) manager, to a per frame manager, which
manages
> > all
> > > > > > players in that frame. I feel that this makes the code a bit
cleaner,
> > e.g.
> > > > in
> > > > > > RenderFrameDeleted(), I can easily destroy all players that belong
to
> > that
> > > > > > frame.
> > > > > > 
> > > > > > The next step in my plan is add a per frame BrowserCdmManager which
is
> > > also
> > > > > > managed by the per page object MediaWebContentsObserver.
> > > > > > 
> > > > > > Let me know if I am missing anything or am totally on a wrong path
:)
> > > > > > 
> > > > > > [1]
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
> > > > > > [2]
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
> > > > > 
> > > > > +jam, in case you missed my reply. Please see my reply and PTAL at the
> CL
> > > > since
> > > > > this CL might depend on how you implement IPC_MESSAGE_XXX macros to
> > support
> > > > > RenderFrameHost* binding.
> > > > 
> > > > yep I had seen this cl; the question I have to you: would you do things
> > > > differently if the IPC message handlers had access to the
RenderFrameHost
> > > > easily? see https://codereview.chromium.org/283623002/
> > > 
> > > btw this landed in r270237
> > 
> > Yeah, since most views will probably only have one media player, having
> another
> > (frame) layer probably is overkill. I'll use your new macro and reduce the
> extra
> > layer. That will actually make my CL simpler.
> > 
> > That being said. In the next CL, I need to separate CDMs from media players.
> > I'll probably add another WebContentsObserver implementation
> (BrowserCdmManager)
> > which will handle CDM IPCs. Then when player->SetCdm() is called, I need to
> find
> > the correct CDM in BrowserCdmManager. This should all work and I don't see
any
> > issues with them. Just an early heads-up :)
> > 
> > I'll update my CL. Thanks.
> 
> jam@, I tried to apply your macro and reduce one layer of redirecting, but I
see
> the following problems:
> 
> (1) Now each IPC handling function in BMPM has to have an extra parameter
> "RenderFrameHost*". Since we have a lot of these functions, this ends up with
> some extra code which makes this class kind of busy.
> 
> (2) Now we need to be able to find the correct "player" given a
> "RenderFrameHost*" and a "player_id". So I need a more complicated data
> structure in BMPM to hold the players, e.g. std::map<RenderFrameHost*,
> std::map<int, Player*> >. This is fine. But see (3).
> 
> (3) BMPM also overrides a lot of media::MediaPlayerManager methods (e.g.
> BMPM::OnTimeUpdate()) which are called by the "players". These methods also
> takes "player_id" so that the correct IPC can be sent to the render side. But
> now we need both "RenderFrameHost*" and a "player_id" to be able to locate a
> player or send the IPC to the correct render frame. I could change the
> MediaPlayerManager interface so that it provides callbacks to the players
> instead of a "manager" interface. Then I can bind "RenderFrameHost*" in the
> callbacks. But that'll be a pretty big change and I still feel we should NOT
> spread the "RenderFrameHost*" too far.
> 
> (4) We will need to revisit the BMPM implementation later when we want to
> support more platforms (not limited to Android). Currently my priority is
> actually to hook up the CDM part to make the new EME spec work. So I probably
> won't spend too much time refactoring the "player manager" code now.
> 
> Given these, I'd like to keep a layer to isolate "RenderFrameHost*" from the
> manager and the players. So I feel I still like my current CL (using
> IPC_MESSAGE_FORWARD) better.
> 
> WDYT?

jam@: kindly ping about your opinions :)

[xhwang, 2014-05-19 16:10:31]

On 2014/05/15 17:56:34, xhwang wrote:
> On 2014/05/14 17:38:59, xhwang wrote:
> > On 2014/05/13 23:59:18, xhwang wrote:
> > > On 2014/05/13 23:29:52, jam wrote:
> > > > On 2014/05/13 20:49:42, jam wrote:
> > > > > On 2014/05/13 15:35:45, xhwang wrote:
> > > > > > On 2014/05/12 20:25:04, xhwang wrote:
> > > > > > > On 2014/05/12 17:09:12, jam wrote:
> > > > > > > > I'm curious why you're changing BrowserMediaPlayerManager so
that
> > it's
> > > > per
> > > > > > > > RenderFrameHost? In doing the refactorings so far, we have tried
> to
> > > make
> > > > > the
> > > > > > > > code in the browser work per-page instead of per-frame. Can you
> give
> > > > more
> > > > > > > > background?
> > > > > > > 
> > > > > > > Well, in short, I thought RenderViewXxx is going away and we
should
> > move
> > > > > > > everything to RenderFrameXxx. Is this wrong?
> > > > > > > 
> > > > > > > More details about this CL:
> > > > > > > 
> > > > > > > At the renderer side, the media player is created in
RenderFrameImpl
> > > [1].
> > > > In
> > > > > > > parallel to that, the CDM (for EME implementation) is also created
> in
> > > > > > > RenderFrameImpl [2].
> > > > > > > 
> > > > > > > At the browser side, we still have a per page object
> > > > > MediaWebContentsObserver
> > > > > > > which receives IPCs for all RenderFrameHosts (implements
> > OnMessage(msg,
> > > > > > > render_frame_host)). Instead of having MediaWebContentsObserver
> manage
> > > all
> > > > > > > players in all frames, I convert the old
BrowserMediaPlayerManager,
> > > which
> > > > > was
> > > > > > a
> > > > > > > per page (render view) manager, to a per frame manager, which
> manages
> > > all
> > > > > > > players in that frame. I feel that this makes the code a bit
> cleaner,
> > > e.g.
> > > > > in
> > > > > > > RenderFrameDeleted(), I can easily destroy all players that belong
> to
> > > that
> > > > > > > frame.
> > > > > > > 
> > > > > > > The next step in my plan is add a per frame BrowserCdmManager
which
> is
> > > > also
> > > > > > > managed by the per page object MediaWebContentsObserver.
> > > > > > > 
> > > > > > > Let me know if I am missing anything or am totally on a wrong path
> :)
> > > > > > > 
> > > > > > > [1]
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
> > > > > > > [2]
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
> > > > > > 
> > > > > > +jam, in case you missed my reply. Please see my reply and PTAL at
the
> > CL
> > > > > since
> > > > > > this CL might depend on how you implement IPC_MESSAGE_XXX macros to
> > > support
> > > > > > RenderFrameHost* binding.
> > > > > 
> > > > > yep I had seen this cl; the question I have to you: would you do
things
> > > > > differently if the IPC message handlers had access to the
> RenderFrameHost
> > > > > easily? see https://codereview.chromium.org/283623002/
> > > > 
> > > > btw this landed in r270237
> > > 
> > > Yeah, since most views will probably only have one media player, having
> > another
> > > (frame) layer probably is overkill. I'll use your new macro and reduce the
> > extra
> > > layer. That will actually make my CL simpler.
> > > 
> > > That being said. In the next CL, I need to separate CDMs from media
players.
> > > I'll probably add another WebContentsObserver implementation
> > (BrowserCdmManager)
> > > which will handle CDM IPCs. Then when player->SetCdm() is called, I need
to
> > find
> > > the correct CDM in BrowserCdmManager. This should all work and I don't see
> any
> > > issues with them. Just an early heads-up :)
> > > 
> > > I'll update my CL. Thanks.
> > 
> > jam@, I tried to apply your macro and reduce one layer of redirecting, but I
> see
> > the following problems:
> > 
> > (1) Now each IPC handling function in BMPM has to have an extra parameter
> > "RenderFrameHost*". Since we have a lot of these functions, this ends up
with
> > some extra code which makes this class kind of busy.
> > 
> > (2) Now we need to be able to find the correct "player" given a
> > "RenderFrameHost*" and a "player_id". So I need a more complicated data
> > structure in BMPM to hold the players, e.g. std::map<RenderFrameHost*,
> > std::map<int, Player*> >. This is fine. But see (3).
> > 
> > (3) BMPM also overrides a lot of media::MediaPlayerManager methods (e.g.
> > BMPM::OnTimeUpdate()) which are called by the "players". These methods also
> > takes "player_id" so that the correct IPC can be sent to the render side.
But
> > now we need both "RenderFrameHost*" and a "player_id" to be able to locate a
> > player or send the IPC to the correct render frame. I could change the
> > MediaPlayerManager interface so that it provides callbacks to the players
> > instead of a "manager" interface. Then I can bind "RenderFrameHost*" in the
> > callbacks. But that'll be a pretty big change and I still feel we should NOT
> > spread the "RenderFrameHost*" too far.
> > 
> > (4) We will need to revisit the BMPM implementation later when we want to
> > support more platforms (not limited to Android). Currently my priority is
> > actually to hook up the CDM part to make the new EME spec work. So I
probably
> > won't spend too much time refactoring the "player manager" code now.
> > 
> > Given these, I'd like to keep a layer to isolate "RenderFrameHost*" from the
> > manager and the players. So I feel I still like my current CL (using
> > IPC_MESSAGE_FORWARD) better.
> > 
> > WDYT?
> 
> jam@: kindly ping about your opinions :)

jam@: I need to get this CL and at least 2 followup CLs landed this week. Kindly
ping....

[jam, 2014-05-20 00:34:49]

sorry for the delay, i've been busy with pdf stuff and didn't see your pings. in
the future, please feel free to IM me if I don't respond in a few hours.

your arguments are reasonable so this cl is fine, with one question about the
currently broken call.

thanks again for doing this conversion to render frames, much appreciated!

https://codereview.chromium.org/278353003/diff/1/content/browser/android/chil...
File content/browser/android/child_process_launcher_android.cc (right):

https://codereview.chromium.org/278353003/diff/1/content/browser/android/chil...
content/browser/android/child_process_launcher_android.cc:33: // identified by
render process handle, render view ID and player ID.
nit: update to render frame ID

https://codereview.chromium.org/278353003/diff/1/content/browser/android/chil...
content/browser/android/child_process_launcher_android.cc:64: // instead of
|render_view_id|.
On 2014/05/12 17:01:58, xhwang wrote:
> qinmin: let me know if this TODO makes sense ;)

hmm, this is going to be broken in the meantime. why can't this be fixed in this
cl?

https://codereview.chromium.org/278353003/diff/1/content/browser/media/androi...
File content/browser/media/android/browser_media_player_manager.h (right):

https://codereview.chromium.org/278353003/diff/1/content/browser/media/androi...
content/browser/media/android/browser_media_player_manager.h:221: const int
routing_id_;
nit: this is redundant since you can always get it from render_frame_host_

https://codereview.chromium.org/278353003/diff/1/content/renderer/render_view...
File content/renderer/render_view_impl.cc (left):

https://codereview.chromium.org/278353003/diff/1/content/renderer/render_view...
content/renderer/render_view_impl.cc:785: media_player_manager_ = new
RendererMediaPlayerManager(this);
remove the other references to media_player_manager_ from this file and the
header?

[xhwang, 2014-05-22 19:06:00]

I think I addressed all comments. I need to land this and some followup CLs
ASAP. Your prompt response will be highly appreciated.

jam@: Please take another look at the RenderView/RenderFrame/WebContents and IPC
forwarding related changes. I have to add some calls
(DidCommitCompositorFrame()/OnFrameInfoUpdated) to make VIDEO_HOLE path happy.

qinmin@: Please review everything, especially the SetSurfacePeer and PauseVideo
path to make sure I don't miss anything.

ycheo@: I don't know exactly how VIDEO_HOLE and
DidCommitCompositorFrame()/OnFrameInfoUpdated work :) I am just plumbing them
through in this CL. Please review make sure the VIDEO_HOLE path is still
working.

ddorwin@: I moved you to the CC list because I already have enough reviewers :)
But if you like feel free to review!

Thanks!

https://codereview.chromium.org/278353003/diff/1/content/browser/android/chil...
File content/browser/android/child_process_launcher_android.cc (right):

https://codereview.chromium.org/278353003/diff/1/content/browser/android/chil...
content/browser/android/child_process_launcher_android.cc:33: // identified by
render process handle, render view ID and player ID.
On 2014/05/20 00:34:49, jam wrote:
> nit: update to render frame ID

Done.

https://codereview.chromium.org/278353003/diff/1/content/browser/android/chil...
content/browser/android/child_process_launcher_android.cc:64: // instead of
|render_view_id|.
On 2014/05/20 00:34:49, jam wrote:
> On 2014/05/12 17:01:58, xhwang wrote:
> > qinmin: let me know if this TODO makes sense ;)
> 
> hmm, this is going to be broken in the meantime. why can't this be fixed in
this
> cl?

Done.

https://codereview.chromium.org/278353003/diff/1/content/browser/media/androi...
File content/browser/media/android/browser_media_player_manager.h (right):

https://codereview.chromium.org/278353003/diff/1/content/browser/media/androi...
content/browser/media/android/browser_media_player_manager.h:221: const int
routing_id_;
On 2014/05/20 00:34:49, jam wrote:
> nit: this is redundant since you can always get it from render_frame_host_

Done.

https://codereview.chromium.org/278353003/diff/1/content/renderer/render_view...
File content/renderer/render_view_impl.cc (left):

https://codereview.chromium.org/278353003/diff/1/content/renderer/render_view...
content/renderer/render_view_impl.cc:785: media_player_manager_ = new
RendererMediaPlayerManager(this);
On 2014/05/20 00:34:49, jam wrote:
> remove the other references to media_player_manager_ from this file and the
> header?

Done.

[xhwang, 2014-05-22 20:37:54]

ycheo:

It seems
org.chromium.android_webview.test.ExternalVideoSurfaceContainerTest#testEnableVideoOverlayForEmbeddedVideo
test is failing:

http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...

Do you have any quick idea on what's wrong with my CL? Or can you let me know
how I can run this test locally to investigate?

[jam, 2014-05-22 23:54:20]

lgtm

[xhwang, 2014-05-23 00:21:02]

On 2014/05/22 23:54:20, jam wrote:
> lgtm

It turned that render_widget only notifies swapped_out_frames about
DidCommitCompositorFrame. When VIDEO_HOLE is used, all media players (i.e. all
RenderFrames that have players) needs to get notified about
DidCommitCompositorFrame. Note that there's no swapped_out_frames for
AndroidWebView. So I made some changes in RenderWidget to make sure RenderFrames
with players will always get notified. It's a bit hacky, but it's the best I can
get right now.

ycheo@: Please help test the VIDEO_HOLE path.

jam@: Please take a look at PS5 to see whether it still looks good to you :)

Thanks!

[qinmin, 2014-05-23 01:40:22]

lgtm % nit

https://codereview.chromium.org/278353003/diff/80001/content/browser/media/an...
File content/browser/media/android/media_web_contents_observer.h (right):

https://codereview.chromium.org/278353003/diff/80001/content/browser/media/an...
content/browser/media/android/media_web_contents_observer.h:46: typedef
std::map<RenderFrameHost*, BrowserMediaPlayerManager*>
nit: what about using ScopedPtrHashMap?

[xhwang, 2014-05-23 04:16:39]

dcheng@: Please OWNERS review content/common/*messages*.h

[ycheo (away), 2014-05-23 05:31:51]

Verified that PS5 works well on Android Webview.

[dcheng, 2014-05-23 21:19:41]

LGTM

https://codereview.chromium.org/278353003/diff/80001/content/renderer/render_...
File content/renderer/render_view_impl.cc (left):

https://codereview.chromium.org/278353003/diff/80001/content/renderer/render_...
content/renderer/render_view_impl.cc:1328: // via
WebMediaPlayerAndroid::Destroy.
Is it worth moving this comment to the new IPC handler?

[xhwang, 2014-05-23 23:34:34]

https://codereview.chromium.org/278353003/diff/80001/content/browser/media/an...
File content/browser/media/android/media_web_contents_observer.h (right):

https://codereview.chromium.org/278353003/diff/80001/content/browser/media/an...
content/browser/media/android/media_web_contents_observer.h:46: typedef
std::map<RenderFrameHost*, BrowserMediaPlayerManager*>
On 2014/05/23 01:40:23, qinmin wrote:
> nit: what about using ScopedPtrHashMap?

Done.

https://codereview.chromium.org/278353003/diff/80001/content/renderer/render_...
File content/renderer/render_view_impl.cc (left):

https://codereview.chromium.org/278353003/diff/80001/content/renderer/render_...
content/renderer/render_view_impl.cc:1328: // via
WebMediaPlayerAndroid::Destroy.
On 2014/05/23 21:19:41, dcheng wrote:
> Is it worth moving this comment to the new IPC handler?

Done.

[kollas, 2014-05-27 02:35:42]

https://codereview.chromium.org/278353003/diff/160001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/278353003/diff/160001/content/renderer/render...
content/renderer/render_frame_impl.cc:433: #endif  // defined(VIDEO_HOLE)
media_player_manger_ is only available #ifdef OS_ANDROID, shouldn't this be
protected with OS_ANDROID as well?.

[xhwang, 2014-05-27 15:42:59]

jam@: Do you want to take another look at content/renderer/render_widget.cc ?
Thanks!

[jam, 2014-05-27 15:53:44]

On 2014/05/27 15:42:59, xhwang wrote:
> jam@: Do you want to take another look at content/renderer/render_widget.cc ?
> Thanks!

slgtm

[xhwang, 2014-05-27 16:25:36]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-05-27 16:26:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xhwang@chromium.org/278353003/160001

[commit-bot: I haz the power, 2014-05-27 18:54:24]

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

[commit-bot: I haz the power, 2014-05-27 18:57:27]

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

[commit-bot: I haz the power, 2014-05-27 18:57:28]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[xhwang, 2014-05-27 19:03:55]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-05-27 19:04:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xhwang@chromium.org/278353003/160001

[commit-bot: I haz the power, 2014-05-27 19:08:36]

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

[commit-bot: I haz the power, 2014-05-27 19:11:09]

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

[commit-bot: I haz the power, 2014-05-27 19:11:10]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[xhwang, 2014-05-27 20:36:27]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-05-27 20:37:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xhwang@chromium.org/278353003/160001

[commit-bot: I haz the power, 2014-05-27 21:17:20]

Change committed as 273033