[Presentation API / Media Router] Clean up default pres URL logic.

[Presentation API / Media Router] Clean up default pres URL logic.

1. In presentation_service.mojom, there were 2 methods for default
presentation: ListenForDefaultSessionStart and SetDefaultPresentationURL.
There is no reason they should be called separately. Furthermore,
ListenForDefaultSessionStart is called after connecting to
PresentationService, which is not optimal. This CL removes the former
method.

2. Remove OnDefaultSessionStarted from PSD::Observer and replace it
with callbacks. The purpose of PSD::Observer is to notify
PresentationServiceImpl that PresentationServiceDelegate is being
destroyed and shouldn't include specific Pres API logic.

3. Fix default request matching logic in PSDImpl to account for all
of (frame, frame url, presentation url). Previously it was only
checking presentation url and always assumes it came from the
current main frame. This is a problem because the main frame of a
WebContents can change over time, i.e. main frame at time of setting
DPU and time of route response could be different). Now we create
a PresentationRequest object to hold all info needed to match default
request and pass it to MRUI, who then binds the request to the route
response callback in PSDImpl/PresentationFrameManager to perform the
matching.

BUG=544259
Committed: https://crrev.com/6104fb062821e3e8584ed2e40cdb3f77502d88f0
Cr-Commit-Position: refs/heads/master@{#358935}

[imcheng, 2015-10-19 20:44:28]

Description was changed from

==========
[Presentation API / Media Router] Clean up default pres URL logic.

1. In presentation_service.mojom, there were 2 methods for default
presentation: ListenForDefaultSessionStart and SetDefaultPresentationURL.
There is no reason they should be called separately. Furthermore,
ListenForDefaultSessionStart is called after connecting to
PresentationService, which is not optimal. This CL removes the former
method.

2. Remove OnDefaultSessionStarted from PSD::Observer and replace it
with callbacks. The purpose of PSD::Observer is to notify
PresentationServiceImpl that PresentationServiceDelegate is being
destroyed and shouldn't include specific Pres API logic.

3. Remove main frame restriction for setting default presentation
URL / request. This means non main frames can now set the "tab level"
default request (which is the one shown in the browser-initiated
dialog) See crbug.com/544259.

4. Fix default request matching logic in PSDImpl to account for all
of (frame, frame url, presentation url). Previously it was only
checking presentation url and always assumes it came from the
current main frame. This is a problem because the main frame of a
WebContents can change over time, i.e. main frame at time of setting
DPU and time of route response could be different). Now we create
a PresentationRequest object to hold all info needed to match default
request and pass it to MRUI, who then binds the request to the route
response callback in PSDImpl to perform the matching.

TODO: write tests 

BUG=544259
==========

to

==========
[Presentation API / Media Router] Clean up default pres URL logic.

1. In presentation_service.mojom, there were 2 methods for default
presentation: ListenForDefaultSessionStart and SetDefaultPresentationURL.
There is no reason they should be called separately. Furthermore,
ListenForDefaultSessionStart is called after connecting to
PresentationService, which is not optimal. This CL removes the former
method.

2. Remove OnDefaultSessionStarted from PSD::Observer and replace it
with callbacks. The purpose of PSD::Observer is to notify
PresentationServiceImpl that PresentationServiceDelegate is being
destroyed and shouldn't include specific Pres API logic.

3. Remove main frame restriction for setting default presentation
URL / request. This means non main frames can now set the "tab level"
default request (which is the one shown in the browser-initiated
dialog) See crbug.com/544259.

4. Fix default request matching logic in PSDImpl to account for all
of (frame, frame url, presentation url). Previously it was only
checking presentation url and always assumes it came from the
current main frame. This is a problem because the main frame of a
WebContents can change over time, i.e. main frame at time of setting
DPU and time of route response could be different). Now we create
a PresentationRequest object to hold all info needed to match default
request and pass it to MRUI, who then binds the request to the route
response callback in PSDImpl to perform the matching.

BUG=544259
==========

[imcheng, 2015-10-19 21:49:13]

Description was changed from

==========
[Presentation API / Media Router] Clean up default pres URL logic.

1. In presentation_service.mojom, there were 2 methods for default
presentation: ListenForDefaultSessionStart and SetDefaultPresentationURL.
There is no reason they should be called separately. Furthermore,
ListenForDefaultSessionStart is called after connecting to
PresentationService, which is not optimal. This CL removes the former
method.

2. Remove OnDefaultSessionStarted from PSD::Observer and replace it
with callbacks. The purpose of PSD::Observer is to notify
PresentationServiceImpl that PresentationServiceDelegate is being
destroyed and shouldn't include specific Pres API logic.

3. Remove main frame restriction for setting default presentation
URL / request. This means non main frames can now set the "tab level"
default request (which is the one shown in the browser-initiated
dialog) See crbug.com/544259.

4. Fix default request matching logic in PSDImpl to account for all
of (frame, frame url, presentation url). Previously it was only
checking presentation url and always assumes it came from the
current main frame. This is a problem because the main frame of a
WebContents can change over time, i.e. main frame at time of setting
DPU and time of route response could be different). Now we create
a PresentationRequest object to hold all info needed to match default
request and pass it to MRUI, who then binds the request to the route
response callback in PSDImpl to perform the matching.

BUG=544259
==========

to

==========
[Presentation API / Media Router] Clean up default pres URL logic.

1. In presentation_service.mojom, there were 2 methods for default
presentation: ListenForDefaultSessionStart and SetDefaultPresentationURL.
There is no reason they should be called separately. Furthermore,
ListenForDefaultSessionStart is called after connecting to
PresentationService, which is not optimal. This CL removes the former
method.

2. Remove OnDefaultSessionStarted from PSD::Observer and replace it
with callbacks. The purpose of PSD::Observer is to notify
PresentationServiceImpl that PresentationServiceDelegate is being
destroyed and shouldn't include specific Pres API logic.

3. Remove main frame restriction for setting default presentation
URL / request. This means non main frames can now set the "tab level"
default request (which is the one shown in the browser-initiated
dialog) See crbug.com/544259.

4. Fix default request matching logic in PSDImpl to account for all
of (frame, frame url, presentation url). Previously it was only
checking presentation url and always assumes it came from the
current main frame. This is a problem because the main frame of a
WebContents can change over time, i.e. main frame at time of setting
DPU and time of route response could be different). Now we create
a PresentationRequest object to hold all info needed to match default
request and pass it to MRUI, who then binds the request to the route
response callback in PSDImpl/PresentationFrameManager to perform the
matching.

BUG=544259
==========

[imcheng, 2015-10-19 22:01:23]

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

[imcheng, 2015-10-19 22:01:23]

mark: PTAL

[mark a. foltz, 2015-10-20 20:15:25]

Thanks for the mojom and general cleanup around this!

I didn't quite follow the refactoring of
PresentationFrame/PresentationFrameManager.  It seems like there still needs to
be a per-WebContents default request to drive the UI.  How is this request
chosen?  I suggest tracking the top level frame id and using the request from
the corresponding PresentationFrame.

