media: Use new wrapper types for media mojo interfaces

media: Use new wrapper types for media mojo interfaces

Now optional parameters are converted to base::Optional<>. I found it annoying for optional array or string which are converted to base::Optional<std::vector<T>> and base::Optional<std::string>:
- It's confusing that it can be null and/or empty. In almost all cases we only need one type of "invalid" parameter.
- In C++ we pass vector/string by const-ref so they cannot be null.
So in this CL, I make array/string parameters mandatory, so that if they are not needed, we'll just pass empty array/string. This is easier to understand, more consistent with C++ code we have, and we don't need to deal with base::Optional.

TBR=dkrahn@chromium.org
BUG=611224
TEST=No functionality change

Committed: https://crrev.com/7f6b08cb25cebbc09e7b61859942c57c1895fb47
Cr-Commit-Position: refs/heads/master@{#424765}

[xhwang, 2016-10-11 00:58:09]

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

[commit-bot: I haz the power, 2016-10-11 00:58:30]

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-10-11 01:10:18]

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

[commit-bot: I haz the power, 2016-10-11 01:10:19]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[xhwang, 2016-10-11 17:07:57]

rebase only

[xhwang, 2016-10-11 17:08:05]

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

[commit-bot: I haz the power, 2016-10-11 17:08:31]

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-10-11 17:19:01]

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

[commit-bot: I haz the power, 2016-10-11 17:19:03]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[xhwang, 2016-10-11 18:01:38]

Description was changed from

==========
media: Use new wrapper types for media mojo interfaces

BUG=611224
TEST=No functionality change
==========

to

==========
media: Use new wrapper types for media mojo interfaces

Now optional parameters are converted to base::Optional<>. I found it annoying
for optional array or string which are converted to
base::Optional<std::vector<T>> and base::Optional<std::string>:
- It's confusing that it can be null and/or empty. In almost all cases we only
need one type of "invalid" parameter.
- In C++ we pass vector/string by const-ref so they cannot be null.
So in this CL, I make array/string parameters mandatory, so that if they are not
needed, we'll just pass empty array/string. This is easier to understand, more
consistent with C++ code we have, and we don't need to deal with base::Optional.

BUG=611224
TEST=No functionality change
==========

[xhwang, 2016-10-11 18:02:56]

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

[commit-bot: I haz the power, 2016-10-11 18:03:17]

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-10-11 18:25:04]

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

[commit-bot: I haz the power, 2016-10-11 18:25:04]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[xhwang, 2016-10-11 19:22:28]

Patchset #3 (id:40001) has been deleted

[xhwang, 2016-10-11 19:22:35]

Patchset #2 (id:20001) has been deleted

[xhwang, 2016-10-11 19:23:17]

Patchset #1 (id:1) has been deleted

[xhwang, 2016-10-11 19:25:12]

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

[commit-bot: I haz the power, 2016-10-11 19:25:46]

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-10-11 19:38:17]

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

[commit-bot: I haz the power, 2016-10-11 19:38:18]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[xhwang, 2016-10-11 20:21:31]

xhwang@chromium.org changed reviewers:
+ dcheng@chromium.org, jrummell@chromium.org

[xhwang, 2016-10-11 20:21:31]

jrummell: PTAL at everything
dcheng: Please OWNERS review mojom files and type converters

[jrummell, 2016-10-11 22:25:21]

LGTM. std::vector<> conversions use "const &" sometimes, other times not. Just
checking that this is the desired pattern.

https://codereview.chromium.org/2411573002/diff/60001/media/mojo/clients/mojo...
File media/mojo/clients/mojo_cdm.cc (right):

https://codereview.chromium.org/2411573002/diff/60001/media/mojo/clients/mojo...
media/mojo/clients/mojo_cdm.cc:257: std::vector<mojom::CdmKeyInformationPtr>
keys_info) {
const & ?

https://codereview.chromium.org/2411573002/diff/60001/media/mojo/clients/mojo...
File media/mojo/clients/mojo_cdm.h (right):

https://codereview.chromium.org/2411573002/diff/60001/media/mojo/clients/mojo...
media/mojo/clients/mojo_cdm.h:99: std::vector<mojom::CdmKeyInformationPtr>
keys_info) final;
const & ?

https://codereview.chromium.org/2411573002/diff/60001/media/mojo/clients/mojo...
File media/mojo/clients/mojo_decryptor.h (right):

https://codereview.chromium.org/2411573002/diff/60001/media/mojo/clients/mojo...
media/mojo/clients/mojo_decryptor.h:57: std::vector<mojom::AudioBufferPtr>
audio_buffers);
const & ?

