[Presentation API] (1st) (1-UA) Add is_offscreen_presentation attribute to MediaRouter

[Presentation API] Add is_offscreen_presentation attribute to MediaRouter

(Splitting of https://codereview.chromium.org/2379703002/)

To support 1-UA mode, we need to tell if a route is created for 1-UA or not.

  - add is_offscreen_presentation attribute to MediaRouter (attribute set in extension)
  - pass route.is_offscreen_presentation from MR extension to MR

We are going to use route.is_offscreen_presentation in PresentationServiceDelegateImpl.

Component Extension side change has been submitted (https://critique.corp.google.com/#review/136174920)

1-UA Design Doc (use chromium.org account): https://docs.google.com/document/d/1XM3jhMJTQyhEC5PDAAJFNIaKh6UUEihqZDz_ztEe4Co/edit#heading=h.hadpx5oi0gml

BUG=525660
Committed: https://crrev.com/72f858dd3c39b90f2c18591dfab290475c1e0ab1
Cr-Commit-Position: refs/heads/master@{#433244}

[zhaobin, 2016-11-02 21:26:58]

The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 21:27:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-03 01:09:22]

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

[commit-bot: I haz the power, 2016-11-03 01:09:22]

Dry run: This issue passed the CQ dry run.

[zhaobin, 2016-11-03 01:31:13]

Patchset #1 (id:1) has been deleted

[zhaobin, 2016-11-03 17:33:32]

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

[zhaobin, 2016-11-03 17:33:33]

It is part of 1-UA messaging change. Split it into smaller pieces for easier
review and landing.

[mark a. foltz, 2016-11-03 18:32:05]

Overall looks good with minor comments.

Please update test coverage in:

media_route_unittest.cc
presentation_service_delegate_impl_unittest.cc
presentation_service_impl_unittest.cc

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

https://codereview.chromium.org/2471883002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/create_presentation_connection_request.h:63: const
MediaRoute& route);
Thanks for doing this, it makes a lot more sense to pass objects instead of IDs
and we should get rid of MediaRoute::Id in our APIs.

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

https://codereview.chromium.org/2471883002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:768:
route_id);
Would be nice to pass |route| here instead of |route_id|.  Optional cleanup to
do in this patch.

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

https://codereview.chromium.org/2471883002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:300: DVLOG(2) <<
"PresentationServiceImpl::OnStartSessionSucceeded"
Please remove log statements here and below.  It's okay to log in one place for
debugging, but extra logging in several places slows down debug builds and
tests.

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

https://codereview.chromium.org/2471883002/diff/20001/content/public/browser/...
content/public/browser/presentation_session.h:36: PresentationSessionInfo(const
GURL& presentation_url,
I would prefer to update the existing constructor, versus adding a new
constructor in this case.  This way the caller has to be explicit about creating
a 1-UA or 2-UA presentation.

[mark a. foltz, 2016-11-03 18:42:17]

One side comment (from a design/architecture POV). In my opinion, long term we
should try to get rid of plumbing is_offscreen into content.  It's really a
browser implementation detail whether the connection is implemented by a
connection to an offscreen presentation or a connection to an external device. 
However since we have two different mechanisms in content/ for talking to them
via PresentationService we'll need this flag.   Eventually I'd like to hide this
detail in the browser (by having the MRPM implement the other side of the
PresentationConnection), but this is a significant architectural change.

[zhaobin, 2016-11-03 23:00:47]

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

https://codereview.chromium.org/2471883002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/create_presentation_connection_request.h:63: const
MediaRoute& route);
On 2016/11/03 18:32:05, mark a. foltz wrote:
> Thanks for doing this, it makes a lot more sense to pass objects instead of
IDs
> and we should get rid of MediaRoute::Id in our APIs.

Will do it in a seperate CL. Create
https://bugs.chromium.org/p/chromium/issues/detail?id=662208# for tracking.

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

https://codereview.chromium.org/2471883002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:768:
route_id);
On 2016/11/03 18:32:05, mark a. foltz wrote:
> Would be nice to pass |route| here instead of |route_id|.  Optional cleanup to
> do in this patch.

Will do it in a seperate CL. |route_id| used in lots of place, create a bug to
track this: https://bugs.chromium.org/p/chromium/issues/detail?id=662208#

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