FYI, the spec is still undecided on whether to allow default requests (or use of
the Presentation API at all by default) from child frames.  So we may need to
update this logic over time.

https://github.com/w3c/presentation-api/issues/79

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/an...
File
chrome/browser/media/android/router/media_router_dialog_controller_android.cc
(right):

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/an...
chrome/browser/media/android/router/media_router_dialog_controller_android.cc:46:
const MediaSource::Id source_id(
I actually preferred the previous code, where you call
PresentationRequest::media_source() to get the corresponding source.  What
motivates this change?

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

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/create_presentation_session_request.h:40: //
|erorr_cb|: Callback to invoke when the request fails. Must be valid.
Nit: typo in error_cb

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/create_presentation_session_request.h:46: const
content::PresentationSessionErrorCallback& error_cb);
It feels weird to have the success callback declared in media_router:: and the
error callback declared in content::

I feel like these should both be declared in media_router:: since this object is
also declared there?

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

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_request.h:36: // URL of default
presentation.
Can this be any presentation URL?

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_request.h:40: const GURL frame_url_;
What is this used for?  The frame may have no src=, or may have navigated, etc.
after issuing the request.

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

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:77: // Sets
this frame's default prsentation request and callback.
Typo in presentation request.

Also a slight preference to use the term "PresentationRequest" since there's an
API object that matches this concept.

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:82: // Clear
this frame's default prsentation request and callback.
Typo here

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:93: void
OnDefaultPresentationSessionStarted(
I wonder if this class needs to expose two different code paths for starting
presentation connections (one via .start() and one via the default request).  In
either case you are going to fire a connectionavailable event on the
PresentationRequest; the difference is that you also need to resolve a pending
Promise in response to start().  Blink could use the presentation URL returned
with the connection to match up to the Promise.

Maybe this is the cleanest way w.r.t. Mojo, since start() will have a pending
Mojo callback and the default request will not.

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:153: void
PresentationFrame::OnDefaultPresentationSessionStarted(
Is this called only for browser initiated requests, or also in response to
presentation.defaultRequest.start()?

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:230: 
Extra newline

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:258:
default_presentation_started_callback_ = callback;
Do we need to reset default_presentation_started_callback_ first?  Maybe this
should call ClearDefaultPresentationRequest().

Or, it seems like a single callback is re-used for multiple calls to start the
default request.
Maybe setting the callback and the default request should be separate?

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:341: const
PresentationRequest* default_presentation_request() const {
Which request is this returning?  Isn't there potentially one per frame?

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:488:
ClearDefaultPresentationRequest();
This design keeps track of default requests at two levels: the WebContents level
and the frame level.  It seems a little redundant?

The PresentationFrameManager could keep track of the RenderFrameHostId for the
top level document, and use that to set and retrieve the default request (if
any) for it.

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

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.h:128: //
initiated media router dialog).
I found this comment a little confusing.  Do you mean to say this is called when
the default PresentationRequest is started or resumed ?

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.h:134: // Adds /
removes an observer for listening to default presentation changes.
Nit: default PresentationRequest changes

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:245: ?
GetHostFromURL(presentation_request_->frame_url())
Is frame_url a GURL?  Can you say presentation_request_->frame_url()->host()?

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:285:
DCHECK_IMPLIES(requesting_route_for_default_source_, presentation_request_);
Remove DCHECK since you're handling this below.

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

https://codereview.chromium.org/1406013003/diff/80001/content/public/browser/...
content/public/browser/presentation_service_delegate.h:106: const
PresentationSessionStartedCallback& callback) = 0;
success_cb for consistency?

[imcheng, 2015-10-24 00:39:52]

On 2015/10/20 20:15:25, mfoltz_ooo_until_11_2 wrote:
> Thanks for the mojom and general cleanup around this!
> 
> I didn't quite follow the refactoring of
> PresentationFrame/PresentationFrameManager.  It seems like there still needs
to
> be a per-WebContents default request to drive the UI.  How is this request
> chosen?  I suggest tracking the top level frame id and using the request from
> the corresponding PresentationFrame.
> 

That is, in fact, what PSDImpl was doing before this patch. Unfortunately, it
does not work for sites that set defaultRequest in an iframe, which doesn't seem
like an uncommon case. In this patch, I changed the tab-level default request to
be the most recent frame-level defaultRequest that was set.


> FYI, the spec is still undecided on whether to allow default requests (or use
of
> the Presentation API at all by default) from child frames.  So we may need to
> update this logic over time.
> 
> https://github.com/w3c/presentation-api/issues/79
> 

Yes. One of the objectives of this patch, is to define our tab-level default
request algorithm to be less restrictive, which is hopefully possible with some
breathing room in the spec.

>
https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/an...
> File
> chrome/browser/media/android/router/media_router_dialog_controller_android.cc
> (right):
> 
>
https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/an...
>
chrome/browser/media/android/router/media_router_dialog_controller_android.cc:46:
> const MediaSource::Id source_id(
> I actually preferred the previous code, where you call
> PresentationRequest::media_source() to get the corresponding source.  What
> motivates this change?
> 
>
https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
> File chrome/browser/media/router/create_presentation_session_request.h
(right):
> 
>
https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
> chrome/browser/media/router/create_presentation_session_request.h:40: //
> |erorr_cb|: Callback to invoke when the request fails. Must be valid.
> Nit: typo in error_cb
> 
>
https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
> chrome/browser/media/router/create_presentation_session_request.h:46: const
> content::PresentationSessionErrorCallback& error_cb);
> It feels weird to have the success callback declared in media_router:: and the
> error callback declared in content::
> 
> I feel like these should both be declared in media_router:: since this object
is
> also declared there?
> 
>
https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
> File chrome/browser/media/router/presentation_request.h (right):
> 
>
https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
> chrome/browser/media/router/presentation_request.h:36: // URL of default
> presentation.
> Can this be any presentation URL?
> 
>
https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
> chrome/browser/media/router/presentation_request.h:40: const GURL frame_url_;
> What is this used for?  The frame may have no src=, or may have navigated,
etc.
> after issuing the request.
> 
>
https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
> File chrome/browser/media/router/presentation_service_delegate_impl.cc
(right):
> 
>
https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
> chrome/browser/media/router/presentation_service_delegate_impl.cc:77: // Sets
> this frame's default prsentation request and callback.
> Typo in presentation request.
> 
> Also a slight preference to use the term "PresentationRequest" since there's
an
> API object that matches this concept.
> 
>
https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
> chrome/browser/media/router/presentation_service_delegate_impl.cc:82: // Clear
> this frame's default prsentation request and callback.
> Typo here
> 
>
https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
> chrome/browser/media/router/presentation_service_delegate_impl.cc:93: void
> OnDefaultPresentationSessionStarted(
> I wonder if this class needs to expose two different code paths for starting
> presentation connections (one via .start() and one via the default request). 
In
> either case you are going to fire a connectionavailable event on the
> PresentationRequest; the difference is that you also need to resolve a pending
> Promise in response to start().  Blink could use the presentation URL returned
> with the connection to match up to the Promise.
> 
> Maybe this is the cleanest way w.r.t. Mojo, since start() will have a pending
> Mojo callback and the default request will not.
> 
>
https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
> chrome/browser/media/router/presentation_service_delegate_impl.cc:153: void
> PresentationFrame::OnDefaultPresentationSessionStarted(
> Is this called only for browser initiated requests, or also in response to
> presentation.defaultRequest.start()?
> 
>
https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
> chrome/browser/media/router/presentation_service_delegate_impl.cc:230: 
> Extra newline
> 
>
https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
> chrome/browser/media/router/presentation_service_delegate_impl.cc:258:
> default_presentation_started_callback_ = callback;
> Do we need to reset default_presentation_started_callback_ first?  Maybe this
> should call ClearDefaultPresentationRequest().
> 
> Or, it seems like a single callback is re-used for multiple calls to start the
> default request.
> Maybe setting the callback and the default request should be separate?
> 
>
https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
> chrome/browser/media/router/presentation_service_delegate_impl.cc:341: const
> PresentationRequest* default_presentation_request() const {
> Which request is this returning?  Isn't there potentially one per frame?
> 
>
https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
> chrome/browser/media/router/presentation_service_delegate_impl.cc:488:
> ClearDefaultPresentationRequest();
> This design keeps track of default requests at two levels: the WebContents
level
> and the frame level.  It seems a little redundant?
> 
> The PresentationFrameManager could keep track of the RenderFrameHostId for the
> top level document, and use that to set and retrieve the default request (if
> any) for it.
> 
>
https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
> File chrome/browser/media/router/presentation_service_delegate_impl.h (right):
> 
>
https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
> chrome/browser/media/router/presentation_service_delegate_impl.h:128: //
> initiated media router dialog).
> I found this comment a little confusing.  Do you mean to say this is called
when
> the default PresentationRequest is started or resumed ?
> 
>
https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
> chrome/browser/media/router/presentation_service_delegate_impl.h:134: // Adds
/
> removes an observer for listening to default presentation changes.
> Nit: default PresentationRequest changes
> 
>
https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):
> 
>
https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/media_router/media_router_ui.cc:245: ?
> GetHostFromURL(presentation_request_->frame_url())
> Is frame_url a GURL?  Can you say presentation_request_->frame_url()->host()?
> 
>
https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/media_router/media_router_ui.cc:285:
> DCHECK_IMPLIES(requesting_route_for_default_source_, presentation_request_);
> Remove DCHECK since you're handling this below.
> 
>
https://codereview.chromium.org/1406013003/diff/80001/content/public/browser/...
> File content/public/browser/presentation_service_delegate.h (right):
> 
>
https://codereview.chromium.org/1406013003/diff/80001/content/public/browser/...
> content/public/browser/presentation_service_delegate.h:106: const
> PresentationSessionStartedCallback& callback) = 0;
> success_cb for consistency?

[imcheng, 2015-10-24 00:41:18]

imcheng@chromium.org changed reviewers:
+ mlamouri@chromium.org

[imcheng, 2015-10-24 00:41:19]

+Mounir to review the patch while mfoltz is OOO. PTAL at everything, thanks.

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/an...
File
chrome/browser/media/android/router/media_router_dialog_controller_android.cc
(right):

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/an...
chrome/browser/media/android/router/media_router_dialog_controller_android.cc:46:
const MediaSource::Id source_id(
On 2015/10/20 20:15:25, mfoltz_ooo_until_11_2 wrote:
> I actually preferred the previous code, where you call
> PresentationRequest::media_source() to get the corresponding source.  What
> motivates this change?

It's because now the CreateSessionRequest now holds a PresentationRequest which
holds a presentation url. I will add a helper method to return the MediaSource
to reduce the clutter here?

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

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/create_presentation_session_request.h:40: //
|erorr_cb|: Callback to invoke when the request fails. Must be valid.
On 2015/10/20 20:15:25, mfoltz_ooo_until_11_2 wrote:
> Nit: typo in error_cb

Done.

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/create_presentation_session_request.h:46: const
content::PresentationSessionErrorCallback& error_cb);
On 2015/10/20 20:15:25, mfoltz_ooo_until_11_2 wrote:
> It feels weird to have the success callback declared in media_router:: and the
> error callback declared in content::
> 
> I feel like these should both be declared in media_router:: since this object
is
> also declared there?

I added a type alias.

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

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_request.h:36: // URL of default
presentation.
On 2015/10/20 20:15:25, mfoltz_ooo_until_11_2 wrote:
> Can this be any presentation URL?

Yes. Removed "default".

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_request.h:40: const GURL frame_url_;
On 2015/10/20 20:15:25, mfoltz_ooo_until_11_2 wrote:
> What is this used for?  The frame may have no src=, or may have navigated,
etc.
> after issuing the request.

This is for matching the the default request that was in the dialog with the one
set in PSDImpl. All of (RFH id, pres url, frame url) has to match in order to
trigger default request.

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

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:77: // Sets
this frame's default prsentation request and callback.
On 2015/10/20 20:15:25, mfoltz_ooo_until_11_2 wrote:
> Typo in presentation request.
> 
> Also a slight preference to use the term "PresentationRequest" since there's
an
> API object that matches this concept.

Done.

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:82: // Clear
this frame's default prsentation request and callback.
On 2015/10/20 20:15:25, mfoltz_ooo_until_11_2 wrote:
> Typo here

Done.

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:93: void
OnDefaultPresentationSessionStarted(
On 2015/10/20 20:15:25, mfoltz_ooo_until_11_2 wrote:
> I wonder if this class needs to expose two different code paths for starting
> presentation connections (one via .start() and one via the default request). 
In
> either case you are going to fire a connectionavailable event on the
> PresentationRequest; the difference is that you also need to resolve a pending
> Promise in response to start().  Blink could use the presentation URL returned
> with the connection to match up to the Promise.
> 
> Maybe this is the cleanest way w.r.t. Mojo, since start() will have a pending
> Mojo callback and the default request will not.

This is motivated by the need to pass in a callback from PSImpl to PSDImpl when
the default PresentationRequest is set, so that PSDImpl can notify PSImpl when
the default PresentationRequest is initiated from the browser.

If there is a default PresentationRequest set, and the page calls
defaultRequest.start() and succeeds, then we only answer the pending callback
from StartSession() not the default_presentation_started_callback_ so we don't
fire connectionavailable twice.

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:153: void
PresentationFrame::OnDefaultPresentationSessionStarted(
On 2015/10/20 20:15:25, mfoltz_ooo_until_11_2 wrote:
> Is this called only for browser initiated requests, or also in response to
> presentation.defaultRequest.start()?

Only for browser initiated requests.

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:230: 
On 2015/10/20 20:15:25, mfoltz_ooo_until_11_2 wrote:
> Extra newline

Ok. But added a new line after L228 for the if block.

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:258:
default_presentation_started_callback_ = callback;
On 2015/10/20 20:15:25, mfoltz_ooo_until_11_2 wrote:
> Do we need to reset default_presentation_started_callback_ first?  Maybe this
> should call ClearDefaultPresentationRequest().
> 
> Or, it seems like a single callback is re-used for multiple calls to start the
> default request.
> Maybe setting the callback and the default request should be separate?

The Equals check below is just an optimization. There's no need to reset
default_presentation_started_callback_ first. Both the callback and the request
can be reused so there's no need to clear them after defaultRequest has been
triggered.

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:341: const
PresentationRequest* default_presentation_request() const {
On 2015/10/20 20:15:25, mfoltz_ooo_until_11_2 wrote:
> Which request is this returning?  Isn't there potentially one per frame?

This returns the "tab-level" default presentation request, which is specific for
our ui.

As you mentioned, how default requests from child frames remains unspecified in
the spec. Before this patch we only consider the the default request from the
main frame to be the tab-level default request. This is too restrictive because
some sites (as documented in internal bug) use an iframe to set the default
request, which would get ignored by our algorithm.

The new patch defines tab-level default request as the latest default request
that was set on this tab. This will solve the issue. The only complication I can
think of is if more than one frame sets the default request, but that should be
unlikely.

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:488:
ClearDefaultPresentationRequest();
On 2015/10/20 20:15:25, mfoltz_ooo_until_11_2 wrote:
> This design keeps track of default requests at two levels: the WebContents
level
> and the frame level.  It seems a little redundant?
> 
> The PresentationFrameManager could keep track of the RenderFrameHostId for the
> top level document, and use that to set and retrieve the default request (if
> any) for it.

This patch fixes the issue where the default request is not set at the top level
document. It does so by changing the tab-level default request algorithm to also
consider defaultRequest from other frames (i.e., whichever frame-level
defaultRequest is seen last is the tab-level default request)

I was thinking in the future we could consider enumerating all default requests
in our ui, which is why I also added frame-level tracking. I removed it for the
time being and moved the callback logic to PFM.

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

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.h:128: //
initiated media router dialog).
On 2015/10/20 20:15:25, mfoltz_ooo_until_11_2 wrote:
> I found this comment a little confusing.  Do you mean to say this is called
when
> the default PresentationRequest is started or resumed ?

Well this method is only called on response to the default presentation request
on the UI, but the UI has to be browser-initiated and not due to a start() call.
I updated the comments, WDYT?

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.h:134: // Adds /
removes an observer for listening to default presentation changes.
On 2015/10/20 20:15:25, mfoltz_ooo_until_11_2 wrote:
> Nit: default PresentationRequest changes

Done.

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:245: ?
GetHostFromURL(presentation_request_->frame_url())
On 2015/10/20 20:15:25, mfoltz_ooo_until_11_2 wrote:
> Is frame_url a GURL?  Can you say presentation_request_->frame_url()->host()?

There's some additional truncating logic in GetHostFromURL().

https://codereview.chromium.org/1406013003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:285:
DCHECK_IMPLIES(requesting_route_for_default_source_, presentation_request_);
On 2015/10/20 20:15:25, mfoltz_ooo_until_11_2 wrote:
> Remove DCHECK since you're handling this below.

Below is how I would handle it in release builds. For debug build it would be
better to fail early IMO.

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

https://codereview.chromium.org/1406013003/diff/80001/content/public/browser/...
content/public/browser/presentation_service_delegate.h:106: const
PresentationSessionStartedCallback& callback) = 0;
On 2015/10/20 20:15:25, mfoltz_ooo_until_11_2 wrote:
> success_cb for consistency?

Since there's no error_cb, I don't think we should call it success_cb.

[imcheng, 2015-10-26 20:41:03]

Ping for Mounir.

[mlamouri (slow - plz ping), 2015-10-27 18:42:52]

It's a big CL so I need to digest it. I left a few comments below. I have one
general design question though. I can't find a RenderFrameObserver or
WebContentsObserver that handles the lifecycle of the frame. What happens if you
attach a frame with a default presentation URL which then get detached from the
WebContents? Will the default presentation URL reset to the empty string?

https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/a...
File
chrome/browser/media/android/router/media_router_dialog_controller_android.cc
(right):

https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/a...
chrome/browser/media/android/router/media_router_dialog_controller_android.cc:42:
TakePresentationRequest());
I think the name |create_session_request| is very confusing. Could you rename
this |presentation_request_creator| or something similar so it doesn't look like
a function.

Also, please initialize with = instead of () if possible:
  scoped_ptr<T> foo = TakeFoo();

https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/create_presentation_session_request.h (right):

https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/create_presentation_session_request.h:30: class
CreatePresentationSessionRequest {
This class name is really confusing.

https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_dialog_controller.h (right):

https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_dialog_controller.h:70: const
CreatePresentationSessionRequest* create_session_request() const {
That name doesn't look like a getter but like a function. I think the object and
member names need to change.

https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/presentation_request.h (right):

https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/presentation_request.h:25: bool Equals(const
PresentationRequest& other) const;
I think you should use operator==.

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

https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl.h:56: virtual
~DefaultPresentationRequestObserver() {}
nit: = default;

https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/render_frame_host_id.h (right):

https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/render_frame_host_id.h:12: using RenderFrameHostId =
std::pair<int, int>;
I would prefer if you could use render_frame_id and render_process_id instead of
creating this for consistency with the current code. This said, I agree that we
need such type.

[imcheng, 2015-10-27 20:50:00]

On 2015/10/27 18:42:52, Mounir Lamouri wrote:
> It's a big CL so I need to digest it. I left a few comments below. I have one
> general design question though. I can't find a RenderFrameObserver or
> WebContentsObserver that handles the lifecycle of the frame. What happens if
you
> attach a frame with a default presentation URL which then get detached from
the
> WebContents? Will the default presentation URL reset to the empty string?
> 

Thanks for taking the time to review the CL!

PresentationServiceImpl inherits from WebContentsObserver. When it observes
changes to the corresponding frame (e.g., navigation), it will invoke
PresentationServiceDelegate::Reset(), and that will cause
PresentationServiceDelegateImpl to clear the default presentation URL, among
other data for that frame.



>
https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/a...
> File
> chrome/browser/media/android/router/media_router_dialog_controller_android.cc
> (right):
> 
>
https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/a...
>
chrome/browser/media/android/router/media_router_dialog_controller_android.cc:42:
> TakePresentationRequest());
> I think the name |create_session_request| is very confusing. Could you rename
> this |presentation_request_creator| or something similar so it doesn't look
like
> a function.
> 
> Also, please initialize with = instead of () if possible:
>   scoped_ptr<T> foo = TakeFoo();
> 
>
https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/r...
> File chrome/browser/media/router/create_presentation_session_request.h
(right):
> 
>
https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/r...
> chrome/browser/media/router/create_presentation_session_request.h:30: class
> CreatePresentationSessionRequest {
> This class name is really confusing.
> 
>
https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/r...
> File chrome/browser/media/router/media_router_dialog_controller.h (right):
> 
>
https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/r...
> chrome/browser/media/router/media_router_dialog_controller.h:70: const
> CreatePresentationSessionRequest* create_session_request() const {
> That name doesn't look like a getter but like a function. I think the object
and
> member names need to change.
> 
>
https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/r...
> File chrome/browser/media/router/presentation_request.h (right):
> 
>
https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/r...
> chrome/browser/media/router/presentation_request.h:25: bool Equals(const
> PresentationRequest& other) const;
> I think you should use operator==.
> 
>
https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/r...
> File chrome/browser/media/router/presentation_service_delegate_impl.h (right):
> 
>
https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/r...
> chrome/browser/media/router/presentation_service_delegate_impl.h:56: virtual
> ~DefaultPresentationRequestObserver() {}
> nit: = default;
> 
>
https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/r...
> File chrome/browser/media/router/render_frame_host_id.h (right):
> 
>
https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/r...
> chrome/browser/media/router/render_frame_host_id.h:12: using RenderFrameHostId
=
> std::pair<int, int>;
> I would prefer if you could use render_frame_id and render_process_id instead
of
> creating this for consistency with the current code. This said, I agree that
we
> need such type.

[imcheng, 2015-10-27 20:50:15]

https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/a...
File
chrome/browser/media/android/router/media_router_dialog_controller_android.cc
(right):

https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/a...
chrome/browser/media/android/router/media_router_dialog_controller_android.cc:42:
TakePresentationRequest());
On 2015/10/27 18:42:51, Mounir Lamouri wrote:
> I think the name |create_session_request| is very confusing. Could you rename
> this |presentation_request_creator| or something similar so it doesn't look
like
> a function.
> 
> Also, please initialize with = instead of () if possible:
>   scoped_ptr<T> foo = TakeFoo();

Why is it confusing? It is an object that represents input and callback for a
request to create a presentation session. |presentation_request_creator| doesn't
sound right to me; it doesn't create presentation requests. See other comments
as well.

Changed to move operator.

https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/create_presentation_session_request.h (right):

https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/create_presentation_session_request.h:30: class
CreatePresentationSessionRequest {
On 2015/10/27 18:42:51, Mounir Lamouri wrote:
> This class name is really confusing.

Any suggestions? :)

https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_dialog_controller.h (right):

https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_router_dialog_controller.h:70: const
CreatePresentationSessionRequest* create_session_request() const {
On 2015/10/27 18:42:51, Mounir Lamouri wrote:
> That name doesn't look like a getter but like a function. I think the object
and
> member names need to change.

The fact that the method name is in unix hacker style should suggest it is a
getter. That said, I do see that it can be a little confusing for the class name
/ member to start with a verb. Suggestion for a new name is welcome.

https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/presentation_request.h (right):

https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/presentation_request.h:25: bool Equals(const
PresentationRequest& other) const;
On 2015/10/27 18:42:51, Mounir Lamouri wrote:
> I think you should use operator==.

We already established the use of an explicit |Equals| method to test for
equality over operator== in MR codebase. Keeping it to be consistent with the
rest of codebase.

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

https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl.h:56: virtual
~DefaultPresentationRequestObserver() {}
On 2015/10/27 18:42:51, Mounir Lamouri wrote:
> nit: = default;

Done.

https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/render_frame_host_id.h (right):

https://codereview.chromium.org/1406013003/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/render_frame_host_id.h:12: using RenderFrameHostId =
std::pair<int, int>;
On 2015/10/27 18:42:51, Mounir Lamouri wrote:
> I would prefer if you could use render_frame_id and render_process_id instead
of
> creating this for consistency with the current code. This said, I agree that
we
> need such type.

There are existing places (PresentationServiceDelegateImpl) where it is used and
is also used as a key in maps. So rather than std::map<std::pair<int, int>,
Foo>, std::map<RenderFrameHostId, Foo> provides more information and is also
less wordy.

[imcheng, 2015-10-29 21:20:46]

imcheng@chromium.org changed reviewers:
+ kmarshall@chromium.org
- mlamouri@chromium.org

[imcheng, 2015-10-30 21:27:51]

imcheng@chromium.org changed reviewers:
- kmarshall@chromium.org

[mark a. foltz, 2015-11-04 02:07:25]

Overall looks good, some feedback on the API in PresentationServiceDelegateImpl.

Per conversation, make sure that this patch maintains the existing behavior of
accepting the default request from the top frame only.

Please have Mounir or Anton review the Android-specific files.

Thanks!

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

https://codereview.chromium.org/1406013003/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/create_presentation_session_request_unittest.cc:16:
const char kFrameUrl[] = "http://frameUrl";
Please use a fully qualified URL as test data.  Otherwise I'm not sure what it
gets resolved to.

https://codereview.chromium.org/1406013003/diff/160001/chrome/browser/media/r...
File chrome/browser/media/router/presentation_request.h (right):

https://codereview.chromium.org/1406013003/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/presentation_request.h:22: const GURL& frame_url);
Can you define this here with = default; ?

https://codereview.chromium.org/1406013003/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/presentation_request.h:23: ~PresentationRequest();
Can you define this here with = default; ?

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

https://codereview.chromium.org/1406013003/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl.h:63: const
PresentationRequest* default_presentation_request) = 0;
I do not like APIs that use pointers to represent optional values, unless
absolutely necessary.

I would prefer the observer be written as,

OnDefaultPresentationChanged(const PR& ...)
OnDefaultPresentationRemoved()

https://codereview.chromium.org/1406013003/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl.h:135:
DefaultPresentationRequestObserver* observer);
What is the lifetime of |observer|?  Why is it safe to take a bare pointer? 
Should this take a scoped_ptr or a weak pointer?

Please document ownership relationship here.  Similar comments apply to
RemoveObserver below.

https://codereview.chromium.org/1406013003/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl.h:141: const
PresentationRequest* GetDefaultPresentationRequest() const;
What is the lifetime of the returned object?  If PresentationRequest can be
cheaply copied, consider returning by value instead, and adding a bool
HasDefaultPresentationRequest() const method.

https://codereview.chromium.org/1406013003/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl.h:143:
content::WebContents* web_contents() const { return web_contents_; }
Why is this safe?  Document is the lifetime of the returned WebContents.

https://codereview.chromium.org/1406013003/diff/160001/content/browser/presen...
File content/browser/presentation/presentation_service_impl_unittest.cc (right):

https://codereview.chromium.org/1406013003/diff/160001/content/browser/presen...
content/browser/presentation/presentation_service_impl_unittest.cc:431:
std::string url1("http://fooUrl");
Prefer fully qualified hostnames in test URLs (here and elsewhere).

[imcheng, 2015-11-04 20:35:18]

imcheng@chromium.org changed reviewers:
+ mlamouri@chromium.org

[imcheng, 2015-11-04 20:35:20]

+Mounir for
chrome/browser/media/android/router/media_router_dialog_controller_android.cc,
but feel free to re-review the whole patch.

Let's have a separate discussion on tab-level default request.

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

https://codereview.chromium.org/1406013003/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/create_presentation_session_request_unittest.cc:16:
const char kFrameUrl[] = "http://frameUrl";
On 2015/11/04 02:07:24, mfoltz_ooo_until_11_2 wrote:
> Please use a fully qualified URL as test data.  Otherwise I'm not sure what it
> gets resolved to.

Done.

https://codereview.chromium.org/1406013003/diff/160001/chrome/browser/media/r...
File chrome/browser/media/router/presentation_request.h (right):

https://codereview.chromium.org/1406013003/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/presentation_request.h:22: const GURL& frame_url);
On 2015/11/04 02:07:24, mfoltz_ooo_until_11_2 wrote:
> Can you define this here with = default; ?

Nope. It appears only no-param ctors can be defaulted.

https://codereview.chromium.org/1406013003/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/presentation_request.h:23: ~PresentationRequest();
On 2015/11/04 02:07:24, mfoltz_ooo_until_11_2 wrote:
> Can you define this here with = default; ?

Yes, but it has to be defined in .cc.

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

https://codereview.chromium.org/1406013003/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl.h:63: const
PresentationRequest* default_presentation_request) = 0;
On 2015/11/04 02:07:25, mfoltz_ooo_until_11_2 wrote:
> I do not like APIs that use pointers to represent optional values, unless
> absolutely necessary.
> 
> I would prefer the observer be written as,
> 
> OnDefaultPresentationChanged(const PR& ...)
> OnDefaultPresentationRemoved()
> 
> 
> 

Done.

https://codereview.chromium.org/1406013003/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl.h:135:
DefaultPresentationRequestObserver* observer);
On 2015/11/04 02:07:25, mfoltz_ooo_until_11_2 wrote:
> What is the lifetime of |observer|?  Why is it safe to take a bare pointer? 
> Should this take a scoped_ptr or a weak pointer?
> 
> Please document ownership relationship here.  Similar comments apply to
> RemoveObserver below.

