Merge autofill::FormFieldData::is_checkable and is_checked

components/autofill/core/common/form_field_data.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/autofill/core/common/form_field_data.h"
 
 #include "base/pickle.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 
@@ skipping 41 matching lines @@
 
 bool DeserializeCommonSection1(base::PickleIterator* iter,
                                FormFieldData* field_data) {
   return iter->ReadString16(&field_data->label) &&
          iter->ReadString16(&field_data->name) &&
          iter->ReadString16(&field_data->value) &&
          iter->ReadString(&field_data->form_control_type) &&
          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);
 }
 
 bool DeserializeCommonSection2(base::PickleIterator* iter,
                                FormFieldData* field_data) {
   return ReadAsInt(iter, &field_data->text_direction) &&
          ReadStringVector(iter, &field_data->option_values) &&
          ReadStringVector(iter, &field_data->option_contents);
 }
 
 bool DeserializeVersion2Specific(base::PickleIterator* iter,
                                  FormFieldData* field_data) {
   return ReadAsInt(iter, &field_data->role);
 }
 
 bool DeserializeVersion3Specific(base::PickleIterator* iter,
                                  FormFieldData* field_data) {
   return iter->ReadString16(&field_data->placeholder);
 }
 
 }  // namespace
 
 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),
       text_direction(base::i18n::UNKNOWN_DIRECTION) {
 }
 
 FormFieldData::FormFieldData(const FormFieldData& other) = default;
 
 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;
   // The option values/contents which are the list of items in the list
   // of a drop-down are currently not considered part of the identity of
   // a form element. This is debatable, since one might base heuristics
   // on the types of elements that are available. Alternatively, one
   // could imagine some forms that dynamically change the element
   // contents (say, insert years starting from the current year) that
   // should not be considered changes in the structure of the form.
@@ skipping 12 matching lines @@
   if (name > field.name) return false;
   if (form_control_type < field.form_control_type) return true;
   if (form_control_type > field.form_control_type) return false;
   if (autocomplete_attribute < field.autocomplete_attribute) return true;
   if (autocomplete_attribute > field.autocomplete_attribute) return false;
   if (placeholder < field.placeholder) return true;
   if (placeholder > field.placeholder) return false;
   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;
   if (should_autocomplete > field.should_autocomplete) return false;
   if (role < field.role) return true;
   if (role > field.role) return false;
   if (text_direction < field.text_direction) return true;
   if (text_direction > field.text_direction) return false;
   // See operator== above for why we don't check option_values/contents.
   return false;
 }
 
 void SerializeFormFieldData(const FormFieldData& field_data,
                             base::Pickle* pickle) {
   pickle->WriteInt(kPickleVersion);
   pickle->WriteString16(field_data.label);
   pickle->WriteString16(field_data.name);
   pickle->WriteString16(field_data.value);
   pickle->WriteString(field_data.form_control_type);
   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);
   pickle->WriteInt(field_data.text_direction);
   AddVectorToPickle(field_data.option_values, pickle);
   AddVectorToPickle(field_data.option_contents, pickle);
   pickle->WriteString16(field_data.placeholder);
 }
 
 bool DeserializeFormFieldData(base::PickleIterator* iter,
@@ skipping 42 matching lines @@
   return true;
 }
 
 std::ostream& operator<<(std::ostream& os, const FormFieldData& field) {
   return os << base::UTF16ToUTF8(field.label) << " "
             << base::UTF16ToUTF8(field.name) << " "
             << base::UTF16ToUTF8(field.value) << " " << field.form_control_type
             << " " << 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;
 }
 
 }  // namespace autofill