Serialize form_data in Gnome keyring password store service.

Serialize form_data in Gnome keyring password store service.

Serializing form_data is required for server-side password generation heuristics. It was implemented on all others platforms before. Here it's implemented in gnome-keyring and libsecret backends (both of them works with Gnome keyring service).

BUG=463126
Committed: https://crrev.com/0985fac147f8f3408638f5a4843031e26af9c412
Cr-Commit-Position: refs/heads/master@{#319628}

[dvadym, 2015-03-04 12:40:17]

dvadym@chromium.org changed reviewers:
+ gcasto@chromium.org

[dvadym, 2015-03-04 12:40:17]

Hi Garrett,

 Could you please review these changes?

Best regards,
Vadym

[Garrett Casto, 2015-03-04 18:05:31]

https://codereview.chromium.org/980583002/diff/1/components/autofill/core/com...
File components/autofill/core/common/form_data.cc (right):

https://codereview.chromium.org/980583002/diff/1/components/autofill/core/com...
components/autofill/core/common/form_data.cc:181: bool DeserializeFormData(const
char* input, FormData* form_data) {
I would just take a StringPiece in the function instead of a const char. It's
more flexible and you are converting to a StringPiece anyway.

https://codereview.chromium.org/980583002/diff/1/components/autofill/core/com...
File components/autofill/core/common/form_data.h (right):

https://codereview.chromium.org/980583002/diff/1/components/autofill/core/com...
components/autofill/core/common/form_data.h:51: void SerializeFormData(const
FormData& form_data, std::string* output);
Function overloading in this way is generally discouraged
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Over...).
I would change the name to something like SerializeFormDataToString() and
DeserializeFormDataFromString().

I would also mention in the comments why this function is useful, in particular
that it's base64 encoded to remove embedded NULLs. Without any context I would
just assume that this coerces the Pickle into a string.

Nit: I would also add some whitespace in between these functions. Feels a little
cramped now.

[dvadym, 2015-03-05 10:38:19]

Thanks Garrett, PTAL

https://codereview.chromium.org/980583002/diff/1/components/autofill/core/com...
File components/autofill/core/common/form_data.cc (right):

https://codereview.chromium.org/980583002/diff/1/components/autofill/core/com...
components/autofill/core/common/form_data.cc:181: bool DeserializeFormData(const
char* input, FormData* form_data) {
On 2015/03/04 18:05:31, Garrett Casto wrote:
> I would just take a StringPiece in the function instead of a const char. It's
> more flexible and you are converting to a StringPiece anyway.

Done.

https://codereview.chromium.org/980583002/diff/1/components/autofill/core/com...
File components/autofill/core/common/form_data.h (right):

https://codereview.chromium.org/980583002/diff/1/components/autofill/core/com...
components/autofill/core/common/form_data.h:51: void SerializeFormData(const
FormData& form_data, std::string* output);
On 2015/03/04 18:05:31, Garrett Casto wrote:
> Function overloading in this way is generally discouraged
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Over...).
> I would change the name to something like SerializeFormDataToString() and
> DeserializeFormDataFromString().
> 
> I would also mention in the comments why this function is useful, in
particular
> that it's base64 encoded to remove embedded NULLs. Without any context I would
> just assume that this coerces the Pickle into a string.
> 
> Nit: I would also add some whitespace in between these functions. Feels a
little
> cramped now.

Done, I've even added mention about base64 in functions names, this seems to be
more informative.

[Garrett Casto, 2015-03-06 18:48:14]

lgtm

[dvadym, 2015-03-09 09:40:37]

The CQ bit was checked by dvadym@chromium.org

[commit-bot: I haz the power, 2015-03-09 09:40:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/980583002/20001

[dvadym, 2015-03-09 11:58:01]

The CQ bit was checked by dvadym@chromium.org

[dvadym, 2015-03-09 11:58:01]

The patchset sent to the CQ was uploaded after l-g-t-m from gcasto@chromium.org
Link to the patchset: https://codereview.chromium.org/980583002/#ps40001 (title:
"Fixed compiler warning.")

[dvadym, 2015-03-09 11:58:14]

The CQ bit was unchecked by dvadym@chromium.org

[dvadym, 2015-03-09 13:45:45]

The CQ bit was checked by dvadym@chromium.org

[dvadym, 2015-03-09 13:45:46]

The patchset sent to the CQ was uploaded after l-g-t-m from gcasto@chromium.org
Link to the patchset: https://codereview.chromium.org/980583002/#ps60001 (title:
"Style fix")

[commit-bot: I haz the power, 2015-03-09 13:45:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/980583002/60001

[commit-bot: I haz the power, 2015-03-09 14:39:52]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-09 14:40:42]

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