[Media Router] 2nd take on fix route response callback lifetime in UI.

[Media Router] 2nd take on fix route response callback lifetime in UI.

This patch changes the MediaRouter CreateRoute/JoinRoute API to take
multiple MediaRouteResponseCallbacks. We have a few scenarios in
which we need to pass in different sets of callbacks:

1) non-presentation route request: update UI only
2) presentation route request as a result of pres API startSession:
    update UI + answer startSession request
3) presentation route request, browser initiated:
    update UI + trigger ondefaultsessionstart

The callbacks are owned by the UI thread and are able to outlive the
MRUI instance that made the CreateRoute call, in cases (2) and (3).

Note that stale presentation API responses (i.e., made obsolete as
a result of navigation) are dropped by PSImpl.

Previous attempt at 1198993002.

BUG=501441
Committed: https://crrev.com/cb9406372121b18de2763ef791b1572641ed5c49
Cr-Commit-Position: refs/heads/master@{#339337}

[imcheng, 2015-07-09 20:50:10]

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

[imcheng, 2015-07-09 20:50:11]

kmarshall, mfoltz: PTAL at everything. I took the feedback from 1198993002 and
started over. I found that if we roll the presentation API request into the
MediaRouterDialogCallbacks object then there is no need for CallbackLists which
result in a cleaner design. I have a document that examines the different
scenarios and edge cases that I can share. I would be happy to go over the
design on the whiteboard as well if needed.

[imcheng, 2015-07-09 21:27:53]

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

[imcheng, 2015-07-09 21:28:25]

+Haibin, he had a comment about instantiating PSDImpl in
MediaRouterDialogCallbacks which is not needed.

[haibinlu, 2015-07-09 21:45:59]

https://codereview.chromium.org/1224093004/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_dialog_controller.h
(right):

https://codereview.chromium.org/1224093004/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.h:126: const
base::WeakPtr<PresentationServiceDelegateImpl>& delegate);
per offline discussion, it is clearer to pass in webcontent there instead of
psdImpl since it is only available when there is a presentation API client.
Users may mirror a tab or desktop without presentation API client.

https://codereview.chromium.org/1224093004/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.h:162: //
this instance will be destroyed at the end of this method.
If UX wants enabling multiple launch simultaneously, then it is possible for one
UI instance to generate several create-route requests to different sinks. In
that case, this instance can only be destroyed after the last create-route
request comes back.

[imcheng, 2015-07-09 22:22:29]

https://codereview.chromium.org/1224093004/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_dialog_controller.h
(right):

https://codereview.chromium.org/1224093004/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.h:126: const
base::WeakPtr<PresentationServiceDelegateImpl>& delegate);
On 2015/07/09 21:45:59, haibinlu wrote:
> per offline discussion, it is clearer to pass in webcontent there instead of
> psdImpl since it is only available when there is a presentation API client.
> Users may mirror a tab or desktop without presentation API client. 

Done. To do that, I needed to make sure initiator WebContents refenrece is valid
throughout lifetime of MediaRouterDialogCallbacks. For that I made MRDC take
ownership of MediaRouterDialogCallbacks. This ownership is needed in order to
avoid memory leaks that I previously hadn't thought of. This also means I am
passing in a WeakPtr<MediaRouterDialogCallbacks> to MRUI since the initiator can
be destroyed before MRUI.

https://codereview.chromium.org/1224093004/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.h:162: //
this instance will be destroyed at the end of this method.
On 2015/07/09 21:45:59, haibinlu wrote:
> If UX wants enabling multiple launch simultaneously, then it is possible for
one
> UI instance to generate several create-route requests to different sinks. In
> that case, this instance can only be destroyed after the last create-route
> request comes back.

UX mentioned that they will only allow one at a time. But in any case, we can
simply change the boolean to an int to count number of outstanding requests, and
only delete if it reaches zero.

[imcheng, 2015-07-09 22:44:33]

