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..fab02362cb94371b81bc9c06cbe1efdd567459f1 100644
--- a/components/autofill/core/common/form_field_data.cc
+++ b/components/autofill/core/common/form_field_data.cc
@@ -14,7 +14,7 @@ namespace {
 
 // Increment this anytime pickle format is modified as well as provide
 // deserialization routine from previous kPickleVersion format.
-const int kPickleVersion = 3;
+const int kPickleVersion = 4;
 
 void AddVectorToPickle(std::vector<base::string16> strings,
                        base::Pickle* pickle) {
@@ -52,17 +52,22 @@ bool ReadAsInt(base::PickleIterator* iter, T* target_value) {
 
 bool DeserializeCommonSection1(base::PickleIterator* iter,

[vabr (Chromium), 2016/06/06 12:39:30]

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.

                                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->ReadBool(&field_data->is_focusable) &&
-         iter->ReadBool(&field_data->should_autocomplete);
+  bool ret = true;

[vabr (Chromium), 2016/06/06 12:39:30]

nit: Please avoid abbreviations [1]. My suggestion is to rename |ret| to
|success|.

[1] https://google.github.io/styleguide/cppguide.html#General_Naming_Rules

+  bool is_checkable = false;
+  bool is_checked = false;
+  ret = ret && iter->ReadString16(&field_data->label);

[vabr (Chromium), 2016/06/06 12:39:30]

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;

+  ret = ret && iter->ReadString16(&field_data->name);
+  ret = ret && iter->ReadString16(&field_data->value);
+  ret = ret && iter->ReadString(&field_data->form_control_type);
+  ret = ret && iter->ReadString(&field_data->autocomplete_attribute);
+  ret = ret && iter->ReadUInt64(&field_data->max_length);
+  ret = ret && iter->ReadBool(&field_data->is_autofilled);
+  ret = ret && iter->ReadBool(&is_checked);
+  ret = ret && iter->ReadBool(&is_checkable);
+  SetCheckStatus(field_data, is_checkable, is_checked);
+  ret = ret && iter->ReadBool(&field_data->is_focusable);
+  ret = ret && iter->ReadBool(&field_data->should_autocomplete);
+  return ret;
 }
 
 bool DeserializeCommonSection2(base::PickleIterator* iter,
@@ -82,13 +87,26 @@ bool DeserializeVersion3Specific(base::PickleIterator* iter,
   return iter->ReadString16(&field_data->placeholder);
 }
 
+bool DeserializeCommonSection1Version4Specific(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) &&
+         ReadAsInt(iter, &field_data->check_status) &&
+         iter->ReadBool(&field_data->is_focusable) &&
+         iter->ReadBool(&field_data->should_autocomplete);
+}
+
 }  // namespace
 
 FormFieldData::FormFieldData()
     : max_length(0),
       is_autofilled(false),
-      is_checked(false),
-      is_checkable(false),
+      check_status(CheckStatus::NOT_CHECKABLE),

[vabr (Chromium), 2016/06/06 12:39:30]

No need for CheckStatus:: because you are not using enum class.

       is_focusable(false),
       should_autocomplete(true),
       role(ROLE_ATTRIBUTE_OTHER),
@@ -109,7 +127,7 @@ bool FormFieldData::SameFieldAs(const FormFieldData& field) const {
          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 &&
+         IsCheckable(check_status) == IsCheckable(field.check_status) &&
          is_focusable == field.is_focusable &&
          should_autocomplete == field.should_autocomplete &&
          role == field.role && text_direction == field.text_direction;
@@ -142,8 +160,8 @@ 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;
+  if (IsCheckable(check_status) < IsCheckable(field.check_status)) return true;
+  if (IsCheckable(check_status) > IsCheckable(field.check_status)) 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 +184,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);
@@ -214,6 +231,17 @@ bool DeserializeFormFieldData(base::PickleIterator* iter,
       }
       break;
     }
+    case 4: {
+      if (!DeserializeCommonSection1Version4Specific(iter,
+            &temp_form_field_data) ||
+          !DeserializeVersion2Specific(iter, &temp_form_field_data) ||
+          !DeserializeCommonSection2(iter, &temp_form_field_data) ||
+          !DeserializeVersion3Specific(iter, &temp_form_field_data)) {
+        LOG(ERROR) << "Could not deserialize FormFieldData from pickle";
+        return false;
+      }
+      break;
+    }
     default: {
       LOG(ERROR) << "Unknown FormFieldData pickle version " << version;
       return false;
@@ -224,17 +252,62 @@ bool DeserializeFormFieldData(base::PickleIterator* iter,
 }
 
 std::ostream& operator<<(std::ostream& os, const FormFieldData& field) {
+  std::string checkStatusStr;

[vabr (Chromium), 2016/06/06 12:39:30]

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

+  switch (field.check_status) {
+    case FormFieldData::CheckStatus::NOT_CHECKABLE:
+      checkStatusStr = "NOT_CHECKABLE";
+      break;
+    case FormFieldData::CheckStatus::CHECKABLE_BUT_UNCHECKED:
+      checkStatusStr = "CHECKABLE_BUT_UNCHECKED";
+      break;
+    case FormFieldData::CheckStatus::CHECKED:
+      checkStatusStr = "CHECKED";
+      break;
+  }
+
+  std::string roleStr;
+  switch (field.role) {
+    case FormFieldData::RoleAttribute::ROLE_ATTRIBUTE_PRESENTATION:
+      roleStr = "ROLE_ATTRIBUTE_PRESENTATION";
+      break;
+    case FormFieldData::RoleAttribute::ROLE_ATTRIBUTE_OTHER:
+      roleStr = "ROLE_ATTRIBUTE_OTHER";
+      break;
+  }
+
   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") << " "
+            << checkStatusStr
             << (field.is_focusable ? "true" : "false") << " "
             << (field.should_autocomplete ? "true" : "false") << " "
-            << field.role << " " << field.text_direction;
+            << roleStr << " " << field.text_direction;
+}
+
+bool IsCheckable(const FormFieldData::CheckStatus& check_status){
+  return check_status == FormFieldData::CheckStatus::NOT_CHECKABLE

[vabr (Chromium), 2016/06/06 12:39:30]

No need for the ternary operator:
X == Y ? false : true 
is the same as
X != Y

+          ? false : true;
+}
+
+bool IsChecked(const FormFieldData::CheckStatus& check_status){
+  return check_status == FormFieldData::CheckStatus::CHECKED ? false : true;

[vabr (Chromium), 2016/06/06 12:39:30]

This returns the opposite of what it should return.

Also, drop the ternary operator, just
return check_status == FormFieldData::CheckStatus::CHECKED;

+}
+
+void SetCheckStatus(FormFieldData* form_field_data, bool isCheckable,
+                    bool isChecked){
+  if(isChecked == true){

[vabr (Chromium), 2016/06/06 12:39:30]

nit: Formatting is off, please make sure to run git cl format.

[vabr (Chromium), 2016/06/06 12:39:30]

Drop " == true" (also below).

+    form_field_data->check_status = FormFieldData::CheckStatus::CHECKED;
+  }else{
+    if(isCheckable == true){
+      form_field_data->check_status =
+        FormFieldData::CheckStatus::CHECKABLE_BUT_UNCHECKED;
+    }else{
+      form_field_data->check_status = FormFieldData::CheckStatus::NOT_CHECKABLE;
+    }
+  }
 }
 
 }  // namespace autofill