Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
548953002:
[Password Manager] Modified to support saving passwords on forms without username fields. (Closed)
|
Created:
6 years, 3 months ago by Pritam Nikam Modified:
6 years, 2 months ago CC:
chromium-reviews, gcasto+watchlist_chromium.org, mkwst+watchlist_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Description[Password Manager] Modified to support saving passwords on forms without user-name fields.
With current implementation forms lacking user-name fields are treated as invalid password forms and browser does not save passwords in such cases.
With this patch password manager saves passwords on forms without user-name fields as well.
BUG=406343
TBR=gcasto@chromium.org,vabr@chromium.org
Committed: https://crrev.com/77d3ffc71361592df3fdf9a5377045f088bcaeac
Cr-Commit-Position: refs/heads/master@{#296943}
Patch Set 1 #Patch Set 2 : Added unit-tests and fixed lint errors. #
Total comments: 24
Patch Set 3 : Incorporated review comments. #Patch Set 4 : Added a dummy username field to autofill password only form. #Patch Set 5 : Incorporated review comments. #
Total comments: 30
Patch Set 6 : #
Total comments: 7
Patch Set 7 : Incorporated review changes and code refactoring. #
Total comments: 4
Patch Set 8 : Reverted refactoring changes. #
Total comments: 2
Patch Set 9 : Incorporated input comments. #
Total comments: 4
Patch Set 10 : Fixed breakages. #
Total comments: 1
Messages
Total messages: 37 (6 generated)
pritam.nikam@samsung.com changed reviewers: + vabr@chromium.org
On 2014/09/09 07:04:20, Pritam Nikam wrote: > mailto:pritam.nikam@samsung.com changed reviewers: > + mailto:vabr@chromium.org Hi Vaclav! As you have suggested, I've provided simplistic implementation (with UX). PTAL! However, with current patch, I'm getting a problem while autofilling password field on next passowrd-only-form reload. On debugging, I could see API responsible to autofill input fields on load is [1] PasswordAutofillManager::OnAddPasswordFormMapping(), and this is not getting called for "passowrd-only-form" case. Reason seems Renderer process fail to send IPC message AutofillHostMsg_AddPasswordFormMapping (from [2]) as [3] fails to retrieve stored forms. [1] https://code.google.com/p/chromium/codesearch#chromium/src/components/passwor... [2] https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... [3] https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... Any pointers in this regard will be a great help. Thanks!
* Simplistic implementation (without UX)
Hi Pritam Nikam, Thanks for your work on this, and sorry for the delay (I was on holiday last week). I have a couple of comments below, please note that there has been a misunderstanding with "no UI" meaning actually "no new UI", as explained below. Regarding the problem that saved passwords are not autofilled -- it could be that FindFormInputElements of password_autofill_agent.cc is trying to find the input field for the empty username or something. You should be able to follow the real cause of failure up when debugging. Please make sure to add a test that checks that autofilling works OK. That test should be reproducing the failure you are seeing now, and obviously pass once you fix it. :) Thanks, Vaclav https://codereview.chromium.org/548953002/diff/20001/chrome/browser/password_... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/548953002/diff/20001/chrome/browser/password_... chrome/browser/password_manager/password_manager_browsertest.cc:1142: EXPECT_FALSE(prompt_observer->IsShowingPrompt()); This might be a misunderstanding caused by "doesn't involve UI" of http://crbug.com/406343#c6. Similarly to the case of a normal password form with input element which is left empty, also here the password prompt should be shown. "Doesn't involve UI" should have rather been phrased "doesn't involve new UI". https://codereview.chromium.org/548953002/diff/20001/components/password_mana... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/548953002/diff/20001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:8: #include <set> Is this related to your other changes in the CL? If not, don't introduce the header here. If you feel it should be included, please do a separate clean-up CL for it. https://codereview.chromium.org/548953002/diff/20001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:99: } Again, this does not seem related to the rest of the CL. Although the semicolon is clearly a typo, it should be removed in a separate clean-up CL, not in this one. https://codereview.chromium.org/548953002/diff/20001/components/password_mana... File components/password_manager/core/browser/password_form_manager.h (right): https://codereview.chromium.org/548953002/diff/20001/components/password_mana... components/password_manager/core/browser/password_form_manager.h:123: // TODO(vabr): Make this private once we switch to the new UI. Why do you change this comment (and the one below)? Are they related to your change-list? While you are right that the style requires an owner of a TODO to be specified, changing unrelated lines in the code makes it generally harder to track down changes (you have to go through several hops in the history), and blows up the CL. Please do not do that. https://codereview.chromium.org/548953002/diff/20001/components/password_mana... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/548953002/diff/20001/components/password_mana... components/password_manager/core/browser/password_manager.cc:7: #include <string> Another change which should have a separate clean-up CL, but should not be done here. https://codereview.chromium.org/548953002/diff/20001/components/password_mana... components/password_manager/core/browser/password_manager.cc:407: provisional_save_manager_->HasValidLogin(); As explained in the browsertest above, there should still be the prompt for forms without usernames. https://codereview.chromium.org/548953002/diff/20001/components/password_mana... File components/password_manager/core/browser/password_manager_unittest.cc (right): https://codereview.chromium.org/548953002/diff/20001/components/password_mana... components/password_manager/core/browser/password_manager_unittest.cc:5: #include <string> Also here -- you don't introduce std::string with this change, so please don't introduce the #include either. If you think this should be done, file a separate clean-up CL. https://codereview.chromium.org/548953002/diff/20001/components/password_mana... components/password_manager/core/browser/password_manager_unittest.cc:178: PasswordForm MakeSimpleFormWithOnlyPasswordField() { nit: To improve readability, consider changing the body to these 4 lines: PasswordForm form(MakeSimpleForm()); form.username_element.clear(); form.username_value.clear(); return form; (See also MakeFormWithOnlyNewPasswordField().) https://codereview.chromium.org/548953002/diff/20001/components/password_mana... components/password_manager/core/browser/password_manager_unittest.cc:775: // Test that observing a newly submitted form shows the save password bar. nits: (1) Do mention what is special about the form (no username element). (2) "save password bar" -> "save password prompt" (because the UI is switching from bars to bubbles, and 'prompt' is more generic) https://codereview.chromium.org/548953002/diff/20001/components/password_mana... components/password_manager/core/browser/password_manager_unittest.cc:786: // Simulate the user generating the password and submitting the form. Don't mix password generation in -- we only allow to save passwords on forms without usernames, not generate them (yet). https://codereview.chromium.org/548953002/diff/20001/components/password_mana... components/password_manager/core/browser/password_manager_unittest.cc:790: // The user should not be presented with an infobar as they have already given This whole comment looks wrong: the save password prompt (not necessarily infobar) should be shown, there was no generated password and no prior consent, and no post-saving notification takes place. https://codereview.chromium.org/548953002/diff/20001/components/password_mana... components/password_manager/core/browser/password_manager_unittest.cc:794: EXPECT_CALL(client_, PromptUserToSavePasswordPtr(_)).Times(Exactly(0)); Please update the expectations following the clarifications above.
Thanks Vaclav for review. I've incorporated all review comments with patch set #3. PTAL! Moreover, on problem with autofilling password form I've debugged a bit. Below are few findings: 1. Current implementation has very tight binding for username-password fields for password form - ref [A]. 2. For password form auto-filling browser always assumes 2 field to get field - Ref [B] and call-stack [D]. With no username field |username_field.name| would be empty string. 3. But cases where |field_name| is an empty string renderer assumes that it's an invalid form and abort autofilling it. Ref [C]. I've tried providing fake-username field while requesting for autofilling password, but that crashed in some other places on renderer. I'm planning to decouple username-password form all possible places. Is that sound okay to you? Thanks! [A]. https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... [B]. https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... [C]. https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... [D]. Breakpoint 2, autofill::InitPasswordFormFillData (form_on_page=..., matches=std::__debug::map with 1 elements, preferred_match=0x1614f207fc20, wait_for_username_before_autofill=false, enable_other_possible_usernames=false, result=0x1614f217c620) at ../../components/autofill/core/common/password_form_fill_data.cc:42 42 FormFieldData username_field; (gdb) n 43 username_field.name = form_on_page.username_element; (gdb) p form_on_page.username_element $5 = "" https://codereview.chromium.org/548953002/diff/20001/chrome/browser/password_... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/548953002/diff/20001/chrome/browser/password_... chrome/browser/password_manager/password_manager_browsertest.cc:1142: EXPECT_FALSE(prompt_observer->IsShowingPrompt()); On 2014/09/15 13:01:45, vabr (Chromium) wrote: > This might be a misunderstanding caused by "doesn't involve UI" of > http://crbug.com/406343#c6. Similarly to the case of a normal password form with > input element which is left empty, also here the password prompt should be > shown. "Doesn't involve UI" should have rather been phrased "doesn't involve new > UI". Done. https://codereview.chromium.org/548953002/diff/20001/components/password_mana... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/548953002/diff/20001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:8: #include <set> On 2014/09/15 13:01:45, vabr (Chromium) wrote: > Is this related to your other changes in the CL? If not, don't introduce the > header here. If you feel it should be included, please do a separate clean-up CL > for it. Done. I've changed this to resolve Lint error. Notified pre- git cl upload script execution. I'll raise a separate review to do cleanup for the same. https://codereview.chromium.org/548953002/diff/20001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:99: } On 2014/09/15 13:01:45, vabr (Chromium) wrote: > Again, this does not seem related to the rest of the CL. Although the semicolon > is clearly a typo, it should be removed in a separate clean-up CL, not in this > one. Done. Ditto! https://codereview.chromium.org/548953002/diff/20001/components/password_mana... File components/password_manager/core/browser/password_form_manager.h (right): https://codereview.chromium.org/548953002/diff/20001/components/password_mana... components/password_manager/core/browser/password_form_manager.h:123: // TODO(vabr): Make this private once we switch to the new UI. On 2014/09/15 13:01:45, vabr (Chromium) wrote: > Why do you change this comment (and the one below)? Are they related to your > change-list? > > While you are right that the style requires an owner of a TODO to be specified, > changing unrelated lines in the code makes it generally harder to track down > changes (you have to go through several hops in the history), and blows up the > CL. Please do not do that. Done. I've changed this to resolve Lint error. Notified pre- git cl upload script execution. https://codereview.chromium.org/548953002/diff/20001/components/password_mana... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/548953002/diff/20001/components/password_mana... components/password_manager/core/browser/password_manager.cc:7: #include <string> On 2014/09/15 13:01:45, vabr (Chromium) wrote: > Another change which should have a separate clean-up CL, but should not be done > here. Done. https://codereview.chromium.org/548953002/diff/20001/components/password_mana... components/password_manager/core/browser/password_manager.cc:407: provisional_save_manager_->HasValidLogin(); On 2014/09/15 13:01:45, vabr (Chromium) wrote: > As explained in the browsertest above, there should still be the prompt for > forms without usernames. Done. https://codereview.chromium.org/548953002/diff/20001/components/password_mana... File components/password_manager/core/browser/password_manager_unittest.cc (right): https://codereview.chromium.org/548953002/diff/20001/components/password_mana... components/password_manager/core/browser/password_manager_unittest.cc:5: #include <string> On 2014/09/15 13:01:45, vabr (Chromium) wrote: > Also here -- you don't introduce std::string with this change, so please don't > introduce the #include either. If you think this should be done, file a separate > clean-up CL. Done. https://codereview.chromium.org/548953002/diff/20001/components/password_mana... components/password_manager/core/browser/password_manager_unittest.cc:178: PasswordForm MakeSimpleFormWithOnlyPasswordField() { On 2014/09/15 13:01:46, vabr (Chromium) wrote: > nit: To improve readability, consider changing the body to these 4 lines: > PasswordForm form(MakeSimpleForm()); > form.username_element.clear(); > form.username_value.clear(); > return form; > > (See also MakeFormWithOnlyNewPasswordField().) Done. https://codereview.chromium.org/548953002/diff/20001/components/password_mana... components/password_manager/core/browser/password_manager_unittest.cc:775: // Test that observing a newly submitted form shows the save password bar. On 2014/09/15 13:01:45, vabr (Chromium) wrote: > nits: > (1) Do mention what is special about the form (no username element). > (2) "save password bar" -> "save password prompt" (because the UI is switching > from bars to bubbles, and 'prompt' is more generic) Done. https://codereview.chromium.org/548953002/diff/20001/components/password_mana... components/password_manager/core/browser/password_manager_unittest.cc:786: // Simulate the user generating the password and submitting the form. On 2014/09/15 13:01:45, vabr (Chromium) wrote: > Don't mix password generation in -- we only allow to save passwords on forms > without usernames, not generate them (yet). Done. https://codereview.chromium.org/548953002/diff/20001/components/password_mana... components/password_manager/core/browser/password_manager_unittest.cc:790: // The user should not be presented with an infobar as they have already given On 2014/09/15 13:01:46, vabr (Chromium) wrote: > This whole comment looks wrong: the save password prompt (not necessarily > infobar) should be shown, there was no generated password and no prior consent, > and no post-saving notification takes place. Done. https://codereview.chromium.org/548953002/diff/20001/components/password_mana... components/password_manager/core/browser/password_manager_unittest.cc:794: EXPECT_CALL(client_, PromptUserToSavePasswordPtr(_)).Times(Exactly(0)); On 2014/09/15 13:01:46, vabr (Chromium) wrote: > Please update the expectations following the clarifications above. Done.
Hi Pritam Nikam, Thanks for the debugging update. Teaching PasswordAutofillAgent and related code, that a username element is not required for a password form to be valid sounds OK to me. In particular, I prefer that to sending a dummy username element name. Interesting about what you saw with the presubmit lint errors. Great that you plan to do a separate clean-up CL, that's the best way to deal with it. Now that I know it was the presubmit bugging you, I would be also fine with including the clean-ups here as a workaround, but doing it in a separate CL is still much preferable. The patch so far looks good to me, but I'd like to have a look at the fix for the autofilling issue, and a corresponding browsertest, before you land this. Thanks! Vaclav
Hi Vaclav! Thanks for your inputs. I've submitted a patch (#4) with your suggestions by sending a dummy username input field. Solution seems working for me on my local build. PTAL! I've added a browser-test for the same, however I'm finding it difficult to get auto-suggestions here & testing is failing as of now. Any pointer in this regard will help. Thanks!
On 2014/09/23 13:18:08, Pritam Nikam wrote: > Hi Vaclav! > > Thanks for your inputs. I've submitted a patch (#4) with your suggestions by > sending > a dummy username input field. Solution seems working for me on my local build. > PTAL! > > I've added a browser-test for the same, however I'm finding it difficult to get > auto-suggestions here & testing is failing as of now. Any pointer in this regard > will > help. > > Thanks! Hi Pritam Nikam, There might be a misunderstanding: I wrote that I would prefer "teaching PasswordAutofillAgent and related code, that a username element is not required for a password form to be valid" to sending a dummy username. In other words, sending a dummy username does not sound good, fixing the code to regard forms as valid even without a username does. Cheers, Vaclav
On 2014/09/23 13:23:00, vabr (Chromium) wrote: > On 2014/09/23 13:18:08, Pritam Nikam wrote: > > Hi Vaclav! > > > > Thanks for your inputs. I've submitted a patch (#4) with your suggestions by > > sending > > a dummy username input field. Solution seems working for me on my local build. > > PTAL! > > > > I've added a browser-test for the same, however I'm finding it difficult to > get > > auto-suggestions here & testing is failing as of now. Any pointer in this > regard > > will > > help. > > > > Thanks! > > Hi Pritam Nikam, > > There might be a misunderstanding: I wrote that I would prefer "teaching > PasswordAutofillAgent and related > code, that a username element is not required for a password form to be valid" > to sending a dummy username. In other words, sending a dummy username does not > sound good, fixing the code to regard forms as valid even without a username > does. > > Cheers, > Vaclav Ah.. Sorry! I will work in that direction. Thanks for quick reply.
On 2014/09/23 13:25:07, Pritam Nikam wrote: > On 2014/09/23 13:23:00, vabr (Chromium) wrote: > > On 2014/09/23 13:18:08, Pritam Nikam wrote: > > > Hi Vaclav! > > > > > > Thanks for your inputs. I've submitted a patch (#4) with your suggestions by > > > sending > > > a dummy username input field. Solution seems working for me on my local > build. > > > PTAL! > > > > > > I've added a browser-test for the same, however I'm finding it difficult to > > get > > > auto-suggestions here & testing is failing as of now. Any pointer in this > > regard > > > will > > > help. > > > > > > Thanks! > > > > Hi Pritam Nikam, > > > > There might be a misunderstanding: I wrote that I would prefer "teaching > > PasswordAutofillAgent and related > > code, that a username element is not required for a password form to be valid" > > to sending a dummy username. In other words, sending a dummy username does not > > sound good, fixing the code to regard forms as valid even without a username > > does. > > > > Cheers, > > Vaclav > > Ah.. Sorry! > I will work in that direction. Thanks! > > Thanks for quick reply.
Hi Vaclav, I've added a patch for review by adding handling for cases where password form without username field. PTAL. Thanks!
Thanks, Pritam Nikam. I added a couple of comments. These are mostly nits, but let's do one more iteration over them, just in case addressing them revealed more problems. Cheers, Vaclav https://codereview.chromium.org/548953002/diff/80001/chrome/browser/password_... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/548953002/diff/80001/chrome/browser/password_... chrome/browser/password_manager/password_manager_browsertest.cc:1242: // The redirection page now redirects via Javascript. It is not clear what is "the redirection page", and why this line is needed. Please explain in the comment. https://codereview.chromium.org/548953002/diff/80001/chrome/test/data/passwor... File chrome/test/data/password/form_with_only_password_field.html (right): https://codereview.chromium.org/548953002/diff/80001/chrome/test/data/passwor... chrome/test/data/password/form_with_only_password_field.html:5: <label for="password">Password</label> nit: Although the name of the file suggest this, could you please also insert a <!-- comment --> stating that there is on purpose no username field. This is just to make sure somebody does not include one later by mistake. https://codereview.chromium.org/548953002/diff/80001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.cc (left): https://codereview.chromium.org/548953002/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:973: if (DoUsernamesMatch(fill_data.basic_data.fields[0].value, Suggestion: if you modify this check to also consider an empty |current_username| and an empty |fill_data.basic_data.fields[0].value| as a match, then you can leave most of the code below (and the line above) as it is. https://codereview.chromium.org/548953002/diff/80001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/548953002/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:74: // Only consider non-empty input fields for autofilling I'm confused by this comment -- http://crbug.com/406343 is about saving passwords on forms without a username, not about empty input fields. Also, the comment says "non-empty input field", whereas the code checks for the field's name (not field itself) being empty. https://codereview.chromium.org/548953002/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:228: static bool FormWithEmptyUsernameField(const PasswordFormFillData& fill_data) { Again -- the comment and the function name say "empty field", whereas the code tests the emptiness of the field's name. My suggestion is: rename the function to FillDataContainUsername (and negate the result to accommodate the change in semantics). You can leave out the explaining comment entirely, because all will be clear from the code. https://codereview.chromium.org/548953002/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:228: static bool FormWithEmptyUsernameField(const PasswordFormFillData& fill_data) { No need to include "static" -- the function is invisible outside of the .cc file because of being in the anonymous namespace already. https://codereview.chromium.org/548953002/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:818: // Check whether password form has username input field grammar nit: add articles -- the password form, a username input field https://codereview.chromium.org/548953002/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:819: // (http://crbug.com/406343). nit: I'm not sure it's necessary to include the reference to the bug. It's already clear that the code tests the existence of the username field, no more context (possibly found on the bug) is necessary to understand the code. https://codereview.chromium.org/548953002/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:820: bool is_password_only_form = FormWithEmptyUsernameField(form_data); (Note: if you change the function to FillDataContainUsername, don't forget to rename the bool appropriately, and stop negating it below.) https://codereview.chromium.org/548953002/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:821: if (!is_password_only_form) nit: Add braces, because the body of the if-clause spans more lines. https://codereview.chromium.org/548953002/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:841: if (!is_password_only_form && Why do you exclude the case of no-username passwords? You apparently add an entry for them in login_to_password_info_. If any two empty WebInputElements are equal, then the find() below should work. If not, then there is no point in adding forms without username element to the login_to_password_info_ map. Please find out what is the situation, and amend accordingly. https://codereview.chromium.org/548953002/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:855: FindFormAndFieldForFormControlElement( nit: missing braces https://codereview.chromium.org/548953002/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:957: (!is_password_only_form && !username_element.form().autoComplete())) nit: no need for the additional parentheses -- && is associative https://codereview.chromium.org/548953002/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:1109: // Password form may not have username input fields Suggested rephrasing to simplify the comment: // There may not be a username field, so get the frame from the password field. https://codereview.chromium.org/548953002/diff/80001/components/password_mana... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/548953002/diff/80001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:229: return (!observed_form_.password_element.empty() || nit: no need for the outer parentheses
Patchset #6 (id:100001) has been deleted
Hi Vaclav, I've added a new patch for review incorporating your review inputs. PTAL Thanks! https://codereview.chromium.org/548953002/diff/80001/chrome/browser/password_... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/548953002/diff/80001/chrome/browser/password_... chrome/browser/password_manager/password_manager_browsertest.cc:1242: // The redirection page now redirects via Javascript. On 2014/09/24 09:47:43, vabr (Chromium) wrote: > It is not clear what is "the redirection page", and why this line is needed. > Please explain in the comment. Done. Added just for confirmation. Removed now as was unnecessary. https://codereview.chromium.org/548953002/diff/80001/chrome/test/data/passwor... File chrome/test/data/password/form_with_only_password_field.html (right): https://codereview.chromium.org/548953002/diff/80001/chrome/test/data/passwor... chrome/test/data/password/form_with_only_password_field.html:5: <label for="password">Password</label> On 2014/09/24 09:47:43, vabr (Chromium) wrote: > nit: Although the name of the file suggest this, could you please also insert a > <!-- comment --> stating that there is on purpose no username field. This is > just to make sure somebody does not include one later by mistake. Done. https://codereview.chromium.org/548953002/diff/80001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.cc (left): https://codereview.chromium.org/548953002/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:973: if (DoUsernamesMatch(fill_data.basic_data.fields[0].value, On 2014/09/24 09:47:44, vabr (Chromium) wrote: > Suggestion: if you modify this check to also consider an empty > |current_username| and an empty |fill_data.basic_data.fields[0].value| as a > match, then you can leave most of the code below (and the line above) as it is. I think, I didn't get this correctly. https://codereview.chromium.org/548953002/diff/80001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/548953002/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:74: // Only consider non-empty input fields for autofilling On 2014/09/24 09:47:43, vabr (Chromium) wrote: > I'm confused by this comment -- http://crbug.com/406343 is about saving > passwords on forms without a username, not about empty input fields. > Also, the comment says "non-empty input field", whereas the code checks for the > field's name (not field itself) being empty. Done. https://codereview.chromium.org/548953002/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:228: static bool FormWithEmptyUsernameField(const PasswordFormFillData& fill_data) { On 2014/09/24 09:47:43, vabr (Chromium) wrote: > Again -- the comment and the function name say "empty field", whereas the code > tests the emptiness of the field's name. > > My suggestion is: rename the function to FillDataContainUsername (and negate the > result to accommodate the change in semantics). You can leave out the explaining > comment entirely, because all will be clear from the code. Done. https://codereview.chromium.org/548953002/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:228: static bool FormWithEmptyUsernameField(const PasswordFormFillData& fill_data) { On 2014/09/24 09:47:44, vabr (Chromium) wrote: > No need to include "static" -- the function is invisible outside of the .cc file > because of being in the anonymous namespace already. Done. https://codereview.chromium.org/548953002/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:818: // Check whether password form has username input field On 2014/09/24 09:47:43, vabr (Chromium) wrote: > grammar nit: add articles -- the password form, a username input field Done. https://codereview.chromium.org/548953002/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:819: // (http://crbug.com/406343). On 2014/09/24 09:47:43, vabr (Chromium) wrote: > nit: I'm not sure it's necessary to include the reference to the bug. It's > already clear that the code tests the existence of the username field, no more > context (possibly found on the bug) is necessary to understand the code. Done. https://codereview.chromium.org/548953002/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:820: bool is_password_only_form = FormWithEmptyUsernameField(form_data); On 2014/09/24 09:47:43, vabr (Chromium) wrote: > (Note: if you change the function to FillDataContainUsername, don't forget to > rename the bool appropriately, and stop negating it below.) Done. https://codereview.chromium.org/548953002/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:821: if (!is_password_only_form) On 2014/09/24 09:47:43, vabr (Chromium) wrote: > nit: Add braces, because the body of the if-clause spans more lines. Done. https://codereview.chromium.org/548953002/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:841: if (!is_password_only_form && On 2014/09/24 09:47:43, vabr (Chromium) wrote: > Why do you exclude the case of no-username passwords? You apparently add an > entry for them in login_to_password_info_. > > If any two empty WebInputElements are equal, then the find() below should work. > If not, then there is no point in adding forms without username element to the > login_to_password_info_ map. Please find out what is the situation, and amend > accordingly. Done. https://codereview.chromium.org/548953002/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:855: FindFormAndFieldForFormControlElement( On 2014/09/24 09:47:43, vabr (Chromium) wrote: > nit: missing braces Done. https://codereview.chromium.org/548953002/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:957: (!is_password_only_form && !username_element.form().autoComplete())) On 2014/09/24 09:47:43, vabr (Chromium) wrote: > nit: no need for the additional parentheses -- && is associative Done. https://codereview.chromium.org/548953002/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:1109: // Password form may not have username input fields On 2014/09/24 09:47:43, vabr (Chromium) wrote: > Suggested rephrasing to simplify the comment: > > // There may not be a username field, so get the frame from the password field. Done. https://codereview.chromium.org/548953002/diff/80001/components/password_mana... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/548953002/diff/80001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:229: return (!observed_form_.password_element.empty() || On 2014/09/24 09:47:44, vabr (Chromium) wrote: > nit: no need for the outer parentheses Done.
vabr@chromium.org changed reviewers: + gcasto@chromium.org
Garrett, Could you please review components/autofill/content/renderer/password_autofill_agent.cc, which I'm not OWNER of? Feel free to just rubberstamp, as I reviewed the whole CL, but obviously you're also welcome to have a deep look. The context is in the BUG. Pritam Nikam, Thanks, I have a handful of comment, but if you address them, the CL LGTM. Cheers, Vaclav https://codereview.chromium.org/548953002/diff/120001/chrome/test/data/passwo... File chrome/test/data/password/form_with_only_password_field.html (right): https://codereview.chromium.org/548953002/diff/120001/chrome/test/data/passwo... chrome/test/data/password/form_with_only_password_field.html:6: intentianally excluded here from this form typo: intentionally https://codereview.chromium.org/548953002/diff/120001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (left): https://codereview.chromium.org/548953002/diff/120001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:973: if (DoUsernamesMatch(fill_data.basic_data.fields[0].value, Wouldn't this actually work just fine if there was no username? If you change base::string16 current_username = username_element->value(); into base::string16 current_username = username_element->value(); if (!username_element->isNull()) current_username = username_element->value(); then the first and the second argument should be empty, thus matching. That would simplify the changes below. https://codereview.chromium.org/548953002/diff/120001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/548953002/diff/120001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:76: if (data.fields[j].name.empty()) Looking at InitPasswordFormFillData, the username field is always at index 0, so please do not do this for every value of |j|. I actually wonder why we do not check that data.fields.size() == 2. I filed http://crbug.com/417295 for that.
Hi Vaclav, Thanks again! for your review inputs. I've addresses all your inputs and did few changes related to refactoring in patch set# 7. Please have a fresh look. Thanks! https://codereview.chromium.org/548953002/diff/120001/chrome/test/data/passwo... File chrome/test/data/password/form_with_only_password_field.html (right): https://codereview.chromium.org/548953002/diff/120001/chrome/test/data/passwo... chrome/test/data/password/form_with_only_password_field.html:6: intentianally excluded here from this form On 2014/09/24 16:17:21, vabr (Chromium) wrote: > typo: intentionally Done. https://codereview.chromium.org/548953002/diff/120001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (left): https://codereview.chromium.org/548953002/diff/120001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:973: if (DoUsernamesMatch(fill_data.basic_data.fields[0].value, On 2014/09/24 16:17:21, vabr (Chromium) wrote: > Wouldn't this actually work just fine if there was no username? > If you change > base::string16 current_username = username_element->value(); > into > base::string16 current_username = username_element->value(); > if (!username_element->isNull()) > current_username = username_element->value(); > > then the first and the second argument should be empty, thus matching. > That would simplify the changes below. Done. Right! I did it this way now. https://codereview.chromium.org/548953002/diff/120001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/548953002/diff/120001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:76: if (data.fields[j].name.empty()) On 2014/09/24 16:17:21, vabr (Chromium) wrote: > Looking at InitPasswordFormFillData, the username field is always at index 0, so > please do not do this for every value of |j|. > > I actually wonder why we do not check that data.fields.size() == 2. I filed > http://crbug.com/417295 for that. Done. I've refactored code, and these changes are unneeded, hence removing.
Hi Pritam Nikam, Please consider removing the refactoring you did in patch set 7, where password fields shift to position 0 in the absence of username fields. As noted in some comments below, this looks like breaking some code, and I have not checked other places. Refactoring this should be done completely separately, and tracked under http://crbug.com/417295. For your change, you don't need this refactoring. It's completely fine to keep the empty/null username in fill_data.basic_data.fields. Cheers, Vaclav https://codereview.chromium.org/548953002/diff/120001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (left): https://codereview.chromium.org/548953002/diff/120001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:973: if (DoUsernamesMatch(fill_data.basic_data.fields[0].value, On 2014/09/25 07:22:27, Pritam Nikam wrote: > On 2014/09/24 16:17:21, vabr (Chromium) wrote: > > Wouldn't this actually work just fine if there was no username? > > If you change > > base::string16 current_username = username_element->value(); > > into > > base::string16 current_username = username_element->value(); > > if (!username_element->isNull()) > > current_username = username_element->value(); > > > > then the first and the second argument should be empty, thus matching. > > That would simplify the changes below. > > Done. > > Right! I did it this way now. Actually, I don't think you did it the way suggested, due to introducing the possibility for the password field to be at index 0 of |fields|. https://codereview.chromium.org/548953002/diff/140001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/548953002/diff/140001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:1001: if (DoUsernamesMatch(fill_data.basic_data.fields[0].value, This potentially tries to match |current_username| against the value of the password field, which is confusing at least. https://codereview.chromium.org/548953002/diff/140001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:1007: // Scan additional logins for a match. This branch is entered for all forms without username, which have a non-empty password field. That sounds wrong.
Hi Vaclav, Reverted refactoring related changes made in #7. Patch set#8 up for review. PTAL. Thanks! https://codereview.chromium.org/548953002/diff/140001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/548953002/diff/140001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:1001: if (DoUsernamesMatch(fill_data.basic_data.fields[0].value, On 2014/09/25 10:39:59, vabr (Chromium) wrote: > This potentially tries to match |current_username| against the value of the > password field, which is confusing at least. Done. Reverted refactoring. https://codereview.chromium.org/548953002/diff/140001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:1007: // Scan additional logins for a match. On 2014/09/25 10:39:59, vabr (Chromium) wrote: > This branch is entered for all forms without username, which have a non-empty > password field. That sounds wrong. Done. Reverted refactoring.
LGTM, with one comment. Thanks! Vaclav https://codereview.chromium.org/548953002/diff/160001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/548953002/diff/160001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:72: size_t j = 0; Out of the for-loop, this looks a bit strange. What about replacing it with: const bool username_is_present = !data.fields[0].name.empty(); and then phrasing the for-loop as // First field is the username, skip it if not present. for (size_t j = (username_is_present ? 1 : 0); j < data.fields.size(); ++j) {
Thanks Vaclav for review. Added changes addressing your inputs. PTAL. Thanks! https://codereview.chromium.org/548953002/diff/160001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/548953002/diff/160001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:72: size_t j = 0; On 2014/09/25 12:39:48, vabr (Chromium) wrote: > Out of the for-loop, this looks a bit strange. What about replacing it with: > > const bool username_is_present = !data.fields[0].name.empty(); > > and then phrasing the for-loop as > > // First field is the username, skip it if not present. > for (size_t j = (username_is_present ? 1 : 0); j < data.fields.size(); ++j) { Done.
Thank you, Pritam Nikam, LGTM! Before landing, you still need an OWNERS rubber stamp from gcasto@ for components/autofill/content/renderer/password_autofill_agent.cc. He is currently very busy with some urgent work, but seemed hopeful to get to this review soon. Cheers, Vaclav
LGTM https://codereview.chromium.org/548953002/diff/180001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/548953002/diff/180001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:953: form_contains_username_field && !username_element.form().autoComplete()) nit: Line break after "&&"
Thanks Vaclav & Garrett for review. For last review comment, "git cl format" does not respect line break and restores it in-line. Since, rest of the patch okayed from you both, I'm assuming (#9) it's good to go. I'll go ahead with commit bit. Thanks once again for your guidance and forbearance. Cheers, Pritam https://codereview.chromium.org/548953002/diff/180001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/548953002/diff/180001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:953: form_contains_username_field && !username_element.form().autoComplete()) On 2014/09/25 20:34:38, Garrett Casto wrote: > nit: Line break after "&&" "git cl format" restores it in-line. Does not respect line-break here.
The CQ bit was checked by pritam.nikam@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/548953002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Pritam Nikam, Currently, the presubmit script fails to recognise that you have the requested OWNERS approving your CL. You apparently do, so if presubmit stops you from landing the CL for _only_ that reason, you can insert TBR=gcasto@chromium.org,vabr@chromium.org in the CL description to override that particular check. If you don't wait for Garrett's reply to the clash between his last comment and the result of git cl format, it would be nice to offer a follow-up CL in case he insists on the non-git cl format way of indenting that particular line. Cheers, Vaclav https://codereview.chromium.org/548953002/diff/180001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/548953002/diff/180001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:953: form_contains_username_field && !username_element.form().autoComplete()) On 2014/09/26 04:07:14, Pritam Nikam wrote: > On 2014/09/25 20:34:38, Garrett Casto wrote: > > nit: Line break after "&&" > > "git cl format" restores it in-line. Does not respect line-break here. Garrett, you are the OWNER here, but I suggest going with clang-format wherever reasonable, to make future refactoring easier, and using the deactivation comments // clang-format off special_formatting(); // clang-format on (see b/14285621) where the clang-format's result actually hurts. How does that sound to you? https://codereview.chromium.org/548953002/diff/200001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/548953002/diff/200001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:76: for (size_t j = (username_is_present ? 0 : 1); j < data.fields.size(); ++j) { Sorry for causing this with the typo in my comment, and thumbs up for fixing it. :)
On 2014/09/26 12:32:39, vabr (Chromium) wrote: > Pritam Nikam, > Currently, the presubmit script fails to recognise that you have the requested > OWNERS approving your CL. You apparently do, so if presubmit stops you from > landing the CL for _only_ that reason, you can insert > mailto:TBR=gcasto@chromium.org,vabr@chromium.org in the CL description to override that > particular check. > > If you don't wait for Garrett's reply to the clash between his last comment and > the result of git cl format, it would be nice to offer a follow-up CL in case he > insists on the non-git cl format way of indenting that particular line. > > Cheers, > Vaclav > > https://codereview.chromium.org/548953002/diff/180001/components/autofill/con... > File components/autofill/content/renderer/password_autofill_agent.cc (right): > > https://codereview.chromium.org/548953002/diff/180001/components/autofill/con... > components/autofill/content/renderer/password_autofill_agent.cc:953: > form_contains_username_field && !username_element.form().autoComplete()) > On 2014/09/26 04:07:14, Pritam Nikam wrote: > > On 2014/09/25 20:34:38, Garrett Casto wrote: > > > nit: Line break after "&&" > > > > "git cl format" restores it in-line. Does not respect line-break here. > > Garrett, you are the OWNER here, but I suggest going with clang-format wherever > reasonable, to make future refactoring easier, and using the deactivation > comments > // clang-format off > special_formatting(); > // clang-format on > (see b/14285621) where the clang-format's result actually hurts. > > How does that sound to you? > > https://codereview.chromium.org/548953002/diff/200001/components/autofill/con... > File components/autofill/content/renderer/password_autofill_agent.cc (right): > > https://codereview.chromium.org/548953002/diff/200001/components/autofill/con... > components/autofill/content/renderer/password_autofill_agent.cc:76: for (size_t > j = (username_is_present ? 0 : 1); j < data.fields.size(); ++j) { > Sorry for causing this with the typo in my comment, and thumbs up for fixing it. > :) Thanks Vaclav once again. So, shall I try commit bit now? I can wait anyway for Garrett's reply. Thanks!
On 2014/09/26 13:10:06, Pritam Nikam wrote: > On 2014/09/26 12:32:39, vabr (Chromium) wrote: > > Pritam Nikam, > > Currently, the presubmit script fails to recognise that you have the requested > > OWNERS approving your CL. You apparently do, so if presubmit stops you from > > landing the CL for _only_ that reason, you can insert > > mailto:TBR=gcasto@chromium.org,vabr@chromium.org in the CL description to > override that > > particular check. > > > > If you don't wait for Garrett's reply to the clash between his last comment > and > > the result of git cl format, it would be nice to offer a follow-up CL in case > he > > insists on the non-git cl format way of indenting that particular line. > > > > Cheers, > > Vaclav > > > > > https://codereview.chromium.org/548953002/diff/180001/components/autofill/con... > > File components/autofill/content/renderer/password_autofill_agent.cc (right): > > > > > https://codereview.chromium.org/548953002/diff/180001/components/autofill/con... > > components/autofill/content/renderer/password_autofill_agent.cc:953: > > form_contains_username_field && !username_element.form().autoComplete()) > > On 2014/09/26 04:07:14, Pritam Nikam wrote: > > > On 2014/09/25 20:34:38, Garrett Casto wrote: > > > > nit: Line break after "&&" > > > > > > "git cl format" restores it in-line. Does not respect line-break here. > > > > Garrett, you are the OWNER here, but I suggest going with clang-format > wherever > > reasonable, to make future refactoring easier, and using the deactivation > > comments > > // clang-format off > > special_formatting(); > > // clang-format on > > (see b/14285621) where the clang-format's result actually hurts. > > > > How does that sound to you? > > > > > https://codereview.chromium.org/548953002/diff/200001/components/autofill/con... > > File components/autofill/content/renderer/password_autofill_agent.cc (right): > > > > > https://codereview.chromium.org/548953002/diff/200001/components/autofill/con... > > components/autofill/content/renderer/password_autofill_agent.cc:76: for > (size_t > > j = (username_is_present ? 0 : 1); j < data.fields.size(); ++j) { > > Sorry for causing this with the typo in my comment, and thumbs up for fixing > it. > > :) > > Thanks Vaclav once again. > > So, shall I try commit bit now? I can wait anyway for Garrett's reply. > > Thanks! You should be fine to send this to the commit queue, as long as you are happy to accommodate Garrett's potential further comments on the formatting in a follow-up CL. Thank you! Vaclav
The CQ bit was checked by pritam.nikam@samsung.com
On 2014/09/26 14:09:29, vabr (Chromium) wrote: > On 2014/09/26 13:10:06, Pritam Nikam wrote: > > On 2014/09/26 12:32:39, vabr (Chromium) wrote: > > > Pritam Nikam, > > > Currently, the presubmit script fails to recognise that you have the > requested > > > OWNERS approving your CL. You apparently do, so if presubmit stops you from > > > landing the CL for _only_ that reason, you can insert > > > mailto:TBR=gcasto@chromium.org,vabr@chromium.org in the CL description to > > override that > > > particular check. > > > > > > If you don't wait for Garrett's reply to the clash between his last comment > > and > > > the result of git cl format, it would be nice to offer a follow-up CL in > case > > he > > > insists on the non-git cl format way of indenting that particular line. > > > > > > Cheers, > > > Vaclav > > > > > > > > > https://codereview.chromium.org/548953002/diff/180001/components/autofill/con... > > > File components/autofill/content/renderer/password_autofill_agent.cc > (right): > > > > > > > > > https://codereview.chromium.org/548953002/diff/180001/components/autofill/con... > > > components/autofill/content/renderer/password_autofill_agent.cc:953: > > > form_contains_username_field && !username_element.form().autoComplete()) > > > On 2014/09/26 04:07:14, Pritam Nikam wrote: > > > > On 2014/09/25 20:34:38, Garrett Casto wrote: > > > > > nit: Line break after "&&" > > > > > > > > "git cl format" restores it in-line. Does not respect line-break here. > > > > > > Garrett, you are the OWNER here, but I suggest going with clang-format > > wherever > > > reasonable, to make future refactoring easier, and using the deactivation > > > comments > > > // clang-format off > > > special_formatting(); > > > // clang-format on > > > (see b/14285621) where the clang-format's result actually hurts. > > > > > > How does that sound to you? > > > > > > > > > https://codereview.chromium.org/548953002/diff/200001/components/autofill/con... > > > File components/autofill/content/renderer/password_autofill_agent.cc > (right): > > > > > > > > > https://codereview.chromium.org/548953002/diff/200001/components/autofill/con... > > > components/autofill/content/renderer/password_autofill_agent.cc:76: for > > (size_t > > > j = (username_is_present ? 0 : 1); j < data.fields.size(); ++j) { > > > Sorry for causing this with the typo in my comment, and thumbs up for fixing > > it. > > > :) > > > > Thanks Vaclav once again. > > > > So, shall I try commit bit now? I can wait anyway for Garrett's reply. > > > > Thanks! > > You should be fine to send this to the commit queue, as long as you are happy to > accommodate Garrett's potential further comments on the formatting in a > follow-up CL. > > Thank you! > Vaclav Sure! Will do that :) Thanks!
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/548953002/200001
Message was sent while issue was closed.
Committed patchset #10 (id:200001) as 0e0e39a99ff7ede1830027369ff5df757947d99e
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/77d3ffc71361592df3fdf9a5377045f088bcaeac Cr-Commit-Position: refs/heads/master@{#296943}
Message was sent while issue was closed.
lgtm https://codereview.chromium.org/548953002/diff/180001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/548953002/diff/180001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:953: form_contains_username_field && !username_element.form().autoComplete()) On 2014/09/26 12:32:39, vabr (Chromium) wrote: > On 2014/09/26 04:07:14, Pritam Nikam wrote: > > On 2014/09/25 20:34:38, Garrett Casto wrote: > > > nit: Line break after "&&" > > > > "git cl format" restores it in-line. Does not respect line-break here. > > Garrett, you are the OWNER here, but I suggest going with clang-format wherever > reasonable, to make future refactoring easier, and using the deactivation > comments > // clang-format off > special_formatting(); > // clang-format on > (see b/14285621) where the clang-format's result actually hurts. > > How does that sound to you? Keeping this as is is fine. I haven't been using clang formatting yet, but moving towards automated formatting is a good thing. I find this particular styling harder to read, but whatever. I'll get used to it. |
