[Presentation API] (alternative) 1-UA: send message between controller and receiver page

[Presentation API] (alternative) 1-UA: send message between controller and receiver page

It is an alternative implementation of 1-UA messaging (substitue Issue 2343013002 and Issue 2355723004):

- add PresentationConnection class to presentation.mojom
- add RenderedPresentationConnection class which implements PresentationConnectionProxy and PresentationConnection mojo interface

Work flow to connect controller RenderedPresentationConnection with receiver RenderedPresentationConnection:

1. controller page starts a presentation, creates a RenderedPresentationConnection object
2. controller RenderedPresentationConnection's mojo handler is passed to PSImpl via PSImpl::SetPresentationConnection
3. PSImpl::SetPresentationConnection registers the mojo handler with OffscreenPresentationManager
4. receiver page initializes and registers receiver_call to OffscreenPresentationManager
5. receiver PSImpl::OnReceiverConnectionAvailable() is invoked for each controller RenderedPresentationConnection's mojo handler belonging to the same presentation
6. PSImpl::OnReceiverConnectionAvailable() calls PDispatcher::OnReceiverConnectionAvailable() which creates a new receiver RenderedPresentationConnection and connects it with controller RenderedPresentationConnection's mojo handler

Messaging happens between controller RenderedPresentationConnection and receiver RenderedPresentationConnection afterwards

design doc: https://docs.google.com/document/d/12xd7Loeme0UfcjOICNYnvsGS32ggeH9HwjDFCi_HQDc/edit#heading=h.icz05x2z8mg

BUG=525660

[zhaobin, 2016-09-28 19:10:54]

zhaobin@chromium.org changed reviewers:
+ imcheng@chromium.org, mfoltz@chromium.org

[zhaobin, 2016-09-28 19:10:55]

(Prototyping to show the design idea, make break into smaller patches later)

An alternative implementation of

https://codereview.chromium.org/2343013002/
https://codereview.chromium.org/2355723004/

[zhaobin, 2016-09-28 19:12:06]

Description was changed from

==========
[Presentation API] (alternative) send message between controller and receiver
page

It is an alternative implementation of 1-UA messaging (substitue Issue
2343013002 and Issue 2355723004):

- add PresentationConnection class to presentation.mojom
- add RenderedPresentationConnection class which implements
PresentationConnectionProxy and PresentationConnection mojo interface

Work flow to connect controller RenderedPresentationConnection with receiver
RenderedPresentationConnection:

1. controller page starts a presentation, creates a
RenderedPresentationConnection object
2. controller RenderedPresentationConnection's mojo handler is passed to PSImpl
via PSImpl::SetPresentationConnection
3. PSImpl::SetPresentationConnection registers the mojo handler with
OffscreenPresentationManager
4. receiver page initializes and registers receiver_call to
OffscreenPresentationManager
5. receiver PSImpl::OnReceiverConnectionAvailable() is invoked for each
controller RenderedPresentationConnection's mojo handler belonging to the same
presentation
6. PSImpl::OnReceiverConnectionAvailable() calls
PDispatcher::OnReceiverConnectionAvailable() which creates a new receiver
RenderedPresentationConnection and connects it with controller
RenderedPresentationConnection's mojo handler

Messaging happens between controller RenderedPresentationConnection and receiver
RenderedPresentationConnection afterwards

BUG=525660
==========

to

==========
[Presentation API] (alternative) 1-UA: send message between controller and
receiver page

It is an alternative implementation of 1-UA messaging (substitue Issue
2343013002 and Issue 2355723004):

- add PresentationConnection class to presentation.mojom
- add RenderedPresentationConnection class which implements
PresentationConnectionProxy and PresentationConnection mojo interface

Work flow to connect controller RenderedPresentationConnection with receiver
RenderedPresentationConnection:

1. controller page starts a presentation, creates a
RenderedPresentationConnection object
2. controller RenderedPresentationConnection's mojo handler is passed to PSImpl
via PSImpl::SetPresentationConnection
3. PSImpl::SetPresentationConnection registers the mojo handler with
OffscreenPresentationManager
4. receiver page initializes and registers receiver_call to
OffscreenPresentationManager
5. receiver PSImpl::OnReceiverConnectionAvailable() is invoked for each
controller RenderedPresentationConnection's mojo handler belonging to the same
presentation
6. PSImpl::OnReceiverConnectionAvailable() calls
PDispatcher::OnReceiverConnectionAvailable() which creates a new receiver
RenderedPresentationConnection and connects it with controller
RenderedPresentationConnection's mojo handler

Messaging happens between controller RenderedPresentationConnection and receiver
RenderedPresentationConnection afterwards

BUG=525660
==========

[mark a. foltz, 2016-10-06 03:13:55]

I was able to review everything up through the offscreen_presentation_* files.

Most of my comments are minor and some of my questions are probably answered by
looking at the changes to the presentation service impl, so be patient :)

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/tab_capture/offscreen_tab.cc (right):

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/offscreen_tab.cc:105: << " Register
with ReceiverPresentationServiceDelegateImpl, id="
Should this go down at L125?

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/offscreen_tab.cc:123: // the
ReceiverPresentationServiceDelegateImpl.  http://crbug.com/513859
It looks like you can delete this TODO :)

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
File chrome/browser/media/router/offscreen_presentation_manager.cc (right):

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.cc:16: //
OffscreenPresentationManager implementation. ////////////////////////////////
I don't think the trailing slashes are needed.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.cc:30: // Create a
new presentation.
It's not clear to me why registering an offscreen controller would create an
offscreen presentation.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.cc:37: }
This logic is duplicated here and in RegisterOffscreenPresentationReceiver.

Would it make sense to have a separate function
RegisterOffscreenPresentation(id, url) that must be called by the controller's
PSImpl before registering a controller's PresentationConnection (in step 3)?

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.cc:53: if
(it->second->pending_controllers_.size() == 0 &&
Even if the last controller connection is closed, we don't terminate the
receiver, so I don't think we should erase the OffscreenPresentation entry here.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.cc:81: //
OffscreenPresentation implementation. ///////////////////////////////////////
Omit trailing slashes

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.cc:94: // Connect
controller PSImpl and receiver PSImpl.
It may be difficult for those unfamiliar with our code to know what "PSImpl"
means.
I would say "PresentationServiceImpl" or just "PresentationService" (the name of
the Mojo interface that is implemented).

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.cc:112: for (auto&
controller : pending_controllers_) {
This must be called only once per receiver, correct? If so,
DCHECK(!receiver_callback_)

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
File chrome/browser/media/router/offscreen_presentation_manager.h (right):

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:30: //          
context);
Is |context| the one that has the controller frames (i.e. started the
presentation) or the receiver WebContents?

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:34: //   void
OnReceiverConnectionAvailable(
Is this called when a connection is available to the receiver or to the
controller or both?

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:52: // To send
message from controller to receiver:
It looks like this is the same for the other direction.  Maybe just mention
that?

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:54: //   In
controll's PSImpl::SendSessionMessage() {
contoller's

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:60: //
OffscreenPresentationConnection object.
If the receiver navigates, the offscreen tab should be closed.  I think the
WebContentsObserver for the receiver WebContents could take care of
unregistering the receiver from the controllers' PSImpls?

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:62: // When the
receiver is no longer associated with an offscreen presentation, it
I believe there are five ways to terminate an offscreen presentation:

1. Presentation attempts to navigate
2. Presentation calls window.close()
3. Presentation calls connection.terminate()
4. Controller calls connection.terminate()
5. Error in tab capture or streaming, or user stops the media route in the
dialog

For #1, #2 the WebContentsObserver can call PSImpl::Reset
For #3 this can be done inside PSImpl?
For #4 this is done here (in the manager)?
For #5 the component will close the capture stream, which closes the WebContents
and then is handled like #1 or #2.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:73: // This class
is not thread safe. All functions must be invoked on the UI
Is this called by another thread or code outside of chrome/browser/media/router?

If so, it would be a good idea to have a ThreadChecker member and DCHECK() for
thread affinity in the public API.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:80: // Register
controller PSImpl to presentation with |presentation_id|,
Nit: Registers, Creates, Unregisters, etc. in function comments, here and below.

Also, should this be "Registers controller PresentationConnection for
|presentation_id|" since you are not passing a PSImpl?

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:86: const
std::string& presentation_url,
const GURL&

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:94: const
RenderFrameHostId& render_frame_id);
Should this take a content::PresentationConnectionPtr instead of a
render_frame_id?

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:96: // Register
receiver PSImpl to presentation with |presentation_id|.
This function doesn't have a PSImpl parameter.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:97: void
RegisterOffscreenPresentationReceiver(
OnOffscreenPresentationReceiverCreated(id, url, callback)?  Since I think this
is called when the receiver WebContents has been created?

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:99: const
std::string& presentation_url,
const GURL&

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:102: void
UnregisterOffscreenPresentationReceiver(
OnOffscreenPresentationReceiverTerminated(presentation_id)?  Since I believe
this will be called only when the receiver has been terminated.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:108: // Contains
callback to the receiver to inform it of new sessions
nit: s/sessions/connections/

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:115: const
std::string& presentation_url);
const GURL&

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:130: //
presentation_id for current presentation
Nit: comments should end in period, here and below.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:134: const
std::string presentation_url_;
const GURL

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:136: // proxy to
receiver PSImpl
I don't think this comment matches the code; the member is a callback not a
proxy?

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:142:
std::unordered_map<RenderFrameHostId,
Is it necessary to be a map, or would a vector<PendingControllerConnection>
work, where PendingControllerConnection is a struct holding RenderFrameHostId,
content::PresentationConnection be okay?

I guess using the keyed map makes it easier to check for duplicate registrations
by RenderFrameHostId.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
File
chrome/browser/media/router/offscreen_presentation_manager_factory_unittest.cc
(right):

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_factory_unittest.cc:36:
// Makes sure a OffscreenPresentationManager can be created from an OTR
Recently, there was some renaming recently to try to use "incognito" everywhere,
so:

s/OTR/incognito/

[zhaobin, 2016-10-07 01:07:04]

Hi Mark,

  Sorry for confusion. Comments in offscreen_presentation_manager.h is outdated.
Just updated.

  GURL change is preliminary here. Will merge after you land
https://codereview.chromium.org/2386633003/

Thanks,
Bin

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/tab_capture/offscreen_tab.cc (right):

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/offscreen_tab.cc:105: << " Register
with ReceiverPresentationServiceDelegateImpl, id="
On 2016/10/06 03:13:54, mark a. foltz wrote:
> Should this go down at L125?

Done.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/offscreen_tab.cc:123: // the
ReceiverPresentationServiceDelegateImpl.  http://crbug.com/513859
On 2016/10/06 03:13:54, mark a. foltz wrote:
> It looks like you can delete this TODO :)

Done.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
File chrome/browser/media/router/offscreen_presentation_manager.cc (right):

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.cc:16: //
OffscreenPresentationManager implementation. ////////////////////////////////
On 2016/10/06 03:13:54, mark a. foltz wrote:
> I don't think the trailing slashes are needed.

Done.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.cc:30: // Create a
new presentation.
On 2016/10/06 03:13:54, mark a. foltz wrote:
> It's not clear to me why registering an offscreen controller would create an
> offscreen presentation.

RegisterController() and RegisterReceiver() are async. We do not know which one
is invoked first. Need to create an entry in offscreen_presentations_ map if
presentation_id is not found.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.cc:37: }
On 2016/10/06 03:13:54, mark a. foltz wrote:
> This logic is duplicated here and in RegisterOffscreenPresentationReceiver.
> 
> Would it make sense to have a separate function
> RegisterOffscreenPresentation(id, url) that must be called by the controller's
> PSImpl before registering a controller's PresentationConnection (in step 3)?

