This CL does the following: (1) Pass the max_length attribute to the password generator; (2) Update…

chrome/browser/autofill/password_generator.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_AUTOFILL_PASSWORD_GENERATOR_H_
 #define CHROME_BROWSER_AUTOFILL_PASSWORD_GENERATOR_H_
 #pragma once
 
 #include <string>
 
 #include "base/basictypes.h"
 
+const unsigned int kDefaultPasswordLength = 12;

[Ilya Sherman, 2012/06/01 01:09:55]

This should still be a member of the PasswordGenerator class, with private or
protected visibility.

[zysxqn, 2012/06/01 19:01:32]

Hm, can you elaborate more on this? In that case we need to wrap up a test class
and make it friend class to access this number, and the only advantage I can
think of is that other classes can't access this const which I don't think is a
big issue here.

[Ilya Sherman, 2012/06/01 19:32:10]

There is only one test that needs access to this constant.  You can make the
constant visible to that test by using the FRIEND_TEST_ALL_PREFIXES macro [1]. 
By making the constant private to the class, you can ensure that there's no
other code in production that accidentally develops a dependency on this
constant; and in general, it's just good code hygiene to keep implementation
details private.
[1]
http://code.google.com/searchframe#OAMlx_jo-ck/src/base/gtest_prod_util.h&q=f...

[zysxqn, 2012/06/01 22:32:54]

Done.

+
 namespace autofill {
 
 // Class to generate random passwords. Currently we just use a generic algorithm
 // for all sites, but eventually we can incorporate additional information to
 // determine passwords that are likely to be accepted (i.e. use pattern field,
 // previous generated passwords, crowdsourcing, etc.)
 class PasswordGenerator {
  public:
   PasswordGenerator();

[Ilya Sherman, 2012/06/01 01:09:55]

nit: Is this constructor still needed?

[zysxqn, 2012/06/01 19:01:32]

Prefer to keep it in case some application just want to use the default length
(e.g. unit tests)

[Ilya Sherman, 2012/06/01 19:32:10]

It's easy enough to re-add in case it's ever needed later.  I've seen lots of
bugs caused by test-specific constructors, and would strongly recommend avoiding
adding any constructors that will be used by unit tests only.

[zysxqn, 2012/06/01 22:32:54]

I'm fine with either way so I remove it as you suggested.

+  explicit PasswordGenerator(int max_length);

[Ilya Sherman, 2012/06/01 01:09:55]

nit: Please comment on the meaning of |max_length|, i.e. it is used as a hint
for the generated password's length.

[zysxqn, 2012/06/01 19:01:32]

Done.

   ~PasswordGenerator();
 
-  // Returns a random password. The string is guaranteed to be printable and
-  // will not include whitespace characters.
+  // Returns a random password such that:
+  // (1) Each of its character is guaranteed to be printable and will not
+  //     include whitespace characters.

[Ilya Sherman, 2012/06/01 01:09:55]

nit: Perhaps: "Each character is guaranteed to be a non-whitespace printable
ASCII character."

[zysxqn, 2012/06/01 19:01:32]

Done.

+  // (2) The generated password will contain AT LEAST one upper case letter, one
+  //     lower case letter, one digit, and one other symbol.
+  // (3) The length of the password is determined by |max_length|, which is
+  //     passed through the corresponding renderer by examining the
+  //     InputElementAttribute. In particular, if |max_length| is reasonable
+  //     (i.e. [4, 20]), we set the |password_length_| to it. Otherwise, we use
+  //     the default length of 12.

[Ilya Sherman, 2012/06/01 01:09:55]

nit: This includes too many implementation details.  Perhaps: "The password
length will be equal to |password_length_| (see comment for the constructor)."
or something like that?

[zysxqn, 2012/06/01 19:01:32]

Done.

   std::string Generate();
 
  private:
+  // The length of the generated password.
+  int password_length_;

[Ilya Sherman, 2012/06/01 01:09:55]

nit: const size_t?

[zysxqn, 2012/06/01 19:01:32]

Won't this make comparisons more complex since all other numbers are int?

[Ilya Sherman, 2012/06/01 19:32:10]

The other numbers should probably also all be unsigned.  In any case, at a
minimum, this should be const.

[zysxqn, 2012/06/01 22:32:54]

Changed all variables related to password length to be size_t.

 
   DISALLOW_COPY_AND_ASSIGN(PasswordGenerator);
 };
 
 }  // namespace autofill
 
 #endif  // CHROME_BROWSER_AUTOFILL_PASSWORD_GENERATOR_H_