[Presentation API] (MR side) 1-UA: notify receiver page when receiver connection becomes available

[Presentation API] (MR side) 1-UA: notify receiver page when receiver connection becomes available

Following files are directly ported from https://codereview.chromium.org/1314413005/

    chrome/browser/chrome_content_browser_client.cc
    chrome/browser/extensions/api/tab_capture/offscreen_tab.cc
    chrome/browser/media/router/offscreen_presentation_manager_factory.cc
    chrome/browser/media/router/offscreen_presentation_manager_factory.h
    chrome/browser/media/router/offscreen_presentation_manager_factory_unittest.cc

Following files are mostly the same as https://codereview.chromium.org/1314413005/, with some functions removed:

    chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc
    chrome/browser/media/router/receiver_presentation_service_delegate_impl.h

New changes in this CL:

- Add RegisterController/Receiver() functions to OffscreenPresentationManager class and PSDImpl.
- For controller PSImpl, call registerController() when session starts; call unregisterController() when connection is closed.
- For receiver PSImpl, call registerReceiver() when receiver frame is created and presentation.receiver() is accessed; call unregisterReceiver() when receiver frame is destroyed.

BUG=525660

[zhaobin, 2016-09-15 23:38:18]

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

[zhaobin, 2016-09-15 23:40:37]

Description was changed from

==========
[Prsentation API] (MR side) 1-UA: notify receiver page when receiver connection
becomes available

Following files are directly ported from
https://codereview.chromium.org/1314413005/

    chrome/browser/chrome_content_browser_client.cc
    chrome/browser/extensions/api/tab_capture/offscreen_tab.cc
    chrome/browser/media/router/offscreen_presentation_manager_factory.cc
    chrome/browser/media/router/offscreen_presentation_manager_factory.h
   
chrome/browser/media/router/offscreen_presentation_manager_factory_unittest.cc

Following files are mostly the same as
https://codereview.chromium.org/1314413005/, with some functions removed:

    chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc
    chrome/browser/media/router/receiver_presentation_service_delegate_impl.h

New changes in this CL:

- Add RegisterController/Receiver() functions to OffscreenPresentationManager
class and PSDImpl.
- For controller PSImpl, call registerController() when session starts; call
unregisterController() when connection is closed.
- For receiver PSImpl, call registerReceiver() when receiver frame is created
and presentation.receiver() is accessed; call unregisterReceiver() when receiver
frame is destroyed.

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

to

==========
[Prsentation API] (MR side) 1-UA: notify receiver page when receiver connection
becomes available

Following files are directly ported from
https://codereview.chromium.org/1314413005/

    chrome/browser/chrome_content_browser_client.cc
    chrome/browser/extensions/api/tab_capture/offscreen_tab.cc
    chrome/browser/media/router/offscreen_presentation_manager_factory.cc
    chrome/browser/media/router/offscreen_presentation_manager_factory.h
   
chrome/browser/media/router/offscreen_presentation_manager_factory_unittest.cc

Following files are mostly the same as
https://codereview.chromium.org/1314413005/, with some functions removed:

    chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc
    chrome/browser/media/router/receiver_presentation_service_delegate_impl.h

New changes in this CL:

- Add RegisterController/Receiver() functions to OffscreenPresentationManager
class and PSDImpl.
- For controller PSImpl, call registerController() when session starts; call
unregisterController() when connection is closed.
- For receiver PSImpl, call registerReceiver() when receiver frame is created
and presentation.receiver() is accessed; call unregisterReceiver() when receiver
frame is destroyed.

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

[zhaobin, 2016-09-15 23:40:37]

zhaobin@chromium.org changed reviewers:
+ mfoltz@chromium.org
- mfoltz@google.com

[zhaobin, 2016-09-15 23:46:12]

Description was changed from

==========
[Prsentation API] (MR side) 1-UA: notify receiver page when receiver connection
becomes available

Following files are directly ported from
https://codereview.chromium.org/1314413005/

    chrome/browser/chrome_content_browser_client.cc
    chrome/browser/extensions/api/tab_capture/offscreen_tab.cc
    chrome/browser/media/router/offscreen_presentation_manager_factory.cc
    chrome/browser/media/router/offscreen_presentation_manager_factory.h
   
chrome/browser/media/router/offscreen_presentation_manager_factory_unittest.cc

Following files are mostly the same as
https://codereview.chromium.org/1314413005/, with some functions removed:

    chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc
    chrome/browser/media/router/receiver_presentation_service_delegate_impl.h

New changes in this CL:

- Add RegisterController/Receiver() functions to OffscreenPresentationManager
class and PSDImpl.
- For controller PSImpl, call registerController() when session starts; call
unregisterController() when connection is closed.
- For receiver PSImpl, call registerReceiver() when receiver frame is created
and presentation.receiver() is accessed; call unregisterReceiver() when receiver
frame is destroyed.

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

to

==========
[Presentation API] (MR side) 1-UA: notify receiver page when receiver connection
becomes available

Following files are directly ported from
https://codereview.chromium.org/1314413005/

    chrome/browser/chrome_content_browser_client.cc
    chrome/browser/extensions/api/tab_capture/offscreen_tab.cc
    chrome/browser/media/router/offscreen_presentation_manager_factory.cc
    chrome/browser/media/router/offscreen_presentation_manager_factory.h
   