[xhwang, 2016-10-11 22:33:41]

On 2016/10/11 22:25:21, jrummell wrote:
> LGTM. std::vector<> conversions use "const &" sometimes, other times not. Just
> checking that this is the desired pattern.
> 
>
https://codereview.chromium.org/2411573002/diff/60001/media/mojo/clients/mojo...
> File media/mojo/clients/mojo_cdm.cc (right):
> 
>
https://codereview.chromium.org/2411573002/diff/60001/media/mojo/clients/mojo...
> media/mojo/clients/mojo_cdm.cc:257: std::vector<mojom::CdmKeyInformationPtr>
> keys_info) {
> const & ?
> 
>
https://codereview.chromium.org/2411573002/diff/60001/media/mojo/clients/mojo...
> File media/mojo/clients/mojo_cdm.h (right):
> 
>
https://codereview.chromium.org/2411573002/diff/60001/media/mojo/clients/mojo...
> media/mojo/clients/mojo_cdm.h:99: std::vector<mojom::CdmKeyInformationPtr>
> keys_info) final;
> const & ?
> 
>
https://codereview.chromium.org/2411573002/diff/60001/media/mojo/clients/mojo...
> File media/mojo/clients/mojo_decryptor.h (right):
> 
>
https://codereview.chromium.org/2411573002/diff/60001/media/mojo/clients/mojo...
> media/mojo/clients/mojo_decryptor.h:57: std::vector<mojom::AudioBufferPtr>
> audio_buffers);
> const & ?

There are from the generated code where it passes the vector by value is the
target type is move-only:

https://docs.google.com/document/d/1dutUW6XUlbSJzsqmtmPnN-J7R8z-iEL-QJ_GR9hWK...

[dcheng, 2016-10-11 23:53:34]

mojo lgtm

out of curiosity, did you find nullable or empty easier to reason about?

https://chromiumcodereview.appspot.com/2411573002/diff/60001/media/mojo/servi...
File media/mojo/services/mojo_cdm_promise.h (right):

https://chromiumcodereview.appspot.com/2411573002/diff/60001/media/mojo/servi...
media/mojo/services/mojo_cdm_promise.h:21: typedef
base::Callback<void(mojom::CdmPromiseResultPtr, const T&...)>
Nit: since we're changing this anyway, let's use using CallbackType =
base::Callback<...>; rather than typedef.

[xhwang, 2016-10-12 00:00:30]

On 2016/10/11 23:53:34, dcheng wrote:
> mojo lgtm
> 
> out of curiosity, did you find nullable or empty easier to reason about?

It really depends. I like the idea of nullable, but in cases where empty string
means "invalid" (which we do in a lot of C++ code), nullable/empty can be
confusing.

Also, I am not a fan of base::Optional syntax yet. Maybe because it's new and I
am not used to it.

>
https://chromiumcodereview.appspot.com/2411573002/diff/60001/media/mojo/servi...
> File media/mojo/services/mojo_cdm_promise.h (right):
> 
>
https://chromiumcodereview.appspot.com/2411573002/diff/60001/media/mojo/servi...
> media/mojo/services/mojo_cdm_promise.h:21: typedef
> base::Callback<void(mojom::CdmPromiseResultPtr, const T&...)>
> Nit: since we're changing this anyway, let's use using CallbackType =
> base::Callback<...>; rather than typedef.

[xhwang, 2016-10-12 00:00:39]

https://chromiumcodereview.appspot.com/2411573002/diff/60001/media/mojo/servi...
File media/mojo/services/mojo_cdm_promise.h (right):

https://chromiumcodereview.appspot.com/2411573002/diff/60001/media/mojo/servi...
media/mojo/services/mojo_cdm_promise.h:21: typedef
base::Callback<void(mojom::CdmPromiseResultPtr, const T&...)>
On 2016/10/11 23:53:34, dcheng wrote:
> Nit: since we're changing this anyway, let's use using CallbackType =
> base::Callback<...>; rather than typedef.

Done.

[xhwang, 2016-10-12 00:01:14]

comments addressed

[xhwang, 2016-10-12 00:01:20]

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

[commit-bot: I haz the power, 2016-10-12 00:02:23]

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-10-12 00:15:41]

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

[commit-bot: I haz the power, 2016-10-12 00:15:42]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[xhwang, 2016-10-12 16:41:49]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2016-10-12 16:41:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
jrummell@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2411573002/#ps80001
(title: "comments addressed")

[commit-bot: I haz the power, 2016-10-12 16:42:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xhwang, 2016-10-12 16:42:42]