Done.

https://codereview.chromium.org/1406013003/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl.h:141: const
PresentationRequest* GetDefaultPresentationRequest() const;
On 2015/11/04 02:07:25, mfoltz_ooo_until_11_2 wrote:
> What is the lifetime of the returned object?  If PresentationRequest can be
> cheaply copied, consider returning by value instead, and adding a bool
> HasDefaultPresentationRequest() const method.

Done.

https://codereview.chromium.org/1406013003/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl.h:143:
content::WebContents* web_contents() const { return web_contents_; }
On 2015/11/04 02:07:25, mfoltz_ooo_until_11_2 wrote:
> Why is this safe?  Document is the lifetime of the returned WebContents.

Done.

https://codereview.chromium.org/1406013003/diff/160001/content/browser/presen...
File content/browser/presentation/presentation_service_impl_unittest.cc (right):

https://codereview.chromium.org/1406013003/diff/160001/content/browser/presen...
content/browser/presentation/presentation_service_impl_unittest.cc:431:
std::string url1("http://fooUrl");
On 2015/11/04 02:07:25, mark a. foltz wrote:
> Prefer fully qualified hostnames in test URLs (here and elsewhere).

Done.

[whywhat, 2015-11-06 11:45:55]

avayvod@chromium.org changed reviewers:
+ avayvod@chromium.org
- mlamouri@chromium.org

