Implement FormStructure and an initial method, EncodeUploadRequest. This als...

Implement FormStructure and an initial method, EncodeUploadRequest.  This also adds SHA1HashString, a utility method to get the SHA-1 hash of an input string, with appropriate unit tests.

BUG=18201
TEST=none

[Ben Goodger (Google), 2009-11-02 23:42:03]

Someone more familiar with base should review the hashing stuff. The rest LGTM.

http://codereview.chromium.org/355003/diff/1/13
File chrome/browser/autofill/field_types.h (right):

http://codereview.chromium.org/355003/diff/1/13#newcode76
Line 76: // No new types can be added.
Would be nice to say why - a limitation of the protocol, right?

http://codereview.chromium.org/355003/diff/1/5
File webkit/glue/form_field.h (right):

http://codereview.chromium.org/355003/diff/1/5#newcode29
Line 29: std::string FieldSignature() const;
You might note what this means, or refer to the documentation in the other
header.

[James Hawkins, 2009-11-02 23:50:02]

Thanks for the quick review!  I'll add someone as a reviewer for the base
addition.

http://codereview.chromium.org/355003/diff/1/13
File chrome/browser/autofill/field_types.h (right):

http://codereview.chromium.org/355003/diff/1/13#newcode76
Line 76: // No new types can be added.
On 2009/11/02 23:42:03, Ben Goodger wrote:
> Would be nice to say why - a limitation of the protocol, right?

Done.

http://codereview.chromium.org/355003/diff/1/5
File webkit/glue/form_field.h (right):

http://codereview.chromium.org/355003/diff/1/5#newcode29
Line 29: std::string FieldSignature() const;
On 2009/11/02 23:42:03, Ben Goodger wrote:
> You might note what this means, or refer to the documentation in the other
> header.

Ah yes, forgot to document this one.

[James Hawkins, 2009-11-03 00:00:51]

+wtc

[wtc, 2009-11-03 00:26:17]

If this feature is urgent, you can check in sha1.cc now
and replace it with sha1_{mac,nss,win}.cc later.  Please
let me know if you need to do that and I'll review sha1.cc.

http://codereview.chromium.org/355003/diff/2002/2009
File base/sha1.cc (right):

http://codereview.chromium.org/355003/diff/2002/2009#newcode11
Line 11: // Implementation of SHA-1. Only handles data in byte-sized blocks,
We should use the existing SHA-1 implementation in the OS.
Please use src/base/hmac* as an example.  The code for doing
SHA-1 should be similar to the code for doing HMAC because
HMAC can be considered a "keyed" hash.

The base/md5* code was added before we had the policy of
using the system crypto libraries.

The base/sha2* code is an exception because Windows XP
doesn't support SHA-2.

http://codereview.chromium.org/355003/diff/2002/2009#newcode16
Line 16: // Identifier names follow notation in FIPS PUB 180-1, where you'll
The current revision of this standard is FIPS PUB 180-3.

http://codereview.chromium.org/355003/diff/2002/2008
File base/sha1.h (right):

http://codereview.chromium.org/355003/diff/2002/2008#newcode12
Line 12: // This function performs SHA-160 operations.
"SHA-160" is a nonstandard term.  Please use "SHA-1" in
comments and "SHA1" in code.

http://codereview.chromium.org/355003/diff/2002/2008#newcode22
Line 22: std::string SHA160HashString(const std::string& str);
We should change SHA256HashString to have the same prototype.
Would you mind doing that in this CL or in a separate CL?

[James Hawkins, 2009-11-03 00:43:48]

http://codereview.chromium.org/355003/diff/2002/2009
File base/sha1.cc (right):

http://codereview.chromium.org/355003/diff/2002/2009#newcode16
Line 16: // Identifier names follow notation in FIPS PUB 180-1, where you'll
On 2009/11/03 00:26:17, wtc wrote:
> The current revision of this standard is FIPS PUB 180-3.

Done.

http://codereview.chromium.org/355003/diff/2002/2008
File base/sha1.h (right):

http://codereview.chromium.org/355003/diff/2002/2008#newcode12
Line 12: // This function performs SHA-160 operations.
On 2009/11/03 00:26:17, wtc wrote:
> "SHA-160" is a nonstandard term.  Please use "SHA-1" in
> comments and "SHA1" in code.

Done.