chrome/browser/media/router/offscreen_presentation_manager_factory_unittest.cc

Following files are mostly the same as
https://codereview.chromium.org/1314413005/, with some functions removed:

    chrome/browser/media/router/receiver_presentation_service_delegate_impl.cc
    chrome/browser/media/router/receiver_presentation_service_delegate_impl.h

New changes in this CL:

- Add RegisterController/Receiver() functions to OffscreenPresentationManager
class and PSDImpl.
- For controller PSImpl, call registerController() when session starts; call
unregisterController() when connection is closed.
- For receiver PSImpl, call registerReceiver() when receiver frame is created
and presentation.receiver() is accessed; call unregisterReceiver() when receiver
frame is destroyed.

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

[imcheng, 2016-09-19 23:01:17]

I reviewed a majority of the files but had some concerns about the overall
design. In particular, it looks like the the methods to register/unregister
OffscreenPresentationClient is invoked regardless of whether the presentation
connection in question if offscreen or not (or common codepaths like
SetClient/Reset). 

Also, it looks like the logic to have the controller and receiver PSImpl talk to
each other directly for offscreen presentation connection operations is not
implemented. 

Perhaps we should discuss the overall design offline? It would help me clarify
what the overall flow for establishing an offscreen presentatiob would look
like, as well.

https://codereview.chromium.org/2343013002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/tab_capture/offscreen_tab.cc (right):

https://codereview.chromium.org/2343013002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/offscreen_tab.cc:20: #if
defined(ENABLE_MEDIA_ROUTER)
No need for the ENABLE_MEDIA_ROUTER check. We should just get rid of it.

https://codereview.chromium.org/2343013002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/offscreen_tab.cc:21: #include
"chrome/browser/media/router/media_router_feature.h"
this include not needed?

https://codereview.chromium.org/2343013002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/offscreen_tab.cc:127: DVLOG(1) <<
"Register with PresentationRouter, id="
I suggest just combining this log statement with the DVLOG above.

https://codereview.chromium.org/2343013002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/offscreen_presentation_manager.cc (right):

https://codereview.chromium.org/2343013002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/offscreen_presentation_manager.cc:29:
base::WrapUnique(new OffscreenPresentation(presentation_id))));
base::MakeUnique<OffscreenPresentation>(presentation_id)

https://codereview.chromium.org/2343013002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/offscreen_presentation_manager.cc:31: auto it =
offscreen_presentations_.find(presentation_id);
The entry should already exist in offscreen_presentations_ at this point?

I would rewrite this as:

auto it = offscreen_presentations_.find(...);
if (it == offscreen_presentations_.end()) {
  it = offscreen_presentations_.insert(...);
}

it->second->RegisterController(...);

https://codereview.chromium.org/2343013002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/offscreen_presentation_manager.h (right):

https://codereview.chromium.org/2343013002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/offscreen_presentation_manager.h:1: // Copyright
2015 The Chromium Authors. All rights reserved.
2016

https://codereview.chromium.org/2343013002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/offscreen_presentation_manager.h:22: // This class
lives in browser context and manages all presentations
Please clarify the comments:

- Instances of this class manages all offscreen presentations started in the
associated Profile and facilitates communication between the controllers and the
receiver of an offscreen presentation.
- Please add example usage that shows the series of calls that results in a
functional communication channel between a controller and receiver
- Please document threading assumptions of this class.

https://codereview.chromium.org/2343013002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/offscreen_presentation_manager.h:43:
content::OffscreenPresentationClient* receiver);
The method signature here doesn't restrict it to ReceiverPSDImpl only - should
we be passing in a ReceiverPSDImpl* instead?

Also please document the ownership of objects passed into these methods.

https://codereview.chromium.org/2343013002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/offscreen_presentation_manager.h:64: class
OffscreenPresentation {
Please add documentation

https://codereview.chromium.org/2343013002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/offscreen_presentation_manager_unittest.cc
(right):

https://codereview.chromium.org/2343013002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/offscreen_presentation_manager_unittest.cc:45: void
OffscreenPresentationManagerTest::verifyPresentationsSize(
These don't need to be their own functions since these are just simple
EXPECT_EQ's

https://codereview.chromium.org/2343013002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/presentation_service_delegate_impl.h (right):

https://codereview.chromium.org/2343013002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/presentation_service_delegate_impl.h:221:
OffscreenPresentationManager* const m_offscreenPresentationManager;
offscreen_presentation_manager_;

https://codereview.chromium.org/2343013002/diff/1/content/browser/presentatio...
File content/browser/presentation/presentation_service_impl.cc (right):

https://codereview.chromium.org/2343013002/diff/1/content/browser/presentatio...
content/browser/presentation/presentation_service_impl.cc:188:
delegate_->RegisterOffscreenPresentationReceiver(this);
I have a few concerns with this approach:

1) We are calling this in a common code path, regardless of whether this PSImpl
corresponds to a controller or a receiver. Needing a comment to state that it
does nothing on the controller frame is indicative that that we are not modeling
the relationship between objects / interfaces properly.

2) It appears what the code is trying to achieve is to bind the entire
controller PSImpl to talk to the receiver PSImpl directly and vice versa. In
reality, we only want to use this special direct path for offscreen presentation
connections. For instance, if a controller page has both offscreen and
non-offscreen presentations, it's not clear to me how it would work in that
case.

