[Media Router] Replace route messaging API.

[Media Router] Replace route messaging API.

Remove old callback-based ListenForRouteMessages API and replace it
with StartListeningForRouteMessages and OnRouteMessagesReceived. The
rationale is return value based API doesn't work well for event pages;
when the event page is suspended, the callback supplied by the old API
is lost and cannot be restored when the extension wakes up. Switching
to the listener model unblocks the work to reduce the nessesity of
keeping the event page alive and should result in considerably
simpler logic on the extension side.

To maintain backwards compatibility with older extensions, a smaller
adapter logic is introduced in media_router_bindings.js.

Add missing WakeReason for StartListeningForRouteMessages.

Also added OWNERs for the mojom file to comply with presubmit.

BUG=625007
Committed: https://crrev.com/bf455be1850399b8b374809cfc2584e0611e1c77
Cr-Commit-Position: refs/heads/master@{#406083}

[imcheng, 2016-07-01 01:39:10]

Patchset #1 (id:1) has been deleted

[imcheng, 2016-07-01 01:39:32]

Description was changed from

==========
[Media Router] Replace route messaging API.

Remove old callback-based ListenForRouteMessages API and replace it
with StartListeningForRouteMessages and OnRouteMessagesReceived. The
rationale is return value based API doesn't work well for event pages;
when the event page is suspended, the callback supplied by the old API
is lost and cannot be restored when the extension wakes up. Switching
to the listener model unblocks the work to reduce the nessesity of
keeping the event page alive and should result in considerably
simpler logic on the extension side.

To maintain backwards compatibility with older extensions, a smaller
adapter logic is introduced in media_router_bindings.js.


Also added OWNERs for the mojom file to comply with presubmit.

BUG=625007
==========

to

==========
[Media Router] Replace route messaging API.

Remove old callback-based ListenForRouteMessages API and replace it
with StartListeningForRouteMessages and OnRouteMessagesReceived. The
rationale is return value based API doesn't work well for event pages;
when the event page is suspended, the callback supplied by the old API
is lost and cannot be restored when the extension wakes up. Switching
to the listener model unblocks the work to reduce the nessesity of
keeping the event page alive and should result in considerably
simpler logic on the extension side.

To maintain backwards compatibility with older extensions, a smaller
adapter logic is introduced in media_router_bindings.js.

Add missing WakeReason for StartListeningForRouteMessages.

Also added OWNERs for the mojom file to comply with presubmit.

BUG=625007
==========

[imcheng, 2016-07-01 01:39:42]

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

[imcheng, 2016-07-01 01:39:43]

PTAL

[mark a. foltz, 2016-07-02 01:18:43]

Overall looks good with a few documentation updates.

Does the mojo_impl unittest need to be updated?

https://codereview.chromium.org/2111303003/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router.mojom (right):

https://codereview.chromium.org/2111303003/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router.mojom:275: // Starts listening for
messages from the given route.
...messages from the media sink for the route given by |route_id|.

https://codereview.chromium.org/2111303003/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router.mojom:278: // Messages will stop
arriving if |StopListeningForRouteMessages| is called.
Messages could still arrive from the sink but the MR won't get them, right?

Maybe:

|StopListeningForRouteMessages| will stop the Media Router from receiving futher
messages for |route_id|.

https://codereview.chromium.org/2111303003/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router.mojom:373: // Called when the a
batch of messages from the given route arrives.
messages arrives from the media sink for the route given by |route_id|.

https://codereview.chromium.org/2111303003/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router.mojom:374: //
|StartListeningForRouteMessages| must be called first in order to recieve
Nit: typo in receive

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

https://codereview.chromium.org/2111303003/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_mojo_metrics.h:37:
START_LISTENING_FOR_ROUTE_MESSAGES = 10,
Please update the corresponding histogram

[mark a. foltz, 2016-07-02 01:23:58]

Regarding backwards compatibility, will we also need to worry about backwards
compatibility with Chromium M53 since the branch was cut today?  I guess it
depends on when the component change lands.

[imcheng, 2016-07-06 17:56:23]

Addressed comments. I plan to merge this patch and the corresponding extension
change back to 53.

https://codereview.chromium.org/2111303003/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router.mojom (right):

https://codereview.chromium.org/2111303003/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router.mojom:275: // Starts listening for
messages from the given route.
On 2016/07/02 01:18:43, mfoltz_ooo_until_july_10 wrote:
> ...messages from the media sink for the route given by |route_id|.

Done.

https://codereview.chromium.org/2111303003/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router.mojom:278: // Messages will stop
arriving if |StopListeningForRouteMessages| is called.
On 2016/07/02 01:18:42, mfoltz_ooo_until_july_10 wrote:
> Messages could still arrive from the sink but the MR won't get them, right?
> 
> Maybe:
> 
> |StopListeningForRouteMessages| will stop the Media Router from receiving
futher
> messages for |route_id|.

Yeah, I meant that the messages will stop arriving at MR. They will still
accumulate, or be dropped if exceeded queue size on the extension side.

https://codereview.chromium.org/2111303003/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router.mojom:373: // Called when the a
batch of messages from the given route arrives.
On 2016/07/02 01:18:42, mfoltz_ooo_until_july_10 wrote:
> messages arrives from the media sink for the route given by |route_id|.

Done.

https://codereview.chromium.org/2111303003/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router.mojom:374: //
|StartListeningForRouteMessages| must be called first in order to recieve
On 2016/07/02 01:18:42, mfoltz_ooo_until_july_10 wrote:
> Nit: typo in receive

Done.

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

https://codereview.chromium.org/2111303003/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_mojo_metrics.h:37:
START_LISTENING_FOR_ROUTE_MESSAGES = 10,
On 2016/07/02 01:18:43, mfoltz_ooo_until_july_10 wrote:
> Please update the corresponding histogram

Done.

[imcheng, 2016-07-06 18:06:50]

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

[imcheng, 2016-07-06 18:06:51]

+apacible, PTAL at everything.
+isherman, PTAL at histograms.xml
-mfoltz as he is on vacation.

Thanks

[Ilya Sherman, 2016-07-06 22:50:04]

lgtm

[apacible, 2016-07-07 15:57:44]

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

https://codereview.chromium.org/2111303003/diff/40001/extensions/renderer/res...
extensions/renderer/resources/media_router_bindings.js:466: * TODO(imcheng):
Remove in M55.
I'm likely missing context here -- what's special about M55?

Can you also file a bug for this so it isn't forgotten?

[imcheng, 2016-07-07 18:00:58]

Thanks. I will fix the unit tests shortly.

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

https://codereview.chromium.org/2111303003/diff/40001/extensions/renderer/res...
extensions/renderer/resources/media_router_bindings.js:466: * TODO(imcheng):
Remove in M55.
On 2016/07/07 15:57:44, apacible wrote:
> I'm likely missing context here -- what's special about M55?
> 
> Can you also file a bug for this so it isn't forgotten?

My intention is for this patch to make it to M53. Generally speaking it should
be safe to remove backwards compatibility support two releases later, which
would be M55. I added a crbug and linked it here.

[apacible, 2016-07-07 18:06:27]

lgtm

[imcheng, 2016-07-07 20:58:06]

The CQ bit was checked by imcheng@chromium.org

[imcheng, 2016-07-07 20:58:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, apacible@chromium.org
Link to the patchset: https://codereview.chromium.org/2111303003/#ps80001
(title: "fix tests")

[commit-bot: I haz the power, 2016-07-07 20:58:36]

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-07 21:09:08]

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

[commit-bot: I haz the power, 2016-07-07 21:09:10]

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...)