http://codereview.chromium.org/355003/diff/2002/2008#newcode22
Line 22: std::string SHA160HashString(const std::string& str);
On 2009/11/03 00:26:17, wtc wrote:
> We should change SHA256HashString to have the same prototype.
> Would you mind doing that in this CL or in a separate CL?

I'll make that change in a follow-up CL, as this CL is getting pretty big.

[wtc, 2009-11-03 01:24:17]

http://codereview.chromium.org/355003/diff/2035/2042
File base/sha1.cc (right):

http://codereview.chromium.org/355003/diff/2035/2042#newcode18
Line 18: // http://www.google.com/search?q=lucky:+FIPS+PUB+180-3
I would just give the URL for the document:
http://csrc.nist.gov/publications/fips/fips180-3/fips180-3_final.pdf

http://codereview.chromium.org/355003/diff/2035/2042#newcode24
Line 24: //   AddBytes(moredata, size of data);
The AddBytes operation is commonly called "Update"
and the Fini operation "Final" in crypto APIs.  So
I'd suggest that you rename these methods "Update"
and "Final".

You're missing "sha." before "AddBytes".

http://codereview.chromium.org/355003/diff/2035/2042#newcode122
Line 122: this->Process();
Remove "this->".

http://codereview.chromium.org/355003/diff/2035/2042#newcode135
Line 135: this->Process();
Remove "this->".

http://codereview.chromium.org/355003/diff/2035/2042#newcode151
Line 151: // memcpy(W, M, sizeof(M));
Why is this memcpy call commented out?

http://codereview.chromium.org/355003/diff/2035/2041
File base/sha1.h (right):

http://codereview.chromium.org/355003/diff/2035/2041#newcode14
Line 14: // TODO(jhawkins): Add unit tests.
Since you implement SHA-1 yourself, I'm going to insist that
you add unit tests as part of this CL.  I found some SHA-1
test data in
http://csrc.nist.gov/groups/ST/toolkit/documents/Examples/SHA1.pdf

http://codereview.chromium.org/355003/diff/2035/2041#newcode17
Line 17: SHA160_LENGTH = 20  // Length in bytes of a SHA-1 hash.
This enum constant should be renamed SHA1_LENGTH.

[James Hawkins, 2009-11-03 02:38:41]

http://codereview.chromium.org/355003/diff/2035/2042
File base/sha1.cc (right):

http://codereview.chromium.org/355003/diff/2035/2042#newcode18
Line 18: // http://www.google.com/search?q=lucky:+FIPS+PUB+180-3
On 2009/11/03 01:24:18, wtc wrote:
> I would just give the URL for the document:
> http://csrc.nist.gov/publications/fips/fips180-3/fips180-3_final.pdf

Done.

http://codereview.chromium.org/355003/diff/2035/2042#newcode24
Line 24: //   AddBytes(moredata, size of data);
On 2009/11/03 01:24:18, wtc wrote:
> The AddBytes operation is commonly called "Update"
> and the Fini operation "Final" in crypto APIs.  So
> I'd suggest that you rename these methods "Update"
> and "Final".
> 
> You're missing "sha." before "AddBytes".

Done.

http://codereview.chromium.org/355003/diff/2035/2042#newcode122
Line 122: this->Process();
On 2009/11/03 01:24:18, wtc wrote:
> Remove "this->".

Done.

http://codereview.chromium.org/355003/diff/2035/2042#newcode135
Line 135: this->Process();
On 2009/11/03 01:24:18, wtc wrote:
> Remove "this->".

Done.

http://codereview.chromium.org/355003/diff/2035/2042#newcode151
Line 151: // memcpy(W, M, sizeof(M));
On 2009/11/03 01:24:18, wtc wrote:
> Why is this memcpy call commented out?

W and M are in a union, so no need for the memcpy.  I've added a comment to
explain this.

http://codereview.chromium.org/355003/diff/2035/2041
File base/sha1.h (right):

http://codereview.chromium.org/355003/diff/2035/2041#newcode14
Line 14: // TODO(jhawkins): Add unit tests.
On 2009/11/03 01:24:18, wtc wrote:
> Since you implement SHA-1 yourself, I'm going to insist that
> you add unit tests as part of this CL.  I found some SHA-1
> test data in
> http://csrc.nist.gov/groups/ST/toolkit/documents/Examples/SHA1.pdf

Done.

