[MediaRouter] Implement send binary message from PSDImpl to MRPManager

[MediaRouter] Implement send binary message from PSDImpl to MRPManager.

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

[USE s.singapati at gmail.com, 2015-07-21 16:06:09]

s.singapati@samsung.com changed reviewers:
+ avayvod@chromium.org, haibinlu@chromium.org, imcheng@chromium.org,
imcheng@google.com, mfoltz@chromium.org

[USE s.singapati at gmail.com, 2015-07-21 16:06:09]

PTAL.

[haibinlu, 2015-07-21 23:00:15]

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

https://codereview.chromium.org/1245213002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_mojo_impl.cc:252: NOTIMPLEMENTED();
implement this since you add method to media_router.mojom.

[USE s.singapati at gmail.com, 2015-07-22 16:00:58]

s.singapati@samsung.com changed reviewers:
+ kmarshall@chromium.org

[USE s.singapati at gmail.com, 2015-07-22 16:00:58]

PTAL.

[USE s.singapati at gmail.com, 2015-07-22 16:09:51]

PTAL.

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

https://codereview.chromium.org/1245213002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_mojo_impl.cc:252: NOTIMPLEMENTED();
On 2015/07/21 23:00:15, haibinlu wrote:
> implement this since you add method to media_router.mojom.

Done.

[Kevin M, 2015-07-22 18:31:50]

https://codereview.chromium.org/1245213002/diff/20002/chrome/browser/media/an...
File chrome/browser/media/android/router/media_router_android.cc (right):

https://codereview.chromium.org/1245213002/diff/20002/chrome/browser/media/an...
chrome/browser/media/android/router/media_router_android.cc:45: const
std::vector<uint8_t>& data,
uint8

https://codereview.chromium.org/1245213002/diff/20002/chrome/browser/media/an...
File chrome/browser/media/android/router/media_router_android.h (right):

https://codereview.chromium.org/1245213002/diff/20002/chrome/browser/media/an...
chrome/browser/media/android/router/media_router_android.h:40: const
std::vector<uint8_t>& data,
uint8

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

https://codereview.chromium.org/1245213002/diff/20002/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:89: // Sends |data| to a MediaSink
connected via MediaRoute with |route_id|.
Can you add some detail to the comment noting why this is different than
SendRouteMessage?

https://codereview.chromium.org/1245213002/diff/20002/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:92: const std::vector<uint8_t>& data,
uint8

https://codereview.chromium.org/1245213002/diff/20002/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_mojo_impl_unittest.cc (right):

https://codereview.chromium.org/1245213002/diff/20002/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl_unittest.cc:51: const uint8_t
kBinaryMessage[] = {0x01, 0x02, 0x03, 0x04};
uint8

https://codereview.chromium.org/1245213002/diff/20002/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl_unittest.cc:396:
in_data.assign(kBinaryMessage, kBinaryMessage + sizeof(kBinaryMessage));
Use arraysize() instead of sizeof()

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

https://codereview.chromium.org/1245213002/diff/20002/extensions/renderer/res...
extensions/renderer/resources/media_router_bindings.js:490: * @param
{!Array.<uint8>} data
"uint8" isn't a valid JS type?

[Kevin M, 2015-07-22 18:34:04]

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

