[Media Router] Custom Controls 3 - add plumbing through MRUI and MRWebUIMessageHandler

[Media Router] Custom Controls 3 - add plumbing through MRUI and
MRWebUIMessageHandler

When the route details view is opened, MediaRouterUI will instantiate an
observer for the controller obtained via MediaRouter::GetRouteController().
MediaRouterUI will send media commands (e.g. play/pause) to the controller
through the reference held by the observer. The observer will be destroyed when
the route details view is closed.

MediaRouterWebUIMessageHandler will parse media commands from the WebUI and pass
them on to MediaRouterUI. It will also forward status updates from the
controller observer to the WebUI.


The Chromium-side implementation of custom controls will be done in these
patches:
1. Mojo/MediaStatus/MediaRouteController:     http://crrev/2727123002
2. MediaRouter::GetRouteController():         http://crrev/2728543009
3. MRUI/MRWebUIMessageHandler:                this patch
4. Custom controls WebUI:                     http://crrev/2725503002

Custom controls design doc:
https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4G0/edit

BUG=684638
Review-Url: https://codereview.chromium.org/2731033002
Cr-Commit-Position: refs/heads/master@{#468864}
Committed: https://chromium.googlesource.com/chromium/src/+/e91ca334b8194bc324044057aa6b9f8bf898b1a7

[takumif, 2017-03-03 22:01:50]

Description was changed from

==========
[Media Router] Custom Controls 3 - add piping through MRUI and
MRWebUIMessageHandler

BUG=
==========

to

==========
[Media Router] Custom Controls 3 - add plumbing through MRUI and
MRWebUIMessageHandler

The Chromium-side implementation of custom controls will be done in these
patches:
1. Mojo/MediaStatus/MediaRouteController:     http://crrev/2727123002
2. MediaRouter::CreateMediaRouteController(): http://crrev/2728543009
3. MRUI/MRWebUIMessageHandler:                this patch
4. Custom controls WebUI:                     http://crrev/2725503002

Custom controls design doc:
https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4...

BUG=684638
==========

[takumif, 2017-03-23 18:11:58]

Description was changed from

==========
[Media Router] Custom Controls 3 - add plumbing through MRUI and
MRWebUIMessageHandler

The Chromium-side implementation of custom controls will be done in these
patches:
1. Mojo/MediaStatus/MediaRouteController:     http://crrev/2727123002
2. MediaRouter::CreateMediaRouteController(): http://crrev/2728543009
3. MRUI/MRWebUIMessageHandler:                this patch
4. Custom controls WebUI:                     http://crrev/2725503002

Custom controls design doc:
https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4...

BUG=684638
==========

to

==========
[Media Router] Custom Controls 3 - add plumbing through MRUI and
MRWebUIMessageHandler

The Chromium-side implementation of custom controls will be done in these
patches:
1. Mojo/MediaStatus/MediaRouteController:     http://crrev/2727123002
2. MediaRouter::GetRouteController():         http://crrev/2728543009
3. MRUI/MRWebUIMessageHandler:                this patch
4. Custom controls WebUI:                     http://crrev/2725503002

Custom controls design doc:
https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4...

BUG=684638
==========

[takumif, 2017-03-23 18:35:46]

Description was changed from

==========
[Media Router] Custom Controls 3 - add plumbing through MRUI and
MRWebUIMessageHandler

The Chromium-side implementation of custom controls will be done in these
patches:
1. Mojo/MediaStatus/MediaRouteController:     http://crrev/2727123002
2. MediaRouter::GetRouteController():         http://crrev/2728543009
3. MRUI/MRWebUIMessageHandler:                this patch
4. Custom controls WebUI:                     http://crrev/2725503002

Custom controls design doc:
https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4...

BUG=684638
==========

to

==========
[Media Router] Custom Controls 3 - add plumbing through MRUI and
MRWebUIMessageHandler

When the route details view is opened, MediaRouterUI will instantiate an
observer for the controller obtained via MediaRouter::GetRouteController(). The
observer will be destroyed when the route details view is closed. MediaRouterUI
will send media commands (e.g. play/pause) to the controller through the
reference held by the observer.

MediaRouterWebUIMessageHandler will parse media commands from the WebUI and pass
them on to MediaRouterUI. It will also forward status updates from the
controller observer to the WebUI.


The Chromium-side implementation of custom controls will be done in these
patches:
1. Mojo/MediaStatus/MediaRouteController:     http://crrev/2727123002
2. MediaRouter::GetRouteController():         http://crrev/2728543009
3. MRUI/MRWebUIMessageHandler:                this patch
4. Custom controls WebUI:                     http://crrev/2725503002

Custom controls design doc:
https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4...

BUG=684638
==========

[takumif, 2017-03-23 21:04:59]

Description was changed from

==========
[Media Router] Custom Controls 3 - add plumbing through MRUI and
MRWebUIMessageHandler

When the route details view is opened, MediaRouterUI will instantiate an
observer for the controller obtained via MediaRouter::GetRouteController(). The
observer will be destroyed when the route details view is closed. MediaRouterUI
will send media commands (e.g. play/pause) to the controller through the
reference held by the observer.

MediaRouterWebUIMessageHandler will parse media commands from the WebUI and pass
them on to MediaRouterUI. It will also forward status updates from the
controller observer to the WebUI.


The Chromium-side implementation of custom controls will be done in these
patches:
1. Mojo/MediaStatus/MediaRouteController:     http://crrev/2727123002
2. MediaRouter::GetRouteController():         http://crrev/2728543009
3. MRUI/MRWebUIMessageHandler:                this patch
4. Custom controls WebUI:                     http://crrev/2725503002

Custom controls design doc:
https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4...

BUG=684638
==========

to

==========
[Media Router] Custom Controls 3 - add plumbing through MRUI and
MRWebUIMessageHandler

When the route details view is opened, MediaRouterUI will instantiate an
observer for the controller obtained via MediaRouter::GetRouteController().
MediaRouterUI will send media commands (e.g. play/pause) to the controller
through the reference held by the observer. The observer will be destroyed when
the route details view is closed.

MediaRouterWebUIMessageHandler will parse media commands from the WebUI and pass
them on to MediaRouterUI. It will also forward status updates from the
controller observer to the WebUI.


The Chromium-side implementation of custom controls will be done in these
patches:
1. Mojo/MediaStatus/MediaRouteController:     http://crrev/2727123002
2. MediaRouter::GetRouteController():         http://crrev/2728543009
3. MRUI/MRWebUIMessageHandler:                this patch
4. Custom controls WebUI:                     http://crrev/2725503002

Custom controls design doc:
https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4...

BUG=684638
==========

[takumif, 2017-03-25 00:39:50]

Patchset #3 (id:40001) has been deleted

[takumif, 2017-03-25 16:20:14]

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

[takumif, 2017-03-25 16:20:15]

Please take a look, thanks!

[takumif, 2017-04-10 18:36:58]

Patchset #6 (id:120001) has been deleted

[takumif, 2017-04-12 23:55:16]

Patchset #6 (id:140001) has been deleted

[takumif, 2017-04-17 18:09:00]

Patchset #7 (id:180001) has been deleted

[takumif, 2017-04-17 18:14:54]

Patchset #6 (id:160001) has been deleted

[takumif, 2017-04-21 18:12:20]

Patchset #7 (id:220001) has been deleted

[imcheng, 2017-04-22 00:22:51]

I wanted to see if we can remove some of the boilerplate for the controller
methods, but otherwise looks good. I will take a look at the tests separately.

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

https://codereview.chromium.org/2731033002/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.cc:805: void
MediaRouterUI::OnUIDetailsViewOpened(MediaRoute::Id route_id) {
const ref for route_id

https://codereview.chromium.org/2731033002/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.cc:808:
route_controller_observer_ = base::MakeUnique<UIMediaRouteControllerObserver>(
What if route_controller_observer_ is already non-null?

https://codereview.chromium.org/2731033002/diff/240001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_ui.h (right):

https://codereview.chromium.org/2731033002/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.h:170: virtual void
PlayRoute();
These are duplicated method signatures from MediaRouteController. Does the
implementation simply delegate to the actual MediaRouteController? Maybe this
class can expose a GetController() method that can be called from
MediaRouterWebUIMessageHandler.

https://codereview.chromium.org/2731033002/diff/240001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
(right):

https://codereview.chromium.org/2731033002/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:346:
web_ui()->CallJavascriptFunctionUnsafe(kOnRouteControllerInvalidated,
Can you call the 1-arg version of CallJavascriptFunctionUnsafe here?

https://codereview.chromium.org/2731033002/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:346:
web_ui()->CallJavascriptFunctionUnsafe(kOnRouteControllerInvalidated,
Would this be called as a result of abnormal termination of connection with the
MR extension? Not needed in this patch, but consider adding an issue to notify
the user here.

https://codereview.chromium.org/2731033002/diff/240001/chrome/common/chrome_f...
File chrome/common/chrome_features.cc (right):

https://codereview.chromium.org/2731033002/diff/240001/chrome/common/chrome_f...
chrome/common/chrome_features.cc:209: // Replaces the current route details view
in the Media Router dialog provided
I would probably phrase it as:

If enabled, replaces the <extensionview> controller in the route details view of
the Media Router dialog with the controller bundled with the WebUI resources.

https://codereview.chromium.org/2731033002/diff/240001/chrome/common/chrome_f...
chrome/common/chrome_features.cc:213: // TODO: Do FEATURE_DISABLED_BY_DEFAULT
instead
Resolve TODO?

https://codereview.chromium.org/2731033002/diff/240001/chrome/common/chrome_f...
File chrome/common/chrome_features.h (right):

https://codereview.chromium.org/2731033002/diff/240001/chrome/common/chrome_f...
chrome/common/chrome_features.h:119: extern const base::Feature
kMediaRouterNewRouteController;
Maybe kMediaRouterUIRouteController?

[takumif, 2017-04-24 20:46:43]

Patchset #8 (id:260001) has been deleted

[takumif, 2017-04-24 20:59:24]

Thanks for reviewing!

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

https://codereview.chromium.org/2731033002/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.cc:805: void
MediaRouterUI::OnUIDetailsViewOpened(MediaRoute::Id route_id) {
On 2017/04/22 00:22:51, imcheng wrote:
> const ref for route_id

Done.

https://codereview.chromium.org/2731033002/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.cc:808:
route_controller_observer_ = base::MakeUnique<UIMediaRouteControllerObserver>(
On 2017/04/22 00:22:51, imcheng wrote:
> What if route_controller_observer_ is already non-null?

That should be fine. Given we can only have the route details view open for one
route at a time per dialog, it's okay to dispose the old observer. That said, it
would be null here because this won't get called without OnUIDetailsViewClosed()
in between.

https://codereview.chromium.org/2731033002/diff/240001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_ui.h (right):

https://codereview.chromium.org/2731033002/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.h:170: virtual void
PlayRoute();
On 2017/04/22 00:22:51, imcheng wrote:
> These are duplicated method signatures from MediaRouteController. Does the
> implementation simply delegate to the actual MediaRouteController? Maybe this
> class can expose a GetController() method that can be called from
> MediaRouterWebUIMessageHandler.

I realized that we should sanitize the arguments for seek and set-volume here,
so leaving them here for now. We could also make the WebUIMessageHandler retain
the current MediaStatus and validate arguments, and call controller methods
directly. But at that point I feel like we could/should just have
WebUIMessageHandler (rather than MRUI) own an observer for the controller.

https://codereview.chromium.org/2731033002/diff/240001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
(right):

https://codereview.chromium.org/2731033002/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:346:
web_ui()->CallJavascriptFunctionUnsafe(kOnRouteControllerInvalidated,
On 2017/04/22 00:22:51, imcheng wrote:
> Would this be called as a result of abnormal termination of connection with
the
> MR extension? Not needed in this patch, but consider adding an issue to notify
> the user here. 

Acknowledged.

https://codereview.chromium.org/2731033002/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:346:
web_ui()->CallJavascriptFunctionUnsafe(kOnRouteControllerInvalidated,
On 2017/04/22 00:22:51, imcheng wrote:
> Can you call the 1-arg version of CallJavascriptFunctionUnsafe here?

Done.

https://codereview.chromium.org/2731033002/diff/240001/chrome/common/chrome_f...
File chrome/common/chrome_features.cc (right):

https://codereview.chromium.org/2731033002/diff/240001/chrome/common/chrome_f...
chrome/common/chrome_features.cc:209: // Replaces the current route details view
in the Media Router dialog provided
On 2017/04/22 00:22:51, imcheng wrote:
> I would probably phrase it as:
> 
> If enabled, replaces the <extensionview> controller in the route details view
of
> the Media Router dialog with the controller bundled with the WebUI resources.

Done.

https://codereview.chromium.org/2731033002/diff/240001/chrome/common/chrome_f...
chrome/common/chrome_features.cc:213: // TODO: Do FEATURE_DISABLED_BY_DEFAULT
instead
On 2017/04/22 00:22:51, imcheng wrote:
> Resolve TODO?

Done.

https://codereview.chromium.org/2731033002/diff/240001/chrome/common/chrome_f...
File chrome/common/chrome_features.h (right):

https://codereview.chromium.org/2731033002/diff/240001/chrome/common/chrome_f...
chrome/common/chrome_features.h:119: extern const base::Feature
kMediaRouterNewRouteController;
On 2017/04/22 00:22:51, imcheng wrote:
> Maybe kMediaRouterUIRouteController?

Done.

[mark a. foltz, 2017-04-26 21:01:19]

No major concerns, mostly bike shedding on naming.

Sorry that this took writing reams of boilerplate code in the WebUI message
handler.   We discussed leveraging mojo for this communication channel during
the design discussions - where did we land with that?

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

https://codereview.chromium.org/2731033002/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.cc:819: this,
router_->GetRouteController(route_id));
If the route is invalid, does |router_| return a nullptr route controller?
Maybe this can be simplified to eliminate IsValidRouteId.

https://codereview.chromium.org/2731033002/diff/280001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_ui.h (right):

https://codereview.chromium.org/2731033002/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.h:170: virtual void
PlayRoute();
This doesn't seem like quite the right API - you're not playing or pausing the
UI.  It also duplicates what is already defined in MediaRouteController.

Can the MediaRouterUI return a constant reference to the MediaRouteController so
its API can be used directly in the handler?

Or can these be rephrased in terms of what is being controlled:
PlayCurrentMedia(), etc.?

https://codereview.chromium.org/2731033002/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.h:176: // Gets a reference
to the MediaRouteController for |route_id| and
Does this need a return value?

https://codereview.chromium.org/2731033002/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.h:178: // virtual for
tests.
I'm not sure the "Marked virtual for tests" comments add a lot.  My thoughts:

- If it's marked virtual the assumption is that it is or can be overridden, in
tests or elsewhere.
- This isn't performance sensitive code so we don't need to justify the expense
of a vtable lookup.

I do think it's important to note public or protected APIs that have that
visibility because of tests.

https://codereview.chromium.org/2731033002/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.h:179: virtual void
OnUIDetailsViewOpened(const MediaRoute::Id& route_id);
This name seems specific to the UI configuration (and not the purpose of the
method).
Perhaps OnMediaControllerUIAvailable() and OnMediaControllerUIClosed()?

https://codereview.chromium.org/2731033002/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.h:255:
base::Optional<MediaStatus> current_media_status_;
This would be simpler as a base::unique_ptr<>.

https://codereview.chromium.org/2731033002/diff/280001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_ui_unittest.cc (right):

https://codereview.chromium.org/2731033002/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui_unittest.cc:682: // Seek
positions greater than the duration or negative should be ignored.
It seems like this is testing behavior of the controller itself (not the UI). 
The test above confirms that commands are relayed to the controller as given.

https://codereview.chromium.org/2731033002/diff/280001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
(right):

https://codereview.chromium.org/2731033002/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:65:
const char kOnRouteDetailsClosed[] = "onRouteDetailsClosed";
onMediaControllerAvailable

https://codereview.chromium.org/2731033002/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:66:
const char kOnRouteDetailsOpened[] = "onRouteDetailsOpened";
OnMediaControllerClosed

https://codereview.chromium.org/2731033002/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:67:
const char kPauseRoute[] = "pauseRoute";
Prefer pauseCurrentMedia, playCurrentMedia, etc.

https://codereview.chromium.org/2731033002/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:342:
std::move(status_value));
imcheng@: What did we conclude about migrating WebUI<->Browser messaging to
Mojo?  Feasible in the long term?

[imcheng, 2017-04-26 21:48:39]

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

https://codereview.chromium.org/2731033002/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.cc:808:
route_controller_observer_ = base::MakeUnique<UIMediaRouteControllerObserver>(
On 2017/04/24 20:59:23, takumif wrote:
> On 2017/04/22 00:22:51, imcheng wrote:
> > What if route_controller_observer_ is already non-null?
> 
> That should be fine. Given we can only have the route details view open for
one
> route at a time per dialog, it's okay to dispose the old observer. That said,
it
> would be null here because this won't get called without
OnUIDetailsViewClosed()
> in between.

In general we can't always trust the WebUI to do the right thing. In this case
we can recover gracefully by destroying the old observer. I might add a DVLOG_IF
to catch this case, but I don't feel strongly.

https://codereview.chromium.org/2731033002/diff/240001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_ui.h (right):

https://codereview.chromium.org/2731033002/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.h:170: virtual void
PlayRoute();
On 2017/04/24 20:59:23, takumif wrote:
> On 2017/04/22 00:22:51, imcheng wrote:
> > These are duplicated method signatures from MediaRouteController. Does the
> > implementation simply delegate to the actual MediaRouteController? Maybe
this
> > class can expose a GetController() method that can be called from
> > MediaRouterWebUIMessageHandler.
> 
> I realized that we should sanitize the arguments for seek and set-volume here,
> so leaving them here for now. We could also make the WebUIMessageHandler
retain
> the current MediaStatus and validate arguments, and call controller methods
> directly. But at that point I feel like we could/should just have
> WebUIMessageHandler (rather than MRUI) own an observer for the controller.

I would have the MessageHandler parse/validate the arguments coming from WebUI
and then have it invoke the controller directly (obtained from MRUI). MRUI
already keeps track of various MediaRouter related objects so it's fine to put
the controller there.

https://codereview.chromium.org/2731033002/diff/280001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
(right):

https://codereview.chromium.org/2731033002/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:342:
std::move(status_value));
On 2017/04/26 21:01:19, mark a. foltz wrote:
> imcheng@: What did we conclude about migrating WebUI<->Browser messaging to
> Mojo?  Feasible in the long term?

We can migrate to Mojo, but just weren't sure how the work will be affected by
the migration to native UI. Most likely we will keep the browser side
MessageHandler logic and the mojom hopefully mostly intact, but the JS-based UI
implementation will have to be rewritten for sure. IMO it's worthwhile to do as
long as most of our work won't be thrown away when it comes to migration.

[takumif, 2017-04-27 03:03:39]

Patchset #9 (id:300001) has been deleted

[takumif, 2017-04-27 03:04:18]

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

https://codereview.chromium.org/2731033002/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.cc:808:
route_controller_observer_ = base::MakeUnique<UIMediaRouteControllerObserver>(
On 2017/04/26 21:48:39, imcheng wrote:
> On 2017/04/24 20:59:23, takumif wrote:
> > On 2017/04/22 00:22:51, imcheng wrote:
> > > What if route_controller_observer_ is already non-null?
> > 
> > That should be fine. Given we can only have the route details view open for
> one
> > route at a time per dialog, it's okay to dispose the old observer. That
said,
> it
> > would be null here because this won't get called without
> OnUIDetailsViewClosed()
> > in between.
> 
> In general we can't always trust the WebUI to do the right thing. In this case
> we can recover gracefully by destroying the old observer. I might add a
DVLOG_IF
> to catch this case, but I don't feel strongly.

Done.

https://codereview.chromium.org/2731033002/diff/240001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_ui.h (right):

https://codereview.chromium.org/2731033002/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.h:170: virtual void
PlayRoute();
On 2017/04/26 21:48:39, imcheng wrote:
> On 2017/04/24 20:59:23, takumif wrote:
> > On 2017/04/22 00:22:51, imcheng wrote:
> > > These are duplicated method signatures from MediaRouteController. Does the
> > > implementation simply delegate to the actual MediaRouteController? Maybe
> this
> > > class can expose a GetController() method that can be called from
> > > MediaRouterWebUIMessageHandler.
> > 
> > I realized that we should sanitize the arguments for seek and set-volume
here,
> > so leaving them here for now. We could also make the WebUIMessageHandler
> retain
> > the current MediaStatus and validate arguments, and call controller methods
> > directly. But at that point I feel like we could/should just have
> > WebUIMessageHandler (rather than MRUI) own an observer for the controller.
> 
> I would have the MessageHandler parse/validate the arguments coming from WebUI
> and then have it invoke the controller directly (obtained from MRUI). MRUI
> already keeps track of various MediaRouter related objects so it's fine to put
> the controller there.

Okay, making the MessageHandler call the controller methods.

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

https://codereview.chromium.org/2731033002/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.cc:819: this,
router_->GetRouteController(route_id));
On 2017/04/26 21:01:18, mark a. foltz wrote:
> If the route is invalid, does |router_| return a nullptr route controller?
> Maybe this can be simplified to eliminate IsValidRouteId.

Right, MediaRouter would return null in that case. Removed IsValidRouteId.

https://codereview.chromium.org/2731033002/diff/280001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_ui.h (right):

https://codereview.chromium.org/2731033002/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.h:170: virtual void
PlayRoute();
On 2017/04/26 21:01:18, mark a. foltz wrote:
> This doesn't seem like quite the right API - you're not playing or pausing the
> UI.  It also duplicates what is already defined in MediaRouteController.
> 
> Can the MediaRouterUI return a constant reference to the MediaRouteController
so
> its API can be used directly in the handler?
> 
> Or can these be rephrased in terms of what is being controlled:
> PlayCurrentMedia(), etc.?

Making MessageHandler call the controller methods directly.

https://codereview.chromium.org/2731033002/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.h:176: // Gets a reference
to the MediaRouteController for |route_id| and
On 2017/04/26 21:01:18, mark a. foltz wrote:
> Does this need a return value?

Maybe the part about obtaining a reference shouldn't be mentioned since it's
more of an implementation detail. Changing the comment.

https://codereview.chromium.org/2731033002/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.h:178: // virtual for
tests.
On 2017/04/26 21:01:18, mark a. foltz wrote:
> I'm not sure the "Marked virtual for tests" comments add a lot.  My thoughts:
> 
> - If it's marked virtual the assumption is that it is or can be overridden, in
> tests or elsewhere.
> - This isn't performance sensitive code so we don't need to justify the
expense
> of a vtable lookup.
> 
> I do think it's important to note public or protected APIs that have that
> visibility because of tests.
> 
> 

Okay, makes sense. removed.

https://codereview.chromium.org/2731033002/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.h:179: virtual void
OnUIDetailsViewOpened(const MediaRoute::Id& route_id);
On 2017/04/26 21:01:18, mark a. foltz wrote:
> This name seems specific to the UI configuration (and not the purpose of the
> method).
> Perhaps OnMediaControllerUIAvailable() and OnMediaControllerUIClosed()?

Done.

https://codereview.chromium.org/2731033002/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.h:255:
base::Optional<MediaStatus> current_media_status_;
On 2017/04/26 21:01:18, mark a. foltz wrote:
> This would be simpler as a base::unique_ptr<>.

Removing, since we'll be calling the controller methods from the MessageHandler
directly.

https://codereview.chromium.org/2731033002/diff/280001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_ui_unittest.cc (right):

https://codereview.chromium.org/2731033002/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui_unittest.cc:682: // Seek
positions greater than the duration or negative should be ignored.
On 2017/04/26 21:01:18, mark a. foltz wrote:
> It seems like this is testing behavior of the controller itself (not the UI). 
> The test above confirms that commands are relayed to the controller as given.

The logic to reject invalid arguments was in MRUI (moved the logic and this test
to the MessageHandler).

https://codereview.chromium.org/2731033002/diff/280001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
(right):

https://codereview.chromium.org/2731033002/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:65:
const char kOnRouteDetailsClosed[] = "onRouteDetailsClosed";
On 2017/04/26 21:01:19, mark a. foltz wrote:
> onMediaControllerAvailable

Done.

https://codereview.chromium.org/2731033002/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:66:
const char kOnRouteDetailsOpened[] = "onRouteDetailsOpened";
On 2017/04/26 21:01:18, mark a. foltz wrote:
> OnMediaControllerClosed

Done.

https://codereview.chromium.org/2731033002/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:67:
const char kPauseRoute[] = "pauseRoute";
On 2017/04/26 21:01:19, mark a. foltz wrote:
> Prefer pauseCurrentMedia, playCurrentMedia, etc.

Done.

[mark a. foltz, 2017-05-01 21:28:43]

LGTM % a couple of remaining comments

Thanks, this looks like a clean design, except for the message handling
boilerplate :-P

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

https://codereview.chromium.org/2731033002/diff/320001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.cc:166:
ui_->handler_->UpdateMediaRouteStatus(status);
Can you make this a method on MediaRouterUI as is done below so the observer
doesn't have to know about |handler_|?

https://codereview.chromium.org/2731033002/diff/320001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_ui.h (right):

https://codereview.chromium.org/2731033002/diff/320001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.h:170: // for media status
updates.
This documentation describes the implementation but not the contract for callers
of the API.
Maybe:

"Called when a media controller UI surface is created.  Creates an observer..."

https://codereview.chromium.org/2731033002/diff/320001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.h:172: // Resets the
observer for MediaRouteController.
Similar comment here.

[imcheng, 2017-05-02 19:16:10]

lgtm

https://codereview.chromium.org/2731033002/diff/320001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
(right):

https://codereview.chromium.org/2731033002/diff/320001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:906:
time_delta <= current_media_status_->duration) {
Is this validation necessary or can it be done in the controller implementation?
(So we can get rid of current_media_status_)

[takumif, 2017-05-02 21:33:08]

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

[commit-bot: I haz the power, 2017-05-02 21:33:46]

Dry run: CQ is trying da patch.

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

[takumif, 2017-05-02 21:37:37]

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

https://codereview.chromium.org/2731033002/diff/320001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.cc:166:
ui_->handler_->UpdateMediaRouteStatus(status);
On 2017/05/01 21:28:42, mark a. foltz wrote:
> Can you make this a method on MediaRouterUI as is done below so the observer
> doesn't have to know about |handler_|?

Done.

https://codereview.chromium.org/2731033002/diff/320001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_ui.h (right):

https://codereview.chromium.org/2731033002/diff/320001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.h:170: // for media status
updates.
On 2017/05/01 21:28:43, mark a. foltz wrote:
> This documentation describes the implementation but not the contract for
callers
> of the API.
> Maybe:
> 
> "Called when a media controller UI surface is created.  Creates an
observer..."

Done.

https://codereview.chromium.org/2731033002/diff/320001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui.h:172: // Resets the
observer for MediaRouteController.
On 2017/05/01 21:28:43, mark a. foltz wrote:
> Similar comment here.

Done.

https://codereview.chromium.org/2731033002/diff/320001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
(right):

https://codereview.chromium.org/2731033002/diff/320001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:906:
time_delta <= current_media_status_->duration) {
On 2017/05/02 19:16:10, imcheng wrote:
> Is this validation necessary or can it be done in the controller
implementation?
> (So we can get rid of current_media_status_)

Yes, we could do the validation in the extension, but I'm more inclined to catch
invalid inputs as soon as possible, rather than relying on the controller
implementation. I feel uncomfortable using the mojo controller interface with
potentially invalid arguments.

[commit-bot: I haz the power, 2017-05-02 21:37:44]

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

[commit-bot: I haz the power, 2017-05-02 21:37:45]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[takumif, 2017-05-02 21:44:55]

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

[commit-bot: I haz the power, 2017-05-02 21:45:24]

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-05-02 22:29:59]

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

[commit-bot: I haz the power, 2017-05-02 22:30:00]

Dry run: This issue passed the CQ dry run.

[takumif, 2017-05-03 01:19:56]

The CQ bit was checked by takumif@chromium.org

[takumif, 2017-05-03 01:19:56]

The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org,
imcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2731033002/#ps360001
(title: "Rebase")

[commit-bot: I haz the power, 2017-05-03 01:21:11]

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-05-03 01:54:06]

CQ is committing da patch.

Bot data: {"patchset_id": 360001, "attempt_start_ts": 1493774396237130,
"parent_rev": "268bd8b3738aa5e18df731c881893a797032b37b", "commit_rev":
"e91ca334b8194bc324044057aa6b9f8bf898b1a7"}

[commit-bot: I haz the power, 2017-05-03 01:54:15]

Description was changed from

==========
[Media Router] Custom Controls 3 - add plumbing through MRUI and
MRWebUIMessageHandler

When the route details view is opened, MediaRouterUI will instantiate an
observer for the controller obtained via MediaRouter::GetRouteController().
MediaRouterUI will send media commands (e.g. play/pause) to the controller
through the reference held by the observer. The observer will be destroyed when
the route details view is closed.

MediaRouterWebUIMessageHandler will parse media commands from the WebUI and pass
them on to MediaRouterUI. It will also forward status updates from the
controller observer to the WebUI.


The Chromium-side implementation of custom controls will be done in these
patches:
1. Mojo/MediaStatus/MediaRouteController:     http://crrev/2727123002
2. MediaRouter::GetRouteController():         http://crrev/2728543009
3. MRUI/MRWebUIMessageHandler:                this patch
4. Custom controls WebUI:                     http://crrev/2725503002

Custom controls design doc:
https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4...

BUG=684638
==========

to

==========
[Media Router] Custom Controls 3 - add plumbing through MRUI and
MRWebUIMessageHandler

When the route details view is opened, MediaRouterUI will instantiate an
observer for the controller obtained via MediaRouter::GetRouteController().
MediaRouterUI will send media commands (e.g. play/pause) to the controller
through the reference held by the observer. The observer will be destroyed when
the route details view is closed.

MediaRouterWebUIMessageHandler will parse media commands from the WebUI and pass
them on to MediaRouterUI. It will also forward status updates from the
controller observer to the WebUI.


The Chromium-side implementation of custom controls will be done in these
patches:
1. Mojo/MediaStatus/MediaRouteController:     http://crrev/2727123002
2. MediaRouter::GetRouteController():         http://crrev/2728543009
3. MRUI/MRWebUIMessageHandler:                this patch
4. Custom controls WebUI:                     http://crrev/2725503002

Custom controls design doc:
https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4...

BUG=684638

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

[commit-bot: I haz the power, 2017-05-03 01:54:19]

Committed patchset #11 (id:360001) as
https://chromium.googlesource.com/chromium/src/+/e91ca334b8194bc324044057aa6b...

[findit-for-me, 2017-05-03 04:00:05]

Findit (https://goo.gl/kROfz5) identified this CL at revision 468864 as the
culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

[msramek, 2017-05-03 07:11:57]

msramek@chromium.org changed reviewers:
+ msramek@chromium.org

[msramek, 2017-05-03 07:11:58]

Sheriff here, I agree with FindIt. This seems to have broken unit_tests on all
OSes. I'll revert.

https://build.chromium.org/p/chromium.win/builders/Win10%20Tests%20x64/builds...

https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%...

https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests/builds/406...

[msramek, 2017-05-03 07:13:27]

A revert of this CL (patchset #11 id:360001) has been created in
https://codereview.chromium.org/2862513002/ by msramek@chromium.org.

The reason for reverting is: Breaks unit_tests on all OSes:

https://build.chromium.org/p/chromium.win/builders/Win10%20Tests%20x64/builds...

https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%...

https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests/builds/406....