[tracing] Add method to create "weak" global dumps

base/trace_event/process_memory_dump.cc

Index: base/trace_event/process_memory_dump.cc
diff --git a/base/trace_event/process_memory_dump.cc b/base/trace_event/process_memory_dump.cc
index ae60bb06b152639c0e80ab2226a6a96b65699351..f889befe7aae56d8396921cb5029cb658f6b8735 100644
--- a/base/trace_event/process_memory_dump.cc
+++ b/base/trace_event/process_memory_dump.cc
@@ -135,10 +135,24 @@ MemoryAllocatorDump* ProcessMemoryDump::CreateSharedGlobalAllocatorDump(
     const MemoryAllocatorDumpGuid& guid) {
   // A shared allocator dump can be shared within a process and the guid could
   // have been created already.
-  MemoryAllocatorDump* allocator_dump = GetSharedGlobalAllocatorDump(guid);
-  return allocator_dump ? allocator_dump
-                        : CreateAllocatorDump(
-                              GetSharedGlobalAllocatorDumpName(guid), guid);
+  MemoryAllocatorDump* mad = GetSharedGlobalAllocatorDump(guid);
+  // The weak flag is unset because this method should create non-weak dump.

[petrcermak, 2016/01/18 11:23:46]

nit: s/non-weak/a non-weak/

[ssid, 2016/01/18 16:28:36]

Done.

+  if (mad)

[petrcermak, 2016/01/18 11:23:46]

I think that the following would be more readable:

MemoryAllocatorDump* mad = GetSharedGlobalAllocatorDump(guid);
if (mad) {
  // Unset the weak flag of the existing dump (if applicable).
  mad->unset_flags(MemoryAllocatorDump::Flag::WEAK);
  return mad;
}
return CreateAllocatorDump(GetSharedGlobalAllocatorDumpName(guid), guid);

[ssid, 2016/01/18 16:28:36]

Done.

+    mad->unset_flags(MemoryAllocatorDump::Flag::WEAK);
+  else
+    mad = CreateAllocatorDump(GetSharedGlobalAllocatorDumpName(guid), guid);
+  return mad;
+}
+
+MemoryAllocatorDump* ProcessMemoryDump::CreateWeakSharedGlobalAllocatorDump(
+    const MemoryAllocatorDumpGuid& guid) {
+  MemoryAllocatorDump* mad = GetSharedGlobalAllocatorDump(guid);
+  if (!mad) {
+    mad = new MemoryAllocatorDump(GetSharedGlobalAllocatorDumpName(guid), this,

[petrcermak, 2016/01/18 11:23:46]

At this point, I wonder whether it wouldn't make more sense to drop the new
MemoryAllocatorDump constructor (with the flags argument) and do the following
here:

if (!mad) {
  mad = CreateAllocatorDump(GetSharedGlobalAllocatorDumpName(guid), guid);
  mad->set_flags(MemoryAllocatorDump::Flag::WEAK);
}
return mad;

Rationale: Make the source code of CreateSharedGlobalAllocatorDump and
CreateWeakSharedGlobalAllocatorDump more similar.


In addition, similarly to the first method, I think that the following would be
more readable:

MemoryAllocatorDump* mad = GetSharedGlobalAllocatorDump(guid);
if (mad)
  return mad;
mad = CreateAllocatorDump(GetSharedGlobalAllocatorDumpName(guid), guid);
mad->set_flags(MemoryAllocatorDump::Flag::WEAK);
return mad;


One more thing, the CreateAllocatorDump(GetSharedGlobalAllocatorDumpName(guid),
guid) could be factored out into a separate method (and shared between
CreateSharedGlobalAllocatorDump and CreateWeakSharedGlobalAllocatorDump), for
example:

MemoryAllocatorDump* ProcessMemoryDump::AddSharedGlobalAllocatorDumpInternal(
    const MemoryAllocatorDumpGuid& guid) {
  return CreateAllocatorDump(GetSharedGlobalAllocatorDumpName(guid), guid);
}

[ssid, 2016/01/18 16:28:36]

Hm not sure if one line method is really needed. Used the second snippet you
suggested.

[petrcermak, 2016/01/18 17:18:12]

Acknowledged.

+                                  guid, MemoryAllocatorDump::Flag::WEAK);
+    AddAllocatorDumpInternal(mad);  // Takes ownership of |mad|.
+  }
+  return mad;
 }
 
 MemoryAllocatorDump* ProcessMemoryDump::GetSharedGlobalAllocatorDump(