Chatted with Kevin, another approach that might simplify things is to have
MediaRouterDialogCallbacks correspond with a dialog route request instead.
MediaRouterDialogCallbacks can have two implementations: one for presentation
api requests and another for browser initiated requests.

For Presentation API dialogs, we pass in CreateSessionRequest object to MRUI.
For the first route request it will construct a presentation api specific
MediaRouterDialogCallbacks with the CSR. For subsequent requests it will just
construct browser initiated MediaRouterDialogCallbacks.


The caveat is that MRUI now needs to keep track of multiple
MediaRouterDialogCallbacks. Also we need to make sure CreateSessionRequest
invokes the error callback on destruction if its callbacks were never invoked.
(we should do that anyways)

[imcheng, 2015-07-13 20:09:05]

I simplified the design a bit further. Now the callback object corresponds 1:1
with presentation route requests. Also got rid of OnDialogDestroyed(), as the
cancellation is handled in MRUI.

[Kevin M, 2015-07-13 21:55:26]

This is much easier to grok. Thanks for your patience.

https://codereview.chromium.org/1224093004/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc
(right):

https://codereview.chromium.org/1224093004/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc:145:
virtual void OnPresentationRouteResponseReceived(scoped_ptr<MediaRoute> route,
Make the body of this function call a different pure virtual method which is
implemented by subclasses. You'll get some nice guarantees that way: that the
callback is always run, that the callbacks are destroyed, and that the
subclasses must provide an implementation or else a compiler error will be
raised.

https://codereview.chromium.org/1224093004/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc:148:
dialog_controller_->DestroyDialogCallbacks(this);
Suggestion: make a note that no additional processing should be done after this
line, because |this| will be deleted immediately.

https://codereview.chromium.org/1224093004/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc:166:
class MediaRouterDialogController::BrowserInitiatedMediaRouterDialogCallbacks
Nit: that's a big identifier. This is just a child class so maybe we could use
that context to give it a shorter name?

BrowserInitiatedCallbacks? PresentationInitiatedCallbacks?

https://codereview.chromium.org/1224093004/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc:184: if
(route) {
Suggestion: DCHECK(route || !error.empty()) ?

https://codereview.chromium.org/1224093004/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_dialog_controller.h
(right):

https://codereview.chromium.org/1224093004/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.h:63: //
route reponse comes back, or nullptr if the presentation route request
Suggestion: ...or nullptr if the request did not originate from the Presentation
API.

https://codereview.chromium.org/1224093004/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.h:73: class
BrowserInitiatedMediaRouterDialogCallbacks;
alpha sort?

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

https://codereview.chromium.org/1224093004/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.cc:309:
&MediaRouterUI::OnRouteResponseReceived, weak_factory_.GetWeakPtr());
Assign route_response_callback in an "else" block?

[imcheng, 2015-07-14 01:00:24]

Thanks! PTAL

https://codereview.chromium.org/1224093004/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc
(right):

https://codereview.chromium.org/1224093004/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc:145:
virtual void OnPresentationRouteResponseReceived(scoped_ptr<MediaRoute> route,
On 2015/07/13 21:55:26, Kevin M wrote:
> Make the body of this function call a different pure virtual method which is
> implemented by subclasses. You'll get some nice guarantees that way: that the
> callback is always run, that the callbacks are destroyed, and that the
> subclasses must provide an implementation or else a compiler error will be
> raised.

Good idea. Done.

https://codereview.chromium.org/1224093004/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc:148:
dialog_controller_->DestroyDialogCallbacks(this);
On 2015/07/13 21:55:26, Kevin M wrote:
> Suggestion: make a note that no additional processing should be done after
this
> line, because |this| will be deleted immediately.

Done.

https://codereview.chromium.org/1224093004/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc:166:
class MediaRouterDialogController::BrowserInitiatedMediaRouterDialogCallbacks
On 2015/07/13 21:55:26, Kevin M wrote:
> Nit: that's a big identifier. This is just a child class so maybe we could use
> that context to give it a shorter name?
> 
> BrowserInitiatedCallbacks? PresentationInitiatedCallbacks?

Done.

https://codereview.chromium.org/1224093004/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc:184: if
(route) {
On 2015/07/13 21:55:26, Kevin M wrote:
> Suggestion: DCHECK(route || !error.empty()) ?

Done.

https://codereview.chromium.org/1224093004/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_dialog_controller.h
(right):

https://codereview.chromium.org/1224093004/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.h:63: //
route reponse comes back, or nullptr if the presentation route request
On 2015/07/13 21:55:26, Kevin M wrote:
> Suggestion: ...or nullptr if the request did not originate from the
Presentation
> API.

Done.

https://codereview.chromium.org/1224093004/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.h:73: class
BrowserInitiatedMediaRouterDialogCallbacks;
On 2015/07/13 21:55:26, Kevin M wrote:
> alpha sort?

Done.

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

https://codereview.chromium.org/1224093004/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.cc:309:
&MediaRouterUI::OnRouteResponseReceived, weak_factory_.GetWeakPtr());
On 2015/07/13 21:55:26, Kevin M wrote:
> Assign route_response_callback in an "else" block?

The value here is used inside "if" so I can avoid writing
"base::Bind(&MRUI::...." twice.

[haibinlu, 2015-07-14 01:44:55]

https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc
(right):

https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc:148:
ProcessPresentationRouteResponse(route.get(), error);
ProcessCreateRouteResponse

https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc:201:
delegate->OnRouteCreated(*route);
how is browser initiated tab-mirroring handled?

https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_dialog_controller.h
(right):

https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.h:55: //
Called by the dialog when it is about to make a presentation route request
create route

https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.h:112: //
MediaRouterDialogCallbacks corresponding to presentation route requests
create route requests

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

https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.cc:317:
route_response_callback = dialog_controller->AddPresentationRouteRequest(
AddPresentationRouteRequest --> AddCreateRouteRequest?

[imcheng, 2015-07-14 16:45:38]

https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc
(right):

https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc:148:
ProcessPresentationRouteResponse(route.get(), error);
On 2015/07/14 01:44:55, haibinlu wrote:
> ProcessCreateRouteResponse

ditto

https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc:201:
delegate->OnRouteCreated(*route);
On 2015/07/14 01:44:55, haibinlu wrote:
> how is browser initiated tab-mirroring handled?

For mirroring, instead of creating an instance of this class and binding its
callback, we bind MRUI::OnRouteResponseReceived as the CreateRoute response
callback.

https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_dialog_controller.h
(right):

https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.h:55: //
Called by the dialog when it is about to make a presentation route request
On 2015/07/14 01:44:55, haibinlu wrote:
> create route

This is specific to presentation route requests.

https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.h:112: //
MediaRouterDialogCallbacks corresponding to presentation route requests
On 2015/07/14 01:44:55, haibinlu wrote:
> create route requests

ditto

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

https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.cc:317:
route_response_callback = dialog_controller->AddPresentationRouteRequest(
On 2015/07/14 01:44:55, haibinlu wrote:
> AddPresentationRouteRequest --> AddCreateRouteRequest?

ditto

[mark a. foltz, 2015-07-14 21:36:32]

It sounds like there have been a few approaches to this from the bug description
and I didn't follow all the offline conversations.

One way to think about this problem is that the object servicing the request (in
this case the media router) could take ownership of the callback(s) and the
callback(s) could hold bound weak pointers to the calling objects (in this case
the dialog and/or the presentation service delegate).

However it depends on which object is making the request and getting the
response, and it seems like the flow is from the dialog towards the MR?  Maybe a
larger reworking of the interaction between the presentation service, the dialog
and the MR might clarify things.

In general it seems like this approach of wrapping one callback in another seems
a little complicated.

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

https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/create_session_request.cc:29: if (!cb_invoked_) {
Or just call MaybeInvokeErrorCallback()?

https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc
(right):

https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc:119: //
progress.
This description doesn't quite match up with the behavior.  This object keeps a
ptr to the dialog controller and can invoke a method on it at any time through
its public API.  Where is the protection against the destruction of the dialog
controller?

https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc:133:
MediaRouterDialogCallbacks(
Nit: This object wraps a single callback, so maybe just
MediaRouterDialogCallback?

https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc:468: //
TODO(imcheng): Don't create PresentationServiceDelegateImpl if it doesn't
Does there just need to be a separate method on PSDI to check for existence? 
Seems like it would be reasonable to do this now.

https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc:481:
MediaRouterDialogController::AddPresentationRouteRequest(
Please split this into two methods:

AddPresentationRouteRequest()
AddBrowserRouteRequest()

instead of having an optional parameter.

https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_dialog_controller.h
(right):

https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.h:9:
#include "base/memory/weak_ptr.h"
Not used

https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.h:64: //
Presentation API.
The comment above suggests that this is only for Presentation API - so
|presentation_request| cannot be nullptr?  Or do you intend for the API to
handle both Presentation and non-Presentation use cases (which seems better)? 
Please clarify.

https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.h:67:
scoped_ptr<CreateSessionRequest> presentation_request);
CreateSessionRequest -> CreatePresentationSessionRequest ?

https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.h:96: void
DestroyDialogCallbacks(MediaRouterDialogCallbacks* callbacks);
This might be better named RemoveDialogCallbacks()?  When I first read this I
thought it was just a method to invoke the MRDC destructor.

[imcheng, 2015-07-16 01:06:41]

On 2015/07/14 21:36:32, mark a. foltz wrote:
> It sounds like there have been a few approaches to this from the bug
description
> and I didn't follow all the offline conversations.
> 
> One way to think about this problem is that the object servicing the request
(in
> this case the media router) could take ownership of the callback(s) and the
> callback(s) could hold bound weak pointers to the calling objects (in this
case
> the dialog and/or the presentation service delegate).
> 
> However it depends on which object is making the request and getting the
> response, and it seems like the flow is from the dialog towards the MR?  Maybe
a
> larger reworking of the interaction between the presentation service, the
dialog
> and the MR might clarify things.
> 
> In general it seems like this approach of wrapping one callback in another
seems
> a little complicated.
> 

You're right, the previous approach makes things more complicated than it needs
to be. A small change to the MediaRouter Create/JoinRoute API and the fact that
we can use weak pointers to bound the callbacks will result in a much simpler
design. I should have thought of that before.

Here's what's in the latest PS:

- Modified MediaRouter's CreateRoute/JoinRoute API to take
std::vector<MediaRouteResponseCallback>. The callbacks will be invoked with the
response in the same order.
- In MRUI, there are 3 cases which results in different lists of callbacks to be
passed into CreateRoute:
1) Non-presentation route request (mirroring) - [Update UI]
2) Presentation route request with presentation API startSession - [Update UI,
answer startSession]
3) Browser initiated presentation route request - [Update UI, notify PSDImpl for
ondefaultsessionstart]

These callbacks are handled in a non-member function so they are owned by the UI
thread. In addition the callback are bound using weak pointers.

I have responded to comments that still apply. PTAL.


>
https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/media/r...
> File chrome/browser/media/router/create_session_request.cc (right):
> 
>
https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/media/r...
> chrome/browser/media/router/create_session_request.cc:29: if (!cb_invoked_) {
> Or just call MaybeInvokeErrorCallback()?
> 
>
https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
> File chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc
> (right):
> 
>
https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
> chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc:119: //
> progress.
> This description doesn't quite match up with the behavior.  This object keeps
a
> ptr to the dialog controller and can invoke a method on it at any time through
> its public API.  Where is the protection against the destruction of the dialog
> controller?
> 
>
https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
> chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc:133:
> MediaRouterDialogCallbacks(
> Nit: This object wraps a single callback, so maybe just
> MediaRouterDialogCallback?
> 
>
https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
> chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc:468: //
> TODO(imcheng): Don't create PresentationServiceDelegateImpl if it doesn't
> Does there just need to be a separate method on PSDI to check for existence? 
> Seems like it would be reasonable to do this now.
> 
>
https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
> chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc:481:
> MediaRouterDialogController::AddPresentationRouteRequest(
> Please split this into two methods:
> 
> AddPresentationRouteRequest()
> AddBrowserRouteRequest()
> 
> instead of having an optional parameter.
> 
>
https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
> File chrome/browser/ui/webui/media_router/media_router_dialog_controller.h
> (right):
> 
>
https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
> chrome/browser/ui/webui/media_router/media_router_dialog_controller.h:9:
> #include "base/memory/weak_ptr.h"
> Not used
> 
>
https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
> chrome/browser/ui/webui/media_router/media_router_dialog_controller.h:64: //
> Presentation API.
> The comment above suggests that this is only for Presentation API - so
> |presentation_request| cannot be nullptr?  Or do you intend for the API to
> handle both Presentation and non-Presentation use cases (which seems better)? 
> Please clarify.
> 
>
https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
> chrome/browser/ui/webui/media_router/media_router_dialog_controller.h:67:
> scoped_ptr<CreateSessionRequest> presentation_request);
> CreateSessionRequest -> CreatePresentationSessionRequest ?
> 
>
https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
> chrome/browser/ui/webui/media_router/media_router_dialog_controller.h:96: void
> DestroyDialogCallbacks(MediaRouterDialogCallbacks* callbacks);
> This might be better named RemoveDialogCallbacks()?  When I first read this I
> thought it was just a method to invoke the MRDC destructor.

[imcheng, 2015-07-16 01:06:53]

https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc
(right):

https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc:468: //
TODO(imcheng): Don't create PresentationServiceDelegateImpl if it doesn't
On 2015/07/14 21:36:32, mark a. foltz wrote:
> Does there just need to be a separate method on PSDI to check for existence? 
> Seems like it would be reasonable to do this now.

Yes, we could call FromWebContents only to check for existence. The only tricky
case is for browser-initiated dialogs, if PSDImpl didn't exist before and is
created when the dialog is already open. In that case, the ideal behavior is to
update the UI with the info from PSDImpl and add a default presentation
observer. Without that logic, the user has to close and reopen the dialog to see
the updated UI. The current logic is correct in that regard. I guess in that
sense, this doesn't need to be fixed. I think splitting up PSDImpl and/or
introducing an interface to talk to it will help make this code cleaner.

https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_dialog_controller.h
(right):

https://codereview.chromium.org/1224093004/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.h:67:
scoped_ptr<CreateSessionRequest> presentation_request);
On 2015/07/14 21:36:32, mark a. foltz wrote:
> CreateSessionRequest -> CreatePresentationSessionRequest ?

Done.

[mark a. foltz, 2015-07-16 22:11:18]

lgtm

Thanks.  This overall is a simpler design.  I am not a big fan of weak pointers
in general, but it looks like the lifetimes of the objects providing the
callbacks is really out of our control.

[imcheng, 2015-07-17 01:01:25]

The CQ bit was checked by imcheng@chromium.org

[commit-bot: I haz the power, 2015-07-17 01:02:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1224093004/220001

[commit-bot: I haz the power, 2015-07-17 01:38:06]

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

[commit-bot: I haz the power, 2015-07-17 01:38:07]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

(exceeded global retry quota)

[imcheng, 2015-07-17 19:40:24]

The CQ bit was checked by imcheng@chromium.org

[imcheng, 2015-07-17 19:40:25]

The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org
Link to the patchset: https://codereview.chromium.org/1224093004/#ps240001
(title: "Fix unit test")

[commit-bot: I haz the power, 2015-07-17 19:41:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1224093004/240001

[commit-bot: I haz the power, 2015-07-17 21:44:14]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-07-17 21:45:21]

Patchset 13 (id:??) landed as
https://crrev.com/cb9406372121b18de2763ef791b1572641ed5c49
Cr-Commit-Position: refs/heads/master@{#339337}