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

chrome/browser/autofill/password_generator.cc

Index: chrome/browser/autofill/password_generator.cc
diff --git a/chrome/browser/autofill/password_generator.cc b/chrome/browser/autofill/password_generator.cc
index 3361fb47d1dc4df19cc243c45dcd5fe97c3f1b2d..c5b8fe6bc518390447f4bfa8603781027fd04dc4 100644
--- a/chrome/browser/autofill/password_generator.cc
+++ b/chrome/browser/autofill/password_generator.cc
@@ -4,22 +4,98 @@
 
 #include "chrome/browser/autofill/password_generator.h"
 
+#include "base/logging.h"
 #include "base/rand_util.h"
 
 const int kMinChar = 33;   // First printable character '!'
 const int kMaxChar = 126;  // Last printable character '~'
-const int kPasswordLength = 12;
+const int kMinUpper = 65;  // First upper case letter 'A'
+const int kMaxUpper = 90;  // Last upper case letter 'Z'
+const int kMinLower = 97;  // First lower case letter 'a'
+const int kMaxLower = 122; // Last lower case letter 'z'
+const int kMinDigit = 48;  // First digit '0'
+const int kMaxDigit = 57;  // Last digit '9'
+// Copy of the other printable symbols from the ascii table since they

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

nit: "ascii" -> "ASCII"

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

Done.

+// are disjointed.
+const char kOtherSymbols[32] =

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

nit: No need to specify "32", and it would be inconvenient to update this
constant in case the number of elements in the array changes (e.g. perhaps some
characters are too often rejected in passwords).

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

Done.

+    {'!', '\"', '#', '$', '%', '&', '\'', '(',
+     ')', '*', '+', ',', '-', '.', '/', ':',
+     ';', '<', '=', '>', '?', '@', '[', '\\',
+     ']', '^', '_', '`', '{', '|', '}', '~'};
+const int kMinPasswordLength = 4;
+const int kMaxPasswordLength = 20;
+
+namespace {
+
+// Classic algorithm to randomly select M elements out of N.
+// One description can be found at: "http://stackoverflow.com/questions/48087/select-a-random-n-elements-from-listt-in-c-sharp/48089#48089"
+int* SelectRandomMFromN(int M, int N) {

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

This should be a void function that takes a std::vector of ints to fill with the
randomly selected elements.

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

nit: M and N are completely obfuscated names.  Please give these descriptive
names.

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

Done.

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

Done.

+  DCHECK_GE(N, M);
+  int* result = new int[M];
+  int number_left = N;
+  int number_needed = M;
+  for (int i = 0; i < N; ++i) {
+    if (number_needed == 0)

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

nit: You can just add this to the loop condition.

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

Done.

+      break;
+    // we have probability = number_needed / number_left to select
+    // this position.
+    int probability = base::RandInt(0, number_left - 1);
+    if (probability < number_needed) {
+      result[4 - number_needed] = i;

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

The hardcoded "4" does not make sense now that this function has been
generalized...

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

Done.

+      --number_needed;
+    }
+    --number_left;
+  }
+  DCHECK_EQ(0, number_needed);
+  return result;
+}
+
+}  // namespace
 
 namespace autofill {
 
-PasswordGenerator::PasswordGenerator() {}
+PasswordGenerator::PasswordGenerator()
+    : password_length_(kDefaultPasswordLength) {}

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

nit: If we need to keep this default constructor for some reason, please add a
blank line between lines 58 and 59.

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

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

+PasswordGenerator::PasswordGenerator(int max_length)
+    : password_length_(
+        max_length >= kMinPasswordLength && max_length <= kMaxPasswordLength ?
+            max_length : kDefaultPasswordLength) {}

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

nit: Please decompose this logic into a tiny helper function, so that the
initializer list isn't so complex-looking.

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

Done.

 PasswordGenerator::~PasswordGenerator() {}
 
 std::string PasswordGenerator::Generate() {
   std::string ret;
-  for (int i = 0; i < kPasswordLength; i++) {
-    ret.push_back(static_cast<char>(base::RandInt(kMinChar, kMaxChar)));
+  // First, randomly select 4 positions to hold one upper case letter,
+  // one lower case letter, one digit, and one other symbol respectively,
+  // to make sure at least one of each category of characters will be
+  // included in the password.
+  int* positions = SelectRandomMFromN(4, password_length_);
+
+  // To enhance the strenght of the password, the upper case letter will be

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

nit: "strenght" -> "strength"

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

Done.

+  // put at "start"th position, lower case letter will be put at
+  // "(start+1)%4"th position, digit will be put at "(start+2)%4"th position,
+  // and other symbol will be put at "(start+3)%4"th position.
+  int start = base::RandInt(0,3);

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

Please use std::random_shuffle [1] instead.
[1] http://www.cplusplus.com/reference/algorithm/random_shuffle/

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

Good to know! Done.

+
+  // Next, generate each character of the password.
+  for (int i = 0; i < password_length_; ++i) {
+    if (i == positions[start]) {
+      // Generate random upper case letter.
+      ret.push_back(static_cast<char>(base::RandInt(kMinUpper, kMaxUpper)));
+    } else if (i == positions[(start + 1) % 4]) {
+      // Generate random lower case letter.
+      ret.push_back(static_cast<char>(base::RandInt(kMinLower, kMaxLower)));
+    } else if (i == positions[(start + 2) % 4]) {
+      // Generate random digit.
+      ret.push_back(static_cast<char>(base::RandInt(kMinDigit, kMaxDigit)));
+    } else if (i == positions[(start + 3) % 4]) {
+      // Generate random other symbol.
+      ret.push_back(kOtherSymbols[base::RandInt(0, 31)]);

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

nit: Please use the arraysize macro [1] rather than a hardcoded constant.
[1] http://code.google.com/searchframe#OAMlx_jo-ck/src/base/basictypes.h&l=130

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

Done.

+    } else {
+      // Generate random character from all categories.
+      ret.push_back(static_cast<char>(base::RandInt(kMinChar, kMaxChar)));
+    }
   }
+  delete positions;

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

Chromium code should almost never need explicit "delete" statements.  Please use
a scoped_ptr... or better yet, an std::vector

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

Done.

   return ret;
 }