Stack sampling profiler: move configuration to chrome/common

Stack sampling profiler: move configuration to chrome/common

Anticipates the use of the configuration by ChromeContentGpuClient,
and preserves git history.

BUG=517958
Committed: https://crrev.com/f22f73e46234e405adc99e163e640470fb72395a
Cr-Commit-Position: refs/heads/master@{#421922}

[Mike Wittman, 2016-09-27 17:15:19]

Description was changed from

==========
Stack sampling profiler: move configuration to chrome/common

Anticipates the use of the configuration by ChromeContentGpuClient.

BUG=517958
==========

to

==========
Stack sampling profiler: move configuration to chrome/common

Anticipates the use of the configuration by ChromeContentGpuClient,
and preserves git history.

BUG=517958
==========

[Mike Wittman, 2016-09-27 17:37:51]

The CQ bit was checked by wittman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 17:38:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mike Wittman, 2016-09-27 17:46:37]

wittman@chromium.org changed reviewers:
+ isherman@chromium.org

[Mike Wittman, 2016-09-27 17:46:37]

Hi Ilya, please take a look. This is a prerequisite CL for
https://codereview.chromium.org/2360143006

[commit-bot: I haz the power, 2016-09-27 18:50:51]

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

[commit-bot: I haz the power, 2016-09-27 18:50:52]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2016-09-28 00:48:27]

LGTM % a nit -- thanks.

https://codereview.chromium.org/2375563002/diff/1/chrome/common/stack_samplin...
File chrome/common/stack_sampling_configuration.h (right):

https://codereview.chromium.org/2375563002/diff/1/chrome/common/stack_samplin...
chrome/common/stack_sampling_configuration.h:20: const std::string&
group_name)>;
nit: Please leave a blank newline after this line.

[Mike Wittman, 2016-09-28 19:31:11]

Thanks Ilya!

https://codereview.chromium.org/2375563002/diff/1/chrome/common/stack_samplin...
File chrome/common/stack_sampling_configuration.h (right):

https://codereview.chromium.org/2375563002/diff/1/chrome/common/stack_samplin...
chrome/common/stack_sampling_configuration.h:20: const std::string&
group_name)>;
On 2016/09/28 00:48:27, Ilya Sherman wrote:
> nit: Please leave a blank newline after this line.

Done.

[Mike Wittman, 2016-09-28 19:32:38]

wittman@chromium.org changed reviewers:
+ sky@chromium.org

[Mike Wittman, 2016-09-28 19:32:39]

Scott, please take a look. This is essentially just moving files around and is a
prerequisite CL for
https://codereview.chromium.org/2360143006

[sky, 2016-09-28 20:34:14]

https://codereview.chromium.org/2375563002/diff/20001/chrome/common/stack_sam...
File chrome/common/stack_sampling_configuration.cc (right):

