[Media Router] Use TimeDelta in mojo interfaces

[Media Router] Use TimeDelta in mojo interfaces

Use TimeDelta instead of an int representing milliseconds in Media
Router mojo interfaces.

BUG=629375
Committed: https://crrev.com/b7917313d7e4f0d8c83ebc93ef6d6db5e79ad643
Cr-Commit-Position: refs/heads/master@{#418609}

[btolsch, 2016-09-09 23:48:21]

btolsch@chromium.org changed reviewers:
+ dcheng@chromium.org, imcheng@chromium.org, rockot@chromium.org

[btolsch, 2016-09-09 23:48:22]

PTAL, thanks!

[miu, 2016-09-10 00:40:16]

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

[miu, 2016-09-10 00:40:17]

Nice! Some drive-by comments:

https://codereview.chromium.org/2324133003/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/mojo/media_router.mojom (right):

https://codereview.chromium.org/2324133003/diff/1/chrome/browser/media/router...
chrome/browser/media/router/mojo/media_router.mojom:171:
mojo.common.mojom.TimeDelta timeout,
OOC, why not use base::TimeDelta instead (here and throughout)? IMHO, would be
much more readable.

https://codereview.chromium.org/2324133003/diff/1/extensions/renderer/resourc...
File extensions/renderer/resources/media_router_bindings.js (right):

https://codereview.chromium.org/2324133003/diff/1/extensions/renderer/resourc...
extensions/renderer/resources/media_router_bindings.js:605: *     request,
measured in microseconds. Otherwise, the default duration will
nit: Please remove "measured in microseconds" since the timebase of a TimeDelta
is an internal implementation detail. Also, please do this in the comments for
other methods below...

[btolsch, 2016-09-10 00:51:40]

https://codereview.chromium.org/2324133003/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/mojo/media_router.mojom (right):

https://codereview.chromium.org/2324133003/diff/1/chrome/browser/media/router...
chrome/browser/media/router/mojo/media_router.mojom:171:
mojo.common.mojom.TimeDelta timeout,
On 2016/09/10 00:40:17, miu wrote:
> OOC, why not use base::TimeDelta instead (here and throughout)? IMHO, would be
> much more readable.

I'm not sure I understand the question but I'll try to give some reasoning. 
Even when using typemaps, the mojo interface needs to use mojo types;
base::TimeDelta is only known to c++.  TimeDelta was formerly declared a
[Native] struct so that there was no additional implementation needed for mojo
to use it (uses existing ipc::ParamTraits and member definition), but this only
works while in c++.  Since the extension bindings (javascript) need this type,
it needs to be a proper mojo struct.

https://codereview.chromium.org/2324133003/diff/1/extensions/renderer/resourc...
File extensions/renderer/resources/media_router_bindings.js (right):

https://codereview.chromium.org/2324133003/diff/1/extensions/renderer/resourc...
extensions/renderer/resources/media_router_bindings.js:605: *     request,
measured in microseconds. Otherwise, the default duration will
On 2016/09/10 00:40:17, miu wrote:
> nit: Please remove "measured in microseconds" since the timebase of a
TimeDelta
> is an internal implementation detail. Also, please do this in the comments for
> other methods below...

Done.

[miu, 2016-09-10 23:25:40]

lgtm

https://codereview.chromium.org/2324133003/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/mojo/media_router.mojom (right):

https://codereview.chromium.org/2324133003/diff/1/chrome/browser/media/router...
chrome/browser/media/router/mojo/media_router.mojom:171:
mojo.common.mojom.TimeDelta timeout,
On 2016/09/10 00:51:39, btolsch wrote:
> On 2016/09/10 00:40:17, miu wrote:
> > OOC, why not use base::TimeDelta instead (here and throughout)? IMHO, would
be
> > much more readable.
> 
> I'm not sure I understand the question but I'll try to give some reasoning. 
> Even when using typemaps, the mojo interface needs to use mojo types;
> base::TimeDelta is only known to c++.  TimeDelta was formerly declared a
> [Native] struct so that there was no additional implementation needed for mojo
> to use it (uses existing ipc::ParamTraits and member definition), but this
only
> works while in c++.  Since the extension bindings (javascript) need this type,
> it needs to be a proper mojo struct.

Whoops! My mistake. Thought I was looking at the C++ header
(media_router_mojo_impl.h). Never mind. ;-)

