Merge autofill::FormFieldData::is_checkable and is_checked

components/autofill/core/common/form_field_data.cc

Index: components/autofill/core/common/form_field_data.cc
diff --git a/components/autofill/core/common/form_field_data.cc b/components/autofill/core/common/form_field_data.cc
index dee52c7908cec94f72058b4a34edbf92bb8a70e4..0eca7acf2a5defa9a4a1eb08df6a6ba54fd605d8 100644
--- a/components/autofill/core/common/form_field_data.cc
+++ b/components/autofill/core/common/form_field_data.cc
@@ -59,8 +59,7 @@ bool DeserializeCommonSection1(base::PickleIterator* iter,
          iter->ReadString(&field_data->autocomplete_attribute) &&
          iter->ReadUInt64(&field_data->max_length) &&
          iter->ReadBool(&field_data->is_autofilled) &&
-         iter->ReadBool(&field_data->is_checked) &&
-         iter->ReadBool(&field_data->is_checkable) &&
+         iter->Readint(&field_data->check_status) &&

[vabr (Chromium), 2016/05/31 15:48:25]

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.

          iter->ReadBool(&field_data->is_focusable) &&
          iter->ReadBool(&field_data->should_autocomplete);
 }
@@ -87,8 +86,7 @@ bool DeserializeVersion3Specific(base::PickleIterator* iter,
 FormFieldData::FormFieldData()
     : max_length(0),
       is_autofilled(false),
-      is_checked(false),
-      is_checkable(false),
+      check_status(CHECK_STATUS_NOT_CHECKABLE);
       is_focusable(false),
       should_autocomplete(true),
       role(ROLE_ATTRIBUTE_OTHER),
@@ -103,13 +101,18 @@ FormFieldData::~FormFieldData() {
 bool FormFieldData::SameFieldAs(const FormFieldData& field) const {
   // A FormFieldData stores a value, but the value is not part of the identity
   // of the field, so we don't want to compare the values.
+  bool is_checkable =

[vabr (Chromium), 2016/05/31 15:48:25]

Why don't you just compare check_status with field.check_status below?

[Hwanseung Lee(hs1217.lee), 2016/06/09 15:41:59]

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.

+    (check_status == CHECK_STATUS_NOT_CHECKABLE) ? false : true;
+  bool field_is_checkable =
+    (field.check_status == CHECK_STATUS_NOT_CHECKABLE) ? false : true;
+
   return label == field.label && name == field.name &&
          form_control_type == field.form_control_type &&
          autocomplete_attribute == field.autocomplete_attribute &&
          placeholder == field.placeholder && max_length == field.max_length &&
          // is_checked and is_autofilled counts as "value" since these change
          // when we fill things in.
-         is_checkable == field.is_checkable &&
+         is_checkable == field_is_checkable &&
          is_focusable == field.is_focusable &&
          should_autocomplete == field.should_autocomplete &&
          role == field.role && text_direction == field.text_direction;
@@ -142,8 +145,12 @@ bool FormFieldData::operator<(const FormFieldData& field) const {
   if (max_length < field.max_length) return true;
   if (max_length > field.max_length) return false;
   // Skip |is_checked| and |is_autofilled| as in SameFieldAs.
-  if (is_checkable < field.is_checkable) return true;
-  if (is_checkable > field.is_checkable) return false;
+  bool is_checkable =

[vabr (Chromium), 2016/05/31 15:48:25]

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.

[Hwanseung Lee(hs1217.lee), 2016/06/09 15:41:59]

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.

+    (check_status == CHECK_STATUS_NOT_CHECKABLE) ? false : true;
+  bool field_is_checkable =
+    (field.check_status == CHECK_STATUS_NOT_CHECKABLE) ? false : true;
+  if (is_checkable < field_is_checkable) return true;
+  if (is_checkable > field_is_checkable) return false;
   if (is_focusable < field.is_focusable) return true;
   if (is_focusable > field.is_focusable) return false;
   if (should_autocomplete < field.should_autocomplete) return true;
@@ -166,8 +173,7 @@ void SerializeFormFieldData(const FormFieldData& field_data,
   pickle->WriteString(field_data.autocomplete_attribute);
   pickle->WriteUInt64(field_data.max_length);
   pickle->WriteBool(field_data.is_autofilled);
-  pickle->WriteBool(field_data.is_checked);
-  pickle->WriteBool(field_data.is_checkable);
+  pickle->WriteInt(field_data.check_status);
   pickle->WriteBool(field_data.is_focusable);
   pickle->WriteBool(field_data.should_autocomplete);
   pickle->WriteInt(field_data.role);
@@ -230,8 +236,7 @@ std::ostream& operator<<(std::ostream& os, const FormFieldData& field) {
             << " " << field.autocomplete_attribute << " " << field.placeholder
             << " " << field.max_length << " "
             << (field.is_autofilled ? "true" : "false") << " "
-            << (field.is_checked ? "true" : "false") << " "
-            << (field.is_checkable ? "true" : "false") << " "
+            << field.check_status

[vabr (Chromium), 2016/05/31 15:48:25]

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.

             << (field.is_focusable ? "true" : "false") << " "
             << (field.should_autocomplete ? "true" : "false") << " "
             << field.role << " " << field.text_direction;