Refactor the naming scheme of Deserialize* functions in form_field_data.cc

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 32 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 15:40:01]

Sorry for nitpicking, but I would still suggest to name the functions in an easy
sequential scheme, like:
DeserializeSection1, DeserializeSection2, DeserializeSection3, etc.

The long names are hard to read, the parts like "Common" bring little useful
information (the fact that they are common is apparent from them appearing
everywhere). Using numbers both to refer to sequence and the versions is also
confusing.

                                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);
+         iter->ReadBool(&field_data->is_autofilled);
+}
+
+bool DeserializeCheckStaus(base::PickleIterator* iter,

[vabr (Chromium), 2016/06/06 15:40:01]

Actually, let's not split DeserializeCommonSection1 here at all, keep this CL
just for the renaming. The split belongs to your main CL
(https://codereview.chromium.org/2022123002/).

When you do it there, you will notice that the second and third part split from
what you'll rename to DeserializeSection1 will need to take numbers 5 and 6,
because 2 and 3 will be already taken. But that's no problem. The main point
will be that you won't have to edit the deserialization code for older versions
at all, and the simple names like "DeserializeSection2" will not need to be
changed (as they are not tied to what is actually done and for which version and
where).

[vabr (Chromium), 2016/06/06 15:40:01]

typo: Staus -> Status

Having said that, this name would become confusing in your new patch (as this
part will actually be replaced by check_status). Please just do the
DeserializeSection<number> as advised above.

+                           FormFieldData* field_data) {
+  return iter->ReadBool(&field_data->is_checked) &&
+         iter->ReadBool(&field_data->is_checkable);
 }
 
 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);
+  return iter->ReadBool(&field_data->is_focusable) &&
+         iter->ReadBool(&field_data->should_autocomplete);
 }
 
 bool DeserializeVersion2Specific(base::PickleIterator* iter,
                                  FormFieldData* field_data) {
   return ReadAsInt(iter, &field_data->role);
 }
 
+bool DeserializeCommonSection3(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 DeserializeVersion3Specific(base::PickleIterator* iter,
                                  FormFieldData* field_data) {
   return iter->ReadString16(&field_data->placeholder);
 }
 
 }  // namespace
 
 FormFieldData::FormFieldData()
     : max_length(0),
       is_autofilled(false),
@@ skipping 92 matching lines @@
   int version;
   FormFieldData temp_form_field_data;
   if (!iter->ReadInt(&version)) {
     LOG(ERROR) << "Bad pickle of FormFieldData, no version present";
     return false;
   }
 
   switch (version) {
     case 1: {
       if (!DeserializeCommonSection1(iter, &temp_form_field_data) ||
-          !DeserializeCommonSection2(iter, &temp_form_field_data)) {
+          !DeserializeCheckStaus(iter, &temp_form_field_data) ||
+          !DeserializeCommonSection2(iter, &temp_form_field_data) ||
+          !DeserializeCommonSection3(iter, &temp_form_field_data)) {
         LOG(ERROR) << "Could not deserialize FormFieldData from pickle";
         return false;
       }
       break;
     }
     case 2: {
       if (!DeserializeCommonSection1(iter, &temp_form_field_data) ||
+          !DeserializeCheckStaus(iter, &temp_form_field_data) ||
+          !DeserializeCommonSection2(iter, &temp_form_field_data) ||
           !DeserializeVersion2Specific(iter, &temp_form_field_data) ||
-          !DeserializeCommonSection2(iter, &temp_form_field_data)) {
+          !DeserializeCommonSection3(iter, &temp_form_field_data)) {
         LOG(ERROR) << "Could not deserialize FormFieldData from pickle";
         return false;
       }
       break;
     }
     case 3: {
       if (!DeserializeCommonSection1(iter, &temp_form_field_data) ||
+          !DeserializeCheckStaus(iter, &temp_form_field_data) ||
+          !DeserializeCommonSection2(iter, &temp_form_field_data) ||
           !DeserializeVersion2Specific(iter, &temp_form_field_data) ||
-          !DeserializeCommonSection2(iter, &temp_form_field_data) ||
+          !DeserializeCommonSection3(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;
     }
@@ skipping 10 matching lines @@
             << " " << field.max_length << " "
             << (field.is_autofilled ? "true" : "false") << " "
             << (field.is_checked ? "true" : "false") << " "
             << (field.is_checkable ? "true" : "false") << " "
             << (field.is_focusable ? "true" : "false") << " "
             << (field.should_autocomplete ? "true" : "false") << " "
             << field.role << " " << field.text_direction;
 }
 
 }  // namespace autofill