[tracing] Allow enabling heap profiling from chrome://flags

[tracing] Allow enabling heap profiling from chrome://flags

BUG=684839
Review-Url: https://codereview.chromium.org/2847823004
Cr-Commit-Position: refs/heads/master@{#468462}
Committed: https://chromium.googlesource.com/chromium/src/+/c6376a644a8db12cb1deebe497db1f390be8d11e

[DmitrySkiba, 2017-04-28 05:33:15]

dskiba@chromium.org changed reviewers:
+ primiano@chromium.org, wez@chromium.org

[Primiano Tucci (use gerrit), 2017-04-28 10:34:18]

you rock! thanks for this
I am not an owner in anything other than
base/trace_event/memory_dump_manager.cc, which LGTM, but I am supportive of
this, given also the discussion in crbug.com/684839

+erikchen/ajwong FYI

[DmitrySkiba, 2017-04-28 15:50:38]

dskiba@chromium.org changed reviewers:
+ isherman@chromium.org, mark@chromium.org

[DmitrySkiba, 2017-04-28 15:50:39]

mark@: please review base/base_switches.*

isherman@: please review histograms.xml

[Wez, 2017-04-28 17:17:12]

Here, let me pick those nits for you. :P

/me drives-by.

https://codereview.chromium.org/2847823004/diff/1/base/base_switches.h
File base/base_switches.h (right):

https://codereview.chromium.org/2847823004/diff/1/base/base_switches.h#newcode20
base/base_switches.h:20: extern const char kEnableHeapProfilingTaskProfiler[];
Is TaskProfiler not a heap-profiling "mode"?  i.e. should it be
kEnableHeapProfilingModeTaskProfiler?

Or, to shorten the mode names, perhaps call these kHeapProfilingModeFoo since
they are the names of the modes, versus the enable flag itself?

https://codereview.chromium.org/2847823004/diff/1/chrome/browser/flag_descrip...
File chrome/browser/flag_descriptions.cc (right):

https://codereview.chromium.org/2847823004/diff/1/chrome/browser/flag_descrip...
chrome/browser/flag_descriptions.cc:3056: extern const char
kEnableHeapProfilingModePseudo[] = "Enabled (pseudo mode)";
Why are these extern and not the ones above?

https://codereview.chromium.org/2847823004/diff/1/chrome/browser/flag_descrip...
File chrome/browser/flag_descriptions.h (right):

https://codereview.chromium.org/2847823004/diff/1/chrome/browser/flag_descrip...
chrome/browser/flag_descriptions.h:3323: extern const char
kEnableHeapProfilingName[];
nit: Can we have a brief comment on this block to explain these?

https://codereview.chromium.org/2847823004/diff/1/chrome/browser/flag_descrip...
chrome/browser/flag_descriptions.h:3325: extern const char
kEnableHeapProfilingModePseudo[];
See comment re kEnable... naming for the mode strings. These should still end in
Description, though.

[DmitrySkiba, 2017-04-28 19:39:13]

https://codereview.chromium.org/2847823004/diff/1/base/base_switches.h
File base/base_switches.h (right):

https://codereview.chromium.org/2847823004/diff/1/base/base_switches.h#newcode20
base/base_switches.h:20: extern const char kEnableHeapProfilingTaskProfiler[];
On 2017/04/28 17:17:12, Wez wrote:
> Is TaskProfiler not a heap-profiling "mode"?  i.e. should it be
> kEnableHeapProfilingModeTaskProfiler?
> 
> Or, to shorten the mode names, perhaps call these kHeapProfilingModeFoo since
> they are the names of the modes, versus the enable flag itself?

Yeah, I can see your logic here, but I don't want to bloat this CL with
renaming.

https://codereview.chromium.org/2847823004/diff/1/chrome/browser/flag_descrip...
File chrome/browser/flag_descriptions.cc (right):

https://codereview.chromium.org/2847823004/diff/1/chrome/browser/flag_descrip...
chrome/browser/flag_descriptions.cc:3056: extern const char
kEnableHeapProfilingModePseudo[] = "Enabled (pseudo mode)";
On 2017/04/28 17:17:12, Wez wrote:
> Why are these extern and not the ones above?

Good catch, thanks!

https://codereview.chromium.org/2847823004/diff/1/chrome/browser/flag_descrip...
File chrome/browser/flag_descriptions.h (right):

https://codereview.chromium.org/2847823004/diff/1/chrome/browser/flag_descrip...
chrome/browser/flag_descriptions.h:3323: extern const char
kEnableHeapProfilingName[];
On 2017/04/28 17:17:12, Wez wrote:
> nit: Can we have a brief comment on this block to explain these?

Do you really want to add comments like "Name of the enable heap profiling flag"
and "Description of the enable heap profiling flag"? Note that most strings
above were moved from xml, where they were mandatory (I think).

I think that's enough that this file (and the namespace) is named
flag_descriptions.

https://codereview.chromium.org/2847823004/diff/1/chrome/browser/flag_descrip...
chrome/browser/flag_descriptions.h:3325: extern const char
kEnableHeapProfilingModePseudo[];
On 2017/04/28 17:17:12, Wez wrote:
> See comment re kEnable... naming for the mode strings. These should still end
in
> Description, though.

I'm not sure, consider the full name:
flag_descriptions::kEnableHeapProfilingModeNative, so it's a description for
kEnableHeapProfilingModeNative flag.

flag_descriptions::kEnableHeapProfilingModeNativeDescription looks worse IMO.

[Ilya Sherman, 2017-04-28 20:56:01]

histograms.xml lgtm

[Wez, 2017-04-29 01:15:10]

https://codereview.chromium.org/2847823004/diff/1/base/base_switches.h
File base/base_switches.h (right):

https://codereview.chromium.org/2847823004/diff/1/base/base_switches.h#newcode20
base/base_switches.h:20: extern const char kEnableHeapProfilingTaskProfiler[];
On 2017/04/28 19:39:13, DmitrySkiba wrote:
> On 2017/04/28 17:17:12, Wez wrote:
> > Is TaskProfiler not a heap-profiling "mode"?  i.e. should it be
> > kEnableHeapProfilingModeTaskProfiler?
> > 
> > Or, to shorten the mode names, perhaps call these kHeapProfilingModeFoo
since
> > they are the names of the modes, versus the enable flag itself?
> 
> Yeah, I can see your logic here, but I don't want to bloat this CL with
> renaming.

Understood; perhaps we can rename in a separate CL. :)

