Deprecated: [Media Router] Ensure dialog shows route details view if there is one local route

Deprecated. New CL is here: https://codereview.chromium.org/2540773005/

[Media Router] Ensure dialog shows route details view if there is one local route

Currently, if the dialog WebUI requests the initial data before MediaRouterUI
receives it, the MediaRouterWebUIMessageHandler sends empty data to the WebUI.
This change removes routes info from the initial data, and makes MediaRouterUI
send the info either when it receives routes data for the first time or when it
is notified that WebUI has been initialized, whichever comes later. The WebUI
holds off on showing the initial view (sink list or route details) until it
receives routes info or 3 seconds passes.

This CL would delay the showing of dialogs in cases in which MediaRouterUI
receives routes data after sending initial data, and there are no local routes.
However, the alternative would be to show the sink list for a moment before
switching to details view (if there's one local route), which would be jarring.

BUG=667361
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

[takumif, 2016-11-21 17:45:07]

Description was changed from

==========
.

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

to

==========
Ensure MediaRouterUI receives routes and sinks data before initializing WebUI

BUG=667361
==========

[takumif, 2016-11-21 22:41:41]

Patchset #2 (id:20001) has been deleted

[takumif, 2016-11-21 22:41:48]

Patchset #1 (id:1) has been deleted

[takumif, 2016-11-21 22:47:21]

Patchset #1 (id:40001) has been deleted

[takumif, 2016-11-21 22:51:36]

Description was changed from

==========
Ensure MediaRouterUI receives routes and sinks data before initializing WebUI

BUG=667361
==========

to

==========
Ensure MediaRouterUI receives routes and sinks data before initializing WebUI

Currently, if the dialog WebUI requests the initial data before MediaRouterUI
receives it, the MediaRouterWebUIMessageHandler sends empty data to the WebUI.
This change ensures that we respond to the request from the WebUI after
MediaRouterUI has the data.

BUG=667361
==========

[takumif, 2016-11-21 22:53:17]

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

[takumif, 2016-11-21 22:53:17]

Please take a look, thank you!

[mark a. foltz, 2016-11-21 23:07:11]

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

[mark a. foltz, 2016-11-21 23:07:11]

Note: make sure we aren't blocking launch/rendering of the WebUI on data from
the Media Router.  We need as much parallelism as we can get.

[takumif, 2016-11-22 00:02:22]

On 2016/11/21 23:07:11, mark a. foltz wrote:
> Note: make sure we aren't blocking launch/rendering of the WebUI on data from
> the Media Router.  We need as much parallelism as we can get.

This change doesn't delay the launch of the dialog.
It may however delay the JavaScript call to show the details view or the sink
list view (user would see a loading wheel in the meantime) until MRUI gets
routes data, but this data is necessary for showing the details view.

[mark a. foltz, 2016-11-22 00:36:10]

On 2016/11/22 at 00:02:22, takumif wrote:
> On 2016/11/21 23:07:11, mark a. foltz wrote:
> > Note: make sure we aren't blocking launch/rendering of the WebUI on data
from
> > the Media Router.  We need as much parallelism as we can get.
> 
> This change doesn't delay the launch of the dialog.
> It may however delay the JavaScript call to show the details view or the sink
list view (user would see a loading wheel in the meantime) until MRUI gets
routes data, but this data is necessary for showing the details view.

If it's not blocking paint or rendering the container shell then it should be
fine.  To be safe, please do before/after comparison of the UI tracing variables
with this change.  imcheng@ or apacible@ can point out which ones are relevant.

[imcheng, 2016-11-22 00:48:34]

Thanks for taking this on Takumi. This change looks a bit complicated and we
have to add even more coordination between the message handler and MRUI.

It seems the race condition is causing the JS code to not show route details on
open due to uninitialized route list in the initial data. I am wondering if we
can decouple the route list from the initial data, and only supply route list
via the setRouteList message. On the UI side we will have a boolean to keep
track of whether this is the first time it has been a route list, and execute
the maybeShowRouteDetailsOnOpen logic if so. WDYT?

[takumif, 2016-11-22 01:26:24]

On 2016/11/22 00:48:34, imcheng wrote:
> Thanks for taking this on Takumi. This change looks a bit complicated and we
> have to add even more coordination between the message handler and MRUI.
> 
> It seems the race condition is causing the JS code to not show route details
on
> open due to uninitialized route list in the initial data. I am wondering if we
> can decouple the route list from the initial data, and only supply route list
> via the setRouteList message. On the UI side we will have a boolean to keep
> track of whether this is the first time it has been a route list, and execute
> the maybeShowRouteDetailsOnOpen logic if so. WDYT?

Discussed with Derek offline. To reduce the coordination between MRUI and
MRWebUIMessageHandler, instead of always including the routes in the initial
data, we won't include it if it's not ready by the time the initial data is
requested, and have MRUI send it later when it's ready. The JS side should wait
until it has the routes data to show the details or sinks list view.

[takumif, 2016-11-29 00:31:07]

Description was changed from

==========
Ensure MediaRouterUI receives routes and sinks data before initializing WebUI

Currently, if the dialog WebUI requests the initial data before MediaRouterUI
receives it, the MediaRouterWebUIMessageHandler sends empty data to the WebUI.
This change ensures that we respond to the request from the WebUI after
MediaRouterUI has the data.

BUG=667361
==========

to

==========
Ensure MediaRouterUI receives routes and sinks data before initializing WebUI

Currently, if the dialog WebUI requests the initial data before MediaRouterUI
receives it, the MediaRouterWebUIMessageHandler sends empty data to the WebUI.
This change ensures that we respond to the request from the WebUI after
MediaRouterUI has the data.

BUG=667361
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[takumif, 2016-11-29 00:44:41]

Patchset #2 (id:80001) has been deleted

[takumif, 2016-11-29 01:05:30]

Description was changed from

==========
Ensure MediaRouterUI receives routes and sinks data before initializing WebUI

Currently, if the dialog WebUI requests the initial data before MediaRouterUI
receives it, the MediaRouterWebUIMessageHandler sends empty data to the WebUI.
This change ensures that we respond to the request from the WebUI after
MediaRouterUI has the data.

BUG=667361
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Ensure MediaRouterUI receives routes and sinks data before initializing WebUI

Currently, if the dialog WebUI requests the initial data before MediaRouterUI
receives it, the MediaRouterWebUIMessageHandler sends empty data to the WebUI.
This change removes routes info from the initial data, and makes MediaRouterUI
send the info either when it receives routes data for the first time or when it
is notified that WebUI has been initialized, whichever comes later. The WebUI
holds off on showing the initial view (sink list or route details) until it
receives routes info or 3 seconds passes.

BUG=667361
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[takumif, 2016-11-29 01:05:54]

Description was changed from

==========
Ensure MediaRouterUI receives routes and sinks data before initializing WebUI

Currently, if the dialog WebUI requests the initial data before MediaRouterUI
receives it, the MediaRouterWebUIMessageHandler sends empty data to the WebUI.
This change removes routes info from the initial data, and makes MediaRouterUI
send the info either when it receives routes data for the first time or when it
is notified that WebUI has been initialized, whichever comes later. The WebUI
holds off on showing the initial view (sink list or route details) until it
receives routes info or 3 seconds passes.

BUG=667361
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Ensure MediaRouterUI receives routes and sinks data before initializing WebUI

Currently, if the dialog WebUI requests the initial data before MediaRouterUI
receives it, the MediaRouterWebUIMessageHandler sends empty data to the WebUI.
This change removes routes info from the initial data, and makes MediaRouterUI
send the info either when it receives routes data for the first time or when it
is notified that WebUI has been initialized, whichever comes later. The WebUI
holds off on showing the initial view (sink list or route details) until it
receives routes info or 3 seconds passes.

BUG=667361
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[takumif, 2016-11-29 01:15:05]

Description was changed from

==========
Ensure MediaRouterUI receives routes and sinks data before initializing WebUI

Currently, if the dialog WebUI requests the initial data before MediaRouterUI
receives it, the MediaRouterWebUIMessageHandler sends empty data to the WebUI.
This change removes routes info from the initial data, and makes MediaRouterUI
send the info either when it receives routes data for the first time or when it
is notified that WebUI has been initialized, whichever comes later. The WebUI
holds off on showing the initial view (sink list or route details) until it
receives routes info or 3 seconds passes.

BUG=667361
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Ensure MediaRouterUI receives routes and sinks data before initializing WebUI

Currently, if the dialog WebUI requests the initial data before MediaRouterUI
receives it, the MediaRouterWebUIMessageHandler sends empty data to the WebUI.
This change removes routes info from the initial data, and makes MediaRouterUI
send the info either when it receives routes data for the first time or when it
is notified that WebUI has been initialized, whichever comes later. The WebUI
holds off on showing the initial view (sink list or route details) until it
receives routes info or 3 seconds passes.

This CL would delay the showing of dialogs in cases in which MediaRouterUI
receives routes data after sending initial data, and there are no local routes.
However, the alternative would be to show the sink list for a moment before
switching to details view (if there's one local route), which would be jarring.

BUG=667361
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[takumif, 2016-11-29 01:15:24]

Description was changed from

==========
Ensure MediaRouterUI receives routes and sinks data before initializing WebUI

Currently, if the dialog WebUI requests the initial data before MediaRouterUI
receives it, the MediaRouterWebUIMessageHandler sends empty data to the WebUI.
This change removes routes info from the initial data, and makes MediaRouterUI
send the info either when it receives routes data for the first time or when it
is notified that WebUI has been initialized, whichever comes later. The WebUI
holds off on showing the initial view (sink list or route details) until it
receives routes info or 3 seconds passes.

This CL would delay the showing of dialogs in cases in which MediaRouterUI
receives routes data after sending initial data, and there are no local routes.
However, the alternative would be to show the sink list for a moment before
switching to details view (if there's one local route), which would be jarring.

BUG=667361
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Ensure MediaRouterUI receives routes and sinks data before initializing WebUI

Currently, if the dialog WebUI requests the initial data before MediaRouterUI
receives it, the MediaRouterWebUIMessageHandler sends empty data to the WebUI.
This change removes routes info from the initial data, and makes MediaRouterUI
send the info either when it receives routes data for the first time or when it
is notified that WebUI has been initialized, whichever comes later. The WebUI
holds off on showing the initial view (sink list or route details) until it
receives routes info or 3 seconds passes.

This CL would delay the showing of dialogs in cases in which MediaRouterUI
receives routes data after sending initial data, and there are no local routes.
However, the alternative would be to show the sink list for a moment before
switching to details view (if there's one local route), which would be jarring.

BUG=667361
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[takumif, 2016-11-29 01:18:12]

As per offline discussions with Derek I've changed this CL (info in the CL
description). Please take a look, thank you!

[takumif, 2016-11-29 19:12:18]

Description was changed from

==========
Ensure MediaRouterUI receives routes and sinks data before initializing WebUI

Currently, if the dialog WebUI requests the initial data before MediaRouterUI
receives it, the MediaRouterWebUIMessageHandler sends empty data to the WebUI.
This change removes routes info from the initial data, and makes MediaRouterUI
send the info either when it receives routes data for the first time or when it
is notified that WebUI has been initialized, whichever comes later. The WebUI
holds off on showing the initial view (sink list or route details) until it
receives routes info or 3 seconds passes.

This CL would delay the showing of dialogs in cases in which MediaRouterUI
receives routes data after sending initial data, and there are no local routes.
However, the alternative would be to show the sink list for a moment before
switching to details view (if there's one local route), which would be jarring.

BUG=667361
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[Media Router] Ensure the dialog shows route details view if there is one local
route

Currently, if the dialog WebUI requests the initial data before MediaRouterUI
receives it, the MediaRouterWebUIMessageHandler sends empty data to the WebUI.
This change removes routes info from the initial data, and makes MediaRouterUI
send the info either when it receives routes data for the first time or when it
is notified that WebUI has been initialized, whichever comes later. The WebUI
holds off on showing the initial view (sink list or route details) until it
receives routes info or 3 seconds passes.

This CL would delay the showing of dialogs in cases in which MediaRouterUI
receives routes data after sending initial data, and there are no local routes.
However, the alternative would be to show the sink list for a moment before
switching to details view (if there's one local route), which would be jarring.

BUG=667361
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[takumif, 2016-11-29 19:12:40]

Description was changed from

==========
[Media Router] Ensure the dialog shows route details view if there is one local
route

Currently, if the dialog WebUI requests the initial data before MediaRouterUI
receives it, the MediaRouterWebUIMessageHandler sends empty data to the WebUI.
This change removes routes info from the initial data, and makes MediaRouterUI
send the info either when it receives routes data for the first time or when it
is notified that WebUI has been initialized, whichever comes later. The WebUI
holds off on showing the initial view (sink list or route details) until it
receives routes info or 3 seconds passes.

This CL would delay the showing of dialogs in cases in which MediaRouterUI
receives routes data after sending initial data, and there are no local routes.
However, the alternative would be to show the sink list for a moment before
switching to details view (if there's one local route), which would be jarring.

BUG=667361
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[Media Router] Ensure dialog shows route details view if there is one local
route

Currently, if the dialog WebUI requests the initial data before MediaRouterUI
receives it, the MediaRouterWebUIMessageHandler sends empty data to the WebUI.
This change removes routes info from the initial data, and makes MediaRouterUI
send the info either when it receives routes data for the first time or when it
is notified that WebUI has been initialized, whichever comes later. The WebUI
holds off on showing the initial view (sink list or route details) until it
receives routes info or 3 seconds passes.

This CL would delay the showing of dialogs in cases in which MediaRouterUI
receives routes data after sending initial data, and there are no local routes.
However, the alternative would be to show the sink list for a moment before
switching to details view (if there's one local route), which would be jarring.

BUG=667361
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[mark a. foltz, 2016-11-29 22:14:05]

On 2016/11/29 at 01:18:12, takumif wrote:
> As per offline discussions with Derek I've changed this CL (info in the CL
description). Please take a look, thank you!

If Derek's okay with this design then LGTM.

This is a bit of an artifact that we have made the MR intentionally not keep a
route list, so getting the current list of routes is always going to be async
(register observer + wait for callback).  The MRUI then caches this data for
synchronous access (which is okay, as long as the observer is reliable at
passing updates).

If we move to a model where the MR is a source of truth for the route list, then
we can create a synchronous API to fetch the route list from MR when
instantiating the MediaRouterUI, and pass it with initial data.  That would
simplify the initialization sequence here IIUC.

[mark a. foltz, 2016-11-29 22:24:10]

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

https://codereview.chromium.org/2518083002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/media_router/media_router_ui_unittest.cc:605:
CreateMediaRouterUI(&profile_);
Nit: Calling this twice in the same test re-uses the same TestWebUI.  Check and
make sure that's a supported use for that object.

[imcheng, 2016-11-29 23:54:09]

On 2016/11/29 22:24:10, mark a. foltz wrote:
>
https://codereview.chromium.org/2518083002/diff/100001/chrome/browser/ui/webu...
> File chrome/browser/ui/webui/media_router/media_router_ui_unittest.cc (right):
> 
>
https://codereview.chromium.org/2518083002/diff/100001/chrome/browser/ui/webu...
> chrome/browser/ui/webui/media_router/media_router_ui_unittest.cc:605:
> CreateMediaRouterUI(&profile_);
> Nit: Calling this twice in the same test re-uses the same TestWebUI.  Check
and
> make sure that's a supported use for that object.

I was thinking there are 2 approaches:
1) Make sure we have all necessary data ready when the WebUI requests for it.
2) Have the C++ code push data to WebUI as they are available then WebUI decides
what it should do with the data.

It's possible to enforce (1) by caching route list on the MR side, which would
work because we always have 2 MediaRoutesObserver for the same routes query in
the background (MediaRouterBase, ephemeral icon logic). We can add an API that
contractually reutrns the current route list. The implementation should be
trivial using the observer in MediaRouterBase. 

I do feel that (1) will require less change (since there's little to no change
to the UI logic). (2) is a more general model (say in the future, if there are
data that can't be returned synchronously) but looking at the the WebUI code, it
is already very complex so it might not be a good idea to add any more to it.

[takumif, 2016-11-30 22:32:18]

Description was changed from

==========
[Media Router] Ensure dialog shows route details view if there is one local
route

Currently, if the dialog WebUI requests the initial data before MediaRouterUI
receives it, the MediaRouterWebUIMessageHandler sends empty data to the WebUI.
This change removes routes info from the initial data, and makes MediaRouterUI
send the info either when it receives routes data for the first time or when it
is notified that WebUI has been initialized, whichever comes later. The WebUI
holds off on showing the initial view (sink list or route details) until it
receives routes info or 3 seconds passes.

This CL would delay the showing of dialogs in cases in which MediaRouterUI
receives routes data after sending initial data, and there are no local routes.
However, the alternative would be to show the sink list for a moment before
switching to details view (if there's one local route), which would be jarring.

BUG=667361
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Deprecated. New CL is here: https://codereview.chromium.org/2540773005/

[Media Router] Ensure dialog shows route details view if there is one local
route

Currently, if the dialog WebUI requests the initial data before MediaRouterUI
receives it, the MediaRouterWebUIMessageHandler sends empty data to the WebUI.
This change removes routes info from the initial data, and makes MediaRouterUI
send the info either when it receives routes data for the first time or when it
is notified that WebUI has been initialized, whichever comes later. The WebUI
holds off on showing the initial view (sink list or route details) until it
receives routes info or 3 seconds passes.

This CL would delay the showing of dialogs in cases in which MediaRouterUI
receives routes data after sending initial data, and there are no local routes.
However, the alternative would be to show the sink list for a moment before
switching to details view (if there's one local route), which would be jarring.

BUG=667361
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[mark a. foltz, 2016-11-30 23:02:45]

Assuming you don't intend on landing this patch, I'll close the issue.  Thx