Description was changed from

==========
media: Use new wrapper types for media mojo interfaces

Now optional parameters are converted to base::Optional<>. I found it annoying
for optional array or string which are converted to
base::Optional<std::vector<T>> and base::Optional<std::string>:
- It's confusing that it can be null and/or empty. In almost all cases we only
need one type of "invalid" parameter.
- In C++ we pass vector/string by const-ref so they cannot be null.
So in this CL, I make array/string parameters mandatory, so that if they are not
needed, we'll just pass empty array/string. This is easier to understand, more
consistent with C++ code we have, and we don't need to deal with base::Optional.

BUG=611224
TEST=No functionality change
==========

to

==========
media: Use new wrapper types for media mojo interfaces

Now optional parameters are converted to base::Optional<>. I found it annoying
for optional array or string which are converted to
base::Optional<std::vector<T>> and base::Optional<std::string>:
- It's confusing that it can be null and/or empty. In almost all cases we only
need one type of "invalid" parameter.
- In C++ we pass vector/string by const-ref so they cannot be null.
So in this CL, I make array/string parameters mandatory, so that if they are not
needed, we'll just pass empty array/string. This is easier to understand, more
consistent with C++ code we have, and we don't need to deal with base::Optional.

TBR=dkrahn@chromium.org
BUG=611224
TEST=No functionality change
==========

[commit-bot: I haz the power, 2016-10-12 16:52:57]

Description was changed from

==========
media: Use new wrapper types for media mojo interfaces

Now optional parameters are converted to base::Optional<>. I found it annoying
for optional array or string which are converted to
base::Optional<std::vector<T>> and base::Optional<std::string>:
- It's confusing that it can be null and/or empty. In almost all cases we only
need one type of "invalid" parameter.
- In C++ we pass vector/string by const-ref so they cannot be null.
So in this CL, I make array/string parameters mandatory, so that if they are not
needed, we'll just pass empty array/string. This is easier to understand, more
consistent with C++ code we have, and we don't need to deal with base::Optional.

TBR=dkrahn@chromium.org
BUG=611224
TEST=No functionality change
==========

to

==========
media: Use new wrapper types for media mojo interfaces

Now optional parameters are converted to base::Optional<>. I found it annoying
for optional array or string which are converted to
base::Optional<std::vector<T>> and base::Optional<std::string>:
- It's confusing that it can be null and/or empty. In almost all cases we only
need one type of "invalid" parameter.
- In C++ we pass vector/string by const-ref so they cannot be null.
So in this CL, I make array/string parameters mandatory, so that if they are not
needed, we'll just pass empty array/string. This is easier to understand, more
consistent with C++ code we have, and we don't need to deal with base::Optional.

TBR=dkrahn@chromium.org
BUG=611224
TEST=No functionality change
==========

[commit-bot: I haz the power, 2016-10-12 16:53:01]

Committed patchset #2 (id:80001)

[commit-bot: I haz the power, 2016-10-12 17:49:15]

Description was changed from

==========
media: Use new wrapper types for media mojo interfaces

Now optional parameters are converted to base::Optional<>. I found it annoying
for optional array or string which are converted to
base::Optional<std::vector<T>> and base::Optional<std::string>:
- It's confusing that it can be null and/or empty. In almost all cases we only
need one type of "invalid" parameter.
- In C++ we pass vector/string by const-ref so they cannot be null.
So in this CL, I make array/string parameters mandatory, so that if they are not
needed, we'll just pass empty array/string. This is easier to understand, more
consistent with C++ code we have, and we don't need to deal with base::Optional.

TBR=dkrahn@chromium.org
BUG=611224
TEST=No functionality change
==========

to

==========
media: Use new wrapper types for media mojo interfaces

Now optional parameters are converted to base::Optional<>. I found it annoying
for optional array or string which are converted to
base::Optional<std::vector<T>> and base::Optional<std::string>:
- It's confusing that it can be null and/or empty. In almost all cases we only
need one type of "invalid" parameter.
- In C++ we pass vector/string by const-ref so they cannot be null.
So in this CL, I make array/string parameters mandatory, so that if they are not
needed, we'll just pass empty array/string. This is easier to understand, more
consistent with C++ code we have, and we don't need to deal with base::Optional.

TBR=dkrahn@chromium.org
BUG=611224
TEST=No functionality change

Committed: https://crrev.com/7f6b08cb25cebbc09e7b61859942c57c1895fb47
Cr-Commit-Position: refs/heads/master@{#424765}
==========

[commit-bot: I haz the power, 2016-10-12 17:49:16]

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