https://codereview.chromium.org/2847823004/diff/1/chrome/browser/flag_descrip...
File chrome/browser/flag_descriptions.h (right):

https://codereview.chromium.org/2847823004/diff/1/chrome/browser/flag_descrip...
chrome/browser/flag_descriptions.h:3323: extern const char
kEnableHeapProfilingName[];
On 2017/04/28 19:39:13, DmitrySkiba wrote:
> On 2017/04/28 17:17:12, Wez wrote:
> > nit: Can we have a brief comment on this block to explain these?
> 
> Do you really want to add comments like "Name of the enable heap profiling
flag"
> and "Description of the enable heap profiling flag"? Note that most strings
> above were moved from xml, where they were mandatory (I think).
> 
> I think that's enough that this file (and the namespace) is named
> flag_descriptions.

The problem is that at-a-glance these look odd - the first two are actually the
name & description for the --enable-heap-profiling flag, while the rest are the
mode flags.

So a pair of comments like:

// Name & description for the heap profiling flag.
...

// Descriptions of the different heap profiling modes.
...

would help clarify why e.g. the first two end in Name and Desription and the
rest don't.

https://codereview.chromium.org/2847823004/diff/1/chrome/browser/flag_descrip...
chrome/browser/flag_descriptions.h:3325: extern const char
kEnableHeapProfilingModePseudo[];
On 2017/04/28 19:39:13, DmitrySkiba wrote:
> On 2017/04/28 17:17:12, Wez wrote:
> > See comment re kEnable... naming for the mode strings. These should still
end
> in
> > Description, though.
> 
> I'm not sure, consider the full name:
> flag_descriptions::kEnableHeapProfilingModeNative, so it's a description for
> kEnableHeapProfilingModeNative flag.
> 
> flag_descriptions::kEnableHeapProfilingModeNativeDescription looks worse IMO.

Agreed; I was mis-reading the convention based on kEnable...Description[] above.

[DmitrySkiba, 2017-05-01 20:41:01]

On 2017/04/29 01:15:10, Wez wrote:
...
> The problem is that at-a-glance these look odd - the first two are actually
the
> name & description for the --enable-heap-profiling flag, while the rest are
the
> mode flags.
> 
> So a pair of comments like:
> 
> // Name & description for the heap profiling flag.
> ...
> 
> // Descriptions of the different heap profiling modes.
> ...
> 
> would help clarify why e.g. the first two end in Name and Desription and the
> rest don't.

Makes sense, done!

[DmitrySkiba, 2017-05-01 20:41:31]

mark@ PTAL

[Mark Mentovai, 2017-05-01 20:49:34]

LGTM in base

[DmitrySkiba, 2017-05-01 20:50:01]

The CQ bit was checked by dskiba@chromium.org

[DmitrySkiba, 2017-05-01 20:50:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2847823004/#ps40001
(title: "Add flag_descriptions comments")

[commit-bot: I haz the power, 2017-05-01 20:50:31]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-01 20:54:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-01 20:54:59]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[DmitrySkiba, 2017-05-01 21:06:40]

The CQ bit was checked by dskiba@chromium.org

[DmitrySkiba, 2017-05-01 21:06:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, mark@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2847823004/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2017-05-01 21:07:36]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-01 22:43:33]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493672800008310,
"parent_rev": "54d7621cf287e13cc182435ae18ae69742839473", "commit_rev":
"c6376a644a8db12cb1deebe497db1f390be8d11e"}

[commit-bot: I haz the power, 2017-05-01 22:43:40]

Description was changed from

==========
[tracing] Allow enabling heap profiling from chrome://flags

BUG=684839
==========

to

==========
[tracing] Allow enabling heap profiling from chrome://flags

BUG=684839

Review-Url: https://codereview.chromium.org/2847823004
Cr-Commit-Position: refs/heads/master@{#468462}
Committed:
https://chromium.googlesource.com/chromium/src/+/c6376a644a8db12cb1deebe497db...
==========

[commit-bot: I haz the power, 2017-05-01 22:43:41]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/c6376a644a8db12cb1deebe497db...