[Media Router] Return NotFoundError on dialog exit if appropriate

[Media Router] Return NotFoundError on dialog exit if appropriate

The MR dialog will always be opened when starting a PresentationRequest,
even if there are no sinks that support the presentation url.  The
default cast mode will then be some form of mirroring.  This means that
whether the user selects a device in the dialog or not, the promise from
start() will be rejected with an AbortError.  Instead, a NotFoundError
should be used if there were no devices which supported the presentation
url.

BUG=621161
Committed: https://crrev.com/21d9a27daad3d0f6c3a9ad15818a099c728a3b3c
Cr-Commit-Position: refs/heads/master@{#403336}

[btolsch, 2016-06-28 23:43:31]

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

[btolsch, 2016-06-28 23:43:31]

Checking the sinks in ~MediaRouterUI() seems like the least intrusive solution,
but let me know if you'd like this handled another way.

Also, I didn't see anywhere that the error is tested but if you know where it is
tested or where a test should be added I will take a look at that as well.

[imcheng, 2016-06-29 17:28:03]

lgtm.

Putting it in ~MediaRouterUI sounds reasonable given that's when the dialog is
being closed.

For tests, we can add a unit test for MediaRouterUI. It would be good to test
this end to end as well with integration tests located in
chrome/test/media_router. Basically we'd have to preload a json that tells the
test provider not to return any sinks, then simulate dialog open with
PresentationRequest.start(), close the dialog and assert the promise is rejected
with the correct error code.

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

https://codereview.chromium.org/2108973002/diff/1/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_router/media_router_ui.cc:206: // request was
never attempted. This could be because the dialog defaults to
nit: I would just leave out the part about defaulting to mirroring.

https://codereview.chromium.org/2108973002/diff/1/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_router/media_router_ui.cc:211: return
sink.cast_modes.find(MediaCastMode::DEFAULT) !=
nit: return sink.cast_modes.ContainsValue(MediaCastMode::DEFAULT);

[btolsch, 2016-06-30 07:29:16]

PTAL at the tests and the new MediaRouterUI::InitForTest, thanks!

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

https://codereview.chromium.org/2108973002/diff/1/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_router/media_router_ui.cc:206: // request was
never attempted. This could be because the dialog defaults to
On 2016/06/29 17:28:03, imcheng wrote:
> nit: I would just leave out the part about defaulting to mirroring.

Done.

https://codereview.chromium.org/2108973002/diff/1/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_router/media_router_ui.cc:211: return
sink.cast_modes.find(MediaCastMode::DEFAULT) !=
On 2016/06/29 17:28:03, imcheng wrote:
> nit: return sink.cast_modes.ContainsValue(MediaCastMode::DEFAULT);

Done.

[imcheng, 2016-06-30 18:24:46]

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

https://codereview.chromium.org/2108973002/diff/1/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_router/media_router_ui.cc:211: return
sink.cast_modes.find(MediaCastMode::DEFAULT) !=
On 2016/06/30 07:29:16, btolsch wrote:
> On 2016/06/29 17:28:03, imcheng wrote:
> > nit: return sink.cast_modes.ContainsValue(MediaCastMode::DEFAULT);
> 
> Done.

Thanks. Apologies for suggesting the wrong syntax :)

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

https://codereview.chromium.org/2108973002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui_unittest.cc:39: struct
PresentationRequestCallbacks {
This should be a class since we have Success and Error methods.

https://codereview.chromium.org/2108973002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui_unittest.cc:52: const
content::PresentationError& expected_error;
remove const ref, rename to expected_error_ and move to private.

https://codereview.chromium.org/2108973002/diff/20001/chrome/test/media_route...
File chrome/test/media_router/resources/no_supported_sinks.json (right):

https://codereview.chromium.org/2108973002/diff/20001/chrome/test/media_route...
chrome/test/media_router/resources/no_supported_sinks.json:2:
"complementSupportedSources": true
It might be easier to just use 'getAvailableSinks'. See my comment in the other
cl.

[btolsch, 2016-06-30 21:22:27]

I updated the test data format to match where the other CL is now and added a
dialog close test for when there are absolutely no sinks available.  PTAL,
thanks.

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

https://codereview.chromium.org/2108973002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui_unittest.cc:39: struct
PresentationRequestCallbacks {
On 2016/06/30 18:24:46, imcheng wrote:
> This should be a class since we have Success and Error methods.

Done.

https://codereview.chromium.org/2108973002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui_unittest.cc:52: const
content::PresentationError& expected_error;
On 2016/06/30 18:24:46, imcheng wrote:
> remove const ref, rename to expected_error_ and move to private.

Done.

https://codereview.chromium.org/2108973002/diff/20001/chrome/test/media_route...
File chrome/test/media_router/resources/no_supported_sinks.json (right):

https://codereview.chromium.org/2108973002/diff/20001/chrome/test/media_route...
chrome/test/media_router/resources/no_supported_sinks.json:2:
"complementSupportedSources": true
On 2016/06/30 18:24:46, imcheng wrote:
> It might be easier to just use 'getAvailableSinks'. See my comment in the
other
> cl.

Done.

[imcheng, 2016-06-30 22:07:18]

lgtm

[btolsch, 2016-06-30 23:24:26]

The CQ bit was checked by btolsch@chromium.org

[commit-bot: I haz the power, 2016-06-30 23:24:56]

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

[commit-bot: I haz the power, 2016-06-30 23:31:24]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-30 23:32:55]

Description was changed from

==========
[Media Router] Return NotFoundError on dialog exit if appropriate

The MR dialog will always be opened when starting a PresentationRequest,
even if there are no sinks that support the presentation url.  The
default cast mode will then be some form of mirroring.  This means that
whether the user selects a device in the dialog or not, the promise from
start() will be rejected with an AbortError.  Instead, a NotFoundError
should be used if there were no devices which supported the presentation
url.

BUG=621161
==========

to

==========
[Media Router] Return NotFoundError on dialog exit if appropriate

The MR dialog will always be opened when starting a PresentationRequest,
even if there are no sinks that support the presentation url.  The
default cast mode will then be some form of mirroring.  This means that
whether the user selects a device in the dialog or not, the promise from
start() will be rejected with an AbortError.  Instead, a NotFoundError
should be used if there were no devices which supported the presentation
url.

BUG=621161

Committed: https://crrev.com/21d9a27daad3d0f6c3a9ad15818a099c728a3b3c
Cr-Commit-Position: refs/heads/master@{#403336}
==========

[commit-bot: I haz the power, 2016-06-30 23:32:56]

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