[Media Router] Adds return value to mojo MediaRouteProvider::TerminateRoute.

[Media Router] Adds return value to mojo MediaRouteProvider::TerminateRoute.

This API change allows the MRPM to pass a Promise to the MR that represents the
outcome of a call to terminateRoute().  The result is logged in a new histogram, MediaRouter.Provider.TerminateRoute.Result.

The change is designed to be backwards compatible with MRPM versions that do not return a Promise.

The MRPM side change will be done next.

BUG=627967
Committed: https://crrev.com/bfbff851505f6314f4b0f7eaf3b22e16348f3528
Cr-Commit-Position: refs/heads/master@{#406897}

[mark a. foltz, 2016-07-13 23:52:44]

Description was changed from

==========
[Media Router] Adds return value to MediaRouter::TerminateRoute.

This API change allows the MRPM to pass a Promise to the MR that represents the
outcome of a call to terminateRoute().  The result is logged in an UMA
histogram.

The change is backwards compatible to MRPM versions that do not return a
Promise.

BUG=627967
==========

to

==========
[Media Router] Adds return value to MediaRouter::TerminateRoute.

This API change allows the MRPM to pass a Promise to the MR that represents the
outcome of a call to terminateRoute().  The result is logged in a new histogram,
MediaRouter.Provider.TerminateRoute.Result.

The change is designed to be backwards compatible with MRPM versions that do not
return a Promise.

The MRPM side change will be done next.

BUG=627967
==========

[mark a. foltz, 2016-07-13 23:56:28]

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

[mark a. foltz, 2016-07-13 23:57:18]

Description was changed from

==========
[Media Router] Adds return value to MediaRouter::TerminateRoute.

This API change allows the MRPM to pass a Promise to the MR that represents the
outcome of a call to terminateRoute().  The result is logged in a new histogram,
MediaRouter.Provider.TerminateRoute.Result.

The change is designed to be backwards compatible with MRPM versions that do not
return a Promise.

The MRPM side change will be done next.

BUG=627967
==========

to

==========
[Media Router] Adds return value to mojo MediaRouteProvider::TerminateRoute.

This API change allows the MRPM to pass a Promise to the MR that represents the
outcome of a call to terminateRoute().  The result is logged in a new histogram,
MediaRouter.Provider.TerminateRoute.Result.

The change is designed to be backwards compatible with MRPM versions that do not
return a Promise.

The MRPM side change will be done next.

BUG=627967
==========

[apacible, 2016-07-14 16:46:36]

lgtm

https://codereview.chromium.org/2145983003/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router_mojo_metrics.cc (right):

https://codereview.chromium.org/2145983003/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_mojo_metrics.cc:61: 
nit: remove extra line.

https://codereview.chromium.org/2145983003/diff/20001/extensions/renderer/res...
File extensions/renderer/resources/media_router_bindings.js (right):

https://codereview.chromium.org/2145983003/diff/20001/extensions/renderer/res...
extensions/renderer/resources/media_router_bindings.js:681: // in M56
nitty nit: period at the end of this line.

https://codereview.chromium.org/2145983003/diff/20001/extensions/renderer/res...
extensions/renderer/resources/media_router_bindings.js:687: function(err) {
return toErrorRouteResponse_(err); });
Styling preference: move closing ");" to separate line.

...
  function(err) { return toErrorRouteResponse_(err); }
);

[mark a. foltz, 2016-07-15 20:03:36]

Responded to apacible@ comments.

imcheng@, PTAL

I went ahead and synchronized the result codes between RouteRequestResult,
media_router.mojom and histograms.xml to make it easier to map between them.

https://codereview.chromium.org/2145983003/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router_mojo_metrics.cc (right):

https://codereview.chromium.org/2145983003/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_mojo_metrics.cc:61: 
On 2016/07/14 at 16:46:36, apacible wrote:
> nit: remove extra line.

Done.

https://codereview.chromium.org/2145983003/diff/20001/extensions/renderer/res...
File extensions/renderer/resources/media_router_bindings.js (right):

https://codereview.chromium.org/2145983003/diff/20001/extensions/renderer/res...
extensions/renderer/resources/media_router_bindings.js:681: // in M56
On 2016/07/14 at 16:46:36, apacible wrote:
> nitty nit: period at the end of this line.

Done.

https://codereview.chromium.org/2145983003/diff/20001/extensions/renderer/res...
extensions/renderer/resources/media_router_bindings.js:687: function(err) {
return toErrorRouteResponse_(err); });
On 2016/07/14 at 16:46:36, apacible wrote:
> Styling preference: move closing ");" to separate line.
> 
> ...
>   function(err) { return toErrorRouteResponse_(err); }
> );

