[Chromoting] Deprecate AggregatedProcessResourceUsage.*

remoting/host/chromoting_param_traits.cc

Index: remoting/host/chromoting_param_traits.cc
diff --git a/remoting/host/chromoting_param_traits.cc b/remoting/host/chromoting_param_traits.cc
index 0a5f26b372a7c96349f63f1345db4d272515a5b8..965c7cadc26007a9c807f4c5ad05d06caae9dd10 100644
--- a/remoting/host/chromoting_param_traits.cc
+++ b/remoting/host/chromoting_param_traits.cc
@@ -280,14 +280,31 @@ void ParamTraits<remoting::DesktopEnvironmentOptions>::Log(
   l->append("DesktopEnvironmentOptions()");
 }
 
+// static
+void ParamTraits<remoting::protocol::AggregatedProcessResourceUsage>::GetSize(
+    base::PickleSizer* s, const param_type& p) {
+  GetParamSize(s, p.usages_size());
+  for (int i = 0; i < p.usages_size(); i++) {
+    const auto& usage = p.usages().Get(i);
+    GetParamSize(s, usage.process_name());
+    GetParamSize(s, usage.processor_usage());
+    GetParamSize(s, usage.working_set_size());
+    GetParamSize(s, usage.pagefile_size());
+  }
+}
+
 // static
 void ParamTraits<remoting::protocol::AggregatedProcessResourceUsage>::Write(
     base::Pickle* m,
     const remoting::protocol::AggregatedProcessResourceUsage& p) {
-  m->WriteString(p.name());
-  m->WriteDouble(p.processor_usage());
-  m->WriteUInt64(p.working_set_size());
-  m->WriteUInt64(p.pagefile_size());
+  m->WriteInt(p.usages_size());

[estark, 2017/07/02 00:24:04]

Can this be a WriteUint64?

[Hzj_jie, 2017/07/02 20:59:05]

According to the generated code
(https://cs.chromium.org/chromium/src/out/Debug/gen/remoting/proto/process_sta...),
this function should return an integer. Though I have no concern to use
WriteUint64(), which should have (almost) the same effect.

[dcheng, 2017/07/06 00:01:56]

Since this is specified to return int (currently), I think it probably makes
sense to keep it as int. =/

https://developers.google.com/protocol-buffers/docs/reference/cpp-generated#r...

However, rather than iterating through usages here, perhaps we can add a
ParamTraits for RepeatedPtrField<T>, similar to how we have one for
std::vector<T>?

[Hzj_jie, 2017/07/06 00:45:37]

Since RepeatedField<T> is a standard instead of an implementation detail, we can
definitely add it.

But it looks like adding ParamTraits<RepeatedField<T>> is out of the scope of
this change: would you mind me to do it in a different change?

[dcheng, 2017/07/06 08:08:10]

Well, it seems like this CL introduces serialization of a repeated proto field,
doesn't it?

(If you want it to do it in a CL that lands before this one, that would be fine
though)

[Hzj_jie, 2017/07/07 00:59:49]

Definitely, the change is at https://codereview.chromium.org/2968003005/, I will
send it to you for reviewing after trybots finished.

+  for (int i = 0; i < p.usages_size(); i++) {
+    const auto& usage = p.usages().Get(i);
+    m->WriteString(usage.process_name());
+    m->WriteDouble(usage.processor_usage());
+    m->WriteUInt64(usage.working_set_size());
+    m->WriteUInt64(usage.pagefile_size());
+  }
 }
 
 // static
@@ -295,21 +312,30 @@ bool ParamTraits<remoting::protocol::AggregatedProcessResourceUsage>::Read(
     const base::Pickle* m,
     base::PickleIterator* iter,
     remoting::protocol::AggregatedProcessResourceUsage* p) {
-  std::string name;
-  double processor_usage;
-  uint64_t working_set_size;
-  uint64_t pagefile_size;
-  if (!iter->ReadString(&name) ||
-      !iter->ReadDouble(&processor_usage) ||
-      !iter->ReadUInt64(&working_set_size) ||
-      !iter->ReadUInt64(&pagefile_size)) {
+  int usages_size = 0;
+  if (!iter->ReadInt(&usages_size)) {
     return false;
   }
 
-  p->set_name(name);
-  p->set_processor_usage(processor_usage);
-  p->set_working_set_size(working_set_size);
-  p->set_pagefile_size(pagefile_size);
+  for (int i = 0; i < usages_size; i++) {

[dcheng, 2017/07/06 00:01:57]

Most importantly, that lets us consolidate checks for things like |size| being
reasonable: while it doesn't necessarily crash if size is negative here, it
would probably be better to return false and fail deserialization.

[Hzj_jie, 2017/07/06 00:45:37]

I have replaced GetInt() with GetLength(). But I do not think it would crash
here. Have I missed anything?

[dcheng, 2017/07/06 08:08:09]

It won't crash here. But in general, it's preferable to signal deserialization
failure if we unpickled an invalid message (and a negative length seems like it
should always be invalid to me)

[Hzj_jie, 2017/07/07 00:59:49]

Got you. The newly prepared change should also fix this issue.

+    std::string process_name;
+    double processor_usage;
+    uint64_t working_set_size;
+    uint64_t pagefile_size;
+    if (!iter->ReadString(&process_name) ||
+        !iter->ReadDouble(&processor_usage) ||
+        !iter->ReadUInt64(&working_set_size) ||
+        !iter->ReadUInt64(&pagefile_size)) {
+      return false;
+    }
+
+    remoting::protocol::ProcessResourceUsage usage;
+    usage.set_process_name(process_name);
+    usage.set_processor_usage(processor_usage);
+    usage.set_working_set_size(working_set_size);
+    usage.set_pagefile_size(pagefile_size);
+    *p->add_usages() = usage;
+  }
   return true;
 }