[Autofill] Switch on use_new_wrapper_types mode.

[Autofill] Switch on use_new_wrapper_types mode.

This CL also adds a custom typemap for base::string16:
"mojo.common.mojom.String16=base::string16"

BUG=624136
Committed: https://crrev.com/35ae41c15a2362884c4122204ed7caf871158b98
Cr-Commit-Position: refs/heads/master@{#408570}

[leonhsl(Using Gerrit), 2016-07-26 06:07:09]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-26 06:07:19]

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-07-26 06:32:27]

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

[commit-bot: I haz the power, 2016-07-26 06:32:28]

Dry run: Try jobs failed on following builders:
  mac_chromium_gyp_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gyp_...)

[leonhsl(Using Gerrit), 2016-07-26 06:50:21]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-26 06:50:38]

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-07-26 07:43:09]

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

[commit-bot: I haz the power, 2016-07-26 07:43:10]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-07-26 08:24:32]

leon.han@intel.com changed reviewers:
+ vabr@chromium.org, yzshen@chromium.org

[leonhsl(Using Gerrit), 2016-07-26 08:24:33]

Would you PTAL at this? Thanks.

Vaclav: for OWNER review
Yuzhu:  for review as Mojo expert

[vabr (Chromium), 2016-07-26 14:56:32]

On 2016/07/26 08:24:33, leonhsl wrote:
> Would you PTAL at this? Thanks.
> 
> Vaclav: for OWNER review
> Yuzhu:  for review as Mojo expert

Hello,

Thanks for the CL. I am sheriffing today and tomorrow, so it might happen that I
only will get to the review on Thursday. Please let me know if it is more
urgent.
Thanks!

Vaclav

[yzshen1, 2016-07-26 16:45:36]

https://codereview.chromium.org/2180093002/diff/20001/chrome/renderer/autofil...
File chrome/renderer/autofill/autofill_renderer_browsertest.cc (right):

https://codereview.chromium.org/2180093002/diff/20001/chrome/renderer/autofil...
chrome/renderer/autofill/autofill_renderer_browsertest.cc:74: forms_.reset(new
std::vector<FormData>(std::move(forms)));
nit: std::move is not necessary here because it has to make a copy anyway.

https://codereview.chromium.org/2180093002/diff/20001/components/autofill/con...
File components/autofill/content/browser/content_autofill_driver.cc (right):

https://codereview.chromium.org/2180093002/diff/20001/components/autofill/con...
components/autofill/content/browser/content_autofill_driver.cc:236:
std::vector<base::string16> values16;
nit: please reserve the same size as |values| to avoid re-allocation.


Btw, looking at all these changes, it makes me wonder whether we should define a
custom type map for base::string16, instead of using mojom built-in string type.
WDYT?

https://codereview.chromium.org/2180093002/diff/20001/components/autofill/con...
components/autofill/content/browser/content_autofill_driver.cc:239:
std::vector<base::string16> labels16;
ditto

https://codereview.chromium.org/2180093002/diff/20001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/2180093002/diff/20001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:737:
std::vector<std::string> values;
Please reserve space.

[leonhsl(Using Gerrit), 2016-07-27 02:50:26]

https://codereview.chromium.org/2180093002/diff/20001/chrome/renderer/autofil...
File chrome/renderer/autofill/autofill_renderer_browsertest.cc (right):

https://codereview.chromium.org/2180093002/diff/20001/chrome/renderer/autofil...
chrome/renderer/autofill/autofill_renderer_browsertest.cc:74: forms_.reset(new
std::vector<FormData>(std::move(forms)));
On 2016/07/26 16:45:36, yzshen1 wrote:
> nit: std::move is not necessary here because it has to make a copy anyway.

Acknowledged.

https://codereview.chromium.org/2180093002/diff/20001/components/autofill/con...
File components/autofill/content/browser/content_autofill_driver.cc (right):

https://codereview.chromium.org/2180093002/diff/20001/components/autofill/con...
components/autofill/content/browser/content_autofill_driver.cc:236:
std::vector<base::string16> values16;
On 2016/07/26 16:45:36, yzshen1 wrote:
> nit: please reserve the same size as |values| to avoid re-allocation.
> 
> 
> Btw, looking at all these changes, it makes me wonder whether we should define
a
> custom type map for base::string16, instead of using mojom built-in string
type.
> WDYT?

Yeah, I have the same feeling.. Seems lots of interface definitions need a
base::string16 as parameter. How about we create the typemap at
mojo/common/common_custom_types.typemap? If you agree, I'll implement within
this CL.

https://codereview.chromium.org/2180093002/diff/20001/components/autofill/con...
components/autofill/content/browser/content_autofill_driver.cc:239:
std::vector<base::string16> labels16;
On 2016/07/26 16:45:36, yzshen1 wrote:
> ditto

Acknowledged.

https://codereview.chromium.org/2180093002/diff/20001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/2180093002/diff/20001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:737:
std::vector<std::string> values;
On 2016/07/26 16:45:36, yzshen1 wrote:
> Please reserve space.

Acknowledged.

[yzshen1, 2016-07-27 06:21:20]