[Ken Rockot(use gerrit already), 2016-09-12 20:56:58]

lgtm

[dcheng, 2016-09-13 00:50:01]

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

https://codereview.chromium.org/2324133003/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router.typemap:11:
"mojo.common.mojom.TimeDelta=base::TimeDelta[copyable_pass_by_value]",
I don't think this line should be in this typemap file.

[Ken Rockot(use gerrit already), 2016-09-13 00:56:29]

On 2016/09/13 at 00:50:01, dcheng wrote:
>
https://codereview.chromium.org/2324133003/diff/20001/chrome/browser/media/ro...
> File chrome/browser/media/router/mojo/media_router.typemap (right):
> 
>
https://codereview.chromium.org/2324133003/diff/20001/chrome/browser/media/ro...
> chrome/browser/media/router/mojo/media_router.typemap:11:
"mojo.common.mojom.TimeDelta=base::TimeDelta[copyable_pass_by_value]",
> I don't think this line should be in this typemap file.

Good catch, I missed that. You're right, it should not be.

[btolsch, 2016-09-13 01:21:49]

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

https://codereview.chromium.org/2324133003/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router.typemap:11:
"mojo.common.mojom.TimeDelta=base::TimeDelta[copyable_pass_by_value]",
On 2016/09/13 00:50:00, dcheng wrote:
> I don't think this line should be in this typemap file.

When I didn't have this, I got compile errors about no conversion from
base::TimeDelta to mojo::common::mojom::TimeDeltaPtr.  I know it is already
declared in mojo/common/common_custom_types.typemap but is there something I
need to add for it to be picked up by the chrome/browser/media/router/mojo code?

[Ken Rockot(use gerrit already), 2016-09-13 01:23:40]

On 2016/09/13 at 01:21:49, btolsch wrote:
>
https://codereview.chromium.org/2324133003/diff/20001/chrome/browser/media/ro...
> File chrome/browser/media/router/mojo/media_router.typemap (right):
> 
>
https://codereview.chromium.org/2324133003/diff/20001/chrome/browser/media/ro...
> chrome/browser/media/router/mojo/media_router.typemap:11:
"mojo.common.mojom.TimeDelta=base::TimeDelta[copyable_pass_by_value]",
> On 2016/09/13 00:50:00, dcheng wrote:
> > I don't think this line should be in this typemap file.
> 
> When I didn't have this, I got compile errors about no conversion from
base::TimeDelta to mojo::common::mojom::TimeDeltaPtr.  I know it is already
declared in mojo/common/common_custom_types.typemap but is there something I
need to add for it to be picked up by the chrome/browser/media/router/mojo code?

The mojom build rule which builds media_router.mojom must express a dependency
on //mojo/common:common_custom_types.

[btolsch, 2016-09-13 01:56:32]

On 2016/09/13 01:23:40, Ken Rockot wrote:
> On 2016/09/13 at 01:21:49, btolsch wrote:
> >
>
https://codereview.chromium.org/2324133003/diff/20001/chrome/browser/media/ro...
> > File chrome/browser/media/router/mojo/media_router.typemap (right):
> > 
> >
>
https://codereview.chromium.org/2324133003/diff/20001/chrome/browser/media/ro...
> > chrome/browser/media/router/mojo/media_router.typemap:11:
> "mojo.common.mojom.TimeDelta=base::TimeDelta[copyable_pass_by_value]",
> > On 2016/09/13 00:50:00, dcheng wrote:
> > > I don't think this line should be in this typemap file.
> > 
> > When I didn't have this, I got compile errors about no conversion from
> base::TimeDelta to mojo::common::mojom::TimeDeltaPtr.  I know it is already
> declared in mojo/common/common_custom_types.typemap but is there something I
> need to add for it to be picked up by the chrome/browser/media/router/mojo
code?
> 
> The mojom build rule which builds media_router.mojom must express a dependency
> on //mojo/common:common_custom_types.

Thanks!

[btolsch, 2016-09-13 01:56:44]

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

