[MediaRouter] wireup send/receive messages.

[MediaRouter] wireup send/receive messages.

BUG=501539
Committed: https://crrev.com/7e639ff6407b6a7939a89426821145359dafe58a
Cr-Commit-Position: refs/heads/master@{#336222}

[haibinlu, 2015-06-22 20:41:50]

haibinlu@chromium.org changed reviewers:
+ kmarshall@chromium.org

[haibinlu, 2015-06-22 20:43:33]

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

[imcheng (use chromium acct), 2015-06-22 21:08:07]

imcheng@google.com changed reviewers:
+ imcheng@google.com

[imcheng (use chromium acct), 2015-06-22 21:08:08]

https://codereview.chromium.org/1199933006/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/presentation_service_delegate_impl.cc (right):

https://codereview.chromium.org/1199933006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/presentation_service_delegate_impl.cc:613: const
auto& route_ids = frame_manager_->GetRouteIds(
Please spell out the type instead of using auto here.

https://codereview.chromium.org/1199933006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/presentation_service_delegate_impl.cc:615: if
(route_ids.size() == 0) {
nit: if (route_ids.empty())

https://codereview.chromium.org/1199933006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/presentation_service_delegate_impl.cc:617: return;
It looks like the callback need to be invoked in this case. Otherwise, the mojo
callback in PresentationServiceImpl will never get invoked.

https://codereview.chromium.org/1199933006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/presentation_service_delegate_impl.cc:632: DVLOG(1)
<< "Presentation session was already closed: "
nit: maybe "Presentation session does not exist: " ? See Kevin's comment in the
other CL.

https://codereview.chromium.org/1199933006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/presentation_service_delegate_impl.cc:634: return;
ditto on invoking the callback in early returns here.

[Kevin M, 2015-06-22 22:42:26]

https://codereview.chromium.org/1199933006/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/presentation_service_delegate_impl.cc (right):

https://codereview.chromium.org/1199933006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/presentation_service_delegate_impl.cc:640:
router_->SendRouteMessage(route_id, *(message_request->message.get()),
.get() is unnecessary here.

[haibinlu, 2015-06-22 23:22:41]

PTAL

https://codereview.chromium.org/1199933006/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/presentation_service_delegate_impl.cc (right):

https://codereview.chromium.org/1199933006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/presentation_service_delegate_impl.cc:613: const
auto& route_ids = frame_manager_->GetRouteIds(
On 2015/06/22 21:08:07, imcheng wrote:
> Please spell out the type instead of using auto here.

Done.

https://codereview.chromium.org/1199933006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/presentation_service_delegate_impl.cc:615: if
(route_ids.size() == 0) {
On 2015/06/22 21:08:07, imcheng wrote:
> nit: if (route_ids.empty())

Done.

https://codereview.chromium.org/1199933006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/presentation_service_delegate_impl.cc:617: return;
On 2015/06/22 21:08:07, imcheng wrote:
> It looks like the callback need to be invoked in this case. Otherwise, the
mojo
> callback in PresentationServiceImpl will never get invoked.

Done.

https://codereview.chromium.org/1199933006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/presentation_service_delegate_impl.cc:632: DVLOG(1)
<< "Presentation session was already closed: "
On 2015/06/22 21:08:07, imcheng wrote:
> nit: maybe "Presentation session does not exist: " ? See Kevin's comment in
the
> other CL.

Done.

https://codereview.chromium.org/1199933006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/presentation_service_delegate_impl.cc:634: return;
On 2015/06/22 21:08:07, imcheng wrote:
> ditto on invoking the callback in early returns here.

Done.

https://codereview.chromium.org/1199933006/diff/1/chrome/browser/media/router...
chrome/browser/media/router/presentation_service_delegate_impl.cc:640:
router_->SendRouteMessage(route_id, *(message_request->message.get()),
On 2015/06/22 22:42:26, Kevin M wrote:
> .get() is unnecessary here.

Done.

[Kevin M, 2015-06-23 17:50:18]

lgtm

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

https://codereview.chromium.org/1199933006/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:633: DVLOG(1)
<< "Binary message is not supported yet.";
DVLOG is too mild.

Consider using NOTIMPLEMENTED() instead.

[haibinlu, 2015-06-23 19:59:41]

haibinlu@chromium.org changed reviewers:
+ avayvod@chromium.org

[haibinlu, 2015-06-23 19:59:42]

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

https://codereview.chromium.org/1199933006/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:633: DVLOG(1)
<< "Binary message is not supported yet.";
On 2015/06/23 17:50:18, Kevin M wrote:
> DVLOG is too mild.
> 
> Consider using NOTIMPLEMENTED() instead.

Done.

[imcheng (use chromium acct), 2015-06-23 21:59:37]

lgtm

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

https://codereview.chromium.org/1199933006/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:619:
message_cb.Run(empty_list.Pass());
why not just
message_cb.Run(scoped_ptr<ScopedVector<content::PresentationSessionMessage>>());
?

https://codereview.chromium.org/1199933006/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:631: if
(message_request->is_binary()) {
Also need to run callback and return early in this case?

[haibinlu, 2015-06-23 23:36:13]

haibinlu@chromium.org changed reviewers:
+ avi@chromium.org

[haibinlu, 2015-06-23 23:36:14]

@avi, Do you mind reviewing the change in
content/public/browser/presentation_service_delegate.h? Thanks!

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

https://codereview.chromium.org/1199933006/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:619:
message_cb.Run(empty_list.Pass());
On 2015/06/23 21:59:37, imcheng wrote:
> why not just
>
message_cb.Run(scoped_ptr<ScopedVector<content::PresentationSessionMessage>>());
> ?

Done.

https://codereview.chromium.org/1199933006/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:631: if
(message_request->is_binary()) {
On 2015/06/23 21:59:37, imcheng wrote:
> Also need to run callback and return early in this case?

Done.

[haibinlu, 2015-06-24 21:22:38]

The CQ bit was checked by haibinlu@chromium.org

[haibinlu, 2015-06-24 21:22:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
kmarshall@chromium.org, imcheng@google.com
Link to the patchset: https://codereview.chromium.org/1199933006/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-06-24 21:23:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1199933006/60001

[commit-bot: I haz the power, 2015-06-24 21:31:23]

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

[commit-bot: I haz the power, 2015-06-24 21:31:24]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[haibinlu, 2015-06-25 00:09:28]

Friendly ping

[Avi (use Gerrit), 2015-06-25 14:36:14]

lgtm

[haibinlu, 2015-06-25 17:15:37]

The CQ bit was checked by haibinlu@chromium.org

[haibinlu, 2015-06-25 17:15:37]

The patchset sent to the CQ was uploaded after l-g-t-m from imcheng@google.com,
kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/1199933006/#ps80001
(title: "Update presentation_service_impl_unittest")

[commit-bot: I haz the power, 2015-06-25 17:16:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1199933006/80001

[commit-bot: I haz the power, 2015-06-25 19:15:02]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-06-25 19:16:04]

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