https://codereview.chromium.org/2180093002/diff/20001/components/autofill/con...
File components/autofill/content/browser/content_autofill_driver.cc (right):

https://codereview.chromium.org/2180093002/diff/20001/components/autofill/con...
components/autofill/content/browser/content_autofill_driver.cc:236:
std::vector<base::string16> values16;
On 2016/07/27 02:50:26, leonhsl wrote:
> On 2016/07/26 16:45:36, yzshen1 wrote:
> > nit: please reserve the same size as |values| to avoid re-allocation.
> > 
> > 
> > Btw, looking at all these changes, it makes me wonder whether we should
define
> a
> > custom type map for base::string16, instead of using mojom built-in string
> type.
> > WDYT?
> 
> Yeah, I have the same feeling.. Seems lots of interface definitions need a
> base::string16 as parameter. How about we create the typemap at
> mojo/common/common_custom_types.typemap? If you agree, I'll implement within
> this CL.

SGTM Thanks!

[leonhsl(Using Gerrit), 2016-07-27 08:21:52]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-27 08:22:12]

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

[leonhsl(Using Gerrit), 2016-07-27 09:19:12]

Description was changed from

==========
[Autofill] Switch on use_new_wrapper_types mode.

BUG=624136
==========

to

==========
[Autofill] Switch on use_new_wrapper_types mode.

This CL also adds a custom typemap for base::string16:
"mojo.common.mojom.String16=base::string16"

BUG=624136
==========

[leonhsl(Using Gerrit), 2016-07-27 09:25:32]

Uploaded ps#3 to address comments, PTAnL, Thanks.

https://codereview.chromium.org/2180093002/diff/20001/chrome/renderer/autofil...
File chrome/renderer/autofill/autofill_renderer_browsertest.cc (right):

https://codereview.chromium.org/2180093002/diff/20001/chrome/renderer/autofil...
chrome/renderer/autofill/autofill_renderer_browsertest.cc:74: forms_.reset(new
std::vector<FormData>(std::move(forms)));
On 2016/07/27 02:50:26, leonhsl wrote:
> On 2016/07/26 16:45:36, yzshen1 wrote:
> > nit: std::move is not necessary here because it has to make a copy anyway.
> 
> Acknowledged.

Done.

https://codereview.chromium.org/2180093002/diff/20001/components/autofill/con...
File components/autofill/content/browser/content_autofill_driver.cc (right):

https://codereview.chromium.org/2180093002/diff/20001/components/autofill/con...
components/autofill/content/browser/content_autofill_driver.cc:236:
std::vector<base::string16> values16;
On 2016/07/27 06:21:20, yzshen1 wrote:
> On 2016/07/27 02:50:26, leonhsl wrote:
> > On 2016/07/26 16:45:36, yzshen1 wrote:
> > > nit: please reserve the same size as |values| to avoid re-allocation.
> > > 
> > > 
> > > Btw, looking at all these changes, it makes me wonder whether we should
> define
> > a
> > > custom type map for base::string16, instead of using mojom built-in string
> > type.
> > > WDYT?
> > 
> > Yeah, I have the same feeling.. Seems lots of interface definitions need a
> > base::string16 as parameter. How about we create the typemap at
> > mojo/common/common_custom_types.typemap? If you agree, I'll implement within
> > this CL.
> 
> SGTM Thanks!

Done. Use base::string16 typemap now, removed these conversion codes.

https://codereview.chromium.org/2180093002/diff/20001/components/autofill/con...
components/autofill/content/browser/content_autofill_driver.cc:239:
std::vector<base::string16> labels16;
On 2016/07/27 02:50:26, leonhsl wrote:
> On 2016/07/26 16:45:36, yzshen1 wrote:
> > ditto
> 
> Acknowledged.

Done. Use base::string16 typemap now, removed these conversion codes.

https://codereview.chromium.org/2180093002/diff/20001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/2180093002/diff/20001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:737:
std::vector<std::string> values;
On 2016/07/27 02:50:26, leonhsl wrote:
> On 2016/07/26 16:45:36, yzshen1 wrote:
> > Please reserve space.
> 
> Acknowledged.

Done. Use base::string16 typemap now, removed these conversion codes.

[leonhsl(Using Gerrit), 2016-07-27 09:27:33]

leon.han@intel.com changed reviewers:
+ tsepez@chromium.org

[leonhsl(Using Gerrit), 2016-07-27 09:27:34]

+tsepez 
Would you PTAL for security review? Thanks.

[vabr (Chromium), 2016-07-27 09:30:24]

LGTM, so much cleaner!
Thanks!
Vaclav

[commit-bot: I haz the power, 2016-07-27 09:31:55]

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

[commit-bot: I haz the power, 2016-07-27 09:31:56]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2016-07-27 15:29:53]

LGTM with one nit

Thanks!

https://codereview.chromium.org/2180093002/diff/40001/components/autofill/con...
File components/autofill/content/browser/content_autofill_driver_unittest.cc
(right):

https://codereview.chromium.org/2180093002/diff/40001/components/autofill/con...
components/autofill/content/browser/content_autofill_driver_unittest.cc:165:
predictions_ = std::move(forms);
nit: no need to std::move.

