Refactor CallStackProfile::Params into base/ for use over IPC.

Refactor CallStackProfile::Params into base/ for use over IPC.

This refactor is to allow non-browser processes to send metrics profiling
parameters/metadata to browser process over IPC. Struct was originally
defined in components/metrics, now moved to base/metrics for wider use.

BUG=517958

[sydli, 2015-08-31 19:07:25]

sydli@google.com changed reviewers:
+ erikchen@chromium.org, wittman@chromium.org

[sydli, 2015-08-31 19:07:26]

+erikchen and +wittman for review.

[erikchen, 2015-08-31 19:09:15]

https://codereview.chromium.org/1318733007/diff/20002/base/metrics/call_stack...
File base/metrics/call_stack_profile_params.cc (right):

https://codereview.chromium.org/1318733007/diff/20002/base/metrics/call_stack...
base/metrics/call_stack_profile_params.cc:9:
CallStackProfileParams::CallStackProfileParams() {}
don't we need to initialize members?

[Mike Wittman, 2015-08-31 19:35:57]

https://codereview.chromium.org/1318733007/diff/20002/base/metrics/call_stack...
File base/metrics/call_stack_profile_params.cc (right):

https://codereview.chromium.org/1318733007/diff/20002/base/metrics/call_stack...
base/metrics/call_stack_profile_params.cc:9:
CallStackProfileParams::CallStackProfileParams() {}
On 2015/08/31 19:09:15, erikchen wrote:
> don't we need to initialize members?

We do.

https://codereview.chromium.org/1318733007/diff/20002/base/metrics/call_stack...
File base/metrics/call_stack_profile_params.h (right):

https://codereview.chromium.org/1318733007/diff/20002/base/metrics/call_stack...
base/metrics/call_stack_profile_params.h:8: #include <string>
All these headers are unused, except for base/base_export.h.

https://codereview.chromium.org/1318733007/diff/20002/base/metrics/call_stack...
base/metrics/call_stack_profile_params.h:28: CallStackProfileParams();
Is it possible to make this private, and friend the IPC code that needs it? If
not, please add a comment saying not to use it, and move after the other
constructors.

https://codereview.chromium.org/1318733007/diff/20002/chrome/browser/metrics/...
File chrome/browser/metrics/thread_watcher.cc (right):

https://codereview.chromium.org/1318733007/diff/20002/chrome/browser/metrics/...
chrome/browser/metrics/thread_watcher.cc:955: thread_id,
GetJankTimeBombSamplingParams(),
nit: move GetJankTimeBombSamplingParams(), to next line

[sydli, 2015-08-31 20:09:42]

https://codereview.chromium.org/1318733007/diff/20002/base/metrics/call_stack...
File base/metrics/call_stack_profile_params.cc (right):

https://codereview.chromium.org/1318733007/diff/20002/base/metrics/call_stack...
base/metrics/call_stack_profile_params.cc:9:
CallStackProfileParams::CallStackProfileParams() {}
On 2015/08/31 19:35:57, Mike Wittman wrote:
> On 2015/08/31 19:09:15, erikchen wrote:
> > don't we need to initialize members?
> 
> We do.

Done.

https://codereview.chromium.org/1318733007/diff/20002/base/metrics/call_stack...
File base/metrics/call_stack_profile_params.h (right):

https://codereview.chromium.org/1318733007/diff/20002/base/metrics/call_stack...
base/metrics/call_stack_profile_params.h:8: #include <string>
On 2015/08/31 19:35:57, Mike Wittman wrote:
> All these headers are unused, except for base/base_export.h.

Done.

https://codereview.chromium.org/1318733007/diff/20002/base/metrics/call_stack...
base/metrics/call_stack_profile_params.h:28: CallStackProfileParams();
On 2015/08/31 19:35:57, Mike Wittman wrote:
> Is it possible to make this private, and friend the IPC code that needs it? If
> not, please add a comment saying not to use it, and move after the other
> constructors.

Not sure which IPC class uses this -- I'll update the comment. Something similar
is done here:
https://code.google.com/p/chromium/codesearch#chromium/src/base/location.h&sq...

https://codereview.chromium.org/1318733007/diff/20002/chrome/browser/metrics/...
File chrome/browser/metrics/thread_watcher.cc (right):

https://codereview.chromium.org/1318733007/diff/20002/chrome/browser/metrics/...
chrome/browser/metrics/thread_watcher.cc:955: thread_id,
GetJankTimeBombSamplingParams(),
On 2015/08/31 19:35:57, Mike Wittman wrote:
> nit: move GetJankTimeBombSamplingParams(), to next line

