Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11539003:
Pop up requestAutocomplete UI when autofill server hints chrome client that it is in a multipage au… (Closed)
|
Created:
8 years ago by Raman Kakilate Modified:
7 years, 11 months ago CC:
chromium-reviews, benquan, dhollowa+watch_chromium.org, ahutter, browser-components-watch_chromium.org, dbeam+watch-autofill_chromium.org, darin-cc_chromium.org, Dane Wallinga, dyu1, estade+watch_chromium.org, Albert Bodenhamer, Ilya Sherman Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionPop up requestAutocomplete UI when autofill server hints chrome client that it is in a multipage autofill flow.
* Added AutofillFlowInfobarDelegate for the Infobar. (Note that this might evolve soon when we have much concrete UX)
* Strings used are also temporary.
* browser/autofill/autofill_flow_util.* : helper functions to build FormStructure representing all the information needed for autofill_flow.
* browser/autofill/autofill_manager.* : Added new methods to pop the UI and callback method to handle data from UI.
* browser/autofill/autofill_xml_parser/form_structure: To get information about step in the autofill flow from autofill servers.
* renderer/autofill/autofill_agent.* : To send SSL Status of the page back to the browser process.
BUG=159830
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=177746
Patch Set 1 #Patch Set 2 : Updated string. Pop the UI only on first page of the flow. #
Total comments: 34
Patch Set 3 : address initial comments #
Total comments: 14
Patch Set 4 : Addressed review comments. #
Total comments: 12
Patch Set 5 : Addressed Albert's comments #
Total comments: 67
Patch Set 6 : Address Ilya comments #Patch Set 7 : More comments addressed. #
Total comments: 31
Patch Set 8 : Addressed some more comments. #Patch Set 9 : Convert AutocheckoutManager to use WeakPtr instead of RefCounted. #
Total comments: 6
Patch Set 10 : Refactor autofill_manager to expose GetWebContents(). #
Total comments: 18
Patch Set 11 : Fixed more nits. #
Total comments: 4
Patch Set 12 : Address Sky's comment #Patch Set 13 : Resolve with AutofillDialogController refactoring. #Messages
Total messages: 34 (0 generated)
PTAL, Once I get LGTM, I will send out for wider chrome team to review.
Some initial comments: https://codereview.chromium.org/11539003/diff/3001/chrome/app/generated_resou... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/11539003/diff/3001/chrome/app/generated_resou... chrome/app/generated_resources.grd:10591: + Ignore This doesn't seem like an appropriate string for the UI. Perhaps "Don't Use Google Wallet"? The Chrome UI leads will probably have better suggestions. https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_manager.cc (right): https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_manager.cc:580: // Autofill serve said so), then trigger payments UI while also returning nit: "serve" -> "server" https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_xml_parser.cc (right): https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_xml_parser.cc:93: } else if (element.compare("wallet_data") == 0) { "wallet_data" does not seem like an appropriate name for this field. This data is not in any way specific to Wallet. https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_xml_parser.h (right): https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_xml_parser.h:73: int GetPageNo() { return page_no_; } nit: Please spell out "number". This method should be named "page_number()"; the variable should be named "page_number_" https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_xml_parser.h:75: int GetTotalPages() { return total_pages_; } nit: This method should be named total_pages() https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_xml_parser.h:99: // Page number of present page in multipage autofill flow. Why is the XML server responsible for keeping track of which page this is? Shouldn't it always be the first page? https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/fo... File chrome/browser/autofill/form_structure.h (right): https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/fo... chrome/browser/autofill/form_structure.h:146: bool is_start_autofillable_flow() const { return page_no_ == 1; } nit: This method is not just doing a simple variable access, so it should be named using CamelCase, and the implementation should be in the .cc file. https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/wa... File chrome/browser/autofill/wallet_infobar_delegate.cc (right): https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet_infobar_delegate.cc:23: FormFieldData BuildField(std::string autocomplete_attribute) { nit: Pass by const reference https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet_infobar_delegate.cc:51: } These functions have nothing to do with the infobar; the AutofillManager class -- or a new class -- is a more appropriate location for them. Also, they should be defined in an anonymous namespace. https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/wa... File chrome/browser/autofill/wallet_infobar_delegate.h (right): https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet_infobar_delegate.h:27: class WalletInfoBarDelegate : public ConfirmInfoBarDelegate { This infobar is not specific to Wallet. The flow should work equally well if only local data is available. https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet_infobar_delegate.h:56: PersonalDataManager* personal_data_; Why do you need the PDM? https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet_infobar_delegate.h:63: AutofillManager* autofill_mgr_; nit: Please spell out "manager" https://codereview.chromium.org/11539003/diff/3001/chrome/renderer/autofill/a... File chrome/renderer/autofill/autofill_agent.cc (right): https://codereview.chromium.org/11539003/diff/3001/chrome/renderer/autofill/a... chrome/renderer/autofill/autofill_agent.cc:710: // QueryFormFieldAutofill to browser process. This comment is entirely redundant with the code. Please either remove it or change to explain *why* the code does what it does; not *what* the code does.
https://codereview.chromium.org/11539003/diff/3001/chrome/app/generated_resou... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/11539003/diff/3001/chrome/app/generated_resou... chrome/app/generated_resources.grd:10591: + Ignore On 2012/12/13 02:29:23, Ilya Sherman wrote: > This doesn't seem like an appropriate string for the UI. Perhaps "Don't Use > Google Wallet"? The Chrome UI leads will probably have better suggestions. We wouldn't to addressing using local data in the present project, so only two options available to user for now are use wallet or ignore. https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_manager.cc (right): https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_manager.cc:580: // Autofill serve said so), then trigger payments UI while also returning On 2012/12/13 02:29:23, Ilya Sherman wrote: > nit: "serve" -> "server" Done. https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_xml_parser.cc (right): https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_xml_parser.cc:93: } else if (element.compare("wallet_data") == 0) { On 2012/12/13 02:29:23, Ilya Sherman wrote: > "wallet_data" does not seem like an appropriate name for this field. This data > is not in any way specific to Wallet. wallet_data is tag that is already added on autofill servers. True, that the concept of filling flow which spans multiple pages is not tied to wallet, but it is where we are starting. https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_xml_parser.h (right): https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_xml_parser.h:73: int GetPageNo() { return page_no_; } On 2012/12/13 02:29:23, Ilya Sherman wrote: > nit: Please spell out "number". This method should be named "page_number()"; > the variable should be named "page_number_" Done. https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_xml_parser.h:75: int GetTotalPages() { return total_pages_; } On 2012/12/13 02:29:23, Ilya Sherman wrote: > nit: This method should be named total_pages() Done. https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_xml_parser.h:99: // Page number of present page in multipage autofill flow. On 2012/12/13 02:29:23, Ilya Sherman wrote: > Why is the XML server responsible for keeping track of which page this is? > Shouldn't it always be the first page? Idea is show autofill_dialog_controller UI on the first page and gather all the data required for the flow. and on subsequent pages, we should just fill the page and navigate. we would have to identify - start of the flow - being inside the flow and - the last step of the flow. All of that information can be inferred using page_number_ and total_pages_ https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/fo... File chrome/browser/autofill/form_structure.h (right): https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/fo... chrome/browser/autofill/form_structure.h:146: bool is_start_autofillable_flow() const { return page_no_ == 1; } On 2012/12/13 02:29:23, Ilya Sherman wrote: > nit: This method is not just doing a simple variable access, so it should be > named using CamelCase, and the implementation should be in the .cc file. Done. https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/wa... File chrome/browser/autofill/wallet_infobar_delegate.cc (right): https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet_infobar_delegate.cc:23: FormFieldData BuildField(std::string autocomplete_attribute) { On 2012/12/13 02:29:23, Ilya Sherman wrote: > nit: Pass by const reference Done. https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet_infobar_delegate.cc:51: } On 2012/12/13 02:29:23, Ilya Sherman wrote: > These functions have nothing to do with the infobar; the AutofillManager class > -- or a new class -- is a more appropriate location for them. Also, they should > be defined in an anonymous namespace. Given that this infobar will be close to wallet, I think this file is good place. BuildField should not be used in general AutofillManager, this function is very specific to almost dummy objects we are creating here to satisfy autofill_dialog_controller interface (I am creating field without proper name etc). I prefer having all these wallet specific code in one place to manage it later. If you are strongly inclined, I will move it. https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/wa... File chrome/browser/autofill/wallet_infobar_delegate.h (right): https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet_infobar_delegate.h:27: class WalletInfoBarDelegate : public ConfirmInfoBarDelegate { On 2012/12/13 02:29:23, Ilya Sherman wrote: > This infobar is not specific to Wallet. The flow should work equally well if > only local data is available. This infobar assumes the kind of data it needs from the user, which is tied to wallet. Extending this feature to also use local autofill data is probably out of scope of the present project. https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet_infobar_delegate.h:56: PersonalDataManager* personal_data_; On 2012/12/13 02:29:23, Ilya Sherman wrote: > Why do you need the PDM? No I don't yet, removed. https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet_infobar_delegate.h:63: AutofillManager* autofill_mgr_; On 2012/12/13 02:29:23, Ilya Sherman wrote: > nit: Please spell out "manager" Done. https://codereview.chromium.org/11539003/diff/3001/chrome/renderer/autofill/a... File chrome/renderer/autofill/autofill_agent.cc (right): https://codereview.chromium.org/11539003/diff/3001/chrome/renderer/autofill/a... chrome/renderer/autofill/autofill_agent.cc:710: // QueryFormFieldAutofill to browser process. On 2012/12/13 02:29:23, Ilya Sherman wrote: > This comment is entirely redundant with the code. Please either remove it or > change to explain *why* the code does what it does; not *what* the code does. Done.
https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_xml_parser.cc (right): https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_xml_parser.cc:93: } else if (element.compare("wallet_data") == 0) { On 2012/12/13 21:34:34, Raman Kakilate wrote: > On 2012/12/13 02:29:23, Ilya Sherman wrote: > > "wallet_data" does not seem like an appropriate name for this field. This > data > > is not in any way specific to Wallet. > > wallet_data is tag that is already added on autofill servers. True, that the > concept of filling flow which spans multiple pages is not tied to wallet, but it > is where we are starting. Please rename the tag in the server code. Baking "wallet" into the name is not appropriate. https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_xml_parser.h (right): https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_xml_parser.h:99: // Page number of present page in multipage autofill flow. On 2012/12/13 21:34:34, Raman Kakilate wrote: > On 2012/12/13 02:29:23, Ilya Sherman wrote: > > Why is the XML server responsible for keeping track of which page this is? > > Shouldn't it always be the first page? > > Idea is show autofill_dialog_controller UI on the first page and gather all the > data required for the flow. and on subsequent pages, we should just fill the > page and navigate. > > we would have to identify > - start of the flow > - being inside the flow and > - the last step of the flow. > > All of that information can be inferred using page_number_ and total_pages_ That all makes sense. What I don't understand is why the server would ever send down data for any page other than the first. I'd expect that when the user navigates to the first page in the flow, the server simply sends down data for that page as well as all subsequent pages in the flow. This allows us to avoid the overhead of making a new connection to the server on each page load. What am I missing? https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/wa... File chrome/browser/autofill/wallet_infobar_delegate.cc (right): https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet_infobar_delegate.cc:40: formdata.fields.push_back(BuildField("billing address-line1")); This should be street-address; or else you should request address-line2 as well. Ditto for the shipping address. https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet_infobar_delegate.cc:51: } On 2012/12/13 21:34:34, Raman Kakilate wrote: > On 2012/12/13 02:29:23, Ilya Sherman wrote: > > These functions have nothing to do with the infobar; the AutofillManager class > > -- or a new class -- is a more appropriate location for them. Also, they > should > > be defined in an anonymous namespace. > > Given that this infobar will be close to wallet, I think this file is good > place. > > BuildField should not be used in general AutofillManager, this function is very > specific to almost dummy objects we are creating here to satisfy > autofill_dialog_controller interface (I am creating field without proper name > etc). > > I prefer having all these wallet specific code in one place to manage it later. > > If you are strongly inclined, I will move it. I am strongly inclined for the code not to live in this file. Infobars have -- unfortunately -- a lot of subtlety to them, so I'd prefer to keep this file as focused as possible on just the code needed for the infobar itself. I agree with you, though, that the AutofillManager class is not a great place for this code either -- I suggested it simply because it was the easiest place to move the code to. Probably the most appropriate change is to create an AutocheckoutManager class, and move this code as well as any of the other Autocheckout-specific code (other than this infobar) into that class. https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/wa... File chrome/browser/autofill/wallet_infobar_delegate.h (right): https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet_infobar_delegate.h:27: class WalletInfoBarDelegate : public ConfirmInfoBarDelegate { On 2012/12/13 21:34:34, Raman Kakilate wrote: > On 2012/12/13 02:29:23, Ilya Sherman wrote: > > This infobar is not specific to Wallet. The flow should work equally well if > > only local data is available. > > This infobar assumes the kind of data it needs from the user, which is tied to > wallet. Extending this feature to also use local autofill data is probably out > of scope of the present project. I don't see anything in the data that is tied to Wallet. The UI this infobar invokes is already being designed to support both Wallet and local data sources, so it is actually more work to restrict to only Wallet data; and is a worse user experience on top of that. If you are still not convinced that restricting to just Wallet data is not appropriate, we should probably schedule a meeting to gain clarity on this point.
Tests? https://codereview.chromium.org/11539003/diff/12001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_xml_parser.cc (right): https://codereview.chromium.org/11539003/diff/12001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_xml_parser.cc:36: page_number_(-1), total_pages_(-1), separate lines. https://codereview.chromium.org/11539003/diff/12001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_xml_parser.cc:99: if (attribute_name.compare("page_no") == 0) { Can't you use == instead of compare? https://codereview.chromium.org/11539003/diff/12001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_xml_parser.h (right): https://codereview.chromium.org/11539003/diff/12001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_xml_parser.h:73: int page_number() { return page_number_; } int page_number() const {... https://codereview.chromium.org/11539003/diff/12001/chrome/browser/autofill/f... File chrome/browser/autofill/form_structure.h (right): https://codereview.chromium.org/11539003/diff/12001/chrome/browser/autofill/f... chrome/browser/autofill/form_structure.h:216: // doesn't belong to any autofill flow, it is set to 0. Should probably be -1 for clarity. https://codereview.chromium.org/11539003/diff/12001/chrome/browser/autofill/w... File chrome/browser/autofill/wallet_infobar_delegate.cc (right): https://codereview.chromium.org/11539003/diff/12001/chrome/browser/autofill/w... chrome/browser/autofill/wallet_infobar_delegate.cc:32: FormData BuildWalletFormData() { Should probably be static. https://codereview.chromium.org/11539003/diff/12001/chrome/browser/autofill/w... chrome/browser/autofill/wallet_infobar_delegate.cc:40: formdata.fields.push_back(BuildField("billing address-line1")); Where's address line 2?
https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_xml_parser.cc (right): https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_xml_parser.cc:93: } else if (element.compare("wallet_data") == 0) { On 2012/12/13 23:23:18, Ilya Sherman wrote: > On 2012/12/13 21:34:34, Raman Kakilate wrote: > > On 2012/12/13 02:29:23, Ilya Sherman wrote: > > > "wallet_data" does not seem like an appropriate name for this field. This > > data > > > is not in any way specific to Wallet. > > > > wallet_data is tag that is already added on autofill servers. True, that the > > concept of filling flow which spans multiple pages is not tied to wallet, but > it > > is where we are starting. > > Please rename the tag in the server code. Baking "wallet" into the name is not > appropriate. Done. https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_xml_parser.h (right): https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_xml_parser.h:99: // Page number of present page in multipage autofill flow. On 2012/12/13 23:23:18, Ilya Sherman wrote: > On 2012/12/13 21:34:34, Raman Kakilate wrote: > > On 2012/12/13 02:29:23, Ilya Sherman wrote: > > > Why is the XML server responsible for keeping track of which page this is? > > > Shouldn't it always be the first page? > > > > Idea is show autofill_dialog_controller UI on the first page and gather all > the > > data required for the flow. and on subsequent pages, we should just fill the > > page and navigate. > > > > we would have to identify > > - start of the flow > > - being inside the flow and > > - the last step of the flow. > > > > All of that information can be inferred using page_number_ and total_pages_ > > That all makes sense. What I don't understand is why the server would ever send > down data for any page other than the first. I'd expect that when the user > navigates to the first page in the flow, the server simply sends down data for > that page as well as all subsequent pages in the flow. This allows us to avoid > the overhead of making a new connection to the server on each page load. What > am I missing? Server sending down data for all the possible flows at once is an option we haven't considered much. We could definitely do it as optimization (will involve changes in both chrome and autofill server). https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/wa... File chrome/browser/autofill/wallet_infobar_delegate.h (right): https://codereview.chromium.org/11539003/diff/3001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet_infobar_delegate.h:27: class WalletInfoBarDelegate : public ConfirmInfoBarDelegate { On 2012/12/13 23:23:18, Ilya Sherman wrote: > On 2012/12/13 21:34:34, Raman Kakilate wrote: > > On 2012/12/13 02:29:23, Ilya Sherman wrote: > > > This infobar is not specific to Wallet. The flow should work equally well > if > > > only local data is available. > > > > This infobar assumes the kind of data it needs from the user, which is tied to > > wallet. Extending this feature to also use local autofill data is probably out > > of scope of the present project. > > I don't see anything in the data that is tied to Wallet. The UI this infobar > invokes is already being designed to support both Wallet and local data sources, > so it is actually more work to restrict to only Wallet data; and is a worse user > experience on top of that. If you are still not convinced that restricting to > just Wallet data is not appropriate, we should probably schedule a meeting to > gain clarity on this point. Renamed it to AutofillFlowInfoBarDelegate as discussed offline. https://codereview.chromium.org/11539003/diff/12001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_xml_parser.cc (right): https://codereview.chromium.org/11539003/diff/12001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_xml_parser.cc:36: page_number_(-1), total_pages_(-1), On 2012/12/17 17:35:39, ahutter wrote: > separate lines. Done. https://codereview.chromium.org/11539003/diff/12001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_xml_parser.cc:99: if (attribute_name.compare("page_no") == 0) { On 2012/12/17 17:35:39, ahutter wrote: > Can't you use == instead of compare? being consistent with other uses in the file. https://codereview.chromium.org/11539003/diff/12001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_xml_parser.h (right): https://codereview.chromium.org/11539003/diff/12001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_xml_parser.h:73: int page_number() { return page_number_; } On 2012/12/17 17:35:39, ahutter wrote: > int page_number() const {... Done. https://codereview.chromium.org/11539003/diff/12001/chrome/browser/autofill/f... File chrome/browser/autofill/form_structure.h (right): https://codereview.chromium.org/11539003/diff/12001/chrome/browser/autofill/f... chrome/browser/autofill/form_structure.h:216: // doesn't belong to any autofill flow, it is set to 0. On 2012/12/17 17:35:39, ahutter wrote: > Should probably be -1 for clarity. Done. https://codereview.chromium.org/11539003/diff/12001/chrome/browser/autofill/w... File chrome/browser/autofill/wallet_infobar_delegate.cc (right): https://codereview.chromium.org/11539003/diff/12001/chrome/browser/autofill/w... chrome/browser/autofill/wallet_infobar_delegate.cc:32: FormData BuildWalletFormData() { On 2012/12/17 17:35:39, ahutter wrote: > Should probably be static. These are utility functions outside any object's scope. Moved them into autofill_flow_util. https://codereview.chromium.org/11539003/diff/12001/chrome/browser/autofill/w... chrome/browser/autofill/wallet_infobar_delegate.cc:40: formdata.fields.push_back(BuildField("billing address-line1")); On 2012/12/17 17:35:39, ahutter wrote: > Where's address line 2? changed to street-address.
https://codereview.chromium.org/11539003/diff/12001/chrome/browser/autofill/w... File chrome/browser/autofill/wallet_infobar_delegate.cc (right): https://codereview.chromium.org/11539003/diff/12001/chrome/browser/autofill/w... chrome/browser/autofill/wallet_infobar_delegate.cc:40: formdata.fields.push_back(BuildField("billing address-line1")); On 2013/01/10 00:54:40, Raman Kakilate wrote: > On 2012/12/17 17:35:39, ahutter wrote: > > Where's address line 2? > > changed to street-address. Not sure if it matters but Sugar expects address line 1 and address line 2 as separate fields. Ilya or Albert, can you comment on this?
I'm going through this today, but it's a large CL. Those can take a while to review. Is there any reasonable way to break it into smaller changes that can get through review more quickly? https://codereview.chromium.org/11539003/diff/12001/chrome/browser/autofill/w... File chrome/browser/autofill/wallet_infobar_delegate.cc (right): https://codereview.chromium.org/11539003/diff/12001/chrome/browser/autofill/w... chrome/browser/autofill/wallet_infobar_delegate.cc:40: formdata.fields.push_back(BuildField("billing address-line1")); Autofill also handles Address line 1 and line 2 as separate fields. IIRC "street-address" is a concatenation of the two. On 2013/01/10 15:58:54, ahutter wrote: > On 2013/01/10 00:54:40, Raman Kakilate wrote: > > On 2012/12/17 17:35:39, ahutter wrote: > > > Where's address line 2? > > > > changed to street-address. > > Not sure if it matters but Sugar expects address line 1 and address line 2 as > separate fields. Ilya or Albert, can you comment on this?
On 2013/01/10 17:54:27, Albert Bodenhamer wrote: > I'm going through this today, but it's a large CL. Those can take a while to > review. > > Is there any reasonable way to break it into smaller changes that can get > through review more quickly? > Breaking up this CL seems to be more painful. I will make sure all my next CLs are much smaller than this. Sorry for the trouble. Updated the CL description to elaborate on the changes. > https://codereview.chromium.org/11539003/diff/12001/chrome/browser/autofill/w... > File chrome/browser/autofill/wallet_infobar_delegate.cc (right): > > https://codereview.chromium.org/11539003/diff/12001/chrome/browser/autofill/w... > chrome/browser/autofill/wallet_infobar_delegate.cc:40: > formdata.fields.push_back(BuildField("billing address-line1")); > Autofill also handles Address line 1 and line 2 as separate fields. IIRC > "street-address" is a concatenation of the two. > > On 2013/01/10 15:58:54, ahutter wrote: > > On 2013/01/10 00:54:40, Raman Kakilate wrote: > > > On 2012/12/17 17:35:39, ahutter wrote: > > > > Where's address line 2? > > > > > > changed to street-address. > > > > Not sure if it matters but Sugar expects address line 1 and address line 2 as > > separate fields. Ilya or Albert, can you comment on this?
Ping {Ilya, Albert} :-)
On 2013/01/11 22:22:30, Raman Kakilate wrote:
> On 2013/01/10 17:54:27, Albert Bodenhamer wrote:
> > I'm going through this today, but it's a large CL. Those can take a while
to
> > review.
> >
> > Is there any reasonable way to break it into smaller changes that can get
> > through review more quickly?
> >
>
> Breaking up this CL seems to be more painful. I will make sure all my next CLs
> are much smaller than this. Sorry for the trouble.
>
> Updated the CL description to elaborate on the changes.
>
> >
>
https://codereview.chromium.org/11539003/diff/12001/chrome/browser/autofill/w...
> > File chrome/browser/autofill/wallet_infobar_delegate.cc (right):
> >
> >
>
https://codereview.chromium.org/11539003/diff/12001/chrome/browser/autofill/w...
> > chrome/browser/autofill/wallet_infobar_delegate.cc:40:
> > formdata.fields.push_back(BuildField("billing address-line1"));
> > Autofill also handles Address line 1 and line 2 as separate fields. IIRC
> > "street-address" is a concatenation of the two.
> >
> > On 2013/01/10 15:58:54, ahutter wrote:
> > > On 2013/01/10 00:54:40, Raman Kakilate wrote:
> > > > On 2012/12/17 17:35:39, ahutter wrote:
> > > > > Where's address line 2?
> > > >
> > > > changed to street-address.
> > >
> > > Not sure if it matters but Sugar expects address line 1 and address line 2
> as
> > > separate fields. Ilya or Albert, can you comment on this?
Sorry for the delay on this one. https://codereview.chromium.org/11539003/diff/22001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_flow_infobar_delegate.h (right): https://codereview.chromium.org/11539003/diff/22001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_infobar_delegate.h:35: static scoped_ptr<ConfirmInfoBarDelegate> Create( It would be good to unify the 2 different Create methods. #if defined(UNIT_TEST) is best avoided (code clutter and consistency) The best approach would probably be to have it always return the pointer and make the caller responsible for adding it to |infobar_service| At a minumum please use the same order for the params. https://codereview.chromium.org/11539003/diff/22001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_infobar_delegate.h:54: void LogUserAction(AutofillMetrics::InfoBarMetric user_action); LogUserAction makes me think that you're recording some debug output. I know the name is consistent with other metrics logging funtions. Maybe add a comment to make this a bit more clear? https://codereview.chromium.org/11539003/diff/22001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_metrics_unittest.cc (right): https://codereview.chromium.org/11539003/diff/22001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_metrics_unittest.cc:236: // no-op. Just used as callback into autofill_flow_infobar_delegate. Is the callback required? https://codereview.chromium.org/11539003/diff/22001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_metrics_unittest.cc:366: GURL url("www.google.com"); I think there's a header for getting google URLs. https://codereview.chromium.org/11539003/diff/22001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_xml_parser.cc (right): https://codereview.chromium.org/11539003/diff/22001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_xml_parser.cc:36: page_number_(-1), nit: I assume -1 is illegal? Maybe create a constant for it? https://codereview.chromium.org/11539003/diff/22001/chrome/browser/autofill/f... File chrome/browser/autofill/form_structure.h (right): https://codereview.chromium.org/11539003/diff/22001/chrome/browser/autofill/f... chrome/browser/autofill/form_structure.h:217: int page_number_; current_page_number_ ?
https://codereview.chromium.org/11539003/diff/22001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_flow_infobar_delegate.h (right): https://codereview.chromium.org/11539003/diff/22001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_infobar_delegate.h:35: static scoped_ptr<ConfirmInfoBarDelegate> Create( On 2013/01/14 22:10:18, Albert Bodenhamer wrote: > It would be good to unify the 2 different Create methods. #if > defined(UNIT_TEST) is best avoided (code clutter and consistency) > > The best approach would probably be to have it always return the pointer and > make the caller responsible for adding it to |infobar_service| > > At a minumum please use the same order for the params. I am being consistent with http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/autofill/au... Infobars were recently refactored to this state. CL: http://src.chromium.org/viewvc/chrome?view=rev&revision=175467 https://codereview.chromium.org/11539003/diff/22001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_infobar_delegate.h:54: void LogUserAction(AutofillMetrics::InfoBarMetric user_action); On 2013/01/14 22:10:18, Albert Bodenhamer wrote: > LogUserAction makes me think that you're recording some debug output. I know > the name is consistent with other metrics logging funtions. Maybe add a comment > to make this a bit more clear? Done. https://codereview.chromium.org/11539003/diff/22001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_metrics_unittest.cc (right): https://codereview.chromium.org/11539003/diff/22001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_metrics_unittest.cc:236: // no-op. Just used as callback into autofill_flow_infobar_delegate. On 2013/01/14 22:10:18, Albert Bodenhamer wrote: > Is the callback required? Yes, Actual AutofillManager's ShowAutofillFlowDialog will try to show UI, and hence tests would fail without this override. https://codereview.chromium.org/11539003/diff/22001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_metrics_unittest.cc:366: GURL url("www.google.com"); On 2013/01/14 22:10:18, Albert Bodenhamer wrote: > I think there's a header for getting google URLs. Done. https://codereview.chromium.org/11539003/diff/22001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_xml_parser.cc (right): https://codereview.chromium.org/11539003/diff/22001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_xml_parser.cc:36: page_number_(-1), On 2013/01/14 22:10:18, Albert Bodenhamer wrote: > nit: I assume -1 is illegal? Maybe create a constant for it? -1 is being used in similar fashion at http://code.google.com/searchframe#OAMlx_jo-ck/src/base/location.cc&exact_pac... http://code.google.com/searchframe#OAMlx_jo-ck/src/ui/aura/window.cc&exact_pa... https://codereview.chromium.org/11539003/diff/22001/chrome/browser/autofill/f... File chrome/browser/autofill/form_structure.h (right): https://codereview.chromium.org/11539003/diff/22001/chrome/browser/autofill/f... chrome/browser/autofill/form_structure.h:217: int page_number_; On 2013/01/14 22:10:18, Albert Bodenhamer wrote: > current_page_number_ ? Done.
https://codereview.chromium.org/11539003/diff/33001/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/11539003/diff/33001/chrome/app/generated_reso... chrome/app/generated_resources.grd:10570: + Do you want to use Accelarated Autofill ? nit: No space before the question mark. https://codereview.chromium.org/11539003/diff/33001/chrome/app/generated_reso... chrome/app/generated_resources.grd:10576: + No nit: "No thanks" is more common than "No" in Chrome UI, I think. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_flow_infobar_delegate.cc (right): https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_infobar_delegate.cc:12: #include "chrome/common/form_data.h" nit: Already included in the header file. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_infobar_delegate.cc:17: #include "googleurl/src/gurl.h" nit: Already included in the header file. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_infobar_delegate.cc:48: autofill_flow_form_data_ = autofill::BuildAutofillFlowFormData(); Why cache this, rather than just passing it to ShowAutofillFlowDialog when needed? https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_infobar_delegate.cc:98: content::SSLStatus ssl_status; nit: Remove these two locals, as they're not used. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_flow_infobar_delegate.h (right): https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_infobar_delegate.h:13: #include "chrome/browser/autofill/autofill_manager.h" nit: Forward-declare https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_infobar_delegate.h:17: #include "webkit/glue/window_open_disposition.h" nit: I'd omit this, as it's only used by a method that you're overriding, i.e. it should already have been included by the parent class's header file. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_infobar_delegate.h:26: class AutofillFlowInfoBarDelegate : public ConfirmInfoBarDelegate { nit: Since Alex has checked in other code that uses the term "Autocheckout", I'd prefer for all the uses of "AutofillFlow" in this CL to be "Autocheckout" instead. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_infobar_delegate.h:31: const AutofillMetrics* metric_logger, nit: Pass by const-ref, not const-pointer. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_infobar_delegate.h:33: const content::SSLStatus& ssl_status); nit: Const params should precede non-const ones. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_infobar_delegate.h:87: // AutofillFlow Formdata to be used. nit: This comment is almost completely redundant with the code. Perhaps: "The form the that will be filled if the user accepts the infobar"? https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_flow_util.cc (right): https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_util.cc:5: #include "chrome/browser/autofill/autofill_flow_util.h" nit: Leave a blank line after the corresponding header file's #include stmt. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_flow_util.h (right): https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_util.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. nit: 2013, since this is a new file. (Applies to all new files in this CL.) https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_util.h:17: FormData BuildAutofillFlowFormData(); Why are these exposed in their own header file? These seem like functions that could easily be in an anonymous namespace in the one file that uses them. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_util.h:17: FormData BuildAutofillFlowFormData(); If you do keep these functions here, please add documentation. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_manager.h (right): https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_manager.h:29: #include "chrome/common/form_data.h" nit: Move this to the .cc file. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_manager.h:111: const content::SSLStatus& ssl_status); Please create a new file named something like AutocheckoutManager to house all of the Autocheckout code. The AutofillManager class is already quite sprawling; let's not exacerbate that while the code is still easy to untangle. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_metrics_unittest.cc (right): https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_metrics_unittest.cc:236: const content::SSLStatus& ssl_status) { nit: virtual and OVERRIDE https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_metrics_unittest.cc:237: // no-op. Just used as callback into autofill_flow_infobar_delegate. nit: "into" -> "from" https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_xml_parser.cc (right): https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_xml_parser.cc:104: } nit: No need for curlies. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_xml_parser.h (right): https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_xml_parser.h:75: int total_pages() const { return total_pages_; } nit: For symmetry, please either store pointers to ints as private data members, or change the existing code to use accessors rather than storing pointers to structures declared externally to this class. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/f... File chrome/browser/autofill/form_structure.cc (right): https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/f... chrome/browser/autofill/form_structure.cc:275: } nit: Please place these methods in the same place in the implementation file as in the header file. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/f... File chrome/browser/autofill/form_structure.h (right): https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/f... chrome/browser/autofill/form_structure.h:129: bool IsInAutofillableFlow() const; nit: Please add documentation for these methods. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/f... chrome/browser/autofill/form_structure.h:216: // doesn't belong to any autofill flow, it is set to -1. What if the received XML specifies that the page number is -1? Where does error-checking take place? https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/f... chrome/browser/autofill/form_structure.h:216: // doesn't belong to any autofill flow, it is set to -1. Is this zero-indexed or 1-indexed? https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/f... File chrome/browser/autofill/form_structure_unittest.cc (right): https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/f... chrome/browser/autofill/form_structure_unittest.cc:103: EXPECT_FALSE(form_structure.IsInAutofillableFlow()); Please add more thorough tests for these methods. As is, they could both be implemented as simply "return false;" https://codereview.chromium.org/11539003/diff/33001/chrome/common/form_data.h File chrome/common/form_data.h (right): https://codereview.chromium.org/11539003/diff/33001/chrome/common/form_data.h... chrome/common/form_data.h:29: // SSL status of the URL contatining the form. nit: "URL" -> "frame". https://codereview.chromium.org/11539003/diff/33001/chrome/renderer/autofill/... File chrome/renderer/autofill/autofill_agent.cc (right): https://codereview.chromium.org/11539003/diff/33001/chrome/renderer/autofill/... chrome/renderer/autofill/autofill_agent.cc:242: render_view()->GetSSLStatusOfFrame(frame))); nit: It seems odd that this IPC message sends the SSL status separately, whereas the other sends it as part of the form_data. Either way is fine, but please make them consistent.
https://codereview.chromium.org/11539003/diff/33001/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/11539003/diff/33001/chrome/app/generated_reso... chrome/app/generated_resources.grd:10570: + Do you want to use Accelarated Autofill ? On 2013/01/15 06:31:52, Ilya Sherman wrote: > nit: No space before the question mark. Done. https://codereview.chromium.org/11539003/diff/33001/chrome/app/generated_reso... chrome/app/generated_resources.grd:10576: + No On 2013/01/15 06:31:52, Ilya Sherman wrote: > nit: "No thanks" is more common than "No" in Chrome UI, I think. Done. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_flow_infobar_delegate.cc (right): https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_infobar_delegate.cc:12: #include "chrome/common/form_data.h" On 2013/01/15 06:31:52, Ilya Sherman wrote: > nit: Already included in the header file. Done. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_infobar_delegate.cc:17: #include "googleurl/src/gurl.h" On 2013/01/15 06:31:52, Ilya Sherman wrote: > nit: Already included in the header file. Done. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_infobar_delegate.cc:48: autofill_flow_form_data_ = autofill::BuildAutofillFlowFormData(); On 2013/01/15 06:31:52, Ilya Sherman wrote: > Why cache this, rather than just passing it to ShowAutofillFlowDialog when > needed? Done. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_infobar_delegate.cc:98: content::SSLStatus ssl_status; On 2013/01/15 06:31:52, Ilya Sherman wrote: > nit: Remove these two locals, as they're not used. My bad, Removed. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_flow_infobar_delegate.h (right): https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_infobar_delegate.h:13: #include "chrome/browser/autofill/autofill_manager.h" On 2013/01/15 06:31:52, Ilya Sherman wrote: > nit: Forward-declare Done. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_infobar_delegate.h:17: #include "webkit/glue/window_open_disposition.h" On 2013/01/15 06:31:52, Ilya Sherman wrote: > nit: I'd omit this, as it's only used by a method that you're overriding, i.e. > it should already have been included by the parent class's header file. Done. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_infobar_delegate.h:26: class AutofillFlowInfoBarDelegate : public ConfirmInfoBarDelegate { On 2013/01/15 06:31:52, Ilya Sherman wrote: > nit: Since Alex has checked in other code that uses the term "Autocheckout", I'd > prefer for all the uses of "AutofillFlow" in this CL to be "Autocheckout" > instead. Done. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_infobar_delegate.h:31: const AutofillMetrics* metric_logger, On 2013/01/15 06:31:52, Ilya Sherman wrote: > nit: Pass by const-ref, not const-pointer. I have been consistent with autofill_cc_inforbar_delegate. Will try to change both of them in another CL. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_infobar_delegate.h:33: const content::SSLStatus& ssl_status); On 2013/01/15 06:31:52, Ilya Sherman wrote: > nit: Const params should precede non-const ones. Done. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_infobar_delegate.h:87: // AutofillFlow Formdata to be used. On 2013/01/15 06:31:52, Ilya Sherman wrote: > nit: This comment is almost completely redundant with the code. Perhaps: "The > form the that will be filled if the user accepts the infobar"? Removed. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_flow_util.cc (right): https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_util.cc:5: #include "chrome/browser/autofill/autofill_flow_util.h" On 2013/01/15 06:31:52, Ilya Sherman wrote: > nit: Leave a blank line after the corresponding header file's #include stmt. Done. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_flow_util.h (right): https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_util.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/01/15 06:31:52, Ilya Sherman wrote: > nit: 2013, since this is a new file. (Applies to all new files in this CL.) Done. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_util.h:17: FormData BuildAutofillFlowFormData(); On 2013/01/15 06:31:52, Ilya Sherman wrote: > If you do keep these functions here, please add documentation. Done. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_util.h:17: FormData BuildAutofillFlowFormData(); On 2013/01/15 06:31:52, Ilya Sherman wrote: > Why are these exposed in their own header file? These seem like functions that > could easily be in an anonymous namespace in the one file that uses them. You asked me move out of infobars as they are complex by themselves. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_manager.h (right): https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_manager.h:29: #include "chrome/common/form_data.h" On 2013/01/15 06:31:52, Ilya Sherman wrote: > nit: Move this to the .cc file. Removed here. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_manager.h:111: const content::SSLStatus& ssl_status); On 2013/01/15 06:31:52, Ilya Sherman wrote: > Please create a new file named something like AutocheckoutManager to house all > of the Autocheckout code. The AutofillManager class is already quite sprawling; > let's not exacerbate that while the code is still easy to untangle. I am little confused about what you mean by AutocheckoutManager. Should it inherit WebContentsObserver similar to AutofillManager and listen to similar messages from renderer and plug the popping up UI thing from AutocheckoutManager ? If yes, we might have to extract FormStructure once more in AutocheckoutManager, and this approach seems like overkill. Or do you mean AutocheckoutManager to be an object contained within AutofillManager (exposing ShowAutocheckoutFlowDialog and ReturnAutocheckoutFlowData methods) ? In this case, we need reference of web_content() to be passed to AutocheckoutManager from Autofill server to init AutofillDialogController. Non of the options look good. Please advice. Present approach I took is extract any utilities required for autocheckout into autocheckout_util, and have the browser->render interactions done in autofill_manager itself. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_metrics_unittest.cc (right): https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_metrics_unittest.cc:236: const content::SSLStatus& ssl_status) { On 2013/01/15 06:31:52, Ilya Sherman wrote: > nit: virtual and OVERRIDE Done. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_metrics_unittest.cc:237: // no-op. Just used as callback into autofill_flow_infobar_delegate. On 2013/01/15 06:31:52, Ilya Sherman wrote: > nit: "into" -> "from" Done. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_xml_parser.cc (right): https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_xml_parser.cc:104: } On 2013/01/15 06:31:52, Ilya Sherman wrote: > nit: No need for curlies. Done. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/f... File chrome/browser/autofill/form_structure.cc (right): https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/f... chrome/browser/autofill/form_structure.cc:275: } On 2013/01/15 06:31:52, Ilya Sherman wrote: > nit: Please place these methods in the same place in the implementation file as > in the header file. Moved it after ParseFieldsTypeFromAutocompleteAttributes. but cc file doesn't have other methods correctly ordered (probably should be cleaned up in a different CL). https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/f... File chrome/browser/autofill/form_structure.h (right): https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/f... chrome/browser/autofill/form_structure.h:129: bool IsInAutofillableFlow() const; On 2013/01/15 06:31:52, Ilya Sherman wrote: > nit: Please add documentation for these methods. Done. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/f... chrome/browser/autofill/form_structure.h:216: // doesn't belong to any autofill flow, it is set to -1. On 2013/01/15 06:31:52, Ilya Sherman wrote: > Is this zero-indexed or 1-indexed? Zero indexed. Added in the comment. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/f... chrome/browser/autofill/form_structure.h:216: // doesn't belong to any autofill flow, it is set to -1. On 2013/01/15 06:31:52, Ilya Sherman wrote: > What if the received XML specifies that the page number is -1? Where does > error-checking take place? Updated implementation of IsStartOfAutofillableFlow() to also check for (total_pages_ > 0). If server returns -1, it would be treated as outside autofillable flow. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/f... File chrome/browser/autofill/form_structure_unittest.cc (right): https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/f... chrome/browser/autofill/form_structure_unittest.cc:103: EXPECT_FALSE(form_structure.IsInAutofillableFlow()); On 2013/01/15 06:31:52, Ilya Sherman wrote: > Please add more thorough tests for these methods. As is, they could both be > implemented as simply "return false;" Added more tests. https://codereview.chromium.org/11539003/diff/33001/chrome/common/form_data.h File chrome/common/form_data.h (right): https://codereview.chromium.org/11539003/diff/33001/chrome/common/form_data.h... chrome/common/form_data.h:29: // SSL status of the URL contatining the form. On 2013/01/15 06:31:52, Ilya Sherman wrote: > nit: "URL" -> "frame". Done. https://codereview.chromium.org/11539003/diff/33001/chrome/renderer/autofill/... File chrome/renderer/autofill/autofill_agent.cc (right): https://codereview.chromium.org/11539003/diff/33001/chrome/renderer/autofill/... chrome/renderer/autofill/autofill_agent.cc:242: render_view()->GetSSLStatusOfFrame(frame))); On 2013/01/15 06:31:52, Ilya Sherman wrote: > nit: It seems odd that this IPC message sends the SSL status separately, whereas > the other sends it as part of the form_data. Either way is fine, but please > make them consistent. Will update the IPC in a separate CL.
https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_flow_infobar_delegate.h (right): https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_infobar_delegate.h:31: const AutofillMetrics* metric_logger, On 2013/01/15 23:02:33, Raman Kakilate wrote: > On 2013/01/15 06:31:52, Ilya Sherman wrote: > > nit: Pass by const-ref, not const-pointer. > > I have been consistent with autofill_cc_inforbar_delegate. Will try to change > both of them in another CL. This isn't desirable consistency, since the style in that file is wrong. Let's just fix it here while you're here, and then fix it there in a separate CL. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_flow_util.h (right): https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_util.h:17: FormData BuildAutofillFlowFormData(); On 2013/01/15 23:02:33, Raman Kakilate wrote: > On 2013/01/15 06:31:52, Ilya Sherman wrote: > > Why are these exposed in their own header file? These seem like functions > that > > could easily be in an anonymous namespace in the one file that uses them. > > You asked me move out of infobars as they are complex by themselves. The infobar code is still calling this, so you've just moved the definitions of the functions out. What I meant with my previous was that the infobar code shouldn't need to worry about this at all -- I'd expect this to instead be within the AutocheckoutManager implementation. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_manager.h (right): https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_manager.h:111: const content::SSLStatus& ssl_status); On 2013/01/15 23:02:33, Raman Kakilate wrote: > On 2013/01/15 06:31:52, Ilya Sherman wrote: > > Please create a new file named something like AutocheckoutManager to house all > > of the Autocheckout code. The AutofillManager class is already quite > sprawling; > > let's not exacerbate that while the code is still easy to untangle. > > I am little confused about what you mean by AutocheckoutManager. Should it > inherit WebContentsObserver similar to AutofillManager and listen to similar > messages from renderer and plug the popping up UI thing from AutocheckoutManager > ? If yes, we might have to extract FormStructure once more in > AutocheckoutManager, and this approach seems like overkill. > > Or do you mean AutocheckoutManager to be an object contained within > AutofillManager (exposing ShowAutocheckoutFlowDialog and > ReturnAutocheckoutFlowData methods) ? In this case, we need reference of > web_content() to be passed to AutocheckoutManager from Autofill server to init > AutofillDialogController. > > Non of the options look good. Please advice. > Present approach I took is extract any utilities required for autocheckout into > autocheckout_util, and have the browser->render interactions done in > autofill_manager itself. I mean option (2): Have the AutofillManager call methods on AutocheckoutManager, and pass in any needed dependencies. What do you dislike about this approach? https://codereview.chromium.org/11539003/diff/33001/chrome/renderer/autofill/... File chrome/renderer/autofill/autofill_agent.cc (right): https://codereview.chromium.org/11539003/diff/33001/chrome/renderer/autofill/... chrome/renderer/autofill/autofill_agent.cc:242: render_view()->GetSSLStatusOfFrame(frame))); On 2013/01/15 23:02:33, Raman Kakilate wrote: > On 2013/01/15 06:31:52, Ilya Sherman wrote: > > nit: It seems odd that this IPC message sends the SSL status separately, > whereas > > the other sends it as part of the form_data. Either way is fine, but please > > make them consistent. > > Will update the IPC in a separate CL. If so, please add a TODO in this CL, so that this is less likely to get lost.
https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_flow_infobar_delegate.h (right): https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_infobar_delegate.h:31: const AutofillMetrics* metric_logger, +1 I'm not sure why it was done that way in the other file. On 2013/01/15 23:24:46, Ilya Sherman wrote: > On 2013/01/15 23:02:33, Raman Kakilate wrote: > > On 2013/01/15 06:31:52, Ilya Sherman wrote: > > > nit: Pass by const-ref, not const-pointer. > > > > I have been consistent with autofill_cc_inforbar_delegate. Will try to change > > both of them in another CL. > > This isn't desirable consistency, since the style in that file is wrong. Let's > just fix it here while you're here, and then fix it there in a separate CL.
https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_flow_infobar_delegate.h (right): https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_infobar_delegate.h:31: const AutofillMetrics* metric_logger, probably my sloppy c++ skills at show here. * AutofillMetrics disallows COPY and ASSIGN. * For passing it as const reference, object needs to be copy'able ? Other option is pass is as simple pointer instead of const pointer. What should be correct way to handle this ? On 2013/01/16 01:08:26, Albert Bodenhamer wrote: > +1 I'm not sure why it was done that way in the other file. > > On 2013/01/15 23:24:46, Ilya Sherman wrote: > > On 2013/01/15 23:02:33, Raman Kakilate wrote: > > > On 2013/01/15 06:31:52, Ilya Sherman wrote: > > > > nit: Pass by const-ref, not const-pointer. > > > > > > I have been consistent with autofill_cc_inforbar_delegate. Will try to > change > > > both of them in another CL. > > > > This isn't desirable consistency, since the style in that file is wrong. > Let's > > just fix it here while you're here, and then fix it there in a separate CL. > https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_flow_util.h (right): https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_util.h:17: FormData BuildAutofillFlowFormData(); On 2013/01/15 23:24:46, Ilya Sherman wrote: > On 2013/01/15 23:02:33, Raman Kakilate wrote: > > On 2013/01/15 06:31:52, Ilya Sherman wrote: > > > Why are these exposed in their own header file? These seem like functions > > that > > > could easily be in an anonymous namespace in the one file that uses them. > > > > You asked me move out of infobars as they are complex by themselves. > > The infobar code is still calling this, so you've just moved the definitions of > the functions out. What I meant with my previous was that the infobar code > shouldn't need to worry about this at all -- I'd expect this to instead be > within the AutocheckoutManager implementation. created AutocheckoutManager and moved the functions there. https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_manager.h (right): https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_manager.h:111: const content::SSLStatus& ssl_status); On 2013/01/15 23:24:46, Ilya Sherman wrote: > On 2013/01/15 23:02:33, Raman Kakilate wrote: > > On 2013/01/15 06:31:52, Ilya Sherman wrote: > > > Please create a new file named something like AutocheckoutManager to house > all > > > of the Autocheckout code. The AutofillManager class is already quite > > sprawling; > > > let's not exacerbate that while the code is still easy to untangle. > > > > I am little confused about what you mean by AutocheckoutManager. Should it > > inherit WebContentsObserver similar to AutofillManager and listen to similar > > messages from renderer and plug the popping up UI thing from > AutocheckoutManager > > ? If yes, we might have to extract FormStructure once more in > > AutocheckoutManager, and this approach seems like overkill. > > > > Or do you mean AutocheckoutManager to be an object contained within > > AutofillManager (exposing ShowAutocheckoutFlowDialog and > > ReturnAutocheckoutFlowData methods) ? In this case, we need reference of > > web_content() to be passed to AutocheckoutManager from Autofill server to init > > AutofillDialogController. > > > > Non of the options look good. Please advice. > > Present approach I took is extract any utilities required for autocheckout > into > > autocheckout_util, and have the browser->render interactions done in > > autofill_manager itself. > > I mean option (2): Have the AutofillManager call methods on AutocheckoutManager, > and pass in any needed dependencies. What do you dislike about this approach? Ack. Refactored code. I disliked AutofillManager acting as a proxy, but I now think its fine. https://codereview.chromium.org/11539003/diff/33001/chrome/renderer/autofill/... File chrome/renderer/autofill/autofill_agent.cc (right): https://codereview.chromium.org/11539003/diff/33001/chrome/renderer/autofill/... chrome/renderer/autofill/autofill_agent.cc:242: render_view()->GetSSLStatusOfFrame(frame))); On 2013/01/15 23:24:46, Ilya Sherman wrote: > On 2013/01/15 23:02:33, Raman Kakilate wrote: > > On 2013/01/15 06:31:52, Ilya Sherman wrote: > > > nit: It seems odd that this IPC message sends the SSL status separately, > > whereas > > > the other sends it as part of the form_data. Either way is fine, but please > > > make them consistent. > > > > Will update the IPC in a separate CL. > > If so, please add a TODO in this CL, so that this is less likely to get lost. Done.
https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_flow_infobar_delegate.h (right): https://codereview.chromium.org/11539003/diff/33001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_flow_infobar_delegate.h:31: const AutofillMetrics* metric_logger, On 2013/01/16 19:20:12, Raman Kakilate wrote: > probably my sloppy c++ skills at show here. > > * AutofillMetrics disallows COPY and ASSIGN. > * For passing it as const reference, object needs to be copy'able ? The object does not need to be copyable to be passed by const-reference. A reference is just a pointer that can't be NULL, plus some syntactic sugar. You can store the the logger either as a const-reference (preferred, IMO) or a const-pointer. If you try to store it as a direct member instead, that will introduce a copy, which will fail because the type is not copyable. https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout_infobar_delegate.h (right): https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_infobar_delegate.h:32: AutofillManager* autofill_manager, This infobar should only need to know about the AutocheckoutManager; not about the AutofillManager. https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout_manager.cc (right): https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_manager.cc:53: const GURL& frame_url, const SSLStatus& ssl_status, nit: Each parameter should be on its own line. https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout_manager.h (right): https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_manager.h:16: struct FormFieldData; nit: These should be interleaved with the above. https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_manager.h:20: struct SSLStatus; nit: alphabetize https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_manager.h:23: class AutocheckoutManager : public base::RefCounted<AutocheckoutManager>{ This class should almost certainly not be reference counted. https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_manager.h:27: content::WebContents* web_contents); The AutocheckoutManager should have a pointer back to the AutofillManager so that it can query it for the WebContents. https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_manager.h:38: static FormData BuildAutocheckoutFormData(); nit: Please tuck these into an anonymous namespace in the implementation file. https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_manager.h:44: }; nit: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_manager.h (right): https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_manager.h:36: class AutocheckoutManager; nit: Omit this; it's redundant with the #include above. https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_manager.h:117: const content::SSLStatus& ssl_status); This should be part of the AutocheckoutManager class. https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_manager.h:349: scoped_refptr<AutocheckoutManager> autocheckout_manager_; Why is this class reference counted? You should just be able to declare this as "AutocheckoutManager autocheckout_manager_;" https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/f... File chrome/browser/autofill/form_structure_unittest.cc (right): https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/f... chrome/browser/autofill/form_structure_unittest.cc:68: } nit: Please move this into the anonymous namespace.
https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout_infobar_delegate.h (right): https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_infobar_delegate.h:32: AutofillManager* autofill_manager, On 2013/01/17 01:21:44, Ilya Sherman wrote: > This infobar should only need to know about the AutocheckoutManager; not about > the AutofillManager. web_contents() is a protected method in AutofillManager. AutcheckoutManager cannot benefit by having pointer to AutofillManager, hence AutofillManager is proxying the call, which is sub-optimal. https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout_manager.cc (right): https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_manager.cc:53: const GURL& frame_url, const SSLStatus& ssl_status, On 2013/01/17 01:21:44, Ilya Sherman wrote: > nit: Each parameter should be on its own line. Done. https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout_manager.h (right): https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_manager.h:16: struct FormFieldData; On 2013/01/17 01:21:44, Ilya Sherman wrote: > nit: These should be interleaved with the above. autofill_manager.h also orders classes followed by structs. I couldn't find anything in style guide. https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_manager.h:20: struct SSLStatus; On 2013/01/17 01:21:44, Ilya Sherman wrote: > nit: alphabetize used class'es followed by struct's https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_manager.h:23: class AutocheckoutManager : public base::RefCounted<AutocheckoutManager>{ On 2013/01/17 01:21:44, Ilya Sherman wrote: > This class should almost certainly not be reference counted. I don't completely understand base:Bind, but from what I could get, it seemed like it needed RefCounted. Compiler failed if it is not. https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_manager.h:27: content::WebContents* web_contents); On 2013/01/17 01:21:44, Ilya Sherman wrote: > The AutocheckoutManager should have a pointer back to the AutofillManager so > that it can query it for the WebContents. AutofillManager.web_contents() is a protected method. Making it public didn't look like good option. https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_manager.h:38: static FormData BuildAutocheckoutFormData(); On 2013/01/17 01:21:44, Ilya Sherman wrote: > nit: Please tuck these into an anonymous namespace in the implementation file. Done. https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_manager.h:44: }; On 2013/01/17 01:21:44, Ilya Sherman wrote: > nit: DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_manager.h (right): https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_manager.h:36: class AutocheckoutManager; On 2013/01/17 01:21:44, Ilya Sherman wrote: > nit: Omit this; it's redundant with the #include above. Done. https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_manager.h:117: const content::SSLStatus& ssl_status); On 2013/01/17 01:21:44, Ilya Sherman wrote: > This should be part of the AutocheckoutManager class. this method is required to proxy web_contents() to AutocheckoutManager. Please suggest if you see any other options. https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_manager.h:349: scoped_refptr<AutocheckoutManager> autocheckout_manager_; On 2013/01/17 01:21:44, Ilya Sherman wrote: > Why is this class reference counted? You should just be able to declare this as > "AutocheckoutManager autocheckout_manager_;" AutcheckoutManager::ReturnAutocheckoutData has to be a callback and hence RefCounted (http://code.google.com/searchframe#OAMlx_jo-ck/src/base/bind_internal.h&exact...). I might be missing a trick here. https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/f... File chrome/browser/autofill/form_structure_unittest.cc (right): https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/f... chrome/browser/autofill/form_structure_unittest.cc:68: } On 2013/01/17 01:21:44, Ilya Sherman wrote: > nit: Please move this into the anonymous namespace. I am accessing private members of FormStructure, which friends FormStructureTest. again, I might be missing something.
https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout_manager.h (right): https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_manager.h:23: class AutocheckoutManager : public base::RefCounted<AutocheckoutManager>{ On 2013/01/17 06:40:33, Raman Kakilate wrote: > On 2013/01/17 01:21:44, Ilya Sherman wrote: > > This class should almost certainly not be reference counted. > > I don't completely understand base:Bind, but from what I could get, it seemed > like it needed RefCounted. Compiler failed if it is not. For base::Bind, you want a WeakPtr, rather than reference counting. https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_manager.h:27: content::WebContents* web_contents); On 2013/01/17 06:40:33, Raman Kakilate wrote: > On 2013/01/17 01:21:44, Ilya Sherman wrote: > > The AutocheckoutManager should have a pointer back to the AutofillManager so > > that it can query it for the WebContents. > > AutofillManager.web_contents() is a protected method. Making it public didn't > look like good option. IMO it would be better to make it public -- in fact, I thought it already was. There's really not much benefit that I can see to keeping it protected, and instead proxying any requests that rely on it. https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/f... File chrome/browser/autofill/form_structure_unittest.cc (right): https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/f... chrome/browser/autofill/form_structure_unittest.cc:68: } On 2013/01/17 06:40:33, Raman Kakilate wrote: > On 2013/01/17 01:21:44, Ilya Sherman wrote: > > nit: Please move this into the anonymous namespace. > > I am accessing private members of FormStructure, which friends > FormStructureTest. Ah, I missed that, sorry. The "right" thing to do is usually not to friend the test, but rather to expose protected accessors to the private data members, and then to create a TestFormStructure class used for testing that exposes those accessors publicly. Since it looks like the test is already friended in this case, though, I guess your current approach is fine for this CL.
https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout_manager.h (right): https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_manager.h:23: class AutocheckoutManager : public base::RefCounted<AutocheckoutManager>{ On 2013/01/17 10:50:23, Ilya Sherman wrote: > On 2013/01/17 06:40:33, Raman Kakilate wrote: > > On 2013/01/17 01:21:44, Ilya Sherman wrote: > > > This class should almost certainly not be reference counted. > > > > I don't completely understand base:Bind, but from what I could get, it seemed > > like it needed RefCounted. Compiler failed if it is not. > > For base::Bind, you want a WeakPtr, rather than reference counting. Isn't WeakPtr doing more complex operations than ref counting ? I remember seeing compiler complain about absence of AddRef() and Release() API on AutocheckoutManager, which doesn't seem to be present in WeakPtr. I will give it a try anyways. https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_manager.h:27: content::WebContents* web_contents); On 2013/01/17 10:50:23, Ilya Sherman wrote: > On 2013/01/17 06:40:33, Raman Kakilate wrote: > > On 2013/01/17 01:21:44, Ilya Sherman wrote: > > > The AutocheckoutManager should have a pointer back to the AutofillManager so > > > that it can query it for the WebContents. > > > > AutofillManager.web_contents() is a protected method. Making it public didn't > > look like good option. > > IMO it would be better to make it public -- in fact, I thought it already was. > There's really not much benefit that I can see to keeping it protected, and > instead proxying any requests that rely on it. ah sorry, I meant http://code.google.com/searchframe#OAMlx_jo-ck/src/content/public/browser/web... defines web_contents() as protected, probably for a good reason. AutofillManager extends WebContentsObserver. I wanted to avoid breaking this encapsulation by exposing web_contents_ through a public API. https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/f... File chrome/browser/autofill/form_structure_unittest.cc (right): https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/f... chrome/browser/autofill/form_structure_unittest.cc:68: } On 2013/01/17 10:50:23, Ilya Sherman wrote: > On 2013/01/17 06:40:33, Raman Kakilate wrote: > > On 2013/01/17 01:21:44, Ilya Sherman wrote: > > > nit: Please move this into the anonymous namespace. > > > > I am accessing private members of FormStructure, which friends > > FormStructureTest. > > Ah, I missed that, sorry. The "right" thing to do is usually not to friend the > test, but rather to expose protected accessors to the private data members, and > then to create a TestFormStructure class used for testing that exposes those > accessors publicly. Since it looks like the test is already friended in this > case, though, I guess your current approach is fine for this CL. Ack.
https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout_manager.h (right): https://codereview.chromium.org/11539003/diff/44001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_manager.h:23: class AutocheckoutManager : public base::RefCounted<AutocheckoutManager>{ On 2013/01/17 16:52:30, Raman Kakilate wrote: > On 2013/01/17 10:50:23, Ilya Sherman wrote: > > On 2013/01/17 06:40:33, Raman Kakilate wrote: > > > On 2013/01/17 01:21:44, Ilya Sherman wrote: > > > > This class should almost certainly not be reference counted. > > > > > > I don't completely understand base:Bind, but from what I could get, it > seemed > > > like it needed RefCounted. Compiler failed if it is not. > > > > For base::Bind, you want a WeakPtr, rather than reference counting. > > Isn't WeakPtr doing more complex operations than ref counting ? > > I remember seeing compiler complain about absence of AddRef() and Release() API > on AutocheckoutManager, which doesn't seem to be present in WeakPtr. I will give > it a try anyways. Done. I was wrong, Moved to WeakPtr based implementation.
https://codereview.chromium.org/11539003/diff/49005/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout_manager.h (right): https://codereview.chromium.org/11539003/diff/49005/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_manager.h:31: ~AutocheckoutManager(); nit: Please include a blank line after this one. https://codereview.chromium.org/11539003/diff/49005/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_manager.cc (right): https://codereview.chromium.org/11539003/diff/49005/chrome/browser/autofill/a... chrome/browser/autofill/autofill_manager.cc:587: *metric_logger_.get(), nit: Omit the ".get()" https://codereview.chromium.org/11539003/diff/49005/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_manager.h (right): https://codereview.chromium.org/11539003/diff/49005/chrome/browser/autofill/a... chrome/browser/autofill/autofill_manager.h:348: scoped_ptr<AutocheckoutManager> autocheckout_manager_; nit: "AutocheckoutManager autocheckout_manager_;" (no scoped_ptr needed).
Refactored AutocheckoutManager to now look at AutofillManager.GetWebContents() instead of AutofillManager proxying web_contents. https://codereview.chromium.org/11539003/diff/49005/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout_manager.h (right): https://codereview.chromium.org/11539003/diff/49005/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_manager.h:31: ~AutocheckoutManager(); On 2013/01/17 23:48:12, Ilya Sherman wrote: > nit: Please include a blank line after this one. Done. https://codereview.chromium.org/11539003/diff/49005/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_manager.cc (right): https://codereview.chromium.org/11539003/diff/49005/chrome/browser/autofill/a... chrome/browser/autofill/autofill_manager.cc:587: *metric_logger_.get(), On 2013/01/17 23:48:12, Ilya Sherman wrote: > nit: Omit the ".get()" Done. https://codereview.chromium.org/11539003/diff/49005/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_manager.h (right): https://codereview.chromium.org/11539003/diff/49005/chrome/browser/autofill/a... chrome/browser/autofill/autofill_manager.h:348: scoped_ptr<AutocheckoutManager> autocheckout_manager_; On 2013/01/17 23:48:12, Ilya Sherman wrote: > nit: "AutocheckoutManager autocheckout_manager_;" (no scoped_ptr needed). Done.
LGTM with nits: https://codereview.chromium.org/11539003/diff/66003/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout_infobar_delegate.h (right): https://codereview.chromium.org/11539003/diff/66003/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_infobar_delegate.h:13: #include "chrome/browser/autofill/autofill_metrics.h" nit: Forward-declare rather than #including. https://codereview.chromium.org/11539003/diff/66003/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_infobar_delegate.h:14: #include "chrome/common/form_data.h" nit: Not used. https://codereview.chromium.org/11539003/diff/66003/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_infobar_delegate.h:32: AutocheckoutManager* autofill_manager, nit: Please rename this variable. (Applies throughout.) https://codereview.chromium.org/11539003/diff/66003/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout_manager.h (right): https://codereview.chromium.org/11539003/diff/66003/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_manager.h:17: struct FormFieldData; nit: Not needed. https://codereview.chromium.org/11539003/diff/66003/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_manager.h:20: class WebContents; nit: Not needed. https://codereview.chromium.org/11539003/diff/66003/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_manager.h:31: ~AutocheckoutManager(); nit: Since this class includes a virtual method, the destructor should be virtual. https://codereview.chromium.org/11539003/diff/66003/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_manager.h:38: AutofillManager* autofill_manager_; nit: Please add the following comment: "// WEAK; owns us". https://codereview.chromium.org/11539003/diff/66003/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_manager.h (right): https://codereview.chromium.org/11539003/diff/66003/chrome/browser/autofill/a... chrome/browser/autofill/autofill_manager.h:113: content::WebContents* GetWebContents() const; nit: This method should either return a const reference to the WebContents, or it shouldn't be marked const. https://codereview.chromium.org/11539003/diff/66003/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_metrics_unittest.cc (right): https://codereview.chromium.org/11539003/diff/66003/chrome/browser/autofill/a... chrome/browser/autofill/autofill_metrics_unittest.cc:267: } nit: It's best to explicitly include a (virtual) destructor with an empty body.
lgtm
+sky for chrome/common, gypi and chrome/app/generated_resources.grd changes. https://codereview.chromium.org/11539003/diff/66003/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout_infobar_delegate.h (right): https://codereview.chromium.org/11539003/diff/66003/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_infobar_delegate.h:13: #include "chrome/browser/autofill/autofill_metrics.h" On 2013/01/18 01:45:02, Ilya Sherman wrote: > nit: Forward-declare rather than #including. Done. https://codereview.chromium.org/11539003/diff/66003/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_infobar_delegate.h:14: #include "chrome/common/form_data.h" On 2013/01/18 01:45:02, Ilya Sherman wrote: > nit: Not used. Done. https://codereview.chromium.org/11539003/diff/66003/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_infobar_delegate.h:32: AutocheckoutManager* autofill_manager, On 2013/01/18 01:45:02, Ilya Sherman wrote: > nit: Please rename this variable. (Applies throughout.) Done. https://codereview.chromium.org/11539003/diff/66003/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout_manager.h (right): https://codereview.chromium.org/11539003/diff/66003/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_manager.h:17: struct FormFieldData; On 2013/01/18 01:45:02, Ilya Sherman wrote: > nit: Not needed. Done. https://codereview.chromium.org/11539003/diff/66003/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_manager.h:20: class WebContents; On 2013/01/18 01:45:02, Ilya Sherman wrote: > nit: Not needed. Done. https://codereview.chromium.org/11539003/diff/66003/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_manager.h:31: ~AutocheckoutManager(); On 2013/01/18 01:45:02, Ilya Sherman wrote: > nit: Since this class includes a virtual method, the destructor should be > virtual. Done. https://codereview.chromium.org/11539003/diff/66003/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout_manager.h:38: AutofillManager* autofill_manager_; On 2013/01/18 01:45:02, Ilya Sherman wrote: > nit: Please add the following comment: "// WEAK; owns us". Done. https://codereview.chromium.org/11539003/diff/66003/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_manager.h (right): https://codereview.chromium.org/11539003/diff/66003/chrome/browser/autofill/a... chrome/browser/autofill/autofill_manager.h:113: content::WebContents* GetWebContents() const; On 2013/01/18 01:45:02, Ilya Sherman wrote: > nit: This method should either return a const reference to the WebContents, or > it shouldn't be marked const. Done. https://codereview.chromium.org/11539003/diff/66003/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_metrics_unittest.cc (right): https://codereview.chromium.org/11539003/diff/66003/chrome/browser/autofill/a... chrome/browser/autofill/autofill_metrics_unittest.cc:267: } On 2013/01/18 01:45:02, Ilya Sherman wrote: > nit: It's best to explicitly include a (virtual) destructor with an empty body. Done.
https://codereview.chromium.org/11539003/diff/68026/chrome/common/form_data.cc File chrome/common/form_data.cc (right): https://codereview.chromium.org/11539003/diff/68026/chrome/common/form_data.c... chrome/common/form_data.cc:28: StringToLowerASCII(method) == StringToLowerASCII(form.method) && Can method be an enum? That way we don't need StringToLowerASCII and you don't need to override operator== https://codereview.chromium.org/11539003/diff/68026/chrome/common/form_data.h File chrome/common/form_data.h (right): https://codereview.chromium.org/11539003/diff/68026/chrome/common/form_data.h... chrome/common/form_data.h:32: FormData(); methods before fields.
https://codereview.chromium.org/11539003/diff/68026/chrome/common/form_data.cc File chrome/common/form_data.cc (right): https://codereview.chromium.org/11539003/diff/68026/chrome/common/form_data.c... chrome/common/form_data.cc:28: StringToLowerASCII(method) == StringToLowerASCII(form.method) && On 2013/01/18 16:56:16, sky wrote: > Can method be an enum? That way we don't need StringToLowerASCII and you don't > need to override operator== I don't know, will try to gather more data after talking to Ilya and will address in a different CL. https://codereview.chromium.org/11539003/diff/68026/chrome/common/form_data.h File chrome/common/form_data.h (right): https://codereview.chromium.org/11539003/diff/68026/chrome/common/form_data.h... chrome/common/form_data.h:32: FormData(); On 2013/01/18 16:56:16, sky wrote: > methods before fields. Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ramankk@chromium.org/11539003/63008
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ramankk@chromium.org/11539003/74002
Message was sent while issue was closed.
Change committed as 177746 |