[whywhat, 2015-11-06 11:45:56]

android/ lgtm with a nit

https://codereview.chromium.org/1406013003/diff/200001/chrome/browser/media/a...
File
chrome/browser/media/android/router/media_router_dialog_controller_android.cc
(right):

https://codereview.chromium.org/1406013003/diff/200001/chrome/browser/media/a...
chrome/browser/media/android/router/media_router_dialog_controller_android.cc:43:
const MediaSource::Id source_id(presentation_request.GetMediaSource().id());
nit: could you be consistent with whether you're using the "type name =
init_value" or "type name(init_value)" form of defining a variable?

[mlamouri (slow - plz ping), 2015-11-06 14:12:14]

mlamouri@chromium.org changed reviewers:
+ mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-11-06 14:46:43]

It sgtm but I wonder if your change wrt ListenForDefaultSessionStart is correct.
I think we should only listen for default session start if there is a
defaultRequest instance. If there is no default request, the renderer will do
nothing with the information. IMO, we should keep this call but don't listen as
soon as the dispatcher is created.

WDYT?

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

https://codereview.chromium.org/1406013003/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/create_presentation_session_request.h:30: class
CreatePresentationSessionRequest {
What about these names:
- PresentationRequestCreator
- PresentationRequestMaker
- PresentationRequestFactory (it's not really a factory, but it's otherwise the
best name IMO)

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

https://codereview.chromium.org/1406013003/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/presentation_request.cc:19:
PresentationRequest::~PresentationRequest() = default;
nit: why not putting this in the header?

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

https://codereview.chromium.org/1406013003/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/presentation_request.h:25: bool Equals(const
PresentationRequest& other) const;
For the record, using Equals() is not a common pattern in Chromium. I will not
insist given that it seems to be commen in MR.

