Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2022123002:
Merge autofill::FormFieldData::is_checkable and is_checked (Closed)
|
Created:
4 years, 6 months ago by Hwanseung Lee(hs1217.lee) Modified:
4 years, 6 months ago CC:
browser-components-watch_chromium.org, chromium-reviews, estade+watch_chromium.org, jdonnelly+autofillwatch_chromium.org, rouslan+autofill_chromium.org, vabr+watchlistautofill_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMerge autofill::FormFieldData::is_checkable and is_checked
Currently, autofill::FormFieldData contains two Boolean data members:
* is_checked, and
* is_checkable.
As expected, the former can only be true when the latter is,
but the code makes this not clear.
We should merge these two Booleans into a single 3-valued enum representing the values:
not checkable, checkable but unchecked, checked.
BUG=614280
Committed: https://crrev.com/61b1d174d7dfd788dfe34cad3f8a0b9856894290
Cr-Commit-Position: refs/heads/master@{#399021}
Patch Set 1 #Patch Set 2 : fixed init value #
Total comments: 10
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Total comments: 17
Patch Set 8 : #Patch Set 9 : #
Total comments: 1
Patch Set 10 : #Patch Set 11 : #Messages
Total messages: 42 (18 generated)
Description was changed from ========== merge these two Booleans into a single 3-valued enum representing the values: not checkable, checkable but unchecked, checked. BUG=614280 ========== to ========== merge these two Booleans into a single 3-valued enum representing the values: not checkable, checkable but unchecked, checked. BUG=614280 ==========
hs1217.lee@gmail.com changed reviewers: + estade@chromium.org, gcasto@chromium.org, vabr@chromium.org
please review this commit.
Thanks for your contribution, but I'm afraid it does not LGTM so far. First, have you signed the CLA? I followed the steps from https://www.chromium.org/developers/contributing-code/external-contributor-ch... and could not verify that there is a signed record for hs1217.lee@gmail.com. Second, please fix the reading of old serializations (see my comments below). Third, please add unittests for your changes (in form_field_data_unittest.cc). Fourth, please fix the CL title and description: * Do not use "these" without refering to anything. Just reuse the bug title "Merge autofill::FormFieldData::is_checkable and is_checked" for the CL title if you are unsure. * Then update the first line of the CL description to match the CL title, leave the second line blank and add more explanation from the third line one, if needed. * When doing that, please keep the lines to 78 chars max. All of that helps to make good GIT commit messages once the CL lands. Thanks so far, and let me know when you want me to re-review. Cheers, Vaclav https://codereview.chromium.org/2022123002/diff/20001/AUTHORS File AUTHORS (left): https://codereview.chromium.org/2022123002/diff/20001/AUTHORS#oldcode244 AUTHORS:244: Hwanseung Lee <rucifer1217@gmail.com> I cannot let you overwrite a different e-mail address. Could you add your line above line 244 instead of replacing it? https://codereview.chromium.org/2022123002/diff/20001/components/autofill/cor... File components/autofill/core/common/form_field_data.cc (right): https://codereview.chromium.org/2022123002/diff/20001/components/autofill/cor... components/autofill/core/common/form_field_data.cc:62: iter->Readint(&field_data->check_status) && No, this would break reading old serializations of your data. The FormFieldData structs are turned into strings and those are sent to servers, and later received and deserialized. If Chrome updates the FormFieldData format, it still needs to be able to read the old serializations. Please look at kPickleVersion which stores the format version and add a new version with proper migration. If you are unsure, look at the changes which changed kPickleVersion in the past for inspiration. https://codereview.chromium.org/2022123002/diff/20001/components/autofill/cor... components/autofill/core/common/form_field_data.cc:104: bool is_checkable = Why don't you just compare check_status with field.check_status below? https://codereview.chromium.org/2022123002/diff/20001/components/autofill/cor... components/autofill/core/common/form_field_data.cc:148: bool is_checkable = Again, just use check_status directly. But you will need to static_cast it to int once you switch to enum class from just enum. https://codereview.chromium.org/2022123002/diff/20001/components/autofill/cor... components/autofill/core/common/form_field_data.cc:239: << field.check_status Once you change the type to enum class, this will no longer work. I suggest specifying an operator<< for CheckStatus, which would print the actual value names, to fix that. https://codereview.chromium.org/2022123002/diff/20001/components/autofill/cor... File components/autofill/core/common/form_field_data.h (right): https://codereview.chromium.org/2022123002/diff/20001/components/autofill/cor... components/autofill/core/common/form_field_data.h:32: enum CheckStatus { Please use enum class instead of enum, to get better protection against unwanted casts and also to isolate the values in their own namespace. Once you do that, please drop the CHECK_STATUS_ prefix from the values, it will be replaced by the namespace.
On 2016/05/31 15:48:25, vabr (Chromium) wrote: > Thanks for your contribution, but I'm afraid it does not LGTM so far. > > First, have you signed the CLA? I followed the steps from > https://www.chromium.org/developers/contributing-code/external-contributor-ch... > and could not verify that there is a signed record for mailto:hs1217.lee@gmail.com. > > Second, please fix the reading of old serializations (see my comments below). > > Third, please add unittests for your changes (in form_field_data_unittest.cc). > > Fourth, please fix the CL title and description: > * Do not use "these" without refering to anything. Just reuse the bug title > "Merge autofill::FormFieldData::is_checkable and is_checked" for the CL title if > you are unsure. > * Then update the first line of the CL description to match the CL title, > leave the second line blank and add more explanation from the third line one, if > needed. > * When doing that, please keep the lines to 78 chars max. > All of that helps to make good GIT commit messages once the CL lands. > > Thanks so far, and let me know when you want me to re-review. > > Cheers, > Vaclav > > https://codereview.chromium.org/2022123002/diff/20001/AUTHORS > File AUTHORS (left): > > https://codereview.chromium.org/2022123002/diff/20001/AUTHORS#oldcode244 > AUTHORS:244: Hwanseung Lee <mailto:rucifer1217@gmail.com> > I cannot let you overwrite a different e-mail address. Could you add your line > above line 244 instead of replacing it? > > https://codereview.chromium.org/2022123002/diff/20001/components/autofill/cor... > File components/autofill/core/common/form_field_data.cc (right): > > https://codereview.chromium.org/2022123002/diff/20001/components/autofill/cor... > components/autofill/core/common/form_field_data.cc:62: > iter->Readint(&field_data->check_status) && > No, this would break reading old serializations of your data. > > The FormFieldData structs are turned into strings and those are sent to servers, > and later received and deserialized. If Chrome updates the FormFieldData format, > it still needs to be able to read the old serializations. > > Please look at kPickleVersion which stores the format version and add a new > version with proper migration. If you are unsure, look at the changes which > changed kPickleVersion in the past for inspiration. > > https://codereview.chromium.org/2022123002/diff/20001/components/autofill/cor... > components/autofill/core/common/form_field_data.cc:104: bool is_checkable = > Why don't you just compare check_status with field.check_status below? > > https://codereview.chromium.org/2022123002/diff/20001/components/autofill/cor... > components/autofill/core/common/form_field_data.cc:148: bool is_checkable = > Again, just use check_status directly. But you will need to static_cast it to > int once you switch to enum class from just enum. > > https://codereview.chromium.org/2022123002/diff/20001/components/autofill/cor... > components/autofill/core/common/form_field_data.cc:239: << field.check_status > Once you change the type to enum class, this will no longer work. I suggest > specifying an operator<< for CheckStatus, which would print the actual value > names, to fix that. > > https://codereview.chromium.org/2022123002/diff/20001/components/autofill/cor... > File components/autofill/core/common/form_field_data.h (right): > > https://codereview.chromium.org/2022123002/diff/20001/components/autofill/cor... > components/autofill/core/common/form_field_data.h:32: enum CheckStatus { > Please use enum class instead of enum, to get better protection against unwanted > casts and also to isolate the values in their own namespace. > > Once you do that, please drop the CHECK_STATUS_ prefix from the values, it will > be replaced by the namespace. i upload new patchset. could review again? i signed CLA by your guide. and i checked about components_unittests. it was passed. i can't change to use enum class. because it was complex to deserialize & serialize. thank you.
Description was changed from ========== merge these two Booleans into a single 3-valued enum representing the values: not checkable, checkable but unchecked, checked. BUG=614280 ========== to ========== Currently, autofill::FormFieldData contains two Boolean data members: * is_checked, and * is_checkable. As expected, the former can only be true when the latter is, but the code makes this not clear. We should merge these two Booleans into a single 3-valued enum representing the values: not checkable, checkable but unchecked, checked. BUG=614280 ==========
Description was changed from ========== Currently, autofill::FormFieldData contains two Boolean data members: * is_checked, and * is_checkable. As expected, the former can only be true when the latter is, but the code makes this not clear. We should merge these two Booleans into a single 3-valued enum representing the values: not checkable, checkable but unchecked, checked. BUG=614280 ========== to ========== Currently, autofill::FormFieldData contains two Boolean data members: * is_checked, and * is_checkable. As expected, the former can only be true when the latter is, but the code makes this not clear. We should merge these two Booleans into a single 3-valued enum representing the values: not checkable, checkable but unchecked, checked. BUG=614280 ==========
Thank you for your improvements to the CL! Some comments: (1) Thanks for signing the SLA. Please address the comments I kept adding to AUTHORS, though. (2) The CL description and title look fine, but please repeat the CL title also as a first line in the CL description, and separate it by a blank line from the rest. That's because upon landing the CL, the git commit only gets created from the CL description, not title. (3) Please see the comments below. Thanks, Vaclav P.S. I acknowledge the use of enum instead of enum class, serialization seems a fair reason. https://codereview.chromium.org/2022123002/diff/120001/AUTHORS File AUTHORS (right): https://codereview.chromium.org/2022123002/diff/120001/AUTHORS#newcode244 AUTHORS:244: Hwanseung Lee <hs1217.lee@gmail.com> You are still replacing the existing line with a new one. Please see my previous comment on this file. https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... components/autofill/content/renderer/form_autofill_util.cc:892: input_element->setChecked(IsCheckable(data.check_status), true); Did you mean IsChecked instead of IsCheckable? https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... File components/autofill/core/browser/autofill_metrics_unittest.cc (right): https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_metrics_unittest.cc:3764: SetCheckStatus(&checkable_field, true, false); Please assign directly: checkable_field.check_status = FormFieldData::CHECKABLE_BUT_UNCHECKED; that is more readable than the function call here (one has to know the SetCheckStatus definition to understand the meaning of the 2nd and 3rd argument). https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... File components/autofill/core/browser/form_field_unittest.cc (right): https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... components/autofill/core/browser/form_field_unittest.cc:134: SetCheckStatus(&field_data, true, false); Please assign directly, see my comment in components/autofill/core/browser/autofill_metrics_unittest.cc. https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... components/autofill/core/browser/form_field_unittest.cc:139: SetCheckStatus(&field_data, false, false); Here as well, please assign directly. https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... File components/autofill/core/common/form_field_data.cc (right): https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... components/autofill/core/common/form_field_data.cc:53: bool DeserializeCommonSection1(base::PickleIterator* iter, This is not a fault of your CL, but I'm afraid the migration code needs some refactoring to make it clear before we add some more. Currently the naming of the Deserialize* functions is a mess and gets worse with more of them and code duplication added in this CL. A similar situation is encountered in DeserializeValueSize in chrome/browser/password_manager/native_backend_kwallet_x.cc. There the issue with the names of functions is avoided by inlining them. That might actually work here as well. As a, perhaps better, alternative, one can just name sections sequentially and have, e.g., DeserializeSection1 work on the block from |label| to |is_autofilled|, DeserializeSection2 to work on |is_checked| and |is_checkable|, etc., and then keep the blocks around line 200 in this file and only let them use the simplified function names. The difference to your current approach would be in two points: * Simpler naming, also for the existing Deserialize* methods, and * Splitting the sections so far, until no code duplication is needed. My suggestion is to implement the last paragraph above, but do it first for the existing code in a separate CL, and then rebase the current CL on that separate CL and add version 4 handling in the same manner. https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... components/autofill/core/common/form_field_data.cc:55: bool ret = true; nit: Please avoid abbreviations [1]. My suggestion is to rename |ret| to |success|. [1] https://google.github.io/styleguide/cppguide.html#General_Naming_Rules https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... components/autofill/core/common/form_field_data.cc:58: ret = ret && iter->ReadString16(&field_data->label); nit: Repeating |ret| clutters the code. Please initialize the variable with the whole Boolean expression in a single step: const bool success = iter->ReadString16(&field_data->label) && iter->ReadString16(&field_data->name) && ... iter->ReadBool(&field_data->should_autocomplete); if (success) SetCheckStatus(...); return success; https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... components/autofill/core/common/form_field_data.cc:109: check_status(CheckStatus::NOT_CHECKABLE), No need for CheckStatus:: because you are not using enum class. https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... components/autofill/core/common/form_field_data.cc:255: std::string checkStatusStr; nit: Please use all-lower-case and underscores for variable names [1]. Also on different other places in the CL. [1] https://google.github.io/styleguide/cppguide.html#Variable_Names https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... components/autofill/core/common/form_field_data.cc:291: return check_status == FormFieldData::CheckStatus::NOT_CHECKABLE No need for the ternary operator: X == Y ? false : true is the same as X != Y https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... components/autofill/core/common/form_field_data.cc:296: return check_status == FormFieldData::CheckStatus::CHECKED ? false : true; This returns the opposite of what it should return. Also, drop the ternary operator, just return check_status == FormFieldData::CheckStatus::CHECKED; https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... components/autofill/core/common/form_field_data.cc:301: if(isChecked == true){ Drop " == true" (also below). https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... components/autofill/core/common/form_field_data.cc:301: if(isChecked == true){ nit: Formatting is off, please make sure to run git cl format. https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... File components/autofill/core/common/form_field_data_unittest.cc (right): https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... components/autofill/core/common/form_field_data_unittest.cc:102: Please update this test to test version 4. That means, ensure that the block above actually creates a version 4 FormFieldData. https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... components/autofill/core/common/form_field_data_unittest.cc:144: TEST(FormFieldDataTest, DeserializeVersion3) { Thanks for adding the version 3 test! https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... components/autofill/core/common/form_field_data_unittest.cc:147: FillVersion2Fields(&data); You should also call FillVersion3Fields(&data); after this line.
Description was changed from ========== Currently, autofill::FormFieldData contains two Boolean data members: * is_checked, and * is_checkable. As expected, the former can only be true when the latter is, but the code makes this not clear. We should merge these two Booleans into a single 3-valued enum representing the values: not checkable, checkable but unchecked, checked. BUG=614280 ========== to ========== Merge autofill::FormFieldData::is_checkable and is_checked Currently, autofill::FormFieldData contains two Boolean data members: * is_checked, and * is_checkable. As expected, the former can only be true when the latter is, but the code makes this not clear. We should merge these two Booleans into a single 3-valued enum representing the values: not checkable, checkable but unchecked, checked. BUG=614280 ==========
On 2016/06/06 12:39:31, vabr (Chromium) wrote: > Thank you for your improvements to the CL! > > Some comments: > > (1) Thanks for signing the SLA. Please address the comments I kept adding to > AUTHORS, though. > > (2) The CL description and title look fine, but please repeat the CL title also > as a first line in the CL description, and separate it by a blank line from the > rest. That's because upon landing the CL, the git commit only gets created from > the CL description, not title. > > (3) Please see the comments below. > > Thanks, > Vaclav > > P.S. I acknowledge the use of enum instead of enum class, serialization seems a > fair reason. > > https://codereview.chromium.org/2022123002/diff/120001/AUTHORS > File AUTHORS (right): > > https://codereview.chromium.org/2022123002/diff/120001/AUTHORS#newcode244 > AUTHORS:244: Hwanseung Lee <mailto:hs1217.lee@gmail.com> > You are still replacing the existing line with a new one. Please see my previous > comment on this file. > > https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... > File components/autofill/content/renderer/form_autofill_util.cc (right): > > https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... > components/autofill/content/renderer/form_autofill_util.cc:892: > input_element->setChecked(IsCheckable(data.check_status), true); > Did you mean IsChecked instead of IsCheckable? > > https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... > File components/autofill/core/browser/autofill_metrics_unittest.cc (right): > > https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... > components/autofill/core/browser/autofill_metrics_unittest.cc:3764: > SetCheckStatus(&checkable_field, true, false); > Please assign directly: > checkable_field.check_status = FormFieldData::CHECKABLE_BUT_UNCHECKED; > > that is more readable than the function call here (one has to know the > SetCheckStatus definition to understand the meaning of the 2nd and 3rd > argument). > > https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... > File components/autofill/core/browser/form_field_unittest.cc (right): > > https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... > components/autofill/core/browser/form_field_unittest.cc:134: > SetCheckStatus(&field_data, true, false); > Please assign directly, see my comment in > components/autofill/core/browser/autofill_metrics_unittest.cc. > > https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... > components/autofill/core/browser/form_field_unittest.cc:139: > SetCheckStatus(&field_data, false, false); > Here as well, please assign directly. > > https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... > File components/autofill/core/common/form_field_data.cc (right): > > https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... > components/autofill/core/common/form_field_data.cc:53: bool > DeserializeCommonSection1(base::PickleIterator* iter, > This is not a fault of your CL, but I'm afraid the migration code needs some > refactoring to make it clear before we add some more. Currently the naming of > the Deserialize* functions is a mess and gets worse with more of them and code > duplication added in this CL. > > A similar situation is encountered in DeserializeValueSize in > chrome/browser/password_manager/native_backend_kwallet_x.cc. There the issue > with the names of functions is avoided by inlining them. That might actually > work here as well. > > As a, perhaps better, alternative, one can just name sections sequentially and > have, e.g., DeserializeSection1 work on the block from |label| to > |is_autofilled|, DeserializeSection2 to work on |is_checked| and |is_checkable|, > etc., and then keep the blocks around line 200 in this file and only let them > use the simplified function names. The difference to your current approach would > be in two points: > * Simpler naming, also for the existing Deserialize* methods, and > * Splitting the sections so far, until no code duplication is needed. > > > My suggestion is to implement the last paragraph above, but do it first for the > existing code in a separate CL, and then rebase the current CL on that separate > CL and add version 4 handling in the same manner. > > https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... > components/autofill/core/common/form_field_data.cc:55: bool ret = true; > nit: Please avoid abbreviations [1]. My suggestion is to rename |ret| to > |success|. > > [1] https://google.github.io/styleguide/cppguide.html#General_Naming_Rules > > https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... > components/autofill/core/common/form_field_data.cc:58: ret = ret && > iter->ReadString16(&field_data->label); > nit: Repeating |ret| clutters the code. Please initialize the variable with the > whole Boolean expression in a single step: > > const bool success = iter->ReadString16(&field_data->label) && > iter->ReadString16(&field_data->name) && > ... > iter->ReadBool(&field_data->should_autocomplete); > if (success) > SetCheckStatus(...); > return success; > > https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... > components/autofill/core/common/form_field_data.cc:109: > check_status(CheckStatus::NOT_CHECKABLE), > No need for CheckStatus:: because you are not using enum class. > > https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... > components/autofill/core/common/form_field_data.cc:255: std::string > checkStatusStr; > nit: Please use all-lower-case and underscores for variable names [1]. > > Also on different other places in the CL. > > [1] https://google.github.io/styleguide/cppguide.html#Variable_Names > > https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... > components/autofill/core/common/form_field_data.cc:291: return check_status == > FormFieldData::CheckStatus::NOT_CHECKABLE > No need for the ternary operator: > X == Y ? false : true > is the same as > X != Y > > https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... > components/autofill/core/common/form_field_data.cc:296: return check_status == > FormFieldData::CheckStatus::CHECKED ? false : true; > This returns the opposite of what it should return. > > Also, drop the ternary operator, just > return check_status == FormFieldData::CheckStatus::CHECKED; > > https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... > components/autofill/core/common/form_field_data.cc:301: if(isChecked == true){ > Drop " == true" (also below). > > https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... > components/autofill/core/common/form_field_data.cc:301: if(isChecked == true){ > nit: Formatting is off, please make sure to run git cl format. > > https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... > File components/autofill/core/common/form_field_data_unittest.cc (right): > > https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... > components/autofill/core/common/form_field_data_unittest.cc:102: > Please update this test to test version 4. That means, ensure that the block > above actually creates a version 4 FormFieldData. > > https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... > components/autofill/core/common/form_field_data_unittest.cc:144: > TEST(FormFieldDataTest, DeserializeVersion3) { > Thanks for adding the version 3 test! > > https://codereview.chromium.org/2022123002/diff/120001/components/autofill/co... > components/autofill/core/common/form_field_data_unittest.cc:147: > FillVersion2Fields(&data); > You should also call > FillVersion3Fields(&data); > after this line. could you review again? it was passed components_unittests.
Thanks, this looks very good. I have one last comment below, as one test is still missing. After that I'm happy to give my approval. Please note that you need the security team to review the change to the IPC header (check the neighbouring OWNERS file for reviewers). Also, I won't be at my computer for most of tomorrow, so my next response may come delayed. Sorry about that. Cheers, Vaclav https://codereview.chromium.org/2022123002/diff/160001/components/autofill/co... File components/autofill/core/common/form_field_data_unittest.cc (right): https://codereview.chromium.org/2022123002/diff/160001/components/autofill/co... components/autofill/core/common/form_field_data_unittest.cc:97: TEST(FormFieldDataTest, SerializeAndDeserialize) { This test case needs to be updated to test version 4. (As I mentioned before: thanks for adding the missing test for version 3, which was for some reason missing from the test. We still need to test the current code as well.)
On 2016/06/08 16:19:55, vabr (Chromium) wrote: > Thanks, this looks very good. I have one last comment below, as one test is > still missing. After that I'm happy to give my approval. > > Please note that you need the security team to review the change to the IPC > header (check the neighbouring OWNERS file for reviewers). > > Also, I won't be at my computer for most of tomorrow, so my next response may > come delayed. Sorry about that. > > Cheers, > Vaclav > > https://codereview.chromium.org/2022123002/diff/160001/components/autofill/co... > File components/autofill/core/common/form_field_data_unittest.cc (right): > > https://codereview.chromium.org/2022123002/diff/160001/components/autofill/co... > components/autofill/core/common/form_field_data_unittest.cc:97: > TEST(FormFieldDataTest, SerializeAndDeserialize) { > This test case needs to be updated to test version 4. > > (As I mentioned before: thanks for adding the missing test for version 3, which > was for some reason missing from the test. We still need to test the current > code as well.) i add test about version 4 in form_field_data_unittest.cc. thank you for your advise. i can understand how to contribute in chromium.
hs1217.lee@gmail.com changed reviewers: + dcheng@chromium.org, inferno@chromium.org, kenrb@chromium.org
On 2016/06/08 22:54:40, hs1217.lee wrote: > mailto:hs1217.lee@gmail.com changed reviewers: > + mailto:dcheng@chromium.org, mailto:inferno@chromium.org, mailto:kenrb@chromium.org could you review about autofill_messages.h. i heard it is need to review from security team. i can find you at /src/components/autofill/content/common/OWNERS.
hs1217.lee@gmail.com changed reviewers: + mkwst@chromium.org, nasko@chromium.org, palmer@chromium.org, tsepez@chromium.org, wfh@chromium.org
On 2016/06/09 14:19:39, hs1217.lee wrote: > mailto:hs1217.lee@gmail.com changed reviewers: > + mailto:mkwst@chromium.org, mailto:nasko@chromium.org, mailto:palmer@chromium.org, > mailto:tsepez@chromium.org, mailto:wfh@chromium.org could you review about autofill_messages.h. i heard it is need to review from security team. i can find you at /src/components/autofill/content/common/OWNERS.
On 2016/06/09 14:19:39, hs1217.lee wrote: > mailto:hs1217.lee@gmail.com changed reviewers: > + mailto:mkwst@chromium.org, mailto:nasko@chromium.org, mailto:palmer@chromium.org, > mailto:tsepez@chromium.org, mailto:wfh@chromium.org could you review about autofill_messages.h. i heard it is need to review from security team. i can find you at /src/components/autofill/content/common/OWNERS.
Changing two booleans to an enum in the IPC LGTM.
Patch set https://codereview.chromium.org/2022123002/#ps180001 LGTM, thanks for this change! One final note: Please do not send a CL to _all_ matching reviewers. Pick one. Imagine the volume of e-mail potential reviewers received if everyone sent out CLs to everybody in the matching OWNERS lists. Anyway, happy coding on Chromium, and thank you for your contribution! :) Vaclav
The CQ bit was checked by hs1217.lee@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2022123002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by hs1217.lee@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2022123002/200001
@vabr i didn't check about browser_tests at patch set #9. so run trybot result was failed. i can figure out about why run failed through trybot log. so i fixed src/chrome/renderer/autofill/form_autofill_browsertest.cc file. could you check new patch set #10 again? https://codereview.chromium.org/2022123002/diff/20001/AUTHORS File AUTHORS (left): https://codereview.chromium.org/2022123002/diff/20001/AUTHORS#oldcode244 AUTHORS:244: Hwanseung Lee <rucifer1217@gmail.com> On 2016/05/31 15:48:25, vabr (Chromium) wrote: > I cannot let you overwrite a different e-mail address. Could you add your line > above line 244 instead of replacing it? rucifer1217@gmail.com is my account. i want just replace it. if you don;t accept, i will write at new line. https://codereview.chromium.org/2022123002/diff/20001/components/autofill/cor... File components/autofill/core/common/form_field_data.cc (right): https://codereview.chromium.org/2022123002/diff/20001/components/autofill/cor... components/autofill/core/common/form_field_data.cc:104: bool is_checkable = On 2016/05/31 15:48:25, vabr (Chromium) wrote: > Why don't you just compare check_status with field.check_status below? i saw this annotation 113 // is_checked and is_autofilled counts as "value" since these change 114 // when we fill things in. so i want compare by only checkable. https://codereview.chromium.org/2022123002/diff/20001/components/autofill/cor... components/autofill/core/common/form_field_data.cc:148: bool is_checkable = On 2016/05/31 15:48:25, vabr (Chromium) wrote: > Again, just use check_status directly. But you will need to static_cast it to > int once you switch to enum class from just enum. i saw this annotation 113 // is_checked and is_autofilled counts as "value" since these change 114 // when we fill things in. so i want compare by only checkable. https://codereview.chromium.org/2022123002/diff/20001/components/autofill/cor... File components/autofill/core/common/form_field_data.h (right): https://codereview.chromium.org/2022123002/diff/20001/components/autofill/cor... components/autofill/core/common/form_field_data.h:32: enum CheckStatus { On 2016/05/31 15:48:25, vabr (Chromium) wrote: > Please use enum class instead of enum, to get better protection against unwanted > casts and also to isolate the values in their own namespace. > > Once you do that, please drop the CHECK_STATUS_ prefix from the values, it will > be replaced by the namespace. if enum class used instead of enum, it is complex to serialize & deserialize. could you teach me how to use easily serialize & deserialize when use enum class?
The CQ bit was unchecked by hs1217.lee@gmail.com
um.. why old comments that was related patchset #1 are attached? never mind below comment.
On 2016/06/09 15:41:59, hs1217.lee wrote: > @vabr > > i didn't check about browser_tests at patch set #9. > so run trybot result was failed. > > i can figure out about why run failed through trybot log. > > so i fixed src/chrome/renderer/autofill/form_autofill_browsertest.cc file. > could you check new patch set #10 again? typo error about patch set number. patch set #9 -> patch set #10 patch set #10 -> patch set #11
On 2016/06/09 15:41:59, hs1217.lee wrote: > @vabr > > i didn't check about browser_tests at patch set #9. > so run trybot result was failed. > > i can figure out about why run failed through trybot log. > > so i fixed src/chrome/renderer/autofill/form_autofill_browsertest.cc file. > could you check new patch set #10 again? typo error about patch set number. patch set #9 -> patch set #10 patch set #10 -> patch set #11
The CQ bit was checked by hs1217.lee@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org, mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/2022123002/#ps200001 (title: " ")
The CQ bit was unchecked by hs1217.lee@gmail.com
Hello, https://codereview.chromium.org/2022123002/#ps200001 LGTM. About the comments: could it be that you sent your recent response with the "Publish+Mail Comments" link, but you had sent all your responses just through the "Reply" link or replying to the code review e-mail notifications? Only the "Publish+Mail Comments" publishes your draft comments (and attaches them to you e-mail). I'm sure I did not see your comments before, so this scenarion seems likely. That also explains why I have never responded to your comments, as draft comments are not visible to others :). Cheers, Vaclav
The CQ bit was checked by hs1217.lee@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2022123002/200001
Message was sent while issue was closed.
Description was changed from ========== Merge autofill::FormFieldData::is_checkable and is_checked Currently, autofill::FormFieldData contains two Boolean data members: * is_checked, and * is_checkable. As expected, the former can only be true when the latter is, but the code makes this not clear. We should merge these two Booleans into a single 3-valued enum representing the values: not checkable, checkable but unchecked, checked. BUG=614280 ========== to ========== Merge autofill::FormFieldData::is_checkable and is_checked Currently, autofill::FormFieldData contains two Boolean data members: * is_checked, and * is_checkable. As expected, the former can only be true when the latter is, but the code makes this not clear. We should merge these two Booleans into a single 3-valued enum representing the values: not checkable, checkable but unchecked, checked. BUG=614280 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Merge autofill::FormFieldData::is_checkable and is_checked Currently, autofill::FormFieldData contains two Boolean data members: * is_checked, and * is_checkable. As expected, the former can only be true when the latter is, but the code makes this not clear. We should merge these two Booleans into a single 3-valued enum representing the values: not checkable, checkable but unchecked, checked. BUG=614280 ========== to ========== Merge autofill::FormFieldData::is_checkable and is_checked Currently, autofill::FormFieldData contains two Boolean data members: * is_checked, and * is_checkable. As expected, the former can only be true when the latter is, but the code makes this not clear. We should merge these two Booleans into a single 3-valued enum representing the values: not checkable, checkable but unchecked, checked. BUG=614280 Committed: https://crrev.com/61b1d174d7dfd788dfe34cad3f8a0b9856894290 Cr-Commit-Position: refs/heads/master@{#399021} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/61b1d174d7dfd788dfe34cad3f8a0b9856894290 Cr-Commit-Position: refs/heads/master@{#399021} |