Done.

[imcheng, 2016-07-15 20:18:46]

https://codereview.chromium.org/2145983003/diff/60001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router_type_converters.cc (right):

https://codereview.chromium.org/2145983003/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_type_converters.cc:189: return
media_router::RouteRequestResult::SINK_NOT_FOUND;
Do you also need to add INVALID_ORIGIN, OFF_THE_RECORD_MISMATCH, and
NO_SUPPORTED_PROVIDER?

https://codereview.chromium.org/2145983003/diff/60001/extensions/renderer/res...
File extensions/renderer/resources/media_router_bindings.js (right):

https://codereview.chromium.org/2145983003/diff/60001/extensions/renderer/res...
extensions/renderer/resources/media_router_bindings.js:675: * @param {!string}
routeId
I think this should return a Promise - see createRoute.

@return !Promise<!Object> Resolved to the result of terminating the route.

Or something similar.

https://codereview.chromium.org/2145983003/diff/60001/extensions/renderer/res...
extensions/renderer/resources/media_router_bindings.js:683: maybePromise.then(
return maybePromise.then(...

https://codereview.chromium.org/2145983003/diff/60001/extensions/renderer/res...
extensions/renderer/resources/media_router_bindings.js:688: return
successResult;
return Promise.resolve(successResult);

[mark a. foltz, 2016-07-15 20:36:49]

https://codereview.chromium.org/2145983003/diff/60001/extensions/renderer/res...
File extensions/renderer/resources/media_router_bindings.js (right):

https://codereview.chromium.org/2145983003/diff/60001/extensions/renderer/res...
extensions/renderer/resources/media_router_bindings.js:675: * @param {!string}
routeId
On 2016/07/15 at 20:18:45, imcheng wrote:
> I think this should return a Promise - see createRoute.

Done.

https://codereview.chromium.org/2145983003/diff/60001/extensions/renderer/res...
extensions/renderer/resources/media_router_bindings.js:683: maybePromise.then(
On 2016/07/15 at 20:18:45, imcheng wrote:
> return maybePromise.then(...

Good catch. Done.

https://codereview.chromium.org/2145983003/diff/60001/extensions/renderer/res...
extensions/renderer/resources/media_router_bindings.js:688: return
successResult;
On 2016/07/15 at 20:18:45, imcheng wrote:
> return Promise.resolve(successResult);

Done.

[imcheng, 2016-07-15 20:40:45]

lgtm after the comment in
	chrome/browser/media/router/mojo/media_router_type_converters.cc and nit in
bindings.js are addressed.

https://codereview.chromium.org/2145983003/diff/80001/extensions/renderer/res...
File extensions/renderer/resources/media_router_bindings.js (right):

https://codereview.chromium.org/2145983003/diff/80001/extensions/renderer/res...
extensions/renderer/resources/media_router_bindings.js:677: *    the result of
the terminate operation, or rejecting with an error message
nit: fix line wrap

[mark a. foltz, 2016-07-15 20:48:41]

https://codereview.chromium.org/2145983003/diff/60001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router_type_converters.cc (right):

https://codereview.chromium.org/2145983003/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_type_converters.cc:189: return
media_router::RouteRequestResult::SINK_NOT_FOUND;
On 2016/07/15 at 20:18:45, imcheng wrote:
> Do you also need to add INVALID_ORIGIN, OFF_THE_RECORD_MISMATCH, and
NO_SUPPORTED_PROVIDER?

Done.

[mark a. foltz, 2016-07-15 20:49:05]

https://codereview.chromium.org/2145983003/diff/80001/extensions/renderer/res...
File extensions/renderer/resources/media_router_bindings.js (right):

https://codereview.chromium.org/2145983003/diff/80001/extensions/renderer/res...
extensions/renderer/resources/media_router_bindings.js:677: *    the result of
the terminate operation, or rejecting with an error message
On 2016/07/15 at 20:40:45, imcheng wrote:
> nit: fix line wrap

Done.

[mark a. foltz, 2016-07-15 20:49:26]

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

[commit-bot: I haz the power, 2016-07-15 20:50:10]

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

[commit-bot: I haz the power, 2016-07-15 20:59:40]

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

[commit-bot: I haz the power, 2016-07-15 20:59:41]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mark a. foltz, 2016-07-18 17:04:30]

mfoltz@chromium.org changed reviewers:
+ isherman@chromium.org, mkwst@chromium.org

[mark a. foltz, 2016-07-18 17:04:31]

Mike, Ilya, PTAL.

+isherman for tools/metrics/histograms
+mkwst for .mojom OWNERS

[Ilya Sherman, 2016-07-18 22:56:57]

LGTM % a nit:

https://codereview.chromium.org/2145983003/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/route_request_result.h (right):

https://codereview.chromium.org/2145983003/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/route_request_result.h:35: // -
MediaRouteProviderResult enum in tools/metrics/histograms.xml
Please also document that this enum should be treated as append-only, in order
to back a histogram.

[mark a. foltz, 2016-07-19 23:53:27]

On 2016/07/18 at 22:56:57, isherman wrote:
> LGTM % a nit:
> 
>
https://codereview.chromium.org/2145983003/diff/120001/chrome/browser/media/r...
> File chrome/browser/media/router/route_request_result.h (right):
> 
>
https://codereview.chromium.org/2145983003/diff/120001/chrome/browser/media/r...
> chrome/browser/media/router/route_request_result.h:35: // -
MediaRouteProviderResult enum in tools/metrics/histograms.xml
> Please also document that this enum should be treated as append-only, in order
to back a histogram.

Done.

[Mike West, 2016-07-20 07:27:42]

Adding the result code to the mojom LGTM.

[mark a. foltz, 2016-07-20 16:30:41]

The CQ bit was checked by mfoltz@chromium.org

[mark a. foltz, 2016-07-20 16:30:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
apacible@chromium.org, imcheng@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2145983003/#ps140001
(title: "Respond to isherman@ comments.")

[commit-bot: I haz the power, 2016-07-20 16:31:08]

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-07-20 16:42:55]

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

[commit-bot: I haz the power, 2016-07-20 16:42:56]

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

[mark a. foltz, 2016-07-21 00:47:25]

The CQ bit was checked by mfoltz@chromium.org

[mark a. foltz, 2016-07-21 00:47:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, imcheng@chromium.org, apacible@chromium.org,
mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2145983003/#ps160001
(title: "Fix typo in media_router_bindings.js")

[commit-bot: I haz the power, 2016-07-21 00:47: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-07-21 00:57:23]

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

[commit-bot: I haz the power, 2016-07-21 00:57:24]

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

[Mike West, 2016-07-21 07:30:14]

From the presubmit failure:

***************
chrome/browser/media/router/mojo/OWNERS is missing the following lines:

per-file *_type_converter*.*=set noparent
per-file *_type_converter*.*=file://ipc/SECURITY_OWNERS

for changed files:
  chrome/browser/media/router/mojo/media_router.mojom
  chrome/browser/media/router/mojo/media_router_type_converters.cc
***************

Would you mind adding this owners' file?

[mark a. foltz, 2016-07-21 16:59:34]

The CQ bit was checked by mfoltz@chromium.org

[commit-bot: I haz the power, 2016-07-21 17:00:00]

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

[mark a. foltz, 2016-07-21 17:01:20]

On 2016/07/21 at 07:30:14, mkwst wrote:
> From the presubmit failure:
> 
> ***************
> chrome/browser/media/router/mojo/OWNERS is missing the following lines:
> 
> per-file *_type_converter*.*=set noparent
> per-file *_type_converter*.*=file://ipc/SECURITY_OWNERS
> 
> for changed files:
>   chrome/browser/media/router/mojo/media_router.mojom
>   chrome/browser/media/router/mojo/media_router_type_converters.cc
> ***************
> 
> Would you mind adding this owners' file?

Yeah, this was fixed in https://codereview.chromium.org/2154303003/.

[commit-bot: I haz the power, 2016-07-21 17:58:01]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-07-21 18:00:47]

Description was changed from

==========
[Media Router] Adds return value to mojo MediaRouteProvider::TerminateRoute.

This API change allows the MRPM to pass a Promise to the MR that represents the
outcome of a call to terminateRoute().  The result is logged in a new histogram,
MediaRouter.Provider.TerminateRoute.Result.

The change is designed to be backwards compatible with MRPM versions that do not
return a Promise.

The MRPM side change will be done next.

BUG=627967
==========

to

==========
[Media Router] Adds return value to mojo MediaRouteProvider::TerminateRoute.

This API change allows the MRPM to pass a Promise to the MR that represents the
outcome of a call to terminateRoute().  The result is logged in a new histogram,
MediaRouter.Provider.TerminateRoute.Result.

The change is designed to be backwards compatible with MRPM versions that do not
return a Promise.

The MRPM side change will be done next.

BUG=627967

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

[commit-bot: I haz the power, 2016-07-21 18:00:48]

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