[imcheng, 2015-11-07 02:00:35]

On 2015/11/06 14:46:43, Mounir Lamouri wrote:
> It sgtm but I wonder if your change wrt ListenForDefaultSessionStart is
correct.
> I think we should only listen for default session start if there is a
> defaultRequest instance. If there is no default request, the renderer will do
> nothing with the information. IMO, we should keep this call but don't listen
as
> soon as the dispatcher is created.
> 
> WDYT?

That is correct and I would say we shouldn't even do
ListenForDefaultSessionStart from the renderer, which is why I removed the mojom
API. Instead, PresentationServiceImpl listens iff a defaultRequest was set and
notifies PresentationDispatcher if the defaultRequest was started.


> 
>
https://codereview.chromium.org/1406013003/diff/200001/chrome/browser/media/r...
> File chrome/browser/media/router/create_presentation_session_request.h
(right):
> 
>
https://codereview.chromium.org/1406013003/diff/200001/chrome/browser/media/r...
> chrome/browser/media/router/create_presentation_session_request.h:30: class
> CreatePresentationSessionRequest {
> What about these names:
> - PresentationRequestCreator
> - PresentationRequestMaker
> - PresentationRequestFactory (it's not really a factory, but it's otherwise
the
> best name IMO)
> 
>
https://codereview.chromium.org/1406013003/diff/200001/chrome/browser/media/r...
> File chrome/browser/media/router/presentation_request.cc (right):
> 
>
https://codereview.chromium.org/1406013003/diff/200001/chrome/browser/media/r...
> chrome/browser/media/router/presentation_request.cc:19:
> PresentationRequest::~PresentationRequest() = default;
> nit: why not putting this in the header?
> 
>
https://codereview.chromium.org/1406013003/diff/200001/chrome/browser/media/r...
> File chrome/browser/media/router/presentation_request.h (right):
> 
>
https://codereview.chromium.org/1406013003/diff/200001/chrome/browser/media/r...
> chrome/browser/media/router/presentation_request.h:25: bool Equals(const
> PresentationRequest& other) const;
> For the record, using Equals() is not a common pattern in Chromium. I will not
> insist given that it seems to be commen in MR.

[imcheng, 2015-11-07 02:00:46]

PTAL

https://codereview.chromium.org/1406013003/diff/200001/chrome/browser/media/a...
File
chrome/browser/media/android/router/media_router_dialog_controller_android.cc
(right):

https://codereview.chromium.org/1406013003/diff/200001/chrome/browser/media/a...
chrome/browser/media/android/router/media_router_dialog_controller_android.cc:43:
const MediaSource::Id source_id(presentation_request.GetMediaSource().id());
On 2015/11/06 11:45:56, whywhat wrote:
> nit: could you be consistent with whether you're using the "type name =
> init_value" or "type name(init_value)" form of defining a variable?

Done.

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

https://codereview.chromium.org/1406013003/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/create_presentation_session_request.h:30: class
CreatePresentationSessionRequest {
On 2015/11/06 14:46:43, Mounir Lamouri wrote:
> What about these names:
> - PresentationRequestCreator
> - PresentationRequestMaker
> - PresentationRequestFactory (it's not really a factory, but it's otherwise
the
> best name IMO)

Well this class doesn't create PresentationRequests which I think is the source
of confusion. It is a representation of a request to create a presentation
session (to be renamed to connection).

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

https://codereview.chromium.org/1406013003/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/presentation_request.cc:19:
PresentationRequest::~PresentationRequest() = default;
On 2015/11/06 14:46:43, Mounir Lamouri wrote:
> nit: why not putting this in the header?

The compiler complains about not inlining complex destructors.

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

https://codereview.chromium.org/1406013003/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/presentation_request.h:25: bool Equals(const
PresentationRequest& other) const;
On 2015/11/06 14:46:43, Mounir Lamouri wrote:
> For the record, using Equals() is not a common pattern in Chromium. I will not
> insist given that it seems to be commen in MR.

Acknowledged.

[mlamouri (slow - plz ping), 2015-11-09 17:54:58]

lgtm

[mark a. foltz, 2015-11-09 18:33:36]

LGTM.  Mostly minor comments around documentation.

https://codereview.chromium.org/1406013003/diff/220001/chrome/browser/media/r...
File chrome/browser/media/router/create_presentation_session_request.h (right):

https://codereview.chromium.org/1406013003/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/create_presentation_session_request.h:30: class
CreatePresentationSessionRequest {
This might be better called "CreatePresentationConnectionRequest" to align
naming with the spec.

https://codereview.chromium.org/1406013003/diff/220001/chrome/browser/media/r...
File chrome/browser/media/router/presentation_request.h (right):

https://codereview.chromium.org/1406013003/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/presentation_request.h:17: // presentation URL of
the request, and information on the originating frame.
Should this also take the tab id to pass to the MRPM?  Or is that obtained
through another route?

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

https://codereview.chromium.org/1406013003/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl.cc:452: 
Extra newline

https://codereview.chromium.org/1406013003/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl.cc:525: const
PresentationRequest& default_presentation_request) {
Is it possible to call this with an empty |default_presentation_request|?

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

https://codereview.chromium.org/1406013003/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl.h:59: //
WebContents has changed.
Nit: Is this also called when the initial assignment to
presentation.defaultRequest is done?  Maybe say "...WebContents has been set or
is changed."

https://codereview.chromium.org/1406013003/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl.h:61: virtual
void OnDefaultPresentationChanged(
Sometimes it's advantageous to pass the old value and the new value in the
observer API, so clients can clean up any state from the old value.  But may not
be necessary here.

https://codereview.chromium.org/1406013003/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl.h:144: // is an
error to call this method if the default presentation request does
How is this error communicated to the caller?

https://codereview.chromium.org/1406013003/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl.h:200:
content::WebContents* const web_contents_;
TODO: It looks like it might be possible to declare this const
content::WebContents *const; however all of the downstream APIs in the MR don't
look const-correct, and if you are adding an observer anywhere to the WC then it
probably won't work.

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

https://codereview.chromium.org/1406013003/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:244:
EXPECT_CALL(observer, OnDefaultPresentationChanged(Equals(observed_request2)))
observer.OnDefaultPresentationCleared() should be exercised in a test case

https://codereview.chromium.org/1406013003/diff/220001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/1406013003/diff/220001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.cc:170: if
(create_session_request_) {
This class is still a little hard to follow because it is keeping track of two
requests: the create_session_request_ and the presentation_request_, which can
have different states depending on how the Presentation API is used.  I think
some further refactoring to clarify the two use cases would be helpful, but not
urgent.

https://codereview.chromium.org/1406013003/diff/220001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.cc:232:
query_result_manager_->StartSinksQuery(
Do you need to stop the sinks query for the previous PR?

[imcheng, 2015-11-10 18:49:13]

Addressed comments. I also added back the main frame logic to PSDImpl (i.e.,
only the main frame of the tab can set/clear the tab-level default request)

https://codereview.chromium.org/1406013003/diff/220001/chrome/browser/media/r...
File chrome/browser/media/router/create_presentation_session_request.h (right):

https://codereview.chromium.org/1406013003/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/create_presentation_session_request.h:30: class
CreatePresentationSessionRequest {
On 2015/11/09 18:33:35, mark a. foltz wrote:
> This might be better called "CreatePresentationConnectionRequest" to align
> naming with the spec.

Done.

https://codereview.chromium.org/1406013003/diff/220001/chrome/browser/media/r...
File chrome/browser/media/router/presentation_request.h (right):

https://codereview.chromium.org/1406013003/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/presentation_request.h:17: // presentation URL of
the request, and information on the originating frame.
On 2015/11/09 18:33:35, mark a. foltz wrote:
> Should this also take the tab id to pass to the MRPM?  Or is that obtained
> through another route?

It can be obtained via initiator WebContents available from MRUI / PSDImpl.

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

https://codereview.chromium.org/1406013003/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl.cc:452: 
On 2015/11/09 18:33:35, mark a. foltz wrote:
> Extra newline

Done.

https://codereview.chromium.org/1406013003/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl.cc:525: const
PresentationRequest& default_presentation_request) {
On 2015/11/09 18:33:35, mark a. foltz wrote:
> Is it possible to call this with an empty |default_presentation_request|?

No, because there's no empty ctor for PresentationRequest.

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

https://codereview.chromium.org/1406013003/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl.h:59: //
WebContents has changed.
On 2015/11/09 18:33:35, mark a. foltz wrote:
> Nit: Is this also called when the initial assignment to
> presentation.defaultRequest is done?  Maybe say "...WebContents has been set
or
> is changed."

Done.

https://codereview.chromium.org/1406013003/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl.h:61: virtual
void OnDefaultPresentationChanged(
On 2015/11/09 18:33:35, mark a. foltz wrote:
> Sometimes it's advantageous to pass the old value and the new value in the
> observer API, so clients can clean up any state from the old value.  But may
not
> be necessary here.

Acknowledged.

https://codereview.chromium.org/1406013003/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl.h:144: // is an
error to call this method if the default presentation request does
On 2015/11/09 18:33:35, mark a. foltz wrote:
> How is this error communicated to the caller?

DCHECK. Since MRUI is the only caller, I don't expect that to happen.

https://codereview.chromium.org/1406013003/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl.h:200:
content::WebContents* const web_contents_;
On 2015/11/09 18:33:35, mark a. foltz wrote:
> TODO: It looks like it might be possible to declare this const
> content::WebContents *const; however all of the downstream APIs in the MR
don't
> look const-correct, and if you are adding an observer anywhere to the WC then
it
> probably won't work.

Yeah, MediaRouterDialogController::GetOrCreateForWebContents(web_contents_);
modifies web_contents_, but I think it may be the only place.

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

https://codereview.chromium.org/1406013003/diff/220001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:244:
EXPECT_CALL(observer, OnDefaultPresentationChanged(Equals(observed_request2)))
On 2015/11/09 18:33:35, mark a. foltz wrote:
> observer.OnDefaultPresentationCleared() should be exercised in a test case

Yep. It is done below.

https://codereview.chromium.org/1406013003/diff/220001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/1406013003/diff/220001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.cc:170: if
(create_session_request_) {
On 2015/11/09 18:33:35, mark a. foltz wrote:
> This class is still a little hard to follow because it is keeping track of two
> requests: the create_session_request_ and the presentation_request_, which can
> have different states depending on how the Presentation API is used.  I think
> some further refactoring to clarify the two use cases would be helpful, but
not
> urgent.

Acknowledged.

https://codereview.chromium.org/1406013003/diff/220001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.cc:232:
query_result_manager_->StartSinksQuery(
On 2015/11/09 18:33:35, mark a. foltz wrote:
> Do you need to stop the sinks query for the previous PR?

QueryResultManager will stop the previous query internally, so there's no need.

[imcheng, 2015-11-10 22:34:57]

The CQ bit was checked by imcheng@chromium.org

[imcheng, 2015-11-10 22:34:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
avayvod@chromium.org, mfoltz@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1406013003/#ps300001
(title: "Compile fix x2")

[commit-bot: I haz the power, 2015-11-10 22:36:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1406013003/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1406013003/300001

[commit-bot: I haz the power, 2015-11-10 22:50:49]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2015-11-10 22:51:27]

Patchset 16 (id:??) landed as
https://crrev.com/6104fb062821e3e8584ed2e40cdb3f77502d88f0
Cr-Commit-Position: refs/heads/master@{#358935}

[imcheng, 2015-11-10 22:57:52]

Description was changed from

==========
[Presentation API / Media Router] Clean up default pres URL logic.

1. In presentation_service.mojom, there were 2 methods for default
presentation: ListenForDefaultSessionStart and SetDefaultPresentationURL.
There is no reason they should be called separately. Furthermore,
ListenForDefaultSessionStart is called after connecting to
PresentationService, which is not optimal. This CL removes the former
method.

2. Remove OnDefaultSessionStarted from PSD::Observer and replace it
with callbacks. The purpose of PSD::Observer is to notify
PresentationServiceImpl that PresentationServiceDelegate is being
destroyed and shouldn't include specific Pres API logic.

3. Remove main frame restriction for setting default presentation
URL / request. This means non main frames can now set the "tab level"
default request (which is the one shown in the browser-initiated
dialog) See crbug.com/544259.

4. Fix default request matching logic in PSDImpl to account for all
of (frame, frame url, presentation url). Previously it was only
checking presentation url and always assumes it came from the
current main frame. This is a problem because the main frame of a
WebContents can change over time, i.e. main frame at time of setting
DPU and time of route response could be different). Now we create
a PresentationRequest object to hold all info needed to match default
request and pass it to MRUI, who then binds the request to the route
response callback in PSDImpl/PresentationFrameManager to perform the
matching.

BUG=544259

Committed: https://crrev.com/6104fb062821e3e8584ed2e40cdb3f77502d88f0
Cr-Commit-Position: refs/heads/master@{#358935}
==========

to

==========
[Presentation API / Media Router] Clean up default pres URL logic.

1. In presentation_service.mojom, there were 2 methods for default
presentation: ListenForDefaultSessionStart and SetDefaultPresentationURL.
There is no reason they should be called separately. Furthermore,
ListenForDefaultSessionStart is called after connecting to
PresentationService, which is not optimal. This CL removes the former
method.

2. Remove OnDefaultSessionStarted from PSD::Observer and replace it
with callbacks. The purpose of PSD::Observer is to notify
PresentationServiceImpl that PresentationServiceDelegate is being
destroyed and shouldn't include specific Pres API logic.

3. Fix default request matching logic in PSDImpl to account for all
of (frame, frame url, presentation url). Previously it was only
checking presentation url and always assumes it came from the
current main frame. This is a problem because the main frame of a
WebContents can change over time, i.e. main frame at time of setting
DPU and time of route response could be different). Now we create
a PresentationRequest object to hold all info needed to match default
request and pass it to MRUI, who then binds the request to the route
response callback in PSDImpl/PresentationFrameManager to perform the
matching.

BUG=544259

Committed: https://crrev.com/6104fb062821e3e8584ed2e40cdb3f77502d88f0
Cr-Commit-Position: refs/heads/master@{#358935}
==========

[robliao, 2015-11-11 01:29:14]

A revert of this CL (patchset #16 id:300001) has been created in
https://codereview.chromium.org/1436703002/ by robliao@chromium.org.

The reason for reverting is: Prime suspect in causing

mojo_runner_unittests mojo_runner_unittests 
failures:
BackgroundApplicationLoaderTest.Load
NativeApplicationLoaderTest.DoesNotExist

https://build.chromium.org/p/chromium.linux/builders/Android%20GN/builds/32013.

[robliao, 2015-11-11 01:47:15]

On 2015/11/11 01:29:14, robliao wrote:
> A revert of this CL (patchset #16 id:300001) has been created in
> https://codereview.chromium.org/1436703002/ by mailto:robliao@chromium.org.
> 
> The reason for reverting is: Prime suspect in causing
> 
> mojo_runner_unittests mojo_runner_unittests 
> failures:
> BackgroundApplicationLoaderTest.Load
> NativeApplicationLoaderTest.DoesNotExist
> 
>
https://build.chromium.org/p/chromium.linux/builders/Android%20GN/builds/32013.

One more failing test on
https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg...

content_unittests content_unittests 
failures:
PresentationServiceImplTest.SetDefaultPresentationUrl