Extract a RegisterOffscreenPresentation(id, url) function. RegisterController()
and RegisterReceiver() are async. Order is not guaranteed, so we need to call
RegisterOffscreenPresentation() in both controller and receiver. Would it be
better if we keep the creation logic in OffscreenPresentationManager?

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.cc:53: if
(it->second->pending_controllers_.size() == 0 &&
On 2016/10/06 03:13:54, mark a. foltz wrote:
> Even if the last controller connection is closed, we don't terminate the
> receiver, so I don't think we should erase the OffscreenPresentation entry
here.

We only remove presentation if no there no controller and no receiver (e.g. fail
to start offscreen tab).

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.cc:81: //
OffscreenPresentation implementation. ///////////////////////////////////////
On 2016/10/06 03:13:54, mark a. foltz wrote:
> Omit trailing slashes

Done.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.cc:94: // Connect
controller PSImpl and receiver PSImpl.
On 2016/10/06 03:13:54, mark a. foltz wrote:
> It may be difficult for those unfamiliar with our code to know what "PSImpl"
> means.
> I would say "PresentationServiceImpl" or just "PresentationService" (the name
of
> the Mojo interface that is implemented).

Outdated comments. Updated.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.cc:112: for (auto&
controller : pending_controllers_) {
On 2016/10/06 03:13:54, mark a. foltz wrote:
> This must be called only once per receiver, correct? If so,
> DCHECK(!receiver_callback_)

Done.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
File chrome/browser/media/router/offscreen_presentation_manager.h (right):

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:30: //          
context);
On 2016/10/06 03:13:55, mark a. foltz wrote:
> Is |context| the one that has the controller frames (i.e. started the
> presentation) or the receiver WebContents?

context is browserContext (profile).

offscreen_presentation_manager_(
          OffscreenPresentationManagerFactory::GetOrCreateForBrowserContext(
              Profile::FromBrowserContext(web_contents_->GetBrowserContext())
                  ->GetOriginalProfile()))

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:34: //   void
OnReceiverConnectionAvailable(
On 2016/10/06 03:13:55, mark a. foltz wrote:
> Is this called when a connection is available to the receiver or to the
> controller or both?

receiver PSImpl::onReceiverConnectionAvailable() is invoked
when registerReceiver() is called for the first time (during receiver PSImpl
initialization), to notify all pending controllers 
or 
when registerController() is called (controller starts a presentation) and there
is an existing receiver for that presentation.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:52: // To send
message from controller to receiver:
On 2016/10/06 03:13:54, mark a. foltz wrote:
> It looks like this is the same for the other direction.  Maybe just mention
> that?

Done.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:54: //   In
controll's PSImpl::SendSessionMessage() {
On 2016/10/06 03:13:55, mark a. foltz wrote:
> contoller's

Done.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:60: //
OffscreenPresentationConnection object.
On 2016/10/06 03:13:54, mark a. foltz wrote:
> If the receiver navigates, the offscreen tab should be closed.  I think the
> WebContentsObserver for the receiver WebContents could take care of
> unregistering the receiver from the controllers' PSImpls?

UnregisterReceiver() called in ReceiverPSDImpl::Reset() (for navigates or
offscreen tab closed).

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:62: // When the
receiver is no longer associated with an offscreen presentation, it
On 2016/10/06 03:13:55, mark a. foltz wrote:
> I believe there are five ways to terminate an offscreen presentation:
> 
> 1. Presentation attempts to navigate
> 2. Presentation calls window.close()
> 3. Presentation calls connection.terminate()
> 4. Controller calls connection.terminate()
> 5. Error in tab capture or streaming, or user stops the media route in the
> dialog
> 
> For #1, #2 the WebContentsObserver can call PSImpl::Reset
> For #3 this can be done inside PSImpl?
> For #4 this is done here (in the manager)?
> For #5 the component will close the capture stream, which closes the
WebContents
> and then is handled like #1 or #2.

(Not implemented yet) 
for #4, it will close the capture stream as in #5, and can be handled as #5.
for #3, we can make receiver's connection.terminate() call its controller's
connection.terminate() and handle it as #4.

need to fire onterminate() event correctly.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:73: // This class
is not thread safe. All functions must be invoked on the UI
On 2016/10/06 03:13:54, mark a. foltz wrote:
> Is this called by another thread or code outside of
chrome/browser/media/router?
> 
> If so, it would be a good idea to have a ThreadChecker member and DCHECK() for
> thread affinity in the public API.

Done.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:80: // Register
controller PSImpl to presentation with |presentation_id|,
On 2016/10/06 03:13:55, mark a. foltz wrote:
> Nit: Registers, Creates, Unregisters, etc. in function comments, here and
below.
> 
> Also, should this be "Registers controller PresentationConnection for
> |presentation_id|" since you are not passing a PSImpl?

Done.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:86: const
std::string& presentation_url,
On 2016/10/06 03:13:54, mark a. foltz wrote:
> const GURL&

Merge GURL changes after https://codereview.chromium.org/2386633003/ lands

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:94: const
RenderFrameHostId& render_frame_id);
On 2016/10/06 03:13:55, mark a. foltz wrote:
> Should this take a content::PresentationConnectionPtr instead of a
> render_frame_id?

content::PresentationConnectionPtr disallows copy and copy constructor (works as
unique ptr). It is a little complicated to pass in
content::PresentatioNConnectionPtr in unregisterController() as parameter.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:96: // Register
receiver PSImpl to presentation with |presentation_id|.
On 2016/10/06 03:13:54, mark a. foltz wrote:
> This function doesn't have a PSImpl parameter.

Done.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:97: void
RegisterOffscreenPresentationReceiver(
On 2016/10/06 03:13:55, mark a. foltz wrote:
> OnOffscreenPresentationReceiverCreated(id, url, callback)?  Since I think this
> is called when the receiver WebContents has been created?

Done.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:99: const
std::string& presentation_url,
On 2016/10/06 03:13:54, mark a. foltz wrote:
> const GURL&

Done.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:102: void
UnregisterOffscreenPresentationReceiver(
On 2016/10/06 03:13:54, mark a. foltz wrote:
> OnOffscreenPresentationReceiverTerminated(presentation_id)?  Since I believe
> this will be called only when the receiver has been terminated.

Done.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:102: void
UnregisterOffscreenPresentationReceiver(
On 2016/10/06 03:13:54, mark a. foltz wrote:
> OnOffscreenPresentationReceiverTerminated(presentation_id)?  Since I believe
> this will be called only when the receiver has been terminated.

Done.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:108: // Contains
callback to the receiver to inform it of new sessions
On 2016/10/06 03:13:55, mark a. foltz wrote:
> nit: s/sessions/connections/

Done.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:115: const
std::string& presentation_url);
On 2016/10/06 03:13:55, mark a. foltz wrote:
> const GURL&

Done.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:130: //
presentation_id for current presentation
On 2016/10/06 03:13:55, mark a. foltz wrote:
> Nit: comments should end in period, here and below.

Done.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:134: const
std::string presentation_url_;
On 2016/10/06 03:13:55, mark a. foltz wrote:
> const GURL

Done.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:136: // proxy to
receiver PSImpl
On 2016/10/06 03:13:54, mark a. foltz wrote:
> I don't think this comment matches the code; the member is a callback not a
> proxy?

Done.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:142:
std::unordered_map<RenderFrameHostId,
On 2016/10/06 03:13:55, mark a. foltz wrote:
> Is it necessary to be a map, or would a vector<PendingControllerConnection>
> work, where PendingControllerConnection is a struct holding RenderFrameHostId,
> content::PresentationConnection be okay?
> 
> I guess using the keyed map makes it easier to check for duplicate
registrations
> by RenderFrameHostId.

It needs to be a map. We cannot pass content::PresentationConnectionPtr (mojo
ptr acts as unique_ptr) to UnregisterController(). So we need to map to
UnregisterController.

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
File
chrome/browser/media/router/offscreen_presentation_manager_factory_unittest.cc
(right):

https://codereview.chromium.org/2379703002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_factory_unittest.cc:36:
// Makes sure a OffscreenPresentationManager can be created from an OTR
On 2016/10/06 03:13:55, mark a. foltz wrote:
> Recently, there was some renaming recently to try to use "incognito"
everywhere,
> so:
> 
> s/OTR/incognito/ 

Done.

[mark a. foltz, 2016-10-08 00:33:42]

Looking really good so far, mostly minor comments on style and naming.  Got up
to presentation_dispatcher.cc.    Once I finish we can take a step back and
discuss any remaining design issues.

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/offscreen_presentation_manager.cc (right):

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.cc:19:
thread_checker_.DetachFromThread();
I don't think this is needed. The thread_checker_ constructor binds to the
thread that calls it and that should be the thread checked in this class.

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.cc:22:
OffscreenPresentationManager::~OffscreenPresentationManager() {}
Could there be any still-registered OffscreenPresentation instances here?  If so
don't forget to delete the map values, or (better) store them as unique_ptr<>.

Otherwise, DCHECK() that the map is empty.

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/offscreen_presentation_manager.h (right):

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:20: namespace
media_router {
From a layering point of view I wonder if this belongs in chrome/browser or
content/browser.   It doesn't actually depend on the media router, which is a
good thing.

However content/browser doesn't seem to have the concept of profile-keyed
services, and we need an object that is tied to the browser context and not a
specific frame/renderer client.  I looked briefly for a similar use case and
didn't find anything obvious.  Maybe chrome/browser is the right place, but in a
parallel folder to the media_router?

Derek, do you have any thoughts?

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:36: //          
Profile::FromBrowserContext(web_contents_->GetBrowserContext())
Where does |web_contents_| come from here?  Is it from the controller's page?

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:37: //             
    ->GetOriginalProfile());
Nit: the factory should take care of returning the manager for any browser
context, so this would be

OffscreenPresentationManagerFactory::GetOrCreateForBrowserContext(
    web_contents_->GetBrowserContext())

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:39: //      
base::Bind(&PresentationServiceImpl::OnReceiverConnectionAvailable));
Should this be bound to the instance of the PSImpl from the receiver's page?
How does the controller get access to this instance?

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:40: //   ...
I would add a comment here like, 

// Invoked on receiver's PresentationServiceImpl when controller connection is
established

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:46: //     
PresenatationConnectionPtr]
Typo

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:124: explicit
OffscreenPresentation(const std::string& presentation_id,
explicit is not needed for two-arg constructors

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/presentation_service_delegate_impl.cc (right):

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:673:
OffscreenPresentationManagerFactory::GetOrCreateForBrowserContext(
Maybe there could be a OffscreenPresentationManagerFactory method that takes a
WebContents, if this is a common use.

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:925:
receiver_available_callback) {}
This is not implemented for the controller's PSImpl because only the receiver's
presentation service needs this callback, correct?

I wonder if we should have PresentationServiceDelegateImplBase,
ControllerPresentationServiceDelegateImpl, and
ReceiverPresentationServiceDelegateImpl, since they have different APIs?

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc
(right):

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc:1: //
Copyright 2015 The Chromium Authors. All rights reserved.
Update copyright

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc:40:
void ReceiverPresentationServiceDelegateImpl::AddObserver(
IMO it would be worth having a base class for the two delegate implementations
so the observer management API isn't copied into two places.

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc:114:
NOTIMPLEMENTED();
This should be implemented eventually right?

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc:121:
NOTIMPLEMENTED();
Should be implemented in the future?

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc:168:
std::string presentation_url = web_contents_->GetURL().spec();
Can you just pass the URL directly?

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/receiver_presentation_service_delegate_impl.h
(right):

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/receiver_presentation_service_delegate_impl.h:1: //
Copyright 2015 The Chromium Authors. All rights reserved.
Update copyright

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/receiver_presentation_service_delegate_impl.h:37: //
rejected.
Nice :)

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/receiver_presentation_service_delegate_impl.h:59:
bool AddScreenAvailabilityListener(
None of these (through JoinSession) are really required for the receiver page,
at least until we support pages that are both controllers and receivers.  It may
be worth considering splitting the presentation service into controller and
receiver interfaces.  I think we considered that a while ago but decided to keep
it a single interface. Derek, do you remember the reasoning there?

https://codereview.chromium.org/2379703002/diff/80001/content/browser/present...
File content/browser/presentation/presentation_service_impl.cc (right):

https://codereview.chromium.org/2379703002/diff/80001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:147: DVLOG(2) <<
"~PresentationServiceImpl: " << render_process_id_ << ", "
__FUNCTION__ as is done elsewhere in this patch?

https://codereview.chromium.org/2379703002/diff/80001/content/browser/present...
File content/browser/presentation/presentation_service_impl_unittest.cc (right):

https://codereview.chromium.org/2379703002/diff/80001/content/browser/present...
content/browser/presentation/presentation_service_impl_unittest.cc:149:
MOCK_METHOD1(RegisterReceiverAvailableCallback,
Please add unit tests for the new APIs added in PresentationServiceImpl

https://codereview.chromium.org/2379703002/diff/80001/content/public/browser/...
File content/public/browser/presentation_service_delegate.h (right):

https://codereview.chromium.org/2379703002/diff/80001/content/public/browser/...
content/public/browser/presentation_service_delegate.h:214: // In
receiverPSDImpl, register receiver_available_callback with
The ReceiverPDSImpl is part of chrome/browser; so usually we just say "the
embedder" instead of mentioning specific implementation details of Chome.

This might be,

"Registers a callback from the embedder when an offscreeen presentation has been
successfully started."

https://codereview.chromium.org/2379703002/diff/80001/content/public/browser/...
content/public/browser/presentation_service_delegate.h:229: //
offscreen_presentation_manager->RegisterOffscreenPresentationController().
// Connect |connection| owned by the controlling frame to the offscreen
presentation represented by |session|.

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
File content/renderer/presentation/presentation_connection_client.h (right):

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
content/renderer/presentation/presentation_connection_client.h:26: explicit
PresentationConnectionClient(
No need for explicit on 2-argument ctor

[mark a. foltz, 2016-10-10 18:28:53]

Okay, remaining comments :)  One design question that I couldn't quite figure
out: is the connection proxy used just for connections to offscreen
presentations, or all presentation connections?

Maybe we could grab 30 minutes after lunch to discuss the design again before
final reviews.

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:398:
RendererPresentationConnection* controller_connection_proxy =
How do you know this connection is for an offscreen presentation?

I think you'll need to add something in |session_info| to know whether to pass
the connection to the OffscreenPresentationManager.

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
File content/renderer/presentation/renderer_presentation_connection.cc (right):

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
content/renderer/presentation/renderer_presentation_connection.cc:64: //
TODO(mfoltz): Do we need to DCHECK(messages[i]->message)?
I'm not sure this TODO makes sense here; we could DCHECK here, but the browser
should be doing validation before passing to the renderer.  Can you check
PSImpl?

Hopefully all the validations will be automatic when crbug.com/632623 is finally
fixed and we have typemaps for all messages passed between the renderer and the
browser.

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
content/renderer/presentation/renderer_presentation_connection.cc:71: //
TODO(mfoltz): Do we need to DCHECK(messages[i]->data)?
See comment above.

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
File content/renderer/presentation/renderer_presentation_connection.h (right):

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
content/renderer/presentation/renderer_presentation_connection.h:15: 
Can you add a class-level comment describing what this is for?

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
content/renderer/presentation/renderer_presentation_connection.h:16: class
RendererPresentationConnection
There is already a PresentationConnection object in Blink and Mojo used in the
renderer, so it's a little ambiguous to have a third object named
PresentationConnection here.

Maybe PresentationConnectionProxy to match how it is used?

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
content/renderer/presentation/renderer_presentation_connection.h:26: void
SetConnection(blink::WebPresentationConnection* connection) override;
Maybe SetSourceConnection?

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
content/renderer/presentation/renderer_presentation_connection.h:33: void
OnSessionMessageReceived(
OnConnectionMessageReceived

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
content/renderer/presentation/renderer_presentation_connection.h:39:
blink::WebPresentationConnection* connection_;
source_connection_

https://codereview.chromium.org/2379703002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp
(right):

https://codereview.chromium.org/2379703002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:149:
WebPresentationConnectionProxy* proxy)
For a 2-UA presentation (e.g. a Cast app) why is a proxy needed?  It seems like
the connection will just pass the the data onto the Media Router in PSImpl.  Or
is the proxy to the MR?

https://codereview.chromium.org/2379703002/diff/80001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h
(right):

https://codereview.chromium.org/2379703002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h:14:
// The implementation the embedder has to provide for the Presentation API to
Or just,

Implemented by the embedder for a PresentationConnection.

https://codereview.chromium.org/2379703002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/presentation/presentation.mojom
(right):

https://codereview.chromium.org/2379703002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:56:
// Called to set the PresentationConnection interface that sends
Called to set the PresentationConnection that is the destination for messages
sent and the source of messages received by this connection.

https://codereview.chromium.org/2379703002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:60:
// Called when connection.send() is invoked at the other end of the
Called when a message is sent by the target connection.

https://codereview.chromium.org/2379703002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:62:
OnSessionMessageReceived(SessionMessage message);
OnConnectionMessageReceived (we'll rename the parameter later)

https://codereview.chromium.org/2379703002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:158:
// See PresentationService::ListeningForReceiverPageRendered.
Can you describe the meaning of this here?  e.g.

Called on a presentation receiver when the first presentation connection is
available from the controlling page.

[imcheng, 2016-10-12 02:22:55]

I was thinking if it is possible to use the PresentationConnection mojo
interface exclusively to send messages, for both 1-ua and 2-ua presentations. I
am still convinced that we can wire things up in the browser process, which may
make things a bit cleaner. The idea which I have taken from the media mojo code,
is to create a InterfaceRequest from a Ptr object using mojo::GetProxy, and
passing the InterfaceRequest back to the controller PresentationDispatcher to be
bound to an implementation. The Ptr is passed back to the receiver
PresentationDispatcher, which when invoked would go directly to the controller
renderer prcess.

Some examples from media code:

- mojom definition for passing InterfaceRequest objects:
https://cs.chromium.org/chromium/src/media/mojo/interfaces/service_factory.mo...
- binding the InterfaceRequest object to an implementation:
https://cs.chromium.org/chromium/src/media/mojo/services/service_factory_impl...
- example usage in tests:
https://cs.chromium.org/chromium/src/media/mojo/services/media_mojo_unittest....

I wonder if we can do the following:

In PSDImpl::OnRouteCreated, if we know it is a 1-ua presentation:
- Create 2 Ptrs, Ptr_r and Ptr_c, and create 2 Proxies from them: Proxy_r and
Proxy_c, i.e., calling Ptr_c will invoke the impl bound to Proxy_c
- Pass Proxy_c back to controller PSImpl -> PDispatcher, which will then bind it
to RendererPresentationConnection
- Pass Ptr_r back to controller PSImpl -> PDispatcher, which will set it on
RendererPresentationConnection
- Pass Proxy_r back to receiver PSImpl -> PDispatcher, which will then bind it
to RendererPresentationConnection
- Pass Ptr_c back to receiver PSImpl -> PDispatcher, which will set it on
RendererPresentationConnection

If it is not a 1-ua:
- Business as usual to PSImpl
- PSImpl will create 2 Ptrs, Ptr_render Ptr_browser and the 2 Proxies
Proxy_render Proxy_browser
- PSImpl keeps Ptr_render and Proxy_browser:
-- Ptr_render is used to send messages to the page
-- Proxy_browser is bound to PSImpl to receive messages from the page
- Ptr_browser and Proxy_render is sent to PDispatcher
-- Create RendererPresentationConnection, binds Proxy_render to it
-- Sets Ptr_browser, which is used to send messages to MR
-- Instead of passing it to PresentationConnectionClient, the PDispatcher should
keep it and use it to send messages (via the message queue) and receive messages

Let me know if that makes sense.

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
File content/renderer/presentation/presentation_connection_client.h (right):

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
content/renderer/presentation/presentation_connection_client.h:26: explicit
PresentationConnectionClient(
Could you also please document what the different constructors are for?

Specifically, could you clarify whether PresentationConnectionClient is supposed
to take ownership of |proxy| (until getOrCreateProxy is called). What happens if
getOrCreateProxy is never called, or more than once?

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:402:
callback->onSuccess(base::MakeUnique<PresentationConnectionClient>(
It looks like we don't need the 2-argument PresentationConnectionClient
constructor. What we are doing here (and in OnReceiverConnectionAvailable) is
create a RendererPresentationConnection, perform some operation on it, then pass
its ownership to PresentationConnectionClient. Another way is:
- create PresentationConnectionClient
- ask it to instantiate a RendererPresentationConnection and return a raw
pointer (non-ownership) to it (createProxy)
- perform operation on the RendererPresentationConnection.

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:408:
session_info.Clone(), controller_connection_proxy->Bind());
|controller_connection_proxy| might have already been deleted at this point:

- PresentationConnectionClient, which is passed into the callback, takes
ownership of it
- The proxy object might be released before the callback returns, see:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/presen...
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/presen...

https://codereview.chromium.org/2379703002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp
(right):

https://codereview.chromium.org/2379703002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:315:
m_proxy->SendString(message->text);
Same question as mark: looks like we are sending message to both the
WebPresentationClient (non 1-ua) and the WebPresentationConnectionProxy (1-ua).
It might be even better if we are only using m_proxy for 1-ua, but see top level
comment.

https://codereview.chromium.org/2379703002/diff/80001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnectionClient.h
(right):

https://codereview.chromium.org/2379703002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnectionClient.h:31:
virtual WebPresentationConnectionProxy* getOrCreateProxy() = 0;
Could you please document this method?

Also, it looks like the caller of this method takes ownership of the returned
object - would it make sense for this to return std::unique_ptr<> ?

[zhaobin, 2016-10-12 02:27:34]

Will address Derek's comments in next iteration.

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/offscreen_presentation_manager.cc (right):

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.cc:19:
thread_checker_.DetachFromThread();
On 2016/10/08 00:33:41, mark a. foltz wrote:
> I don't think this is needed. The thread_checker_ constructor binds to the
> thread that calls it and that should be the thread checked in this class.

Done.

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.cc:22:
OffscreenPresentationManager::~OffscreenPresentationManager() {}
On 2016/10/08 00:33:41, mark a. foltz wrote:
> Could there be any still-registered OffscreenPresentation instances here?  If
so
> don't forget to delete the map values, or (better) store them as unique_ptr<>.
> 
> Otherwise, DCHECK() that the map is empty.

Done.

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/offscreen_presentation_manager.h (right):

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:20: namespace
media_router {
On 2016/10/08 00:33:41, mark a. foltz wrote:
> From a layering point of view I wonder if this belongs in chrome/browser or
> content/browser.   It doesn't actually depend on the media router, which is a
> good thing.
> 
> However content/browser doesn't seem to have the concept of profile-keyed
> services, and we need an object that is tied to the browser context and not a
> specific frame/renderer client.  I looked briefly for a similar use case and
> didn't find anything obvious.  Maybe chrome/browser is the right place, but in
a
> parallel folder to the media_router?
> 
> Derek, do you have any thoughts?

Leave it in media/router folder for now. We will move it later when necessary.

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:36: //          
Profile::FromBrowserContext(web_contents_->GetBrowserContext())
On 2016/10/08 00:33:41, mark a. foltz wrote:
> Where does |web_contents_| come from here?  Is it from the controller's page?

For controller, it is from controller's page;
For receiver, it is from offscreen tab, but calls GetOriginalProfile() to get
the same profile as controller.

(controller) In PresentationServiceDelegateImpl's contructor:

offscreen_presentation_manager_(
          OffscreenPresentationManagerFactory::GetOrCreateForBrowserContext(
              web_contents_->GetBrowserContext()))


(receiver) In ReceiverPresentationServiceDelegateImpl's constructor:

offscreen_presentation_manager_(
          OffscreenPresentationManagerFactory::GetOrCreateForBrowserContext(
              Profile::FromBrowserContext(web_contents_->GetBrowserContext())
                  ->GetOriginalProfile())) {

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:37: //             
    ->GetOriginalProfile());
On 2016/10/08 00:33:41, mark a. foltz wrote:
> Nit: the factory should take care of returning the manager for any browser
> context, so this would be
> 
> OffscreenPresentationManagerFactory::GetOrCreateForBrowserContext(
>     web_contents_->GetBrowserContext())

Done.

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:39: //      
base::Bind(&PresentationServiceImpl::OnReceiverConnectionAvailable));
On 2016/10/08 00:33:41, mark a. foltz wrote:
> Should this be bound to the instance of the PSImpl from the receiver's page?
> How does the controller get access to this instance?

Yes.

The object is bind to the function:

delegate_->RegisterReceiverAvailableCallback(
       base::Bind(&PresentationServiceImpl::OnReceiverConnectionAvailable,
                  weak_factory_.GetWeakPtr()));

And callback in invoked in OffscreenPresentationManager, which have access to
controller PresentationConnectionPtr and receiver callback:

OffscreenPresentationManager::OffscreenPresentation::RegisterReceiver()
OffscreenPresentationManager::OffscreenPresentation::RegisterController()

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:40: //   ...
On 2016/10/08 00:33:41, mark a. foltz wrote:
> I would add a comment here like, 
> 
> // Invoked on receiver's PresentationServiceImpl when controller connection is
> established

Done.

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:46: //     
PresenatationConnectionPtr]
On 2016/10/08 00:33:41, mark a. foltz wrote:
> Typo

Done.

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:124: explicit
OffscreenPresentation(const std::string& presentation_id,
On 2016/10/08 00:33:41, mark a. foltz wrote:
> explicit is not needed for two-arg constructors

Done.

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/presentation_service_delegate_impl.cc (right):

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:673:
OffscreenPresentationManagerFactory::GetOrCreateForBrowserContext(
On 2016/10/08 00:33:41, mark a. foltz wrote:
> Maybe there could be a OffscreenPresentationManagerFactory method that takes a
> WebContents, if this is a common use.

Done.

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:925:
receiver_available_callback) {}
On 2016/10/08 00:33:41, mark a. foltz wrote:
> This is not implemented for the controller's PSImpl because only the
receiver's
> presentation service needs this callback, correct?
> 
> I wonder if we should have PresentationServiceDelegateImplBase,
> ControllerPresentationServiceDelegateImpl, and
> ReceiverPresentationServiceDelegateImpl, since they have different APIs?

Done.

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc
(right):

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc:1: //
Copyright 2015 The Chromium Authors. All rights reserved.
On 2016/10/08 00:33:41, mark a. foltz wrote:
> Update copyright

Done.

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc:40:
void ReceiverPresentationServiceDelegateImpl::AddObserver(
On 2016/10/08 00:33:42, mark a. foltz wrote:
> IMO it would be worth having a base class for the two delegate implementations
> so the observer management API isn't copied into two places.

Done.

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc:114:
NOTIMPLEMENTED();
On 2016/10/08 00:33:41, mark a. foltz wrote:
> This should be implemented eventually right?

Maybe not if close/terminate can be handled at render process. Receiver does not
need to talk to Media Router directly, it may delegate close/terminate to its
corresponding controller?

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc:121:
NOTIMPLEMENTED();
On 2016/10/08 00:33:41, mark a. foltz wrote:
> Should be implemented in the future?

Same as above.

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc:168:
std::string presentation_url = web_contents_->GetURL().spec();
On 2016/10/08 00:33:41, mark a. foltz wrote:
> Can you just pass the URL directly?

Done.

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/receiver_presentation_service_delegate_impl.h
(right):

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/receiver_presentation_service_delegate_impl.h:1: //
Copyright 2015 The Chromium Authors. All rights reserved.
On 2016/10/08 00:33:42, mark a. foltz wrote:
> Update copyright

Done.

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/receiver_presentation_service_delegate_impl.h:37: //
rejected.
On 2016/10/08 00:33:42, mark a. foltz wrote:
> Nice :)

Derek writes this :)

https://codereview.chromium.org/2379703002/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/receiver_presentation_service_delegate_impl.h:59:
bool AddScreenAvailabilityListener(
On 2016/10/08 00:33:42, mark a. foltz wrote:
> None of these (through JoinSession) are really required for the receiver page,
> at least until we support pages that are both controllers and receivers.  It
may
> be worth considering splitting the presentation service into controller and
> receiver interfaces.  I think we considered that a while ago but decided to
keep
> it a single interface. Derek, do you remember the reasoning there?

Will not split presentation service for now. May do that later as a seperate
project.

https://codereview.chromium.org/2379703002/diff/80001/content/browser/present...
File content/browser/presentation/presentation_service_impl.cc (right):

https://codereview.chromium.org/2379703002/diff/80001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:147: DVLOG(2) <<
"~PresentationServiceImpl: " << render_process_id_ << ", "
On 2016/10/08 00:33:42, mark a. foltz wrote:
> __FUNCTION__ as is done elsewhere in this patch?

Done.

https://codereview.chromium.org/2379703002/diff/80001/content/browser/present...
File content/browser/presentation/presentation_service_impl_unittest.cc (right):

https://codereview.chromium.org/2379703002/diff/80001/content/browser/present...
content/browser/presentation/presentation_service_impl_unittest.cc:149:
MOCK_METHOD1(RegisterReceiverAvailableCallback,
On 2016/10/08 00:33:42, mark a. foltz wrote:
> Please add unit tests for the new APIs added in PresentationServiceImpl

Done.

https://codereview.chromium.org/2379703002/diff/80001/content/public/browser/...
File content/public/browser/presentation_service_delegate.h (right):

https://codereview.chromium.org/2379703002/diff/80001/content/public/browser/...
content/public/browser/presentation_service_delegate.h:214: // In
receiverPSDImpl, register receiver_available_callback with
On 2016/10/08 00:33:42, mark a. foltz wrote:
> The ReceiverPDSImpl is part of chrome/browser; so usually we just say "the
> embedder" instead of mentioning specific implementation details of Chome.
> 
> This might be,
> 
> "Registers a callback from the embedder when an offscreeen presentation has
been
> successfully started."

Done.

https://codereview.chromium.org/2379703002/diff/80001/content/public/browser/...
content/public/browser/presentation_service_delegate.h:229: //
offscreen_presentation_manager->RegisterOffscreenPresentationController().
On 2016/10/08 00:33:42, mark a. foltz wrote:
> // Connect |connection| owned by the controlling frame to the offscreen
> presentation represented by |session|.

Done.

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
File content/renderer/presentation/presentation_connection_client.h (right):

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
content/renderer/presentation/presentation_connection_client.h:26: explicit
PresentationConnectionClient(
On 2016/10/08 00:33:42, mark a. foltz wrote:
> No need for explicit on 2-argument ctor

Done.

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:398:
RendererPresentationConnection* controller_connection_proxy =
On 2016/10/10 18:28:52, mark a. foltz wrote:
> How do you know this connection is for an offscreen presentation?
> 
> I think you'll need to add something in |session_info| to know whether to pass
> the connection to the OffscreenPresentationManager.

Done.

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
File content/renderer/presentation/renderer_presentation_connection.cc (right):

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
content/renderer/presentation/renderer_presentation_connection.cc:64: //
TODO(mfoltz): Do we need to DCHECK(messages[i]->message)?
On 2016/10/10 18:28:52, mark a. foltz wrote:
> I'm not sure this TODO makes sense here; we could DCHECK here, but the browser
> should be doing validation before passing to the renderer.  Can you check
> PSImpl?
> 
> Hopefully all the validations will be automatic when crbug.com/632623 is
finally
> fixed and we have typemaps for all messages passed between the renderer and
the
> browser.

Done.

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
content/renderer/presentation/renderer_presentation_connection.cc:71: //
TODO(mfoltz): Do we need to DCHECK(messages[i]->data)?
On 2016/10/10 18:28:52, mark a. foltz wrote:
> See comment above.

Done.

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
File content/renderer/presentation/renderer_presentation_connection.h (right):

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
content/renderer/presentation/renderer_presentation_connection.h:15: 
On 2016/10/10 18:28:52, mark a. foltz wrote:
> Can you add a class-level comment describing what this is for?

Done.

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
content/renderer/presentation/renderer_presentation_connection.h:16: class
RendererPresentationConnection
On 2016/10/10 18:28:52, mark a. foltz wrote:
> There is already a PresentationConnection object in Blink and Mojo used in the
> renderer, so it's a little ambiguous to have a third object named
> PresentationConnection here.
> 
> Maybe PresentationConnectionProxy to match how it is used?

Done.

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
content/renderer/presentation/renderer_presentation_connection.h:26: void
SetConnection(blink::WebPresentationConnection* connection) override;
On 2016/10/10 18:28:52, mark a. foltz wrote:
> Maybe SetSourceConnection?

Done.

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
content/renderer/presentation/renderer_presentation_connection.h:33: void
OnSessionMessageReceived(
On 2016/10/10 18:28:52, mark a. foltz wrote:
> OnConnectionMessageReceived

Done.

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
content/renderer/presentation/renderer_presentation_connection.h:39:
blink::WebPresentationConnection* connection_;
On 2016/10/10 18:28:52, mark a. foltz wrote:
> source_connection_

Done.

https://codereview.chromium.org/2379703002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp
(right):

https://codereview.chromium.org/2379703002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:149:
WebPresentationConnectionProxy* proxy)
On 2016/10/10 18:28:52, mark a. foltz wrote:
> For a 2-UA presentation (e.g. a Cast app) why is a proxy needed?  It seems
like
> the connection will just pass the the data onto the Media Router in PSImpl. 
Or
> is the proxy to the MR?

Done.

https://codereview.chromium.org/2379703002/diff/80001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h
(right):

https://codereview.chromium.org/2379703002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h:14:
// The implementation the embedder has to provide for the Presentation API to
On 2016/10/10 18:28:52, mark a. foltz wrote:
> Or just,
> 
> Implemented by the embedder for a PresentationConnection.

Done.

https://codereview.chromium.org/2379703002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/presentation/presentation.mojom
(right):

https://codereview.chromium.org/2379703002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:56:
// Called to set the PresentationConnection interface that sends
On 2016/10/10 18:28:52, mark a. foltz wrote:
> Called to set the PresentationConnection that is the destination for messages
> sent and the source of messages received by this connection.

Done.

https://codereview.chromium.org/2379703002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:60:
// Called when connection.send() is invoked at the other end of the
On 2016/10/10 18:28:52, mark a. foltz wrote:
> Called when a message is sent by the target connection.

Done.

https://codereview.chromium.org/2379703002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:62:
OnSessionMessageReceived(SessionMessage message);
On 2016/10/10 18:28:52, mark a. foltz wrote:
> OnConnectionMessageReceived (we'll rename the parameter later)

Done.

https://codereview.chromium.org/2379703002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:158:
// See PresentationService::ListeningForReceiverPageRendered.
On 2016/10/10 18:28:53, mark a. foltz wrote:
> Can you describe the meaning of this here?  e.g.
> 
> Called on a presentation receiver when the first presentation connection is
> available from the controlling page.

Done.

[zhaobin, 2016-10-13 02:33:52]

Resolving code review comments from Derek in Patch Set 5
(https://codereview.chromium.org/2379703002/#ps80001)

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
File content/renderer/presentation/presentation_connection_client.h (right):

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
content/renderer/presentation/presentation_connection_client.h:26: explicit
PresentationConnectionClient(
On 2016/10/12 02:22:54, imcheng wrote:
> Could you also please document what the different constructors are for?
> 
> Specifically, could you clarify whether PresentationConnectionClient is
supposed
> to take ownership of |proxy| (until getOrCreateProxy is called). What happens
if
> getOrCreateProxy is never called, or more than once?

Done.

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:402:
callback->onSuccess(base::MakeUnique<PresentationConnectionClient>(
On 2016/10/12 02:22:55, imcheng wrote:
> It looks like we don't need the 2-argument PresentationConnectionClient
> constructor. What we are doing here (and in OnReceiverConnectionAvailable) is
> create a RendererPresentationConnection, perform some operation on it, then
pass
> its ownership to PresentationConnectionClient. Another way is:
> - create PresentationConnectionClient
> - ask it to instantiate a RendererPresentationConnection and return a raw
> pointer (non-ownership) to it (createProxy)
> - perform operation on the RendererPresentationConnection.

Did some changes in new patch so we use 2-argument constructor for 1-UA mode;
and original constructors for other connections.

https://codereview.chromium.org/2379703002/diff/80001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:408:
session_info.Clone(), controller_connection_proxy->Bind());
On 2016/10/12 02:22:54, imcheng wrote:
> |controller_connection_proxy| might have already been deleted at this point:
> 
> - PresentationConnectionClient, which is passed into the callback, takes
> ownership of it
> - The proxy object might be released before the callback returns, see:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/presen...
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/presen...

Done.

https://codereview.chromium.org/2379703002/diff/80001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnectionClient.h
(right):

https://codereview.chromium.org/2379703002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnectionClient.h:31:
virtual WebPresentationConnectionProxy* getOrCreateProxy() = 0;
On 2016/10/12 02:22:55, imcheng wrote:
> Could you please document this method?
> 
> Also, it looks like the caller of this method takes ownership of the returned
> object - would it make sense for this to return std::unique_ptr<> ?

Added documents in presentation_connection_client.h

[zhaobin, 2016-10-14 17:53:06]

Patchset #7 (id:120001) has been deleted

[mark a. foltz, 2016-10-21 00:53:04]

Okay, reviewed the code in content/ and Blink.  Mostly minor comments and naming
suggestions.  Would like to do a final design oriented review before looping in
other OWNERS.

I think it would be a good idea to write up a design doc (I think you started
one already) and include a link in the CL description.

https://codereview.chromium.org/2379703002/diff/160001/chrome/browser/media/r...
File chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc
(right):

https://codereview.chromium.org/2379703002/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:41:
class MockDelegateObserver
Please add a unit test for RegisterOffscreenPresentationConnection

https://codereview.chromium.org/2379703002/diff/160001/content/public/browser...
File content/public/browser/presentation_service_delegate.h (right):

https://codereview.chromium.org/2379703002/diff/160001/content/public/browser...
content/public/browser/presentation_service_delegate.h:222: virtual void
RegisterOffscreenPresentationConnection(
ConnectToOffscreenPresentation?

https://codereview.chromium.org/2379703002/diff/160001/content/public/browser...
content/public/browser/presentation_service_delegate.h:230: // forwarded from
receiver PresentationServiceImpl.
handle PresentationService calls from a presentation receiver.

https://codereview.chromium.org/2379703002/diff/160001/content/public/browser...
content/public/browser/presentation_service_delegate.h:236: // successfully
started.
Can you rewrap this comment?

https://codereview.chromium.org/2379703002/diff/160001/content/public/browser...
content/public/browser/presentation_service_delegate.h:239: virtual void
RegisterReceiverAvailableCallback(
RegisterReceiverConnectionAvailableCallback

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_client.h (right):

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
content/renderer/presentation/presentation_connection_client.h:26: //
|session_info|: contains presentation id and url info
nit: "Contains presentation ID and URL info."

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
content/renderer/presentation/presentation_connection_client.h:27: // |proxy|:
proxy to blink PresentationConnection object in current render
nit: Proxy

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
content/renderer/presentation/presentation_connection_client.h:39: //
(PresentationConnection).
Rewrap comments at 80 columns.

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
content/renderer/presentation/presentation_connection_client.h:40:
std::unique_ptr<blink::WebPresentationConnectionProxy> getProxy() override;
Can this be takeProxy() to denote change of ownership?

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_proxy.cc (right):

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:23: return;
This feels like a bug; the connection should not be sending messages before it's
connected.
Can you DCHECK() here?

Same comment applies below.

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:25:
blink::mojom::SessionMessagePtr session_message =
Can you use the existing type converter for this?

Same comment applies below.

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:48:
source_connection_ = connection;
DCHECK(!source_connection_) before setting.

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:53:
target_connection_ = std::move(connection);
DCHECK(!target_connection_) before setting.

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_proxy.h (right):

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:16: // This class
connects PrensentationConnection object living in
typo in PresentationConnection

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:17: //
controller(receiver) render process
... one frame with a PresentationConnection owned by a different frame.

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:19: // process.
It's up to the embedder to decide whether the controller and receiver live in
different processes or not, so it would be more accurate to talk about
connections owned by frames instead of processes.

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:29: // To send
message from controller page to receiver page:
controlling frame to receiver frame:

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:43: // Instance of
this class is only created for 1-UA mode.
s/1-UA mode/offscreen presentations/

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.cc:386: if
(session_info->is_offscreen_presentation) {
Can you factor out the common code to create and set the controller connection
proxy?

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.cc:395:
std::move(session_info), std::move(controller_connection_proxy)));
Is it legal to std::move(nullptr)?

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.cc:434: return;
This seems like a bug, so DCHECK(). IIUC this must be set by Blink before it can
start the connection handshake.

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp
(right):

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:155:
// proxy != nullptr if presentation connection is
Can you wrap this comment at 80 columns?

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:158:
if (!proxy)
Since the proxy is optional, I have a slight preference to setting it outside
the constructor, i.e. have a SetPresentationConnectionProxy(proxy) method.

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:320:
m_proxy->SendString(message->text);
Do we want to send the string to both |m_proxy| and |client|?

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:326:
m_proxy->SendArrayBuffer(
Same comment applies here.

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnectionProxy.h
(right):

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnectionProxy.h:13:
// work.
Can you describe what functions specifically have to be implemented by this
class?

For example,

"Used to send and receive messages from a PresentationConnection hosted in a
different frame."

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/presentation/presentation.mojom
(right):

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:14:
bool is_offscreen_presentation;
Or just is_offscreen?  Since it's part of PresentationSessionInfo.

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:56:
interface PresentationConnection {
Can you add a TODO to migrate SendSessionMessage from PresenationService =>
PresentationConnection.Send()?

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:62:
OnConnectionMessageReceived(SessionMessage message);
Or just OnMessage() (similar to the Presentation API) since it seems clear as a
member of PresentationConnection that it's a connection message.

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:68: 
Can you add a comment block starting here that the functions here are for the
controller part of the API?

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:102: 
... And end the comment block.  The functions below here are shared between
controller and receiver I believe.

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:123:
SetPresentationConnection(PresentationSessionInfo sessionInfo,
Document this function.  Is it called on the controller, the receiver or both?

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:128:
// Called when the client tries to listen for screen availability changes for
Can you add a block comment above here that these functions are called only on
the controlling page.

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:139: 
And end the comment as the functions below except for the last two are shared.

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:156:
OnDefaultSessionStarted(PresentationSessionInfo sessionInfo);
Can this be moved above with the other controller-only functions?

Maybe we can eventually combine this with OnReceiverConnectionAvailable and just
have OnConnectionAvailable.  (Not to be done in this change.)

In the controlling page, it signals that the browser has started a presentation.
In the receiving page, it signals that the controller has connected to the
receiver.

Would that make sense, or is it more confusing?  Not sure.

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:160:
OnReceiverConnectionAvailable(PresentationSessionInfo sessionInfo,
Maybe this should be OnFirstConnectionAvailable?  Or is it called for all
connections to the receiver page?

[zhaobin, 2016-10-21 21:00:18]

Description was changed from

==========
[Presentation API] (alternative) 1-UA: send message between controller and
receiver page

It is an alternative implementation of 1-UA messaging (substitue Issue
2343013002 and Issue 2355723004):

- add PresentationConnection class to presentation.mojom
- add RenderedPresentationConnection class which implements
PresentationConnectionProxy and PresentationConnection mojo interface

Work flow to connect controller RenderedPresentationConnection with receiver
RenderedPresentationConnection:

1. controller page starts a presentation, creates a
RenderedPresentationConnection object
2. controller RenderedPresentationConnection's mojo handler is passed to PSImpl
via PSImpl::SetPresentationConnection
3. PSImpl::SetPresentationConnection registers the mojo handler with
OffscreenPresentationManager
4. receiver page initializes and registers receiver_call to
OffscreenPresentationManager
5. receiver PSImpl::OnReceiverConnectionAvailable() is invoked for each
controller RenderedPresentationConnection's mojo handler belonging to the same
presentation
6. PSImpl::OnReceiverConnectionAvailable() calls
PDispatcher::OnReceiverConnectionAvailable() which creates a new receiver
RenderedPresentationConnection and connects it with controller
RenderedPresentationConnection's mojo handler

Messaging happens between controller RenderedPresentationConnection and receiver
RenderedPresentationConnection afterwards

BUG=525660
==========

to

==========
[Presentation API] (alternative) 1-UA: send message between controller and
receiver page

It is an alternative implementation of 1-UA messaging (substitue Issue
2343013002 and Issue 2355723004):

- add PresentationConnection class to presentation.mojom
- add RenderedPresentationConnection class which implements
PresentationConnectionProxy and PresentationConnection mojo interface

Work flow to connect controller RenderedPresentationConnection with receiver
RenderedPresentationConnection:

1. controller page starts a presentation, creates a
RenderedPresentationConnection object
2. controller RenderedPresentationConnection's mojo handler is passed to PSImpl
via PSImpl::SetPresentationConnection
3. PSImpl::SetPresentationConnection registers the mojo handler with
OffscreenPresentationManager
4. receiver page initializes and registers receiver_call to
OffscreenPresentationManager
5. receiver PSImpl::OnReceiverConnectionAvailable() is invoked for each
controller RenderedPresentationConnection's mojo handler belonging to the same
presentation
6. PSImpl::OnReceiverConnectionAvailable() calls
PDispatcher::OnReceiverConnectionAvailable() which creates a new receiver
RenderedPresentationConnection and connects it with controller
RenderedPresentationConnection's mojo handler

Messaging happens between controller RenderedPresentationConnection and receiver
RenderedPresentationConnection afterwards

design doc:
https://docs.google.com/document/d/12xd7Loeme0UfcjOICNYnvsGS32ggeH9HwjDFCi_HQ...

BUG=525660
==========

[zhaobin, 2016-10-22 02:44:14]

https://codereview.chromium.org/2379703002/diff/160001/chrome/browser/media/r...
File chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc
(right):

https://codereview.chromium.org/2379703002/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:41:
class MockDelegateObserver
On 2016/10/21 00:53:02, mark a. foltz wrote:
> Please add a unit test for RegisterOffscreenPresentationConnection

Done.

https://codereview.chromium.org/2379703002/diff/160001/content/public/browser...
File content/public/browser/presentation_service_delegate.h (right):

https://codereview.chromium.org/2379703002/diff/160001/content/public/browser...
content/public/browser/presentation_service_delegate.h:222: virtual void
RegisterOffscreenPresentationConnection(
On 2016/10/21 00:53:02, mark a. foltz wrote:
> ConnectToOffscreenPresentation?

Done.

https://codereview.chromium.org/2379703002/diff/160001/content/public/browser...
content/public/browser/presentation_service_delegate.h:230: // forwarded from
receiver PresentationServiceImpl.
On 2016/10/21 00:53:02, mark a. foltz wrote:
> handle PresentationService calls from a presentation receiver.

Done.

https://codereview.chromium.org/2379703002/diff/160001/content/public/browser...
content/public/browser/presentation_service_delegate.h:236: // successfully
started.
On 2016/10/21 00:53:02, mark a. foltz wrote:
> Can you rewrap this comment?

Done.

https://codereview.chromium.org/2379703002/diff/160001/content/public/browser...
content/public/browser/presentation_service_delegate.h:239: virtual void
RegisterReceiverAvailableCallback(
On 2016/10/21 00:53:02, mark a. foltz wrote:
> RegisterReceiverConnectionAvailableCallback

Done.

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_client.h (right):

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
content/renderer/presentation/presentation_connection_client.h:26: //
|session_info|: contains presentation id and url info
On 2016/10/21 00:53:02, mark a. foltz wrote:
> nit: "Contains presentation ID and URL info."

Done.

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
content/renderer/presentation/presentation_connection_client.h:27: // |proxy|:
proxy to blink PresentationConnection object in current render
On 2016/10/21 00:53:02, mark a. foltz wrote:
> nit: Proxy

Done.

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
content/renderer/presentation/presentation_connection_client.h:39: //
(PresentationConnection).
On 2016/10/21 00:53:02, mark a. foltz wrote:
> Rewrap comments at 80 columns.

Done.

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
content/renderer/presentation/presentation_connection_client.h:40:
std::unique_ptr<blink::WebPresentationConnectionProxy> getProxy() override;
On 2016/10/21 00:53:02, mark a. foltz wrote:
> Can this be takeProxy() to denote change of ownership?

Done.

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_proxy.cc (right):

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:23: return;
On 2016/10/21 00:53:03, mark a. foltz wrote:
> This feels like a bug; the connection should not be sending messages before
it's
> connected.
> Can you DCHECK() here?
> 
> Same comment applies below.

Done.

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:25:
blink::mojom::SessionMessagePtr session_message =
On 2016/10/21 00:53:03, mark a. foltz wrote:
> Can you use the existing type converter for this?
> 
> Same comment applies below.

Done.

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:48:
source_connection_ = connection;
On 2016/10/21 00:53:02, mark a. foltz wrote:
> DCHECK(!source_connection_) before setting.

Done.

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:53:
target_connection_ = std::move(connection);
On 2016/10/21 00:53:02, mark a. foltz wrote:
> DCHECK(!target_connection_) before setting.

Done.

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_proxy.h (right):

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:16: // This class
connects PrensentationConnection object living in
On 2016/10/21 00:53:03, mark a. foltz wrote:
> typo in PresentationConnection

Done.

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:17: //
controller(receiver) render process
On 2016/10/21 00:53:03, mark a. foltz wrote:
> ... one frame with a PresentationConnection owned by a different frame.

Done.

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:19: // process.
On 2016/10/21 00:53:03, mark a. foltz wrote:
> It's up to the embedder to decide whether the controller and receiver live in
> different processes or not, so it would be more accurate to talk about
> connections owned by frames instead of processes.

Done.

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:29: // To send
message from controller page to receiver page:
On 2016/10/21 00:53:03, mark a. foltz wrote:
> controlling frame to receiver frame:

Done.

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:43: // Instance of
this class is only created for 1-UA mode.
On 2016/10/21 00:53:03, mark a. foltz wrote:
> s/1-UA mode/offscreen presentations/

Done.

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.cc:386: if
(session_info->is_offscreen_presentation) {
On 2016/10/21 00:53:03, mark a. foltz wrote:
> Can you factor out the common code to create and set the controller connection
> proxy?

Done.

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.cc:395:
std::move(session_info), std::move(controller_connection_proxy)));
On 2016/10/21 00:53:03, mark a. foltz wrote:
> Is it legal to std::move(nullptr)?

It works fine...

https://codereview.chromium.org/2379703002/diff/160001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.cc:434: return;
On 2016/10/21 00:53:03, mark a. foltz wrote:
> This seems like a bug, so DCHECK(). IIUC this must be set by Blink before it
can
> start the connection handshake.  

Done.

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp
(right):

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:155:
// proxy != nullptr if presentation connection is
On 2016/10/21 00:53:03, mark a. foltz wrote:
> Can you wrap this comment at 80 columns?

Done.

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:158:
if (!proxy)
On 2016/10/21 00:53:03, mark a. foltz wrote:
> Since the proxy is optional, I have a slight preference to setting it outside
> the constructor, i.e. have a SetPresentationConnectionProxy(proxy) method.

Done.

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:320:
m_proxy->SendString(message->text);
On 2016/10/21 00:53:03, mark a. foltz wrote:
> Do we want to send the string to both |m_proxy| and |client|?

Done.

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:326:
m_proxy->SendArrayBuffer(
On 2016/10/21 00:53:03, mark a. foltz wrote:
> Same comment applies here.

Done.

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnectionProxy.h
(right):

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnectionProxy.h:13:
// work.
On 2016/10/21 00:53:03, mark a. foltz wrote:
> Can you describe what functions specifically have to be implemented by this
> class?
> 
> For example,
> 
> "Used to send and receive messages from a PresentationConnection hosted in a
> different frame."

Done.

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/presentation/presentation.mojom
(right):

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:14:
bool is_offscreen_presentation;
On 2016/10/21 00:53:03, mark a. foltz wrote:
> Or just is_offscreen?  Since it's part of PresentationSessionInfo.

Done.

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:56:
interface PresentationConnection {
On 2016/10/21 00:53:03, mark a. foltz wrote:
> Can you add a TODO to migrate SendSessionMessage from PresenationService =>
> PresentationConnection.Send()?

Done.

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:62:
OnConnectionMessageReceived(SessionMessage message);
On 2016/10/21 00:53:03, mark a. foltz wrote:
> Or just OnMessage() (similar to the Presentation API) since it seems clear as
a
> member of PresentationConnection that it's a connection message.

Done.

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:68: 
On 2016/10/21 00:53:04, mark a. foltz wrote:
> Can you add a comment block starting here that the functions here are for the
> controller part of the API?

Done.

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:102: 
On 2016/10/21 00:53:03, mark a. foltz wrote:
> ... And end the comment block.  The functions below here are shared between
> controller and receiver I believe.

Done.

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:123:
SetPresentationConnection(PresentationSessionInfo sessionInfo,
On 2016/10/21 00:53:03, mark a. foltz wrote:
> Document this function.  Is it called on the controller, the receiver or both?

Done.

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:128:
// Called when the client tries to listen for screen availability changes for
On 2016/10/21 00:53:03, mark a. foltz wrote:
> Can you add a block comment above here that these functions are called only on
> the controlling page.

Done.

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:139: 
On 2016/10/21 00:53:03, mark a. foltz wrote:
> And end the comment as the functions below except for the last two are shared.

Done.

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:156:
OnDefaultSessionStarted(PresentationSessionInfo sessionInfo);
On 2016/10/21 00:53:03, mark a. foltz wrote:
> Can this be moved above with the other controller-only functions?
> 
> Maybe we can eventually combine this with OnReceiverConnectionAvailable and
just
> have OnConnectionAvailable.  (Not to be done in this change.)
> 
> In the controlling page, it signals that the browser has started a
presentation.
> In the receiving page, it signals that the controller has connected to the
> receiver.
> 
> Would that make sense, or is it more confusing?  Not sure.

Done.

https://codereview.chromium.org/2379703002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:160:
OnReceiverConnectionAvailable(PresentationSessionInfo sessionInfo,
On 2016/10/21 00:53:03, mark a. foltz wrote:
> Maybe this should be OnFirstConnectionAvailable?  Or is it called for all
> connections to the receiver page?

It is called for all connections to the receiver page.

[imcheng, 2016-11-01 17:20:30]

Thanks Bin. This is looking good and much better than my original design. I want
to make sure we have thought about how state changes would work from both the
controller and receiver's perspective.

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/offscreen_presentation_manager.cc (right):

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/offscreen_presentation_manager.cc:22:
OffscreenPresentationManager::OffscreenPresentationMap::iterator
it looks like you can just return OffscreenPresentation* here.

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/offscreen_presentation_manager.cc:32: .first;
add ->second

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/offscreen_presentation_manager.cc:65: if
(it->second->pending_controllers_.size() == 0 &&
nit: define a IsValid method in OffscreenPresentation that encapsulates this
logic.

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/offscreen_presentation_manager.cc:81: void
OffscreenPresentationManager::OnOffscreenPresentationReceiverTerminated(
how will the controllers be informed that the PresentationConnection has
terminated?

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/offscreen_presentation_manager.cc:95:
receiver_callback_(nullptr) {}
is it necessary to initialize receiver_callback_?

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/offscreen_presentation_manager.cc:103: // Create a
receiver PresentationConnectionPtr, and connect it with
It sounds like this comment belongs in the definition of the callback being
passed in.

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/offscreen_presentation_manager.h (right):

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/offscreen_presentation_manager.h:37: //  
manager->RegisterOffscreenPresentationReceiver(presentation_id,
please update comments, or update the method name below
(OnOffscreenPresentationReceiverCreated)

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/offscreen_presentation_manager.h:114: void
OnOffscreenPresentationReceiverTerminated(
Please add documentation

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/presentation_service_delegate_impl.cc (right):

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl.cc:671:
GetOrCreateForControllerBrowserContext(web_contents_)),
that means we are creating an OffscreenPresentationManager even we did not start
a 1-ua presentation. It seems to be we don't need to store it as a member of
PSDImpl since it is only called in one place.

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/presentation_service_delegate_impl.h (right):

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl.h:211:
OffscreenPresentationManager* offscreen_presentation_manager_;
* const, but see comment in .cc file.

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc
(right):

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc:80:
presentation_id_, web_contents_->GetURL(), receiver_available_callback);
GetURL() is deprecated - can you use GetLastCommittedURL()?

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/receiver_presentation_service_delegate_impl.h
(right):

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/receiver_presentation_service_delegate_impl.h:71:
explicit ReceiverPresentationServiceDelegateImpl(
no explicit

https://codereview.chromium.org/2379703002/diff/200001/content/browser/presen...
File content/browser/presentation/presentation_service_impl.cc (right):

https://codereview.chromium.org/2379703002/diff/200001/content/browser/presen...
content/browser/presentation/presentation_service_impl.cc:145: if
(receiver_delegate_) {
nit: Move this logic to CreateMojoService

https://codereview.chromium.org/2379703002/diff/200001/content/browser/presen...
content/browser/presentation/presentation_service_impl.cc:526: DVLOG(2) <<
"PresentationServiceImpl::OnReceiverConnectionAvailable";
nit: combine these DVLOGs

https://codereview.chromium.org/2379703002/diff/200001/content/browser/presen...
content/browser/presentation/presentation_service_impl.cc:616: << "
[is_offscreen]: " << connection.is_offscreen;
also log is_offscreen in On{Start,Join}SessionSucceeded?

https://codereview.chromium.org/2379703002/diff/200001/content/browser/presen...
File content/browser/presentation/presentation_service_impl.h (right):

https://codereview.chromium.org/2379703002/diff/200001/content/browser/presen...
content/browser/presentation/presentation_service_impl.h:152: // by this class.
Please add documentation for |controller_delegate| / |receiver_delegate|

https://codereview.chromium.org/2379703002/diff/200001/content/browser/presen...
content/browser/presentation/presentation_service_impl.h:264:
PresentationServiceDelegateBase* GetPresentationServiceDelegate();
Please add documentation.

https://codereview.chromium.org/2379703002/diff/200001/content/browser/presen...
content/browser/presentation/presentation_service_impl.h:268: // May be null if
current page is receiver page or
nit: It is definitely nullptr if either of those are true, isn't it?

https://codereview.chromium.org/2379703002/diff/200001/content/browser/presen...
content/browser/presentation/presentation_service_impl.h:270:
ControllerPresentationServiceDelegate* controller_delegate_;
* const

https://codereview.chromium.org/2379703002/diff/200001/content/browser/presen...
content/browser/presentation/presentation_service_impl.h:274: // May be null if
current page is receiver page or
ditto about nullptr

https://codereview.chromium.org/2379703002/diff/200001/content/browser/presen...
content/browser/presentation/presentation_service_impl.h:276:
ReceiverPresentationServiceDelegate* receiver_delegate_;
* const

https://codereview.chromium.org/2379703002/diff/200001/content/public/browser...
File content/public/browser/presentation_session.cc (right):

https://codereview.chromium.org/2379703002/diff/200001/content/public/browser...
content/public/browser/presentation_session.cc:12: :
presentation_url(presentation_url),
nit: Use the delegated constructor pattern:
PresentationSessionInfo(presentation_url, presentation_id, false)

https://codereview.chromium.org/2379703002/diff/200001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_client.cc (right):

https://codereview.chromium.org/2379703002/diff/200001/content/renderer/prese...
content/renderer/presentation/presentation_connection_client.cc:18:
proxy_(nullptr) {}
Can you see if you can use delegating constructors?

https://codereview.chromium.org/2379703002/diff/200001/content/renderer/prese...
content/renderer/presentation/presentation_connection_client.cc:30: if (proxy)
It doesn't look like this check is needed?

https://codereview.chromium.org/2379703002/diff/200001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_client.h (right):

https://codereview.chromium.org/2379703002/diff/200001/content/renderer/prese...
content/renderer/presentation/presentation_connection_client.h:23:
PresentationConnectionClient(const GURL& url, const mojo::String& id);
do we actually still need this? This is only called in
PresentationDispatcher::OnSessionMessagesReceived, and in there we already have
a blink::mojom::PresentationSessionInfoPtr.

https://codereview.chromium.org/2379703002/diff/200001/content/renderer/prese...
content/renderer/presentation/presentation_connection_client.h:24: // Call this
constructor to create PresentationConnectionClient for
nit: add extra line above comments

https://codereview.chromium.org/2379703002/diff/200001/content/renderer/prese...
content/renderer/presentation/presentation_connection_client.h:24: // Call this
constructor to create PresentationConnectionClient for
I would word this as: Creates a PresentationConnectionClient for a 1-UA
presentation.

https://codereview.chromium.org/2379703002/diff/200001/content/renderer/prese...
content/renderer/presentation/presentation_connection_client.h:37: // Returns
proxy passed into this class. It will transfer ownership of
nit: add extra line above comments

https://codereview.chromium.org/2379703002/diff/200001/content/renderer/prese...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/2379703002/diff/200001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.cc:382:
presentation_service_->ListenForSessionMessages(session_info.Clone());
It seems we shouldn't be listening for messages from MR for 1-ua presentations.

https://codereview.chromium.org/2379703002/diff/200001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.cc:403:
presentation_service_->ListenForSessionMessages(session_info.Clone());
ditto

https://codereview.chromium.org/2379703002/diff/200001/content/renderer/prese...
File content/renderer/presentation/presentation_dispatcher.h (right):

https://codereview.chromium.org/2379703002/diff/200001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.h:36: class
PresentationConnectionProxy;
nit: add blank line below

https://codereview.chromium.org/2379703002/diff/200001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.h:47: static
blink::mojom::SessionMessagePtr ToMojoTextMessage(
do these need to be public static methods or can they just be functions in
anonymous namespace?

https://codereview.chromium.org/2379703002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp
(right):

https://codereview.chromium.org/2379703002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:199:
if (auto proxy = client->takeProxy())
this check is not needed? std::move should work on nullptrs.

https://codereview.chromium.org/2379703002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:219:
if (auto proxy = client->takeProxy())
this should always be non-null, right?

https://codereview.chromium.org/2379703002/diff/200001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/presentation/presentation.mojom
(right):

https://codereview.chromium.org/2379703002/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:110:
SetPresentationConnection(PresentationSessionInfo sessionInfo,
It seems we are only calling this for 1-ua presentations. We should make this
clear in the method name and documentation.

[zhaobin, 2016-11-02 03:55:48]

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/offscreen_presentation_manager.cc (right):

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/offscreen_presentation_manager.cc:22:
OffscreenPresentationManager::OffscreenPresentationMap::iterator
On 2016/11/01 17:20:28, imcheng wrote:
> it looks like you can just return OffscreenPresentation* here.

Done.

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/offscreen_presentation_manager.cc:32: .first;
On 2016/11/01 17:20:29, imcheng wrote:
> add ->second

Done.

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/offscreen_presentation_manager.cc:65: if
(it->second->pending_controllers_.size() == 0 &&
On 2016/11/01 17:20:28, imcheng wrote:
> nit: define a IsValid method in OffscreenPresentation that encapsulates this
> logic.

Done.

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/offscreen_presentation_manager.cc:81: void
OffscreenPresentationManager::OnOffscreenPresentationReceiverTerminated(
On 2016/11/01 17:20:28, imcheng wrote:
> how will the controllers be informed that the PresentationConnection has
> terminated?

Not implemented yet. Controller connection should already connect with receiver
connection by the time we call connection.close() or connection.terminate().
Thinking of doing it by communicating state changes between connections directly
instead of via OPM.

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/offscreen_presentation_manager.cc:95:
receiver_callback_(nullptr) {}
On 2016/11/01 17:20:28, imcheng wrote:
> is it necessary to initialize receiver_callback_?

Done.

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/offscreen_presentation_manager.cc:103: // Create a
receiver PresentationConnectionPtr, and connect it with
On 2016/11/01 17:20:28, imcheng wrote:
> It sounds like this comment belongs in the definition of the callback being
> passed in.

Done.

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/offscreen_presentation_manager.h (right):

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/offscreen_presentation_manager.h:37: //  
manager->RegisterOffscreenPresentationReceiver(presentation_id,
On 2016/11/01 17:20:29, imcheng wrote:
> please update comments, or update the method name below
> (OnOffscreenPresentationReceiverCreated) 

Done.

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/offscreen_presentation_manager.h:114: void
OnOffscreenPresentationReceiverTerminated(
On 2016/11/01 17:20:29, imcheng wrote:
> Please add documentation

Done.

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/presentation_service_delegate_impl.cc (right):

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl.cc:671:
GetOrCreateForControllerBrowserContext(web_contents_)),
On 2016/11/01 17:20:29, imcheng wrote:
> that means we are creating an OffscreenPresentationManager even we did not
start
> a 1-ua presentation. It seems to be we don't need to store it as a member of
> PSDImpl since it is only called in one place.

Done.

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/presentation_service_delegate_impl.h (right):

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl.h:211:
OffscreenPresentationManager* offscreen_presentation_manager_;
On 2016/11/01 17:20:29, imcheng wrote:
> * const, but see comment in .cc file.

Done.

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc
(right):

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc:80:
presentation_id_, web_contents_->GetURL(), receiver_available_callback);
On 2016/11/01 17:20:29, imcheng wrote:
> GetURL() is deprecated - can you use GetLastCommittedURL()?

Done.

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/receiver_presentation_service_delegate_impl.h
(right):

https://codereview.chromium.org/2379703002/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/receiver_presentation_service_delegate_impl.h:71:
explicit ReceiverPresentationServiceDelegateImpl(
On 2016/11/01 17:20:29, imcheng wrote:
> no explicit

Done.

https://codereview.chromium.org/2379703002/diff/200001/content/browser/presen...
File content/browser/presentation/presentation_service_impl.cc (right):

https://codereview.chromium.org/2379703002/diff/200001/content/browser/presen...
content/browser/presentation/presentation_service_impl.cc:145: if
(receiver_delegate_) {
On 2016/11/01 17:20:29, imcheng wrote:
> nit: Move this logic to CreateMojoService

Done.

https://codereview.chromium.org/2379703002/diff/200001/content/browser/presen...
content/browser/presentation/presentation_service_impl.cc:526: DVLOG(2) <<
"PresentationServiceImpl::OnReceiverConnectionAvailable";
On 2016/11/01 17:20:29, imcheng wrote:
> nit: combine these DVLOGs

log is for debug purpose, removed.

https://codereview.chromium.org/2379703002/diff/200001/content/browser/presen...
content/browser/presentation/presentation_service_impl.cc:616: << "
[is_offscreen]: " << connection.is_offscreen;
On 2016/11/01 17:20:29, imcheng wrote:
> also log is_offscreen in On{Start,Join}SessionSucceeded?

Done.

https://codereview.chromium.org/2379703002/diff/200001/content/browser/presen...
File content/browser/presentation/presentation_service_impl.h (right):

https://codereview.chromium.org/2379703002/diff/200001/content/browser/presen...
content/browser/presentation/presentation_service_impl.h:152: // by this class.
On 2016/11/01 17:20:29, imcheng wrote:
> Please add documentation for |controller_delegate| / |receiver_delegate|

Done.

https://codereview.chromium.org/2379703002/diff/200001/content/browser/presen...
content/browser/presentation/presentation_service_impl.h:264:
PresentationServiceDelegateBase* GetPresentationServiceDelegate();
On 2016/11/01 17:20:29, imcheng wrote:
> Please add documentation.

Done.

https://codereview.chromium.org/2379703002/diff/200001/content/browser/presen...
content/browser/presentation/presentation_service_impl.h:268: // May be null if
current page is receiver page or
On 2016/11/01 17:20:29, imcheng wrote:
> nit: It is definitely nullptr if either of those are true, isn't it?

Done.

https://codereview.chromium.org/2379703002/diff/200001/content/browser/presen...
content/browser/presentation/presentation_service_impl.h:270:
ControllerPresentationServiceDelegate* controller_delegate_;
On 2016/11/01 17:20:29, imcheng wrote:
> * const

Cannot be const since we set them to nullptr in Reset().

https://codereview.chromium.org/2379703002/diff/200001/content/browser/presen...
content/browser/presentation/presentation_service_impl.h:274: // May be null if
current page is receiver page or
On 2016/11/01 17:20:29, imcheng wrote:
> ditto about nullptr

Done.

https://codereview.chromium.org/2379703002/diff/200001/content/browser/presen...
content/browser/presentation/presentation_service_impl.h:276:
ReceiverPresentationServiceDelegate* receiver_delegate_;
On 2016/11/01 17:20:29, imcheng wrote:
> * const

Cannot be const since we set them to nullptr in Reset().

https://codereview.chromium.org/2379703002/diff/200001/content/public/browser...
File content/public/browser/presentation_session.cc (right):

https://codereview.chromium.org/2379703002/diff/200001/content/public/browser...
content/public/browser/presentation_session.cc:12: :
presentation_url(presentation_url),
On 2016/11/01 17:20:29, imcheng wrote:
> nit: Use the delegated constructor pattern:
> PresentationSessionInfo(presentation_url, presentation_id, false)

Done.

https://codereview.chromium.org/2379703002/diff/200001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_client.cc (right):

https://codereview.chromium.org/2379703002/diff/200001/content/renderer/prese...
content/renderer/presentation/presentation_connection_client.cc:18:
proxy_(nullptr) {}
On 2016/11/01 17:20:30, imcheng wrote:
> Can you see if you can use delegating constructors?

Done.

https://codereview.chromium.org/2379703002/diff/200001/content/renderer/prese...
content/renderer/presentation/presentation_connection_client.cc:30: if (proxy)
On 2016/11/01 17:20:30, imcheng wrote:
> It doesn't look like this check is needed?

Done.

https://codereview.chromium.org/2379703002/diff/200001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_client.h (right):

https://codereview.chromium.org/2379703002/diff/200001/content/renderer/prese...
content/renderer/presentation/presentation_connection_client.h:23:
PresentationConnectionClient(const GURL& url, const mojo::String& id);
On 2016/11/01 17:20:30, imcheng wrote:
> do we actually still need this? This is only called in
> PresentationDispatcher::OnSessionMessagesReceived, and in there we already
have
> a blink::mojom::PresentationSessionInfoPtr.

Done.

https://codereview.chromium.org/2379703002/diff/200001/content/renderer/prese...
content/renderer/presentation/presentation_connection_client.h:24: // Call this
constructor to create PresentationConnectionClient for
On 2016/11/01 17:20:30, imcheng wrote:
> nit: add extra line above comments

Done.

https://codereview.chromium.org/2379703002/diff/200001/content/renderer/prese...
content/renderer/presentation/presentation_connection_client.h:24: // Call this
constructor to create PresentationConnectionClient for
On 2016/11/01 17:20:30, imcheng wrote:
> I would word this as: Creates a PresentationConnectionClient for a 1-UA
> presentation.

Done.

https://codereview.chromium.org/2379703002/diff/200001/content/renderer/prese...
content/renderer/presentation/presentation_connection_client.h:37: // Returns
proxy passed into this class. It will transfer ownership of
On 2016/11/01 17:20:30, imcheng wrote:
> nit: add extra line above comments

Done.

https://codereview.chromium.org/2379703002/diff/200001/content/renderer/prese...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/2379703002/diff/200001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.cc:382:
presentation_service_->ListenForSessionMessages(session_info.Clone());
On 2016/11/01 17:20:30, imcheng wrote:
> It seems we shouldn't be listening for messages from MR for 1-ua
presentations.

Done.

https://codereview.chromium.org/2379703002/diff/200001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.cc:403:
presentation_service_->ListenForSessionMessages(session_info.Clone());
On 2016/11/01 17:20:30, imcheng wrote:
> ditto

Done.

https://codereview.chromium.org/2379703002/diff/200001/content/renderer/prese...
File content/renderer/presentation/presentation_dispatcher.h (right):

https://codereview.chromium.org/2379703002/diff/200001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.h:36: class
PresentationConnectionProxy;
On 2016/11/01 17:20:30, imcheng wrote:
> nit: add blank line below

Done.

https://codereview.chromium.org/2379703002/diff/200001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.h:47: static
blink::mojom::SessionMessagePtr ToMojoTextMessage(
On 2016/11/01 17:20:30, imcheng wrote:
> do these need to be public static methods or can they just be functions in
> anonymous namespace?

Done.

https://codereview.chromium.org/2379703002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp
(right):

https://codereview.chromium.org/2379703002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:199:
if (auto proxy = client->takeProxy())
On 2016/11/01 17:20:30, imcheng wrote:
> this check is not needed? std::move should work on nullptrs.

Done.

https://codereview.chromium.org/2379703002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:219:
if (auto proxy = client->takeProxy())
On 2016/11/01 17:20:30, imcheng wrote:
> this should always be non-null, right?

Done.

https://codereview.chromium.org/2379703002/diff/200001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/presentation/presentation.mojom
(right):

https://codereview.chromium.org/2379703002/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:110:
SetPresentationConnection(PresentationSessionInfo sessionInfo,
On 2016/11/01 17:20:30, imcheng wrote:
> It seems we are only calling this for 1-ua presentations. We should make this
> clear in the method name and documentation.

Done.

[mark a. foltz, 2016-12-13 23:42:56]

zhaobin@, I think we split this into smaller changes to land.  So, you can close
this issue to take it out of our review queue.