Add support for mojo::TypeConverter to control how convenient conversion should be.

Add support for mojo::TypeConverter to control how convenient conversion should be.

BUG=None
TEST=None

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266814

[yzshen1, 2014-04-23 00:14:24]

Hi, Darin.

Would you please take a look? Thanks!

https://codereview.chromium.org/209453003/diff/120001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/type_conversion_unittest.cc (right):

https://codereview.chromium.org/209453003/diff/120001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/type_conversion_unittest.cc:1: // Copyright 2013
The Chromium Authors. All rights reserved.
It would be nice to also write some no-compile tests, but I realized that the
support for no-compile tests is currently
disabled.(https://code.google.com/p/chromium/issues/detail?id=105388)

So I didn't add any in this CL.

https://codereview.chromium.org/209453003/diff/120001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/type_converter.h (right):

https://codereview.chromium.org/209453003/diff/120001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/type_converter.h:99: #define
MOJO_ALLOW_DIRECT_TYPE_CONVERSION() \
I think it is more clear to put the macro definitions here, but please let me
know if you think they should be in mojo/public/cpp/system/macros.h

[darin (slow to review), 2014-04-23 18:01:01]

https://codereview.chromium.org/209453003/diff/120001/mojo/common/common_type...
File mojo/common/common_type_converters.h (right):

https://codereview.chromium.org/209453003/diff/120001/mojo/common/common_type...
mojo/common/common_type_converters.h:22: MOJO_ALLOW_DIRECT_TYPE_CONVERSION();
Why don't you like using the term "implicit" here? Isn't that the opposite of
explicit, and in C++ the explicit keyword is what you use to disallow implicit
constructors. Seems like implicit has the right meaning here, no?

MOJO_ALLOW_IMPLICIT_TYPE_CONVERSION()

https://codereview.chromium.org/209453003/diff/120001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/type_converter.h (right):

https://codereview.chromium.org/209453003/diff/120001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/type_converter.h:99: #define
MOJO_ALLOW_DIRECT_TYPE_CONVERSION() \
On 2014/04/23 00:14:24, yzshen1 (OOO until Apr 21) wrote:
> I think it is more clear to put the macro definitions here, but please let me
> know if you think they should be in mojo/public/cpp/system/macros.h

Yes, this is the right place for this. It doesn't need to go into the general
macros.h since it is not a general purpose macro.

[yzshen1, 2014-04-23 18:20:57]

Thanks Darin!

https://codereview.chromium.org/209453003/diff/120001/mojo/common/common_type...
File mojo/common/common_type_converters.h (right):

https://codereview.chromium.org/209453003/diff/120001/mojo/common/common_type...
mojo/common/common_type_converters.h:22: MOJO_ALLOW_DIRECT_TYPE_CONVERSION();
On 2014/04/23 18:01:01, darin wrote:
> Why don't you like using the term "implicit" here? Isn't that the opposite of
> explicit, and in C++ the explicit keyword is what you use to disallow implicit
> constructors. Seems like implicit has the right meaning here, no?
> 
> MOJO_ALLOW_IMPLICIT_TYPE_CONVERSION()

Because it sounds like we allow "explicit conversion" without the macro. And
"explicit conversion" makes me think of things such as "A a =
static_cast<A>(b);", which we don't support without the macro.

(But I don't have a very strong preference. I am fine if you think that is
better.)

[darin (slow to review), 2014-04-23 18:25:49]

That's a good point. Hmm... I'd probably still go with "implicit" since I
think it makes it clear what it enables. I agree there could be confusion
about what "not implicit" means. Hmm...


On Wed, Apr 23, 2014 at 11:20 AM, <yzshen@chromium.org> wrote:

> Thanks Darin!
>
>
>
> https://codereview.chromium.org/209453003/diff/120001/
> mojo/common/common_type_converters.h
> File mojo/common/common_type_converters.h (right):
>
> https://codereview.chromium.org/209453003/diff/120001/
> mojo/common/common_type_converters.h#newcode22
> mojo/common/common_type_converters.h:22:
> MOJO_ALLOW_DIRECT_TYPE_CONVERSION();
> On 2014/04/23 18:01:01, darin wrote:
>
>> Why don't you like using the term "implicit" here? Isn't that the
>>
> opposite of
>
>> explicit, and in C++ the explicit keyword is what you use to disallow
>>
> implicit
>
>> constructors. Seems like implicit has the right meaning here, no?
>>
>
>  MOJO_ALLOW_IMPLICIT_TYPE_CONVERSION()
>>
>
> Because it sounds like we allow "explicit conversion" without the macro.
> And "explicit conversion" makes me think of things such as "A a =
> static_cast<A>(b);", which we don't support without the macro.
>
> (But I don't have a very strong preference. I am fine if you think that
> is better.)
>
> https://codereview.chromium.org/209453003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[yzshen1, 2014-04-23 18:41:09]

On 2014/04/23 18:25:49, darin wrote:
> That's a good point. Hmm... I'd probably still go with "implicit" since I
> think it makes it clear what it enables. I agree there could be confusion
> about what "not implicit" means. Hmm...

Done. I have made the change.

Thanks!

[yzshen1, 2014-04-28 23:16:47]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2014-04-28 23:17:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/209453003/140001

[commit-bot: I haz the power, 2014-04-28 23:17:17]

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

[commit-bot: I haz the power, 2014-04-28 23:17:18]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[yzshen1, 2014-04-28 23:25:06]

Friendly ping, Darin. :)

[darin (slow to review), 2014-04-28 23:57:33]

LGTM

[yzshen1, 2014-04-29 00:02:32]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2014-04-29 00:03:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/209453003/140001

[commit-bot: I haz the power, 2014-04-29 00:51:40]

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

[commit-bot: I haz the power, 2014-04-29 00:51:41]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[yzshen1, 2014-04-29 03:56:19]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2014-04-29 03:56:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/209453003/140001

[commit-bot: I haz the power, 2014-04-29 09:48:42]

Change committed as 266814