http://codereview.chromium.org/355003/diff/2035/2041#newcode17
Line 17: SHA160_LENGTH = 20  // Length in bytes of a SHA-1 hash.
On 2009/11/03 01:24:18, wtc wrote:
> This enum constant should be renamed SHA1_LENGTH.

Done.

[James Hawkins, 2009-11-03 03:01:47]

On Mon, Nov 2, 2009 at 7:51 PM,  <wtc@chromium.org> wrote:
> LGTM.
>
>
> http://codereview.chromium.org/355003/diff/3008/4014
> File base/sha1.cc (right):
>
> http://codereview.chromium.org/355003/diff/3008/4014#newcode11
> Line 11: // Implementation of SHA-1. Only handles data in byte-sized
> blocks,
> Let's add a TODO comment here that this will be replaced by
> calling the SHA-1 functions in the system crypto libraries
> directly.
>
> Note: as a result, I didn't review your SHA-1 implementation
> carefully.
>

Done.

> http://codereview.chromium.org/355003/diff/3008/4014#newcode150
> Line 150: // a.
> Nit: would be nice to explain what these a, b, c, d, and e
> in the comments mean. =A0I guess they refer to sections in
> the standard?
>

Yes.  Added a comment.

> http://codereview.chromium.org/355003/diff/3008/4024
> File base/sha1_unittest.cc (right):
>
> http://codereview.chromium.org/355003/diff/3008/4024#newcode1
> Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights
> reserved.
> This should be 2009.
>

Done.

> Did you copy this file from sha2_unittest.cc? =A0If so, could
> you use "svn cp" to add this file? =A0Don't worry about it if
> it's too much trouble to re-add this file that way.
>

I didn't copy the file directly, just the license header.  I grokked
the style from sha2_unittest.cc, but wrote my own tests.

> http://codereview.chromium.org/355003/diff/3008/4024#newcode7
> Line 7: #include <string>
> This shouldn't be necessary as sha1.h already includes <string>.
>

I'd prefer to keep it in.  I'm a strict proponent of
include-what-you-use per-file, for the sake of dependencies and
readability.

> http://codereview.chromium.org/355003
>

[darin (slow to review), 2009-11-03 03:40:43]

http://codereview.chromium.org/355003/diff/4044/4045
File webkit/glue/form_field.cc (right):

http://codereview.chromium.org/355003/diff/4044/4045#newcode26
Line 26: return hash.str();
UintToString?  please do not use std streams outside of logging per google
style.

http://codereview.chromium.org/355003/diff/4044/4048
File webkit/glue/form_field.h (right):

http://codereview.chromium.org/355003/diff/4044/4048#newcode11
Line 11: #include "chrome/browser/autofill/field_types.h"
oops, this dependency on chrome/browser is not permitted.  src/chrome can depend
on src/webkit, but not the other way around.

http://codereview.chromium.org/355003/diff/4044/4047
File webkit/glue/form_field_values.cc (right):

http://codereview.chromium.org/355003/diff/4044/4047#newcode86
Line 86: if (type.length() == 0)
nit: if (type.empty())

[James Hawkins, 2009-11-03 04:35:44]

http://codereview.chromium.org/355003/diff/4044/4045
File webkit/glue/form_field.cc (right):

http://codereview.chromium.org/355003/diff/4044/4045#newcode26
Line 26: return hash.str();
On 2009/11/03 03:40:45, darin wrote:
> UintToString?  please do not use std streams outside of logging per google
> style.

Thanks!  I was looking for a function like this.

http://codereview.chromium.org/355003/diff/4044/4048
File webkit/glue/form_field.h (right):

http://codereview.chromium.org/355003/diff/4044/4048#newcode11
Line 11: #include "chrome/browser/autofill/field_types.h"
On 2009/11/03 03:40:45, darin wrote:
> oops, this dependency on chrome/browser is not permitted.  src/chrome can
depend
> on src/webkit, but not the other way around.

Ok.  I added AutoFillField which inherits from webkit_glue::FormField, adding
the autofill-specific bits.

http://codereview.chromium.org/355003/diff/4044/4047
File webkit/glue/form_field_values.cc (right):

http://codereview.chromium.org/355003/diff/4044/4047#newcode86
Line 86: if (type.length() == 0)
On 2009/11/03 03:40:45, darin wrote:
> nit: if (type.empty())

Done.

[darin (slow to review), 2009-11-04 19:20:00]

OK, LGTM