Done.

[Mike Wittman, 2015-08-31 20:13:06]

lgtm % nit

https://codereview.chromium.org/1318733007/diff/70001/base/metrics/call_stack...
File base/metrics/call_stack_profile_params.cc (right):

https://codereview.chromium.org/1318733007/diff/70001/base/metrics/call_stack...
base/metrics/call_stack_profile_params.cc:19: :
trigger(base::CallStackProfileParams::UNKNOWN),
nit: use a delegating constructor here, like the first constructor does

[erikchen, 2015-08-31 21:41:41]

lgtm

[sydli, 2015-08-31 22:02:43]

sydli@google.com changed reviewers:
+ asvitkine@chromium.org, thakis@chromium.org

[sydli, 2015-08-31 22:02:44]

Owner review: +asvitkine for metrics/ changes and +thakis for
chrome_browser_main changes.

https://codereview.chromium.org/1318733007/diff/70001/base/metrics/call_stack...
File base/metrics/call_stack_profile_params.cc (right):

https://codereview.chromium.org/1318733007/diff/70001/base/metrics/call_stack...
base/metrics/call_stack_profile_params.cc:19: :
trigger(base::CallStackProfileParams::UNKNOWN),
On 2015/08/31 20:13:06, Mike Wittman wrote:
> nit: use a delegating constructor here, like the first constructor does

Done.

[Alexei Svitkine (slow), 2015-09-01 14:54:33]

https://codereview.chromium.org/1318733007/diff/90001/base/metrics/call_stack...
File base/metrics/call_stack_profile_params.h (right):

https://codereview.chromium.org/1318733007/diff/90001/base/metrics/call_stack...
base/metrics/call_stack_profile_params.h:13: struct BASE_EXPORT
CallStackProfileParams {
Sorry, but to me this seems really out of place in base/, given that no code in
base/ is using it and that it's meaningless on its own.

You mention the motivation for this is to have non-browser processes send this
struct via IPC?

If it's just communication between non-browser process and browser process,
couldn't it live in chrome/common?

[sydli, 2015-09-01 16:39:42]

https://codereview.chromium.org/1318733007/diff/90001/base/metrics/call_stack...
File base/metrics/call_stack_profile_params.h (right):

https://codereview.chromium.org/1318733007/diff/90001/base/metrics/call_stack...
base/metrics/call_stack_profile_params.h:13: struct BASE_EXPORT
CallStackProfileParams {
On 2015/09/01 14:54:33, Alexei Svitkine wrote:
> Sorry, but to me this seems really out of place in base/, given that no code
in
> base/ is using it and that it's meaningless on its own.
> 
> You mention the motivation for this is to have non-browser processes send this
> struct via IPC?
> 
> If it's just communication between non-browser process and browser process,
> couldn't it live in chrome/common?

Some processes we'd like to profile (eg GPU) live in content. Would it be
reasonable to move this to content/common?

[Alexei Svitkine (slow), 2015-09-01 16:52:26]

If content/ is actually using it, it does seem reasonable for it to live in
content.

I guess the other challenge is having the metrics component depend on it.
Generally, we don't want components/metrics to depend on content/, since
content/ isn't available on all platforms (specifically, on iOS).

But if iOS is not planning to use the profiler code, one possibility is to make
a separate target for the profiler code that does depend on content, but that is
not built on iOS.

[Alexei Svitkine (slow), 2015-09-01 16:53:37]

The other possibility is to simply having a different type used for IPC and
have a conversion function. It might be simplest.

On Tue, Sep 1, 2015 at 12:52 PM, <asvitkine@chromium.org> wrote:

> If content/ is actually using it, it does seem reasonable for it to live in
> content.
>
> I guess the other challenge is having the metrics component depend on it.
> Generally, we don't want components/metrics to depend on content/, since
> content/ isn't available on all platforms (specifically, on iOS).
>
> But if iOS is not planning to use the profiler code, one possibility is to
> make
> a separate target for the profiler code that does depend on content, but
> that is
> not built on iOS.
>
> https://codereview.chromium.org/1318733007/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sydli, 2015-09-01 17:27:08]

> The other possibility is to simply having a different type used for IPC and
> have a conversion function. It might be simplest.

Since I'm not sure we want to restrict profiling to non-iOS platforms, this
option is definitely better. However, it will involve duplicating the Trigger
enum and keeping it in sync:
https://code.google.com/p/chromium/codesearch#chromium/src/components/metrics...