https://codereview.chromium.org/2375563002/diff/20001/chrome/common/stack_sam...
chrome/common/stack_sampling_configuration.cc:79: const
RegisterSyntheticFieldTrialFunction& register_field_trial) const {
How come you need a callback here? Could this instead return a struct with the
two strings?

[Mike Wittman, 2016-09-28 21:51:27]

https://codereview.chromium.org/2375563002/diff/20001/chrome/common/stack_sam...
File chrome/common/stack_sampling_configuration.cc (right):

https://codereview.chromium.org/2375563002/diff/20001/chrome/common/stack_sam...
chrome/common/stack_sampling_configuration.cc:79: const
RegisterSyntheticFieldTrialFunction& register_field_trial) const {
On 2016/09/28 20:34:14, sky wrote:
> How come you need a callback here? Could this instead return a struct with the
> two strings?

The callback is not strictly necessary, but it allows the decisions about both
the field trial parameters and whether to register the field trial to be
encapsulated within the configuration class.

[sky, 2016-09-29 02:48:57]

What is different about this code that warrants a closure, rather than
returning the parameters to the caller? Closures have their place, but
it seems like overkill for a function that could just as well return
the result to the caller and let the caller do what it wants.

  -Scott

On Wed, Sep 28, 2016 at 2:51 PM,  <wittman@chromium.org> wrote:
>
>
https://codereview.chromium.org/2375563002/diff/20001/chrome/common/stack_sam...
> File chrome/common/stack_sampling_configuration.cc (right):
>
>
https://codereview.chromium.org/2375563002/diff/20001/chrome/common/stack_sam...
> chrome/common/stack_sampling_configuration.cc:79: const
> RegisterSyntheticFieldTrialFunction& register_field_trial) const {
> On 2016/09/28 20:34:14, sky wrote:
>> How come you need a callback here? Could this instead return a struct
> with the
>> two strings?
>
> The callback is not strictly necessary, but it allows the decisions
> about both the field trial parameters and whether to register the field
> trial to be encapsulated within the configuration class.
>
> https://codereview.chromium.org/2375563002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mike Wittman, 2016-09-29 16:36:01]

I don't have a strong preference on this, so changed to pass the configuration
to the caller.

On 2016/09/29 02:48:57, sky wrote:
> What is different about this code that warrants a closure, rather than
> returning the parameters to the caller? Closures have their place, but
> it seems like overkill for a function that could just as well return
> the result to the caller and let the caller do what it wants.
> 
>   -Scott
> 
> On Wed, Sep 28, 2016 at 2:51 PM,  <mailto:wittman@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/2375563002/diff/20001/chrome/common/stack_sam...
> > File chrome/common/stack_sampling_configuration.cc (right):
> >
> >
>
https://codereview.chromium.org/2375563002/diff/20001/chrome/common/stack_sam...
> > chrome/common/stack_sampling_configuration.cc:79: const
> > RegisterSyntheticFieldTrialFunction& register_field_trial) const {
> > On 2016/09/28 20:34:14, sky wrote:
> >> How come you need a callback here? Could this instead return a struct
> > with the
> >> two strings?
> >
> > The callback is not strictly necessary, but it allows the decisions
> > about both the field trial parameters and whether to register the field
> > trial to be encapsulated within the configuration class.
> >
> > https://codereview.chromium.org/2375563002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
>

[sky, 2016-09-29 18:21:36]

https://codereview.chromium.org/2375563002/diff/40001/chrome/common/stack_sam...
File chrome/common/stack_sampling_configuration.cc (right):

https://codereview.chromium.org/2375563002/diff/40001/chrome/common/stack_sam...
chrome/common/stack_sampling_configuration.cc:85: *group_name = "";
Why is this needed? Doesn't the switch statement covert it all?

[Mike Wittman, 2016-09-29 18:27:41]

https://codereview.chromium.org/2375563002/diff/40001/chrome/common/stack_sam...
File chrome/common/stack_sampling_configuration.cc (right):

https://codereview.chromium.org/2375563002/diff/40001/chrome/common/stack_sam...
chrome/common/stack_sampling_configuration.cc:85: *group_name = "";
On 2016/09/29 18:21:36, sky wrote:
> Why is this needed? Doesn't the switch statement covert it all?

It does currently. But in the case of future changes I want to explicitly avoid
the possibility of registering a synthetic field trial if the caller happens to
pass a non-empty string to this function.

[sky, 2016-09-29 19:12:50]

LGTM

https://codereview.chromium.org/2375563002/diff/40001/chrome/common/stack_sam...
File chrome/common/stack_sampling_configuration.cc (right):

https://codereview.chromium.org/2375563002/diff/40001/chrome/common/stack_sam...
chrome/common/stack_sampling_configuration.cc:85: *group_name = "";
On 2016/09/29 18:27:40, Mike Wittman wrote:
> On 2016/09/29 18:21:36, sky wrote:
> > Why is this needed? Doesn't the switch statement covert it all?
> 
> It does currently. But in the case of future changes I want to explicitly
avoid
> the possibility of registering a synthetic field trial if the caller happens
to
> pass a non-empty string to this function.

Ok, please convert "" to std::string() then.

[Mike Wittman, 2016-09-29 19:25:55]

https://codereview.chromium.org/2375563002/diff/40001/chrome/common/stack_sam...
File chrome/common/stack_sampling_configuration.cc (right):

https://codereview.chromium.org/2375563002/diff/40001/chrome/common/stack_sam...
chrome/common/stack_sampling_configuration.cc:85: *group_name = "";
On 2016/09/29 19:12:49, sky wrote:
> On 2016/09/29 18:27:40, Mike Wittman wrote:
> > On 2016/09/29 18:21:36, sky wrote:
> > > Why is this needed? Doesn't the switch statement covert it all?
> > 
> > It does currently. But in the case of future changes I want to explicitly
> avoid
> > the possibility of registering a synthetic field trial if the caller happens
> to
> > pass a non-empty string to this function.
> 
> Ok, please convert "" to std::string() then.

Done.

[Mike Wittman, 2016-09-29 19:26:26]

The CQ bit was checked by wittman@chromium.org

[Mike Wittman, 2016-09-29 19:26:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2375563002/#ps60001
(title: "initialize to std::string()")

[commit-bot: I haz the power, 2016-09-29 19:26:53]

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, 2016-09-29 20:30:34]

Description was changed from

==========
Stack sampling profiler: move configuration to chrome/common

Anticipates the use of the configuration by ChromeContentGpuClient,
and preserves git history.

BUG=517958
==========

to

==========
Stack sampling profiler: move configuration to chrome/common

Anticipates the use of the configuration by ChromeContentGpuClient,
and preserves git history.

BUG=517958
==========

[commit-bot: I haz the power, 2016-09-29 20:30:35]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-29 20:33:41]

Description was changed from

==========
Stack sampling profiler: move configuration to chrome/common

Anticipates the use of the configuration by ChromeContentGpuClient,
and preserves git history.

BUG=517958
==========

to

==========
Stack sampling profiler: move configuration to chrome/common

Anticipates the use of the configuration by ChromeContentGpuClient,
and preserves git history.

BUG=517958

Committed: https://crrev.com/f22f73e46234e405adc99e163e640470fb72395a
Cr-Commit-Position: refs/heads/master@{#421922}
==========

[commit-bot: I haz the power, 2016-09-29 20:33:43]

Patchset 4 (id:??) landed as
https://crrev.com/f22f73e46234e405adc99e163e640470fb72395a
Cr-Commit-Position: refs/heads/master@{#421922}