[Media Router] Add a supports_web_ui_controller attribute to MediaRoute

[Media Router] Add a supports_web_ui_controller attribute to MediaRoute

Out of Cast/Hangouts/DIAL MRPs, Cast is the only one that currently supports the
WebUI route controller. To make Hangouts/DIAL routes use extensionview, we'll
set a bit in the MediaRoute object to indicate whether it supports the WebUI
controller. When this bool is set to true and the WebUI controller is enabled,
we use it in the Media Router dialog. Otherwise we show the extensionview
controller (or in the case of DIAL, no controller).

This CL will need to land after the corresponding extension-side CL:
cl/159052870

BUG=732997, 732995
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2938173004
Cr-Commit-Position: refs/heads/master@{#480581}
Committed: https://chromium.googlesource.com/chromium/src/+/b79c1f4430e87e318dc197fda352722333a62262

[takumif, 2017-06-15 21:38:23]

Description was changed from

==========
[Media Router] Add a supports_web_ui_controller attribute to MediaRoute

BUG=732997,732995
==========

to

==========
[Media Router] Add a supports_web_ui_controller attribute to MediaRoute

BUG=732997,732995
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[takumif, 2017-06-15 21:54:13]

Description was changed from

==========
[Media Router] Add a supports_web_ui_controller attribute to MediaRoute

BUG=732997,732995
==========

to

==========
[Media Router] Add a supports_web_ui_controller attribute to MediaRoute

Out of Cast/Hangouts/DIAL MRPs, Cast is the only one that currently supports the
WebUI route controller. To make Hangouts/DIAL routes use extensionview, we'll
set a bit in the MediaRoute object to indicate whether it supports the WebUI
controller. When this bool is set to true and the WebUI controller is enabled,
we use it in the Media Router dialog. Otherwise we show the extensionview
controller (or in the case of DIAL, no controller).

This CL will need to land after the corresponding extension-side CL:
cl/159052870

BUG=732997,732995
==========

[takumif, 2017-06-15 21:54:13]

takumif@chromium.org changed reviewers:
+ dcheng@chromium.org, imcheng@chromium.org

[takumif, 2017-06-15 21:54:54]

Description was changed from

==========
[Media Router] Add a supports_web_ui_controller attribute to MediaRoute

Out of Cast/Hangouts/DIAL MRPs, Cast is the only one that currently supports the
WebUI route controller. To make Hangouts/DIAL routes use extensionview, we'll
set a bit in the MediaRoute object to indicate whether it supports the WebUI
controller. When this bool is set to true and the WebUI controller is enabled,
we use it in the Media Router dialog. Otherwise we show the extensionview
controller (or in the case of DIAL, no controller).

This CL will need to land after the corresponding extension-side CL:
cl/159052870

BUG=732997,732995
==========

to

==========
[Media Router] Add a supports_web_ui_controller attribute to MediaRoute

Out of Cast/Hangouts/DIAL MRPs, Cast is the only one that currently supports the
WebUI route controller. To make Hangouts/DIAL routes use extensionview, we'll
set a bit in the MediaRoute object to indicate whether it supports the WebUI
controller. When this bool is set to true and the WebUI controller is enabled,
we use it in the Media Router dialog. Otherwise we show the extensionview
controller (or in the case of DIAL, no controller).

This CL will need to land after the corresponding extension-side CL:
cl/159052870

BUG=732997,732995
==========

[takumif, 2017-06-15 21:59:41]

Please take a look at:
imcheng: everything
dcheng: media_router.mojom and struct traits
Thanks!

https://codereview.chromium.org/2938173004/diff/1/chrome/test/data/webui/medi...
File chrome/test/data/webui/media_router/media_router_container_route_tests.js
(right):

https://codereview.chromium.org/2938173004/diff/1/chrome/test/data/webui/medi...
chrome/test/data/webui/media_router/media_router_container_route_tests.js:208:
});
Hmm I'm not super happy with how the code is formatted here, but this is what
`git cl format --js` does.

[takumif, 2017-06-15 22:01:54]

Description was changed from

==========
[Media Router] Add a supports_web_ui_controller attribute to MediaRoute

Out of Cast/Hangouts/DIAL MRPs, Cast is the only one that currently supports the
WebUI route controller. To make Hangouts/DIAL routes use extensionview, we'll
set a bit in the MediaRoute object to indicate whether it supports the WebUI
controller. When this bool is set to true and the WebUI controller is enabled,
we use it in the Media Router dialog. Otherwise we show the extensionview
controller (or in the case of DIAL, no controller).

This CL will need to land after the corresponding extension-side CL:
cl/159052870

BUG=732997,732995
==========

to

==========
[Media Router] Add a supports_web_ui_controller attribute to MediaRoute

Out of Cast/Hangouts/DIAL MRPs, Cast is the only one that currently supports the
WebUI route controller. To make Hangouts/DIAL routes use extensionview, we'll
set a bit in the MediaRoute object to indicate whether it supports the WebUI
controller. When this bool is set to true and the WebUI controller is enabled,
we use it in the Media Router dialog. Otherwise we show the extensionview
controller (or in the case of DIAL, no controller).