https://codereview.chromium.org/1245213002/diff/20002/extensions/renderer/res...
extensions/renderer/resources/media_router_bindings.js:497: .then(function() {
Please refer to the fix in CL #1249083003 and make a similar change here

[imcheng, 2015-07-22 20:51:31]

https://codereview.chromium.org/1245213002/diff/20002/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_mojo_impl_unittest.cc (right):

https://codereview.chromium.org/1245213002/diff/20002/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl_unittest.cc:395:
std::vector<uint8_t> in_data;
nit: you can pass in the data range during construction of |in_data|.

https://codereview.chromium.org/1245213002/diff/20002/chrome/browser/media/ro...
File chrome/browser/media/router/test_helper.h (right):

https://codereview.chromium.org/1245213002/diff/20002/chrome/browser/media/ro...
chrome/browser/media/router/test_helper.h:66: const SendRouteMessageCallback&
callback) {
nit: override

[USE s.singapati at gmail.com, 2015-07-23 19:52:53]

PTAL.

https://codereview.chromium.org/1245213002/diff/20002/chrome/browser/media/an...
File chrome/browser/media/android/router/media_router_android.cc (right):

https://codereview.chromium.org/1245213002/diff/20002/chrome/browser/media/an...
chrome/browser/media/android/router/media_router_android.cc:45: const
std::vector<uint8_t>& data,
On 2015/07/22 18:31:50, Kevin M wrote:
> uint8

Done.

https://codereview.chromium.org/1245213002/diff/20002/chrome/browser/media/an...
File chrome/browser/media/android/router/media_router_android.h (right):

https://codereview.chromium.org/1245213002/diff/20002/chrome/browser/media/an...
chrome/browser/media/android/router/media_router_android.h:40: const
std::vector<uint8_t>& data,
On 2015/07/22 18:31:50, Kevin M wrote:
> uint8

Done.

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

https://codereview.chromium.org/1245213002/diff/20002/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:89: // Sends |data| to a MediaSink
connected via MediaRoute with |route_id|.
On 2015/07/22 18:31:50, Kevin M wrote:
> Can you add some detail to the comment noting why this is different than
> SendRouteMessage?

Done.

https://codereview.chromium.org/1245213002/diff/20002/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:92: const std::vector<uint8_t>& data,
On 2015/07/22 18:31:50, Kevin M wrote:
> uint8

Done.

https://codereview.chromium.org/1245213002/diff/20002/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_mojo_impl_unittest.cc (right):

https://codereview.chromium.org/1245213002/diff/20002/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl_unittest.cc:51: const uint8_t
kBinaryMessage[] = {0x01, 0x02, 0x03, 0x04};
On 2015/07/22 18:31:50, Kevin M wrote:
> uint8
kBinaryMessage is not used now.

https://codereview.chromium.org/1245213002/diff/20002/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl_unittest.cc:395:
std::vector<uint8_t> in_data;
On 2015/07/22 20:51:31, imcheng1 wrote:
> nit: you can pass in the data range during construction of |in_data|.

Done. Filling the array during construction.

https://codereview.chromium.org/1245213002/diff/20002/chrome/browser/media/ro...
File chrome/browser/media/router/test_helper.h (right):

https://codereview.chromium.org/1245213002/diff/20002/chrome/browser/media/ro...
chrome/browser/media/router/test_helper.h:66: const SendRouteMessageCallback&
callback) {
On 2015/07/22 20:51:31, imcheng1 wrote:
> nit: override

Done.

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

https://codereview.chromium.org/1245213002/diff/20002/extensions/renderer/res...
extensions/renderer/resources/media_router_bindings.js:490: * @param
{!Array.<uint8>} data
On 2015/07/22 18:31:50, Kevin M wrote:
> "uint8" isn't a valid JS type?

Done. Changed to Uint8Array.

https://codereview.chromium.org/1245213002/diff/20002/extensions/renderer/res...
extensions/renderer/resources/media_router_bindings.js:497: .then(function() {
On 2015/07/22 18:34:04, Kevin M wrote:
> Please refer to the fix in CL #1249083003 and make a similar change here

Done.

[Kevin M, 2015-07-25 00:24:09]

lgtm

https://codereview.chromium.org/1245213002/diff/50001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_mojo_impl_unittest.cc (right):

https://codereview.chromium.org/1245213002/diff/50001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl_unittest.cc:403:
cb.Run(true);
Verify that |data| matches |expected_binary_data|?

[haibinlu, 2015-07-25 01:15:53]

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

https://codereview.chromium.org/1245213002/diff/50001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:90: // This supports additional data
types: Blob, ArrayBuffer and ArrayBufferView.
Is "This supports additional data types: Blob, ArrayBuffer and ArrayBufferView."
relevant here?

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

https://codereview.chromium.org/1245213002/diff/50001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:397: route_id,
mojo::Array<uint8>::From(data), callback);
"From" makes a copy of the underlying data. Is there way to avoid that? maybe
pass in 
scoped_ptr<std::vector>

[USE s.singapati at gmail.com, 2015-07-28 15:33:10]

The CQ bit was checked by s.singapati@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-07-28 15:33:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1245213002/70001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1245213002/70001

[commit-bot: I haz the power, 2015-07-28 16:34:20]

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

[commit-bot: I haz the power, 2015-07-28 16:34:21]

Dry run: This issue passed the CQ dry run.

[haibinlu, 2015-07-28 18:57:19]

lgtm

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

https://codereview.chromium.org/1245213002/diff/70001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:254:
base::Unretained(this), route_id, data.get(),
data.Pass()

[USE s.singapati at gmail.com, 2015-07-28 19:11:47]

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

https://codereview.chromium.org/1245213002/diff/70001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:254:
base::Unretained(this), route_id, data.get(),
On 2015/07/28 18:57:19, haibinlu wrote:
> data.Pass()

Then DoSendSessionBinaryMessage() has to take scoped_ptr<std::vector<uint8>>.
And base::Bind() did not allow to take scoped_ptr<>. I will check it once again.

[Kevin M, 2015-07-28 20:03:37]

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

https://codereview.chromium.org/1245213002/diff/70001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:254:
base::Unretained(this), route_id, data.get(),
On 2015/07/28 19:11:46, s.singapati wrote:
> On 2015/07/28 18:57:19, haibinlu wrote:
> > data.Pass()
> 
> Then DoSendSessionBinaryMessage() has to take scoped_ptr<std::vector<uint8>>.
> And base::Bind() did not allow to take scoped_ptr<>. I will check it once
again.
>  

Use base::Passed(data.Pass()).

If you just pass the bare pointer with .get(), you're passing a bad reference,
because |data| goes out of scope and is deleted as soon as
SendRouteBinaryMessage finishes.

[Kevin M, 2015-07-28 20:04:26]

not lgtm

Canceling LGTM until ownership of byte array is resolved

[USE s.singapati at gmail.com, 2015-07-29 11:22:01]

PTAL.

https://codereview.chromium.org/1245213002/diff/50001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_mojo_impl_unittest.cc (right):

https://codereview.chromium.org/1245213002/diff/50001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl_unittest.cc:403:
cb.Run(true);
On 2015/07/25 00:24:09, Kevin M wrote:
> Verify that |data| matches |expected_binary_data|?

Done.

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

https://codereview.chromium.org/1245213002/diff/70001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:254:
base::Unretained(this), route_id, data.get(),
On 2015/07/28 20:03:36, Kevin M wrote:
> On 2015/07/28 19:11:46, s.singapati wrote:
> > On 2015/07/28 18:57:19, haibinlu wrote:
> > > data.Pass()
> > 
> > Then DoSendSessionBinaryMessage() has to take
scoped_ptr<std::vector<uint8>>.
> > And base::Bind() did not allow to take scoped_ptr<>. I will check it once
> again.
> >  
> 
> Use base::Passed(data.Pass()).
> 
> If you just pass the bare pointer with .get(), you're passing a bad reference,
> because |data| goes out of scope and is deleted as soon as
> SendRouteBinaryMessage finishes.

Done.

[USE s.singapati at gmail.com, 2015-07-30 04:43:36]

The CQ bit was checked by s.singapati@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-07-30 04:44:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1245213002/110001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1245213002/110001

[commit-bot: I haz the power, 2015-07-30 04:44:17]

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

[commit-bot: I haz the power, 2015-07-30 04:44:18]

Dry run: A disapproval has been posted by following reviewers:
kmarshall@chromium.org.
Please make sure to get positive review before another CQ attempt.

[USE s.singapati at gmail.com, 2015-07-30 04:44:55]

PTAL.

[Kevin M, 2015-07-30 20:30:34]

lgtm

[USE s.singapati at gmail.com, 2015-07-30 21:43:49]

The CQ bit was checked by s.singapati@samsung.com

[USE s.singapati at gmail.com, 2015-07-30 21:43:49]

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/1245213002/#ps110001
(title: "updated UT to verify the |data|")

[commit-bot: I haz the power, 2015-07-30 21:45:18]

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

[commit-bot: I haz the power, 2015-07-30 21:57:57]

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

[commit-bot: I haz the power, 2015-07-30 21:57:58]

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

[USE s.singapati at gmail.com, 2015-07-31 02:57:19]

Hi Anton, could you please review chrome/browser/media/android/router/ ?

Thanks.

[whywhat, 2015-07-31 11:07:29]

The CQ bit was checked by avayvod@chromium.org

[whywhat, 2015-07-31 11:07:30]

lgtm

[commit-bot: I haz the power, 2015-07-31 11:07:51]

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

[commit-bot: I haz the power, 2015-07-31 11:13:12]

Committed patchset #7 (id:110001)

[commit-bot: I haz the power, 2015-07-31 11:14:01]

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