[password autofill] Add serialization for FormData and FormFieldData

[password autofill] Add serialization for FormData and FormFieldData

These objects will soon be added to PasswordForm, which needs to be persisted
by the PasswordStore.

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

[Evan Stade, 2013-08-26 23:20:43]

https://codereview.chromium.org/23033010/diff/7001/components/autofill/core/c...
File components/autofill/core/common/form_data.h (right):

https://codereview.chromium.org/23033010/diff/7001/components/autofill/core/c...
components/autofill/core/common/form_data.h:42: void SerializeFormData(const
FormData& form_data, Pickle* pickle);
any reason not to make this return a scoped_ptr<Pickle>?

https://codereview.chromium.org/23033010/diff/7001/components/autofill/core/c...
components/autofill/core/common/form_data.h:43: bool
DeserializeFormData(PickleIterator* iter, FormData* form_data);
Why does this take a PickleIterator* instead of const Pickle&?

[Garrett Casto, 2013-08-27 17:21:04]

https://codereview.chromium.org/23033010/diff/7001/components/autofill/core/c...
File components/autofill/core/common/form_data.h (right):

https://codereview.chromium.org/23033010/diff/7001/components/autofill/core/c...
components/autofill/core/common/form_data.h:42: void SerializeFormData(const
FormData& form_data, Pickle* pickle);
On 2013/08/26 23:20:44, Evan Stade wrote:
> any reason not to make this return a scoped_ptr<Pickle>?

One of the PasswordStore implementations (NativeBackendKWallet) Pickles the
entire PasswordForm, so we want to be able to append to that Pickle.

https://codereview.chromium.org/23033010/diff/7001/components/autofill/core/c...
components/autofill/core/common/form_data.h:43: bool
DeserializeFormData(PickleIterator* iter, FormData* form_data);
On 2013/08/26 23:20:44, Evan Stade wrote:
> Why does this take a PickleIterator* instead of const Pickle&?

Same reason, we want to deserialize part of a Pickle as a FormData, not
necessarily the whole thing.

[Evan Stade, 2013-08-28 22:42:11]

lgtm

https://codereview.chromium.org/23033010/diff/7001/components/autofill/core/c...
File components/autofill/core/common/form_data.h (right):

https://codereview.chromium.org/23033010/diff/7001/components/autofill/core/c...
components/autofill/core/common/form_data.h:42: void SerializeFormData(const
FormData& form_data, Pickle* pickle);
On 2013/08/27 17:21:05, Garrett Casto wrote:
> On 2013/08/26 23:20:44, Evan Stade wrote:
> > any reason not to make this return a scoped_ptr<Pickle>?
> 
> One of the PasswordStore implementations (NativeBackendKWallet) Pickles the
> entire PasswordForm, so we want to be able to append to that Pickle.

can you improve the comment then? Something along the lines of:

Serialized data will be appended to |pickle|.

https://codereview.chromium.org/23033010/diff/7001/components/autofill/core/c...
components/autofill/core/common/form_data.h:43: bool
DeserializeFormData(PickleIterator* iter, FormData* form_data);
On 2013/08/27 17:21:05, Garrett Casto wrote:
> On 2013/08/26 23:20:44, Evan Stade wrote:
> > Why does this take a PickleIterator* instead of const Pickle&?
> 
> Same reason, we want to deserialize part of a Pickle as a FormData, not
> necessarily the whole thing.

also needs better comments.

https://codereview.chromium.org/23033010/diff/7001/components/autofill/core/c...
File components/autofill/core/common/form_field_data.cc (right):

https://codereview.chromium.org/23033010/diff/7001/components/autofill/core/c...
components/autofill/core/common/form_field_data.cc:23: bool
ReadVector(PickleIterator* iter, std::vector<base::string16>* strings) {
suggested fn name: ReadStringVector or ReadStrings

https://codereview.chromium.org/23033010/diff/7001/components/autofill/core/c...
components/autofill/core/common/form_field_data.cc:28: string16 temp;
nit: temp is not a super useful var name, how about pickle_data?

https://codereview.chromium.org/23033010/diff/7001/components/autofill/core/c...
components/autofill/core/common/form_field_data.cc:40: int temp;
ditto

[Garrett Casto, 2013-08-29 06:48:34]

https://codereview.chromium.org/23033010/diff/7001/components/autofill/core/c...
File components/autofill/core/common/form_data.h (right):

https://codereview.chromium.org/23033010/diff/7001/components/autofill/core/c...
components/autofill/core/common/form_data.h:42: void SerializeFormData(const
FormData& form_data, Pickle* pickle);
On 2013/08/28 22:42:11, Evan Stade wrote:
> On 2013/08/27 17:21:05, Garrett Casto wrote:
> > On 2013/08/26 23:20:44, Evan Stade wrote:
> > > any reason not to make this return a scoped_ptr<Pickle>?
> > 
> > One of the PasswordStore implementations (NativeBackendKWallet) Pickles the
> > entire PasswordForm, so we want to be able to append to that Pickle.
> 
> can you improve the comment then? Something along the lines of:
> 
> Serialized data will be appended to |pickle|.

Done.

https://codereview.chromium.org/23033010/diff/7001/components/autofill/core/c...
components/autofill/core/common/form_data.h:43: bool
DeserializeFormData(PickleIterator* iter, FormData* form_data);
On 2013/08/28 22:42:11, Evan Stade wrote:
> On 2013/08/27 17:21:05, Garrett Casto wrote:
> > On 2013/08/26 23:20:44, Evan Stade wrote:
> > > Why does this take a PickleIterator* instead of const Pickle&?
> > 
> > Same reason, we want to deserialize part of a Pickle as a FormData, not
> > necessarily the whole thing.
> 
> also needs better comments.

Done.

https://codereview.chromium.org/23033010/diff/7001/components/autofill/core/c...
File components/autofill/core/common/form_field_data.cc (right):

https://codereview.chromium.org/23033010/diff/7001/components/autofill/core/c...
components/autofill/core/common/form_field_data.cc:23: bool
ReadVector(PickleIterator* iter, std::vector<base::string16>* strings) {
On 2013/08/28 22:42:11, Evan Stade wrote:
> suggested fn name: ReadStringVector or ReadStrings

Done.

https://codereview.chromium.org/23033010/diff/7001/components/autofill/core/c...
components/autofill/core/common/form_field_data.cc:28: string16 temp;
On 2013/08/28 22:42:11, Evan Stade wrote:
> nit: temp is not a super useful var name, how about pickle_data?

Done.

https://codereview.chromium.org/23033010/diff/7001/components/autofill/core/c...
components/autofill/core/common/form_field_data.cc:40: int temp;
On 2013/08/28 22:42:11, Evan Stade wrote:
> ditto

Done.

[Garrett Casto, 2013-08-29 06:50:07]

Joi: Can LGTM the changes to components/components_tests.gypi

[Jói, 2013-08-29 09:43:01]

//components/components_tests.gypi LGTM

[commit-bot: I haz the power, 2013-08-29 17:23:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gcasto@chromium.org/23033010/18001

[commit-bot: I haz the power, 2013-08-29 17:48:09]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-08-29 19:28:54]

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

[commit-bot: I haz the power, 2013-08-29 20:54:43]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-08-29 21:06:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gcasto@chromium.org/23033010/58001

[commit-bot: I haz the power, 2013-08-29 23:48:37]

Change committed as 220437