3) It doesn't seem like we are actually using the special path in here - I see
we are storing |offscreen_presentation_observers_|, but they aren't used. It is
also not clear how the multiple controllers case will work.

[zhaobin, 2016-09-23 17:18:19]

Change implementation of OffscreenPresentationManager:
  - recevier_ becomes a callback
  - controllers_ only contains controllers registered before receiver callback

Invoke RegisterOffscreenController(receiver) in PSDImpl instead of PSImpl

https://codereview.chromium.org/2343013002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/tab_capture/offscreen_tab.cc (right):

https://codereview.chromium.org/2343013002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/offscreen_tab.cc:20: #if
defined(ENABLE_MEDIA_ROUTER)
On 2016/09/19 23:01:17, imcheng wrote:
> No need for the ENABLE_MEDIA_ROUTER check. We should just get rid of it.

Done.

https://codereview.chromium.org/2343013002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/offscreen_tab.cc:21: #include
"chrome/browser/media/router/media_router_feature.h"
On 2016/09/19 23:01:17, imcheng wrote:
> this include not needed?

Done.

https://codereview.chromium.org/2343013002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/tab_capture/offscreen_tab.cc:127: DVLOG(1) <<
"Register with PresentationRouter, id="
On 2016/09/19 23:01:17, imcheng wrote:
> I suggest just combining this log statement with the DVLOG above.

Done.

https://codereview.chromium.org/2343013002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/offscreen_presentation_manager.cc (right):

https://codereview.chromium.org/2343013002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/offscreen_presentation_manager.cc:29:
base::WrapUnique(new OffscreenPresentation(presentation_id))));
On 2016/09/19 23:01:17, imcheng wrote:
> base::MakeUnique<OffscreenPresentation>(presentation_id)

Done.

https://codereview.chromium.org/2343013002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/offscreen_presentation_manager.cc:31: auto it =
offscreen_presentations_.find(presentation_id);
On 2016/09/19 23:01:17, imcheng wrote:
> The entry should already exist in offscreen_presentations_ at this point?
> 
> I would rewrite this as:
> 
> auto it = offscreen_presentations_.find(...);
> if (it == offscreen_presentations_.end()) {
>   it = offscreen_presentations_.insert(...);
> }
> 
> it->second->RegisterController(...);

Done.

https://codereview.chromium.org/2343013002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/offscreen_presentation_manager.h (right):

https://codereview.chromium.org/2343013002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/offscreen_presentation_manager.h:1: // Copyright
2015 The Chromium Authors. All rights reserved.
On 2016/09/19 23:01:17, imcheng wrote:
> 2016

Done.

https://codereview.chromium.org/2343013002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/offscreen_presentation_manager.h:22: // This class
lives in browser context and manages all presentations
On 2016/09/19 23:01:17, imcheng wrote:
> Please clarify the comments:
> 
> - Instances of this class manages all offscreen presentations started in the
> associated Profile and facilitates communication between the controllers and
the
> receiver of an offscreen presentation.
> - Please add example usage that shows the series of calls that results in a
> functional communication channel between a controller and receiver
> - Please document threading assumptions of this class.

Done.

https://codereview.chromium.org/2343013002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/offscreen_presentation_manager.h:43:
content::OffscreenPresentationClient* receiver);
On 2016/09/19 23:01:17, imcheng wrote:
> The method signature here doesn't restrict it to ReceiverPSDImpl only - should
> we be passing in a ReceiverPSDImpl* instead?
> 
> Also please document the ownership of objects passed into these methods.

Done.

https://codereview.chromium.org/2343013002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/offscreen_presentation_manager.h:64: class
OffscreenPresentation {
On 2016/09/19 23:01:17, imcheng wrote:
> Please add documentation

Done.

https://codereview.chromium.org/2343013002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/offscreen_presentation_manager_unittest.cc
(right):

https://codereview.chromium.org/2343013002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/offscreen_presentation_manager_unittest.cc:45: void
OffscreenPresentationManagerTest::verifyPresentationsSize(
On 2016/09/19 23:01:17, imcheng wrote:
> These don't need to be their own functions since these are just simple
> EXPECT_EQ's

Create these functions to access private variable in
OffscreenPresentationManager. Or we will need to add all TestMethods as friend
class to OffscreenPresentationManager.

https://codereview.chromium.org/2343013002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/presentation_service_delegate_impl.h (right):

https://codereview.chromium.org/2343013002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/presentation_service_delegate_impl.h:221:
OffscreenPresentationManager* const m_offscreenPresentationManager;
On 2016/09/19 23:01:17, imcheng wrote:
> offscreen_presentation_manager_;

Done.

https://codereview.chromium.org/2343013002/diff/1/content/browser/presentatio...
File content/browser/presentation/presentation_service_impl.cc (right):

https://codereview.chromium.org/2343013002/diff/1/content/browser/presentatio...
content/browser/presentation/presentation_service_impl.cc:188:
delegate_->RegisterOffscreenPresentationReceiver(this);
On 2016/09/19 23:01:17, imcheng wrote:
> I have a few concerns with this approach:
> 
> 1) We are calling this in a common code path, regardless of whether this
PSImpl
> corresponds to a controller or a receiver. Needing a comment to state that it
> does nothing on the controller frame is indicative that that we are not
modeling
> the relationship between objects / interfaces properly.
> 
> 2) It appears what the code is trying to achieve is to bind the entire
> controller PSImpl to talk to the receiver PSImpl directly and vice versa. In
> reality, we only want to use this special direct path for offscreen
presentation
> connections. For instance, if a controller page has both offscreen and
> non-offscreen presentations, it's not clear to me how it would work in that
> case.
> 
> 3) It doesn't seem like we are actually using the special path in here - I see
> we are storing |offscreen_presentation_observers_|, but they aren't used. It
is
> also not clear how the multiple controllers case will work.

