Change TypeConverter<X,Y>::ConvertFrom and ConvertTo into a single symmetric

Change TypeConverter<X,Y>::ConvertFrom and ConvertTo into a single symmetric
Convert method.

Also added a ConvertTo<X> helper function.

Also changed it from class to struct, since every method was public.

BUG=406492
Committed: https://crrev.com/278d42a659161b96461b27e166d44224da697eaa
Cr-Commit-Position: refs/heads/master@{#293027}

[Matt Perry, 2014-08-27 00:24:01]

mpcomplete@chromium.org changed reviewers:
+ ben@chromium.org

[darin (slow to review), 2014-08-27 03:30:37]

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

[darin (slow to review), 2014-08-27 03:30:37]

https://codereview.chromium.org/507173003/diff/20001/extensions/browser/api/s...
File extensions/browser/api/serial/serial_api.cc (right):

https://codereview.chromium.org/507173003/diff/20001/extensions/browser/api/s...
extensions/browser/api/serial/serial_api.cc:439: ::Convert(const
device::serial::DeviceInfoPtr& device) {
It seems like the second template argument might not be necessary. TypeConverter
could be defined like so:

  template <typename T>
  struct TypeConverter {
    static T Convert(const A& a);
    static T Convert(const B& b);
    static T Convert(const C& c);
  };

Another approach might be to rely on argument-dependent lookup so that these
converters wouldn't need to be in the mojo:: namespace. This would require the
return value to be an out param, like so:

  void Convert(const A& a, T* obj);
  void Convert(const B& b, T* obj);
  void Convert(const C& c, T* obj);

WDYT?

[Matt Perry, 2014-08-27 18:51:31]

https://codereview.chromium.org/507173003/diff/20001/extensions/browser/api/s...
File extensions/browser/api/serial/serial_api.cc (right):

https://codereview.chromium.org/507173003/diff/20001/extensions/browser/api/s...
extensions/browser/api/serial/serial_api.cc:439: ::Convert(const
device::serial::DeviceInfoPtr& device) {
On 2014/08/27 03:30:37, darin wrote:
> It seems like the second template argument might not be necessary.
TypeConverter
> could be defined like so:
> 
>   template <typename T>
>   struct TypeConverter {
>     static T Convert(const A& a);
>     static T Convert(const B& b);
>     static T Convert(const C& c);
>   };
> 
> Another approach might be to rely on argument-dependent lookup so that these
> converters wouldn't need to be in the mojo:: namespace. This would require the
> return value to be an out param, like so:
> 
>   void Convert(const A& a, T* obj);
>   void Convert(const B& b, T* obj);
>   void Convert(const C& c, T* obj);
> 
> WDYT?

I think the converters would still need to be in the mojo namespace so that
things like Array::From() still work.

We could potentially still have mojo::Convert(const A& a, T* obj) - e.g.
function overloading rather than template specializations. I think the drawback
there is that it's easier for the compiler to mess up which conversion function
is used (which would only really happen if there are two similar types). This
might happen if there are 2 similar Convert functions defined in different
translation units, and you include the wrong one.

Based on my reading of this article [1], it's safer to have template
specializations in a struct rather than a global function. I believe the same
reasoning would apply to your first example. (OTOH, I can't think of a concrete
example where this would matter, so maybe it's overly paranoid.)

[1]: http://www.gotw.ca/publications/mill17.htm

[Matt Perry, 2014-08-27 19:30:24]

On 2014/08/27 18:51:31, Matt Perry wrote:
>
https://codereview.chromium.org/507173003/diff/20001/extensions/browser/api/s...
> File extensions/browser/api/serial/serial_api.cc (right):
> 
>
https://codereview.chromium.org/507173003/diff/20001/extensions/browser/api/s...
> extensions/browser/api/serial/serial_api.cc:439: ::Convert(const
> device::serial::DeviceInfoPtr& device) {
> On 2014/08/27 03:30:37, darin wrote:
> > It seems like the second template argument might not be necessary.
> TypeConverter
> > could be defined like so:
> > 
> >   template <typename T>
> >   struct TypeConverter {
> >     static T Convert(const A& a);
> >     static T Convert(const B& b);
> >     static T Convert(const C& c);
> >   };

Another disadvantage of this approach is that you can't have multiple converters
to the same type defined in multiple places. E.g. say you want mojo::String to
be convertible from either a std::string and blink::WebString. You'd have to
define both converters in the same place.

[darin (slow to review), 2014-08-27 19:54:43]

On 2014/08/27 19:30:24, Matt Perry wrote:
> On 2014/08/27 18:51:31, Matt Perry wrote:
> >
>
https://codereview.chromium.org/507173003/diff/20001/extensions/browser/api/s...
> > File extensions/browser/api/serial/serial_api.cc (right):
> > 
> >
>
https://codereview.chromium.org/507173003/diff/20001/extensions/browser/api/s...
> > extensions/browser/api/serial/serial_api.cc:439: ::Convert(const
> > device::serial::DeviceInfoPtr& device) {
> > On 2014/08/27 03:30:37, darin wrote:
> > > It seems like the second template argument might not be necessary.
> > TypeConverter
> > > could be defined like so:
> > > 
> > >   template <typename T>
> > >   struct TypeConverter {
> > >     static T Convert(const A& a);
> > >     static T Convert(const B& b);
> > >     static T Convert(const C& c);
> > >   };
> 
> Another disadvantage of this approach is that you can't have multiple
converters
> to the same type defined in multiple places. E.g. say you want mojo::String to
> be convertible from either a std::string and blink::WebString. You'd have to
> define both converters in the same place.

Good point. I thought about that, but I wasn't sure how much to worry about it.
The string example is excellent though. I agree we wouldn't want all such
converters in one place.

[darin (slow to review), 2014-08-27 19:56:07]

On 2014/08/27 18:51:31, Matt Perry wrote:
>
https://codereview.chromium.org/507173003/diff/20001/extensions/browser/api/s...
> File extensions/browser/api/serial/serial_api.cc (right):
> 
>
https://codereview.chromium.org/507173003/diff/20001/extensions/browser/api/s...
> extensions/browser/api/serial/serial_api.cc:439: ::Convert(const
> device::serial::DeviceInfoPtr& device) {
> On 2014/08/27 03:30:37, darin wrote:
> > It seems like the second template argument might not be necessary.
> TypeConverter
> > could be defined like so:
> > 
> >   template <typename T>
> >   struct TypeConverter {
> >     static T Convert(const A& a);
> >     static T Convert(const B& b);
> >     static T Convert(const C& c);
> >   };
> > 
> > Another approach might be to rely on argument-dependent lookup so that these
> > converters wouldn't need to be in the mojo:: namespace. This would require
the
> > return value to be an out param, like so:
> > 
> >   void Convert(const A& a, T* obj);
> >   void Convert(const B& b, T* obj);
> >   void Convert(const C& c, T* obj);
> > 
> > WDYT?
> 
> I think the converters would still need to be in the mojo namespace so that
> things like Array::From() still work.

Hmm... I don't follow.


> We could potentially still have mojo::Convert(const A& a, T* obj) - e.g.
> function overloading rather than template specializations. I think the
drawback
> there is that it's easier for the compiler to mess up which conversion
function
> is used (which would only really happen if there are two similar types). This
> might happen if there are 2 similar Convert functions defined in different
> translation units, and you include the wrong one.
> 
> Based on my reading of this article [1], it's safer to have template
> specializations in a struct rather than a global function. I believe the same
> reasoning would apply to your first example. (OTOH, I can't think of a
concrete
> example where this would matter, so maybe it's overly paranoid.)
> 
> [1]: http://www.gotw.ca/publications/mill17.htm

[Matt Perry, 2014-08-27 20:03:22]

On 2014/08/27 19:56:07, darin wrote:
> On 2014/08/27 18:51:31, Matt Perry wrote:
> >
>
https://codereview.chromium.org/507173003/diff/20001/extensions/browser/api/s...
> > File extensions/browser/api/serial/serial_api.cc (right):
> > 
> >
>
https://codereview.chromium.org/507173003/diff/20001/extensions/browser/api/s...
> > extensions/browser/api/serial/serial_api.cc:439: ::Convert(const
> > device::serial::DeviceInfoPtr& device) {
> > On 2014/08/27 03:30:37, darin wrote:
> > > It seems like the second template argument might not be necessary.
> > TypeConverter
> > > could be defined like so:
> > > 
> > >   template <typename T>
> > >   struct TypeConverter {
> > >     static T Convert(const A& a);
> > >     static T Convert(const B& b);
> > >     static T Convert(const C& c);
> > >   };
> > > 
> > > Another approach might be to rely on argument-dependent lookup so that
these
> > > converters wouldn't need to be in the mojo:: namespace. This would require
> the
> > > return value to be an out param, like so:
> > > 
> > >   void Convert(const A& a, T* obj);
> > >   void Convert(const B& b, T* obj);
> > >   void Convert(const C& c, T* obj);
> > > 
> > > WDYT?
> > 
> > I think the converters would still need to be in the mojo namespace so that
> > things like Array::From() still work.
> 
> Hmm... I don't follow.

Nevermind, they could be in the global namespace as well.

[darin (slow to review), 2014-08-27 21:25:59]

On 2014/08/27 20:03:22, Matt Perry wrote:
> On 2014/08/27 19:56:07, darin wrote:
> > On 2014/08/27 18:51:31, Matt Perry wrote:
> > >
> >
>
https://codereview.chromium.org/507173003/diff/20001/extensions/browser/api/s...
> > > File extensions/browser/api/serial/serial_api.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/507173003/diff/20001/extensions/browser/api/s...
> > > extensions/browser/api/serial/serial_api.cc:439: ::Convert(const
> > > device::serial::DeviceInfoPtr& device) {
> > > On 2014/08/27 03:30:37, darin wrote:
> > > > It seems like the second template argument might not be necessary.
> > > TypeConverter
> > > > could be defined like so:
> > > > 
> > > >   template <typename T>
> > > >   struct TypeConverter {
> > > >     static T Convert(const A& a);
> > > >     static T Convert(const B& b);
> > > >     static T Convert(const C& c);
> > > >   };
> > > > 
> > > > Another approach might be to rely on argument-dependent lookup so that
> these
> > > > converters wouldn't need to be in the mojo:: namespace. This would
require
> > the
> > > > return value to be an out param, like so:
> > > > 
> > > >   void Convert(const A& a, T* obj);
> > > >   void Convert(const B& b, T* obj);
> > > >   void Convert(const C& c, T* obj);
> > > > 
> > > > WDYT?
> > > 
> > > I think the converters would still need to be in the mojo namespace so
that
> > > things like Array::From() still work.
> > 
> > Hmm... I don't follow.
> 
> Nevermind, they could be in the global namespace as well.

OK... anyways, I would only bother with this change in structure if it allowed
for the conversion functions to live in the namespace of the user, rather than
in the mojo namespace. It would be nifty if users didn't have to add code to
the mojo namespace :)

[Matt Perry, 2014-08-27 21:31:43]

On 2014/08/27 21:25:59, darin wrote:
> On 2014/08/27 20:03:22, Matt Perry wrote:
> > On 2014/08/27 19:56:07, darin wrote:
> > > On 2014/08/27 18:51:31, Matt Perry wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/507173003/diff/20001/extensions/browser/api/s...
> > > > File extensions/browser/api/serial/serial_api.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/507173003/diff/20001/extensions/browser/api/s...
> > > > extensions/browser/api/serial/serial_api.cc:439: ::Convert(const
> > > > device::serial::DeviceInfoPtr& device) {
> > > > On 2014/08/27 03:30:37, darin wrote:
> > > > > It seems like the second template argument might not be necessary.
> > > > TypeConverter
> > > > > could be defined like so:
> > > > > 
> > > > >   template <typename T>
> > > > >   struct TypeConverter {
> > > > >     static T Convert(const A& a);
> > > > >     static T Convert(const B& b);
> > > > >     static T Convert(const C& c);
> > > > >   };
> > > > > 
> > > > > Another approach might be to rely on argument-dependent lookup so that
> > these
> > > > > converters wouldn't need to be in the mojo:: namespace. This would
> require
> > > the
> > > > > return value to be an out param, like so:
> > > > > 
> > > > >   void Convert(const A& a, T* obj);
> > > > >   void Convert(const B& b, T* obj);
> > > > >   void Convert(const C& c, T* obj);
> > > > > 
> > > > > WDYT?
> > > > 
> > > > I think the converters would still need to be in the mojo namespace so
> that
> > > > things like Array::From() still work.
> > > 
> > > Hmm... I don't follow.
> > 
> > Nevermind, they could be in the global namespace as well.
> 
> OK... anyways, I would only bother with this change in structure if it allowed
> for the conversion functions to live in the namespace of the user, rather than
> in the mojo namespace. It would be nifty if users didn't have to add code to
> the mojo namespace :)

Yeah, it has to be in a known namespace for Array::from (or InterfacePtr::From,
etc) to work. See
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi...

[darin (slow to review), 2014-08-27 21:33:03]

OK


On Wed, Aug 27, 2014 at 2:31 PM, <mpcomplete@chromium.org> wrote:

> On 2014/08/27 21:25:59, darin wrote:
>
>> On 2014/08/27 20:03:22, Matt Perry wrote:
>> > On 2014/08/27 19:56:07, darin wrote:
>> > > On 2014/08/27 18:51:31, Matt Perry wrote:
>> > > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/507173003/diff/20001/
> extensions/browser/api/serial/serial_api.cc
>
>> > > > File extensions/browser/api/serial/serial_api.cc (right):
>> > > >
>> > > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/507173003/diff/20001/
> extensions/browser/api/serial/serial_api.cc#newcode439
>
>> > > > extensions/browser/api/serial/serial_api.cc:439: ::Convert(const
>> > > > device::serial::DeviceInfoPtr& device) {
>> > > > On 2014/08/27 03:30:37, darin wrote:
>> > > > > It seems like the second template argument might not be necessary.
>> > > > TypeConverter
>> > > > > could be defined like so:
>> > > > >
>> > > > >   template <typename T>
>> > > > >   struct TypeConverter {
>> > > > >     static T Convert(const A& a);
>> > > > >     static T Convert(const B& b);
>> > > > >     static T Convert(const C& c);
>> > > > >   };
>> > > > >
>> > > > > Another approach might be to rely on argument-dependent lookup so
>> that
>> > these
>> > > > > converters wouldn't need to be in the mojo:: namespace. This would
>> require
>> > > the
>> > > > > return value to be an out param, like so:
>> > > > >
>> > > > >   void Convert(const A& a, T* obj);
>> > > > >   void Convert(const B& b, T* obj);
>> > > > >   void Convert(const C& c, T* obj);
>> > > > >
>> > > > > WDYT?
>> > > >
>> > > > I think the converters would still need to be in the mojo namespace
>> so
>> that
>> > > > things like Array::From() still work.
>> > >
>> > > Hmm... I don't follow.
>> >
>> > Nevermind, they could be in the global namespace as well.
>>
>
>  OK... anyways, I would only bother with this change in structure if it
>> allowed
>> for the conversion functions to live in the namespace of the user, rather
>> than
>> in the mojo namespace. It would be nifty if users didn't have to add code
>> to
>> the mojo namespace :)
>>
>
> Yeah, it has to be in a known namespace for Array::from (or
> InterfacePtr::From,
> etc) to work. See
> https://code.google.com/p/chromium/codesearch#chromium/
> src/mojo/public/cpp/bindings/array.h&q=Array&sq=package:
> chromium&type=cs&l=52
>
> https://codereview.chromium.org/507173003/
>

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

[Matt Perry, 2014-08-28 23:43:16]

Any other thoughts about this CL?

[darin (slow to review), 2014-08-29 06:22:13]

OK, LGTM

[Matt Perry, 2014-08-29 19:43:40]

The CQ bit was checked by mpcomplete@chromium.org

[Matt Perry, 2014-08-29 23:14:57]

The CQ bit was checked by mpcomplete@chromium.org

[commit-bot: I haz the power, 2014-08-29 23:15:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mpcomplete@chromium.org/507173003/40001

[commit-bot: I haz the power, 2014-08-30 00:15:39]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/48382)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/53712)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-30 00:22:13]

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

[commit-bot: I haz the power, 2014-08-30 00:22:14]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/48412)

[Matt Perry, 2014-09-02 20:30:18]

The CQ bit was checked by mpcomplete@chromium.org

[commit-bot: I haz the power, 2014-09-02 20:32:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mpcomplete@chromium.org/507173003/80001

[Matt Perry, 2014-09-02 21:50:02]

The CQ bit was checked by mpcomplete@chromium.org

[commit-bot: I haz the power, 2014-09-02 21:50:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mpcomplete@chromium.org/507173003/100001

[commit-bot: I haz the power, 2014-09-02 23:23:29]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)

[commit-bot: I haz the power, 2014-09-03 00:06:12]

Committed patchset #6 (id:100001) as 7898a495a1e735991fe94ff57b81444384cd490f

[commit-bot: I haz the power, 2014-09-10 03:22:01]

Patchset 6 (id:??) landed as
https://crrev.com/278d42a659161b96461b27e166d44224da697eaa
Cr-Commit-Position: refs/heads/master@{#293027}