Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10458018:
This CL does the following: (1) Pass the max_length attribute to the password generator; (2) Update… (Closed)
|
Created:
8 years, 6 months ago by zysxqn Modified:
8 years, 6 months ago CC:
chromium-reviews, dhollowa+watch_chromium.org, brettw-cc_chromium.org, Ilya Sherman, dyu1, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionThis CL does the following: (1) Pass the max_length attribute to the password generator; (2) Update the password generation algorithm to contain at least one upper case letter, one lower case letter, one digit, and one other symbol.
BUG=
TEST=PasswordGeneratorTest, AutofillManagerTest, PasswordGenerationManagerTest.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=140812
Patch Set 1 #Patch Set 2 : Fix a bug in Windows UI. #Patch Set 3 : Fix a bug in Windows UI. #
Total comments: 48
Patch Set 4 : Make password generator independent of platforms. #
Total comments: 52
Patch Set 5 : Some changes based on the code review. #
Total comments: 10
Patch Set 6 : Address comments. #Patch Set 7 : Fix a bug. #
Total comments: 8
Patch Set 8 : Addressing some more comments. #Patch Set 9 : Some nit. #
Total comments: 8
Patch Set 10 : Futher comments. #
Total comments: 4
Patch Set 11 : Solve conflicts. #
Total comments: 4
Patch Set 12 : Update some comments. #Patch Set 13 : Fix a windows UI bug. #Messages
Total messages: 36 (0 generated)
This change is not UI-specific, so I am surprised by the number of UI-specific headers being touched by this change. Is there not a shared location where the max_length can be stored? If not, I would recommend creating a platform-agnostic parent class for this -- might be a good opportunity to factor out other shared UI code there as well. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... File chrome/browser/autofill/password_generator.cc (right): http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.cc:21: int length = max_length > 3 ? max_length : kPasswordLength; nit: The header does not give any indication that the max_length will be ignored if it is too small. Please document this behavior. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.cc:27: // There is a classific algorithm for this and one description can be nit: "classific" -> "classic" http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.cc:29: // "elements-from-listt-in-c-sharp/48089#48089" nit: This comment and the block implementing it should probably be extracted to a helper function in the anonymous namespace. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.cc:29: // "elements-from-listt-in-c-sharp/48089#48089" nit: URLs should not be broken across multiple lines; this is an exception to the 80-col rule. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.cc:32: int position[4]; nit: "positions" (plural)? http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.cc:36: } nit: No need for curly braces, since this is a one-line if-stmt. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.cc:38: int prob = base::RandInt(0, number_left - 1); nit: "prob" -> "probability" (per the style guide, we prefer to avoid abbreviations) http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.cc:40: position[4 - number_needed--] = i; nit: Please avoid using the "--" operator nested within a statement. You should write this as two lines. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.cc:44: CHECK(number_needed == 0); nit: DCHECK_EQ(0, number_needed) http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.cc:49: // Generate random upper case letter. 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. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.cc:50: ret.push_back(static_cast<char>(base::RandInt(65, 90))); Please use named constants rather than raw numeric values for ASCII constants. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.cc:59: // There are 32 such characters from 4 intervals in ascii. 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. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... File chrome/browser/autofill/password_generator_unittest.cc (right): http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator_unittest.cc:15: // Check max_length. nit: Please separate this out to a separate named test. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator_unittest.cc:15: // Check max_length. nit: Please add a test case for max_length < 0. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator_unittest.cc:19: EXPECT_TRUE(password.size() == 12); nit: 12 is a magic number here. If you need to use this constant in both the implementation file and the test file, you should export it in the header (probably in the private: section, with this test listed as a friend). http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator_unittest.cc:20: // Check password. nit: Please separate this out to a separate named test. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator_unittest.cc:36: } nit: No need for curly braces, since these are all one-line if-stmts. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator_unittest.cc:38: EXPECT_TRUE(num_upper_case_letters > 0); nit: EXPECT_GT
It looks like Ilya and I collided with some of our comments. I've tried to remove the duplicates but I might not have gotten them all. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager.cc (right): http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.cc:727: browser->window()->ShowPasswordGenerationBubble(bounds, max_length); Note that if we end up having the password generator be passed into the bubble, I would change this to call some function on Browser (Browser::CreatePasswordGenerationBubble?) that would create a PasswordGenerator and then call BrowserWindow::ShowPasswordGenerationBubble using that generator. This would make the PasswordGenerator code platform agnostic. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... File chrome/browser/autofill/password_generator.cc (right): http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.cc:21: int length = max_length > 3 ? max_length : kPasswordLength; 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. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.cc:27: // There is a classific algorithm for this and one description can be classific? http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.cc:40: position[4 - number_needed--] = i; On 2012/05/30 00:04:32, Ilya Sherman wrote: > nit: Please avoid using the "--" operator nested within a statement. You should > write this as two lines. Seconded. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.cc:59: // There are 32 such characters from 4 intervals in ascii. On 2012/05/30 00:04:32, Ilya Sherman wrote: > 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. I was actually going to suggest that all of these 4 cases be written out explicitly. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... File chrome/browser/autofill/password_generator.h (right): http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.h:28: std::string Generate(int max_length); I was wondering if we might want to change the way that we create the PasswordGenerator. Right now the bubble itself owns the generator, which has some limitations. 1) The lifetime of the generator is bound to the bubble. 2) We need to change each bubble implementation whenever we change how the generator works. I was thinking that it would be better to pass in a PasswordGenerator when we instantiate a bubble. This means that the UI code doesn't need to know about how we generate the password, since that information isn't UI specific. It should make changing this easier in the future, like when we want to use the pattern attribute. Alternatively we can have a parent class for all the bubbles. This has the advantage that it makes adding things like UMA stats much easier when the time comes for that.
Made the password generator independent of platforms. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager.cc (right): http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.cc:727: browser->window()->ShowPasswordGenerationBubble(bounds, max_length); On 2012/05/30 00:46:35, Garrett Casto wrote: > Note that if we end up having the password generator be passed into the bubble, > I would change this to call some function on Browser > (Browser::CreatePasswordGenerationBubble?) that would create a PasswordGenerator > and then call BrowserWindow::ShowPasswordGenerationBubble using that generator. > This would make the PasswordGenerator code platform agnostic. Why can't we create a PasswordGenerator directly here? http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... File chrome/browser/autofill/password_generator.cc (right): http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.cc:21: int length = max_length > 3 ? max_length : kPasswordLength; On 2012/05/30 00:04:32, Ilya Sherman wrote: > nit: The header does not give any indication that the max_length will be ignored > if it is too small. Please document this behavior. Done. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.cc:21: int length = max_length > 3 ? max_length : kPasswordLength; On 2012/05/30 00:46:35, Garrett Casto wrote: > 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. Use separate kMinPasswordLength, kMaxPasswordLength, and kDefaultPasswordLength. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.cc:27: // There is a classific algorithm for this and one description can be On 2012/05/30 00:04:32, Ilya Sherman wrote: > nit: "classific" -> "classic" Done. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.cc:27: // There is a classific algorithm for this and one description can be On 2012/05/30 00:46:35, Garrett Casto wrote: > classific? Done. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.cc:29: // "elements-from-listt-in-c-sharp/48089#48089" On 2012/05/30 00:04:32, Ilya Sherman wrote: > nit: This comment and the block implementing it should probably be extracted to > a helper function in the anonymous namespace. Done. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.cc:29: // "elements-from-listt-in-c-sharp/48089#48089" On 2012/05/30 00:04:32, Ilya Sherman wrote: > nit: URLs should not be broken across multiple lines; this is an exception to > the 80-col rule. Done. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.cc:32: int position[4]; On 2012/05/30 00:04:32, Ilya Sherman wrote: > nit: "positions" (plural)? Done. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.cc:36: } On 2012/05/30 00:04:32, Ilya Sherman wrote: > nit: No need for curly braces, since this is a one-line if-stmt. Done. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.cc:38: int prob = base::RandInt(0, number_left - 1); On 2012/05/30 00:04:32, Ilya Sherman wrote: > nit: "prob" -> "probability" (per the style guide, we prefer to avoid > abbreviations) Done. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.cc:40: position[4 - number_needed--] = i; On 2012/05/30 00:04:32, Ilya Sherman wrote: > nit: Please avoid using the "--" operator nested within a statement. You should > write this as two lines. Done. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.cc:40: position[4 - number_needed--] = i; On 2012/05/30 00:46:35, Garrett Casto wrote: > On 2012/05/30 00:04:32, Ilya Sherman wrote: > > nit: Please avoid using the "--" operator nested within a statement. You > should > > write this as two lines. > > Seconded. Done. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.cc:44: CHECK(number_needed == 0); On 2012/05/30 00:04:32, Ilya Sherman wrote: > nit: DCHECK_EQ(0, number_needed) Done. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.cc:49: // Generate random upper case letter. On 2012/05/30 00:04:32, Ilya Sherman wrote: > 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. 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. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.cc:50: ret.push_back(static_cast<char>(base::RandInt(65, 90))); On 2012/05/30 00:04:32, Ilya Sherman wrote: > Please use named constants rather than raw numeric values for ASCII constants. Done. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.cc:59: // There are 32 such characters from 4 intervals in ascii. On 2012/05/30 00:04:32, Ilya Sherman wrote: > 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. Done. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.cc:59: // There are 32 such characters from 4 intervals in ascii. On 2012/05/30 00:46:35, Garrett Casto wrote: > On 2012/05/30 00:04:32, Ilya Sherman wrote: > > 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. > > I was actually going to suggest that all of these 4 cases be written out > explicitly. Done. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... File chrome/browser/autofill/password_generator.h (right): http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator.h:28: std::string Generate(int max_length); On 2012/05/30 00:46:35, Garrett Casto wrote: > I was wondering if we might want to change the way that we create the > PasswordGenerator. Right now the bubble itself owns the generator, which has > some limitations. > > 1) The lifetime of the generator is bound to the bubble. > 2) We need to change each bubble implementation whenever we change how the > generator works. > > I was thinking that it would be better to pass in a PasswordGenerator when we > instantiate a bubble. This means that the UI code doesn't need to know about how > we generate the password, since that information isn't UI specific. It should > make changing this easier in the future, like when we want to use the pattern > attribute. > > Alternatively we can have a parent class for all the bubbles. This has the > advantage that it makes adding things like UMA stats much easier when the time > comes for that. I like the idea of passing a PasswordGenerator to the bubble. Changed. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... File chrome/browser/autofill/password_generator_unittest.cc (right): http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator_unittest.cc:15: // Check max_length. On 2012/05/30 00:04:32, Ilya Sherman wrote: > nit: Please separate this out to a separate named test. Done. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator_unittest.cc:15: // Check max_length. On 2012/05/30 00:04:32, Ilya Sherman wrote: > nit: Please separate this out to a separate named test. Done. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator_unittest.cc:19: EXPECT_TRUE(password.size() == 12); On 2012/05/30 00:04:32, Ilya Sherman wrote: > nit: 12 is a magic number here. If you need to use this constant in both the > implementation file and the test file, you should export it in the header > (probably in the private: section, with this test listed as a friend). Seems that it's easier to just define that constant in the header file so both test and .cc files can access it. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator_unittest.cc:20: // Check password. On 2012/05/30 00:04:32, Ilya Sherman wrote: > nit: Please separate this out to a separate named test. Done. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator_unittest.cc:36: } On 2012/05/30 00:04:32, Ilya Sherman wrote: > nit: No need for curly braces, since these are all one-line if-stmts. Done. http://codereview.chromium.org/10458018/diff/3001/chrome/browser/autofill/pas... chrome/browser/autofill/password_generator_unittest.cc:38: EXPECT_TRUE(num_upper_case_letters > 0); On 2012/05/30 00:04:32, Ilya Sherman wrote: > nit: EXPECT_GT Done.
https://chromiumcodereview.appspot.com/10458018/diff/11001/chrome/browser/aut... File chrome/browser/autofill/autofill_manager.cc (right): https://chromiumcodereview.appspot.com/10458018/diff/11001/chrome/browser/aut... chrome/browser/autofill/autofill_manager.cc:729: new autofill::PasswordGenerator(max_length)); This will be deleted immediately at the end of the function, which is probably not what you wanted, right? If that's not a problem, I would skip the heap-allocation and just declare the PasswordGenerator on the stack. https://chromiumcodereview.appspot.com/10458018/diff/11001/chrome/browser/aut... File chrome/browser/autofill/password_generator.cc (right): https://chromiumcodereview.appspot.com/10458018/diff/11001/chrome/browser/aut... chrome/browser/autofill/password_generator.cc:18: // Copy of the other printable symbols from the ascii table since they nit: "ascii" -> "ASCII" https://chromiumcodereview.appspot.com/10458018/diff/11001/chrome/browser/aut... chrome/browser/autofill/password_generator.cc:20: const char kOtherSymbols[32] = 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). https://chromiumcodereview.appspot.com/10458018/diff/11001/chrome/browser/aut... chrome/browser/autofill/password_generator.cc:32: int* SelectRandomMFromN(int M, int N) { This should be a void function that takes a std::vector of ints to fill with the randomly selected elements. https://chromiumcodereview.appspot.com/10458018/diff/11001/chrome/browser/aut... chrome/browser/autofill/password_generator.cc:32: int* SelectRandomMFromN(int M, int N) { nit: M and N are completely obfuscated names. Please give these descriptive names. https://chromiumcodereview.appspot.com/10458018/diff/11001/chrome/browser/aut... chrome/browser/autofill/password_generator.cc:38: if (number_needed == 0) nit: You can just add this to the loop condition. https://chromiumcodereview.appspot.com/10458018/diff/11001/chrome/browser/aut... chrome/browser/autofill/password_generator.cc:44: result[4 - number_needed] = i; The hardcoded "4" does not make sense now that this function has been generalized... https://chromiumcodereview.appspot.com/10458018/diff/11001/chrome/browser/aut... chrome/browser/autofill/password_generator.cc:58: : password_length_(kDefaultPasswordLength) {} nit: If we need to keep this default constructor for some reason, please add a blank line between lines 58 and 59. https://chromiumcodereview.appspot.com/10458018/diff/11001/chrome/browser/aut... chrome/browser/autofill/password_generator.cc:62: max_length : kDefaultPasswordLength) {} nit: Please decompose this logic into a tiny helper function, so that the initializer list isn't so complex-looking. https://chromiumcodereview.appspot.com/10458018/diff/11001/chrome/browser/aut... chrome/browser/autofill/password_generator.cc:73: // To enhance the strenght of the password, the upper case letter will be nit: "strenght" -> "strength" https://chromiumcodereview.appspot.com/10458018/diff/11001/chrome/browser/aut... chrome/browser/autofill/password_generator.cc:77: int start = base::RandInt(0,3); Please use std::random_shuffle [1] instead. [1] http://www.cplusplus.com/reference/algorithm/random_shuffle/ https://chromiumcodereview.appspot.com/10458018/diff/11001/chrome/browser/aut... chrome/browser/autofill/password_generator.cc:92: ret.push_back(kOtherSymbols[base::RandInt(0, 31)]); 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 https://chromiumcodereview.appspot.com/10458018/diff/11001/chrome/browser/aut... chrome/browser/autofill/password_generator.cc:98: delete positions; Chromium code should almost never need explicit "delete" statements. Please use a scoped_ptr... or better yet, an std::vector https://chromiumcodereview.appspot.com/10458018/diff/11001/chrome/browser/aut... File chrome/browser/autofill/password_generator.h (right): https://chromiumcodereview.appspot.com/10458018/diff/11001/chrome/browser/aut... chrome/browser/autofill/password_generator.h:13: const unsigned int kDefaultPasswordLength = 12; This should still be a member of the PasswordGenerator class, with private or protected visibility. https://chromiumcodereview.appspot.com/10458018/diff/11001/chrome/browser/aut... chrome/browser/autofill/password_generator.h:23: PasswordGenerator(); nit: Is this constructor still needed? https://chromiumcodereview.appspot.com/10458018/diff/11001/chrome/browser/aut... chrome/browser/autofill/password_generator.h:24: explicit PasswordGenerator(int max_length); nit: Please comment on the meaning of |max_length|, i.e. it is used as a hint for the generated password's length. https://chromiumcodereview.appspot.com/10458018/diff/11001/chrome/browser/aut... chrome/browser/autofill/password_generator.h:29: // include whitespace characters. nit: Perhaps: "Each character is guaranteed to be a non-whitespace printable ASCII character." https://chromiumcodereview.appspot.com/10458018/diff/11001/chrome/browser/aut... chrome/browser/autofill/password_generator.h:36: // the default length of 12. 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? https://chromiumcodereview.appspot.com/10458018/diff/11001/chrome/browser/aut... chrome/browser/autofill/password_generator.h:41: int password_length_; nit: const size_t? https://chromiumcodereview.appspot.com/10458018/diff/11001/chrome/browser/aut... File chrome/browser/autofill/password_generator_unittest.cc (right): https://chromiumcodereview.appspot.com/10458018/diff/11001/chrome/browser/aut... chrome/browser/autofill/password_generator_unittest.cc:13: TEST(PasswordGeneratorTest, TestPasswordLength) { nit: "TestPasswordLength" -> "PasswordLength" (and likewise for the other tests in this file) https://chromiumcodereview.appspot.com/10458018/diff/11001/chrome/browser/aut... chrome/browser/autofill/password_generator_unittest.cc:16: EXPECT_EQ(password.size(), 10u); nit: Please add a blank line after this line. https://chromiumcodereview.appspot.com/10458018/diff/11001/chrome/browser/aut... chrome/browser/autofill/password_generator_unittest.cc:19: EXPECT_EQ(password.size(), kDefaultPasswordLength); nit: Please add a blank line after this line.
http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_manager.cc (right): http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_manager.cc:729: new autofill::PasswordGenerator(max_length)); On 2012/06/01 01:09:55, Ilya Sherman wrote: > This will be deleted immediately at the end of the function, which is probably > not what you wanted, right? If that's not a problem, I would skip the > heap-allocation and just declare the PasswordGenerator on the stack. I don't think it's a problem here. Only when user click the generation icon on the password form again do we need to re-show the bubble and re-generate the password, and this will be done by create a new bubble and a new password generator. Changed to stack allocation. http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/pa... File chrome/browser/autofill/password_generator.cc (right): http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator.cc:18: // Copy of the other printable symbols from the ascii table since they On 2012/06/01 01:09:55, Ilya Sherman wrote: > nit: "ascii" -> "ASCII" Done. http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator.cc:20: const char kOtherSymbols[32] = On 2012/06/01 01:09:55, Ilya Sherman wrote: > 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). Done. http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator.cc:32: int* SelectRandomMFromN(int M, int N) { On 2012/06/01 01:09:55, Ilya Sherman wrote: > This should be a void function that takes a std::vector of ints to fill with the > randomly selected elements. Done. http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator.cc:32: int* SelectRandomMFromN(int M, int N) { On 2012/06/01 01:09:55, Ilya Sherman wrote: > nit: M and N are completely obfuscated names. Please give these descriptive > names. Done. http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator.cc:38: if (number_needed == 0) On 2012/06/01 01:09:55, Ilya Sherman wrote: > nit: You can just add this to the loop condition. Done. http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator.cc:44: result[4 - number_needed] = i; On 2012/06/01 01:09:55, Ilya Sherman wrote: > The hardcoded "4" does not make sense now that this function has been > generalized... Done. http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator.cc:58: : password_length_(kDefaultPasswordLength) {} On 2012/06/01 01:09:55, Ilya Sherman wrote: > nit: If we need to keep this default constructor for some reason, please add a > blank line between lines 58 and 59. Prefer to keep this in case some application just want to use the default length (e.g. unit tests). http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator.cc:62: max_length : kDefaultPasswordLength) {} On 2012/06/01 01:09:55, Ilya Sherman wrote: > nit: Please decompose this logic into a tiny helper function, so that the > initializer list isn't so complex-looking. Done. http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator.cc:73: // To enhance the strenght of the password, the upper case letter will be On 2012/06/01 01:09:55, Ilya Sherman wrote: > nit: "strenght" -> "strength" Done. http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator.cc:77: int start = base::RandInt(0,3); On 2012/06/01 01:09:55, Ilya Sherman wrote: > Please use std::random_shuffle [1] instead. > [1] http://www.cplusplus.com/reference/algorithm/random_shuffle/ Good to know! Done. http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator.cc:92: ret.push_back(kOtherSymbols[base::RandInt(0, 31)]); On 2012/06/01 01:09:55, Ilya Sherman wrote: > 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 Done. http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator.cc:98: delete positions; On 2012/06/01 01:09:55, Ilya Sherman wrote: > Chromium code should almost never need explicit "delete" statements. Please use > a scoped_ptr... or better yet, an std::vector Done. http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/pa... File chrome/browser/autofill/password_generator.h (right): http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator.h:13: const unsigned int kDefaultPasswordLength = 12; On 2012/06/01 01:09:55, Ilya Sherman wrote: > This should still be a member of the PasswordGenerator class, with private or > protected visibility. 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. http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator.h:23: PasswordGenerator(); On 2012/06/01 01:09:55, Ilya Sherman wrote: > nit: Is this constructor still needed? Prefer to keep it in case some application just want to use the default length (e.g. unit tests) http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator.h:24: explicit PasswordGenerator(int max_length); On 2012/06/01 01:09:55, Ilya Sherman wrote: > nit: Please comment on the meaning of |max_length|, i.e. it is used as a hint > for the generated password's length. Done. http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator.h:29: // include whitespace characters. On 2012/06/01 01:09:55, Ilya Sherman wrote: > nit: Perhaps: "Each character is guaranteed to be a non-whitespace printable > ASCII character." Done. http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator.h:36: // the default length of 12. On 2012/06/01 01:09:55, Ilya Sherman wrote: > 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? Done. http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator.h:41: int password_length_; On 2012/06/01 01:09:55, Ilya Sherman wrote: > nit: const size_t? Won't this make comparisons more complex since all other numbers are int? http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/pa... File chrome/browser/autofill/password_generator_unittest.cc (right): http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator_unittest.cc:13: TEST(PasswordGeneratorTest, TestPasswordLength) { On 2012/06/01 01:09:55, Ilya Sherman wrote: > nit: "TestPasswordLength" -> "PasswordLength" (and likewise for the other tests > in this file) Done. http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator_unittest.cc:16: EXPECT_EQ(password.size(), 10u); On 2012/06/01 01:09:55, Ilya Sherman wrote: > nit: Please add a blank line after this line. Done. http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator_unittest.cc:19: EXPECT_EQ(password.size(), kDefaultPasswordLength); On 2012/06/01 01:09:55, Ilya Sherman wrote: > nit: Please add a blank line after this line. Done.
https://chromiumcodereview.appspot.com/10458018/diff/11001/chrome/browser/aut... File chrome/browser/autofill/password_generator.h (right): https://chromiumcodereview.appspot.com/10458018/diff/11001/chrome/browser/aut... chrome/browser/autofill/password_generator.h:13: const unsigned int kDefaultPasswordLength = 12; On 2012/06/01 19:01:32, zysxqn wrote: > On 2012/06/01 01:09:55, Ilya Sherman wrote: > > This should still be a member of the PasswordGenerator class, with private or > > protected visibility. > > 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. 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... https://chromiumcodereview.appspot.com/10458018/diff/11001/chrome/browser/aut... chrome/browser/autofill/password_generator.h:23: PasswordGenerator(); On 2012/06/01 19:01:32, zysxqn wrote: > On 2012/06/01 01:09:55, Ilya Sherman wrote: > > nit: Is this constructor still needed? > > Prefer to keep it in case some application just want to use the default length > (e.g. unit tests) 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. https://chromiumcodereview.appspot.com/10458018/diff/11001/chrome/browser/aut... chrome/browser/autofill/password_generator.h:41: int password_length_; On 2012/06/01 19:01:32, zysxqn wrote: > On 2012/06/01 01:09:55, Ilya Sherman wrote: > > nit: const size_t? > > Won't this make comparisons more complex since all other numbers are int? The other numbers should probably also all be unsigned. In any case, at a minimum, this should be const. https://chromiumcodereview.appspot.com/10458018/diff/15001/chrome/browser/aut... File chrome/browser/autofill/password_generator.cc (right): https://chromiumcodereview.appspot.com/10458018/diff/15001/chrome/browser/aut... chrome/browser/autofill/password_generator.cc:37: void RandomSelection(int num_select, nit: Perhaps "GetRandomSelection", "num_to_select"? https://chromiumcodereview.appspot.com/10458018/diff/15001/chrome/browser/aut... chrome/browser/autofill/password_generator.cc:70: max_length : kDefaultPasswordLength; 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.) https://chromiumcodereview.appspot.com/10458018/diff/15001/chrome/browser/aut... chrome/browser/autofill/password_generator.cc:84: random_shuffle(positions.begin(), positions.end()); nit: Shouldn't this be std::random_shuffle? https://chromiumcodereview.appspot.com/10458018/diff/15001/chrome/browser/aut... chrome/browser/autofill/password_generator.cc:88: if (i == positions.at(0)) { Please don't use std::vector::at() -- it can throw an exception, and the Chromium style guide disallows exceptions. https://chromiumcodereview.appspot.com/10458018/diff/15001/chrome/browser/aut... File chrome/browser/autofill/password_generator.h (right): https://chromiumcodereview.appspot.com/10458018/diff/15001/chrome/browser/aut... chrome/browser/autofill/password_generator.h:42: int SetLength(int max_length); nit: Please tuck this into the anonymous namespace in the implementation file, and name it something more like "GetLengthBasedOnHint()" (a terser name might be better, if you can think of a good one).
http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_manager.cc (right): http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_manager.cc:729: new autofill::PasswordGenerator(max_length)); On 2012/06/01 19:01:32, zysxqn wrote: > On 2012/06/01 01:09:55, Ilya Sherman wrote: > > This will be deleted immediately at the end of the function, which is probably > > not what you wanted, right? If that's not a problem, I would skip the > > heap-allocation and just declare the PasswordGenerator on the stack. > > I don't think it's a problem here. Only when user click the generation icon on > the password form again do we need to re-show the bubble and re-generate the > password, and this will be done by create a new bubble and a new password > generator. > > Changed to stack allocation. I don't think that either of these solutions is really what you want. The only reason that this happens to work right now is that we are generating the password in the constructor. When we add the button to regenerate passwords this is going to segfault. Either you shouldn't store the PasswordGenerator as a member variable, or you should heap allocate here and grab with a scoped_ptr in the bubbles. I prefer the latter since I think that we are going to want the ability to regenerate passwords eventually.
http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_manager.cc (right): http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_manager.cc:729: new autofill::PasswordGenerator(max_length)); On 2012/06/01 20:02:02, Garrett Casto wrote: > On 2012/06/01 19:01:32, zysxqn wrote: > > On 2012/06/01 01:09:55, Ilya Sherman wrote: > > > This will be deleted immediately at the end of the function, which is > probably > > > not what you wanted, right? If that's not a problem, I would skip the > > > heap-allocation and just declare the PasswordGenerator on the stack. > > > > I don't think it's a problem here. Only when user click the generation icon on > > the password form again do we need to re-show the bubble and re-generate the > > password, and this will be done by create a new bubble and a new password > > generator. > > > > Changed to stack allocation. > > I don't think that either of these solutions is really what you want. The only > reason that this happens to work right now is that we are generating the > password in the constructor. When we add the button to regenerate passwords this > is going to segfault. Either you shouldn't store the PasswordGenerator as a > member variable, or you should heap allocate here and grab with a scoped_ptr in > the bubbles. I prefer the latter since I think that we are going to want the > ability to regenerate passwords eventually. Done. http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/pa... File chrome/browser/autofill/password_generator.h (right): http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator.h:13: const unsigned int kDefaultPasswordLength = 12; On 2012/06/01 19:32:10, Ilya Sherman wrote: > On 2012/06/01 19:01:32, zysxqn wrote: > > On 2012/06/01 01:09:55, Ilya Sherman wrote: > > > This should still be a member of the PasswordGenerator class, with private > or > > > protected visibility. > > > > 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. > > 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... Done. http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator.h:23: PasswordGenerator(); On 2012/06/01 19:32:10, Ilya Sherman wrote: > On 2012/06/01 19:01:32, zysxqn wrote: > > On 2012/06/01 01:09:55, Ilya Sherman wrote: > > > nit: Is this constructor still needed? > > > > Prefer to keep it in case some application just want to use the default length > > (e.g. unit tests) > > 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. I'm fine with either way so I remove it as you suggested. http://codereview.chromium.org/10458018/diff/11001/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator.h:41: int password_length_; On 2012/06/01 19:32:10, Ilya Sherman wrote: > On 2012/06/01 19:01:32, zysxqn wrote: > > On 2012/06/01 01:09:55, Ilya Sherman wrote: > > > nit: const size_t? > > > > Won't this make comparisons more complex since all other numbers are int? > > The other numbers should probably also all be unsigned. In any case, at a > minimum, this should be const. Changed all variables related to password length to be size_t. http://codereview.chromium.org/10458018/diff/15001/chrome/browser/autofill/pa... File chrome/browser/autofill/password_generator.cc (right): http://codereview.chromium.org/10458018/diff/15001/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator.cc:37: void RandomSelection(int num_select, On 2012/06/01 19:32:10, Ilya Sherman wrote: > nit: Perhaps "GetRandomSelection", "num_to_select"? Done. http://codereview.chromium.org/10458018/diff/15001/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator.cc:70: max_length : kDefaultPasswordLength; On 2012/06/01 19:32:10, Ilya Sherman wrote: > 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.) Done. http://codereview.chromium.org/10458018/diff/15001/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator.cc:84: random_shuffle(positions.begin(), positions.end()); On 2012/06/01 19:32:10, Ilya Sherman wrote: > nit: Shouldn't this be std::random_shuffle? Done. http://codereview.chromium.org/10458018/diff/15001/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator.cc:88: if (i == positions.at(0)) { On 2012/06/01 19:32:10, Ilya Sherman wrote: > Please don't use std::vector::at() -- it can throw an exception, and the > Chromium style guide disallows exceptions. Done. http://codereview.chromium.org/10458018/diff/15001/chrome/browser/autofill/pa... File chrome/browser/autofill/password_generator.h (right): http://codereview.chromium.org/10458018/diff/15001/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator.h:42: int SetLength(int max_length); On 2012/06/01 19:32:10, Ilya Sherman wrote: > nit: Please tuck this into the anonymous namespace in the implementation file, > and name it something more like "GetLengthBasedOnHint()" (a terser name might be > better, if you can think of a good one). Done.
http://codereview.chromium.org/10458018/diff/26002/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_manager.cc (right): http://codereview.chromium.org/10458018/diff/26002/chrome/browser/autofill/au... chrome/browser/autofill/autofill_manager.cc:729: new autofill::PasswordGenerator(max_length)); This will be freed as soon as the method call returns. If you want this to outlive the method call, you'll need to store it as a member variable in the AutofillManager class. Alternately, if you mean to transfer the ownership, you should call password_generator.release() below, and document the transfer of ownership very explicitly in comments for all of the affected interfaces. http://codereview.chromium.org/10458018/diff/26002/chrome/browser/autofill/pa... File chrome/browser/autofill/password_generator.h (right): http://codereview.chromium.org/10458018/diff/26002/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator.h:36: const size_t kDefaultPasswordLength_; nit: This variable should be static, and the name should not include a trailing underscore. http://codereview.chromium.org/10458018/diff/26002/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator.h:37: friend class PasswordGeneratorTest; nit: Please use the FRIEND_TEST_ALL_PREFIXES macro that I mentioned. No need to create a test class just for friending. http://codereview.chromium.org/10458018/diff/26002/chrome/browser/ui/browser_... File chrome/browser/ui/browser_window.h (right): http://codereview.chromium.org/10458018/diff/26002/chrome/browser/ui/browser_... chrome/browser/ui/browser_window.h:369: autofill::PasswordGenerator* password_generator) {} This default implementation should be careful not to leak the |password_generator| if you decide on a transfer-of-ownership semantics.
http://codereview.chromium.org/10458018/diff/26002/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_manager.cc (right): http://codereview.chromium.org/10458018/diff/26002/chrome/browser/autofill/au... chrome/browser/autofill/autofill_manager.cc:729: new autofill::PasswordGenerator(max_length)); On 2012/06/04 21:17:52, Ilya Sherman wrote: > This will be freed as soon as the method call returns. If you want this to > outlive the method call, you'll need to store it as a member variable in the > AutofillManager class. > > Alternately, if you mean to transfer the ownership, you should call > password_generator.release() below, and document the transfer of ownership very > explicitly in comments for all of the affected interfaces. Originally I did mean to transfer the ownership -- limiting the lifetime of password_generator to be the same as the UI is sufficient for now. After a second thought, I decided to make it a member variable in the AutofillManager class so it will live longer - it could be useful later when we need some data about the generated passwords. http://codereview.chromium.org/10458018/diff/26002/chrome/browser/autofill/pa... File chrome/browser/autofill/password_generator.h (right): http://codereview.chromium.org/10458018/diff/26002/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator.h:36: const size_t kDefaultPasswordLength_; On 2012/06/04 21:17:52, Ilya Sherman wrote: > nit: This variable should be static, and the name should not include a trailing > underscore. Done. http://codereview.chromium.org/10458018/diff/26002/chrome/browser/autofill/pa... chrome/browser/autofill/password_generator.h:37: friend class PasswordGeneratorTest; On 2012/06/04 21:17:52, Ilya Sherman wrote: > nit: Please use the FRIEND_TEST_ALL_PREFIXES macro that I mentioned. No need to > create a test class just for friending. Done. http://codereview.chromium.org/10458018/diff/26002/chrome/browser/ui/browser_... File chrome/browser/ui/browser_window.h (right): http://codereview.chromium.org/10458018/diff/26002/chrome/browser/ui/browser_... chrome/browser/ui/browser_window.h:369: autofill::PasswordGenerator* password_generator) {} On 2012/06/04 21:17:52, Ilya Sherman wrote: > This default implementation should be careful not to leak the > |password_generator| if you decide on a transfer-of-ownership semantics. NA now.
http://codereview.chromium.org/10458018/diff/39002/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_manager.h (right): http://codereview.chromium.org/10458018/diff/39002/chrome/browser/autofill/au... chrome/browser/autofill/autofill_manager.h:365: // To be passed to the Password Generation UI to generate the password. nit: "password generation" probably shouldn't be capitalized, as it's not a feature name per sé. http://codereview.chromium.org/10458018/diff/39002/chrome/browser/ui/gtk/pass... File chrome/browser/ui/gtk/password_generation_bubble_gtk.h (right): http://codereview.chromium.org/10458018/diff/39002/chrome/browser/ui/gtk/pass... chrome/browser/ui/gtk/password_generation_bubble_gtk.h:54: scoped_ptr<autofill::PasswordGenerator> password_generator_; If the AutofillManager owns this object, this should be a weak reference rather than a scoped_ptr.
http://codereview.chromium.org/10458018/diff/39002/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_manager.cc (right): http://codereview.chromium.org/10458018/diff/39002/chrome/browser/autofill/au... chrome/browser/autofill/autofill_manager.cc:730: bounds, password_generator_.get()); Is this bubble guaranteed to be hidden prior to the next call to OnShowPasswordGenerationPopup()? That is, is it actually safe for the AutofillManager to free the old |password_generator_| as soon as a new bubble is requested, or is it possible that an old bubble still needs the existing one?
http://codereview.chromium.org/10458018/diff/39002/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_manager.cc (right): http://codereview.chromium.org/10458018/diff/39002/chrome/browser/autofill/au... chrome/browser/autofill/autofill_manager.cc:730: bounds, password_generator_.get()); On 2012/06/04 22:52:32, Ilya Sherman wrote: > Is this bubble guaranteed to be hidden prior to the next call to > OnShowPasswordGenerationPopup()? That is, is it actually safe for the > AutofillManager to free the old |password_generator_| as soon as a new bubble is > requested, or is it possible that an old bubble still needs the existing one? At the moment the bubble is closed as soon as it loses focus, so this is safe. http://codereview.chromium.org/10458018/diff/39002/chrome/browser/ui/gtk/pass... File chrome/browser/ui/gtk/password_generation_bubble_gtk.h (right): http://codereview.chromium.org/10458018/diff/39002/chrome/browser/ui/gtk/pass... chrome/browser/ui/gtk/password_generation_bubble_gtk.h:54: scoped_ptr<autofill::PasswordGenerator> password_generator_; On 2012/06/04 22:49:15, Ilya Sherman wrote: > If the AutofillManager owns this object, this should be a weak reference rather > than a scoped_ptr. Is a weak_ptr necessary, or does just a bare pointer suffice? I wouldn't think that the TabContentsWrapper could disappear while input is being processed. The overhead for the weak reference isn't that high, I'm just mostly curious.
http://codereview.chromium.org/10458018/diff/39002/chrome/browser/ui/gtk/pass... File chrome/browser/ui/gtk/password_generation_bubble_gtk.h (right): http://codereview.chromium.org/10458018/diff/39002/chrome/browser/ui/gtk/pass... chrome/browser/ui/gtk/password_generation_bubble_gtk.h:54: scoped_ptr<autofill::PasswordGenerator> password_generator_; On 2012/06/04 22:58:57, Garrett Casto wrote: > On 2012/06/04 22:49:15, Ilya Sherman wrote: > > If the AutofillManager owns this object, this should be a weak reference > rather > > than a scoped_ptr. > > Is a weak_ptr necessary, or does just a bare pointer suffice? I wouldn't think > that the TabContentsWrapper could disappear while input is being processed. The > overhead for the weak reference isn't that high, I'm just mostly curious. A bare pointer suffices. I didn't actually mean to conjure up thoughts of weak_ptr when I said "weak reference" -- a bare pointer is what I was thinking of.
http://codereview.chromium.org/10458018/diff/39002/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_manager.h (right): http://codereview.chromium.org/10458018/diff/39002/chrome/browser/autofill/au... chrome/browser/autofill/autofill_manager.h:365: // To be passed to the Password Generation UI to generate the password. On 2012/06/04 22:49:15, Ilya Sherman wrote: > nit: "password generation" probably shouldn't be capitalized, as it's not a > feature name per sé. Done. http://codereview.chromium.org/10458018/diff/39002/chrome/browser/ui/gtk/pass... File chrome/browser/ui/gtk/password_generation_bubble_gtk.h (right): http://codereview.chromium.org/10458018/diff/39002/chrome/browser/ui/gtk/pass... chrome/browser/ui/gtk/password_generation_bubble_gtk.h:54: scoped_ptr<autofill::PasswordGenerator> password_generator_; On 2012/06/04 23:08:22, Ilya Sherman wrote: > On 2012/06/04 22:58:57, Garrett Casto wrote: > > On 2012/06/04 22:49:15, Ilya Sherman wrote: > > > If the AutofillManager owns this object, this should be a weak reference > > rather > > > than a scoped_ptr. > > > > Is a weak_ptr necessary, or does just a bare pointer suffice? I wouldn't think > > that the TabContentsWrapper could disappear while input is being processed. > The > > overhead for the weak reference isn't that high, I'm just mostly curious. > > A bare pointer suffices. I didn't actually mean to conjure up thoughts of > weak_ptr when I said "weak reference" -- a bare pointer is what I was thinking > of. Done.
LGTM, thanks.
http://codereview.chromium.org/10458018/diff/33003/chrome/browser/ui/gtk/pass... File chrome/browser/ui/gtk/password_generation_bubble_gtk.h (right): http://codereview.chromium.org/10458018/diff/33003/chrome/browser/ui/gtk/pass... chrome/browser/ui/gtk/password_generation_bubble_gtk.h:11: #include "base/basictypes.h" What is this needed for? http://codereview.chromium.org/10458018/diff/33003/chrome/browser/ui/views/pa... File chrome/browser/ui/views/password_generation_bubble_view.h (right): http://codereview.chromium.org/10458018/diff/33003/chrome/browser/ui/views/pa... chrome/browser/ui/views/password_generation_bubble_view.h:9: #include "base/basictypes.h" Same here.
http://codereview.chromium.org/10458018/diff/33003/chrome/browser/ui/gtk/pass... File chrome/browser/ui/gtk/password_generation_bubble_gtk.h (right): http://codereview.chromium.org/10458018/diff/33003/chrome/browser/ui/gtk/pass... chrome/browser/ui/gtk/password_generation_bubble_gtk.h:11: #include "base/basictypes.h" On 2012/06/04 23:50:52, Garrett Casto wrote: > What is this needed for? For DISALLOW_COPY_AND_ASSIGN. Previously it can compile because it's indirectly included from password_generator.h which is moved now. Also I think the code style requires to include directly. http://codereview.chromium.org/10458018/diff/33003/chrome/browser/ui/views/pa... File chrome/browser/ui/views/password_generation_bubble_view.h (right): http://codereview.chromium.org/10458018/diff/33003/chrome/browser/ui/views/pa... chrome/browser/ui/views/password_generation_bubble_view.h:9: #include "base/basictypes.h" On 2012/06/04 23:50:52, Garrett Casto wrote: > Same here. Also for DISALLOW_COPY_AND_ASSIGN.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zysxqn@google.com/10458018/33003
Failed to apply patch for chrome/browser/autofill/autofill_manager.cc: While running patch -p1 --forward --force; patching file chrome/browser/autofill/autofill_manager.cc Hunk #2 FAILED at 718. 1 out of 2 hunks FAILED -- saving rejects to file chrome/browser/autofill/autofill_manager.cc.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zysxqn@google.com/10458018/33003
Failed to apply patch for chrome/browser/autofill/autofill_manager.cc: While running patch -p1 --forward --force; patching file chrome/browser/autofill/autofill_manager.cc Hunk #2 FAILED at 718. 1 out of 2 hunks FAILED -- saving rejects to file chrome/browser/autofill/autofill_manager.cc.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zysxqn@google.com/10458018/33003
Failed to apply patch for chrome/browser/autofill/autofill_manager.cc: While running patch -p1 --forward --force; patching file chrome/browser/autofill/autofill_manager.cc Hunk #2 FAILED at 718. 1 out of 2 hunks FAILED -- saving rejects to file chrome/browser/autofill/autofill_manager.cc.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zysxqn@google.com/10458018/54001
Presubmit check for 10458018-54001 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome/browser/ui/gtk
chrome/browser/ui
Presubmit checks took 5.5s to calculate.
+ben, ben, can you take a look at and approve the UI part? Thanks, Yue
Looks like Ben is OOO this week. +Peter and Scott for the UI part. Thanks, Yue
LGTM http://codereview.chromium.org/10458018/diff/54001/chrome/browser/ui/browser_... File chrome/browser/ui/browser_window.h (right): http://codereview.chromium.org/10458018/diff/54001/chrome/browser/ui/browser_... chrome/browser/ui/browser_window.h:391: autofill::PasswordGenerator* password_generator, Document ownership of |password_generator| http://codereview.chromium.org/10458018/diff/54001/chrome/browser/ui/views/pa... File chrome/browser/ui/views/password_generation_bubble_view.h (right): http://codereview.chromium.org/10458018/diff/54001/chrome/browser/ui/views/pa... chrome/browser/ui/views/password_generation_bubble_view.h:79: autofill::PasswordGenerator* password_generator_; Document ownership.
Submitting... http://codereview.chromium.org/10458018/diff/54001/chrome/browser/ui/browser_... File chrome/browser/ui/browser_window.h (right): http://codereview.chromium.org/10458018/diff/54001/chrome/browser/ui/browser_... chrome/browser/ui/browser_window.h:391: autofill::PasswordGenerator* password_generator, On 2012/06/06 04:04:07, sky wrote: > Document ownership of |password_generator| Done. http://codereview.chromium.org/10458018/diff/54001/chrome/browser/ui/views/pa... File chrome/browser/ui/views/password_generation_bubble_view.h (right): http://codereview.chromium.org/10458018/diff/54001/chrome/browser/ui/views/pa... chrome/browser/ui/views/password_generation_bubble_view.h:79: autofill::PasswordGenerator* password_generator_; On 2012/06/06 04:04:07, sky wrote: > Document ownership. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zysxqn@google.com/10458018/49006
Try job failure for 10458018-49006 (retry) on linux_chromeos for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zysxqn@google.com/10458018/51004
Change committed as 140812 |
