Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
597983003:
Refactor PasswordAutofillAgent: methods to functions. (Closed)
|
Created:
6 years, 3 months ago by Deepak Modified:
6 years, 2 months ago CC:
chromium-reviews, benquan, browser-components-watch_chromium.org, jam, rouslan+autofillwatch_chromium.org, darin-cc_chromium.org, Dane Wallinga, dyu1, mkwst+moarreviews-renderer_chromium.org, estade+watch_chromium.org, gcasto+watchlist_chromium.org, Ilya Sherman, mkwst+watchlist_chromium.org, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionRefactor PasswordAutofillAgent: methods to functions.
The functions that are not using the member variables or just
using few member variables are made helper function so that
PasswordAutofillAgent class become thin and have relevent
member functions only.
BUG=398436
Committed: https://crrev.com/b7db6e91ad6f817fe62a3e29b3fdc2018d27fcec
Cr-Commit-Position: refs/heads/master@{#297586}
Patch Set 1 #
Total comments: 19
Patch Set 2 : changes as per review comments. #
Total comments: 8
Patch Set 3 : Changes as per review comments. #Patch Set 4 : Changes as per review comments. #
Total comments: 35
Patch Set 5 : changes as per review comments. #
Total comments: 17
Patch Set 6 : Changes as per review comments. #Patch Set 7 : Changes as Per Review comments #Patch Set 8 : Changes as per review comments. #Patch Set 9 : Changes as per review comments. #Patch Set 10 : Changes as per review comments. #
Total comments: 8
Patch Set 11 : changes as per new review comments. #Patch Set 12 : nit changes. #
Total comments: 2
Patch Set 13 : Changes as per new comments. #
Total comments: 2
Patch Set 14 : nit changes. #
Messages
Total messages: 38 (4 generated)
deepak.m1@samsung.com changed reviewers: + gcasto@chromium.org, vabr@chromium.org
Attempt to slim down the definition of the PasswordAutofillAgent class by converting member functions to helper functions. So that relevant function only remain the member function. As vabr guided & suggested. PTAL.
Hi Deepak, Thanks for your CL. I left a couple of comments, please have a look. Note that comments starting "Just for future" are not meant to be addressed here, rather something to watch for in your future CLs. Cheers, Vaclav https://codereview.chromium.org/597983003/diff/1/components/autofill/content/... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/597983003/diff/1/components/autofill/content/... components/autofill/content/renderer/password_autofill_agent.cc:221: bool FillUserNameAndPassword(struct ParametersNeedUpdate* param, nit: Please also document that the return value indicates whether the username and password were filled. (Slightly unrelated note: apparently, the return value of FillUserNameAndPassword was not used (but we start to use it in this CL). If you are looking for clean-up CLs in the future, changing return value type of functions to void if the returned value is always ignored is a good thing to look for.) https://codereview.chromium.org/597983003/diff/1/components/autofill/content/... components/autofill/content/renderer/password_autofill_agent.cc:221: bool FillUserNameAndPassword(struct ParametersNeedUpdate* param, (Just for future: if you passed a struct argument like this, you can drop "struct".) https://codereview.chromium.org/597983003/diff/1/components/autofill/content/... components/autofill/content/renderer/password_autofill_agent.cc:221: bool FillUserNameAndPassword(struct ParametersNeedUpdate* param, Please just pass a OtherPossibleUsernamesUsage* usernames_usage out param (should be the last argument). You can change usernames_usage_ through that one directly. https://codereview.chromium.org/597983003/diff/1/components/autofill/content/... components/autofill/content/renderer/password_autofill_agent.cc:279: // Input matches the username, fill in required values. Why did you leave out the "TODO(kent)" present at this point in the original code? https://codereview.chromium.org/597983003/diff/1/components/autofill/content/... components/autofill/content/renderer/password_autofill_agent.cc:298: param->gate_keeper = true; There is no need to signal this through |gate_keeper| -- observe that gate_keeper is true if and only if the whole method returns true. https://codereview.chromium.org/597983003/diff/1/components/autofill/content/... components/autofill/content/renderer/password_autofill_agent.cc:304: void GetSuggestions(struct ParametersNeedUpdate* param, Please keep the order of the 3 functions as it was -- it made sense to group the Fill* methods together. Also, keeping the same order makes it easier to diff your change against the ToT when reviewing it. https://codereview.chromium.org/597983003/diff/1/components/autofill/content/... components/autofill/content/renderer/password_autofill_agent.cc:372: FillUserNameAndPassword(param, Forward the return value here, to signal out if the gatekeeper should register the password element. https://codereview.chromium.org/597983003/diff/1/components/autofill/content/... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/597983003/diff/1/components/autofill/content/... components/autofill/content/renderer/password_autofill_agent.h:26: // This struct is used in helper function to indicate parameter modification. Please don't introduce this struct. I indicated below in the comments, how you can do this without it. (BTW., just for the future: even if you needed it, the right place to define it would be in the .cc file, because it is only used by the methods in the .cc file.) https://codereview.chromium.org/597983003/diff/1/components/autofill/content/... components/autofill/content/renderer/password_autofill_agent.h:30: ParametersNeedUpdate() { (Just for future: it is considered better practice to initialise the member variables through the constructor's member initialiser list: ParametersNeedUpdate() : usernames_usage(false), gate_keeper(false) {} That's more readable, and can be more efficient for more complex data members, where construction and assignment takes more time than just construction with the right values.)
PTAL https://codereview.chromium.org/597983003/diff/1/components/autofill/content/... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/597983003/diff/1/components/autofill/content/... components/autofill/content/renderer/password_autofill_agent.cc:221: bool FillUserNameAndPassword(struct ParametersNeedUpdate* param, On 2014/09/25 10:25:49, vabr (Chromium) wrote: > Please just pass a OtherPossibleUsernamesUsage* usernames_usage out param > (should be the last argument). You can change usernames_usage_ through that one > directly. I have made OtherPossibleUsernamesUsage enum as public so that we can use that in the helper functions, as earlier it is private and that can not be accessed outside class. https://codereview.chromium.org/597983003/diff/1/components/autofill/content/... components/autofill/content/renderer/password_autofill_agent.cc:221: bool FillUserNameAndPassword(struct ParametersNeedUpdate* param, On 2014/09/25 10:25:49, vabr (Chromium) wrote: > Please just pass a OtherPossibleUsernamesUsage* usernames_usage out param > (should be the last argument). You can change usernames_usage_ through that one > directly. Acknowledged. https://codereview.chromium.org/597983003/diff/1/components/autofill/content/... components/autofill/content/renderer/password_autofill_agent.cc:221: bool FillUserNameAndPassword(struct ParametersNeedUpdate* param, On 2014/09/25 10:25:49, vabr (Chromium) wrote: > Please just pass a OtherPossibleUsernamesUsage* usernames_usage out param > (should be the last argument). You can change usernames_usage_ through that one > directly. Acknowledged. https://codereview.chromium.org/597983003/diff/1/components/autofill/content/... components/autofill/content/renderer/password_autofill_agent.cc:279: // Input matches the username, fill in required values. On 2014/09/25 10:25:49, vabr (Chromium) wrote: > Why did you leave out the "TODO(kent)" present at this point in the original > code? Acknowledged. https://codereview.chromium.org/597983003/diff/1/components/autofill/content/... components/autofill/content/renderer/password_autofill_agent.cc:298: param->gate_keeper = true; On 2014/09/25 10:25:49, vabr (Chromium) wrote: > There is no need to signal this through |gate_keeper| -- observe that > gate_keeper is true if and only if the whole method returns true. Acknowledged. https://codereview.chromium.org/597983003/diff/1/components/autofill/content/... components/autofill/content/renderer/password_autofill_agent.cc:304: void GetSuggestions(struct ParametersNeedUpdate* param, On 2014/09/25 10:25:49, vabr (Chromium) wrote: > Please keep the order of the 3 functions as it was -- it made sense to group the > Fill* methods together. Also, keeping the same order makes it easier to diff > your change against the ToT when reviewing it. We are calling FillUserNameAndPassword() from FillFormOnPasswordRecieved(), so FillUserNameAndPassword() should be before FillFormOnPasswordRecieved(). https://codereview.chromium.org/597983003/diff/1/components/autofill/content/... components/autofill/content/renderer/password_autofill_agent.cc:372: FillUserNameAndPassword(param, On 2014/09/25 10:25:49, vabr (Chromium) wrote: > Forward the return value here, to signal out if the gatekeeper should register > the password element. Acknowledged. https://codereview.chromium.org/597983003/diff/1/components/autofill/content/... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/597983003/diff/1/components/autofill/content/... components/autofill/content/renderer/password_autofill_agent.h:26: // This struct is used in helper function to indicate parameter modification. On 2014/09/25 10:25:49, vabr (Chromium) wrote: > Please don't introduce this struct. I indicated below in the comments, how you > can do this without it. > > (BTW., just for the future: even if you needed it, the right place to define it > would be in the .cc file, because it is only used by the methods in the .cc > file.) Thanks,I will take care in future. https://codereview.chromium.org/597983003/diff/1/components/autofill/content/... components/autofill/content/renderer/password_autofill_agent.h:30: ParametersNeedUpdate() { On 2014/09/25 10:25:49, vabr (Chromium) wrote: > (Just for future: it is considered better practice to initialise the member > variables through the constructor's member initialiser list: > ParametersNeedUpdate() : usernames_usage(false), gate_keeper(false) {} > That's more readable, and can be more efficient for more complex data members, > where construction and assignment takes more time than just construction with > the right values.) Thanks,I will take care in future.
https://codereview.chromium.org/597983003/diff/1/components/autofill/content/... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/597983003/diff/1/components/autofill/content/... components/autofill/content/renderer/password_autofill_agent.cc:1086: struct ParametersNeedUpdate param; no 'struct' here as well. This is a C[ism].
On 2014/09/25 20:00:23, tfarina wrote: > https://codereview.chromium.org/597983003/diff/1/components/autofill/content/... > File components/autofill/content/renderer/password_autofill_agent.cc (right): > > https://codereview.chromium.org/597983003/diff/1/components/autofill/content/... > components/autofill/content/renderer/password_autofill_agent.cc:1086: struct > ParametersNeedUpdate param; > no 'struct' here as well. This is a C[ism]. vabr already suggested me to remove struct, as it is not needed. I have done changes and uploaded patch again yesterday itself. my request is to review patch set 2. Thanks
Hi Deepak, Please have a look at my comments below. Before addressing them though, you might want to wait for gcasto@'s input as well, because he expressed some concerns to me about whether getting rid of the 3 methods is worth the introduced changes. I addressed some of his concerns by the suggestions made below, but I'll let him speak for himself. He is the OWNER of the code, so you'd have to get his approval anyway, and waiting now can spare you some work later. Thanks, Vaclav P.S. A piece of advice for the future, on the use of the review tool: The canned responses in the comments, "Done" and "Acknowledged" differ a bit: "Acknowledged" means that you read and understood the comment, but took no action. This is appropriate for comments like "In the future, watch out for..." or "There is an alternative, but I'm fine with the current state," which do not require a follow-up action or response. "Done" should be used in comments which you addressed by a straightforward action, like following the suggestion, or reverting a questioned change. Everything else should have a standard reply. It's not a big deal if you use Acknowledged where Done were appropriate, the important thing is your new code addresses the comment. But choosing the right answer helps the reviewer. https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:221: void GetSuggestions( nit: Please provide a short comment describing what this function does, the meaning of arguments, and the (new) return value. https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:221: void GetSuggestions( I suggest removing the |usernames_usage| and signalling the needed change through the return value instead (see my comment in the header file). https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:251: OTHER_POSSIBLE_USERNAME_SELECTED; Please keep the correct value. SHOWN, not SELECTED! https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:259: bool FillUserNameAndPassword( nit: Please also here provide a short comment describing what this function does, the meaning of arguments, and the return value. https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:259: bool FillUserNameAndPassword( I suggest the following changes to the signature of this function, and of FillFormOnPasswordRecieved as well: (1) Remove |usernames_usage|, use the return value to signal the need for updating that (similarly to what I suggest for GetSuggestions). (2) Because the return value can no longer be used for signalling the gatekeeper registration, and also to avoid the need to call the gatekeeper on each callsite, pass a callback argument |password_registration| of type base::Callback< void(blink::WebInputElement*)>. Mention in the documentation commend for both functions that |password_registration| should be the gatekeeper registration call for the autofilled password field, if any. https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:349: // such a password exists. nit: Please describe the meaning of the (new) return value. https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:471: bool gate_keeper = FillUserNameAndPassword(&username, (The following is made obsolete by my other comments, but just for learning purposes, this would be my comment if the code stayed this way: nit: Calling this bool |gate_keeper| in the presence of the gatekeeper object named |gatekeeper_| is rather confusing. I suggest naming the bool after the meaning of the return value, e.g., |password_filled|.) https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.h:30: enum OtherPossibleUsernamesUsage { Please keep this private, not public. Instead, use the return value of the 3 moved functions to indicate whether or not other possible usernames were shown/selected. Also please document that meaning of the return value.
Thanks a lot vabr for your time & guidance in review. yes, you are right, we should wait for gcasto opinion, but in the mean time give me opportunity to have a query. On 2014/09/26 10:06:48, vabr (Chromium) wrote: > Hi Deepak, > > Please have a look at my comments below. > > Before addressing them though, you might want to wait for gcasto@'s input as > well, because he expressed some concerns to me about whether getting rid of the > 3 methods is worth the introduced changes. I addressed some of his concerns by > the suggestions made below, but I'll let him speak for himself. He is the OWNER > of the code, so you'd have to get his approval anyway, and waiting now can spare > you some work later. > > Thanks, > Vaclav > > P.S. A piece of advice for the future, on the use of the review tool: > > The canned responses in the comments, "Done" and "Acknowledged" differ a bit: > "Acknowledged" means that you read and understood the comment, but took no > action. This is appropriate for comments like "In the future, watch out for..." > or "There is an alternative, but I'm fine with the current state," which do not > require a follow-up action or response. "Done" should be used in comments which > you addressed by a straightforward action, like following the suggestion, or > reverting a questioned change. Everything else should have a standard reply. > > It's not a big deal if you use Acknowledged where Done were appropriate, the > important thing is your new code addresses the comment. But choosing the right > answer helps the reviewer. > > https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... > File components/autofill/content/renderer/password_autofill_agent.cc (right): > > https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... > components/autofill/content/renderer/password_autofill_agent.cc:221: void > GetSuggestions( > nit: Please provide a short comment describing what this function does, the > meaning of arguments, and the (new) return value. > > https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... > components/autofill/content/renderer/password_autofill_agent.cc:221: void > GetSuggestions( > I suggest removing the |usernames_usage| and signalling the needed change > through the return value instead (see my comment in the header file). > > https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... > components/autofill/content/renderer/password_autofill_agent.cc:251: > OTHER_POSSIBLE_USERNAME_SELECTED; > Please keep the correct value. SHOWN, not SELECTED! > > https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... > components/autofill/content/renderer/password_autofill_agent.cc:259: bool > FillUserNameAndPassword( > nit: Please also here provide a short comment describing what this function > does, the meaning of arguments, and the return value. > > https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... > components/autofill/content/renderer/password_autofill_agent.cc:259: bool > FillUserNameAndPassword( > I suggest the following changes to the signature of this function, and of > FillFormOnPasswordRecieved as well: > > (1) Remove |usernames_usage|, use the return value to signal the need for > updating that (similarly to what I suggest for GetSuggestions). > > (2) Because the return value can no longer be used for signalling the gatekeeper > registration, and also to avoid the need to call the gatekeeper on each > callsite, pass a callback argument |password_registration| of type > base::Callback< void(blink::WebInputElement*)>. Mention in the documentation > commend for both functions that |password_registration| should be the gatekeeper > registration call for the autofilled password field, if any. This part (2) I am not able to understood fully, when I will call RegisterElement() from callback, as PasswordValueGatekeeper class is the private member of PasswordAutofillAgent class,then we don't have 'this' pointer then which object data it is modifying?, and this call will not effect the gatekeeper_ object data. and secondly as username_usage_ value can not be decided as true or false for FillUserNameAndPassword() as we are returning false base on (current_username != username) also and secondly It is possible that control does not come to check possible usernames, Then in the end of function I can not return false,as value of usernames_usage_ is not set. I wanted to purpose that I can use enum value as the retun from these 3 API's and I will define that enum inside 'namespace autofill',but outside anonymus namespace in .cc file like. namespace autofill { enum UsernameGatekeeperUsage { NOTHING_USAGE, USERNAMES_USAGE, GATEKEEPER_USAGE, USERNAMES_AND_GATEKEEPER_USAGE }; namespace { that will fix our issue as we can act base on return value only. I would like to know opinion for this. > https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... > components/autofill/content/renderer/password_autofill_agent.cc:349: // such a > password exists. > nit: Please describe the meaning of the (new) return value. > > https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... > components/autofill/content/renderer/password_autofill_agent.cc:471: bool > gate_keeper = FillUserNameAndPassword(&username, > (The following is made obsolete by my other comments, but just for learning > purposes, this would be my comment if the code stayed this way: > > nit: Calling this bool |gate_keeper| in the presence of the gatekeeper object > named |gatekeeper_| is rather confusing. I suggest naming the bool after the > meaning of the return value, e.g., |password_filled|.) > > https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... > File components/autofill/content/renderer/password_autofill_agent.h (right): > > https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... > components/autofill/content/renderer/password_autofill_agent.h:30: enum > OtherPossibleUsernamesUsage { > Please keep this private, not public. > > Instead, use the return value of the 3 moved functions to indicate whether or > not other possible usernames were shown/selected. Also please document that > meaning of the return value.
Hi Deepak, Please see my answers below. > https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... > > components/autofill/content/renderer/password_autofill_agent.cc:259: bool > > FillUserNameAndPassword( > > I suggest the following changes to the signature of this function, and of > > FillFormOnPasswordRecieved as well: > > > > (1) Remove |usernames_usage|, use the return value to signal the need for > > updating that (similarly to what I suggest for GetSuggestions). > > > > (2) Because the return value can no longer be used for signalling the > gatekeeper > > registration, and also to avoid the need to call the gatekeeper on each > > callsite, pass a callback argument |password_registration| of type > > base::Callback< void(blink::WebInputElement*)>. Mention in the documentation > > commend for both functions that |password_registration| should be the > gatekeeper > > registration call for the autofilled password field, if any. > > This part (2) I am not able to understood fully, when I will call > RegisterElement() from callback, as PasswordValueGatekeeper class is the private > member of PasswordAutofillAgent class,then we don't have 'this' pointer then > which object data it is modifying?, and this call will not effect the > gatekeeper_ object data. You can bind the address of the gatekeeper object, and the offset to the method to the base::Callback object at the callsite. There should be plenty of examples demonstrating this, e.g., https://code.google.com/p/chromium/codesearch#chromium/src/base/timer/timer.h.... > > and secondly as username_usage_ value can not be decided as true or false for > FillUserNameAndPassword() as we are returning false base on > (current_username != username) also and secondly It is possible that control > does not come to check possible usernames, Then in the end of function I can not > return false,as value of usernames_usage_ is not set. Let's think of the proposed change in two steps: (a) Get rid of the return value of FillUserNameAndPassword completely (returning void). After that, the "else if (current_username != username)" check would only result in an early "return;". (b) Introduce a local variable other_usernames_selected, initialised to false, and only set to true at the place where |usernames_usage| is currently modified. (c) Replace all the early "return;" statements, plus the implicit one at the end of the function, with "return other_usernames_selected;" Note that we can do (a), because the current (in ToT) return value is ignored, and we'll use the callback for the gatekeeper. I believe that if you do the above, you don't have to introduce the enum you present below. Please let me know if I missed something, or if you have more questions. Cheers, Vaclav > > I wanted to purpose that I can use enum value as the retun from these 3 API's > and I will define that enum inside 'namespace autofill',but outside anonymus > namespace in .cc file like. > namespace autofill { > > enum UsernameGatekeeperUsage { > NOTHING_USAGE, > USERNAMES_USAGE, > GATEKEEPER_USAGE, > USERNAMES_AND_GATEKEEPER_USAGE > }; > > namespace { > > > that will fix our issue as we can act base on return value only. > I would like to know opinion for this. > > > > https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... > > components/autofill/content/renderer/password_autofill_agent.cc:349: // such a > > password exists. > > nit: Please describe the meaning of the (new) return value. > > > > > https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... > > components/autofill/content/renderer/password_autofill_agent.cc:471: bool > > gate_keeper = FillUserNameAndPassword(&username, > > (The following is made obsolete by my other comments, but just for learning > > purposes, this would be my comment if the code stayed this way: > > > > nit: Calling this bool |gate_keeper| in the presence of the gatekeeper object > > named |gatekeeper_| is rather confusing. I suggest naming the bool after the > > meaning of the return value, e.g., |password_filled|.) > > > > > https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... > > File components/autofill/content/renderer/password_autofill_agent.h (right): > > > > > https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... > > components/autofill/content/renderer/password_autofill_agent.h:30: enum > > OtherPossibleUsernamesUsage { > > Please keep this private, not public. > > > > Instead, use the return value of the 3 moved functions to indicate whether or > > not other possible usernames were shown/selected. Also please document that > > meaning of the return value.
On 2014/09/26 12:56:07, vabr (Chromium) wrote: > Hi Deepak, > > Please see my answers below. > > > > https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... > > > components/autofill/content/renderer/password_autofill_agent.cc:259: bool > > > FillUserNameAndPassword( > > > I suggest the following changes to the signature of this function, and of > > > FillFormOnPasswordRecieved as well: > > > > > > (1) Remove |usernames_usage|, use the return value to signal the need for > > > updating that (similarly to what I suggest for GetSuggestions). > > > > > > (2) Because the return value can no longer be used for signalling the > > gatekeeper > > > registration, and also to avoid the need to call the gatekeeper on each > > > callsite, pass a callback argument |password_registration| of type > > > base::Callback< void(blink::WebInputElement*)>. Mention in the > documentation > > > commend for both functions that |password_registration| should be the > > gatekeeper > > > registration call for the autofilled password field, if any. > > > > This part (2) I am not able to understood fully, when I will call > > RegisterElement() from callback, as PasswordValueGatekeeper class is the > private > > member of PasswordAutofillAgent class,then we don't have 'this' pointer then > > which object data it is modifying?, and this call will not effect the > > gatekeeper_ object data. > > You can bind the address of the gatekeeper object, and the offset to the method > to the base::Callback object at the callsite. There should be plenty of examples > demonstrating this, e.g., > https://code.google.com/p/chromium/codesearch#chromium/src/base/timer/timer.h.... > > > > > and secondly as username_usage_ value can not be decided as true or false for > > FillUserNameAndPassword() as we are returning false base on > > (current_username != username) also and secondly It is possible that control > > does not come to check possible usernames, Then in the end of function I can > not > > return false,as value of usernames_usage_ is not set. > > Let's think of the proposed change in two steps: > (a) Get rid of the return value of FillUserNameAndPassword completely (returning > void). After that, the "else if (current_username != username)" check would only > result in an early "return;". > (b) Introduce a local variable other_usernames_selected, initialised to false, > and only set to true at the place where |usernames_usage| is currently modified. > (c) Replace all the early "return;" statements, plus the implicit one at the end > of the function, with "return other_usernames_selected;" > > Note that we can do (a), because the current (in ToT) return value is ignored, > and we'll use the callback for the gatekeeper. > I believe that if you do the above, you don't have to introduce the enum you > present below. > > Please let me know if I missed something, or if you have more questions. > > Cheers, > Vaclav > > > > > I wanted to purpose that I can use enum value as the retun from these 3 API's > > and I will define that enum inside 'namespace autofill',but outside anonymus > > namespace in .cc file like. > > namespace autofill { > > > > enum UsernameGatekeeperUsage { > > NOTHING_USAGE, > > USERNAMES_USAGE, > > GATEKEEPER_USAGE, > > USERNAMES_AND_GATEKEEPER_USAGE > > }; > > > > namespace { > > > > > > that will fix our issue as we can act base on return value only. > > I would like to know opinion for this. > > > > > > > > https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... > > > components/autofill/content/renderer/password_autofill_agent.cc:349: // such > a > > > password exists. > > > nit: Please describe the meaning of the (new) return value. > > > > > > > > > https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... > > > components/autofill/content/renderer/password_autofill_agent.cc:471: bool > > > gate_keeper = FillUserNameAndPassword(&username, > > > (The following is made obsolete by my other comments, but just for learning > > > purposes, this would be my comment if the code stayed this way: > > > > > > nit: Calling this bool |gate_keeper| in the presence of the gatekeeper > object > > > named |gatekeeper_| is rather confusing. I suggest naming the bool after the > > > meaning of the return value, e.g., |password_filled|.) > > > > > > > > > https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... > > > File components/autofill/content/renderer/password_autofill_agent.h (right): > > > > > > > > > https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... > > > components/autofill/content/renderer/password_autofill_agent.h:30: enum > > > OtherPossibleUsernamesUsage { > > > Please keep this private, not public. > > > > > > Instead, use the return value of the 3 moved functions to indicate whether > or > > > not other possible usernames were shown/selected. Also please document that > > > meaning of the return value. helper function: bool FillUserNameAndPassword(...,const base::Callback<void(blink::WebInputElement*)>& callback_) { . callback_.Run(password_element); //Calling place } calling site: FillUserNameAndPassword( ,base::Bind(&PasswordAutofillAgent::PasswordValueGatekeeper::RegisterElement)); or FillUserNameAndPassword(..,base::Bind(&gatekeeper.RegisterElement)); Then I am getting compiling error like : ../../components/autofill/content/renderer/password_autofill_agent.cc:486:57: error: cannot create a non-constant pointer to member function base::Bind(&gatekeeper_.RegisterElement)); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../../components/autofill/content/renderer/password_autofill_agent.cc:998:39: error: cannot create a non-constant pointer to member function password_element,base::Bind(&gatekeeper_.RegisterElement)); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../../components/autofill/content/renderer/password_autofill_agent.cc:1099:22: error: no matching function for call to 'FillUserNameAndPassword' bool gate_keeper = FillUserNameAndPassword(&username, ^~~~~~~~~~~~~~~~~~~~~~~ ../../components/autofill/content/renderer/password_autofill_agent.cc:270:6: note: candidate function not viable: no known conversion from 'Callback<typename internal::BindState<typename internal::FunctorTraits<void (PasswordValueGatekeeper::*)(WebInputElement *)>:: RunnableType, typename internal::FunctorTraits<void (PasswordValueGatekeeper::*)(WebInputElement *)>::RunType, void ()>::UnboundRunType>' to 'const Callback<void (blink::WebInputElement *)>' for 6th argument bool FillUserNameAndPassword( ^ 3 errors generated. I think we can not give class::class:memberfunction.any clue or guidance will be useful to me.
On 2014/09/26 15:10:30, Deepak wrote: > On 2014/09/26 12:56:07, vabr (Chromium) wrote: > > Hi Deepak, > > > > Please see my answers below. > > > > > > > > https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... > > > > components/autofill/content/renderer/password_autofill_agent.cc:259: bool > > > > FillUserNameAndPassword( > > > > I suggest the following changes to the signature of this function, and of > > > > FillFormOnPasswordRecieved as well: > > > > > > > > (1) Remove |usernames_usage|, use the return value to signal the need for > > > > updating that (similarly to what I suggest for GetSuggestions). > > > > > > > > (2) Because the return value can no longer be used for signalling the > > > gatekeeper > > > > registration, and also to avoid the need to call the gatekeeper on each > > > > callsite, pass a callback argument |password_registration| of type > > > > base::Callback< void(blink::WebInputElement*)>. Mention in the > > documentation > > > > commend for both functions that |password_registration| should be the > > > gatekeeper > > > > registration call for the autofilled password field, if any. > > > > > > This part (2) I am not able to understood fully, when I will call > > > RegisterElement() from callback, as PasswordValueGatekeeper class is the > > private > > > member of PasswordAutofillAgent class,then we don't have 'this' pointer then > > > which object data it is modifying?, and this call will not effect the > > > gatekeeper_ object data. > > > > You can bind the address of the gatekeeper object, and the offset to the > method > > to the base::Callback object at the callsite. There should be plenty of > examples > > demonstrating this, e.g., > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/timer/timer.h.... > > > > > > > > and secondly as username_usage_ value can not be decided as true or false > for > > > FillUserNameAndPassword() as we are returning false base on > > > (current_username != username) also and secondly It is possible that control > > > does not come to check possible usernames, Then in the end of function I can > > not > > > return false,as value of usernames_usage_ is not set. > > > > Let's think of the proposed change in two steps: > > (a) Get rid of the return value of FillUserNameAndPassword completely > (returning > > void). After that, the "else if (current_username != username)" check would > only > > result in an early "return;". > > (b) Introduce a local variable other_usernames_selected, initialised to false, > > and only set to true at the place where |usernames_usage| is currently > modified. > > (c) Replace all the early "return;" statements, plus the implicit one at the > end > > of the function, with "return other_usernames_selected;" > > > > Note that we can do (a), because the current (in ToT) return value is ignored, > > and we'll use the callback for the gatekeeper. > > I believe that if you do the above, you don't have to introduce the enum you > > present below. > > > > Please let me know if I missed something, or if you have more questions. > > > > Cheers, > > Vaclav > > > > > > > > I wanted to purpose that I can use enum value as the retun from these 3 > API's > > > and I will define that enum inside 'namespace autofill',but outside anonymus > > > namespace in .cc file like. > > > namespace autofill { > > > > > > enum UsernameGatekeeperUsage { > > > NOTHING_USAGE, > > > USERNAMES_USAGE, > > > GATEKEEPER_USAGE, > > > USERNAMES_AND_GATEKEEPER_USAGE > > > }; > > > > > > namespace { > > > > > > > > > that will fix our issue as we can act base on return value only. > > > I would like to know opinion for this. > > > > > > > > > > > > > https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... > > > > components/autofill/content/renderer/password_autofill_agent.cc:349: // > such > > a > > > > password exists. > > > > nit: Please describe the meaning of the (new) return value. > > > > > > > > > > > > > > https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... > > > > components/autofill/content/renderer/password_autofill_agent.cc:471: bool > > > > gate_keeper = FillUserNameAndPassword(&username, > > > > (The following is made obsolete by my other comments, but just for > learning > > > > purposes, this would be my comment if the code stayed this way: > > > > > > > > nit: Calling this bool |gate_keeper| in the presence of the gatekeeper > > object > > > > named |gatekeeper_| is rather confusing. I suggest naming the bool after > the > > > > meaning of the return value, e.g., |password_filled|.) > > > > > > > > > > > > > > https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... > > > > File components/autofill/content/renderer/password_autofill_agent.h > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/597983003/diff/20001/components/autofill/cont... > > > > components/autofill/content/renderer/password_autofill_agent.h:30: enum > > > > OtherPossibleUsernamesUsage { > > > > Please keep this private, not public. > > > > > > > > Instead, use the return value of the 3 moved functions to indicate whether > > or > > > > not other possible usernames were shown/selected. Also please document > that > > > > meaning of the return value. > > > helper function: > bool FillUserNameAndPassword(...,const > base::Callback<void(blink::WebInputElement*)>& callback_) { > . > callback_.Run(password_element); //Calling place > } > > calling site: > FillUserNameAndPassword( > ,base::Bind(&PasswordAutofillAgent::PasswordValueGatekeeper::RegisterElement)); The errors tell you that the signature of the created callback do not match expectations. Indeed, here you create a callback which requires 2 arguments: the address of the gatekeeper (recall that all methods have the implicit 0-th argument |this|), and the password element. What you should be creating here is a 1 argument callback (only the password element). You can do that by passing the address of the gatekeeper as a 2nd argument of the Bind call (see the example I sent you in my last e-mail). > or > FillUserNameAndPassword(..,base::Bind(&gatekeeper.RegisterElement)); > > Then I am getting compiling error like : > > ../../components/autofill/content/renderer/password_autofill_agent.cc:486:57: > error: cannot create a non-constant pointer to member function > > base::Bind(&gatekeeper_.RegisterElement)); > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ../../components/autofill/content/renderer/password_autofill_agent.cc:998:39: > error: cannot create a non-constant pointer to member function > password_element,base::Bind(&gatekeeper_.RegisterElement)); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ../../components/autofill/content/renderer/password_autofill_agent.cc:1099:22: > error: no matching function for call to 'FillUserNameAndPassword' > bool gate_keeper = FillUserNameAndPassword(&username, > ^~~~~~~~~~~~~~~~~~~~~~~ > ../../components/autofill/content/renderer/password_autofill_agent.cc:270:6: > note: candidate function not viable: no known conversion from > 'Callback<typename internal::BindState<typename internal::FunctorTraits<void > (PasswordValueGatekeeper::*)(WebInputElement *)>:: > RunnableType, typename internal::FunctorTraits<void > (PasswordValueGatekeeper::*)(WebInputElement *)>::RunType, void > ()>::UnboundRunType>' to > 'const Callback<void (blink::WebInputElement *)>' for 6th argument > bool FillUserNameAndPassword( > ^ > 3 errors generated. > > I think we can not give class::class:memberfunction.any clue or guidance will be > useful to me.
As Vaclav mentioned, it's not clear to me that this refactoring is a net win. Removing functions from class scope makes it clearer which member functions these classes are touching, but because the member definitions are themselves private (and I think we want to keep them that way) you need to manipulate them indirectly (return values, callbacks). This means the function signatures for the free functions are more complicated (and require better documentation) and it takes more effort to determine how exactly the member functions are modified. We could disagree on which is more comprehensible, but I don't think that it moves the needle much either way. I do agree with the general sentiment of the bug though, which is that PasswordAutofillAgent is large and hard to understand. I think that one of the root causes of this is that it's trying to do two mostly orthagonal things. Mediate loading/saving of passwords and actually displaying these credentials. If we are trying to make this code easier to understand, I think that it's better to try and separate out these two functionalities.
On 2014/09/26 18:31:28, Garrett Casto wrote: > As Vaclav mentioned, it's not clear to me that this refactoring is a net win. > Removing functions from class scope makes it clearer which member functions > these classes are touching, but because the member definitions are themselves > private (and I think we want to keep them that way) you need to manipulate them > indirectly (return values, callbacks). This means the function signatures for > the free functions are more complicated (and require better documentation) and > it takes more effort to determine how exactly the member functions are modified. > We could disagree on which is more comprehensible, but I don't think that it > moves the needle much either way. > > I do agree with the general sentiment of the bug though, which is that > PasswordAutofillAgent is large and hard to understand. I think that one of the > root causes of this is that it's trying to do two mostly orthagonal things. > Mediate loading/saving of passwords and actually displaying these credentials. > If we are trying to make this code easier to understand, I think that it's > better to try and separate out these two functionalities. @vabr, should I go ahead with the changes that you suggested.
In a summary, I'd still recommend going ahead with this. Garrett, this is ultimately up to you, we'd need your approval anyway. If, after reading my responses, you personally prefer not changing this, please just clearly say so, and we can stop this CL. On 2014/09/29 02:42:14, Deepak wrote: > On 2014/09/26 18:31:28, Garrett Casto wrote: > > As Vaclav mentioned, it's not clear to me that this refactoring is a net win. > > Removing functions from class scope makes it clearer which member functions > > these classes are touching, but because the member definitions are themselves > > private (and I think we want to keep them that way) you need to manipulate > them > > indirectly (return values, callbacks). This means the function signatures for > > the free functions are more complicated (and require better documentation) and > > it takes more effort to determine how exactly the member functions are > modified. In the way I suggested, the number of arguments increases by at most one (gatekeeper callback) + return values become bool (at some places there already were so). The documentation is currently partly lacking, IMO, so properly documenting the existing API should not really count as increasing complexity by this change. If the touched member variables were crucial to the core function of the PasswordAutofillAgent, then I would be against moving the methods out of the class. But they touch a UMA signal flag, and a wrapper around executing the command "fill a password field". The main work done by the 3 functions is not bound to the data of PasswordAutofillAgent, so the proposed change makes sense to me. > > We could disagree on which is more comprehensible, but I don't think that it > > moves the needle much either way. > > > > I do agree with the general sentiment of the bug though, which is that > > PasswordAutofillAgent is large and hard to understand. I think that one of the > > root causes of this is that it's trying to do two mostly orthagonal things. I love hearing this, as that's what I was thinking recently as well, but felt I won't push too much changes in the design in a short time. :) I wanted to learn this class properly anyway, so I'll see if I can come up with something in that direction soon. > > Mediate loading/saving of passwords and actually displaying these credentials. > > If we are trying to make this code easier to understand, I think that it's > > better to try and separate out these two functionalities. > > @vabr, should I go ahead with the changes that you suggested. As explained above, my suggestion is to go forward with this, but gcasto@ should have the final say. I'd go with whatever he decides this time, to avoid an endless back and forth. Cheers, Vaclav
On 2014/09/29 08:41:07, vabr (Chromium) wrote: > In a summary, I'd still recommend going ahead with this. > Garrett, this is ultimately up to you, we'd need your approval anyway. > If, after reading my responses, you personally prefer not changing this, please > just clearly say so, and we can stop this CL. > > On 2014/09/29 02:42:14, Deepak wrote: > > On 2014/09/26 18:31:28, Garrett Casto wrote: > > > As Vaclav mentioned, it's not clear to me that this refactoring is a net > win. > > > Removing functions from class scope makes it clearer which member functions > > > these classes are touching, but because the member definitions are > themselves > > > private (and I think we want to keep them that way) you need to manipulate > > them > > > indirectly (return values, callbacks). This means the function signatures > for > > > the free functions are more complicated (and require better documentation) > and > > > it takes more effort to determine how exactly the member functions are > > modified. > In the way I suggested, the number of arguments increases by at most one > (gatekeeper callback) + return values become bool (at some places there already > were so). > > The documentation is currently partly lacking, IMO, so properly documenting the > existing API should not really count as increasing complexity by this change. > > If the touched member variables were crucial to the core function of the > PasswordAutofillAgent, then I would be against moving the methods out of the > class. > But they touch a UMA signal flag, and a wrapper around executing the command > "fill a password field". The main work done by the 3 functions is not bound to > the data of PasswordAutofillAgent, so the proposed change makes sense to me. > > > > We could disagree on which is more comprehensible, but I don't think that it > > > moves the needle much either way. > > > > > > I do agree with the general sentiment of the bug though, which is that > > > PasswordAutofillAgent is large and hard to understand. I think that one of > the > > > root causes of this is that it's trying to do two mostly orthagonal things. > > I love hearing this, as that's what I was thinking recently as well, but felt I > won't push too much changes in the design in a short time. :) > I wanted to learn this class properly anyway, so I'll see if I can come up with > something in that direction soon. > > > > Mediate loading/saving of passwords and actually displaying these > credentials. > > > If we are trying to make this code easier to understand, I think that it's > > > better to try and separate out these two functionalities. > > > > @vabr, should I go ahead with the changes that you suggested. > > As explained above, my suggestion is to go forward with this, but gcasto@ should > have the final say. I'd go with whatever he decides this time, to avoid an > endless back and forth. > > Cheers, > Vaclav I have tried to use the callback,But I can not use callback without making PasswordValueGatekeeper class as public. I refer to http://www.chromium.org/developers/coding-style/cpp-dos-and-donts their it suggest to include implementation on inner class to the .cc file. I have made PasswordValueGatekeeper class forward decleration in '.h' file and used its scoped_ptr. via that I am able to call Register() of PasswordValueGatekeeper class. Please have a look at my changes and let me know your opinion. Thanks
On 2014/09/29 08:41:07, vabr (Chromium) wrote: > In a summary, I'd still recommend going ahead with this. > Garrett, this is ultimately up to you, we'd need your approval anyway. > If, after reading my responses, you personally prefer not changing this, please > just clearly say so, and we can stop this CL. > > On 2014/09/29 02:42:14, Deepak wrote: > > On 2014/09/26 18:31:28, Garrett Casto wrote: > > > As Vaclav mentioned, it's not clear to me that this refactoring is a net > win. > > > Removing functions from class scope makes it clearer which member functions > > > these classes are touching, but because the member definitions are > themselves > > > private (and I think we want to keep them that way) you need to manipulate > > them > > > indirectly (return values, callbacks). This means the function signatures > for > > > the free functions are more complicated (and require better documentation) > and > > > it takes more effort to determine how exactly the member functions are > > modified. > In the way I suggested, the number of arguments increases by at most one > (gatekeeper callback) + return values become bool (at some places there already > were so). > > The documentation is currently partly lacking, IMO, so properly documenting the > existing API should not really count as increasing complexity by this change. > > If the touched member variables were crucial to the core function of the > PasswordAutofillAgent, then I would be against moving the methods out of the > class. > But they touch a UMA signal flag, and a wrapper around executing the command > "fill a password field". The main work done by the 3 functions is not bound to > the data of PasswordAutofillAgent, so the proposed change makes sense to me. > > > > We could disagree on which is more comprehensible, but I don't think that it > > > moves the needle much either way. > > > > > > I do agree with the general sentiment of the bug though, which is that > > > PasswordAutofillAgent is large and hard to understand. I think that one of > the > > > root causes of this is that it's trying to do two mostly orthagonal things. > > I love hearing this, as that's what I was thinking recently as well, but felt I > won't push too much changes in the design in a short time. :) > I wanted to learn this class properly anyway, so I'll see if I can come up with > something in that direction soon. > > > > Mediate loading/saving of passwords and actually displaying these > credentials. > > > If we are trying to make this code easier to understand, I think that it's > > > better to try and separate out these two functionalities. > > > > @vabr, should I go ahead with the changes that you suggested. > > As explained above, my suggestion is to go forward with this, but gcasto@ should > have the final say. I'd go with whatever he decides this time, to avoid an > endless back and forth. > > Cheers, > Vaclav I have tried to use the callback,But I can not use callback without making PasswordValueGatekeeper class as public. I refer to http://www.chromium.org/developers/coding-style/cpp-dos-and-donts their it suggest to include implementation on inner class to the .cc file. I have made PasswordValueGatekeeper class forward decleration in '.h' file and used its scoped_ptr. via that I am able to call Register() of PasswordValueGatekeeper class. Please have a look at my changes and let me know your opinion. Thanks
On 2014/09/29 12:39:32, Deepak wrote:
> On 2014/09/29 08:41:07, vabr (Chromium) wrote:
> > In a summary, I'd still recommend going ahead with this.
> > Garrett, this is ultimately up to you, we'd need your approval anyway.
> > If, after reading my responses, you personally prefer not changing this,
> please
> > just clearly say so, and we can stop this CL.
> >
> > On 2014/09/29 02:42:14, Deepak wrote:
> > > On 2014/09/26 18:31:28, Garrett Casto wrote:
> > > > As Vaclav mentioned, it's not clear to me that this refactoring is a net
> > win.
> > > > Removing functions from class scope makes it clearer which member
> functions
> > > > these classes are touching, but because the member definitions are
> > themselves
> > > > private (and I think we want to keep them that way) you need to
manipulate
> > > them
> > > > indirectly (return values, callbacks). This means the function
signatures
> > for
> > > > the free functions are more complicated (and require better
documentation)
> > and
> > > > it takes more effort to determine how exactly the member functions are
> > > modified.
> > In the way I suggested, the number of arguments increases by at most one
> > (gatekeeper callback) + return values become bool (at some places there
> already
> > were so).
> >
> > The documentation is currently partly lacking, IMO, so properly documenting
> the
> > existing API should not really count as increasing complexity by this
change.
> >
> > If the touched member variables were crucial to the core function of the
> > PasswordAutofillAgent, then I would be against moving the methods out of the
> > class.
> > But they touch a UMA signal flag, and a wrapper around executing the command
> > "fill a password field". The main work done by the 3 functions is not bound
to
> > the data of PasswordAutofillAgent, so the proposed change makes sense to me.
> >
> > > > We could disagree on which is more comprehensible, but I don't think
that
> it
> > > > moves the needle much either way.
> > > >
> > > > I do agree with the general sentiment of the bug though, which is that
> > > > PasswordAutofillAgent is large and hard to understand. I think that one
of
> > the
> > > > root causes of this is that it's trying to do two mostly orthagonal
> things.
> >
> > I love hearing this, as that's what I was thinking recently as well, but
felt
> I
> > won't push too much changes in the design in a short time. :)
> > I wanted to learn this class properly anyway, so I'll see if I can come up
> with
> > something in that direction soon.
> >
> > > > Mediate loading/saving of passwords and actually displaying these
> > credentials.
> > > > If we are trying to make this code easier to understand, I think that
it's
> > > > better to try and separate out these two functionalities.
> > >
> > > @vabr, should I go ahead with the changes that you suggested.
> >
> > As explained above, my suggestion is to go forward with this, but gcasto@
> should
> > have the final say. I'd go with whatever he decides this time, to avoid an
> > endless back and forth.
> >
> > Cheers,
> > Vaclav
>
> I have tried to use the callback,But I can not use callback without making
> PasswordValueGatekeeper class as public.
Where did you create that callback? It needs to be created in a method of
PasswordAutofillAgent (or the gatekeeper itself), then there is no problem with
the private access.
The following proof of concept compiles on the current ToT:
In the anonymous namespace of the .cc file, define
void f(base::Callback<void(blink::WebInputElement*)> callback) {...}
Then you can call |f| inside of PasswordAutofillAgent methods as
f(base::Bind(&PasswordValueGatekeeper::RegisterElement,
base::Unretained(&gatekeeper_)));
> I refer to http://www.chromium.org/developers/coding-style/cpp-dos-and-donts
> their it suggest to include implementation on inner class
> to the .cc file.
> I have made PasswordValueGatekeeper class forward decleration in '.h' file and
> used its scoped_ptr.
> via that I am able to call Register() of PasswordValueGatekeeper class.
> Please have a look at my changes and let me know your opinion.
> Thanks
On 2014/09/29 13:06:29, vabr (Chromium) wrote:
> On 2014/09/29 12:39:32, Deepak wrote:
> > On 2014/09/29 08:41:07, vabr (Chromium) wrote:
> > > In a summary, I'd still recommend going ahead with this.
> > > Garrett, this is ultimately up to you, we'd need your approval anyway.
> > > If, after reading my responses, you personally prefer not changing this,
> > please
> > > just clearly say so, and we can stop this CL.
> > >
> > > On 2014/09/29 02:42:14, Deepak wrote:
> > > > On 2014/09/26 18:31:28, Garrett Casto wrote:
> > > > > As Vaclav mentioned, it's not clear to me that this refactoring is a
net
> > > win.
> > > > > Removing functions from class scope makes it clearer which member
> > functions
> > > > > these classes are touching, but because the member definitions are
> > > themselves
> > > > > private (and I think we want to keep them that way) you need to
> manipulate
> > > > them
> > > > > indirectly (return values, callbacks). This means the function
> signatures
> > > for
> > > > > the free functions are more complicated (and require better
> documentation)
> > > and
> > > > > it takes more effort to determine how exactly the member functions are
> > > > modified.
> > > In the way I suggested, the number of arguments increases by at most one
> > > (gatekeeper callback) + return values become bool (at some places there
> > already
> > > were so).
> > >
> > > The documentation is currently partly lacking, IMO, so properly
documenting
> > the
> > > existing API should not really count as increasing complexity by this
> change.
> > >
> > > If the touched member variables were crucial to the core function of the
> > > PasswordAutofillAgent, then I would be against moving the methods out of
the
> > > class.
> > > But they touch a UMA signal flag, and a wrapper around executing the
command
> > > "fill a password field". The main work done by the 3 functions is not
bound
> to
> > > the data of PasswordAutofillAgent, so the proposed change makes sense to
me.
> > >
> > > > > We could disagree on which is more comprehensible, but I don't think
> that
> > it
> > > > > moves the needle much either way.
> > > > >
> > > > > I do agree with the general sentiment of the bug though, which is that
> > > > > PasswordAutofillAgent is large and hard to understand. I think that
one
> of
> > > the
> > > > > root causes of this is that it's trying to do two mostly orthagonal
> > things.
> > >
> > > I love hearing this, as that's what I was thinking recently as well, but
> felt
> > I
> > > won't push too much changes in the design in a short time. :)
> > > I wanted to learn this class properly anyway, so I'll see if I can come up
> > with
> > > something in that direction soon.
> > >
> > > > > Mediate loading/saving of passwords and actually displaying these
> > > credentials.
> > > > > If we are trying to make this code easier to understand, I think that
> it's
> > > > > better to try and separate out these two functionalities.
> > > >
> > > > @vabr, should I go ahead with the changes that you suggested.
> > >
> > > As explained above, my suggestion is to go forward with this, but gcasto@
> > should
> > > have the final say. I'd go with whatever he decides this time, to avoid an
> > > endless back and forth.
> > >
> > > Cheers,
> > > Vaclav
> >
> > I have tried to use the callback,But I can not use callback without making
> > PasswordValueGatekeeper class as public.
>
> Where did you create that callback? It needs to be created in a method of
> PasswordAutofillAgent (or the gatekeeper itself), then there is no problem
with
> the private access.
> The following proof of concept compiles on the current ToT:
> In the anonymous namespace of the .cc file, define
> void f(base::Callback<void(blink::WebInputElement*)> callback) {...}
> Then you can call |f| inside of PasswordAutofillAgent methods as
> f(base::Bind(&PasswordValueGatekeeper::RegisterElement,
> base::Unretained(&gatekeeper_)));
>
>
> > I refer to http://www.chromium.org/developers/coding-style/cpp-dos-and-donts
> > their it suggest to include implementation on inner class
> > to the .cc file.
> > I have made PasswordValueGatekeeper class forward decleration in '.h' file
and
> > used its scoped_ptr.
> > via that I am able to call Register() of PasswordValueGatekeeper class.
> > Please have a look at my changes and let me know your opinion.
> > Thanks
Thanks vabr for your valuable help,It will be great learning for me.
Now I am using callback for calling Register() from helper function.
PTAL
Thanks, Deepak! Your patch set 4 (https://codereview.chromium.org/597983003/#ps60001) LGTM, with some minor comments about the code comments. As long as you address my comments below, you don't have to wait for my further approval. But, please wait for Garrett's decision, whether he wants to stop this or not. Cheers, Vaclav https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:229: // fill_data based on the |input| and return true when suggestion typo: suggestion -> suggestions https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:230: // get filled from usernamescollection else return false. nit: Not clear what is meant by the username collection here. Maybe just write "from |fill_data|" instead? https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:270: // returns true when username get selected from possible usernames typo: "username get" -> "a username gets" https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:271: // else returns false.it will also register the password element when nit: "false.it" -> "false. It" https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:271: // else returns false.it will also register the password element when nit: Specify that it will register the password element by passing it to the |gatekeeper_callback|. Or rename |gatekeeper_callback| to |registration_callback|. https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:370: // such a password exists.gatekeeper_ptr will register the password element. nit: space after a full-stop https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:370: // such a password exists.gatekeeper_ptr will register the password element. nit: gatekeeper_ptr sounds like a typo. Maybe say here the same what you'll say about the callback for the function above. https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:503: usernames_usage_ = OTHER_POSSIBLE_USERNAME_SELECTED; nit: Please add curly braces around the statement, because the if condition spans more lines. Also at the modified if-blocks below.
LGTM after all comments are addressed. I still think that the benefits of the actual code refactoring is marginal in this case, but I can see how this might be clearer. The more descriptive documentation makes it worthwhile regardless. https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:228: // This helper function will fill the |suggestions| and |realms| form the "fill the |suggestions| and |realms| form the fill_data", I think you mean "will fill |suggestions| and |realms| from |fill_data|". https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:229: // fill_data based on the |input| and return true when suggestion Just "|input|" not "the |input|". "Input" is also not very descriptive. Maybe rename to something like "current_username"? Also break up the sentence at that point and start a new sentence for "Return true ...". https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:230: // get filled from usernamescollection else return false. On 2014/09/29 15:19:09, vabr (Chromium) wrote: > nit: Not clear what is meant by the username collection here. Maybe just write > "from |fill_data|" instead? From |fill_data| isn't specific, all suggestions are coming from fill_data. The function only returns true when data filled is from fill_data.other_possible_usernames. You should specify this instead of filled from PasswordFormFillData::UsernamesCollection, as the type that contains other_possible_usernames isn't particularly relevant. https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:269: // and |password_element| values with the |fill_data| Line wrapping too early. You should wrap closer to 80 chars (approximately when the next word will put you over 80 chars). https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:270: // returns true when username get selected from possible usernames I think there is supposed to be a period and "returns" should be capitalized. https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:280: bool username_selected = false; |username_selected| is too vague. It's not just if any username is selected, just an other_possible_username. https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:366: // Attempts to fill |username_element| and |password_element| with the As above, be consistent with pronouns. It's more common to refer to parameters without "the", just "|fill_data|". https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:371: // returns true when username is selected else return false. Username is selected is misleading, it's just other_possible_username. Also capitalize "Returns" and add a comma after "selected".
Thanks vabr & Garrett for your time & guidance for enhancing my learning. I have address all the suggestions you mentioned. PTAL. https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:228: // This helper function will fill the |suggestions| and |realms| form the On 2014/09/30 00:16:41, Garrett Casto wrote: > "fill the |suggestions| and |realms| form the fill_data", I think you mean "will > fill |suggestions| and |realms| from |fill_data|". Done. https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:229: // fill_data based on the |input| and return true when suggestion On 2014/09/29 15:19:09, vabr (Chromium) wrote: > typo: suggestion -> suggestions Done. https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:229: // fill_data based on the |input| and return true when suggestion On 2014/09/30 00:16:40, Garrett Casto wrote: > Just "|input|" not "the |input|". "Input" is also not very descriptive. Maybe > rename to something like "current_username"? Also break up the sentence at that > point and start a new sentence for "Return true ...". Done. https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:230: // get filled from usernamescollection else return false. On 2014/09/29 15:19:09, vabr (Chromium) wrote: > nit: Not clear what is meant by the username collection here. Maybe just write > "from |fill_data|" instead? Done. https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:230: // get filled from usernamescollection else return false. On 2014/09/30 00:16:40, Garrett Casto wrote: > On 2014/09/29 15:19:09, vabr (Chromium) wrote: > > nit: Not clear what is meant by the username collection here. Maybe just write > > "from |fill_data|" instead? > > From |fill_data| isn't specific, all suggestions are coming from fill_data. The > function only returns true when data filled is from > fill_data.other_possible_usernames. You should specify this instead of filled > from PasswordFormFillData::UsernamesCollection, as the type that contains > other_possible_usernames isn't particularly relevant. I have made this like: return true when suggestions get filled from other_possible_usernames of |fill_data| else return false. This will address both. https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:269: // and |password_element| values with the |fill_data| On 2014/09/30 00:16:41, Garrett Casto wrote: > Line wrapping too early. You should wrap closer to 80 chars (approximately when > the next word will put you over 80 chars). Acknowledged. https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:270: // returns true when username get selected from possible usernames On 2014/09/29 15:19:10, vabr (Chromium) wrote: > typo: "username get" -> "a username gets" Done. https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:270: // returns true when username get selected from possible usernames On 2014/09/30 00:16:40, Garrett Casto wrote: > I think there is supposed to be a period and "returns" should be capitalized. Done. https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:271: // else returns false.it will also register the password element when On 2014/09/29 15:19:10, vabr (Chromium) wrote: > nit: Specify that it will register the password element by passing it to the > |gatekeeper_callback|. Or rename |gatekeeper_callback| to > |registration_callback|. Done. https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:280: bool username_selected = false; On 2014/09/30 00:16:41, Garrett Casto wrote: > |username_selected| is too vague. It's not just if any username is selected, > just an other_possible_username. I have made this "possible_username_selected" to make it distinct that other possible username is selected. https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:370: // such a password exists.gatekeeper_ptr will register the password element. On 2014/09/29 15:19:09, vabr (Chromium) wrote: > nit: gatekeeper_ptr sounds like a typo. Maybe say here the same what you'll say > about the callback for the function above. Done. https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:370: // such a password exists.gatekeeper_ptr will register the password element. On 2014/09/29 15:19:10, vabr (Chromium) wrote: > nit: space after a full-stop Done. https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:371: // returns true when username is selected else return false. On 2014/09/30 00:16:41, Garrett Casto wrote: > Username is selected is misleading, it's just other_possible_username. Also > capitalize "Returns" and add a comma after "selected". Done. https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:503: usernames_usage_ = OTHER_POSSIBLE_USERNAME_SELECTED; On 2014/09/29 15:19:10, vabr (Chromium) wrote: > nit: Please add curly braces around the statement, because the if condition > spans more lines. Also at the modified if-blocks below. Done.
Hi Deepak, Thanks for your work. Please find some more comments from me below. You seem to be creating some trivial errors as you fix others -- before your next patch set, please do take a short break before sending it out, then read it again, as if it was written by someone else, and try to catch some obvious mistakes. That will likely spare you some more reviewing cycles. Cheers, Vaclav https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:228: // This helper function will fill the |suggestions| and |realms| form the On 2014/09/30 04:07:21, Deepak wrote: > On 2014/09/30 00:16:41, Garrett Casto wrote: > > "fill the |suggestions| and |realms| form the fill_data", I think you mean > "will > > fill |suggestions| and |realms| from |fill_data|". > > Done. The form->from typo seems to be still present. :) https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:229: // fill_data based on the |input| and return true when suggestion On 2014/09/30 04:07:21, Deepak wrote: > On 2014/09/30 00:16:40, Garrett Casto wrote: > > Just "|input|" not "the |input|". "Input" is also not very descriptive. Maybe > > rename to something like "current_username"? Also break up the sentence at > that > > point and start a new sentence for "Return true ...". > > Done. I believe Garrett meant renaming the argument |input| to |current_username|, rather than specifying that only in the comment. Also, "break up a sentence" is not the same as "break a line" -- please follow the comment of Garrett, and use as much space on comment lines, as is permitted by the 80 character length limit without breaking words in the middle. (Tip: write your comments on a single line, ignoring the length limit, and then run git cl format. It will break the lines in the comments for you.) https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:230: // get filled from usernamescollection else return false. On 2014/09/30 04:07:21, Deepak wrote: > On 2014/09/30 00:16:40, Garrett Casto wrote: > > On 2014/09/29 15:19:09, vabr (Chromium) wrote: > > > nit: Not clear what is meant by the username collection here. Maybe just > write > > > "from |fill_data|" instead? > > > > From |fill_data| isn't specific, all suggestions are coming from fill_data. > The > > function only returns true when data filled is from > > fill_data.other_possible_usernames. You should specify this instead of filled > > from PasswordFormFillData::UsernamesCollection, as the type that contains > > other_possible_usernames isn't particularly relevant. > > I have made this like: > return true when suggestions get filled from other_possible_usernames > of |fill_data| else return false. > > This will address both. 2 nits: (1) Replace other_possible_usernames with |fill_data.other_possible_usernames|, to be precise. (2) "Return" -> "Returns" -- to be consistent with the previous sentence which speaks about the function in 3rd person. https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:280: bool username_selected = false; On 2014/09/30 04:07:21, Deepak wrote: > On 2014/09/30 00:16:41, Garrett Casto wrote: > > |username_selected| is too vague. It's not just if any username is selected, > > just an other_possible_username. > > I have made this "possible_username_selected" to make it distinct that other > possible username is selected. I would actually suggest |other_possible_username_selected|, to make it clear it relates to the |other_possible_usernames| field. It's rather long, but should not cause formatting issues. https://codereview.chromium.org/597983003/diff/80001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/597983003/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:269: // This function attempts to set |username_element| and |password_element| "set" -> "fill" -- to be consistent with the function name. Please consider replacing the whole comment with the suggestion below, fixing that and a number of other unclear points and typos: This function attempts to fill |username_element| and |password_element| with values from |fill_data|. The |password_element| will only have the |suggestedValue| set, and will be registered for copying that to the real value through |registration_callback|. The function returns true when selected username comes from |fill_data.other_possible_usernames|. https://codereview.chromium.org/597983003/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:367: // |fill_data|will use the data corresponding to the preferred username, Why did you remove the full stop and capital W? https://codereview.chromium.org/597983003/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:368: // unless |username_element| already has a value set.In that case, Always include a space after the full stop. (Here and also 2 lines below you don't.) https://codereview.chromium.org/597983003/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:369: // attempts to fill password matching already filled username, if such a password -> the password already filled username -> the already filled username https://codereview.chromium.org/597983003/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:370: // password exists.registration_callback will register the password element End each sentence with a full stop. https://codereview.chromium.org/597983003/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:370: // password exists.registration_callback will register the password element The sentence explaining what |registration_callback| (do use the pipes to mark this as a code text) does is still a bit vague. Try to use something like I suggested for FillUserNameAndPassword. https://codereview.chromium.org/597983003/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:371: // Return true when username get selected from other_possible_usernames, else Return -> Returns get -> gets other_possible_usernames -> |other_possible_usernames| https://codereview.chromium.org/597983003/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:372: // return false. return -> returns
PTAL https://codereview.chromium.org/597983003/diff/80001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/597983003/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:269: // This function attempts to set |username_element| and |password_element| On 2014/09/30 08:19:41, vabr (Chromium) wrote: > "set" -> "fill" -- to be consistent with the function name. > Please consider replacing the whole comment with the suggestion below, fixing > that and a number of other unclear points and typos: > > This function attempts to fill |username_element| and |password_element| with > values from |fill_data|. The |password_element| will only have the > |suggestedValue| set, and will be registered for copying that to the real value > through |registration_callback|. The function returns true when selected > username comes from |fill_data.other_possible_usernames|. Done. https://codereview.chromium.org/597983003/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:367: // |fill_data|will use the data corresponding to the preferred username, On 2014/09/30 08:19:41, vabr (Chromium) wrote: > Why did you remove the full stop and capital W? Actually I got comment in other review that I should use all small letters in the commnets except starting one. https://codereview.chromium.org/597983003/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:368: // unless |username_element| already has a value set.In that case, On 2014/09/30 08:19:41, vabr (Chromium) wrote: > Always include a space after the full stop. (Here and also 2 lines below you > don't.) Done. https://codereview.chromium.org/597983003/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:369: // attempts to fill password matching already filled username, if such a On 2014/09/30 08:19:41, vabr (Chromium) wrote: > password -> the password > already filled username -> the already filled username Done. https://codereview.chromium.org/597983003/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:370: // password exists.registration_callback will register the password element On 2014/09/30 08:19:41, vabr (Chromium) wrote: > The sentence explaining what |registration_callback| (do use the pipes to mark > this as a code text) does is still a bit vague. Try to use something like I > suggested for FillUserNameAndPassword. Done. https://codereview.chromium.org/597983003/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:370: // password exists.registration_callback will register the password element On 2014/09/30 08:19:41, vabr (Chromium) wrote: > End each sentence with a full stop. Done. https://codereview.chromium.org/597983003/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:371: // Return true when username get selected from other_possible_usernames, else On 2014/09/30 08:19:41, vabr (Chromium) wrote: > Return -> Returns > get -> gets > other_possible_usernames -> |other_possible_usernames| Done. https://codereview.chromium.org/597983003/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:372: // return false. On 2014/09/30 08:19:41, vabr (Chromium) wrote: > return -> returns Done.
Hi Deepak, Another round of comments below. Cheers, Vaclav https://codereview.chromium.org/597983003/diff/80001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/597983003/diff/80001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:367: // |fill_data|will use the data corresponding to the preferred username, On 2014/09/30 11:54:57, Deepak wrote: > On 2014/09/30 08:19:41, vabr (Chromium) wrote: > > Why did you remove the full stop and capital W? > > Actually I got comment in other review that I should use all small letters in > the commnets except starting one. I doubt anybody would recommend starting a sentence with a lowercase letter, and in any case I (and the standards for written English) would oppose that. Every sentence deserves a capital first letter. https://codereview.chromium.org/597983003/diff/180001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/597983003/diff/180001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:229: // |fill_data| based on |input| that is current_username. Please use the full length of the lines in this comment block. https://codereview.chromium.org/597983003/diff/180001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:372: // it will be registered for copying that to the real value through Here "that" is unclear; and using just "value" above this sentence sounds like speaking about replacing the element value with the very same value. https://codereview.chromium.org/597983003/diff/180001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:374: // Returns true when username gets selected from |other_possible_usernames|, No need to break the line before "Returns". https://codereview.chromium.org/597983003/diff/180001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:374: // Returns true when username gets selected from |other_possible_usernames|, Add "the" before "username" (here "username" is not a name of a variable).
PTAL https://codereview.chromium.org/597983003/diff/180001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/597983003/diff/180001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:229: // |fill_data| based on |input| that is current_username. On 2014/09/30 12:25:55, vabr (Chromium) wrote: > Please use the full length of the lines in this comment block. Done. https://codereview.chromium.org/597983003/diff/180001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:372: // it will be registered for copying that to the real value through On 2014/09/30 12:25:54, vabr (Chromium) wrote: > Here "that" is unclear; and using just "value" above this sentence sounds like > speaking about replacing the element value with the very same value. I have changes context to |suggestedValue|: // Attempts to fill |username_element| and |password_element| with // |fill_data|. Will use the data corresponding to the preferred username, // unless |username_element| already has a value set. In that case, // attempts to fill the password matching the already filled username, // if such a password exists. commnet was present earlier, I have taken that from .h file. https://codereview.chromium.org/597983003/diff/180001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:374: // Returns true when username gets selected from |other_possible_usernames|, On 2014/09/30 12:25:54, vabr (Chromium) wrote: > No need to break the line before "Returns". Done. https://codereview.chromium.org/597983003/diff/180001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:374: // Returns true when username gets selected from |other_possible_usernames|, On 2014/09/30 12:25:54, vabr (Chromium) wrote: > Add "the" before "username" (here "username" is not a name of a variable). Done.
Hi Deepak, I found two earlier comments which were not completely addressed. Please have a look. Vaclav https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:280: bool username_selected = false; On 2014/09/30 08:19:41, vabr (Chromium) wrote: > On 2014/09/30 04:07:21, Deepak wrote: > > On 2014/09/30 00:16:41, Garrett Casto wrote: > > > |username_selected| is too vague. It's not just if any username is selected, > > > just an other_possible_username. > > > > I have made this "possible_username_selected" to make it distinct that other > > possible username is selected. > > I would actually suggest |other_possible_username_selected|, to make it clear it > relates to the |other_possible_usernames| field. It's rather long, but should > not cause formatting issues. You seem to have missed this comment as well. https://codereview.chromium.org/597983003/diff/220001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/597983003/diff/220001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:229: // based on |input| that is current_username. Returns true when |suggestions| I don't think you addressed the issue with |current_username| (see my comment at https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont...). In particular, "current_username" is not a defined object (yet). As suggested, you should rename |input| to |current_username|.
On 2014/09/30 13:54:54, vabr (Chromium) wrote: > Hi Deepak, > > I found two earlier comments which were not completely addressed. Please have a > look. > > Vaclav > > https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... > File components/autofill/content/renderer/password_autofill_agent.cc (right): > > https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont... > components/autofill/content/renderer/password_autofill_agent.cc:280: bool > username_selected = false; > On 2014/09/30 08:19:41, vabr (Chromium) wrote: > > On 2014/09/30 04:07:21, Deepak wrote: > > > On 2014/09/30 00:16:41, Garrett Casto wrote: > > > > |username_selected| is too vague. It's not just if any username is > selected, > > > > just an other_possible_username. > > > > > > I have made this "possible_username_selected" to make it distinct that other > > > possible username is selected. > > > > I would actually suggest |other_possible_username_selected|, to make it clear > it > > relates to the |other_possible_usernames| field. It's rather long, but should > > not cause formatting issues. > > You seem to have missed this comment as well. > > https://codereview.chromium.org/597983003/diff/220001/components/autofill/con... > File components/autofill/content/renderer/password_autofill_agent.cc (right): > > https://codereview.chromium.org/597983003/diff/220001/components/autofill/con... > components/autofill/content/renderer/password_autofill_agent.cc:229: // based on > |input| that is current_username. Returns true when |suggestions| > I don't think you addressed the issue with |current_username| (see my comment at > https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont...). > In particular, "current_username" is not a defined object (yet). As suggested, > you should rename |input| to |current_username|. Both the issues addressed. |input| -> |current_username| username_selected to other_possible_username_selected username_shown to other_possible_username_shown changes done. PTAL
PTAL https://codereview.chromium.org/597983003/diff/220001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/597983003/diff/220001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:229: // based on |input| that is current_username. Returns true when |suggestions| On 2014/09/30 13:54:53, vabr (Chromium) wrote: > I don't think you addressed the issue with |current_username| (see my comment at > https://codereview.chromium.org/597983003/diff/60001/components/autofill/cont...). > In particular, "current_username" is not a defined object (yet). As suggested, > you should rename |input| to |current_username|. Done.
Thank you, Deepak, LGTM. I left one last comment, but that's just line breaking, and should be straightforward to implement. Once you do that, if that's the only change you make, you don't have to wait for another review from me. Also, it looks like you addressed all comments from Garrett, so you should be free to land this. Thank you for your work, Vaclav https://codereview.chromium.org/597983003/diff/240001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/597983003/diff/240001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:229: // based on |current_username|. Returns true when |suggestions| Please use all available space for the line.
Changes done. It is a good learning experience for me, as I get more familiar with review comments and expectations. I will use this learning in my future patches. Thanks vabr for your time and help. https://codereview.chromium.org/597983003/diff/240001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/597983003/diff/240001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:229: // based on |current_username|. Returns true when |suggestions| On 2014/09/30 14:46:45, vabr (Chromium) wrote: > Please use all available space for the line. Done.
The CQ bit was checked by deepak.m1@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/597983003/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by deepak.m1@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/597983003/260001
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as bc4f96366314d7287fc4071c025767c63de726f8
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/b7db6e91ad6f817fe62a3e29b3fdc2018d27fcec Cr-Commit-Position: refs/heads/master@{#297586} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