Acknowledged.

[imcheng, 2016-09-28 07:28:37]

This is looking better. The messaging part made more sense after reading your
follow up patch, but other parts will make more sense after we have extracted
PresentationConnection into its own mojo interface.

I think the end picture (with PDispatchers sending message to
each other without going thru browser process) will look something like this:

1) When PresentationFrame sees that an offscreen presentation has been created,
create a two way pipe for messaging consisting of two mojo handles A and B.

2a) A is sent back to the controller PSImpl via the DelegateObserver interface,
maybe something like OnOffscreenPresentationStarted(Connection*,
PresentationSessionInfo).

3a) PSImpl sends A to its corresponding PDispatcher.

2b) B is sent back to the receiver PSImpl via the OnReceiverConnectionAvailable
callback. The tricky part here is to either make sure the callback has already
been registered at this point, or we need to save B until the callback is
registered.

An alternative to consider here is to also use the DelegateObserver interface to
send back B. Maybe something like: ReceiverPSDImpl registers callback to OPM on
construction -> when callback is invoked with B, ReceiverPSDImpl will send back
B via DelegateObserver. Or save it until the receiver PSImpl registers itself as
a DelegateObserver, which can happen during construction.

3b) Similarly the receiver PSImpl sends B to its corresponding PDispatcher.
Now the controller/receiver PDispatcher can send messages to each other
directly.

This should work even with multiple controllers, as long as we are associating
each PresentationConnection js object with a corresponding mojo handle.

