Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6213002:
Propagate correct data to the Toolbar servers (Closed)
|
Created:
9 years, 11 months ago by GeorgeY Modified:
9 years, 6 months ago CC:
chromium-reviews, Paweł Hajdan Jr., James Hawkins Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionPropagate correct data to the Toolbar servers
BUG=67219
TEST=unit-tested
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72292
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 14
Patch Set 4 : '' #
Total comments: 12
Patch Set 5 : '' #
Total comments: 8
Patch Set 6 : '' #Patch Set 7 : '' #Patch Set 8 : '' #
Total comments: 3
Patch Set 9 : '' #Patch Set 10 : '' #Patch Set 11 : '' #Patch Set 12 : '' #
Total comments: 7
Patch Set 13 : '' #Patch Set 14 : '' #Patch Set 15 : '' #Patch Set 16 : '' #
Total comments: 21
Patch Set 17 : '' #
Total comments: 7
Patch Set 18 : '' #Patch Set 19 : '' #Patch Set 20 : '' #
Total comments: 13
Patch Set 21 : '' #Patch Set 22 : '' #Patch Set 23 : '' #
Total comments: 2
Messages
Total messages: 41 (0 generated)
http://codereview.chromium.org/6213002/diff/25001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager.cc (right): http://codereview.chromium.org/6213002/diff/25001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.cc:524: download_manager_.StartUploadRequest(*(upload_form_structure_.get()), nit: For the sake of matching indentation, please move this argument down to a separate line as well. http://codereview.chromium.org/6213002/diff/25001/chrome/browser/autofill/per... File chrome/browser/autofill/personal_data_manager.cc (right): http://codereview.chromium.org/6213002/diff/25001/chrome/browser/autofill/per... chrome/browser/autofill/personal_data_manager.cc:647: size_t data_size = (MAX_VALID_FIELD_TYPE + 7) >> 3; nit: Please either use named constants for "7" and "3" or include a comment explaining these. http://codereview.chromium.org/6213002/diff/25001/chrome/browser/autofill/per... chrome/browser/autofill/personal_data_manager.cc:654: } nit: Please use PersonalDataManager::GetProfileByGUID instead (and likewise for credit cards below). http://codereview.chromium.org/6213002/diff/25001/chrome/browser/autofill/per... chrome/browser/autofill/personal_data_manager.cc:835: continue; // We do not support CREDIT_CARD_VERIFICATION_CODE nit: I think it would be better to remove the NOTREACHED() in CreditCard.cc rather than add this special case here. http://codereview.chromium.org/6213002/diff/25001/chrome/browser/autofill/per... chrome/browser/autofill/personal_data_manager.cc:838: binary_data_mask[i >> 3] |= (0x80 >> (i & 7)); nit: A comment explaining this line would make for a friendly gesture =) http://codereview.chromium.org/6213002/diff/25001/chrome/browser/autofill/per... File chrome/browser/autofill/personal_data_manager.h (right): http://codereview.chromium.org/6213002/diff/25001/chrome/browser/autofill/per... chrome/browser/autofill/personal_data_manager.h:170: protected: nit: Do we really want all of the below code to be protected rather than private? http://codereview.chromium.org/6213002/diff/25001/chrome/browser/autofill/per... chrome/browser/autofill/personal_data_manager.h:218: void SetDataPresenseBits(FormGroup const* profile, uint8* binary_data_mask); nit: "Presence", not "Presense"
Preliminary nits. http://codereview.chromium.org/6213002/diff/39001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_download.h (right): http://codereview.chromium.org/6213002/diff/39001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_download.h:76: char const* present_data); We should use const std::string& for |present_data|. http://codereview.chromium.org/6213002/diff/39001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager.h (right): http://codereview.chromium.org/6213002/diff/39001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.h:188: struct AutofilledFormInfo { I think |FilledFormInfo| would be preferable. We'll be crowd-sourcing both "auto" and "manually" filled fields. http://codereview.chromium.org/6213002/diff/39001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.h:191: // Guid of the address profile that was used for autofill. s/Guid/GUID s/autofill/AutoFill/ http://codereview.chromium.org/6213002/diff/39001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.h:193: // Guid of the credit card profile that was used for autofill. s/Guid/GUID s/autofill/AutoFill/ http://codereview.chromium.org/6213002/diff/39001/chrome/browser/autofill/for... File chrome/browser/autofill/form_structure.h (right): http://codereview.chromium.org/6213002/diff/39001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure.h:44: bool EncodeUploadRequest(bool auto_fill_used, char const* present_data, We should use const std::string& for |present_data|. http://codereview.chromium.org/6213002/diff/39001/chrome/browser/autofill/per... File chrome/browser/autofill/personal_data_manager.cc (right): http://codereview.chromium.org/6213002/diff/39001/chrome/browser/autofill/per... chrome/browser/autofill/personal_data_manager.cc:664: for (; data_end > 0 && !binary_data_mask[data_end - 1]; --data_end); Use of ";" as loop body causes compiler warning on gcc and is against style guide. Please replace with "{}".
http://codereview.chromium.org/6213002/diff/25001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager.cc (right): http://codereview.chromium.org/6213002/diff/25001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.cc:524: download_manager_.StartUploadRequest(*(upload_form_structure_.get()), On 2011/01/11 23:17:37, Ilya Sherman wrote: > nit: For the sake of matching indentation, please move this argument down to a > separate line as well. Indentation restored http://codereview.chromium.org/6213002/diff/25001/chrome/browser/autofill/per... File chrome/browser/autofill/personal_data_manager.cc (right): http://codereview.chromium.org/6213002/diff/25001/chrome/browser/autofill/per... chrome/browser/autofill/personal_data_manager.cc:647: size_t data_size = (MAX_VALID_FIELD_TYPE + 7) >> 3; On 2011/01/11 23:17:37, Ilya Sherman wrote: > nit: Please either use named constants for "7" and "3" or include a comment > explaining these. Added comment http://codereview.chromium.org/6213002/diff/25001/chrome/browser/autofill/per... chrome/browser/autofill/personal_data_manager.cc:654: } On 2011/01/11 23:17:37, Ilya Sherman wrote: > nit: Please use PersonalDataManager::GetProfileByGUID instead (and likewise for > credit cards below). Code removed http://codereview.chromium.org/6213002/diff/25001/chrome/browser/autofill/per... chrome/browser/autofill/personal_data_manager.cc:835: continue; // We do not support CREDIT_CARD_VERIFICATION_CODE On 2011/01/11 23:17:37, Ilya Sherman wrote: > nit: I think it would be better to remove the NOTREACHED() in CreditCard.cc > rather than add this special case here. Code removed http://codereview.chromium.org/6213002/diff/25001/chrome/browser/autofill/per... chrome/browser/autofill/personal_data_manager.cc:838: binary_data_mask[i >> 3] |= (0x80 >> (i & 7)); On 2011/01/11 23:17:37, Ilya Sherman wrote: > nit: A comment explaining this line would make for a friendly gesture =) Added http://codereview.chromium.org/6213002/diff/25001/chrome/browser/autofill/per... File chrome/browser/autofill/personal_data_manager.h (right): http://codereview.chromium.org/6213002/diff/25001/chrome/browser/autofill/per... chrome/browser/autofill/personal_data_manager.h:170: protected: On 2011/01/11 23:17:37, Ilya Sherman wrote: > nit: Do we really want all of the below code to be protected rather than > private? Probably not, but this is for another cl - this one does not change the file anymore. http://codereview.chromium.org/6213002/diff/25001/chrome/browser/autofill/per... chrome/browser/autofill/personal_data_manager.h:218: void SetDataPresenseBits(FormGroup const* profile, uint8* binary_data_mask); On 2011/01/11 23:17:37, Ilya Sherman wrote: > nit: "Presence", not "Presense" Fixed http://codereview.chromium.org/6213002/diff/39001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_download.h (right): http://codereview.chromium.org/6213002/diff/39001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_download.h:76: char const* present_data); On 2011/01/11 23:29:29, dhollowa wrote: > We should use const std::string& for |present_data|. Does not really matter much: the data passed directly to XML build functions that require const char* anyway, and changing id to std::string will increase size of the unit-test file :) http://codereview.chromium.org/6213002/diff/39001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager.h (right): http://codereview.chromium.org/6213002/diff/39001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.h:188: struct AutofilledFormInfo { On 2011/01/11 23:29:29, dhollowa wrote: > I think |FilledFormInfo| would be preferable. We'll be crowd-sourcing both > "auto" and "manually" filled fields. removed in new version. http://codereview.chromium.org/6213002/diff/39001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.h:191: // Guid of the address profile that was used for autofill. On 2011/01/11 23:29:29, dhollowa wrote: > s/Guid/GUID > s/autofill/AutoFill/ removed in new version. http://codereview.chromium.org/6213002/diff/39001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.h:193: // Guid of the credit card profile that was used for autofill. On 2011/01/11 23:29:29, dhollowa wrote: > s/Guid/GUID > s/autofill/AutoFill/ removed in new version. http://codereview.chromium.org/6213002/diff/39001/chrome/browser/autofill/for... File chrome/browser/autofill/form_structure.h (right): http://codereview.chromium.org/6213002/diff/39001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure.h:44: bool EncodeUploadRequest(bool auto_fill_used, char const* present_data, On 2011/01/11 23:29:29, dhollowa wrote: > We should use const std::string& for |present_data|. See my comment above - does not matter much. AFAIK, both forms are acceptable. http://codereview.chromium.org/6213002/diff/39001/chrome/browser/autofill/per... File chrome/browser/autofill/personal_data_manager.cc (right): http://codereview.chromium.org/6213002/diff/39001/chrome/browser/autofill/per... chrome/browser/autofill/personal_data_manager.cc:664: for (; data_end > 0 && !binary_data_mask[data_end - 1]; --data_end); On 2011/01/11 23:29:29, dhollowa wrote: > Use of ";" as loop body causes compiler warning on gcc and is against style > guide. Please replace with "{}". Code moved and fixed in the process :)
http://codereview.chromium.org/6213002/diff/53001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_download.cc (right): http://codereview.chromium.org/6213002/diff/53001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_download.cc:90: char const* present_data) { nit: You use |present_data| here and |data_present| elsewhere -- please pick one and stick with it ;) http://codereview.chromium.org/6213002/diff/53001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager.h (right): http://codereview.chromium.org/6213002/diff/53001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.h:71: // bitfield of the fields that we autofilled. Looks to me like |data_present| indicates fields that match profile data, whether they were autofilled or not. It might be simpler, though, to just set |data_present| to be a bitfield of all 1's -- does that break anything? http://codereview.chromium.org/6213002/diff/53001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.h:84: std::string ConvertPresenceBitsToString(std::vector<uint8> const& bitfield); nit: I think these should live in autofill_download rather than here (in an effort to avoid autofill_manager becoming our dumping ground).
http://codereview.chromium.org/6213002/diff/53001/chrome/browser/autofill/for... File chrome/browser/autofill/form_structure.cc (right): http://codereview.chromium.org/6213002/diff/53001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure.cc:117: buzz::XmlElement autofil_request_xml(buzz::QName("autofillupload")); s/autofil_request_xml/autofill_request_xml/ http://codereview.chromium.org/6213002/diff/87001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager.h (right): http://codereview.chromium.org/6213002/diff/87001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.h:72: void HandleSubmit(std::string const& data_present); I'd like to see a bit of refactoring here. This is a snake-pit. Here goes: 1. Rename |HandleSubmit| to be |SaveFormData| 2. Remove the calls to |UploadFormData| from methods |HandleSubmit| and |OnInfoBarClosed| 3. Add calls to |UploadFormData| and |SaveFormData| to |OnFormSubmitted| 4. Remove the |data_autofilled_| member as we no longer need it. Currently, the AutoFillManager *pushes* data into the download manager. I'd like to see more of the computation done in the download manager and a move to more of a *pull* model. I know this is somewhat vague... so I'm working on concrete expressions in code. But the spirit of it is this... AutoFillManager should "notify" the AutoFillDownloadManager that it is time to upload. The AutoFillDownloadManager should, in turn, query any necessary information from the AFM and the PDM and process it into the XML it needs. The AFM, PDM, and FormStructure classes should not have to know about "datapresent" concepts, or how to encode upload/download requests. This should all happen in the AutofillDownloadManager based on information provided by the AFM, PDM, and FormStructure classes.
http://codereview.chromium.org/6213002/diff/53001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_download.cc (right): http://codereview.chromium.org/6213002/diff/53001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_download.cc:90: char const* present_data) { On 2011/01/13 23:44:37, Ilya Sherman wrote: > nit: You use |present_data| here and |data_present| elsewhere -- please pick one > and stick with it ;) Done. data_presence all the way :) http://codereview.chromium.org/6213002/diff/53001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager.h (right): http://codereview.chromium.org/6213002/diff/53001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.h:71: // bitfield of the fields that we autofilled. On 2011/01/13 23:44:37, Ilya Sherman wrote: > Looks to me like |data_present| indicates fields that match profile data, > whether they were autofilled or not. It might be simpler, though, to just set > |data_present| to be a bitfield of all 1's -- does that break anything? I think it does. It would mean that UNKNOWN fields are actually matched! http://codereview.chromium.org/6213002/diff/53001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.h:84: std::string ConvertPresenceBitsToString(std::vector<uint8> const& bitfield); On 2011/01/13 23:44:37, Ilya Sherman wrote: > nit: I think these should live in autofill_download rather than here (in an > effort to avoid autofill_manager becoming our dumping ground). np. done. http://codereview.chromium.org/6213002/diff/53001/chrome/browser/autofill/for... File chrome/browser/autofill/form_structure.cc (right): http://codereview.chromium.org/6213002/diff/53001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure.cc:117: buzz::XmlElement autofil_request_xml(buzz::QName("autofillupload")); On 2011/01/14 18:40:58, dhollowa wrote: > s/autofil_request_xml/autofill_request_xml/ Done. http://codereview.chromium.org/6213002/diff/87001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager.h (right): http://codereview.chromium.org/6213002/diff/87001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.h:72: void HandleSubmit(std::string const& data_present); On 2011/01/14 18:40:58, dhollowa wrote: > I'd like to see a bit of refactoring here. This is a snake-pit. Here goes: > > 1. Rename |HandleSubmit| to be |SaveFormData| > 2. Remove the calls to |UploadFormData| from methods |HandleSubmit| and > |OnInfoBarClosed| > 3. Add calls to |UploadFormData| and |SaveFormData| to |OnFormSubmitted| > 4. Remove the |data_autofilled_| member as we no longer need it. > > Currently, the AutoFillManager *pushes* data into the download manager. I'd > like to see more of the computation done in the download manager and a move to > more of a *pull* model. I know this is somewhat vague... so I'm working on > concrete expressions in code. > > But the spirit of it is this... AutoFillManager should "notify" the > AutoFillDownloadManager that it is time to upload. The AutoFillDownloadManager > should, in turn, query any necessary information from the AFM and the PDM and > process it into the XML it needs. > > The AFM, PDM, and FormStructure classes should not have to know about > "datapresent" concepts, or how to encode upload/download requests. This should > all happen in the AutofillDownloadManager based on information provided by the > AFM, PDM, and FormStructure classes. Done.
http://codereview.chromium.org/6213002/diff/87001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager.h (right): http://codereview.chromium.org/6213002/diff/87001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.h:72: void HandleSubmit(std::string const& data_present); Almost. #4 didn't get done. Here's a patch of what I had in mind. http://codereview.chromium.org/6286003 On 2011/01/18 21:04:56, GeorgeY wrote: > On 2011/01/14 18:40:58, dhollowa wrote: > > I'd like to see a bit of refactoring here. This is a snake-pit. Here goes: > > > > 1. Rename |HandleSubmit| to be |SaveFormData| > > 2. Remove the calls to |UploadFormData| from methods |HandleSubmit| and > > |OnInfoBarClosed| > > 3. Add calls to |UploadFormData| and |SaveFormData| to |OnFormSubmitted| > > 4. Remove the |data_autofilled_| member as we no longer need it. > > > > Currently, the AutoFillManager *pushes* data into the download manager. I'd > > like to see more of the computation done in the download manager and a move to > > more of a *pull* model. I know this is somewhat vague... so I'm working on > > concrete expressions in code. > > > > But the spirit of it is this... AutoFillManager should "notify" the > > AutoFillDownloadManager that it is time to upload. The > AutoFillDownloadManager > > should, in turn, query any necessary information from the AFM and the PDM and > > process it into the XML it needs. > > > > The AFM, PDM, and FormStructure classes should not have to know about > > "datapresent" concepts, or how to encode upload/download requests. This > should > > all happen in the AutofillDownloadManager based on information provided by the > > AFM, PDM, and FormStructure classes. > > Done.
On 2011/01/18 21:48:09, dhollowa wrote: > http://codereview.chromium.org/6213002/diff/87001/chrome/browser/autofill/aut... > File chrome/browser/autofill/autofill_manager.h (right): > > http://codereview.chromium.org/6213002/diff/87001/chrome/browser/autofill/aut... > chrome/browser/autofill/autofill_manager.h:72: void HandleSubmit(std::string > const& data_present); > Almost. #4 didn't get done. Oops. Removed from the code, but not from the header... > > Here's a patch of what I had in mind. http://codereview.chromium.org/6286003 > > On 2011/01/18 21:04:56, GeorgeY wrote: > > On 2011/01/14 18:40:58, dhollowa wrote: > > > I'd like to see a bit of refactoring here. This is a snake-pit. Here goes: > > > > > > 1. Rename |HandleSubmit| to be |SaveFormData| > > > 2. Remove the calls to |UploadFormData| from methods |HandleSubmit| and > > > |OnInfoBarClosed| > > > 3. Add calls to |UploadFormData| and |SaveFormData| to |OnFormSubmitted| > > > 4. Remove the |data_autofilled_| member as we no longer need it. > > > > > > Currently, the AutoFillManager *pushes* data into the download manager. I'd > > > like to see more of the computation done in the download manager and a move > to > > > more of a *pull* model. I know this is somewhat vague... so I'm working on > > > concrete expressions in code. > > > > > > But the spirit of it is this... AutoFillManager should "notify" the > > > AutoFillDownloadManager that it is time to upload. The > > AutoFillDownloadManager > > > should, in turn, query any necessary information from the AFM and the PDM > and > > > process it into the XML it needs. > > > > > > The AFM, PDM, and FormStructure classes should not have to know about > > > "datapresent" concepts, or how to encode upload/download requests. This > > should > > > all happen in the AutofillDownloadManager based on information provided by > the > > > AFM, PDM, and FormStructure classes. > > > > Done.
http://codereview.chromium.org/6213002/diff/115001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_download.h (right): http://codereview.chromium.org/6213002/diff/115001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_download.h:133: std::vector<uint8> presence_bitfield_; I'm wondering why we need to store this. The |form| argument of |StartUploadRequest| is a FormStructure object that contains fields that have the information about the "possible types" computed from the PDM. I.e. |AutoFillField::possible_types()|. I think it would be preferable to add an additional output parameter to |EncodeFormRequest| that returns back the bitfield or the datapresent string directly. It should have all the information it needs. And then we can remove |presence_bitfield_| and the methods above.
http://codereview.chromium.org/6213002/diff/115001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_manager.cc (right): http://codereview.chromium.org/6213002/diff/115001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_manager.cc:503: } else { As per our discussion, let's pull this logging stuff out into separate method. As in CL: http://codereview.chromium.org/6286003/ http://codereview.chromium.org/6213002/diff/115001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_manager.cc:591: if (*it == upload_form_structure_->FormSignature()) { nit: Style guide says no braces if the body and conditional are single lines. http://codereview.chromium.org/6213002/diff/115001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_manager.h (right): http://codereview.chromium.org/6213002/diff/115001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_manager.h:215: std::string data_autofilled_; This is unused.
http://codereview.chromium.org/6213002/diff/115001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_manager.cc (right): http://codereview.chromium.org/6213002/diff/115001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_manager.cc:503: } else { On 2011/01/18 22:31:26, dhollowa wrote: > As per our discussion, let's pull this logging stuff out into separate method. > As in CL: http://codereview.chromium.org/6286003/ Moved back (with a slight change) so you could commit your cl http://codereview.chromium.org/6213002/diff/115001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_manager.cc:591: if (*it == upload_form_structure_->FormSignature()) { On 2011/01/18 22:31:26, dhollowa wrote: > nit: Style guide says no braces if the body and conditional are single lines. Done. http://codereview.chromium.org/6213002/diff/115001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_manager.h (right): http://codereview.chromium.org/6213002/diff/115001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_manager.h:215: std::string data_autofilled_; On 2011/01/18 22:31:26, dhollowa wrote: > This is unused. removed
The present data unit tests seem to have gone missing along the way. Also, once you've integrated with ToT (with my refactoring) I believe AFM changes will go away.
http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_manager.cc (right): http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_manager.cc:488: DCHECK(!field_types.empty()); What happened to this bit of code? if (field_types.size() > 1) { // If there is more than one possibility, do not poison the crowd-sourced // data by returning UNKNOWN_TYPE. field_types.clear(); field_types.insert(UNKNOWN_TYPE); } http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/fo... File chrome/browser/autofill/form_structure.cc (right): http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/fo... chrome/browser/autofill/form_structure.cc:455: presence_bitfield[*field_type >> 3] |= (0x80 >> (*field_type & 7)); nit: Is it actually less efficient to write "/ 8" in place of ">> 3" and "& 7" in place of "% 8"? I thought compilers were generally smart enough to make that optimization on their own. http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/fo... chrome/browser/autofill/form_structure.cc:467: // Print all non-zero bytes into the string. nit: There might be zero bytes printed as well, as long as they are not leading ones. http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/pe... File chrome/browser/autofill/personal_data_manager_unittest.cc (right): http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/pe... chrome/browser/autofill/personal_data_manager_unittest.cc:1396: nit: Are you sure you want to add this line? Also in form_structure.cc.
On 2011/01/20 22:21:25, dhollowa wrote: > The present data unit tests seem to have gone missing along the way. Also, once > you've integrated with ToT (with my refactoring) I believe AFM changes will go > away. Not missing, moved to FormStructureTest.EncodeUploadRequest, look at the change of "datapresent" field. I probably need to add a couple more cases though. AFM changes need to be removed from this cl and added to separate one - see my response to Ilya.
http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_manager.cc (right): http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_manager.cc:488: DCHECK(!field_types.empty()); On 2011/01/21 00:25:58, Ilya Sherman wrote: > What happened to this bit of code? > > if (field_types.size() > 1) { > // If there is more than one possibility, do not poison the crowd-sourced > // data by returning UNKNOWN_TYPE. > field_types.clear(); > field_types.insert(UNKNOWN_TYPE); > } Do not want to bloat this cl too much, going to to that in separate cl, as your metrics tests need to be adjusted with that change http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/fo... File chrome/browser/autofill/form_structure.cc (right): http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/fo... chrome/browser/autofill/form_structure.cc:455: presence_bitfield[*field_type >> 3] |= (0x80 >> (*field_type & 7)); On 2011/01/21 00:25:58, Ilya Sherman wrote: > nit: Is it actually less efficient to write "/ 8" in place of ">> 3" and "& 7" > in place of "% 8"? I thought compilers were generally smart enough to make that > optimization on their own. Yes, but AFAIK it is both acceptable :) http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/fo... chrome/browser/autofill/form_structure.cc:467: // Print all non-zero bytes into the string. On 2011/01/21 00:25:58, Ilya Sherman wrote: > nit: There might be zero bytes printed as well, as long as they are not leading > ones. Rephrased. http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/pe... File chrome/browser/autofill/personal_data_manager_unittest.cc (right): http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/pe... chrome/browser/autofill/personal_data_manager_unittest.cc:1396: On 2011/01/21 00:25:58, Ilya Sherman wrote: > nit: Are you sure you want to add this line? Also in form_structure.cc. Yes, otherwise lint reports it as an error.
http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_manager.cc (right): http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_manager.cc:488: DCHECK(!field_types.empty()); On 2011/01/21 00:39:02, GeorgeY wrote: > On 2011/01/21 00:25:58, Ilya Sherman wrote: > > What happened to this bit of code? > > > > if (field_types.size() > 1) { > > // If there is more than one possibility, do not poison the > crowd-sourced > > // data by returning UNKNOWN_TYPE. > > field_types.clear(); > > field_types.insert(UNKNOWN_TYPE); > > } > > Do not want to bloat this cl too much, going to to that in separate cl, as your > metrics tests need to be adjusted with that change If we want to make the changes separately, we should make that change first, so as to not pollute the server data. With the logging code split out from this method though, there shouldn't need to be updates to the metrics test -- or am I missing something? http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/fo... File chrome/browser/autofill/form_structure.cc (right): http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/fo... chrome/browser/autofill/form_structure.cc:455: presence_bitfield[*field_type >> 3] |= (0x80 >> (*field_type & 7)); On 2011/01/21 00:39:02, GeorgeY wrote: > On 2011/01/21 00:25:58, Ilya Sherman wrote: > > nit: Is it actually less efficient to write "/ 8" in place of ">> 3" and "& 7" > > in place of "% 8"? I thought compilers were generally smart enough to make > that > > optimization on their own. > > Yes, but AFAIK it is both acceptable :) Fair enough -- just a bit weird that the code doesn't obviously match the comment. http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/pe... File chrome/browser/autofill/personal_data_manager_unittest.cc (right): http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/pe... chrome/browser/autofill/personal_data_manager_unittest.cc:1396: On 2011/01/21 00:39:02, GeorgeY wrote: > On 2011/01/21 00:25:58, Ilya Sherman wrote: > > nit: Are you sure you want to add this line? Also in form_structure.cc. > > Yes, otherwise lint reports it as an error. Ah... I guess we should fix our other autofill files as well, then.
http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_manager.cc (right): http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_manager.cc:488: DCHECK(!field_types.empty()); Ilya is correct. I wrote a "Washington Irving" unit test and am getting incorrect results. Please see: http://codereview.chromium.org/6373007 for details. However, I would argue that we continue to set the full list of possible types here, but modify the xml formatting code to interpret multiple values as UNKNOWN. This code should not know the details of how xml is generated and precompute those results. On 2011/01/21 00:46:51, Ilya Sherman wrote: > On 2011/01/21 00:39:02, GeorgeY wrote: > > On 2011/01/21 00:25:58, Ilya Sherman wrote: > > > What happened to this bit of code? > > > > > > if (field_types.size() > 1) { > > > // If there is more than one possibility, do not poison the > > crowd-sourced > > > // data by returning UNKNOWN_TYPE. > > > field_types.clear(); > > > field_types.insert(UNKNOWN_TYPE); > > > } > > > > Do not want to bloat this cl too much, going to to that in separate cl, as > your > > metrics tests need to be adjusted with that change > > If we want to make the changes separately, we should make that change first, so > as to not pollute the server data. > > With the logging code split out from this method though, there shouldn't need to > be updates to the metrics test -- or am I missing something?
The original unit test for data present tested |ConvertPresenceBitsToString()| in isolation. We should get that back, it was good. On 2011/01/21 00:34:49, GeorgeY wrote: > On 2011/01/20 22:21:25, dhollowa wrote: > > The present data unit tests seem to have gone missing along the way. Also, > once > > you've integrated with ToT (with my refactoring) I believe AFM changes will go > > away. > > Not missing, moved to FormStructureTest.EncodeUploadRequest, look at the change > of "datapresent" field. > I probably need to add a couple more cases though. > AFM changes need to be removed from this cl and added to separate one - see my > response to Ilya.
http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/fo... File chrome/browser/autofill/form_structure.cc (right): http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/fo... chrome/browser/autofill/form_structure.cc:111: bool auto_fillable = IsAutoFillable(false); I am unclear why this test is needed. If I have a form where all fields are of type UNKNOWN, but I fill in "George" and "Washington" surely I still want to crowd-source that.
http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/fo... File chrome/browser/autofill/form_structure.cc (right): http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/fo... chrome/browser/autofill/form_structure.cc:111: bool auto_fillable = IsAutoFillable(false); On 2011/01/21 03:18:57, dhollowa wrote: > I am unclear why this test is needed. If I have a form where all fields are of > type UNKNOWN, but I fill in "George" and "Washington" surely I still want to > crowd-source that. Yeah, this should be changed to ShouldBeParsed(true) to match the (recently revised) expectations in AutoFillManager::OnFormSubmitted().
Will re-upload when tests pass http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_manager.cc (right): http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_manager.cc:488: DCHECK(!field_types.empty()); On 2011/01/21 02:14:57, dhollowa wrote: > Ilya is correct. I wrote a "Washington Irving" unit test and am getting > incorrect results. Please see: http://codereview.chromium.org/6373007 for > details. > > However, I would argue that we continue to set the full list of possible types > here, but modify the xml formatting code to interpret multiple values as > UNKNOWN. This code should not know the details of how xml is generated and > precompute those results. > > On 2011/01/21 00:46:51, Ilya Sherman wrote: > > On 2011/01/21 00:39:02, GeorgeY wrote: > > > On 2011/01/21 00:25:58, Ilya Sherman wrote: > > > > What happened to this bit of code? > > > > > > > > if (field_types.size() > 1) { > > > > // If there is more than one possibility, do not poison the > > > crowd-sourced > > > > // data by returning UNKNOWN_TYPE. > > > > field_types.clear(); > > > > field_types.insert(UNKNOWN_TYPE); > > > > } > > > > > > Do not want to bloat this cl too much, going to to that in separate cl, as > > your > > > metrics tests need to be adjusted with that change > > > > If we want to make the changes separately, we should make that change first, > so > > as to not pollute the server data. > > > > With the logging code split out from this method though, there shouldn't need > to > > be updates to the metrics test -- or am I missing something? > Done. http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/fo... File chrome/browser/autofill/form_structure.cc (right): http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/fo... chrome/browser/autofill/form_structure.cc:111: bool auto_fillable = IsAutoFillable(false); On 2011/01/21 03:18:57, dhollowa wrote: > I am unclear why this test is needed. If I have a form where all fields are of > type UNKNOWN, but I fill in "George" and "Washington" surely I still want to > crowd-source that. Non autofillable forms are bigger than 50 items or shorter than 3. We do not crowd source that. http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/fo... chrome/browser/autofill/form_structure.cc:455: presence_bitfield[*field_type >> 3] |= (0x80 >> (*field_type & 7)); Changed. On 2011/01/21 00:46:51, Ilya Sherman wrote: > On 2011/01/21 00:39:02, GeorgeY wrote: > > On 2011/01/21 00:25:58, Ilya Sherman wrote: > > > nit: Is it actually less efficient to write "/ 8" in place of ">> 3" and "& > 7" > > > in place of "% 8"? I thought compilers were generally smart enough to make > > that > > > optimization on their own. > > > > Yes, but AFAIK it is both acceptable :) > > Fair enough -- just a bit weird that the code doesn't obviously match the > comment. http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/pe... File chrome/browser/autofill/personal_data_manager_unittest.cc (right): http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/pe... chrome/browser/autofill/personal_data_manager_unittest.cc:1396: I am fixing it when lint reports it :) On 2011/01/21 00:46:51, Ilya Sherman wrote: > On 2011/01/21 00:39:02, GeorgeY wrote: > > On 2011/01/21 00:25:58, Ilya Sherman wrote: > > > nit: Are you sure you want to add this line? Also in form_structure.cc. > > > > Yes, otherwise lint reports it as an error. > > Ah... I guess we should fix our other autofill files as well, then.
http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/fo... File chrome/browser/autofill/form_structure.cc (right): http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/fo... chrome/browser/autofill/form_structure.cc:111: bool auto_fillable = IsAutoFillable(false); On 2011/01/21 03:41:32, GeorgeY wrote: > On 2011/01/21 03:18:57, dhollowa wrote: > > I am unclear why this test is needed. If I have a form where all fields are > of > > type UNKNOWN, but I fill in "George" and "Washington" surely I still want to > > crowd-source that. > > Non autofillable forms are bigger than 50 items or shorter than 3. We do not > crowd source that. IsAutoFillable() judges the field count based on heuristics. ShouldBeParsed() judges the field count based on, well, the raw field count. Hence, we should use ShouldBeParsed() =) Also, I don't recall seeing any code that filters out forms with > 50 items -- where do we do that?
http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/fo... File chrome/browser/autofill/form_structure.cc (right): http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/fo... chrome/browser/autofill/form_structure.cc:437: std::string FormStructure::ConvertPresenceBitsToString() const { It looks like we're setting a bit for UNKNOWN_TYPE. We probably shouldn't be.
http://codereview.chromium.org/6213002/diff/102012/chrome/browser/autofill/fo... File chrome/browser/autofill/form_structure.cc (right): http://codereview.chromium.org/6213002/diff/102012/chrome/browser/autofill/fo... chrome/browser/autofill/form_structure.cc:444: presence_bitfield.resize((MAX_VALID_FIELD_TYPE + 0x7) >> 3); I'm with Ilya, I'd prefer we use div "/" and mod "%" (throughout) instead of bit shifting. It is more readable. http://codereview.chromium.org/6213002/diff/102012/chrome/browser/autofill/fo... File chrome/browser/autofill/form_structure_unittest.cc (right): http://codereview.chromium.org/6213002/diff/102012/chrome/browser/autofill/fo... chrome/browser/autofill/form_structure_unittest.cc:1885: // #1 == UNKNOWN_TYPE Why would we set UNKNOWN_TYPE in datapresent? It should be only NAME_FIRST and EMAIL_ADDRESS.
http://codereview.chromium.org/6213002/diff/102012/chrome/browser/autofill/pe... File chrome/browser/autofill/personal_data_manager_unittest.cc (right): http://codereview.chromium.org/6213002/diff/102012/chrome/browser/autofill/pe... chrome/browser/autofill/personal_data_manager_unittest.cc:281: // The message loop will exit when the mock observer is notified. Are the changes in this file still needed?
http://codereview.chromium.org/6213002/diff/102012/chrome/browser/autofill/fo... File chrome/browser/autofill/form_structure_unittest.cc (right): http://codereview.chromium.org/6213002/diff/102012/chrome/browser/autofill/fo... chrome/browser/autofill/form_structure_unittest.cc:1878: // Match second field as both first and last. s/second/third/
Yes, there was a bug that we did not process the cleanup correctly, which can be noticed only if you do something on the data load notification (which I did in first version). It is still a good idea to cleanup correctly. On 2011/01/21 16:47:37, dhollowa wrote: > http://codereview.chromium.org/6213002/diff/102012/chrome/browser/autofill/pe... > File chrome/browser/autofill/personal_data_manager_unittest.cc (right): > > http://codereview.chromium.org/6213002/diff/102012/chrome/browser/autofill/pe... > chrome/browser/autofill/personal_data_manager_unittest.cc:281: // The message > loop will exit when the mock observer is notified. > Are the changes in this file still needed?
http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/fo... File chrome/browser/autofill/form_structure.cc (right): http://codereview.chromium.org/6213002/diff/137001/chrome/browser/autofill/fo... chrome/browser/autofill/form_structure.cc:437: std::string FormStructure::ConvertPresenceBitsToString() const { On 2011/01/21 04:05:43, dhollowa wrote: > It looks like we're setting a bit for UNKNOWN_TYPE. We probably shouldn't be. Why not? Isn't it a valid bit? I do not really want to special case it (and NONE, etc.) http://codereview.chromium.org/6213002/diff/102012/chrome/browser/autofill/fo... chrome/browser/autofill/form_structure.cc:444: presence_bitfield[i] = 0; On 2011/01/21 16:44:57, dhollowa wrote: > I'm with Ilya, I'd prefer we use div "/" and mod "%" (throughout) instead of bit > shifting. It is more readable. OK, but I am not really agree on more readable - different development backgrounds, I guess http://codereview.chromium.org/6213002/diff/102012/chrome/browser/autofill/fo... File chrome/browser/autofill/form_structure_unittest.cc (right): http://codereview.chromium.org/6213002/diff/102012/chrome/browser/autofill/fo... chrome/browser/autofill/form_structure_unittest.cc:1878: // Match second field as both first and last. On 2011/01/21 17:17:05, dhollowa wrote: > s/second/third/ Done. http://codereview.chromium.org/6213002/diff/102012/chrome/browser/autofill/fo... chrome/browser/autofill/form_structure_unittest.cc:1885: // #1 == UNKNOWN_TYPE On 2011/01/21 16:44:57, dhollowa wrote: > Why would we set UNKNOWN_TYPE in datapresent? It should be only NAME_FIRST and > EMAIL_ADDRESS. I do not want to special case some of the types, and AFAIK it processed correctly on server side.
New version is in :)
LGTM with below nits addressed. http://codereview.chromium.org/6213002/diff/95004/chrome/browser/autofill/for... File chrome/browser/autofill/form_structure.cc (right): http://codereview.chromium.org/6213002/diff/95004/chrome/browser/autofill/for... chrome/browser/autofill/form_structure.cc:460: } nit: You already have this above; no need to do it again. http://codereview.chromium.org/6213002/diff/95004/chrome/browser/autofill/for... chrome/browser/autofill/form_structure.cc:506: DCHECK((arraysize(allowed_multi_fields) & 1) == 0); nit: Please write this as "% 2" rather than "& 1" http://codereview.chromium.org/6213002/diff/95004/chrome/browser/autofill/for... chrome/browser/autofill/form_structure.cc:513: } nit: This was the extra code complexity that I was referring to ;) http://codereview.chromium.org/6213002/diff/95004/chrome/browser/autofill/for... File chrome/browser/autofill/form_structure.h (right): http://codereview.chromium.org/6213002/diff/95004/chrome/browser/autofill/for... chrome/browser/autofill/form_structure.h:131: // form structure and converts it to to string for uploading. nit: "to to" -> "to" http://codereview.chromium.org/6213002/diff/95004/chrome/browser/autofill/for... File chrome/browser/autofill/form_structure_unittest.cc (right): http://codereview.chromium.org/6213002/diff/95004/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_unittest.cc:1751: TEST(FormStructureTest, CheckDataPresence) { nit: Please include a quick comment describing this test http://codereview.chromium.org/6213002/diff/95004/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_unittest.cc:1751: TEST(FormStructureTest, CheckDataPresence) { Might be good to test EmptyType as well. http://codereview.chromium.org/6213002/diff/95004/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_unittest.cc:1828: TEST(FormStructureTest, CheckMultipleTypes) { nit: Ditto.
http://codereview.chromium.org/6213002/diff/95004/chrome/browser/autofill/for... File chrome/browser/autofill/form_structure.cc (right): http://codereview.chromium.org/6213002/diff/95004/chrome/browser/autofill/for... chrome/browser/autofill/form_structure.cc:460: } On 2011/01/22 01:12:35, Ilya Sherman wrote: > nit: You already have this above; no need to do it again. Done. http://codereview.chromium.org/6213002/diff/95004/chrome/browser/autofill/for... chrome/browser/autofill/form_structure.cc:506: DCHECK((arraysize(allowed_multi_fields) & 1) == 0); On 2011/01/22 01:12:35, Ilya Sherman wrote: > nit: Please write this as "% 2" rather than "& 1" Done. http://codereview.chromium.org/6213002/diff/95004/chrome/browser/autofill/for... chrome/browser/autofill/form_structure.cc:513: } On 2011/01/22 01:12:35, Ilya Sherman wrote: > nit: This was the extra code complexity that I was referring to ;) removed http://codereview.chromium.org/6213002/diff/95004/chrome/browser/autofill/for... File chrome/browser/autofill/form_structure.h (right): http://codereview.chromium.org/6213002/diff/95004/chrome/browser/autofill/for... chrome/browser/autofill/form_structure.h:131: // form structure and converts it to to string for uploading. On 2011/01/22 01:12:35, Ilya Sherman wrote: > nit: "to to" -> "to" Done. http://codereview.chromium.org/6213002/diff/95004/chrome/browser/autofill/for... File chrome/browser/autofill/form_structure_unittest.cc (right): http://codereview.chromium.org/6213002/diff/95004/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_unittest.cc:1751: TEST(FormStructureTest, CheckDataPresence) { On 2011/01/22 01:12:35, Ilya Sherman wrote: > nit: Please include a quick comment describing this test Done. http://codereview.chromium.org/6213002/diff/95004/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_unittest.cc:1828: TEST(FormStructureTest, CheckMultipleTypes) { On 2011/01/22 01:12:35, Ilya Sherman wrote: > nit: Ditto. Changed comment in next 4 lines
LGTM! Let the data flow.
Oh wait... one problem. If the set of |possible_types()| is empty for a given field, we should still be outputting UNKNOWN_TYPE <field> elements, and "datapresent" bits accordingly. On 2011/01/22 01:42:46, dhollowa wrote: > LGTM! Let the data flow.
On 2011/01/22 21:22:42, dhollowa wrote: > Oh wait... one problem. If the set of |possible_types()| is empty for a given > field, we should still be outputting UNKNOWN_TYPE <field> elements, and > "datapresent" bits accordingly. We are already doing this -- see bottom of PersonalDataManager::GetPossibleFieldTypes: http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/chrome/browse...
Ah, great. Ok then. On 2011/01/23 00:03:34, Ilya Sherman wrote: > On 2011/01/22 21:22:42, dhollowa wrote: > > Oh wait... one problem. If the set of |possible_types()| is empty for a given > > field, we should still be outputting UNKNOWN_TYPE <field> elements, and > > "datapresent" bits accordingly. > > We are already doing this -- see bottom of > PersonalDataManager::GetPossibleFieldTypes: > http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/chrome/browse...
On 2011/01/22 21:22:42, dhollowa wrote: > Oh wait... one problem. If the set of |possible_types()| is empty for a given > field, we should still be outputting UNKNOWN_TYPE <field> elements, and > "datapresent" bits accordingly. > > On 2011/01/22 01:42:46, dhollowa wrote: > > LGTM! Let the data flow. done. Not sure it ever happens though...
http://codereview.chromium.org/6213002/diff/126003/chrome/browser/autofill/fo... File chrome/browser/autofill/form_structure.cc (right): http://codereview.chromium.org/6213002/diff/126003/chrome/browser/autofill/fo... chrome/browser/autofill/form_structure.cc:416: types.insert(UNKNOWN_TYPE); This is already covered in PersonalDataManager::GetPossibleFieldTypes -- DCHECK(!types.empty()) would be better. http://codereview.chromium.org/6213002/diff/126003/chrome/browser/autofill/fo... chrome/browser/autofill/form_structure.cc:452: types.insert(UNKNOWN_TYPE); Ditto
On 2011/01/23 01:08:02, Ilya Sherman wrote: > http://codereview.chromium.org/6213002/diff/126003/chrome/browser/autofill/fo... > File chrome/browser/autofill/form_structure.cc (right): > > http://codereview.chromium.org/6213002/diff/126003/chrome/browser/autofill/fo... > chrome/browser/autofill/form_structure.cc:416: types.insert(UNKNOWN_TYPE); > This is already covered in PersonalDataManager::GetPossibleFieldTypes -- > DCHECK(!types.empty()) would be better. > > http://codereview.chromium.org/6213002/diff/126003/chrome/browser/autofill/fo... > chrome/browser/autofill/form_structure.cc:452: types.insert(UNKNOWN_TYPE); > Ditto Wasn't sure that's ever happens now i *know*. Changed to DCHECK()
On 2011/01/23 06:32:46, GeorgeY wrote: > On 2011/01/23 01:08:02, Ilya Sherman wrote: > > > http://codereview.chromium.org/6213002/diff/126003/chrome/browser/autofill/fo... > > File chrome/browser/autofill/form_structure.cc (right): > > > > > http://codereview.chromium.org/6213002/diff/126003/chrome/browser/autofill/fo... > > chrome/browser/autofill/form_structure.cc:416: types.insert(UNKNOWN_TYPE); > > This is already covered in PersonalDataManager::GetPossibleFieldTypes -- > > DCHECK(!types.empty()) would be better. > > > > > http://codereview.chromium.org/6213002/diff/126003/chrome/browser/autofill/fo... > > chrome/browser/autofill/form_structure.cc:452: types.insert(UNKNOWN_TYPE); > > Ditto > > Wasn't sure that's ever happens now i *know*. Changed to DCHECK() Looks like this check currently fails for AutoFillDownloadTest.QueryAndUploadTest... reverting your commit of this patch for that reason.
On 2011/01/23 08:37:35, Ilya Sherman wrote: > On 2011/01/23 06:32:46, GeorgeY wrote: > > On 2011/01/23 01:08:02, Ilya Sherman wrote: > > > > > > http://codereview.chromium.org/6213002/diff/126003/chrome/browser/autofill/fo... > > > File chrome/browser/autofill/form_structure.cc (right): > > > > > > > > > http://codereview.chromium.org/6213002/diff/126003/chrome/browser/autofill/fo... > > > chrome/browser/autofill/form_structure.cc:416: types.insert(UNKNOWN_TYPE); > > > This is already covered in PersonalDataManager::GetPossibleFieldTypes -- > > > DCHECK(!types.empty()) would be better. > > > > > > > > > http://codereview.chromium.org/6213002/diff/126003/chrome/browser/autofill/fo... > > > chrome/browser/autofill/form_structure.cc:452: types.insert(UNKNOWN_TYPE); > > > Ditto > > > > Wasn't sure that's ever happens now i *know*. Changed to DCHECK() > > Looks like this check currently fails for > AutoFillDownloadTest.QueryAndUploadTest... reverting your commit of this patch > for that reason. Removed the DCHECKs as they were breaking unit-tests |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
