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

chrome/browser/autofill/password_generator.cc

 // 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.
 
 #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;
 
 namespace autofill {
 
 PasswordGenerator::PasswordGenerator() {}
 PasswordGenerator::~PasswordGenerator() {}
 
-std::string PasswordGenerator::Generate() {
+std::string PasswordGenerator::Generate(int max_length) {
   std::string ret;
-  for (int i = 0; i < kPasswordLength; i++) {
-    ret.push_back(static_cast<char>(base::RandInt(kMinChar, kMaxChar)));
+  int length = max_length > 3 ? max_length : kPasswordLength;

[Ilya Sherman, 2012/05/30 00:04:32]

nit: The header does not give any indication that the max_length will be ignored
if it is too small.  Please document this behavior.

[Garrett Casto, 2012/05/30 00:46:35]

If maxLength isn't set on an element, it looks like WebKit returns 2^19. So you
probably should cap the size here to kPasswordLength.

[zysxqn, 2012/05/31 21:54:08]

Done.

[zysxqn, 2012/05/31 21:54:08]

Use separate kMinPasswordLength, kMaxPasswordLength, and kDefaultPasswordLength.

+  // First, randomly select 4 places 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.
+  //
+  // There is a classific algorithm for this and one description can be

[Ilya Sherman, 2012/05/30 00:04:32]

nit: "classific" -> "classic"

[Garrett Casto, 2012/05/30 00:46:35]

classific?

[zysxqn, 2012/05/31 21:54:08]

Done.

[zysxqn, 2012/05/31 21:54:08]

Done.

+  // found at: "http://stackoverflow.com/questions/48087/select-a-random-n-"
+  //           "elements-from-listt-in-c-sharp/48089#48089"

[Ilya Sherman, 2012/05/30 00:04:32]

nit: This comment and the block implementing it should probably be extracted to
a helper function in the anonymous namespace.

[Ilya Sherman, 2012/05/30 00:04:32]

nit: URLs should not be broken across multiple lines; this is an exception to
the 80-col rule.

[zysxqn, 2012/05/31 21:54:08]

Done.

[zysxqn, 2012/05/31 21:54:08]

Done.

+  int number_left = length;
+  int number_needed = 4;
+  int position[4];

[Ilya Sherman, 2012/05/30 00:04:32]

nit: "positions" (plural)?

[zysxqn, 2012/05/31 21:54:08]

Done.

+  for (int i = 0; i < length; i++) {
+    if (number_needed == 0) {
+      break;
+    }

[Ilya Sherman, 2012/05/30 00:04:32]

nit: No need for curly braces, since this is a one-line if-stmt.

[zysxqn, 2012/05/31 21:54:08]

Done.

+    // we have prob = number_needed / number_left to select this position.
+    int prob = base::RandInt(0, number_left - 1);

[Ilya Sherman, 2012/05/30 00:04:32]

nit: "prob" -> "probability" (per the style guide, we prefer to avoid
abbreviations)

[zysxqn, 2012/05/31 21:54:08]

Done.

+    if (prob < number_needed) {
+      position[4 - number_needed--] = i;

[Ilya Sherman, 2012/05/30 00:04:32]

nit: Please avoid using the "--" operator nested within a statement.  You should
write this as two lines.

[Garrett Casto, 2012/05/30 00:46:35]

Seconded.

[zysxqn, 2012/05/31 21:54:08]

Done.

[zysxqn, 2012/05/31 21:54:08]

Done.

+    }
+    --number_left;
+  }
+  CHECK(number_needed == 0);

[Ilya Sherman, 2012/05/30 00:04:32]

nit: DCHECK_EQ(0, number_needed)

[zysxqn, 2012/05/31 21:54:08]

Done.

+
+  // Next, generate each character of the password.
+  for (int i = 0; i < length; i++) {
+    if (i == position[0]) {
+      // Generate random upper case letter.

[Ilya Sherman, 2012/05/30 00:04:32]

This approach guarantees that the upper-case letter will always precede the
lower-case letter, which will always precede the digit, etc.  This effectively
reduces the security of the password.

[zysxqn, 2012/05/31 21:54:08]

This is true. For solving this ideally we can randomly select one permutation
from all 24 possible permutations. However, there seems no elegant algorithm for
this. For simplicity, I changed the code to arrange this 4 categories randomly
like a "rotation". This increases the strength of the password by 4 times.

+      ret.push_back(static_cast<char>(base::RandInt(65, 90)));

[Ilya Sherman, 2012/05/30 00:04:32]

Please use named constants rather than raw numeric values for ASCII constants.

[zysxqn, 2012/05/31 21:54:08]

Done.

+    } else if (i == position[1]) {
+      // Generate random lower case letter.
+      ret.push_back(static_cast<char>(base::RandInt(97, 122)));
+    } else if (i == position[2]) {
+      // Generate random digit.
+      ret.push_back(static_cast<char>(base::RandInt(48, 57)));
+    } else if (i == position[3]) {
+      // Generate random other symbol.
+      // There are 32 such characters from 4 intervals in ascii.

[Ilya Sherman, 2012/05/30 00:04:32]

It might be clearer to just write out an array including these 32 characters. 
The current code has a lot of unnecessary complexity, from a readability point
of view.

[Garrett Casto, 2012/05/30 00:46:35]

I was actually going to suggest that all of these 4 cases be written out
explicitly. 

[zysxqn, 2012/05/31 21:54:08]

Done.

[zysxqn, 2012/05/31 21:54:08]

Done.

+      int index = base::RandInt(0, 31);
+      if (index < 15) {
+        ret.push_back(static_cast<char>(index + 33));
+      } else if (index < 22) {
+        ret.push_back(static_cast<char>(index + 43));
+      } else if (index < 28) {
+        ret.push_back(static_cast<char>(index + 69));
+      } else {
+        ret.push_back(static_cast<char>(index + 95));
+      }
+    } else {
+      // Generate random character from all categories.
+      ret.push_back(static_cast<char>(base::RandInt(kMinChar, kMaxChar)));
+    }
   }
   return ret;
 }
 
 }  // namespace autofill