Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9600038:
Add Password Autofill Manager to New Autofill (Closed)
|
Created:
8 years, 9 months ago by csharp Modified:
8 years, 8 months ago Reviewers:
Ilya Sherman CC:
chromium-reviews, GeorgeY, dhollowa+watch_chromium.org, darin-cc_chromium.org, dyu1, brettw-cc_chromium.org, Ilya Sherman Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdd Password Autofill Manager to New Autofill
Copied over the password Autofill Manager functions from the renderer to the browser to allow the new Autofill UI to have access to them.
BUG=51644
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=130824
Patch Set 1 : #
Total comments: 26
Patch Set 2 : Responding to comments #
Total comments: 3
Patch Set 3 : Hiding renderer popup #
Total comments: 26
Patch Set 4 : Remove frame id #
Total comments: 10
Patch Set 5 : Fixing up login password clearing #
Total comments: 8
Patch Set 6 : #
Total comments: 26
Patch Set 7 : Adding password popup #
Total comments: 16
Patch Set 8 : #
Total comments: 7
Patch Set 9 : #Patch Set 10 : Fixing up FindLoginInfo #
Total comments: 18
Patch Set 11 : Fixing final nits #Messages
Total messages: 34 (0 generated)
First attempt at moving the required password manager code from the renderer to the browser. The basic functionality seems to be in but there still seem to be a few bugs (not sure if they are caused by this new code or just surfaced by it). https://chromiumcodereview.appspot.com/9600038/diff/3001/webkit/forms/form_fi... File webkit/forms/form_field.h (right): https://chromiumcodereview.appspot.com/9600038/diff/3001/webkit/forms/form_fi... webkit/forms/form_field.h:28: bool operator<(const FormField& field) const; Not sure how best to deal with this. Using the map in the manager needs some sorting ability so adding a comparison function would be the right move, but I'm not 100% sure I need the map (maybe just a vector instead). Thoughts? We need some sorting to be able to use the map in the password autofill manager, should I change this into a comparison function or try and use something other than a map in the manager?
On 2012/03/06 16:09:23, csharp wrote: > The basic functionality seems to be in but there still seem to be a few bugs > (not sure if they are caused by this new code or just surfaced by it). Can you elaborate a bit? :) https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/browser/autof... File chrome/browser/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/browser/autof... chrome/browser/autofill/password_autofill_manager.cc:32: tab_contents_wrapper_->web_contents()->GetRenderViewHost()) { Hmm, can any of these actually ever be NULL? https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/browser/autof... chrome/browser/autofill/password_autofill_manager.cc:40: (key_code == ui::VKEY_BACK || key_code == ui::VKEY_DELETE); It looks like you're also doing this work on the renderer side. Why do we need to do it in both places? https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/browser/autof... chrome/browser/autofill/password_autofill_manager.cc:60: value)); Why not send the full fill data here, rather than having the renderer side redo the work? https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/browser/autof... chrome/browser/autofill/password_autofill_manager.cc:100: } Does anything go wrong if we just empty the map whenever the frame containing the to-be-autofilled element closes? https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/browser/autof... chrome/browser/autofill/password_autofill_manager.cc:121: } nit: The if-stmt body is 1 line, so no need for curlys. https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/browser/autof... File chrome/browser/autofill/password_autofill_manager.h (right): https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/browser/autof... chrome/browser/autofill/password_autofill_manager.h:12: // Once the move is completed the repeated code in the render half should be nit: "render" -> "renderer". Also, please annotate this comment with a link to the crbug. https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/browser/autof... chrome/browser/autofill/password_autofill_manager.h:54: void FrameClosing(long long frame_id); Why "long long"? https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/renderer/auto... File chrome/renderer/autofill/autofill_agent.h (right): https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/renderer/auto... chrome/renderer/autofill/autofill_agent.h:113: void OnPasswordHandleKeyDown(int key_code); Hmm, why can't we just let the key event bubble naturally by not handling it on the browser side? https://chromiumcodereview.appspot.com/9600038/diff/3001/webkit/forms/form_fi... File webkit/forms/form_field.h (right): https://chromiumcodereview.appspot.com/9600038/diff/3001/webkit/forms/form_fi... webkit/forms/form_field.h:28: bool operator<(const FormField& field) const; On 2012/03/06 16:09:23, csharp wrote: > Not sure how best to deal with this. Using the map in the manager needs some > sorting ability so adding a comparison function would be the right move, but I'm > not 100% sure I need the map (maybe just a vector instead). Thoughts? > > We need some sorting to be able to use the map in the password autofill manager, > should I change this into a comparison function or try and use something other > than a map in the manager? This seems fine, but please add a comment that this operator is exposed for the sake of using an STL map.
Ok, I dug a bit more into the weird failures I was getting and figured out the problem. I thought I was properly showing my new popup, but I wasn't, it was just the old one. The problem is that it seems after a page is load, the renderer requests and gets all password from the browser right away. Unlike the normal autofill there doesn't seems to be a call for data after it decides to show a popup that I can take to prevent the popup from showing. Since the code stays in the renderer right now I think the best option might be to add an IPC to let the renderer password manager know there is an external delegate and then a simple if in PasswordAutofillManager::ShowSuggestionPopup that send an IPC call to show the popup if there is a delegate and otherwise it stays with the default behaviour. I don't really like the renderer needing to know about the external delegate but I can't see any better way to handle this case, since we need to hide the popup. Thoughts? https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/browser/autof... File chrome/browser/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/browser/autof... chrome/browser/autofill/password_autofill_manager.cc:32: tab_contents_wrapper_->web_contents()->GetRenderViewHost()) { On 2012/03/06 20:32:44, Ilya Sherman wrote: > Hmm, can any of these actually ever be NULL? I don't think so, but I since I only use this to send IPC messages I pass in a NULL pointer in the unit tests. Is there a better way to do this? Should I just take in a pointer to RenderViewHost instead of TabContentsWrapper in the constructor and then just make sure it isn't NULL here? https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/browser/autof... chrome/browser/autofill/password_autofill_manager.cc:40: (key_code == ui::VKEY_BACK || key_code == ui::VKEY_DELETE); On 2012/03/06 20:32:44, Ilya Sherman wrote: > It looks like you're also doing this work on the renderer side. Why do we need > to do it in both places? I had thought I still had code here that looked at the backspace_pressed_last value but that seems to have been moved out. Removing the handling from here. https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/browser/autof... chrome/browser/autofill/password_autofill_manager.cc:60: value)); On 2012/03/06 20:32:44, Ilya Sherman wrote: > Why not send the full fill data here, rather than having the renderer side redo > the work? We kinda get the full fill data for free in the renderer, since the the PasswordInfo class here is fairly different from the one in the renderer, it lacks a few variables uses webkit::forms:FormField instead of WebKit::WebInputElement. This means that we need to call FindLoginInfo to get those missing variables anyhow, and the fill data is stored with them so we still get it. https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/browser/autof... chrome/browser/autofill/password_autofill_manager.cc:100: } On 2012/03/06 20:32:44, Ilya Sherman wrote: > Does anything go wrong if we just empty the map whenever the frame containing > the to-be-autofilled element closes? From my understanding of this code, I don't think so. But this is how it is done in the renderer side, so I assume there is a good reason for doing it this way (maybe when there are multiple frames on the same page?). https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/browser/autof... chrome/browser/autofill/password_autofill_manager.cc:121: } On 2012/03/06 20:32:44, Ilya Sherman wrote: > nit: The if-stmt body is 1 line, so no need for curlys. Done. https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/browser/autof... File chrome/browser/autofill/password_autofill_manager.h (right): https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/browser/autof... chrome/browser/autofill/password_autofill_manager.h:12: // Once the move is completed the repeated code in the render half should be On 2012/03/06 20:32:44, Ilya Sherman wrote: > nit: "render" -> "renderer". Also, please annotate this comment with a link to > the crbug. Done. https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/browser/autof... chrome/browser/autofill/password_autofill_manager.h:54: void FrameClosing(long long frame_id); On 2012/03/06 20:32:44, Ilya Sherman wrote: > Why "long long"? WebFrame's unique identifier is a long long. https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/renderer/auto... File chrome/renderer/autofill/autofill_agent.h (right): https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/renderer/auto... chrome/renderer/autofill/autofill_agent.h:113: void OnPasswordHandleKeyDown(int key_code); On 2012/03/06 20:32:44, Ilya Sherman wrote: > Hmm, why can't we just let the key event bubble naturally by not handling it on > the browser side? Done. https://chromiumcodereview.appspot.com/9600038/diff/3001/webkit/forms/form_fi... File webkit/forms/form_field.h (right): https://chromiumcodereview.appspot.com/9600038/diff/3001/webkit/forms/form_fi... webkit/forms/form_field.h:28: bool operator<(const FormField& field) const; On 2012/03/06 20:32:44, Ilya Sherman wrote: > On 2012/03/06 16:09:23, csharp wrote: > > Not sure how best to deal with this. Using the map in the manager needs some > > sorting ability so adding a comparison function would be the right move, but > I'm > > not 100% sure I need the map (maybe just a vector instead). Thoughts? > > > > We need some sorting to be able to use the map in the password autofill > manager, > > should I change this into a comparison function or try and use something other > > than a map in the manager? > > This seems fine, but please add a comment that this operator is exposed for the > sake of using an STL map. Done.
On 2012/03/07 15:49:16, csharp wrote: > Ok, I dug a bit more into the weird failures I was getting and figured out the > problem. > > I thought I was properly showing my new popup, but I wasn't, it was just the > old one. The problem is that it seems after a page is load, the renderer > requests and gets all password from the browser right away. Unlike the normal > autofill there doesn't seems to be a call for data after it decides to show a > popup that I can take to prevent the popup from showing. > > Since the code stays in the renderer right now I think the best option might > be to add an IPC to let the renderer password manager know there is an > external delegate and then a simple if in > PasswordAutofillManager::ShowSuggestionPopup that send an IPC call to show the > popup if there is a delegate and otherwise it stays with the default behavior. > > I don't really like the renderer needing to know about the external delegate > but I can't see any better way to handle this case, since we need to hide the > popup. > > Thoughts? If we decide to keep the logic that sends the password data to the renderer, perhaps we should add a boolean to that IPC call that tells the renderer whether or not it should be showing a popup. It does feel a little hacky to have the renderer know about the external popup; but it's not too hacky, and is probably cleaner than other options. https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/browser/autof... File chrome/browser/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/browser/autof... chrome/browser/autofill/password_autofill_manager.cc:32: tab_contents_wrapper_->web_contents()->GetRenderViewHost()) { On 2012/03/07 15:49:16, csharp wrote: > On 2012/03/06 20:32:44, Ilya Sherman wrote: > > Hmm, can any of these actually ever be NULL? > > I don't think so, but I since I only use this to send IPC messages I pass in a > NULL pointer in the unit tests. > > Is there a better way to do this? Should I just take in a pointer to > RenderViewHost instead of TabContentsWrapper in the constructor and then just > make sure it isn't NULL here? If the RenderViewHost remains constant for a TabContentsWrapper, it would definitely be better to take a pointer to that instead, in part to more narrowly define the class dependency. https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/browser/autof... chrome/browser/autofill/password_autofill_manager.cc:60: value)); On 2012/03/07 15:49:16, csharp wrote: > On 2012/03/06 20:32:44, Ilya Sherman wrote: > > Why not send the full fill data here, rather than having the renderer side > redo > > the work? > > We kinda get the full fill data for free in the renderer, since the the > PasswordInfo class here is fairly different from the one in the renderer, it > lacks a few variables uses webkit::forms:FormField instead of > WebKit::WebInputElement. > > This means that we need to call FindLoginInfo to get those missing variables > anyhow, and the fill data is stored with them so we still get it. Hmm, it seems a little wrong to have both the browser and the renderer tracking this info, though. What are the missing bits from the browser side that the renderer has access to? https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/browser/autof... File chrome/browser/autofill/password_autofill_manager.h (right): https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/browser/autof... chrome/browser/autofill/password_autofill_manager.h:54: void FrameClosing(long long frame_id); On 2012/03/07 15:49:16, csharp wrote: > On 2012/03/06 20:32:44, Ilya Sherman wrote: > > Why "long long"? > > WebFrame's unique identifier is a long long. In that case, why "int" above? https://chromiumcodereview.appspot.com/9600038/diff/8001/chrome/browser/autof... File chrome/browser/autofill/password_autofill_manager.h (right): https://chromiumcodereview.appspot.com/9600038/diff/8001/chrome/browser/autof... chrome/browser/autofill/password_autofill_manager.h:32: bool WouldHandleKeyDown(const webkit::forms::FormField& field); I didn't see any place where this method is used. Can we just get rid of it?
https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/browser/autof... File chrome/browser/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/browser/autof... chrome/browser/autofill/password_autofill_manager.cc:32: tab_contents_wrapper_->web_contents()->GetRenderViewHost()) { On 2012/03/07 22:33:18, Ilya Sherman wrote: > On 2012/03/07 15:49:16, csharp wrote: > > On 2012/03/06 20:32:44, Ilya Sherman wrote: > > > Hmm, can any of these actually ever be NULL? > > > > I don't think so, but I since I only use this to send IPC messages I pass in a > > NULL pointer in the unit tests. > > > > Is there a better way to do this? Should I just take in a pointer to > > RenderViewHost instead of TabContentsWrapper in the constructor and then just > > make sure it isn't NULL here? > > If the RenderViewHost remains constant for a TabContentsWrapper, it would > definitely be better to take a pointer to that instead, in part to more narrowly > define the class dependency. Done. https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/browser/autof... chrome/browser/autofill/password_autofill_manager.cc:60: value)); On 2012/03/07 22:33:18, Ilya Sherman wrote: > On 2012/03/07 15:49:16, csharp wrote: > > On 2012/03/06 20:32:44, Ilya Sherman wrote: > > > Why not send the full fill data here, rather than having the renderer side > > redo > > > the work? > > > > We kinda get the full fill data for free in the renderer, since the the > > PasswordInfo class here is fairly different from the one in the renderer, it > > lacks a few variables uses webkit::forms:FormField instead of > > WebKit::WebInputElement. > > > > This means that we need to call FindLoginInfo to get those missing variables > > anyhow, and the fill data is stored with them so we still get it. > > Hmm, it seems a little wrong to have both the browser and the renderer tracking > this info, though. What are the missing bits from the browser side that the > renderer has access to? Ok, so I think we can removed the duplicated data. Basically we keep the PasswordInfo class in the renderer, but remove the fill_data from it and just store that information on the browser side. It should be fairly easy to just pass the required fill data when we need it. I'm not sure that at this point it makes sense to do this though. Since the old autofill UI still needs the fill_data on the render we can't remove it from the password_info class. I think the best approach here is probably to file a bug on myself to remove this duplicated data and once we have switched over to the new UI for all platform, fix the code. https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/browser/autof... File chrome/browser/autofill/password_autofill_manager.h (right): https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/browser/autof... chrome/browser/autofill/password_autofill_manager.h:54: void FrameClosing(long long frame_id); On 2012/03/07 22:33:18, Ilya Sherman wrote: > On 2012/03/07 15:49:16, csharp wrote: > > On 2012/03/06 20:32:44, Ilya Sherman wrote: > > > Why "long long"? > > > > WebFrame's unique identifier is a long long. > > In that case, why "int" above? Fixed. https://chromiumcodereview.appspot.com/9600038/diff/8001/chrome/browser/autof... File chrome/browser/autofill/password_autofill_manager.h (right): https://chromiumcodereview.appspot.com/9600038/diff/8001/chrome/browser/autof... chrome/browser/autofill/password_autofill_manager.h:32: bool WouldHandleKeyDown(const webkit::forms::FormField& field); On 2012/03/07 22:33:18, Ilya Sherman wrote: > I didn't see any place where this method is used. Can we just get rid of it? I'll be using this when the keyboard cl lands (should be sending that out for review for) and I'd like to submit this with all the other password code. https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... File chrome/browser/autofill/autofill_external_delegate.cc (right): https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.cc:56: if (!renderer_hiding_popup_) { I tried to send this message in the constructor, but that failed (I assume because the renderer isn't ready to take messages at that point) and I'd like to keep this call within this class, so this seems like the best way to do it... but it seems hacky to be checking this every query.
Sorry that I keep going back and forth on some of these issues -- I'm much less familiar with the password autofill code than the other autofill code, so I'm still learning new things about how the code works with each review pass. https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/browser/autof... File chrome/browser/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9600038/diff/3001/chrome/browser/autof... chrome/browser/autofill/password_autofill_manager.cc:60: value)); On 2012/03/08 16:48:37, csharp wrote: > On 2012/03/07 22:33:18, Ilya Sherman wrote: > > On 2012/03/07 15:49:16, csharp wrote: > > > On 2012/03/06 20:32:44, Ilya Sherman wrote: > > > > Why not send the full fill data here, rather than having the renderer side > > > redo > > > > the work? > > > > > > We kinda get the full fill data for free in the renderer, since the the > > > PasswordInfo class here is fairly different from the one in the renderer, it > > > lacks a few variables uses webkit::forms:FormField instead of > > > WebKit::WebInputElement. > > > > > > This means that we need to call FindLoginInfo to get those missing variables > > > anyhow, and the fill data is stored with them so we still get it. > > > > Hmm, it seems a little wrong to have both the browser and the renderer > tracking > > this info, though. What are the missing bits from the browser side that the > > renderer has access to? > > Ok, so I think we can removed the duplicated data. Basically we keep the > PasswordInfo class in the renderer, but remove the fill_data from it and just > store that information on the browser side. It should be fairly easy to just > pass the required fill data when we need it. > > I'm not sure that at this point it makes sense to do this though. Since the old > autofill UI still needs the fill_data on the render we can't remove it from the > password_info class. I think the best approach here is probably to file a bug on > myself to remove this duplicated data and once we have switched over to the new > UI for all platform, fix the code. Ok, I think I better understand now why it's hard to separate the data. The renderer side wants to know all of the login info immediately, in case there's no need to show UI. Both sides want to know all the data (really just the usernames, not the passwords), so that they know when to show the UI. I don't see a clean way to support those needs without duplicating some of the data, so I guess I'm ok with having the duplicate tracking for now. https://chromiumcodereview.appspot.com/9600038/diff/8001/chrome/browser/autof... File chrome/browser/autofill/password_autofill_manager.h (right): https://chromiumcodereview.appspot.com/9600038/diff/8001/chrome/browser/autof... chrome/browser/autofill/password_autofill_manager.h:32: bool WouldHandleKeyDown(const webkit::forms::FormField& field); On 2012/03/08 16:48:37, csharp wrote: > On 2012/03/07 22:33:18, Ilya Sherman wrote: > > I didn't see any place where this method is used. Can we just get rid of it? > > I'll be using this when the keyboard cl lands (should be sending that out for > review for) and I'd like to submit this with all the other password code. Ok. In that case, I'd prefer to wait on committing this CL until the keyboard code lands -- or at least until I can see how the pieces will fit together -- as this method's semantics are currently confusing to me. https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... File chrome/browser/autofill/autofill_external_delegate.cc (right): https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.cc:56: if (!renderer_hiding_popup_) { On 2012/03/08 16:48:37, csharp wrote: > I tried to send this message in the constructor, but that failed (I assume > because the renderer isn't ready to take messages at that point) and I'd like to > keep this call within this class, so this seems like the best way to do it... > but it seems hacky to be checking this every query. Hmm, I don't think I like having this message dissociated from the message that sends the list of passwords over the renderer, as that could lead to race conditions. I'd prefer to just add a boolean to the AutofillMsg_FillPasswordForm message. If you prefer to dissociate the messages, could you please describe your reasoning a bit? https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.cc:215: void AutofillExternalDelegate::FrameClosing(int frame_id) { nit: int -> long long? https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.cc:222: int frame_id) { nit: int -> long long? https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... File chrome/browser/autofill/autofill_external_delegate.h (right): https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.h:78: void FrameClosing(int frame_id); nit: int -> long long? https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.h:83: int frame_id); nit: int -> long long? https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... File chrome/browser/autofill/autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... chrome/browser/autofill/autofill_manager.cc:699: void AutofillManager::OnFrameClosing(int frame_id) { nit: int -> long long? https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... chrome/browser/autofill/autofill_manager.cc:707: int frame_id) { nit: int -> long long? https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... File chrome/browser/autofill/autofill_manager.h (right): https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... chrome/browser/autofill/autofill_manager.h:169: void OnFrameClosing(int frame_id); nit: int -> long long? https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... chrome/browser/autofill/autofill_manager.h:173: int frame_id); nit: int -> long long? https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... File chrome/browser/autofill/password_autofill_manager.h (right): https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... chrome/browser/autofill/password_autofill_manager.h:56: void FrameClosing(long long frame_id); I think it should be safe, and much simpler, to just reset the login_to_password_info_ map as part of AutofillManager::Reset() https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... File chrome/browser/autofill/password_autofill_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... chrome/browser/autofill/password_autofill_manager_unittest.cc:63: const int frame_id_; nit: int -> long long? https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/renderer/aut... File chrome/renderer/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:471: Send(new AutofillHostMsg_FillPasswordForm( If I'm now understanding the code correctly, this message name is really misleading. This is actually telling the browser the mapping between the fields on the page and the username/password combinations that correspond to them -- right? Is that something we could do purely browser-side based on the FormsSeen message, or do we need to bounce through the renderer to pick up extra info?
https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... File chrome/browser/autofill/autofill_external_delegate.cc (right): https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.cc:56: if (!renderer_hiding_popup_) { On 2012/03/08 23:22:14, Ilya Sherman wrote: > On 2012/03/08 16:48:37, csharp wrote: > > I tried to send this message in the constructor, but that failed (I assume > > because the renderer isn't ready to take messages at that point) and I'd like > to > > keep this call within this class, so this seems like the best way to do it... > > but it seems hacky to be checking this every query. > > Hmm, I don't think I like having this message dissociated from the message that > sends the list of passwords over the renderer, as that could lead to race > conditions. I'd prefer to just add a boolean to the > AutofillMsg_FillPasswordForm message. If you prefer to dissociate the messages, > could you please describe your reasoning a bit? I hadn't thought about adding it there. I've set it up to do that now. https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.cc:215: void AutofillExternalDelegate::FrameClosing(int frame_id) { On 2012/03/08 23:22:14, Ilya Sherman wrote: > nit: int -> long long? Removed https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.cc:222: int frame_id) { On 2012/03/08 23:22:14, Ilya Sherman wrote: > nit: int -> long long? Removed https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... File chrome/browser/autofill/autofill_external_delegate.h (right): https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.h:78: void FrameClosing(int frame_id); On 2012/03/08 23:22:14, Ilya Sherman wrote: > nit: int -> long long? Removed. https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.h:83: int frame_id); On 2012/03/08 23:22:14, Ilya Sherman wrote: > nit: int -> long long? Removed. https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... File chrome/browser/autofill/autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... chrome/browser/autofill/autofill_manager.cc:699: void AutofillManager::OnFrameClosing(int frame_id) { On 2012/03/08 23:22:14, Ilya Sherman wrote: > nit: int -> long long? removed https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... chrome/browser/autofill/autofill_manager.cc:707: int frame_id) { On 2012/03/08 23:22:14, Ilya Sherman wrote: > nit: int -> long long? removed https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... File chrome/browser/autofill/autofill_manager.h (right): https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... chrome/browser/autofill/autofill_manager.h:169: void OnFrameClosing(int frame_id); On 2012/03/08 23:22:14, Ilya Sherman wrote: > nit: int -> long long? Removed. https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... chrome/browser/autofill/autofill_manager.h:173: int frame_id); On 2012/03/08 23:22:14, Ilya Sherman wrote: > nit: int -> long long? Removed https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... File chrome/browser/autofill/password_autofill_manager.h (right): https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... chrome/browser/autofill/password_autofill_manager.h:56: void FrameClosing(long long frame_id); On 2012/03/08 23:22:14, Ilya Sherman wrote: > I think it should be safe, and much simpler, to just reset the > login_to_password_info_ map as part of AutofillManager::Reset() k, I'll make that change. I'll also add a comment in FrameClosing mentioning this choice in case it starts causing trouble later. https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... File chrome/browser/autofill/password_autofill_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/browser/auto... chrome/browser/autofill/password_autofill_manager_unittest.cc:63: const int frame_id_; On 2012/03/08 23:22:14, Ilya Sherman wrote: > nit: int -> long long? removed https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/renderer/aut... File chrome/renderer/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:471: Send(new AutofillHostMsg_FillPasswordForm( On 2012/03/08 23:22:14, Ilya Sherman wrote: > If I'm now understanding the code correctly, this message name is really > misleading. This is actually telling the browser the mapping between the fields > on the page and the username/password combinations that correspond to them -- > right? Yes. Changed the name to try and be a bit clearer. Is that something we could do purely browser-side based on the FormsSeen > message, or do we need to bounce through the renderer to pick up extra info? I don't think we stay just in the browser-side. The renderer code here gives the elements that we are looking to fill in, and it can be a 1 to many mapping from form_data to these elements, so I'm fairly certain we need to renderer. Potentially we could just pass back the field and not the form_data, but then we'd have to keep track of the form_data on the browser half and I think we'd be introducing a potential race condition where we end up getting mismatched pairs.
https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/renderer/aut... File chrome/renderer/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9600038/diff/18001/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:471: Send(new AutofillHostMsg_FillPasswordForm( On 2012/03/09 16:20:19, csharp wrote: > On 2012/03/08 23:22:14, Ilya Sherman wrote: > > If I'm now understanding the code correctly, this message name is really > > misleading. This is actually telling the browser the mapping between the > fields > > on the page and the username/password combinations that correspond to them -- > > right? > Yes. Changed the name to try and be a bit clearer. > > Is that something we could do purely browser-side based on the FormsSeen > > message, or do we need to bounce through the renderer to pick up extra info? > > I don't think we stay just in the browser-side. The renderer code here gives the > elements that we are looking to fill in, and it can be a 1 to many mapping from > form_data to these elements, so I'm fairly certain we need to renderer. > > Potentially we could just pass back the field and not the form_data, but then > we'd have to keep track of the form_data on the browser half and I think we'd be > introducing a potential race condition where we end up getting mismatched pairs. Ok. The current approach should be fine -- I'm just a little worried about the extra ping-ponging we're doing between the browser and renderer. It might be helpful to add a long-form comment somewhere describing the typical chain of messages that get passed back and forth, so that it's a little easier for someone unfamiliar with the code to get up to speed on how everything ties together. https://chromiumcodereview.appspot.com/9600038/diff/32001/chrome/browser/auto... File chrome/browser/autofill/autofill_external_delegate.h (right): https://chromiumcodereview.appspot.com/9600038/diff/32001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.h:81: void PasswordFormMapping( nit: Perhaps "AddPasswordFormMapping"? https://chromiumcodereview.appspot.com/9600038/diff/32001/chrome/browser/auto... File chrome/browser/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9600038/diff/32001/chrome/browser/auto... chrome/browser/autofill/password_autofill_manager.cc:81: } I don't think this is safe, because the frame that's closing could be an unrelated sub-frame. My suggestion was to clear() the map as part of AutofillManager's Reset() (i.e. when the *main* frame is closed/navigated) https://chromiumcodereview.appspot.com/9600038/diff/32001/chrome/browser/auto... File chrome/browser/autofill/password_autofill_manager.h (right): https://chromiumcodereview.appspot.com/9600038/diff/32001/chrome/browser/auto... chrome/browser/autofill/password_autofill_manager.h:61: }; nit: Probably no need to keep this as a struct anymore ;) https://chromiumcodereview.appspot.com/9600038/diff/32001/chrome/common/autof... File chrome/common/autofill_messages.h (right): https://chromiumcodereview.appspot.com/9600038/diff/32001/chrome/common/autof... chrome/common/autofill_messages.h:86: // and not show it. nit: This is kind of a run-on sentence. Perhaps leave the period where it previously was, and add a new sentence? Also, I'd suggest replacing "and not show it", which feels a little redundant to me, with "because the browser process will own the popup UI" (or something like that). https://chromiumcodereview.appspot.com/9600038/diff/32001/chrome/common/autof... chrome/common/autofill_messages.h:194: // Instruct the browse that a password mapping has been found for a field. nit: "browse" -> "browser"
https://chromiumcodereview.appspot.com/9600038/diff/32001/chrome/browser/auto... File chrome/browser/autofill/autofill_external_delegate.h (right): https://chromiumcodereview.appspot.com/9600038/diff/32001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.h:81: void PasswordFormMapping( On 2012/03/09 22:00:26, Ilya Sherman wrote: > nit: Perhaps "AddPasswordFormMapping"? Done. https://chromiumcodereview.appspot.com/9600038/diff/32001/chrome/browser/auto... File chrome/browser/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9600038/diff/32001/chrome/browser/auto... chrome/browser/autofill/password_autofill_manager.cc:81: } On 2012/03/09 22:00:26, Ilya Sherman wrote: > I don't think this is safe, because the frame that's closing could be an > unrelated sub-frame. My suggestion was to clear() the map as part of > AutofillManager's Reset() (i.e. when the *main* frame is closed/navigated) Ah, that makes much more sense, sorry for misreading your last comment. Fixed. https://chromiumcodereview.appspot.com/9600038/diff/32001/chrome/browser/auto... File chrome/browser/autofill/password_autofill_manager.h (right): https://chromiumcodereview.appspot.com/9600038/diff/32001/chrome/browser/auto... chrome/browser/autofill/password_autofill_manager.h:61: }; On 2012/03/09 22:00:26, Ilya Sherman wrote: > nit: Probably no need to keep this as a struct anymore ;) Done :) https://chromiumcodereview.appspot.com/9600038/diff/32001/chrome/common/autof... File chrome/common/autofill_messages.h (right): https://chromiumcodereview.appspot.com/9600038/diff/32001/chrome/common/autof... chrome/common/autofill_messages.h:86: // and not show it. On 2012/03/09 22:00:26, Ilya Sherman wrote: > nit: This is kind of a run-on sentence. Perhaps leave the period where it > previously was, and add a new sentence? Also, I'd suggest replacing "and not > show it", which feels a little redundant to me, with "because the browser > process will own the popup UI" (or something like that). Done. https://chromiumcodereview.appspot.com/9600038/diff/32001/chrome/common/autof... chrome/common/autofill_messages.h:194: // Instruct the browse that a password mapping has been found for a field. On 2012/03/09 22:00:26, Ilya Sherman wrote: > nit: "browse" -> "browser" Done.
https://chromiumcodereview.appspot.com/9600038/diff/31022/chrome/browser/auto... File chrome/browser/autofill/autofill_external_delegate.cc (right): https://chromiumcodereview.appspot.com/9600038/diff/31022/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.cc:186: HideAutofillPopup(); nit: How about moving this up prior to the password_autofill_manager_.DidAcceptAutofillSuggestion() check, and having it only in the one place? https://chromiumcodereview.appspot.com/9600038/diff/31022/chrome/browser/auto... File chrome/browser/autofill/autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9600038/diff/31022/chrome/browser/auto... chrome/browser/autofill/autofill_manager.cc:703: const webkit::forms::PasswordFormFillData fill_data) { nit: This should be pass-by-reference. https://chromiumcodereview.appspot.com/9600038/diff/31022/chrome/common/autof... File chrome/common/autofill_messages.h (right): https://chromiumcodereview.appspot.com/9600038/diff/31022/chrome/common/autof... chrome/common/autofill_messages.h:88: webkit::forms::PasswordFormFillData, nit: As long as you're editing this code, pelase add a comment describing the first parameter as well. https://chromiumcodereview.appspot.com/9600038/diff/31022/chrome/renderer/aut... File chrome/renderer/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9600038/diff/31022/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:348: (key_code == ui::VKEY_BACK || key_code == ui::VKEY_DELETE); Hmm, I thought we were letting the event bubble naturally by ignoring it on the browser-side. Why do we need to handle it explicitly here (in addition to handling the event in TextFieldHandlingKeyDown())?
http://codereview.chromium.org/9600038/diff/31022/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_external_delegate.cc (right): http://codereview.chromium.org/9600038/diff/31022/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.cc:186: HideAutofillPopup(); On 2012/03/12 23:46:13, Ilya Sherman wrote: > nit: How about moving this up prior to the > password_autofill_manager_.DidAcceptAutofillSuggestion() check, and having it > only in the one place? Done. http://codereview.chromium.org/9600038/diff/31022/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager.cc (right): http://codereview.chromium.org/9600038/diff/31022/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.cc:703: const webkit::forms::PasswordFormFillData fill_data) { On 2012/03/12 23:46:13, Ilya Sherman wrote: > nit: This should be pass-by-reference. Done. http://codereview.chromium.org/9600038/diff/31022/chrome/common/autofill_mess... File chrome/common/autofill_messages.h (right): http://codereview.chromium.org/9600038/diff/31022/chrome/common/autofill_mess... chrome/common/autofill_messages.h:88: webkit::forms::PasswordFormFillData, On 2012/03/12 23:46:13, Ilya Sherman wrote: > nit: As long as you're editing this code, pelase add a comment describing the > first parameter as well. Done. http://codereview.chromium.org/9600038/diff/31022/chrome/renderer/autofill/pa... File chrome/renderer/autofill/password_autofill_manager.cc (right): http://codereview.chromium.org/9600038/diff/31022/chrome/renderer/autofill/pa... chrome/renderer/autofill/password_autofill_manager.cc:348: (key_code == ui::VKEY_BACK || key_code == ui::VKEY_DELETE); On 2012/03/12 23:46:13, Ilya Sherman wrote: > Hmm, I thought we were letting the event bubble naturally by ignoring it on the > browser-side. Why do we need to handle it explicitly here (in addition to > handling the event in TextFieldHandlingKeyDown())? This code was from when I was handling it in the browser half and should have been removed.
https://chromiumcodereview.appspot.com/9600038/diff/39002/chrome/browser/auto... File chrome/browser/autofill/autofill_external_delegate.h (right): https://chromiumcodereview.appspot.com/9600038/diff/39002/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.h:125: // Password Autofill manager, handles all password related Autofilling. nit: "password related" -> "password-related" https://chromiumcodereview.appspot.com/9600038/diff/39002/chrome/browser/auto... File chrome/browser/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9600038/diff/39002/chrome/browser/auto... chrome/browser/autofill/password_autofill_manager.cc:27: return true; nit: You can combine lines 24-27 as """return iter != login_to_password_info_.end();""" https://chromiumcodereview.appspot.com/9600038/diff/39002/chrome/browser/auto... chrome/browser/autofill/password_autofill_manager.cc:80: string16 current_username = username_element.value; Hmm, is this always going to be the empty string, since the username_element is cached? Should this be comparing to the value of the |field| passed into DidAcceptAutofillSuggestion() instead? https://chromiumcodereview.appspot.com/9600038/diff/39002/chrome/browser/auto... chrome/browser/autofill/password_autofill_manager.cc:83: if (fill_data.basic_data.fields[0].value == current_username) { Why is this a full string equality check, rather than a prefix comparison? If I've saved "isherman/password1" as a login pair, type "ish", and select the "isherman" suggestion in the dropdown, shouldn't that cause both the username and the password to be filled? https://chromiumcodereview.appspot.com/9600038/diff/39002/chrome/browser/auto... File chrome/browser/autofill/password_autofill_manager.h (right): https://chromiumcodereview.appspot.com/9600038/diff/39002/chrome/browser/auto... chrome/browser/autofill/password_autofill_manager.h:33: bool WouldHandleKeyDown(const webkit::forms::FormField& field); I still don't understand why this method will be necessary once the keyboard handling code lands. If you still want to keep this in this CL, I'll prefer to hold off on the final L G T M until the other CL lands. (Will take a look at that in a moment, though I'm probably not the best reviewer for it.) https://chromiumcodereview.appspot.com/9600038/diff/39002/chrome/browser/auto... chrome/browser/autofill/password_autofill_manager.h:49: // Invoked when a password form is filled. nit: Please update this comment. https://chromiumcodereview.appspot.com/9600038/diff/39002/chrome/browser/auto... chrome/browser/autofill/password_autofill_manager.h:54: // Invoked when to clear any page specific cached values. nit: "when to clear" -> "to clear"? https://chromiumcodereview.appspot.com/9600038/diff/39002/chrome/browser/auto... chrome/browser/autofill/password_autofill_manager.h:60: LoginToPasswordInfoMap; If I'm understanding correctly, we could remove this map from the browser-side if we could distinguish a _password autofill_ suggestion from an _autocomplete_ suggestion based on something like the _unique id_, as we do for distinguishing _autocomplete_ vs. _autofill_ suggestions. If so, please add a TODO and file a bug to do that -- it would make this code much simpler. https://chromiumcodereview.appspot.com/9600038/diff/39002/chrome/renderer/aut... File chrome/renderer/autofill/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/9600038/diff/39002/chrome/renderer/aut... chrome/renderer/autofill/autofill_agent.cc:406: value); nit: Can we DCHECK that this returns true? (Common bug warning: You'll need to call the function outside of the DCHECK, and just check the return value in the DCHECK.) https://chromiumcodereview.appspot.com/9600038/diff/39002/chrome/renderer/aut... File chrome/renderer/autofill/autofill_agent.h (right): https://chromiumcodereview.appspot.com/9600038/diff/39002/chrome/renderer/aut... chrome/renderer/autofill/autofill_agent.h:113: void OnPasswordAcceptAutofillSuggestion(const string16& value); nit: Perhaps "OnAcceptPasswordAutofillSuggestion"?
http://codereview.chromium.org/9600038/diff/39002/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_external_delegate.h (right): http://codereview.chromium.org/9600038/diff/39002/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.h:125: // Password Autofill manager, handles all password related Autofilling. On 2012/03/15 18:27:41, Ilya Sherman wrote: > nit: "password related" -> "password-related" Done. http://codereview.chromium.org/9600038/diff/39002/chrome/browser/autofill/pas... File chrome/browser/autofill/password_autofill_manager.cc (right): http://codereview.chromium.org/9600038/diff/39002/chrome/browser/autofill/pas... chrome/browser/autofill/password_autofill_manager.cc:27: return true; On 2012/03/15 18:27:41, Ilya Sherman wrote: > nit: You can combine lines 24-27 as """return iter != > login_to_password_info_.end();""" Done. http://codereview.chromium.org/9600038/diff/39002/chrome/browser/autofill/pas... chrome/browser/autofill/password_autofill_manager.cc:80: string16 current_username = username_element.value; It is. I failed to copy over an assignment statement that happened in the renderer. Fixed http://codereview.chromium.org/9600038/diff/39002/chrome/browser/autofill/pas... chrome/browser/autofill/password_autofill_manager.cc:83: if (fill_data.basic_data.fields[0].value == current_username) { On 2012/03/15 18:27:41, Ilya Sherman wrote: > Why is this a full string equality check, rather than a prefix comparison? If > I've saved "isherman/password1" as a login pair, type "ish", and select the > "isherman" suggestion in the dropdown, shouldn't that cause both the username > and the password to be filled? If you've selected isherman then current_username will be isherman, the selected value, so we only want full matches otherwise this might want to use the password for isherman1. http://codereview.chromium.org/9600038/diff/39002/chrome/browser/autofill/pas... File chrome/browser/autofill/password_autofill_manager.h (right): http://codereview.chromium.org/9600038/diff/39002/chrome/browser/autofill/pas... chrome/browser/autofill/password_autofill_manager.h:33: bool WouldHandleKeyDown(const webkit::forms::FormField& field); On 2012/03/15 18:27:41, Ilya Sherman wrote: > I still don't understand why this method will be necessary once the keyboard > handling code lands. If you still want to keep this in this CL, I'll prefer to > hold off on the final L G T M until the other CL lands. (Will take a look at > that in a moment, though I'm probably not the best reviewer for it.) I wanted to keep this method to make sure that the normal Autofill manager didn't steal a key press that password manager would have handled (since the browser Autofill manager will go before the renderer Password Manager). The only key press that the password manager cares about though is backspace, which currently the browser Autofill won't take. I'll take this code out but I'll add a warning to the renderer keypress handler letting it know that the browser autofill code could eat inputs it wants in the future. http://codereview.chromium.org/9600038/diff/39002/chrome/browser/autofill/pas... chrome/browser/autofill/password_autofill_manager.h:49: // Invoked when a password form is filled. On 2012/03/15 18:27:41, Ilya Sherman wrote: > nit: Please update this comment. Done. http://codereview.chromium.org/9600038/diff/39002/chrome/browser/autofill/pas... chrome/browser/autofill/password_autofill_manager.h:54: // Invoked when to clear any page specific cached values. On 2012/03/15 18:27:41, Ilya Sherman wrote: > nit: "when to clear" -> "to clear"? Done. http://codereview.chromium.org/9600038/diff/39002/chrome/browser/autofill/pas... chrome/browser/autofill/password_autofill_manager.h:60: LoginToPasswordInfoMap; On 2012/03/15 18:27:41, Ilya Sherman wrote: > If I'm understanding correctly, we could remove this map from the browser-side > if we could distinguish a _password autofill_ suggestion from an _autocomplete_ > suggestion based on something like the _unique id_, as we do for distinguishing > _autocomplete_ vs. _autofill_ suggestions. If so, please add a TODO and file a > bug to do that -- it would make this code much simpler. I think you're right. I think there might be 1 tough spot to deal with but I can remove most of the mapping code (I think). Added a todo and filled a bug. http://codereview.chromium.org/9600038/diff/39002/chrome/renderer/autofill/au... File chrome/renderer/autofill/autofill_agent.cc (right): http://codereview.chromium.org/9600038/diff/39002/chrome/renderer/autofill/au... chrome/renderer/autofill/autofill_agent.cc:406: value); On 2012/03/15 18:27:41, Ilya Sherman wrote: > nit: Can we DCHECK that this returns true? (Common bug warning: You'll need to > call the function outside of the DCHECK, and just check the return value in the > DCHECK.) Done. http://codereview.chromium.org/9600038/diff/39002/chrome/renderer/autofill/au... File chrome/renderer/autofill/autofill_agent.h (right): http://codereview.chromium.org/9600038/diff/39002/chrome/renderer/autofill/au... chrome/renderer/autofill/autofill_agent.h:113: void OnPasswordAcceptAutofillSuggestion(const string16& value); On 2012/03/15 18:27:41, Ilya Sherman wrote: > nit: Perhaps "OnAcceptPasswordAutofillSuggestion"? Done. http://codereview.chromium.org/9600038/diff/52001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_external_delegate.cc (right): http://codereview.chromium.org/9600038/diff/52001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.cc:174: // TODO(csharp) Fix how password are handled so that FindLoginInfo works. I think this will work in mostly cases right now. The reason I can't use the function from the password manager is that since the password code doesn't every call OnQuery (like the other autofill code) when dealing with passwords we don't know what element autofill is working on. The two basic solutions I see are: 1) add the field as a parameter to one of the callbacks and store it (I'm not sure where a good place for this would be though). 2) I think with the removal and cleanup the password map in the manager I will be able to keep the code like this. I'd like to leave it like this for now and try and fix this when I address the bug I filled against myself. If you want me to add the field as a parameter to make the work in the meantime just let me know.
https://chromiumcodereview.appspot.com/9600038/diff/39002/chrome/browser/auto... File chrome/browser/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9600038/diff/39002/chrome/browser/auto... chrome/browser/autofill/password_autofill_manager.cc:83: if (fill_data.basic_data.fields[0].value == current_username) { On 2012/03/16 20:21:12, csharp wrote: > On 2012/03/15 18:27:41, Ilya Sherman wrote: > > Why is this a full string equality check, rather than a prefix comparison? If > > I've saved "isherman/password1" as a login pair, type "ish", and select the > > "isherman" suggestion in the dropdown, shouldn't that cause both the username > > and the password to be filled? > > If you've selected isherman then current_username will be isherman, the selected > value, so we only want full matches otherwise this might want to use the > password for isherman1. What happens if I type "ish" and click on the suggestion? Is that impossible for some reason? Even if the renderer very consistently autocompletes, is the popup dismissed when I hit backspace? https://chromiumcodereview.appspot.com/9600038/diff/39002/chrome/browser/auto... File chrome/browser/autofill/password_autofill_manager.h (right): https://chromiumcodereview.appspot.com/9600038/diff/39002/chrome/browser/auto... chrome/browser/autofill/password_autofill_manager.h:60: LoginToPasswordInfoMap; On 2012/03/16 20:21:12, csharp wrote: > On 2012/03/15 18:27:41, Ilya Sherman wrote: > > If I'm understanding correctly, we could remove this map from the browser-side > > if we could distinguish a _password autofill_ suggestion from an > _autocomplete_ > > suggestion based on something like the _unique id_, as we do for > distinguishing > > _autocomplete_ vs. _autofill_ suggestions. If so, please add a TODO and file > a > > bug to do that -- it would make this code much simpler. > > I think you're right. I think there might be 1 tough spot to deal with but I can > remove most of the mapping code (I think). Added a todo and filled a bug. I'd recommend not having the mixed strategy of unique_id for some things and the map for other things. I'd recommend just using the map for now; and switching to something like the unique_id strategy when we completely deprecate the WebKit popup UI. https://chromiumcodereview.appspot.com/9600038/diff/52001/chrome/browser/auto... File chrome/browser/autofill/autofill_external_delegate.cc (right): https://chromiumcodereview.appspot.com/9600038/diff/52001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.cc:148: } nit: No need for curly braces. https://chromiumcodereview.appspot.com/9600038/diff/52001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.cc:153: empty.push_back(string16()); Hmm, why are the suggestions all empty? https://chromiumcodereview.appspot.com/9600038/diff/52001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.cc:174: // TODO(csharp) Fix how password are handled so that FindLoginInfo works. On 2012/03/16 20:21:12, csharp wrote: > I think this will work in mostly cases right now. The reason I can't use the > function from the password manager is that since the password code doesn't every > call OnQuery (like the other autofill code) when dealing with passwords we don't > know what element autofill is working on. > > The two basic solutions I see are: > 1) add the field as a parameter to one of the callbacks and store it (I'm not > sure where a good place for this would be though). > 2) I think with the removal and cleanup the password map in the manager I will > be able to keep the code like this. > > I'd like to leave it like this for now and try and fix this when I address the > bug I filled against myself. If you want me to add the field as a parameter to > make the work in the meantime just let me know. It seems like this should work in all cases, but you'd still need to let the password manager know which suggestion was selected, right? Right now you're just returning early -- how does the password ever get filled this way? https://chromiumcodereview.appspot.com/9600038/diff/52001/chrome/browser/auto... File chrome/browser/autofill/autofill_external_delegate.h (right): https://chromiumcodereview.appspot.com/9600038/diff/52001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.h:156: const int kPasswordEntryIds = -2; nit: "Ids" -> "Id" https://chromiumcodereview.appspot.com/9600038/diff/52001/chrome/renderer/aut... File chrome/renderer/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9600038/diff/52001/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.cc:500: suggestions)); Hmm, I really don't like that the PasswordAutofillManager code (in the renderer) mirrors so much of the AutofillAgent logic. Have you explored whether it would be feasible to refactor the code so that more of it is shared? (It might not be viable, since the password autofill code tries to do inline autocompletion, whereas the rest of the autofill code currently doesn't.) What happens if both the Autofill code and the Password Autofill code try to show a popup? Do they race? https://chromiumcodereview.appspot.com/9600038/diff/52001/chrome/renderer/aut... File chrome/renderer/autofill/password_autofill_manager.h (right): https://chromiumcodereview.appspot.com/9600038/diff/52001/chrome/renderer/aut... chrome/renderer/autofill/password_autofill_manager.h:40: // the keys handled there or here change, this issue may appear. nit: Please move this comment inside the implementation file, as it is more of an implementation detail.
http://codereview.chromium.org/9600038/diff/52001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_external_delegate.cc (right): http://codereview.chromium.org/9600038/diff/52001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.cc:148: } On 2012/03/20 00:58:55, Ilya Sherman wrote: > nit: No need for curly braces. Done. http://codereview.chromium.org/9600038/diff/52001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.cc:153: empty.push_back(string16()); On 2012/03/20 00:58:55, Ilya Sherman wrote: > Hmm, why are the suggestions all empty? The suggestions still have values, it just the label and icon that are made blank because they have no meaning for a password field (this is also done in PasswordAutofillManager::ShowSuggestionPopup in the renderer). http://codereview.chromium.org/9600038/diff/52001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_external_delegate.h (right): http://codereview.chromium.org/9600038/diff/52001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.h:156: const int kPasswordEntryIds = -2; On 2012/03/20 00:58:55, Ilya Sherman wrote: > nit: "Ids" -> "Id" Done. http://codereview.chromium.org/9600038/diff/52001/chrome/renderer/autofill/pa... File chrome/renderer/autofill/password_autofill_manager.cc (right): http://codereview.chromium.org/9600038/diff/52001/chrome/renderer/autofill/pa... chrome/renderer/autofill/password_autofill_manager.cc:500: suggestions)); On 2012/03/20 00:58:55, Ilya Sherman wrote: > Hmm, I really don't like that the PasswordAutofillManager code (in the renderer) > mirrors so much of the AutofillAgent logic. Have you explored whether it would > be feasible to refactor the code so that more of it is shared? (It might not be > viable, since the password autofill code tries to do inline autocompletion, > whereas the rest of the autofill code currently doesn't.) > > What happens if both the Autofill code and the Password Autofill code try to > show a popup? Do they race? I didn't look much into if we could refactor to share the code between the classes. With my little poking around I don't feel like there are too many easy gains, a fair amount of the code looked just different enough to make refactoring it to be shared a challenge. I could be wrong though. As far as I can tell there should never be a race condition between them because the Autofill code will only be aware of the option to show a popup after the Password manager has declined it. Even if they both try to show one at the same time, they both go to the same function: webview->applyAutofillSuggestions( user_input, suggestions, labels, icons, ids, -1); So one will just replace the other, there will never be two popups. http://codereview.chromium.org/9600038/diff/52001/chrome/renderer/autofill/pa... File chrome/renderer/autofill/password_autofill_manager.h (right): http://codereview.chromium.org/9600038/diff/52001/chrome/renderer/autofill/pa... chrome/renderer/autofill/password_autofill_manager.h:40: // the keys handled there or here change, this issue may appear. On 2012/03/20 00:58:55, Ilya Sherman wrote: > nit: Please move this comment inside the implementation file, as it is more of > an implementation detail. Done. http://codereview.chromium.org/9600038/diff/61001/chrome/renderer/autofill/au... File chrome/renderer/autofill/autofill_agent.cc (right): http://codereview.chromium.org/9600038/diff/61001/chrome/renderer/autofill/au... chrome/renderer/autofill/autofill_agent.cc:236: autofill_query_element_ = element; I had to add this here (and below) because otherwise some of the IPC calls made from the browser would fail because they didn't know what element they were suppose to work on. I'm fairly certain that this should have no impact on the current Autofill implementation because all the code the accesses this variable gets called after it is set in the normal place. Does this seem fine to you? Or should this be wrapped behind a flag so it is only done when the external popup is used?
https://chromiumcodereview.appspot.com/9600038/diff/39002/chrome/browser/auto... File chrome/browser/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9600038/diff/39002/chrome/browser/auto... chrome/browser/autofill/password_autofill_manager.cc:83: if (fill_data.basic_data.fields[0].value == current_username) { On 2012/03/20 00:58:55, Ilya Sherman wrote: > On 2012/03/16 20:21:12, csharp wrote: > > On 2012/03/15 18:27:41, Ilya Sherman wrote: > > > Why is this a full string equality check, rather than a prefix comparison? > If > > > I've saved "isherman/password1" as a login pair, type "ish", and select the > > > "isherman" suggestion in the dropdown, shouldn't that cause both the > username > > > and the password to be filled? > > > > If you've selected isherman then current_username will be isherman, the > selected > > value, so we only want full matches otherwise this might want to use the > > password for isherman1. > > What happens if I type "ish" and click on the suggestion? Is that impossible > for some reason? Even if the renderer very consistently autocompletes, is the > popup dismissed when I hit backspace? Bump. https://chromiumcodereview.appspot.com/9600038/diff/39002/chrome/browser/auto... File chrome/browser/autofill/password_autofill_manager.h (right): https://chromiumcodereview.appspot.com/9600038/diff/39002/chrome/browser/auto... chrome/browser/autofill/password_autofill_manager.h:60: LoginToPasswordInfoMap; On 2012/03/20 00:58:55, Ilya Sherman wrote: > On 2012/03/16 20:21:12, csharp wrote: > > On 2012/03/15 18:27:41, Ilya Sherman wrote: > > > If I'm understanding correctly, we could remove this map from the > browser-side > > > if we could distinguish a _password autofill_ suggestion from an > > _autocomplete_ > > > suggestion based on something like the _unique id_, as we do for > > distinguishing > > > _autocomplete_ vs. _autofill_ suggestions. If so, please add a TODO and > file > > a > > > bug to do that -- it would make this code much simpler. > > > > I think you're right. I think there might be 1 tough spot to deal with but I > can > > remove most of the mapping code (I think). Added a todo and filled a bug. > > I'd recommend not having the mixed strategy of unique_id for some things and the > map for other things. I'd recommend just using the map for now; and switching > to something like the unique_id strategy when we completely deprecate the WebKit > popup UI. Bump. https://chromiumcodereview.appspot.com/9600038/diff/52001/chrome/browser/auto... File chrome/browser/autofill/autofill_external_delegate.cc (right): https://chromiumcodereview.appspot.com/9600038/diff/52001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.cc:153: empty.push_back(string16()); On 2012/03/21 14:10:31, csharp wrote: > On 2012/03/20 00:58:55, Ilya Sherman wrote: > > Hmm, why are the suggestions all empty? > > The suggestions still have values, it just the label and icon that are made > blank because they have no meaning for a password field (this is also done in > PasswordAutofillManager::ShowSuggestionPopup in the renderer). Ah, sorry, misread the code. You could write this more briefly as: std::vector<string16> empty(suggestions.size(), string16()); std::vector<int> password_ids(suggestions.size(), kPasswordEntryId); https://chromiumcodereview.appspot.com/9600038/diff/52001/chrome/browser/auto... File chrome/browser/autofill/autofill_external_delegate.h (right): https://chromiumcodereview.appspot.com/9600038/diff/52001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.h:156: const int kPasswordEntryIds = -2; On 2012/03/21 14:10:31, csharp wrote: > On 2012/03/20 00:58:55, Ilya Sherman wrote: > > nit: "Ids" -> "Id" > > Done. Methinks you interpreted this nit backward ;) https://chromiumcodereview.appspot.com/9600038/diff/52001/chrome/browser/auto... File chrome/browser/autofill/password_autofill_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/9600038/diff/52001/chrome/browser/auto... chrome/browser/autofill/password_autofill_manager_unittest.cc:64: invalid_username.value = ASCIIToUTF16("no_user"); It looks like you've removed the test for the case where the user has a password saved for username "alice" but types "bob" (or "no_user", in the pre-existing test code). Is that intentional? https://chromiumcodereview.appspot.com/9600038/diff/61001/chrome/browser/auto... File chrome/browser/autofill/autofill_external_delegate.h (right): https://chromiumcodereview.appspot.com/9600038/diff/61001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.h:63: virtual void SetBounds(const gfx::Rect& bounds) = 0; nit: Perhaps "OnSetBounds()" for consistency with the other methods in this class? https://chromiumcodereview.appspot.com/9600038/diff/61001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.h:66: void ShowPasswordSuggestions(const std::vector<string16>& suggestions); nit: Perhaps "OnShowPasswordSuggestions()" for consistency with the other methods in this class? https://chromiumcodereview.appspot.com/9600038/diff/61001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.h:66: void ShowPasswordSuggestions(const std::vector<string16>& suggestions); nit: Why does OnQuery() include the desired bounds, whereas ShowPasswordSuggestions() does not? https://chromiumcodereview.appspot.com/9600038/diff/61001/chrome/renderer/aut... File chrome/renderer/autofill/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/9600038/diff/61001/chrome/renderer/aut... chrome/renderer/autofill/autofill_agent.cc:236: autofill_query_element_ = element; On 2012/03/21 14:10:31, csharp wrote: > I had to add this here (and below) because otherwise some of the IPC calls made > from the browser would fail because they didn't know what element they were > suppose to work on. > > I'm fairly certain that this should have no impact on the current Autofill > implementation because all the code the accesses this variable gets called after > it is set in the normal place. > > Does this seem fine to you? Or should this be wrapped behind a flag so it is > only done when the external popup is used? This seems fine to me.
http://codereview.chromium.org/9600038/diff/61001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_external_delegate.h (right): http://codereview.chromium.org/9600038/diff/61001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.h:63: virtual void SetBounds(const gfx::Rect& bounds) = 0; On 2012/03/22 01:20:06, Ilya Sherman wrote: > nit: Perhaps "OnSetBounds()" for consistency with the other methods in this > class? Moved this to be a protected method since I don't need it to be public anymore so I'll leave the name the same (since it doesn't get called as a result of an IPC let all the other On* functions) http://codereview.chromium.org/9600038/diff/61001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.h:66: void ShowPasswordSuggestions(const std::vector<string16>& suggestions); On 2012/03/22 01:20:06, Ilya Sherman wrote: > nit: Why does OnQuery() include the desired bounds, whereas > ShowPasswordSuggestions() does not? Fixed.
http://codereview.chromium.org/9600038/diff/52001/chrome/browser/autofill/pas... File chrome/browser/autofill/password_autofill_manager_unittest.cc (right): http://codereview.chromium.org/9600038/diff/52001/chrome/browser/autofill/pas... chrome/browser/autofill/password_autofill_manager_unittest.cc:64: invalid_username.value = ASCIIToUTF16("no_user"); On 2012/03/22 01:20:06, Ilya Sherman wrote: > It looks like you've removed the test for the case where the user has a password > saved for username "alice" but types "bob" (or "no_user", in the pre-existing > test code). Is that intentional? Added back.
http://codereview.chromium.org/9600038/diff/39002/chrome/browser/autofill/pas... File chrome/browser/autofill/password_autofill_manager.cc (right): http://codereview.chromium.org/9600038/diff/39002/chrome/browser/autofill/pas... chrome/browser/autofill/password_autofill_manager.cc:83: if (fill_data.basic_data.fields[0].value == current_username) { On 2012/03/22 01:20:06, Ilya Sherman wrote: > On 2012/03/20 00:58:55, Ilya Sherman wrote: > > On 2012/03/16 20:21:12, csharp wrote: > > > On 2012/03/15 18:27:41, Ilya Sherman wrote: > > > > Why is this a full string equality check, rather than a prefix comparison? > > > If > > > > I've saved "isherman/password1" as a login pair, type "ish", and select > the > > > > "isherman" suggestion in the dropdown, shouldn't that cause both the > > username > > > > and the password to be filled? > > > > > > If you've selected isherman then current_username will be isherman, the > > selected > > > value, so we only want full matches otherwise this might want to use the > > > password for isherman1. > > > > What happens if I type "ish" and click on the suggestion? Is that impossible > > for some reason? Even if the renderer very consistently autocompletes, is the > > popup dismissed when I hit backspace? > > Bump. current_username will be equal to what you selected from the popup box, not what you've typed in the box so it will always have a valid full match. The popup is not dismissed after hitting backspace (same as current behaviour) and it will correctly fill in the password field as well if it is selected after hitting delete. http://codereview.chromium.org/9600038/diff/39002/chrome/browser/autofill/pas... File chrome/browser/autofill/password_autofill_manager.h (right): http://codereview.chromium.org/9600038/diff/39002/chrome/browser/autofill/pas... chrome/browser/autofill/password_autofill_manager.h:60: LoginToPasswordInfoMap; On 2012/03/22 01:20:06, Ilya Sherman wrote: > On 2012/03/20 00:58:55, Ilya Sherman wrote: > > On 2012/03/16 20:21:12, csharp wrote: > > > On 2012/03/15 18:27:41, Ilya Sherman wrote: > > > > If I'm understanding correctly, we could remove this map from the > > browser-side > > > > if we could distinguish a _password autofill_ suggestion from an > > > _autocomplete_ > > > > suggestion based on something like the _unique id_, as we do for > > > distinguishing > > > > _autocomplete_ vs. _autofill_ suggestions. If so, please add a TODO and > > file > > > a > > > > bug to do that -- it would make this code much simpler. > > > > > > I think you're right. I think there might be 1 tough spot to deal with but I > > can > > > remove most of the mapping code (I think). Added a todo and filled a bug. > > > > I'd recommend not having the mixed strategy of unique_id for some things and > the > > map for other things. I'd recommend just using the map for now; and switching > > to something like the unique_id strategy when we completely deprecate the > WebKit > > popup UI. > > Bump. Just using the map now. Had to add the field to one of the password ipc calls, since it wasn't getting set the normal way so the field value I was comparing to the map was garbage, which is why I was having trouble.
LGTM with a last few nits -- thanks :) https://chromiumcodereview.appspot.com/9600038/diff/80001/chrome/browser/auto... File chrome/browser/autofill/autofill_external_delegate.cc (right): https://chromiumcodereview.appspot.com/9600038/diff/80001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.cc:199: // User selected an Autocomplete or Password entry, so we fill directly. nit: Is it actually possible for the user to select a password entry and end up in this case, rather than in the case above? https://chromiumcodereview.appspot.com/9600038/diff/80001/chrome/browser/auto... File chrome/browser/autofill/autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9600038/diff/80001/chrome/browser/auto... chrome/browser/autofill/autofill_manager.cc:657: } nit: No need for curly braces. https://chromiumcodereview.appspot.com/9600038/diff/80001/chrome/browser/auto... File chrome/browser/autofill/password_autofill_manager.cc (right): https://chromiumcodereview.appspot.com/9600038/diff/80001/chrome/browser/auto... chrome/browser/autofill/password_autofill_manager.cc:15: content::WebContents* web_contents) : web_contents_(web_contents) { nit: Can we store a pointer just to the RenderViewHost rather than the WebContents? In fact, thought I remembered you already making this change in an earlier patch set -- did you realize that this approach doesn't work for some reason? https://chromiumcodereview.appspot.com/9600038/diff/80001/chrome/browser/auto... chrome/browser/autofill/password_autofill_manager.cc:73: nit: No need for this blank line (line 73) https://chromiumcodereview.appspot.com/9600038/diff/80001/chrome/browser/auto... chrome/browser/autofill/password_autofill_manager.cc:98: *found_input = field; nit: It looks like the caller always knows the value that will get written into |found_input| ahead of time. If I'm not misreading anything, I'd recommend just removing this parameter, since it seems to be redundant information. https://chromiumcodereview.appspot.com/9600038/diff/80001/chrome/browser/auto... File chrome/browser/autofill/password_autofill_manager.h (right): https://chromiumcodereview.appspot.com/9600038/diff/80001/chrome/browser/auto... chrome/browser/autofill/password_autofill_manager.h:60: bool WillFillUserNameAndPassword( nit: Please add a brief comment describing this method. https://chromiumcodereview.appspot.com/9600038/diff/80001/chrome/common/autof... File chrome/common/autofill_messages.h (right): https://chromiumcodereview.appspot.com/9600038/diff/80001/chrome/common/autof... chrome/common/autofill_messages.h:123: string16) nit: Please add a quick comment to describe the string16 parameter https://chromiumcodereview.appspot.com/9600038/diff/80001/chrome/common/autof... chrome/common/autofill_messages.h:195: nit: No need for this extra blank line (line 195) https://chromiumcodereview.appspot.com/9600038/diff/80001/chrome/common/autof... chrome/common/autofill_messages.h:206: webkit::forms::PasswordFormFillData /*the form fill data */) nit: There should be a space between "/*" and "the" in the comment. Also, "form fill data" strikes me as really vague -- is there a concise way to describe what that data actually consists of? (If not, it's fine to leave the vague comment -- concision is probably more important here.)
http://codereview.chromium.org/9600038/diff/80001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_external_delegate.cc (right): http://codereview.chromium.org/9600038/diff/80001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.cc:199: // User selected an Autocomplete or Password entry, so we fill directly. On 2012/04/02 22:07:29, Ilya Sherman wrote: > nit: Is it actually possible for the user to select a password entry and end up > in this case, rather than in the case above? Nope, forget to clean up comment when I moved the autofill code out. Fixed. http://codereview.chromium.org/9600038/diff/80001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager.cc (right): http://codereview.chromium.org/9600038/diff/80001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.cc:657: } On 2012/04/02 22:07:29, Ilya Sherman wrote: > nit: No need for curly braces. Done. http://codereview.chromium.org/9600038/diff/80001/chrome/browser/autofill/pas... File chrome/browser/autofill/password_autofill_manager.cc (right): http://codereview.chromium.org/9600038/diff/80001/chrome/browser/autofill/pas... chrome/browser/autofill/password_autofill_manager.cc:15: content::WebContents* web_contents) : web_contents_(web_contents) { On 2012/04/02 22:07:29, Ilya Sherman wrote: > nit: Can we store a pointer just to the RenderViewHost rather than the > WebContents? In fact, thought I remembered you already making this change in an > earlier patch set -- did you realize that this approach doesn't work for some > reason? Ya, I had passed in a pointer to the RenderViewHost before but it seems that between this object is constructed and when it needs to start send messages the pointer becomes invalid. I couldn't see any cleaner way to make a valid RenderViewHost for this code. I added a comment to the pointer definition explaining this choice. http://codereview.chromium.org/9600038/diff/80001/chrome/browser/autofill/pas... chrome/browser/autofill/password_autofill_manager.cc:73: On 2012/04/02 22:07:29, Ilya Sherman wrote: > nit: No need for this blank line (line 73) Done. http://codereview.chromium.org/9600038/diff/80001/chrome/browser/autofill/pas... chrome/browser/autofill/password_autofill_manager.cc:98: *found_input = field; On 2012/04/02 22:07:29, Ilya Sherman wrote: > nit: It looks like the caller always knows the value that will get written into > |found_input| ahead of time. If I'm not misreading anything, I'd recommend just > removing this parameter, since it seems to be redundant information. Done. http://codereview.chromium.org/9600038/diff/80001/chrome/browser/autofill/pas... File chrome/browser/autofill/password_autofill_manager.h (right): http://codereview.chromium.org/9600038/diff/80001/chrome/browser/autofill/pas... chrome/browser/autofill/password_autofill_manager.h:60: bool WillFillUserNameAndPassword( On 2012/04/02 22:07:29, Ilya Sherman wrote: > nit: Please add a brief comment describing this method. Done. http://codereview.chromium.org/9600038/diff/80001/chrome/common/autofill_mess... File chrome/common/autofill_messages.h (right): http://codereview.chromium.org/9600038/diff/80001/chrome/common/autofill_mess... chrome/common/autofill_messages.h:123: string16) On 2012/04/02 22:07:29, Ilya Sherman wrote: > nit: Please add a quick comment to describe the string16 parameter Done. http://codereview.chromium.org/9600038/diff/80001/chrome/common/autofill_mess... chrome/common/autofill_messages.h:195: On 2012/04/02 22:07:29, Ilya Sherman wrote: > nit: No need for this extra blank line (line 195) Done. http://codereview.chromium.org/9600038/diff/80001/chrome/common/autofill_mess... chrome/common/autofill_messages.h:206: webkit::forms::PasswordFormFillData /*the form fill data */) On 2012/04/02 22:07:29, Ilya Sherman wrote: > nit: There should be a space between "/*" and "the" in the comment. Also, "form > fill data" strikes me as really vague -- is there a concise way to describe what > that data actually consists of? (If not, it's fine to leave the vague comment > -- concision is probably more important here.) Changed the text to "password pairing", still a bit vague but I think it gives a better ideas of what it represents.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/9600038/90001
Try job failure for 9600038-90001 (retry) on win_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/9600038/90001
Try job failure for 9600038-90001 (retry) (retry) on win_rel for steps "browser_tests, unit_tests". It's a second try, previously, steps "browser_tests, unit_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/9600038/90001
Try job failure for 9600038-90001 (retry) on win for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/9600038/90001
Try job failure for 9600038-90001 (retry) on win_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/9600038/90001
Try job failure for 9600038-90001 (retry) on linux_chromeos for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/9600038/90001
Change committed as 130824 |