https://codereview.chromium.org/2324133003/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router.typemap:11:
"mojo.common.mojom.TimeDelta=base::TimeDelta[copyable_pass_by_value]",
On 2016/09/13 01:21:48, btolsch wrote:
> On 2016/09/13 00:50:00, dcheng wrote:
> > I don't think this line should be in this typemap file.
> 
> When I didn't have this, I got compile errors about no conversion from
> base::TimeDelta to mojo::common::mojom::TimeDeltaPtr.  I know it is already
> declared in mojo/common/common_custom_types.typemap but is there something I
> need to add for it to be picked up by the chrome/browser/media/router/mojo
code?

Done.

[dcheng, 2016-09-13 02:42:35]

https://codereview.chromium.org/2324133003/diff/60001/chrome/browser/media/ro...
File chrome/browser/media/router/BUILD.gn (right):

https://codereview.chromium.org/2324133003/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/BUILD.gn:88: "//mojo/common:common_custom_types",
Does this need to be a public dep?

[Ken Rockot(use gerrit already), 2016-09-13 02:45:55]

On 2016/09/13 at 02:42:35, dcheng wrote:
>
https://codereview.chromium.org/2324133003/diff/60001/chrome/browser/media/ro...
> File chrome/browser/media/router/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2324133003/diff/60001/chrome/browser/media/ro...
> chrome/browser/media/router/BUILD.gn:88: "//mojo/common:common_custom_types",
> Does this need to be a public dep?

mojom deps are all treated public deps anyway. But using deps is DEPrecated (see
the note in mojom.gni), so yes, please use public_deps.

[dcheng, 2016-09-13 02:48:48]

On 2016/09/13 02:45:55, Ken Rockot wrote:
> On 2016/09/13 at 02:42:35, dcheng wrote:
> >
>
https://codereview.chromium.org/2324133003/diff/60001/chrome/browser/media/ro...
> > File chrome/browser/media/router/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/2324133003/diff/60001/chrome/browser/media/ro...
> > chrome/browser/media/router/BUILD.gn:88:
"//mojo/common:common_custom_types",
> > Does this need to be a public dep?
> 
> mojom deps are all treated public deps anyway. But using deps is DEPrecated
(see
> the note in mojom.gni), so yes, please use public_deps.

Thanks for the clarification. LGTM

[imcheng, 2016-09-13 22:03:48]

lgtm

[btolsch, 2016-09-14 01:12:35]

The CQ bit was checked by btolsch@chromium.org

[btolsch, 2016-09-14 01:12:35]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
miu@chromium.org
Link to the patchset: https://codereview.chromium.org/2324133003/#ps60001
(title: "Fix timeout rename for joinRoute")

[commit-bot: I haz the power, 2016-09-14 01:13:04]

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-09-14 01:31:15]

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

[commit-bot: I haz the power, 2016-09-14 01:31:17]

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

[btolsch, 2016-09-14 09:31:38]

The CQ bit was checked by btolsch@chromium.org

[btolsch, 2016-09-14 09:31:39]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
imcheng@chromium.org, miu@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2324133003/#ps80001
(title: "Fix mojo test compile")

[commit-bot: I haz the power, 2016-09-14 09:32:11]

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-09-14 10:06:54]

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

[commit-bot: I haz the power, 2016-09-14 10:06:55]

Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[btolsch, 2016-09-14 10:10:28]

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

[commit-bot: I haz the power, 2016-09-14 10:10:39]

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-09-14 11:02:01]

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

[commit-bot: I haz the power, 2016-09-14 11:02:02]

Dry run: This issue passed the CQ dry run.

[btolsch, 2016-09-14 18:03:12]

The CQ bit was checked by btolsch@chromium.org

[commit-bot: I haz the power, 2016-09-14 18:03:47]

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-09-14 18:09:25]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-14 18:11:12]

Description was changed from

==========
[Media Router] Use TimeDelta in mojo interfaces

Use TimeDelta instead of an int representing milliseconds in Media
Router mojo interfaces.

BUG=629375
==========

to

==========
[Media Router] Use TimeDelta in mojo interfaces

Use TimeDelta instead of an int representing milliseconds in Media
Router mojo interfaces.

BUG=629375

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

[commit-bot: I haz the power, 2016-09-14 18:11:14]

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