If that is the case then we may as well reuse DelegateObserver for now to
simplify some of the flows currently and since there wouldn't be
OffscreenPresentationClient in the end picture.

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

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/offscreen_tab.cc:125: #if
defined(ENABLE_MEDIA_ROUTER)
nit: Remove the #if

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

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.cc:63: if (it !=
offscreen_presentations_.end())
Does the first RegisterOffscreenPresentationController for an offscreen
presentation always occur before RegisterOffscreenPresentationReceiver? Or would
it be ok to assume that the they can occur in any order?

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.cc:70:
offscreen_presentations_.erase(presentation_id);
question: how do we inform controllers that the receiver has left?

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.cc:85:
content::PresentationSessionInfo(presentation_url_, presentation_id_),
Looks like we also need to pass in presentation_url to this class?

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

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:10: #include <set>
<set> and <vector> not used?

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:26: // receiver of
an offscreen presentation.
nit: this can go in previous line.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:69: // with
nit: line wrap

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:92: int
render_frame_id,
Please use RenderFrameHostId here and below for all |render_frame_id|. See:
https://cs.chromium.org/chromium/src/chrome/browser/media/router/presentation...

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:93:
content::OffscreenPresentationClient* controller);
Can you forward declare OffscreenPresentationClient?

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:129: class
OffscreenPresentation {
It looks like this class is not accessed from outside the .cc and the unit test.
Maybe make this a private nested class of OPM?

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:158: std::map<int,
content::OffscreenPresentationClient*> controllers_;
RenderFrameHostId

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:158: std::map<int,
content::OffscreenPresentationClient*> controllers_;
nit: I'd probably name it pending_controllers_ to be clear.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/offscreen_presentation_manager_factory.h
(right):

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_factory.h:19: class
OffscreenPresentationManagerFactory
Please add a comment here that OffscreenPresentation manager is shared between a
Profile and its associated incognito Profiles.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_factory.h:23:
content::BrowserContext*);
nit: Please add parameter names here and below

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/offscreen_presentation_manager_unittest.cc
(right):

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_unittest.cc:35: void
verifyPresentationsSize(size_t expected);
nit: VerifyNumPresentations

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_unittest.cc:36: void
verifyControllerSize(size_t expected,
nit: VerifyNumControllers

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_unittest.cc:50: void
OffscreenPresentationManagerTest::verifyPresentationsSize(
These helper methods can be inlined.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_unittest.cc:62:
RegisterControllerCreateNewPresentation) {
nit: I would just name the test RegisterController

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_unittest.cc:69:
TEST_F(OffscreenPresentationManagerTest, RegisterOneReceiverDoNothing) {
RegisterReceiver

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_unittest.cc:78:
TEST_F(OffscreenPresentationManagerTest, UnregisterControllerNoCrash) {
UnregisterNonexistentController

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_unittest.cc:79:
MockOffscreenPresentationClient controller;
Unused?

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_unittest.cc:84:
TEST_F(OffscreenPresentationManagerTest, UnregisterReceiverNoCrash) {
UnregisterNonexistentReceiver

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_unittest.cc:90:
RegisterControllerCreatePresentationWithTwoControllers) {
RegisterMultipleControllersSamePresentation ?

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_unittest.cc:101:
RegisterControllerCreateTwoPresentations) {
RegisterMultipleControllersDifferentPresentations

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_unittest.cc:112:
RegisterControllerAndReceiverFireOnConnectionAvailable) {
RegisterControllerThenReceiverInvokesCallback ?

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_unittest.cc:117:
EXPECT_CALL(receiver_callback, OnReceiverConnectionAvailable(_, &controller));
Move this EXPECT to just before RegisterOffscreenPresentationReceiver. Same for
below.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_unittest.cc:127:
TEST_F(OffscreenPresentationManagerTest, RegisterUnregisterController) {
Combine this case with RegisterController

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_unittest.cc:135:
TEST_F(OffscreenPresentationManagerTest, UnregisterReceiverOnly) {
UnregisterReceiverFromConnectedPresentation

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_unittest.cc:151:
TEST_F(OffscreenPresentationManagerTest, UnregisterControllerOnly) {
UnregisterControllerFromConnectedPresentation

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

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:214:
offscreen_presentation_manager_->RegisterOffscreenPresentationController(
Don't we need to check the started presentation is an offscreen presentation
before calling this?

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:272:
offscreen_presentation_manager_->UnregisterOffscreenPresentationController(
Something to think about: how do we tell the receiver side of the
PresentationConnection that the controller side has closed the connection and
vice versa? i.e. the receiver PresentationConnection should get a state change
to CLOSED.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/presentation_service_delegate_impl.h (right):

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.h:133:
state_changed_cb) override;
nit: add back the empty line to separate PSD implementation methods from the
others.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.h:167: void
RegisterReceiverAvailableCallback(
Move these up to group them with the rest of PSD methods.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.h:169:
receiver_available_callback) override {}
Add a NOTREACHED() since we don't expect this to be called in PSDImpl. Also move
the definition to the cc file.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.h:174:
content::OffscreenPresentationClient*) override;
Please name the client parameter

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

https://codereview.chromium.org/2343013002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:184: if (delegate_) {
1) We might as well call these in the PSImpl constructor... I don't see a reason
why it needs to be called here.
2) Maybe we can add a is_receiver() method on PSD. It can be used here to
determine whether RegisterReceiverAvailableCallback or
RegisterOffscreenPresentationClient should be called, instead of calling both.

https://codereview.chromium.org/2343013002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:441: for (const auto&
iter : offscreen_presentation_observers_)
This probably doesn't make sense until we have extracted CloseConnection into
part of the PresentationConnection mojo interface since we want to close the
corresponding connection on the other side, not all of them.

https://codereview.chromium.org/2343013002/diff/20001/content/browser/present...
File content/browser/presentation/presentation_service_impl.h (right):

https://codereview.chromium.org/2343013002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:239: void
OnReceiverConnectionAvailable(const content::PresentationSessionInfo&,
nit: add parameter name and add comment for this method.

https://codereview.chromium.org/2343013002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:243: void
SetOffscreenPresentationClient(OffscreenPresentationClient*) override;
nit: add parameter name here and below

https://codereview.chromium.org/2343013002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:243: void
SetOffscreenPresentationClient(OffscreenPresentationClient*) override;
AddOffscreenPresentationClient?

https://codereview.chromium.org/2343013002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:244: 
rm blank line

https://codereview.chromium.org/2343013002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:301:
std::set<OffscreenPresentationClient*> offscreen_presentation_observers_;
suggestion: offscreen_presentation_clients_

https://codereview.chromium.org/2343013002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:301:
std::set<OffscreenPresentationClient*> offscreen_presentation_observers_;
nit: Place this field before render_process_id_. Please also document ownership
of these clients, i.e., they are not owned by this object.

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

https://codereview.chromium.org/2343013002/diff/20001/content/public/browser/...
content/public/browser/presentation_service_delegate.h:57: virtual void
SetOffscreenPresentationClient(OffscreenPresentationClient*) = 0;
param name.

https://codereview.chromium.org/2343013002/diff/20001/content/public/browser/...
content/public/browser/presentation_service_delegate.h:57: virtual void
SetOffscreenPresentationClient(OffscreenPresentationClient*) = 0;
Please document ownership of the input client.

https://codereview.chromium.org/2343013002/diff/20001/content/public/browser/...
content/public/browser/presentation_service_delegate.h:60:
OffscreenPresentationClient*) = 0;
param name

https://codereview.chromium.org/2343013002/diff/20001/content/public/browser/...
content/public/browser/presentation_service_delegate.h:63:
DISALLOW_COPY_AND_ASSIGN(OffscreenPresentationClient);
This is not needed for interfaces.

https://codereview.chromium.org/2343013002/diff/20001/content/public/browser/...
content/public/browser/presentation_service_delegate.h:245:
OffscreenPresentationClient*) = 0;
param name

[zhaobin, 2016-09-29 17:20:45]

Hi Derek,

Thanks a lot for the comments! I have addressed most of them in new patch
(https://codereview.chromium.org/2379703002/). Please review the new CR. This
one is going to be closed soon. Sorry for the inconvenience.

Bin

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

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/offscreen_tab.cc:125: #if
defined(ENABLE_MEDIA_ROUTER)
On 2016/09/28 07:28:35, imcheng wrote:
> nit: Remove the #if

Done.

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

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.cc:63: if (it !=
offscreen_presentations_.end())
On 2016/09/28 07:28:35, imcheng wrote:
> Does the first RegisterOffscreenPresentationController for an offscreen
> presentation always occur before RegisterOffscreenPresentationReceiver? Or
would
> it be ok to assume that the they can occur in any order?

They are async so they can occur in any order. Generally
RegisterOffscreenPresentationReceiver occurs later because starting a new render
process takes some time.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.cc:70:
offscreen_presentations_.erase(presentation_id);
On 2016/09/28 07:28:35, imcheng wrote:
> question: how do we inform controllers that the receiver has left?

Receiver PSImpl should have controller's PSImpl (offscreenPresentationClient
API) when unregisterReceiver() is called. Plan to notify controller in
receiver's PSImpl (reset() maybe).

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.cc:85:
content::PresentationSessionInfo(presentation_url_, presentation_id_),
On 2016/09/28 07:28:35, imcheng wrote:
> Looks like we also need to pass in presentation_url to this class?

Done.

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

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:10: #include <set>
On 2016/09/28 07:28:36, imcheng wrote:
> <set> and <vector> not used?

Done.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:26: // receiver of
an offscreen presentation.
On 2016/09/28 07:28:36, imcheng wrote:
> nit: this can go in previous line.

Done.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:69: // with
On 2016/09/28 07:28:36, imcheng wrote:
> nit: line wrap

Done.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:92: int
render_frame_id,
On 2016/09/28 07:28:36, imcheng wrote:
> Please use RenderFrameHostId here and below for all |render_frame_id|. See:
>
https://cs.chromium.org/chromium/src/chrome/browser/media/router/presentation...

Done.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:93:
content::OffscreenPresentationClient* controller);
On 2016/09/28 07:28:36, imcheng wrote:
> Can you forward declare OffscreenPresentationClient?

changed to content::PresentationConnectionPtr controller in new patch

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:129: class
OffscreenPresentation {
On 2016/09/28 07:28:36, imcheng wrote:
> It looks like this class is not accessed from outside the .cc and the unit
test.
> Maybe make this a private nested class of OPM?

Done.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:158: std::map<int,
content::OffscreenPresentationClient*> controllers_;
On 2016/09/28 07:28:36, imcheng wrote:
> RenderFrameHostId

Done.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager.h:158: std::map<int,
content::OffscreenPresentationClient*> controllers_;
On 2016/09/28 07:28:36, imcheng wrote:
> nit: I'd probably name it pending_controllers_ to be clear.

Done.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/offscreen_presentation_manager_factory.h
(right):

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_factory.h:19: class
OffscreenPresentationManagerFactory
On 2016/09/28 07:28:36, imcheng wrote:
> Please add a comment here that OffscreenPresentation manager is shared between
a
> Profile and its associated incognito Profiles.

Done.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_factory.h:23:
content::BrowserContext*);
On 2016/09/28 07:28:36, imcheng wrote:
> nit: Please add parameter names here and below

Done.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/offscreen_presentation_manager_unittest.cc
(right):

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_unittest.cc:35: void
verifyPresentationsSize(size_t expected);
On 2016/09/28 07:28:36, imcheng wrote:
> nit: VerifyNumPresentations

Done.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_unittest.cc:36: void
verifyControllerSize(size_t expected,
On 2016/09/28 07:28:36, imcheng wrote:
> nit: VerifyNumControllers

Done.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_unittest.cc:50: void
OffscreenPresentationManagerTest::verifyPresentationsSize(
On 2016/09/28 07:28:36, imcheng wrote:
> These helper methods can be inlined.

Done.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_unittest.cc:62:
RegisterControllerCreateNewPresentation) {
On 2016/09/28 07:28:36, imcheng wrote:
> nit: I would just name the test RegisterController

Done.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_unittest.cc:69:
TEST_F(OffscreenPresentationManagerTest, RegisterOneReceiverDoNothing) {
On 2016/09/28 07:28:36, imcheng wrote:
> RegisterReceiver

Done.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_unittest.cc:78:
TEST_F(OffscreenPresentationManagerTest, UnregisterControllerNoCrash) {
On 2016/09/28 07:28:36, imcheng wrote:
> UnregisterNonexistentController

Done.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_unittest.cc:79:
MockOffscreenPresentationClient controller;
On 2016/09/28 07:28:36, imcheng wrote:
> Unused?

Done.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_unittest.cc:84:
TEST_F(OffscreenPresentationManagerTest, UnregisterReceiverNoCrash) {
On 2016/09/28 07:28:36, imcheng wrote:
> UnregisterNonexistentReceiver

Done.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_unittest.cc:90:
RegisterControllerCreatePresentationWithTwoControllers) {
On 2016/09/28 07:28:36, imcheng wrote:
> RegisterMultipleControllersSamePresentation ?

Done.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_unittest.cc:101:
RegisterControllerCreateTwoPresentations) {
On 2016/09/28 07:28:36, imcheng wrote:
> RegisterMultipleControllersDifferentPresentations

Done.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_unittest.cc:112:
RegisterControllerAndReceiverFireOnConnectionAvailable) {
On 2016/09/28 07:28:36, imcheng wrote:
> RegisterControllerThenReceiverInvokesCallback ?

Done.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_unittest.cc:117:
EXPECT_CALL(receiver_callback, OnReceiverConnectionAvailable(_, &controller));
On 2016/09/28 07:28:36, imcheng wrote:
> Move this EXPECT to just before RegisterOffscreenPresentationReceiver. Same
for
> below.

Done.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_unittest.cc:127:
TEST_F(OffscreenPresentationManagerTest, RegisterUnregisterController) {
On 2016/09/28 07:28:36, imcheng wrote:
> Combine this case with RegisterController

Done.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_unittest.cc:135:
TEST_F(OffscreenPresentationManagerTest, UnregisterReceiverOnly) {
On 2016/09/28 07:28:36, imcheng wrote:
> UnregisterReceiverFromConnectedPresentation

Done.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/offscreen_presentation_manager_unittest.cc:151:
TEST_F(OffscreenPresentationManagerTest, UnregisterControllerOnly) {
On 2016/09/28 07:28:36, imcheng wrote:
> UnregisterControllerFromConnectedPresentation

Done.

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

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:214:
offscreen_presentation_manager_->RegisterOffscreenPresentationController(
On 2016/09/28 07:28:37, imcheng wrote:
> Don't we need to check the started presentation is an offscreen presentation
> before calling this?

Yes, we need to check if it is offscreen presentation.

Would it still be a problem if we change to PresentationConnectionProxy
approach? We do not create PresentationConnection for MR session, right?

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:272:
offscreen_presentation_manager_->UnregisterOffscreenPresentationController(
On 2016/09/28 07:28:37, imcheng wrote:
> Something to think about: how do we tell the receiver side of the
> PresentationConnection that the controller side has closed the connection and
> vice versa? i.e. the receiver PresentationConnection should get a state change
> to CLOSED.

Original plan: In receiver's PSImpl::CloseConnection(), it can invoke
offscreen_presentation_client_ (controller's PSImpl)->OnConnectionStateChanged()
to notify connection state change.

OPM will not handle connection state change.

Code has been removed in new patch.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/presentation_service_delegate_impl.h (right):

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.h:133:
state_changed_cb) override;
On 2016/09/28 07:28:37, imcheng wrote:
> nit: add back the empty line to separate PSD implementation methods from the
> others.

Done.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.h:167: void
RegisterReceiverAvailableCallback(
On 2016/09/28 07:28:37, imcheng wrote:
> Move these up to group them with the rest of PSD methods.

Done.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.h:169:
receiver_available_callback) override {}
On 2016/09/28 07:28:37, imcheng wrote:
> Add a NOTREACHED() since we don't expect this to be called in PSDImpl. Also
move
> the definition to the cc file.

Done.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.h:174:
content::OffscreenPresentationClient*) override;
On 2016/09/28 07:28:37, imcheng wrote:
> Please name the client parameter

Done.

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

https://codereview.chromium.org/2343013002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:184: if (delegate_) {
On 2016/09/28 07:28:37, imcheng wrote:
> 1) We might as well call these in the PSImpl constructor... I don't see a
reason
> why it needs to be called here.
> 2) Maybe we can add a is_receiver() method on PSD. It can be used here to
> determine whether RegisterReceiverAvailableCallback or
> RegisterOffscreenPresentationClient should be called, instead of calling both.

1) RegisterReceiverAvailableCallback() invokes
dispatcher_->OnReceiverConnectionAvailable() if there is any pending controller.
dispatcher_ is null in constructor. It will make OffscreenPresentationManager
code complicated to handle that.

2) delegate_->RegisterOffscreenPresentationClient() removed in new patch

https://codereview.chromium.org/2343013002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:441: for (const auto&
iter : offscreen_presentation_observers_)
On 2016/09/28 07:28:37, imcheng wrote:
> This probably doesn't make sense until we have extracted CloseConnection into
> part of the PresentationConnection mojo interface since we want to close the
> corresponding connection on the other side, not all of them.

code removed in new patch

https://codereview.chromium.org/2343013002/diff/20001/content/browser/present...
File content/browser/presentation/presentation_service_impl.h (right):

https://codereview.chromium.org/2343013002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:239: void
OnReceiverConnectionAvailable(const content::PresentationSessionInfo&,
On 2016/09/28 07:28:37, imcheng wrote:
> nit: add parameter name and add comment for this method.

Done.

https://codereview.chromium.org/2343013002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:243: void
SetOffscreenPresentationClient(OffscreenPresentationClient*) override;
On 2016/09/28 07:28:37, imcheng wrote:
> nit: add parameter name here and below

Function has been removed in new patch.

https://codereview.chromium.org/2343013002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:243: void
SetOffscreenPresentationClient(OffscreenPresentationClient*) override;
On 2016/09/28 07:28:37, imcheng wrote:
> nit: add parameter name here and below

code removed in new patch

https://codereview.chromium.org/2343013002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:244: 
On 2016/09/28 07:28:37, imcheng wrote:
> rm blank line

Done.

https://codereview.chromium.org/2343013002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:301:
std::set<OffscreenPresentationClient*> offscreen_presentation_observers_;
On 2016/09/28 07:28:37, imcheng wrote:
> suggestion: offscreen_presentation_clients_

object removed in new patch

https://codereview.chromium.org/2343013002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:301:
std::set<OffscreenPresentationClient*> offscreen_presentation_observers_;
On 2016/09/28 07:28:37, imcheng wrote:
> nit: Place this field before render_process_id_. Please also document
ownership
> of these clients, i.e., they are not owned by this object.

object removed in new patch

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

https://codereview.chromium.org/2343013002/diff/20001/content/public/browser/...
content/public/browser/presentation_service_delegate.h:57: virtual void
SetOffscreenPresentationClient(OffscreenPresentationClient*) = 0;
On 2016/09/28 07:28:37, imcheng wrote:
> param name.

code removed in new patch

https://codereview.chromium.org/2343013002/diff/20001/content/public/browser/...
content/public/browser/presentation_service_delegate.h:57: virtual void
SetOffscreenPresentationClient(OffscreenPresentationClient*) = 0;
On 2016/09/28 07:28:37, imcheng wrote:
> param name.

code removed in new patch

https://codereview.chromium.org/2343013002/diff/20001/content/public/browser/...
content/public/browser/presentation_service_delegate.h:60:
OffscreenPresentationClient*) = 0;
On 2016/09/28 07:28:37, imcheng wrote:
> param name

code removed in new patch

https://codereview.chromium.org/2343013002/diff/20001/content/public/browser/...
content/public/browser/presentation_service_delegate.h:63:
DISALLOW_COPY_AND_ASSIGN(OffscreenPresentationClient);
On 2016/09/28 07:28:37, imcheng wrote:
> This is not needed for interfaces.

code removed in new patch

https://codereview.chromium.org/2343013002/diff/20001/content/public/browser/...
content/public/browser/presentation_service_delegate.h:245:
OffscreenPresentationClient*) = 0;
On 2016/09/28 07:28:37, imcheng wrote:
> param name

Done.

[mark a. foltz, 2016-10-04 00:32:20]

Sending a few comments now.  It looks like the current PS doesn't reflect
changes to resolve Derek's review comments. Can you please upload a current PS
and I'll finish the review?

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:3007: //
ReceiverPresentationServiceDelegateImpl are for WebContents created for
...exist only for WebContents

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:3008: // offscreen
presentations. The WebContents is from an incognito profile.
The WebContents must belong to an...

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:3009: if (auto* impl =
media_router::ReceiverPresentationServiceDelegateImpl::
Does this imply that the same WebContents cannot be a presentation receiver and
a presentation controller?

This is not something we plan on supporting in the near term, but might be worth
adding to the note above.

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

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/offscreen_tab.cc:9: #include <utility>
These #includes don't seem to be used in your patch - is this to fix missing
#includes?

It looks like this should #include <vector> as well.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/offscreen_tab.cc:105: << " Register
with PresentationRouter, id="
Did you mean ReceiverPresentationServiceDelegateImpl instead of
PresentationRouter?

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/offscreen_tab.cc:123: // the
PresentationRouter.  http://crbug.com/513859
s/PresentationRouter/ReceiverPresentationServiceDelegateImpl/

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/offscreen_tab.cc:125: #if
defined(ENABLE_MEDIA_ROUTER)
On 2016/09/29 at 17:20:42, zhaobin wrote:
> On 2016/09/28 07:28:35, imcheng wrote:
> > nit: Remove the #if
> 
> Done.

Still not done (did you upload the most recent patchset?)

[zhaobin, 2016-10-04 02:43:00]

Hi Mark,

  Sorry for the confusion. This CL is obsolete and I have addressed Derek's
comments and your comments in this new CL
(https://codereview.chromium.org/2379703002/).

  (Not sure how to port Derek's comments over. Would it be easier to review if I
port the changes in new CL back here?)

  In this CL, controller's PresentationServiceImpl holds ptr to receiver's
PresenatationServiceImpl.

  In the new CL, controller's PresentationConnectionProxy holds ptr to
receiver's PresenatationConnectionProxy.

Bin

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:3007: //
ReceiverPresentationServiceDelegateImpl are for WebContents created for
On 2016/10/04 00:32:20, mark a. foltz wrote:
> ...exist only for WebContents

Done.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:3008: // offscreen
presentations. The WebContents is from an incognito profile.
On 2016/10/04 00:32:20, mark a. foltz wrote:
> The WebContents must belong to an...

Done.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:3009: if (auto* impl =
media_router::ReceiverPresentationServiceDelegateImpl::
On 2016/10/04 00:32:20, mark a. foltz wrote:
> Does this imply that the same WebContents cannot be a presentation receiver
and
> a presentation controller?
> 
> This is not something we plan on supporting in the near term, but might be
worth
> adding to the note above.

Done.

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

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/offscreen_tab.cc:105: << " Register
with PresentationRouter, id="
On 2016/10/04 00:32:20, mark a. foltz wrote:
> Did you mean ReceiverPresentationServiceDelegateImpl instead of
> PresentationRouter?

Done.

https://codereview.chromium.org/2343013002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/offscreen_tab.cc:123: // the
PresentationRouter.  http://crbug.com/513859
On 2016/10/04 00:32:20, mark a. foltz wrote:
> s/PresentationRouter/ReceiverPresentationServiceDelegateImpl/

Done.

[mark a. foltz, 2016-10-04 16:10:17]

Ah okay, I'll pick up the review there.  Maybe close this issue then?