[Tom Sepez, 2016-07-27 17:19:31]

Are you sure you want to pass string16's over the wire instead of converting to
the canonical mojo string representation, so that other language bindings could
theoretically interoperate without issue?  Especially given something like 

mojo/public/cpp/bindings/string_traits_string16.h

which looks to be the missing helper you'd required (if I understand this
correctly).

[leonhsl(Using Gerrit), 2016-07-28 02:59:08]

On 2016/07/27 17:19:31, Tom Sepez wrote:
> Are you sure you want to pass string16's over the wire instead of converting
to
> the canonical mojo string representation, so that other language bindings
could
> theoretically interoperate without issue?  Especially given something like 
> 
> mojo/public/cpp/bindings/string_traits_string16.h
> 
> which looks to be the missing helper you'd required (if I understand this
> correctly).

Yeah I think we're sure that autofill driver<--->agent code only depends on C++
mojom bindings, should have no chance to interoperate with other languages such
as Java, Javascript etc, currently and even in future? Vaclav could correct me
if it's my misunderstanding, Thanks~

About mojo/public/cpp/bindings/string_traits_string16.h, it is just one
specification of mojo::StringTraits template(there are also several other
specifications for base::StringPiece, std::string, WTF::String (in blink)),
should be the helper for serialization/deserialization of mojom string type, and
is used when we're typemapping a custom struct with a mojom struct containing a
mojom string type field. this means, it only provides the data format mapping
between base::string16 and mojom string type, but have no influence on mojom
generator, mojom string would always be generated as std::string(Now mojom C++
bindings use STL types by default).
But mojo.common.mojom.String16 we added here is a new mojom struct type, which
is typemapped with base::string16, this means mojo.common.mojom.String16 would
be generated as base::string16 directly by mojom generator, either as a struct
field or an interface function parameter. And this is what we expect in this CL.

[vabr (Chromium), 2016-07-28 07:19:37]

On 2016/07/28 02:59:08, leonhsl wrote:
> On 2016/07/27 17:19:31, Tom Sepez wrote:
> > Are you sure you want to pass string16's over the wire instead of converting
> to
> > the canonical mojo string representation, so that other language bindings
> could
> > theoretically interoperate without issue?  Especially given something like 
> > 
> > mojo/public/cpp/bindings/string_traits_string16.h
> > 
> > which looks to be the missing helper you'd required (if I understand this
> > correctly).
> 
> Yeah I think we're sure that autofill driver<--->agent code only depends on
C++
> mojom bindings, should have no chance to interoperate with other languages
such
> as Java, Javascript etc, currently and even in future? Vaclav could correct me
> if it's my misunderstanding, Thanks~

In the case of desktop and Android, this is likely to always be just C++. If we
decide to reuse the Mojo interfaces for iOS later, there the renderer code is
partly implemented in JavaScript. I am not sure yet how Mojo is best used there,
but so far the iOS equivalent of *AutofillAgent is in C++, inject JavaScript
into the page, as opposed to being completely in JavaScript. So string16 sounds
fine.

[Tom Sepez, 2016-07-28 17:15:00]

Ok, LGTM.

[leonhsl(Using Gerrit), 2016-07-29 02:22:32]

Thanks all a lot! Uploaded ps#4 to address nit and will land now.

https://codereview.chromium.org/2180093002/diff/40001/components/autofill/con...
File components/autofill/content/browser/content_autofill_driver_unittest.cc
(right):

https://codereview.chromium.org/2180093002/diff/40001/components/autofill/con...
components/autofill/content/browser/content_autofill_driver_unittest.cc:165:
predictions_ = std::move(forms);
On 2016/07/27 15:29:53, yzshen1 wrote:
> nit: no need to std::move.

Done.

[leonhsl(Using Gerrit), 2016-07-29 02:22:43]

The CQ bit was checked by leon.han@intel.com

[leonhsl(Using Gerrit), 2016-07-29 02:22:44]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
tsepez@chromium.org, yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/2180093002/#ps60001
(title: "Address nit")

[commit-bot: I haz the power, 2016-07-29 02:23:02]

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-07-29 03:32:34]

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

[commit-bot: I haz the power, 2016-07-29 03:32:36]

Failed to apply the patch.

[commit-bot: I haz the power, 2016-07-29 03:34:12]

Description was changed from

==========
[Autofill] Switch on use_new_wrapper_types mode.

This CL also adds a custom typemap for base::string16:
"mojo.common.mojom.String16=base::string16"

BUG=624136
==========

to

==========
[Autofill] Switch on use_new_wrapper_types mode.

This CL also adds a custom typemap for base::string16:
"mojo.common.mojom.String16=base::string16"

BUG=624136

Committed: https://crrev.com/35ae41c15a2362884c4122204ed7caf871158b98
Cr-Commit-Position: refs/heads/master@{#408570}
==========

[commit-bot: I haz the power, 2016-07-29 03:34:13]

Patchset 4 (id:??) landed as
https://crrev.com/35ae41c15a2362884c4122204ed7caf871158b98
Cr-Commit-Position: refs/heads/master@{#408570}