[Presentation API] Change ListenForSessionMessages API to client-style.

[Presentation API] Change ListenForSessionMessages API to observers.

ListenForSessionMessages is now observer style and listens for a single
presentation as opposed to all sessions in that frame.

This change also separate presentation url / id from the
various SessionMessage structs from presentation mojom / content.
Now SendSessionMessage / ListenForSessionMessages must pass in an
explicit PresentationSessionInfo. Doing this allows us to avoid parsing
presenation url/id from route id for presentation session - MediaRoute
association, because previously we would have to fill out the url/id
field of content::PresentationSessionMessage struct and there was no
good way to do it without parsing. Now we simply bind the
PresentationSessionInfo to the callback that receives the messages.
Removing the pres url/id from the message struct also avoids
duplication when sending a batch of messages for the same session.

Added PresentationSessionMessagesObserver class and MediaRouter API
to implement the observer style API. MediaRouterMojoImpl contains
logic to adapt the observer stlye API to the getNext style API for
listening for route messages from the Media Route Provider.

Another change that were needed to completely remove the route id
parsing logic is to invoke MediaRouteResponseCallbacks with the
resulting presentation id generated by MediaRouter. This is needed
due to browser-initiated presentations where the presentation id is
not known in advance.

BUG=510297, 510267
Committed: https://crrev.com/a0b6dc9205a195a4fa8c52a71fa1cb2ae8abb591
Cr-Commit-Position: refs/heads/master@{#342249}

[imcheng, 2015-07-29 21:40:12]

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

[imcheng, 2015-07-29 21:40:13]

mark, Haibin: PTAL, thanks. I'm not sure on the
PresentationSessionMessagesObserver naming so I am open to suggestions.

[mark a. foltz, 2015-07-30 22:52:44]

On 2015/07/29 at 21:40:13, imcheng wrote:
> mark, Haibin: PTAL, thanks. I'm not sure on the
PresentationSessionMessagesObserver naming so I am open to suggestions.

I'll have to look at this when I return from my trip.  Feel free to loop in wez@
or kmarshall@ in the meantime.

[imcheng, 2015-07-30 22:57:27]

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

[imcheng, 2015-07-30 22:58:00]

Move mfoltz@ to CC
Add kmarshall@ as reviewer. Kevin, PTAL at everything. Thanks.

[haibinlu, 2015-07-30 23:16:31]

send out first few comments.

https://codereview.chromium.org/1259073004/diff/40001/content/common/presenta...
File content/common/presentation/presentation_service.mojom (right):

https://codereview.chromium.org/1259073004/diff/40001/content/common/presenta...
content/common/presentation/presentation_service.mojom:104:
ListenForSessionMessages(PresentationSessionInfo sessionInfo);
when will this be called? After api client attaches onmessage handler? If so,
shall there be also StopListen...?

https://codereview.chromium.org/1259073004/diff/40001/content/renderer/presen...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/1259073004/diff/40001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:196:
request.session_info.Clone(), request.message.Clone(),
why clone the potentially large message? move is better here.

https://codereview.chromium.org/1259073004/diff/40001/content/renderer/presen...
File content/renderer/presentation/presentation_dispatcher.h (right):

https://codereview.chromium.org/1259073004/diff/40001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.h:43: static
SendMessageRequest* GetSendStringMessageRequest(
TextMessage?

https://codereview.chromium.org/1259073004/diff/40001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.h:47: static
SendMessageRequest* GetSendDataMessageRequest(
BinaryMessage

[haibinlu, 2015-07-30 23:59:11]

https://codereview.chromium.org/1259073004/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/1259073004/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:30: // |presentation_id|: The
presentation ID of the route created, or empty string
make it clear that this is MR generated unique ID.

https://codereview.chromium.org/1259073004/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/1259073004/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:366: // Note that if we
are still listening for messages for that route, we will
issue stop listening right away.

https://codereview.chromium.org/1259073004/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_mojo_impl.h (right):

https://codereview.chromium.org/1259073004/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.h:190: //
|OnRouteMessagesReceived()| to be invoked after all observers for that
if all observers for the route message have been removed, we should stop listen
for messages of this route. Without observers, sending messages from extension
to MR is bad. Shall we add a StopListenForMessage method too?

https://codereview.chromium.org/1259073004/diff/40001/content/browser/present...
File content/browser/presentation/presentation_service_impl.cc (right):

https://codereview.chromium.org/1259073004/diff/40001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:39: output->data =
mojo::Array<uint8_t>::From(input.data);
"From" uses copy instead of move

[imcheng, 2015-08-03 18:56:32]

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

[imcheng, 2015-08-03 18:56:33]

PTAL. I will rebase in a separate PS.

https://codereview.chromium.org/1259073004/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/1259073004/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:30: // |presentation_id|: The
presentation ID of the route created, or empty string
On 2015/07/30 23:59:11, haibinlu wrote:
> make it clear that this is MR generated unique ID.

Done.

https://codereview.chromium.org/1259073004/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/1259073004/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:366: // Note that if we
are still listening for messages for that route, we will
On 2015/07/30 23:59:11, haibinlu wrote:
> issue stop listening right away.

Added a TODO here (see other comment). Also changed OnRouteMessagesReceived
logic to stop listening if empty array is received.

https://codereview.chromium.org/1259073004/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_mojo_impl.h (right):

https://codereview.chromium.org/1259073004/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.h:190: //
|OnRouteMessagesReceived()| to be invoked after all observers for that
On 2015/07/30 23:59:11, haibinlu wrote:
> if all observers for the route message have been removed, we should stop
listen
> for messages of this route. Without observers, sending messages from extension
> to MR is bad. Shall we add a StopListenForMessage method too? 

Currently we stop listening when we see there are no more observers when
messages from the extension arrive. It would be ideal to introduce a
StopListenForMessage so it would work better in the corner cases. Shall we do
this in a future patch after the extension side of StopListenForMessage is
implemented?

https://codereview.chromium.org/1259073004/diff/40001/content/browser/present...
File content/browser/presentation/presentation_service_impl.cc (right):

https://codereview.chromium.org/1259073004/diff/40001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:39: output->data =
mojo::Array<uint8_t>::From(input.data);
On 2015/07/30 23:59:11, haibinlu wrote:
> "From" uses copy instead of move

We use copy here because the original message (|input| here) has to be
distributed to more than one observers.

https://codereview.chromium.org/1259073004/diff/40001/content/common/presenta...
File content/common/presentation/presentation_service.mojom (right):

https://codereview.chromium.org/1259073004/diff/40001/content/common/presenta...
content/common/presentation/presentation_service.mojom:104:
ListenForSessionMessages(PresentationSessionInfo sessionInfo);
On 2015/07/30 23:16:30, haibinlu wrote:
> when will this be called? After api client attaches onmessage handler? If so,
> shall there be also StopListen...?

This is called after session is created which is the same as current behavior.
Blink changes are required to only call this when an event handler is attached,
in which case it will make sense to add a StopListen, similar to
ScreenAvailability.

https://codereview.chromium.org/1259073004/diff/40001/content/renderer/presen...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/1259073004/diff/40001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:196:
request.session_info.Clone(), request.message.Clone(),
On 2015/07/30 23:16:30, haibinlu wrote:
> why clone the potentially large message? move is better here.

Yes, it should be safe to use Pass() here. The |message_request_queue_| logic is
a bit strange, perhaps it should be addressed in a separate patch.

https://codereview.chromium.org/1259073004/diff/40001/content/renderer/presen...
File content/renderer/presentation/presentation_dispatcher.h (right):

https://codereview.chromium.org/1259073004/diff/40001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.h:43: static
SendMessageRequest* GetSendStringMessageRequest(
On 2015/07/30 23:16:30, haibinlu wrote:
> TextMessage?

Done.

https://codereview.chromium.org/1259073004/diff/40001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.h:47: static
SendMessageRequest* GetSendDataMessageRequest(
On 2015/07/30 23:16:30, haibinlu wrote:
> BinaryMessage

Done.

[imcheng, 2015-08-04 23:06:59]

Ping

[mark a. foltz, 2015-08-04 23:47:00]

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

[mark a. foltz, 2015-08-04 23:47:02]

Looks good overall, mostly minor comments on naming and documentation.  This API
makes more sense as well.

Once the messages are handed off from WebPresentationClient, ideally there
should be zero copies made of the message data before they are handed off to the
Mojo client for the component extension.  Please verify this is the case.

https://codereview.chromium.org/1259073004/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/create_presentation_session_request.cc (right):

https://codereview.chromium.org/1259073004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/create_presentation_session_request.cc:40: //
Overwrite presentation ID.
Update comment?

https://codereview.chromium.org/1259073004/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/1259073004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:29: // |route|: The route created.
created or joined.

https://codereview.chromium.org/1259073004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:31: //     The presentation ID of the
route created. In the case of
.. of the route created or joined.

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

https://codereview.chromium.org/1259073004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:37: std::string
actual_presentation_id;
Is this necessary?  You could use a ternary below.

https://codereview.chromium.org/1259073004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:382: // TODO(imcheng):
Queue a task to stop listening for messages (by asking
It seems like the scenario when an observer is unregistered is when the route is
closed or the PresentationSession goes away (not based on other actions by MRP).

https://codereview.chromium.org/1259073004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:442:
listening_messages_for_routes_.insert(route_id);
It seems like listening_messages_for_routes_ is the same as the keys of
messages_observers_.

https://codereview.chromium.org/1259073004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:455: // can stop
listening.
See my comment above, why do we need an extra protocol to manage the
registration of the observer?

What if we receive the empty message list and there are still observers?  It
seems like the next batch of messages will still go through to them...

https://codereview.chromium.org/1259073004/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_mojo_impl.h (right):

https://codereview.chromium.org/1259073004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.h:194:
scoped_ptr<base::ObserverList<PresentationSessionMessagesObserver>>>
Consider a using declaration for PresentationSessionMessagesObserverList.

https://codereview.chromium.org/1259073004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.h:200:
base::hash_set<MediaRoute::Id> listening_messages_for_routes_;
I think std::set would use a little less memory (no empty buckets).  But you
might check my reasoning on that.

Also route_ids_listening_for_messages_?

https://codereview.chromium.org/1259073004/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/presentation_service_delegate_impl.cc (right):

https://codereview.chromium.org/1259073004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:703: const
std::string& presentation_id,
Should OnRouteReponse just take a content::PresentationSessionInfo?

https://codereview.chromium.org/1259073004/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/presentation_session_messages_observer.h
(right):

https://codereview.chromium.org/1259073004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_session_messages_observer.h:18: //
Observes messages originating from the MediaSink connected to a MediaRoute
Could this class be used for the 1-UA case?  In this case it would handle all
messages going *to* a PresentationSession, not just from a sink.

https://codereview.chromium.org/1259073004/diff/80001/content/browser/present...
File content/browser/presentation/presentation_service_impl.cc (right):

https://codereview.chromium.org/1259073004/diff/80001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:39: output->data =
mojo::Array<uint8_t>::From(*input.data);
Why this change?  It is more efficient to swap pointers.

https://codereview.chromium.org/1259073004/diff/80001/content/renderer/presen...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/1259073004/diff/80001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:430:
PresentationDispatcher::GetSendTextMessageRequest(
Prefer naming these functions CreateXXX since they explicitly new up a
SendMessageRequest.

Or, declare them as SendMessageRequest::CreateTextMessage() etc.

https://codereview.chromium.org/1259073004/diff/80001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:463: std::vector<uint8>
tmp_data_vector(data, data + length);
Can session_message take ownership of |data| and avoid allocating a copy?

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

https://codereview.chromium.org/1259073004/diff/80001/extensions/renderer/res...
extensions/renderer/resources/media_router_bindings.js:509:
MediaRouteProvider.prototype.listenForRouteMessages = function(routeId) {
This looks like a breaking change.  Don't you need to submit a patch to the
component extension?

[mark a. foltz, 2015-08-04 23:48:06]

Couple of more things:

- Please file a crbug motivating the change and update the description
- I didn't quite follow the observer lifetime, maybe a few more comments are
needed there

[haibinlu, 2015-08-05 17:19:52]

https://codereview.chromium.org/1259073004/diff/80001/content/browser/present...
File content/browser/presentation/presentation_service_impl.cc (right):

https://codereview.chromium.org/1259073004/diff/80001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:38:
PRESENTATION_MESSAGE_TYPE_ARRAY_BUFFER;
The majority of cases are one-to-one, not one-to-many. Can we do:

Use "move" for conversion, 
then, if there is more than one presentation sessions that need this message,
use "copy" of presentation::SessionMessage.

[imcheng, 2015-08-05 21:38:35]

PTAL. Updated descriptions and comments. Verified the SendMessage only copies
once in PresentationDispatcher. Note that for string case, we do an additional
copy in MediaRouterMojoImpl if the request is deferred. The behavior exists
currently and we can change that separately if we wish to optimize for that
case.

https://codereview.chromium.org/1259073004/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/create_presentation_session_request.cc (right):

https://codereview.chromium.org/1259073004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/create_presentation_session_request.cc:40: //
Overwrite presentation ID.
On 2015/08/04 23:47:02, mark a. foltz wrote:
> Update comment?

I partially reverted the code here since we still had to override the
presentation id. We should clean up the presentation id here and elsewhere in a
separate patch.

https://codereview.chromium.org/1259073004/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/1259073004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:29: // |route|: The route created.
On 2015/08/04 23:47:02, mark a. foltz wrote:
> created or joined.

Done.

https://codereview.chromium.org/1259073004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:31: //     The presentation ID of the
route created. In the case of
On 2015/08/04 23:47:02, mark a. foltz wrote:
> .. of the route created or joined.

Done.

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

https://codereview.chromium.org/1259073004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:37: std::string
actual_presentation_id;
On 2015/08/04 23:47:02, mark a. foltz wrote:
> Is this necessary?  You could use a ternary below.

I wanted to avoid duplicating the check for error condition in callback.Run()
below.

https://codereview.chromium.org/1259073004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:382: // TODO(imcheng):
Queue a task to stop listening for messages (by asking
On 2015/08/04 23:47:02, mark a. foltz wrote:
> It seems like the scenario when an observer is unregistered is when the route
is
> closed or the PresentationSession goes away (not based on other actions by
MRP).

Yes. The issue here is mojo callbacks cannot be dropped, so if we no longer want
to listen for messages and there is an oustanding mojo callback for it, we have
to induce invocation of callback with empty list on exension side. When MR
receives empty list of messages, it will no longer ask the extension for next
batch of messages (i.e. create another mojo callback)

https://codereview.chromium.org/1259073004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:442:
listening_messages_for_routes_.insert(route_id);
On 2015/08/04 23:47:02, mark a. foltz wrote:
> It seems like listening_messages_for_routes_ is the same as the keys of
> messages_observers_.

They are most of the time. However adding the observer and issuing the command
to extension to start listening doesn't always occur at the same time.
|listening_messages_for_routes_| is more like "the set of route ids for which we
have an oustanding mojo callback for".

https://codereview.chromium.org/1259073004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:455: // can stop
listening.
On 2015/08/04 23:47:02, mark a. foltz wrote:
> See my comment above, why do we need an extra protocol to manage the
> registration of the observer?
> 
> What if we receive the empty message list and there are still observers?  It
> seems like the next batch of messages will still go through to them...

Okay, I reworked this a little bit. When the last observer is gone, we should
still tell the extension to invoke the callback with empty. However, when the
callback is invoked, we should only check if there are any observers to decide
if we should listen again. We only check for emptiness when we notify the
observers.

https://codereview.chromium.org/1259073004/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_mojo_impl.h (right):

https://codereview.chromium.org/1259073004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.h:194:
scoped_ptr<base::ObserverList<PresentationSessionMessagesObserver>>>
On 2015/08/04 23:47:02, mark a. foltz wrote:
> Consider a using declaration for PresentationSessionMessagesObserverList.

Done.

https://codereview.chromium.org/1259073004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.h:200:
base::hash_set<MediaRoute::Id> listening_messages_for_routes_;
On 2015/08/04 23:47:02, mark a. foltz wrote:
> I think std::set would use a little less memory (no empty buckets).  But you
> might check my reasoning on that.
> 
> Also route_ids_listening_for_messages_?

Done.

https://codereview.chromium.org/1259073004/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/presentation_service_delegate_impl.cc (right):

https://codereview.chromium.org/1259073004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:703: const
std::string& presentation_id,
On 2015/08/04 23:47:02, mark a. foltz wrote:
> Should OnRouteReponse just take a content::PresentationSessionInfo?

Taking presentation_id here is easier because it can be used directly in the
MediaRouter API.

https://codereview.chromium.org/1259073004/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/presentation_session_messages_observer.h
(right):

https://codereview.chromium.org/1259073004/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_session_messages_observer.h:18: //
Observes messages originating from the MediaSink connected to a MediaRoute
On 2015/08/04 23:47:02, mark a. foltz wrote:
> Could this class be used for the 1-UA case?  In this case it would handle all
> messages going *to* a PresentationSession, not just from a sink.

That depends if 1-UA messaging will bypass the media router.

https://codereview.chromium.org/1259073004/diff/80001/content/browser/present...
File content/browser/presentation/presentation_service_impl.cc (right):

https://codereview.chromium.org/1259073004/diff/80001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:38:
PRESENTATION_MESSAGE_TYPE_ARRAY_BUFFER;
On 2015/08/05 17:19:51, haibinlu wrote:
> The majority of cases are one-to-one, not one-to-many. Can we do:
> 
> Use "move" for conversion, 
> then, if there is more than one presentation sessions that need this message,
> use "copy" of presentation::SessionMessage.

We could do that but then we will need to have 2 versions of the conversion, the
observer has to take 2 callbacks and implement 2 methods, etc. Let's chat about
that before moving the change?

https://codereview.chromium.org/1259073004/diff/80001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:39: output->data =
mojo::Array<uint8_t>::From(*input.data);
On 2015/08/04 23:47:02, mark a. foltz wrote:
> Why this change?  It is more efficient to swap pointers.

|input| is sent to potentially multiple observers so it cannot be modified here.

https://codereview.chromium.org/1259073004/diff/80001/content/renderer/presen...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/1259073004/diff/80001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:430:
PresentationDispatcher::GetSendTextMessageRequest(
On 2015/08/04 23:47:02, mark a. foltz wrote:
> Prefer naming these functions CreateXXX since they explicitly new up a
> SendMessageRequest.
> 
> Or, declare them as SendMessageRequest::CreateTextMessage() etc.

Done.

https://codereview.chromium.org/1259073004/diff/80001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:463: std::vector<uint8>
tmp_data_vector(data, data + length);
On 2015/08/04 23:47:02, mark a. foltz wrote:
> Can session_message take ownership of |data| and avoid allocating a copy?

Not currently. We will have to change the Blink interface. Comment from
WebPresentationClient.h:

// Embedder copies the |data| and the ownership is not transferred.

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

https://codereview.chromium.org/1259073004/diff/80001/extensions/renderer/res...
extensions/renderer/resources/media_router_bindings.js:509:
MediaRouteProvider.prototype.listenForRouteMessages = function(routeId) {
On 2015/08/04 23:47:02, mark a. foltz wrote:
> This looks like a breaking change.  Don't you need to submit a patch to the
> component extension?

We are still sending an array to the extension so it shouldn't break.

[haibinlu, 2015-08-05 22:17:20]

lgtm

[mark a. foltz, 2015-08-06 00:39:52]

I'm not a fan of the design that relies on the extension to pass an empty
message list to clear  out the pending mojo callbacks.  It's not obvious that
that is what is going on, and is more fragile to rely on another component to
clean up your internal state.

Would it be possible to track the pending callbacks and cancel them explicitly
when the route is closed?

[imcheng, 2015-08-06 18:05:29]

Chatted with mark offline yesterday. There is currently no mechanism to cancel
the mojo callback from the MR side. So the only way to clean up the outstanding
callback is to ask the extension to invoke it. I added comments in MRMojoImpl to
clarify that. In the longer term we can look into converting it into an observer
API over a channel where excessive messages are buffered on extension side only.

[mark a. foltz, 2015-08-06 18:54:01]

lgtm

[Kevin M, 2015-08-06 19:52:00]

lgtm

[imcheng (use chromium acct), 2015-08-06 20:15:49]

The CQ bit was checked by imcheng@google.com

[imcheng (use chromium acct), 2015-08-06 20:15:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
haibinlu@chromium.org
Link to the patchset: https://codereview.chromium.org/1259073004/#ps140001
(title: "update comments")

[commit-bot: I haz the power, 2015-08-06 20:17:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1259073004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1259073004/140001

[commit-bot: I haz the power, 2015-08-06 22:16:47]

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

[commit-bot: I haz the power, 2015-08-06 22:16:48]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[imcheng, 2015-08-06 23:03:42]

The CQ bit was checked by imcheng@chromium.org

[commit-bot: I haz the power, 2015-08-06 23:04:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1259073004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1259073004/140001

[commit-bot: I haz the power, 2015-08-07 01:00:55]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-08-07 01:01:48]

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