[imcheng, 2016-07-07 22:27:36]

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

[imcheng, 2016-07-07 22:27:36]

dcheng: Could you please take a look at the mojom changes?

[dcheng, 2016-07-08 06:15:22]

Has this mojom been security reviewed in the past? It looks like the OWNERS file
is new.

[imcheng, 2016-07-08 06:47:11]

I don't know if we've had a security review specifically for the mojom. We've
had a security review for Media Router in general (crbug.com/510517). The OWNERS
file was added because presubmit requires it, but the mojom file has been there
for more than a year. If a security review for the mojom is required, could you
please let me know how I can get the process started?

[imcheng, 2016-07-11 17:50:14]

Ping for dcheng.

[dcheng, 2016-07-18 16:14:35]

rs lgtm for mojo changes (since this is just rearranging some plumbing and
apparently needs to be merged). Let's address the comments in followups.

https://codereview.chromium.org/2111303003/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/2111303003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:764:
observer_it.GetNext() != nullptr && observer_it.GetNext() == nullptr;
This is... not nice.

1) //content/browser/presentation is also using legacy type converters, which
need to be migrated.
2) If it's important not to repeat work, we should just do the conversion once
anyway.

Also, can we just have RouteMesage or SessionMessage instead of having both?
They're already very similar.

[imcheng, 2016-07-18 18:56:42]

