memory-infra: Start disentangling tracing from memory-infra

base/trace_event/memory_tracing_observer.h

+// Copyright 2017 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.
+
+#ifndef BASE_TRACE_EVENT_MEMORY_TRACING_OBSERVER_H_
+#define BASE_TRACE_EVENT_MEMORY_TRACING_OBSERVER_H_
+
+#include "base/trace_event/memory_dump_manager.h"
+#include "base/trace_event/trace_event.h"
+
+namespace base {
+
+namespace trace_event {
+
+// Observes TraceLog for Enable/Disable events and sets the correct state in
+// MemoryDumpManager to handle XXX.

[Primiano Tucci (use gerrit), 2017/04/21 10:10:27]

xxx? :)

[hjd, 2017/04/21 11:51:58]

Done.

+class BASE_EXPORT MemoryTracingObserver
+    : public TraceLog::EnabledStateObserver {
+ public:
+  static const char* const kTraceCategory;
+  static void MaybeAddDumpToTrace(const MemoryDumpRequestArgs*,

[Primiano Tucci (use gerrit), 2017/04/21 10:10:27]

plz make this non-static. It's always weird to see a singleton (well, this is
singleton de facto) class which has static methods. Instead just make
MDM::FinalizeAndAddTotrace non-static. We are slowly migrating to a saner model
where MDM is always operational, so there will be less need for that level of
paranoy

Also I think this should have a bool return value (see top-level comment to this
CL, ping me if I forgot to add it :) )

[hjd, 2017/04/21 11:51:58]

Done.

+                                  const ProcessId,
+                                  const ProcessMemoryDump*);
+
+  MemoryTracingObserver(TraceLog*, MemoryDumpManager*);

[Primiano Tucci (use gerrit), 2017/04/21 10:10:27]

since we don't have any testing need, I'd just use TraceLog::GetInstancE() and
MDM::GetInstance().
I'm in favor of this pattern if there is a need for testing, but this seems no
more

[hjd, 2017/04/21 11:51:58]

It seems to me that using the singletons directly is just making debt for
ourselves to clean up later. Part of our problem is we have a bunch of
singletons, some with complex life cycles, which all talk to each other. I can
kind of see the argument for using TraceLog::GetInstance() but MDM constructs
MTO - it seems so weird for MDM to create MTO but then for MTO to talk back to
MDM via MDM::GetInstance(). Why should MTO care that MDM happens to be a
singleton when it is no harder to pass MDM into MTO since MDM creates MTO? For
me the principal value of using GetInstance is that it means you don't have to
pass tons unrelated things to just to construct an object - but TraceLog and MDM
aren't unrelated to MTO they are the principal objects MTO ought to know about
to do its job. Is there a downside I'm missing?

I would prefer to keep this pattern but lets discuss :)

[Primiano Tucci (use gerrit), 2017/04/21 13:14:16]

I might be overlooking some future plan, but is this something we will ever
cleanup later?
The only reason I see right now to change this, is if we want to test this class
to inject mocks. But as we discussed, there seems to be very little to test here
and the test should probably be end-to-end to be meaningful.
Well IMHO extra fields just increase cognitive load. When I see a field like
memory_dump_manager_ the first thing I ask myself is: "who owns this? what is
its lifetime? There must be a reason why we cache this field".
well, MDM is a (real) singleton so in general should be fine to have something
which has a narrower scope to refer to the owning singleton using GetInstance().
My only driving principle here is simplicity: don't add extra code/variables
unless we need them. It's nothing about performance or memory, just readability
and simplicity.
Honestly this is not something I care too much, so feel free to do what you
think it's better.
It won't be a major game changer in either case and we have other game changer
CLs to work on :)
So whatever works really

+  ~MemoryTracingObserver() override;
+
+  // TraceLog::EnabledStateObserver implementation.
+  void OnTraceLogEnabled() override;
+  void OnTraceLogDisabled() override;
+
+ private:
+  static bool IsMemoryInfraTracingEnabled();

[Primiano Tucci (use gerrit), 2017/04/21 10:10:27]

I think this can be just a method in the anonymouse namespace of the .cc file

[hjd, 2017/04/21 11:51:58]

Done.

+  bool enabled_;

[Primiano Tucci (use gerrit), 2017/04/21 10:10:27]

nit: add a blank line between functions and var declarations

[hjd, 2017/04/21 11:51:58]

Done.

+  MemoryDumpManager* memory_dump_manager_;

[Primiano Tucci (use gerrit), 2017/04/21 10:10:27]

make both const variables (not pointers), as they are initialized only in the
ctor and never touched again, i.e.

MemoryDumpManager* const memory_dump_manager_;
(which is !== const MemoryDumpManager* because C++ syntax is great) 

EDIT: I think we just don't need these at all (see comment above)

[hjd, 2017/04/21 11:51:58]

See comment above,

+  TraceLog* trace_log_;
+};

[Primiano Tucci (use gerrit), 2017/04/21 10:10:27]

DISALLOW_COPY_AND_ASSIGN ( +include base/macros.h)

[hjd, 2017/04/21 11:51:58]

Done.

+
+}  // namespace trace_event
+}  // namespace base
+
+#endif  // BASE_TRACE_EVENT_MEMORY_TRACING_OBSERVER_H_