https://codereview.chromium.org/2471883002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:300: DVLOG(2) <<
"PresentationServiceImpl::OnStartSessionSucceeded"
On 2016/11/03 18:32:05, mark a. foltz wrote:
> Please remove log statements here and below.  It's okay to log in one place
for
> debugging, but extra logging in several places slows down debug builds and
> tests.

Done.

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

https://codereview.chromium.org/2471883002/diff/20001/content/public/browser/...
content/public/browser/presentation_session.h:36: PresentationSessionInfo(const
GURL& presentation_url,
On 2016/11/03 18:32:05, mark a. foltz wrote:
> I would prefer to update the existing constructor, versus adding a new
> constructor in this case.  This way the caller has to be explicit about
creating
> a 1-UA or 2-UA presentation. 

Done.

[zhaobin, 2016-11-04 23:03:05]

Description was changed from

==========
[Presentation API] Add is_offscreen_presentation attribute to MediaRouter

(Splitting of https://codereview.chromium.org/2379703002/)

To support 1-UA mode, we need to tell if a route is created for 1-UA or not.

  - add is_offscreen_presentation attribute to MediaRouter (attribute set in
extension)
  - add is_offscreen attribute to PresentationSessionInfo
  - pass route.is_offscreen_presentation from MR extension to MR
  - use route.is_offscreen_presentation when creating PresentationSessionInfo

Component Extension side change has been submitted
(https://critique.corp.google.com/#review/136174920)

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

to

==========
[Presentation API] Add is_offscreen_presentation attribute to MediaRouter

(Splitting of https://codereview.chromium.org/2379703002/)

To support 1-UA mode, we need to tell if a route is created for 1-UA or not.

  - add is_offscreen_presentation attribute to MediaRouter (attribute set in
extension)
  - add is_offscreen attribute to PresentationSessionInfo
  - pass route.is_offscreen_presentation from MR extension to MR
  - use route.is_offscreen_presentation when creating PresentationSessionInfo

Component Extension side change has been submitted
(https://critique.corp.google.com/#review/136174920)

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

[imcheng, 2016-11-07 21:43:00]

lgtm

[mark a. foltz, 2016-11-09 19:28:23]

Mostly minor comments; but would definitely prefer a solution that doesn't
require plumbing a new flag end-to-end :)

https://codereview.chromium.org/2471883002/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_route.h (right):

https://codereview.chromium.org/2471883002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_route.h:83: void
set_is_offscreen_presentation(bool is_offscreen_presentation) {
Can this be set_offscreen_presentation()?

https://codereview.chromium.org/2471883002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_route.h:99: bool incognito_;
Side comment: Looking at the rest of this object, this should be is_incognito_
and the getter is_incognito(); my fault :)

Not necessary to fix in this patch at all.

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

https://codereview.chromium.org/2471883002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:734: const
GURL& presentation_url,
Thanks for doing this; it is necessary for supporting multiple URLs per request
:)

https://codereview.chromium.org/2471883002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:832:
base::Bind(&PresentationServiceDelegateImpl::OnJoinRouteResponse,
It would be ideal to also include the presentation_id in the arguments bound to
the callback so we can verify that the presentation id passed in the result
matches.

[zhaobin, 2016-11-11 18:41:51]

Resolving minor comments. Thinking about getting rid of the is_offscreen flag:

For the is_offscreen flag in presentation.mojom, it seems that we need to keep
it till we can have a unified PresentationConnectionProxy impl for both 1-UA and
2-UA.

For is_offscreen_presentation flag in media_router.mojom, is it possible to
determine if a presentation is offscreen or not in controller render frame's
PSImpl::StartSession (e.g. solely based on presentation_url, no MR, OPM, MRP, or
receiver frame PSImpl are involved). If yes, we can set the flag in PSImpl and
get rid of it in media_router.mojom. If not and only MRP can decide if a
presentation is offscreen or not, we might to keep the flag.

https://codereview.chromium.org/2471883002/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_route.h (right):

https://codereview.chromium.org/2471883002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_route.h:83: void
set_is_offscreen_presentation(bool is_offscreen_presentation) {
On 2016/11/09 19:28:22, mark a. foltz wrote:
> Can this be set_offscreen_presentation()?

Done.

https://codereview.chromium.org/2471883002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_route.h:99: bool incognito_;
On 2016/11/09 19:28:22, mark a. foltz wrote:
> Side comment: Looking at the rest of this object, this should be is_incognito_
> and the getter is_incognito(); my fault :)
> 
> Not necessary to fix in this patch at all.

Will do it later in seperate patch.

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

https://codereview.chromium.org/2471883002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:734: const
GURL& presentation_url,
On 2016/11/09 19:28:23, mark a. foltz wrote:
> Thanks for doing this; it is necessary for supporting multiple URLs per
request
> :)

Done.

https://codereview.chromium.org/2471883002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:832:
base::Bind(&PresentationServiceDelegateImpl::OnJoinRouteResponse,
On 2016/11/09 19:28:23, mark a. foltz wrote:
> It would be ideal to also include the presentation_id in the arguments bound
to
> the callback so we can verify that the presentation id passed in the result
> matches.

Done.

[zhaobin, 2016-11-16 02:14:28]

Removed is_offscreen attribute from PresentationSessionInfo class (get rid of
plumbing is_offscreen into content).

[imcheng, 2016-11-17 19:42:23]

Still lgtm.

Looking at https://codereview.chromium.org/2379703002/, the
PresentationConnection wiring is initiated from the controller
PresentationDispatcher. Without the offscreen presentation bit on the
PresentationSessionInfo, we will have to rearrange where we start the wiring of
PresentationConnection.

I would still prefer to do it in PresentationServiceDelegate /
OffscreenPresentationManager. This is probably do-able with the refactoring of
non-1UA presentations to use the same PresentationConnection mojo interface. But
I am open to other ideas.

[mark a. foltz, 2016-11-17 21:05:42]

lgtm

[mark a. foltz, 2016-11-17 21:06:48]

Before committing:

- Please update patch description to match current design.
- You'll need ipc/OWNERS approval for the mojom changes.  dcheng@ has reviewed
other patches of ours.

[mark a. foltz, 2016-11-17 21:07:08]

On 2016/11/17 at 21:06:48, mark a. foltz wrote:
> Before committing:
> 
> - Please update patch description to match current design.
> - You'll need ipc/OWNERS approval for the mojom changes.  dcheng@ has reviewed
other patches of ours.

Also, you might want to put a design doc link in the patch description.

[zhaobin, 2016-11-17 22:42:40]

Description was changed from

==========
[Presentation API] Add is_offscreen_presentation attribute to MediaRouter

(Splitting of https://codereview.chromium.org/2379703002/)

To support 1-UA mode, we need to tell if a route is created for 1-UA or not.

  - add is_offscreen_presentation attribute to MediaRouter (attribute set in
extension)
  - add is_offscreen attribute to PresentationSessionInfo
  - pass route.is_offscreen_presentation from MR extension to MR
  - use route.is_offscreen_presentation when creating PresentationSessionInfo

Component Extension side change has been submitted
(https://critique.corp.google.com/#review/136174920)

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

to

==========
[Presentation API] Add is_offscreen_presentation attribute to MediaRouter

(Splitting of https://codereview.chromium.org/2379703002/)

To support 1-UA mode, we need to tell if a route is created for 1-UA or not.

  - add is_offscreen_presentation attribute to MediaRouter (attribute set in
extension)
  - pass route.is_offscreen_presentation from MR extension to MR
  
We are going to use route.is_offscreen_presentation in
PresentationServiceDelegateImpl.

Component Extension side change has been submitted
(https://critique.corp.google.com/#review/136174920)

1-UA Design Doc (use chromium.org account):
https://docs.google.com/document/d/1XM3jhMJTQyhEC5PDAAJFNIaKh6UUEihqZDz_ztEe4...

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

[zhaobin, 2016-11-17 22:45:02]

zhaobin@chromium.org changed reviewers:
+ dcheng@chromium.org

[zhaobin, 2016-11-17 22:45:02]

+dcheng@ to review mojo related changes.

[dcheng, 2016-11-17 23:30:31]

mojom lgtm

https://codereview.chromium.org/2471883002/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_type_converters.cc (right):

https://codereview.chromium.org/2471883002/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_type_converters.cc:101:
media_route->set_offscreen_presentation(input->is_offscreen_presentation);
FWIW, type converters are deprecated (I guess I'll add this to the TODOs in
https://crbug.com/632623)

[zhaobin, 2016-11-18 00:55:25]

The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-18 00:56:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-18 04:31:25]

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

[commit-bot: I haz the power, 2016-11-18 04:31:26]

Dry run: This issue passed the CQ dry run.

[zhaobin, 2016-11-18 18:22:24]

The CQ bit was checked by zhaobin@chromium.org

[zhaobin, 2016-11-18 18:22:24]

The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org,
imcheng@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2471883002/#ps120001
(title: "merge with master")

[commit-bot: I haz the power, 2016-11-18 18:23:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-18 18:36:04]

Description was changed from

==========
[Presentation API] Add is_offscreen_presentation attribute to MediaRouter

(Splitting of https://codereview.chromium.org/2379703002/)

To support 1-UA mode, we need to tell if a route is created for 1-UA or not.

  - add is_offscreen_presentation attribute to MediaRouter (attribute set in
extension)
  - pass route.is_offscreen_presentation from MR extension to MR
  
We are going to use route.is_offscreen_presentation in
PresentationServiceDelegateImpl.

Component Extension side change has been submitted
(https://critique.corp.google.com/#review/136174920)

1-UA Design Doc (use chromium.org account):
https://docs.google.com/document/d/1XM3jhMJTQyhEC5PDAAJFNIaKh6UUEihqZDz_ztEe4...

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

to

==========
[Presentation API] Add is_offscreen_presentation attribute to MediaRouter

(Splitting of https://codereview.chromium.org/2379703002/)

To support 1-UA mode, we need to tell if a route is created for 1-UA or not.

  - add is_offscreen_presentation attribute to MediaRouter (attribute set in
extension)
  - pass route.is_offscreen_presentation from MR extension to MR
  
We are going to use route.is_offscreen_presentation in
PresentationServiceDelegateImpl.

Component Extension side change has been submitted
(https://critique.corp.google.com/#review/136174920)

1-UA Design Doc (use chromium.org account):
https://docs.google.com/document/d/1XM3jhMJTQyhEC5PDAAJFNIaKh6UUEihqZDz_ztEe4...

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

[commit-bot: I haz the power, 2016-11-18 18:36:06]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-11-18 18:37:51]

Description was changed from

==========
[Presentation API] Add is_offscreen_presentation attribute to MediaRouter

(Splitting of https://codereview.chromium.org/2379703002/)

To support 1-UA mode, we need to tell if a route is created for 1-UA or not.

  - add is_offscreen_presentation attribute to MediaRouter (attribute set in
extension)
  - pass route.is_offscreen_presentation from MR extension to MR
  
We are going to use route.is_offscreen_presentation in
PresentationServiceDelegateImpl.

Component Extension side change has been submitted
(https://critique.corp.google.com/#review/136174920)

1-UA Design Doc (use chromium.org account):
https://docs.google.com/document/d/1XM3jhMJTQyhEC5PDAAJFNIaKh6UUEihqZDz_ztEe4...

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

to

==========
[Presentation API] Add is_offscreen_presentation attribute to MediaRouter

(Splitting of https://codereview.chromium.org/2379703002/)

To support 1-UA mode, we need to tell if a route is created for 1-UA or not.

  - add is_offscreen_presentation attribute to MediaRouter (attribute set in
extension)
  - pass route.is_offscreen_presentation from MR extension to MR

We are going to use route.is_offscreen_presentation in
PresentationServiceDelegateImpl.

Component Extension side change has been submitted
(https://critique.corp.google.com/#review/136174920)

1-UA Design Doc (use chromium.org account):
https://docs.google.com/document/d/1XM3jhMJTQyhEC5PDAAJFNIaKh6UUEihqZDz_ztEe4...

BUG=525660

Committed: https://crrev.com/72f858dd3c39b90f2c18591dfab290475c1e0ab1
Cr-Commit-Position: refs/heads/master@{#433244}
==========

[commit-bot: I haz the power, 2016-11-18 18:37:53]

Patchset 6 (id:??) landed as
https://crrev.com/72f858dd3c39b90f2c18591dfab290475c1e0ab1
Cr-Commit-Position: refs/heads/master@{#433244}