Thanks!

https://codereview.chromium.org/2111303003/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/2111303003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:764:
observer_it.GetNext() != nullptr && observer_it.GetNext() == nullptr;
On 2016/07/18 16:14:34, dcheng wrote:
> This is... not nice.
> 
> 1) //content/browser/presentation is also using legacy type converters, which
> need to be migrated.
> 2) If it's important not to repeat work, we should just do the conversion once
> anyway.
> 
> Also, can we just have RouteMesage or SessionMessage instead of having both?
> They're already very similar.

The message type enum on the two definitions are slightly different. Binary
messages are represented in two types (ARRAY_BUFFER and BLOB) in SessionMessage,
versus just one in RouteMessage. Although I don't see places where ARRAY_BUFFER
and BLOB are treated differently - will need to follow up with avayvod@ /
mlamouri@ on that.

[imcheng, 2016-07-18 19:09:28]

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

[commit-bot: I haz the power, 2016-07-18 19:10:37]

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-18 20:23:46]

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

[commit-bot: I haz the power, 2016-07-18 20:23:47]

Dry run: This issue passed the CQ dry run.

[imcheng, 2016-07-18 20:26:43]

The CQ bit was checked by imcheng@chromium.org

[imcheng, 2016-07-18 20:26:44]

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

[commit-bot: I haz the power, 2016-07-18 20:27:37]

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-18 20:35:04]

Description was changed from

==========
[Media Router] Replace route messaging API.

Remove old callback-based ListenForRouteMessages API and replace it
with StartListeningForRouteMessages and OnRouteMessagesReceived. The
rationale is return value based API doesn't work well for event pages;
when the event page is suspended, the callback supplied by the old API
is lost and cannot be restored when the extension wakes up. Switching
to the listener model unblocks the work to reduce the nessesity of
keeping the event page alive and should result in considerably
simpler logic on the extension side.

To maintain backwards compatibility with older extensions, a smaller
adapter logic is introduced in media_router_bindings.js.

Add missing WakeReason for StartListeningForRouteMessages.

Also added OWNERs for the mojom file to comply with presubmit.

BUG=625007
==========

to

==========
[Media Router] Replace route messaging API.

Remove old callback-based ListenForRouteMessages API and replace it
with StartListeningForRouteMessages and OnRouteMessagesReceived. The
rationale is return value based API doesn't work well for event pages;
when the event page is suspended, the callback supplied by the old API
is lost and cannot be restored when the extension wakes up. Switching
to the listener model unblocks the work to reduce the nessesity of
keeping the event page alive and should result in considerably
simpler logic on the extension side.

To maintain backwards compatibility with older extensions, a smaller
adapter logic is introduced in media_router_bindings.js.

Add missing WakeReason for StartListeningForRouteMessages.

Also added OWNERs for the mojom file to comply with presubmit.

BUG=625007
==========

[commit-bot: I haz the power, 2016-07-18 20:35:06]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-07-18 20:35:10]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-18 20:36:21]

Description was changed from

==========
[Media Router] Replace route messaging API.

Remove old callback-based ListenForRouteMessages API and replace it
with StartListeningForRouteMessages and OnRouteMessagesReceived. The
rationale is return value based API doesn't work well for event pages;
when the event page is suspended, the callback supplied by the old API
is lost and cannot be restored when the extension wakes up. Switching
to the listener model unblocks the work to reduce the nessesity of
keeping the event page alive and should result in considerably
simpler logic on the extension side.

To maintain backwards compatibility with older extensions, a smaller
adapter logic is introduced in media_router_bindings.js.

Add missing WakeReason for StartListeningForRouteMessages.

Also added OWNERs for the mojom file to comply with presubmit.

BUG=625007
==========

to

==========
[Media Router] Replace route messaging API.

Remove old callback-based ListenForRouteMessages API and replace it
with StartListeningForRouteMessages and OnRouteMessagesReceived. The
rationale is return value based API doesn't work well for event pages;
when the event page is suspended, the callback supplied by the old API
is lost and cannot be restored when the extension wakes up. Switching
to the listener model unblocks the work to reduce the nessesity of
keeping the event page alive and should result in considerably
simpler logic on the extension side.

To maintain backwards compatibility with older extensions, a smaller
adapter logic is introduced in media_router_bindings.js.

Add missing WakeReason for StartListeningForRouteMessages.

Also added OWNERs for the mojom file to comply with presubmit.

BUG=625007

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

[commit-bot: I haz the power, 2016-07-18 20:36:22]

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