Refactor JSONValueConverter Out of DisplayLayout

ash/display/json_converter.cc

+// Copyright 2016 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 "ash/display/json_converter.h"
+
+#include <map>
+#include <string>
+#include <unordered_map>
+
+#include "ash/display/display_layout.h"
+#include "ash/display/display_pref_util.h"
+#include "base/memory/scoped_vector.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/values.h"
+
+namespace ash {
+
+namespace {
+
+// Persistent key names
+const char kMirroredKey[] = "mirrored";
+const char kDefaultUnifiedKey[] = "default_unified";
+const char kPrimaryIdKey[] = "primary-id";
+const char kDisplayPlacementKey[] = "display_placement";
+
+// DisplayPlacement key names
+const char kPositionKey[] = "position";
+const char kOffsetKey[] = "offset";
+const char kDisplayPlacementDisplayIdKey[] = "display_id";
+const char kDisplayPlacementParentDisplayIdKey[] = "parent_display_id";
+
+bool AddLegacyValuesFromValue(const base::Value& value, DisplayLayout* layout) {
+  const base::DictionaryValue* dict_value = nullptr;
+  if (!value.GetAsDictionary(&dict_value) || dict_value == nullptr)
+    return false;
+  int offset;
+  if (dict_value->GetInteger(kOffsetKey, &offset)) {
+    DisplayPlacement::Position position;
+    std::string position_str;
+    if (!dict_value->GetString(kPositionKey, &position_str))
+      return false;
+    DisplayPlacement::StringToPosition(position_str, &position);
+    layout->placement_list.push_back(new DisplayPlacement(position, offset));
+  }
+  return true;
+}
+
+// Returns true if
+//     The key is missing - output is left unchanged

[oshima, 2016/03/17 23:19:38]

can we have a separate function that checks for this? It makes it a bit hard to
follow, and there are very few cases that key may not exist in the code below.

[robliao, 2016/03/18 21:39:57]

This oddly enough is the behavior of JSONValueConverter (see
https://code.google.com/p/chromium/codesearch#chromium/src/base/json/json_val...
and 
https://code.google.com/p/chromium/codesearch#chromium/src/base/json/json_val...
)

Having said that, I suspect we would end up doing something similar to this
regardless of what we decide to do with regards to a missing key since we would
rubber stamp the below for each key.

So the real question is what missing keys should trigger an error?

[oshima, 2016/03/18 21:53:23]

It should use just a default value, maybe error message in case we need to
diagnose the feedback. We had to update the format once in a while, and we tries
to be backward compatible.

[robliao, 2016/03/18 22:55:58]

Sounds like this is what we want then. The input is the default value and so
we'll keep the default if we can't find the field. Added a warning message.

+//     The key matches the type - output is updated to the value.
+template <typename Getter, typename Output>
+bool UpdateFromDict(const base::DictionaryValue* dict_value,
+                    const std::string& field_name,
+                    Getter getter,
+                    Output* output) {
+  const base::Value* field = nullptr;
+  if (!dict_value->Get(field_name, &field))
+    return true;
+
+  return (field->*getter)(output);
+}
+
+// No implementation here as specialization is required.
+template <typename Output>
+bool UpdateFromDict(const base::DictionaryValue* dict_value,
+                    const std::string& field_name,
+                    Output* output);
+
+template <>
+bool UpdateFromDict(const base::DictionaryValue* dict_value,
+                    const std::string& field_name,
+                    bool* output) {
+  return UpdateFromDict(dict_value, field_name, &base::Value::GetAsBoolean,
+                        output);
+}
+
+template <>
+bool UpdateFromDict(const base::DictionaryValue* dict_value,
+                    const std::string& field_name,
+                    int* output) {
+  return UpdateFromDict(dict_value, field_name, &base::Value::GetAsInteger,
+                        output);
+}
+
+template <>
+bool UpdateFromDict(const base::DictionaryValue* dict_value,
+                    const std::string& field_name,
+                    DisplayPlacement::Position* output) {
+  bool (base::Value::*getter)(std::string*) const = &base::Value::GetAsString;
+  std::string value;
+  if (!UpdateFromDict(dict_value, field_name, getter, &value))
+    return false;
+
+  return value.empty() ? true
+                       : DisplayPlacement::StringToPosition(value, output);
+}
+
+template <>
+bool UpdateFromDict(const base::DictionaryValue* dict_value,
+                    const std::string& field_name,
+                    int64_t* output) {
+  bool (base::Value::*getter)(std::string*) const = &base::Value::GetAsString;
+  std::string value;
+  if (!UpdateFromDict(dict_value, field_name, getter, &value))
+    return false;
+
+  return value.empty() ? true : base::StringToInt64(value, output);
+}
+
+template <>
+bool UpdateFromDict(const base::DictionaryValue* dict_value,
+                    const std::string& field_name,
+                    ScopedVector<DisplayPlacement>* output) {
+  bool (base::Value::*getter)(const base::ListValue**) const =
+      &base::Value::GetAsList;
+  const base::ListValue* list = nullptr;
+  if (!UpdateFromDict(dict_value, field_name, getter, &list))
+    return false;
+
+  if (list == nullptr)
+    return true;
+
+  output->reserve(list->GetSize());
+  for (const auto& list_item : *list) {
+    const base::DictionaryValue* item_values = nullptr;
+    if (!list_item->GetAsDictionary(&item_values) || item_values == nullptr)

[oshima, 2016/03/17 23:19:38]

do you need || item_values == nullptr?

[robliao, 2016/03/18 21:39:57]

That's a good question. I just pulled this from the existing
ConvertFromValue/ConvertToValue code.

Looking at the GetAsDictionary implementations, it looks like we can remove
this.

+      return false;
+
+    scoped_ptr<DisplayPlacement> item(new DisplayPlacement);
+    if (!UpdateFromDict(item_values, kOffsetKey, &item->offset) ||
+        !UpdateFromDict(item_values, kPositionKey, &item->position) ||
+        !UpdateFromDict(item_values, kDisplayPlacementDisplayIdKey,
+                        &item->display_id) ||
+        !UpdateFromDict(item_values, kDisplayPlacementParentDisplayIdKey,
+                        &item->parent_display_id)) {
+      return false;
+    }
+
+    output->push_back(std::move(item));
+  }
+  return true;
+}
+
+}  // namespace
+
+bool JsonToDisplayLayout(const base::Value& value, DisplayLayout* layout) {
+  layout->placement_list.clear();
+  const base::DictionaryValue* dict_value = nullptr;
+  if (!value.GetAsDictionary(&dict_value) || dict_value == nullptr)

[oshima, 2016/03/17 23:19:38]

ditto

[robliao, 2016/03/18 21:39:57]

Resolved. See above.

+    return false;
+
+  if (!UpdateFromDict(dict_value, kMirroredKey, &layout->mirrored) ||
+      !UpdateFromDict(dict_value, kDefaultUnifiedKey,
+                      &layout->default_unified) ||
+      !UpdateFromDict(dict_value, kPrimaryIdKey, &layout->primary_id)) {
+    return false;
+  }
+
+  UpdateFromDict(dict_value, kDisplayPlacementKey, &layout->placement_list);
+
+  if (layout->placement_list.size() != 0u)
+    return true;
+
+  // For compatibility with old format.
+  return AddLegacyValuesFromValue(value, layout);
+}
+
+bool DisplayLayoutToJson(const DisplayLayout& layout, base::Value* value) {
+  base::DictionaryValue* dict_value = nullptr;
+  if (!value->GetAsDictionary(&dict_value) || dict_value == nullptr)
+    return false;
+
+  dict_value->SetBoolean(kMirroredKey, layout.mirrored);
+  dict_value->SetBoolean(kDefaultUnifiedKey, layout.default_unified);
+  dict_value->SetString(kPrimaryIdKey, base::Int64ToString(layout.primary_id));
+
+  scoped_ptr<base::ListValue> placement_list(new base::ListValue);
+  for (const auto* placement : layout.placement_list) {
+    scoped_ptr<base::DictionaryValue> placement_value(
+        new base::DictionaryValue);
+    placement_value->SetString(
+        kPositionKey, DisplayPlacement::PositionToString(placement->position));
+    placement_value->SetInteger(kOffsetKey, placement->offset);
+    placement_value->SetString(kDisplayPlacementDisplayIdKey,
+                               base::Int64ToString(placement->display_id));
+    placement_value->SetString(
+        kDisplayPlacementParentDisplayIdKey,
+        base::Int64ToString(placement->parent_display_id));
+    placement_list->Append(std::move(placement_value));
+  }
+  dict_value->Set(kDisplayPlacementKey, std::move(placement_list));
+  return true;
+}
+
+}  // namespace ash