This CL will need to land after the corresponding extension-side CL:
cl/159052870

BUG=732997,732995
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[imcheng, 2017-06-16 00:12:32]

Looks good overall. I have a suggestion on how to simplify the logic slightly.

Context for Daniel. We are trying to replace extensionview-based controller with
a WebUI / MediaRouteController-based version in our Media Router UI. Not all
MediaRoutes support the new version of controller. So rather than blindly
calling MediaRouter to create a MediaRouteController for any route and have it
fail, we are adding a boolean on MediaRoute to indicate whether it supports
creating a MediaRouteController. The Media Router UI will use this bit to decide
whether to use the new controller, the old controller, or no controller at all.

https://codereview.chromium.org/2938173004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/media_router/media_router_data.js (right):

https://codereview.chromium.org/2938173004/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/media_router_data.js:226:
this.supportsWebUiController = false;
This value should be supplied by the constructor, right?

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

https://codereview.chromium.org/2938173004/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:165:
dictionary->SetBoolean("supportsWebUiController",
Since we now have this boolean per route; can we just AND it with the
|webUiRouteControlsAvailable| bit here? That way you won't have to send
|webUiRouteControlsAvailable| to the WebUI, and might make the logic there a bit
simpler.

https://codereview.chromium.org/2938173004/diff/20001/chrome/common/media_rou...
File chrome/common/media_router/media_route.h (right):

https://codereview.chromium.org/2938173004/diff/20001/chrome/common/media_rou...
chrome/common/media_router/media_route.h:125: bool supports_web_ui_controller_;
= false;

https://codereview.chromium.org/2938173004/diff/20001/chrome/common/media_rou...
File chrome/common/media_router/mojo/media_router.mojom (right):

https://codereview.chromium.org/2938173004/diff/20001/chrome/common/media_rou...
chrome/common/media_router/mojo/media_router.mojom:91: // well as its own route
controls within an extensionview in the media router
s/media router/Media Router

https://codereview.chromium.org/2938173004/diff/20001/chrome/common/media_rou...
chrome/common/media_router/mojo/media_router.mojom:95: bool
supports_web_ui_controller;
per discussion: supports_media_route_controller makes more sense and is more
future-proof.

[takumif, 2017-06-16 18:12:25]

Patchset #3 (id:40001) has been deleted

[takumif, 2017-06-16 18:16:53]

Thanks for reviewing!

https://codereview.chromium.org/2938173004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/media_router/media_router_data.js (right):

https://codereview.chromium.org/2938173004/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/media_router_data.js:226:
this.supportsWebUiController = false;
On 2017/06/16 00:12:32, imcheng wrote:
> This value should be supplied by the constructor, right?

The ctor is only used by tests, and since most of the tests don't care about
this value, I thought it'd be okay not to.

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

https://codereview.chromium.org/2938173004/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:165:
dictionary->SetBoolean("supportsWebUiController",
On 2017/06/16 00:12:32, imcheng wrote:
> Since we now have this boolean per route; can we just AND it with the
> |webUiRouteControlsAvailable| bit here? That way you won't have to send
> |webUiRouteControlsAvailable| to the WebUI, and might make the logic there a
bit
> simpler.

Makes sense. Done.

https://codereview.chromium.org/2938173004/diff/20001/chrome/common/media_rou...
File chrome/common/media_router/media_route.h (right):

https://codereview.chromium.org/2938173004/diff/20001/chrome/common/media_rou...
chrome/common/media_router/media_route.h:125: bool supports_web_ui_controller_;
On 2017/06/16 00:12:32, imcheng wrote:
> = false;

Done.

https://codereview.chromium.org/2938173004/diff/20001/chrome/common/media_rou...
File chrome/common/media_router/mojo/media_router.mojom (right):

https://codereview.chromium.org/2938173004/diff/20001/chrome/common/media_rou...
chrome/common/media_router/mojo/media_router.mojom:91: // well as its own route
controls within an extensionview in the media router
On 2017/06/16 00:12:32, imcheng wrote:
> s/media router/Media Router

Done.

https://codereview.chromium.org/2938173004/diff/20001/chrome/common/media_rou...
chrome/common/media_router/mojo/media_router.mojom:95: bool
supports_web_ui_controller;
On 2017/06/16 00:12:32, imcheng wrote:
> per discussion: supports_media_route_controller makes more sense and is more
> future-proof.

Done. In WebUIMessageHandler and JS, I think it's okay to say WebUI route
controller.

[imcheng, 2017-06-16 21:03:11]

lgtm % comments

https://codereview.chromium.org/2938173004/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html
(right):

https://codereview.chromium.org/2938173004/diff/60001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:108:
use-web-ui-route-controls="[[useWebUiRouteControls_]]">
route-details can determine this from the route; See also comment in next file.

https://codereview.chromium.org/2938173004/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/2938173004/diff/60001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1828:
if (this.useWebUiRouteControls_ &&
I was wondering whether we still need this.useWebUiRouteControls_. This check
seems redundant because this method can only be called if we are using
WebUI-based controls.

[dcheng, 2017-06-16 21:23:05]

ipc lgtm (sorry for the review delay)

[takumif, 2017-06-16 23:59:52]

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

[commit-bot: I haz the power, 2017-06-17 00:00: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, 2017-06-17 00:18:13]

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

[commit-bot: I haz the power, 2017-06-17 00:18:14]

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

[takumif, 2017-06-19 18:10:28]

https://codereview.chromium.org/2938173004/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html
(right):

https://codereview.chromium.org/2938173004/diff/60001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.html:108:
use-web-ui-route-controls="[[useWebUiRouteControls_]]">
On 2017/06/16 21:03:11, imcheng wrote:
> route-details can determine this from the route; See also comment in next
file.

Removed.

https://codereview.chromium.org/2938173004/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/2938173004/diff/60001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1828:
if (this.useWebUiRouteControls_ &&
On 2017/06/16 21:03:11, imcheng wrote:
> I was wondering whether we still need this.useWebUiRouteControls_. This check
> seems redundant because this method can only be called if we are using
> WebUI-based controls.

Right. Removed.

https://codereview.chromium.org/2938173004/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/media_router/elements/route_details/route_details.js
(right):

https://codereview.chromium.org/2938173004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/route_details/route_details.js:248:
return !!route && !route.supportsWebUiController;
The Closure compiler complains that the return type is boolean|null without
this.

[takumif, 2017-06-19 18:10:37]

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

[commit-bot: I haz the power, 2017-06-19 18:10:50]

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, 2017-06-19 18:13:17]

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

[commit-bot: I haz the power, 2017-06-19 18:13:18]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[takumif, 2017-06-19 18:36:03]

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

[commit-bot: I haz the power, 2017-06-19 18:36:22]

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, 2017-06-19 19:00:11]

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

[commit-bot: I haz the power, 2017-06-19 19:00:12]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[takumif, 2017-06-19 19:52:40]

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

[commit-bot: I haz the power, 2017-06-19 19:52:59]

Dry run: CQ is trying da patch.

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

[imcheng, 2017-06-19 19:56:13]

lgtm

https://codereview.chromium.org/2938173004/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/media_router/elements/route_details/route_details.js
(right):

https://codereview.chromium.org/2938173004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/route_details/route_details.js:248:
return !!route && !route.supportsWebUiController;
On 2017/06/19 18:10:28, takumif wrote:
> The Closure compiler complains that the return type is boolean|null without
> this.

Acknowledged.

[commit-bot: I haz the power, 2017-06-19 20:57:58]

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

[commit-bot: I haz the power, 2017-06-19 20:57:59]

Dry run: This issue passed the CQ dry run.

[takumif, 2017-06-19 21:05:08]

The CQ bit was checked by takumif@chromium.org

[takumif, 2017-06-19 21:05:09]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2938173004/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2017-06-19 21:05:32]

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, 2017-06-19 21:11:03]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1497906308620120,
"parent_rev": "d4a5e98235da0cc8c4fb1ae2f67826b0b480ce4c", "commit_rev":
"b79c1f4430e87e318dc197fda352722333a62262"}

