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..7f31ba8b419c7b5394b9fe09d466fbd12999e828 100644
--- a/chrome/browser/autofill/password_generator.cc
+++ b/chrome/browser/autofill/password_generator.cc
@@ -4,21 +4,103 @@
 
 #include "chrome/browser/autofill/password_generator.h"
 
+#include <algorithm>
+#include <vector>
+
+#include "base/basictypes.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 are
+// disjointed.
+const char kOtherSymbols[] =
+    {'!', '\"', '#', '$', '%', '&', '\'', '(',
+     ')', '*', '+', ',', '-', '.', '/', ':',
+     ';', '<', '=', '>', '?', '@', '[', '\\',
+     ']', '^', '_', '`', '{', '|', '}', '~'};
+const int kMinPasswordLength = 4;
+const int kMaxPasswordLength = 20;
+
+namespace {
+
+// Classic algorithm to randomly select |num_select| elements out of
+// |num_total| elements. One description can be found at:
+// "http://stackoverflow.com/questions/48087/select-a-random-n-elements-from-listt-in-c-sharp/48089#48089"
+void RandomSelection(int num_select,

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

nit: Perhaps "GetRandomSelection", "num_to_select"?

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

Done.

+                     int num_total,
+                     std::vector<int>* selections) {
+  DCHECK_GE(num_total, num_select);
+  int num_left = num_total;
+  int num_needed = num_select;
+  for (int i = 0; i < num_total && num_needed > 0; ++i) {
+    // we have probability = |num_needed| / |num_left| to select
+    // this position.
+    int probability = base::RandInt(0, num_left - 1);
+    if (probability < num_needed) {
+      selections->push_back(i);
+      --num_needed;
+    }
+    --num_left;
+  }
+  DCHECK_EQ(num_select, static_cast<int>(selections->size()));
+}
+
+}  // namespace
 
 namespace autofill {
 
-PasswordGenerator::PasswordGenerator() {}
+PasswordGenerator::PasswordGenerator()
+    : password_length_(kDefaultPasswordLength) {}
+
+PasswordGenerator::PasswordGenerator(int max_length)
+    : password_length_(SetLength(max_length)) {}
+
 PasswordGenerator::~PasswordGenerator() {}
 
-std::string PasswordGenerator::Generate() {
+int PasswordGenerator::SetLength(int max_length) {
+  return max_length >= kMinPasswordLength && max_length <= kMaxPasswordLength ?
+      max_length : kDefaultPasswordLength;

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

nit: This is probably fine as is, but I think it would be a little cleaner if
written as an if-else rather than as a ternary operator.  (In general, I prefer
to avoid ternary operators if they can't fit on one line.)

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

Done.

+}
+
+std::string PasswordGenerator::Generate() const {
   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.
+  std::vector<int> positions;
+  RandomSelection(4, password_length_, &positions);
+
+  // To enhance the strengh of the password, we random suffle the positions so
+  // that the 4 catagories can be put at a random position in it.
+  random_shuffle(positions.begin(), positions.end());

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

nit: Shouldn't this be std::random_shuffle?

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

Done.

+
+  // Next, generate each character of the password.
+  for (int i = 0; i < password_length_; ++i) {
+    if (i == positions.at(0)) {

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

Please don't use std::vector::at() -- it can throw an exception, and the
Chromium style guide disallows exceptions.

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

Done.

+      // Generate random upper case letter.
+      ret.push_back(static_cast<char>(base::RandInt(kMinUpper, kMaxUpper)));
+    } else if (i == positions.at(1)) {
+      // Generate random lower case letter.
+      ret.push_back(static_cast<char>(base::RandInt(kMinLower, kMaxLower)));
+    } else if (i == positions.at(2)) {
+      // Generate random digit.
+      ret.push_back(static_cast<char>(base::RandInt(kMinDigit, kMaxDigit)));
+    } else if (i == positions.at(3)) {
+      // Generate random other symbol.
+      ret.push_back(kOtherSymbols[base::RandInt(0, arraysize(kOtherSymbols))]);
+    } else {
+      // Generate random character from all categories.
+      ret.push_back(static_cast<char>(base::RandInt(kMinChar, kMaxChar)));
+    }
   }
   return ret;
 }