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"
 
 namespace autofill {
 
 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) {
   pickle->WriteInt(static_cast<int>(strings.size()));
   for (size_t i = 0; i < strings.size(); ++i) {
     pickle->WriteString16(strings[i]);
   }
 }
 
 bool ReadStringVector(base::PickleIterator* iter,
@@ skipping 15 matching lines @@
 template <typename T>
 bool ReadAsInt(base::PickleIterator* iter, T* target_value) {
   int pickle_data;
   if (!iter->ReadInt(&pickle_data))
     return false;
 
   *target_value = static_cast<T>(pickle_data);
   return true;
 }
 
 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,
                                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);
 }
 
+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),
       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.
   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 &&
+         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;
   // 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;
+  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;
   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 26 matching lines @@
     case 3: {
       if (!DeserializeCommonSection1(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;
     }
+    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;
     }
   }
   *field_data = temp_form_field_data;
   return true;
 }
 
 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