[commit-bot: I haz the power, 2017-06-19 21:11:17]

Description was changed from

==========
[Media Router] Add a supports_web_ui_controller attribute to MediaRoute

Out of Cast/Hangouts/DIAL MRPs, Cast is the only one that currently supports the
WebUI route controller. To make Hangouts/DIAL routes use extensionview, we'll
set a bit in the MediaRoute object to indicate whether it supports the WebUI
controller. When this bool is set to true and the WebUI controller is enabled,
we use it in the Media Router dialog. Otherwise we show the extensionview
controller (or in the case of DIAL, no controller).

This CL will need to land after the corresponding extension-side CL:
cl/159052870

BUG=732997,732995
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[Media Router] Add a supports_web_ui_controller attribute to MediaRoute

Out of Cast/Hangouts/DIAL MRPs, Cast is the only one that currently supports the
WebUI route controller. To make Hangouts/DIAL routes use extensionview, we'll
set a bit in the MediaRoute object to indicate whether it supports the WebUI
controller. When this bool is set to true and the WebUI controller is enabled,
we use it in the Media Router dialog. Otherwise we show the extensionview
controller (or in the case of DIAL, no controller).

This CL will need to land after the corresponding extension-side CL:
cl/159052870

BUG=732997,732995
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2938173004
Cr-Commit-Position: refs/heads/master@{#480581}
Committed:
https://chromium.googlesource.com/chromium/src/+/b79c1f4430e87e318dc197fda352...
==========

[commit-bot: I haz the power, 2017-06-19 21:11:20]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/b79c1f4430e87e318dc197fda352...