Expand aura::PropertyConverter support.

ui/aura/mus/property_converter.cc

Index: ui/aura/mus/property_converter.cc
diff --git a/ui/aura/mus/property_converter.cc b/ui/aura/mus/property_converter.cc
index 070c8ff07e7f9c66fd6c6d5d19cddf2c7ed47394..a66f373b92024cf73526637d46de9bdda822ee58 100644
--- a/ui/aura/mus/property_converter.cc
+++ b/ui/aura/mus/property_converter.cc
@@ -14,64 +14,157 @@ namespace aura {
 
 namespace {
 
-// Get the aura property key and name for a mus property transport |name|.
-bool GetPropertyKeyForTransportName(const std::string& name,
-                                    const void** key_out,
-                                    const char** name_out) {
-  if (name == ui::mojom::WindowManager::kAlwaysOnTop_Property) {
-    *key_out = client::kAlwaysOnTopKey;
-    *name_out = client::kAlwaysOnTopKey->name;
-    return true;
-  }
-  return false;
+// Get the WindowProperty's value as a byte array. Only supports aura properties
+// that point to types with a matching std::vector<uint8_t> mojo::TypeConverter.
+template <typename T>
+std::unique_ptr<std::vector<uint8_t>> GetArray(Window* window,

[sky, 2016/11/16 17:56:13]

optional: naming this as get implies it isn't doing conversion, where as this is
really a conversion function. Maybe ConvertWindowValueToArray? I would be
inclined to name all of these functions with Convert in them. But I'm listing
this as optional, so it's up to you.

[msw, 2016/11/16 22:47:45]

Done.

+                                               const WindowProperty<T>* key) {
+  const T value = window->GetProperty(key);
+  if (!value)
+    return base::MakeUnique<std::vector<uint8_t>>();
+  return base::MakeUnique<std::vector<uint8_t>>(
+      mojo::ConvertTo<std::vector<uint8_t>>(*value));
+}
+
+// Get the primitive aura property value to store for a mus transport value.
+int64_t GetAuraValue(const std::vector<uint8_t>& data) {
+  CHECK_LE(data.size(), 8u);
+  int64_t value = 0;
+  for (size_t i = data.size(); i > 0; --i)
+    value = (value << 8) + data[i - 1];
+  return value;
+}
+
+// Get the mus transport value to store for a primitive aura property value.
+mojo::Array<uint8_t> GetMusValue(int64_t value) {
+  mojo::Array<uint8_t> transport_value(8);
+  transport_value[7] = (value >> 56) & 0xFF;

[sky, 2016/11/16 17:56:13]

optional: it's more common that the higher bits are first. See
services/ui/public/cpp/property_type_converters for what I mean. Doing that
would mean you don't have to iterate backwards on line 33 (which is more error
prone with size_t IMO).

[msw, 2016/11/16 22:47:44]

Done. I can use existing type converters with static casts to int64_t.

+  transport_value[6] = (value >> 48) & 0xFF;
+  transport_value[5] = (value >> 40) & 0xFF;
+  transport_value[4] = (value >> 32) & 0xFF;
+  transport_value[3] = (value >> 24) & 0xFF;
+  transport_value[2] = (value >> 16) & 0xFF;
+  transport_value[1] = (value >> 8) & 0xFF;
+  transport_value[0] = value & 0xFF;
+  return transport_value;
 }
 
 }  // namespace
 
-PropertyConverter::PropertyConverter() {}
+PropertyConverter::PropertyConverter() {
+  // Add known aura properties with associated mus properties.
+  RegisterPrimitiveProperty(client::kAlwaysOnTopKey,
+                            client::kAlwaysOnTopKey->name,
+                            ui::mojom::WindowManager::kAlwaysOnTop_Property);
+  RegisterPrimitiveProperty(client::kExcludeFromMruKey,
+                            client::kExcludeFromMruKey->name,
+                            ui::mojom::WindowManager::kExcludeFromMru_Property);
+  RegisterRectProperty(client::kRestoreBoundsKey,
+                       ui::mojom::WindowManager::kRestoreBounds_Property);
+  RegisterStringProperty(client::kAppIdKey,
+                         ui::mojom::WindowManager::kAppID_Property);
+}
 
 PropertyConverter::~PropertyConverter() {}
 
 bool PropertyConverter::ConvertPropertyForTransport(
     Window* window,
     const void* key,
-    std::string* server_property_name,
-    std::unique_ptr<std::vector<uint8_t>>* server_property_value) {
-  *server_property_name = GetTransportNameForPropertyKey(key);
-  if (!server_property_name->empty()) {
-    // TODO(msw): Using the int64_t accessor is wasteful for bool, etc.
-    const int64_t value = window->GetPropertyInternal(key, 0);
-    *server_property_value = base::MakeUnique<std::vector<uint8_t>>(
-        mojo::ConvertTo<std::vector<uint8_t>>(value));
+    std::string* transport_name,
+    std::unique_ptr<std::vector<uint8_t>>* transport_value) {
+  *transport_name = GetTransportNameForPropertyKey(key);
+  if (transport_name->empty()) {
+    DVLOG(2) << "Unknown aura property key: " << key;

[sky, 2016/11/16 17:56:13]

I'm ok with a DVLOG, but be warned there are going to be plenty of properties
that shouldn't be mapped, so if this is log level 2 is enabled this is going to
log a bunch of stuff.

[msw, 2016/11/16 22:47:45]

Yeah, this logging probably isn't too helpful; removed.

+    return false;
+  }
+
+  auto rect_key = static_cast<const WindowProperty<gfx::Rect*>*>(key);
+  if (rect_properties_.count(rect_key) > 0) {
+    *transport_value = GetArray(window, rect_key);
     return true;
   }
-  DVLOG(2) << "Unknown aura property key: " << key;
-  return false;
+
+  auto string_key = static_cast<const WindowProperty<std::string*>*>(key);
+  if (string_properties_.count(string_key) > 0) {
+    *transport_value = GetArray(window, string_key);
+    return true;
+  }
+
+  // Handle primitive property types generically.
+  DCHECK_GT(primitive_properties_.count(key), 0u);
+  // TODO(msw): Using the int64_t accessor is wasteful for smaller types.
+  const int64_t value = window->GetPropertyInternal(key, 0);
+  *transport_value = base::MakeUnique<std::vector<uint8_t>>(GetMusValue(value));
+  return true;
 }
 
 std::string PropertyConverter::GetTransportNameForPropertyKey(const void* key) {
-  if (key == client::kAlwaysOnTopKey)
-    return ui::mojom::WindowManager::kAlwaysOnTop_Property;
+  if (primitive_properties_.count(key) > 0)
+    return primitive_properties_[key].second;
+
+  auto rect_key = static_cast<const WindowProperty<gfx::Rect*>*>(key);
+  if (rect_properties_.count(rect_key) > 0)
+    return rect_properties_[rect_key];
+
+  auto string_key = static_cast<const WindowProperty<std::string*>*>(key);
+  if (string_properties_.count(string_key) > 0)
+    return string_properties_[string_key];
+
   return std::string();
 }
 
 void PropertyConverter::SetPropertyFromTransportValue(
     Window* window,
-    const std::string& server_property_name,
+    const std::string& transport_name,
     const std::vector<uint8_t>* data) {
-  const void* key = nullptr;
-  const char* name = nullptr;
-  if (GetPropertyKeyForTransportName(server_property_name, &key, &name)) {
-    DCHECK(key);
-    DCHECK(name);
-    // Aura window only supports property types that fit in int64_t.
-    CHECK_LE(8u, data->size()) << " Property type not supported: " << key;
-    const int64_t value = mojo::ConvertTo<int64_t>(*data);
-    // TODO(msw): Should aura::Window just store all properties by name?
-    window->SetPropertyInternal(key, name, nullptr, value, 0);
-  } else {
-    DVLOG(2) << "Unknown mus property name: " << server_property_name;
+  for (const auto& primitive_property : primitive_properties_) {
+    if (primitive_property.second.second == transport_name) {
+      // aura::Window only supports property types that fit in int64_t.
+      CHECK_LE(data->size(), 8u) << " Property too big: " << transport_name;

[sky, 2016/11/16 17:56:13]

I think this should DVLOG and ignore the value. The reason is if you have code
like this, then a malicious client can easily crash ash by sending a value with
a length < 8.

[msw, 2016/11/16 22:47:44]

Done.

+      const int64_t value = GetAuraValue(*data);
+      // TODO(msw): Should aura::Window just store all properties by name?
+      window->SetPropertyInternal(primitive_property.first,
+                                  primitive_property.second.first, nullptr,
+                                  value, 0);
+      return;
+    }
+  }
+
+  for (const auto& rect_property : rect_properties_) {
+    if (rect_property.second == transport_name) {
+      const gfx::Rect value = mojo::ConvertTo<gfx::Rect>(*data);

[sky, 2016/11/16 17:56:13]

Similar comment here and 143 about verifying type. I'm ok if you want to tackle
it later, just add TODOs.

[msw, 2016/11/16 22:47:44]

I added a size check for rect, but I'm not sure what to do for string, so I
added a TODO there. Let me know if you have thoughts for stronger validation, or
how to validate array->string.

+      window->SetProperty(rect_property.first, new gfx::Rect(value));
+      return;
+    }
   }
+
+  for (const auto& string_property : string_properties_) {
+    if (string_property.second == transport_name) {
+      const std::string value = mojo::ConvertTo<std::string>(*data);
+      window->SetProperty(string_property.first, new std::string(value));
+      return;
+    }
+  }
+
+  DVLOG(2) << "Unknown mus property name: " << transport_name;
+}
+
+void PropertyConverter::RegisterPrimitiveProperty(const void* key,
+                                                  const char* aura_name,
+                                                  const char* transport_name) {
+  primitive_properties_[key] = PropertyNames(aura_name, transport_name);
+}
+
+void PropertyConverter::RegisterRectProperty(
+    const WindowProperty<gfx::Rect*>* property,
+    const char* transport_name) {
+  rect_properties_[property] = transport_name;
+}
+
+void PropertyConverter::RegisterStringProperty(
+    const WindowProperty<std::string*>* property,
+    const char* transport_name) {
+  string_properties_[property] = transport_name;
 }
 
 }  // namespace aura