[Media Router] Close dialog after resolving presentation request

[Media Router] Close dialog after resolving presentation request

This change causes the Media Router dialog to close after it resolves a
route that is from a presentation request. This prevents the dialog from
staying open (by the user keeping their mouse over it) and reusing the
same presentation request for another route request (which is an error).

BUG=617321
R=imcheng@chromium.org

Committed: https://crrev.com/cbe9df4959c78e3d8a4fbaa1fef5ccde9b4c0f3f
Cr-Commit-Position: refs/heads/master@{#398683}

[btolsch, 2016-06-08 00:35:24]

Description was changed from

==========
[MediaRouter] Close dialog after resolving presentation request

This change causes the Media Router dialog to close after it resolves a
route that is from a presentation request. This prevents the dialog from
staying open (by the user keeping their mouse over it) and reusing the
same presentation request for another route request (which is an error).

BUG=617321
R=imcheng@chromium.org
==========

to

==========
[Media Router] Close dialog after resolving presentation request

This change causes the Media Router dialog to close after it resolves a
route that is from a presentation request. This prevents the dialog from
staying open (by the user keeping their mouse over it) and reusing the
same presentation request for another route request (which is an error).

BUG=617321
R=imcheng@chromium.org
==========

[btolsch, 2016-06-08 00:36:18]

PTAL, thanks!

[imcheng, 2016-06-08 01:49:20]

https://codereview.chromium.org/2038423006/diff/1/chrome/browser/ui/webui/med...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/2038423006/diff/1/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_router/media_router_ui.cc:425:
route_response_callbacks->push_back(
I don't think we should invoke MediaRouterUI::OnRouteResponseReceived in the
Presentation API case, because it will issue some unnecessary calls to the WebUI
that you will be closing anyway. Otherwise you might see a quick flash to route
details before it is closed.

[btolsch, 2016-06-08 02:28:50]

https://codereview.chromium.org/2038423006/diff/1/chrome/browser/ui/webui/med...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/2038423006/diff/1/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_router/media_router_ui.cc:425:
route_response_callbacks->push_back(
On 2016/06/08 01:49:20, imcheng wrote:
> I don't think we should invoke MediaRouterUI::OnRouteResponseReceived in the
> Presentation API case, because it will issue some unnecessary calls to the
WebUI
> that you will be closing anyway. Otherwise you might see a quick flash to
route
> details before it is closed.

Done.

[imcheng, 2016-06-08 19:02:18]

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

https://codereview.chromium.org/2038423006/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:414: if
(!create_session_request_) {
Should this be (!for_default_source || !create_session_request_)?

We'd still want to update the UI if user selected mirroring on a dialog
initiated from Presentation API.

I think we should just remove the mirroring cast modes for a dialog initiated
from Presentation API as they do not make sense in the context of a
PresentationRequest. But that can be done in another patch.

[btolsch, 2016-06-08 19:36:55]

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

https://codereview.chromium.org/2038423006/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:414: if
(!create_session_request_) {
On 2016/06/08 19:02:18, imcheng wrote:
> Should this be (!for_default_source || !create_session_request_)?
> 
> We'd still want to update the UI if user selected mirroring on a dialog
> initiated from Presentation API.
> 
> I think we should just remove the mirroring cast modes for a dialog initiated
> from Presentation API as they do not make sense in the context of a
> PresentationRequest. But that can be done in another patch.

Done.

[amp, 2016-06-08 19:53:50]

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

https://codereview.chromium.org/2038423006/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:414: if
(!create_session_request_) {
On 2016/06/08 19:02:18, imcheng wrote:
> Should this be (!for_default_source || !create_session_request_)?
> 
> We'd still want to update the UI if user selected mirroring on a dialog
> initiated from Presentation API.
> 
> I think we should just remove the mirroring cast modes for a dialog initiated
> from Presentation API as they do not make sense in the context of a
> PresentationRequest. But that can be done in another patch.

Would this mean that sinks which only support mirroring won't show up for
presentation requests?  That doesn't sound right to me, but perhaps I am
misunderstanding.

[imcheng, 2016-06-08 20:26:16]

On 2016/06/08 19:53:50, amp wrote:
>
https://codereview.chromium.org/2038423006/diff/20001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):
> 
>
https://codereview.chromium.org/2038423006/diff/20001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/media_router/media_router_ui.cc:414: if
> (!create_session_request_) {
> On 2016/06/08 19:02:18, imcheng wrote:
> > Should this be (!for_default_source || !create_session_request_)?
> > 
> > We'd still want to update the UI if user selected mirroring on a dialog
> > initiated from Presentation API.
> > 
> > I think we should just remove the mirroring cast modes for a dialog
initiated
> > from Presentation API as they do not make sense in the context of a
> > PresentationRequest. But that can be done in another patch.
> 
> Would this mean that sinks which only support mirroring won't show up for
> presentation requests?  That doesn't sound right to me, but perhaps I am
> misunderstanding.

Yes. IMO, dialog initiated from PresentationRequest.start() should be
specialized to only handle the request itself. In some ways it can avoid
confusing the user (e.g., choosing a sink that only does mirroring and expecting
the page to transition to presentation mode), but the tradeoff is user now has
to go to browser/context menu to do mirroring and these options are less visible
to the user. So I want to get every on board with it before we make the change,
and perhaps also consider rethinking the UI and/or make it more visible to the
user.

[imcheng, 2016-06-08 20:27:02]

lgtm

[btolsch, 2016-06-08 20:29:51]

The CQ bit was checked by btolsch@chromium.org

[commit-bot: I haz the power, 2016-06-08 20:30:39]

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

[commit-bot: I haz the power, 2016-06-08 21:31:09]

Description was changed from

==========
[Media Router] Close dialog after resolving presentation request

This change causes the Media Router dialog to close after it resolves a
route that is from a presentation request. This prevents the dialog from
staying open (by the user keeping their mouse over it) and reusing the
same presentation request for another route request (which is an error).

BUG=617321
R=imcheng@chromium.org
==========

to

==========
[Media Router] Close dialog after resolving presentation request

This change causes the Media Router dialog to close after it resolves a
route that is from a presentation request. This prevents the dialog from
staying open (by the user keeping their mouse over it) and reusing the
same presentation request for another route request (which is an error).

BUG=617321
R=imcheng@chromium.org
==========

[commit-bot: I haz the power, 2016-06-08 21:31:10]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-08 21:33:08]

Description was changed from

==========
[Media Router] Close dialog after resolving presentation request

This change causes the Media Router dialog to close after it resolves a
route that is from a presentation request. This prevents the dialog from
staying open (by the user keeping their mouse over it) and reusing the
same presentation request for another route request (which is an error).

BUG=617321
R=imcheng@chromium.org
==========

to

==========
[Media Router] Close dialog after resolving presentation request

This change causes the Media Router dialog to close after it resolves a
route that is from a presentation request. This prevents the dialog from
staying open (by the user keeping their mouse over it) and reusing the
same presentation request for another route request (which is an error).

BUG=617321
R=imcheng@chromium.org

Committed: https://crrev.com/cbe9df4959c78e3d8a4fbaa1fef5ccde9b4c0f3f
Cr-Commit-Position: refs/heads/master@{#398683}
==========

[commit-bot: I haz the power, 2016-06-08 21:33:09]

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