Putting histogram names of tracked prefs in tracked_preference_histogram_name file.

Putting string names of UMA_HISTOGRAM_ENUMERATION at one place to manage them effectively and efficiently.

BUG=568998
Committed: https://crrev.com/13481c8ba871e04264ebfd0c449df6f0518be6fd
Cr-Commit-Position: refs/heads/master@{#365465}

[Deepak, 2015-12-11 12:39:40]

Description was changed from

==========
Using Fallthrough for the cases in switch that return same value in
tracked_preference_helper.cc

BUG=
==========

to

==========
Using Fallthrough for the cases in switch that return same value in
tracked_preference_helper.cc
And putting string names of UMA_HISTOGRAM_ENUMERATION at one place to manage
them effectively.

BUG=568998
==========

[Deepak, 2015-12-11 13:34:17]

deepak.m1@samsung.com changed reviewers:
+ bauerb@chromium.org

[Deepak, 2015-12-11 13:34:17]

PTAL

[Bernhard Bauer, 2015-12-11 14:11:44]

bauerb@chromium.org changed reviewers:
+ gab@chromium.org

[Bernhard Bauer, 2015-12-11 14:11:44]

+Gab, tracked prefs OWNER.

https://codereview.chromium.org/1519043002/diff/30001/components/user_prefs/t...
File components/user_prefs/tracked/pref_names.h (right):

https://codereview.chromium.org/1519043002/diff/30001/components/user_prefs/t...
components/user_prefs/tracked/pref_names.h:14: namespace user_tracked_prefs {
Why not make this an inner namespace user_prefs::tracked? That would directly
correspond to the file name.

https://codereview.chromium.org/1519043002/diff/30001/components/user_prefs/t...
components/user_prefs/tracked/pref_names.h:25: extern const char
kTrackedSplitPreferenceChanged[];
These strings aren't actually pref names, they are histogram names. (They are
histograms _about_ prefs, but not prefs themselves, in contrast to the existing
string above.)

Maybe add these to a new file histogram_names.{cc,h} or constants.{cc,h}?

https://codereview.chromium.org/1519043002/diff/30001/components/user_prefs/t...
components/user_prefs/tracked/pref_names.h:26: }  // namespace user_prefs
Nit: empty line before.

[Deepak, 2015-12-14 03:53:46]

@Dear Bernhard
Thanks for Review.
Changes done as suggested.
PTAL

https://codereview.chromium.org/1519043002/diff/30001/components/user_prefs/t...
File components/user_prefs/tracked/pref_names.h (right):

https://codereview.chromium.org/1519043002/diff/30001/components/user_prefs/t...
components/user_prefs/tracked/pref_names.h:14: namespace user_tracked_prefs {
On 2015/12/11 14:11:44, Bernhard Bauer wrote:
> Why not make this an inner namespace user_prefs::tracked? That would directly
> correspond to the file name.

Now making separate file for these names.

https://codereview.chromium.org/1519043002/diff/30001/components/user_prefs/t...
components/user_prefs/tracked/pref_names.h:25: extern const char
kTrackedSplitPreferenceChanged[];
On 2015/12/11 14:11:44, Bernhard Bauer wrote:
> These strings aren't actually pref names, they are histogram names. (They are
> histograms _about_ prefs, but not prefs themselves, in contrast to the
existing
> string above.)
> 
> Maybe add these to a new file histogram_names.{cc,h} or constants.{cc,h}?

Done.

https://codereview.chromium.org/1519043002/diff/30001/components/user_prefs/t...
components/user_prefs/tracked/pref_names.h:26: }  // namespace user_prefs
On 2015/12/11 14:11:44, Bernhard Bauer wrote:
> Nit: empty line before.

Done.

[Bernhard Bauer, 2015-12-14 09:52:17]

https://codereview.chromium.org/1519043002/diff/70001/components/user_prefs/t...
File components/user_prefs/tracked/tracked_preference_histogram_names.h (right):

https://codereview.chromium.org/1519043002/diff/70001/components/user_prefs/t...
components/user_prefs/tracked/tracked_preference_histogram_names.h:8: namespace
tracked_pref_histogram_names {
I think the best namespace for this would be user_prefs, or user_prefs::tracked.
Either of those is going to be shorter (tracked_pref_histogram_names is very
unwieldy in the files above), they adhere to the namespace naming convention for
components, and inside of that namespace it's probably fine to use these symbols
directly, i.e. not have a namespace just for this file.

[Deepak, 2015-12-14 10:16:18]

@Bernhard
PTAL

https://codereview.chromium.org/1519043002/diff/70001/components/user_prefs/t...
File components/user_prefs/tracked/tracked_preference_histogram_names.h (right):

https://codereview.chromium.org/1519043002/diff/70001/components/user_prefs/t...
components/user_prefs/tracked/tracked_preference_histogram_names.h:8: namespace
tracked_pref_histogram_names {
On 2015/12/14 09:52:16, Bernhard Bauer wrote:
> I think the best namespace for this would be user_prefs, or
user_prefs::tracked.
> Either of those is going to be shorter (tracked_pref_histogram_names is very
> unwieldy in the files above), they adhere to the namespace naming convention
for
> components, and inside of that namespace it's probably fine to use these
symbols
> directly, i.e. not have a namespace just for this file.

I agree, tracked_pref_histogram_names namespace is too big.
I have made this like:
user_prefs::tracked
and keeping the names same.

[Bernhard Bauer, 2015-12-14 17:32:37]

https://codereview.chromium.org/1519043002/diff/90001/components/user_prefs/t...
File components/user_prefs/tracked/tracked_preference_histogram_names.cc
(right):

https://codereview.chromium.org/1519043002/diff/90001/components/user_prefs/t...
components/user_prefs/tracked/tracked_preference_histogram_names.cc:29: }
Same here.

https://codereview.chromium.org/1519043002/diff/90001/components/user_prefs/t...
File components/user_prefs/tracked/tracked_preference_histogram_names.h (right):

https://codereview.chromium.org/1519043002/diff/90001/components/user_prefs/t...
components/user_prefs/tracked/tracked_preference_histogram_names.h:5: #ifndef
COMPONENTS_TRACKED_PREFERENCE_HISTOGRAM_NAMES_H_
This header guard isn't correct. Shouldn't there have been a presubmit check
warning about this? If there wasn't, please check that your checkout is
correctly set up.

https://codereview.chromium.org/1519043002/diff/90001/components/user_prefs/t...
components/user_prefs/tracked/tracked_preference_histogram_names.h:22: }
Empty line before this to match the opening namespace declaration. Also, add a
comment // namespace tracked.

https://codereview.chromium.org/1519043002/diff/90001/components/user_prefs/t...
components/user_prefs/tracked/tracked_preference_histogram_names.h:24: }  //
namespace tracked_pref_histogram_names
namespace user_prefs

[gab, 2015-12-14 19:29:27]

Is there a global effort to extract histogram names to their own files? First
time I see this paradigm for histograms and the bug doesn't link to any such
effort..?

As for the fall-through, see my comment but I think it helps readability not to
do it in this case (and I doubt it hurts anything else, the compiler may even
generate the same code).

https://codereview.chromium.org/1519043002/diff/90001/components/user_prefs/t...
File components/user_prefs/tracked/tracked_preference_helper.cc (left):

https://codereview.chromium.org/1519043002/diff/90001/components/user_prefs/t...
components/user_prefs/tracked/tracked_preference_helper.cc:35: return
DONT_RESET;
I preferred this format as it groups things that are the same thing (i.e. some
ValueState map to the same ResetAction but for different reasons, splitting them
here I think helped readability).

Pretty sure this doesn't impact performance and IMO it helps readability so I'd
say keep it.

https://codereview.chromium.org/1519043002/diff/90001/components/user_prefs/t...
File components/user_prefs/tracked/tracked_preference_histogram_names.h (right):

https://codereview.chromium.org/1519043002/diff/90001/components/user_prefs/t...
components/user_prefs/tracked/tracked_preference_histogram_names.h:9: 
no empty line between consecutive nested namespaces (same at bottom when closing
and in .cc)

[Deepak, 2015-12-15 04:37:09]

Description was changed from

==========
Using Fallthrough for the cases in switch that return same value in
tracked_preference_helper.cc
And putting string names of UMA_HISTOGRAM_ENUMERATION at one place to manage
them effectively.

BUG=568998
==========

to

==========
Putting string names of UMA_HISTOGRAM_ENUMERATION at one place to manage them
effectively and efficiently.

BUG=568998
==========

[Deepak, 2015-12-15 04:39:36]

@Bernhard and @gab
Thanks for review. Review comments addressed.
PTAL

https://codereview.chromium.org/1519043002/diff/90001/components/user_prefs/t...
File components/user_prefs/tracked/tracked_preference_helper.cc (left):

https://codereview.chromium.org/1519043002/diff/90001/components/user_prefs/t...
components/user_prefs/tracked/tracked_preference_helper.cc:35: return
DONT_RESET;
On 2015/12/14 19:29:27, gab wrote:
> I preferred this format as it groups things that are the same thing (i.e. some
> ValueState map to the same ResetAction but for different reasons, splitting
them
> here I think helped readability).
> 
> Pretty sure this doesn't impact performance and IMO it helps readability so
I'd
> say keep it.

Done.

https://codereview.chromium.org/1519043002/diff/90001/components/user_prefs/t...
File components/user_prefs/tracked/tracked_preference_histogram_names.cc
(right):

https://codereview.chromium.org/1519043002/diff/90001/components/user_prefs/t...
components/user_prefs/tracked/tracked_preference_histogram_names.cc:29: }
On 2015/12/14 17:32:36, Bernhard Bauer wrote:
> Same here.

Done.

https://codereview.chromium.org/1519043002/diff/90001/components/user_prefs/t...
File components/user_prefs/tracked/tracked_preference_histogram_names.h (right):

https://codereview.chromium.org/1519043002/diff/90001/components/user_prefs/t...
components/user_prefs/tracked/tracked_preference_histogram_names.h:5: #ifndef
COMPONENTS_TRACKED_PREFERENCE_HISTOGRAM_NAMES_H_
On 2015/12/14 17:32:36, Bernhard Bauer wrote:
> This header guard isn't correct. Shouldn't there have been a presubmit check
> warning about this? If there wasn't, please check that your checkout is
> correctly set up.

Done.

https://codereview.chromium.org/1519043002/diff/90001/components/user_prefs/t...
components/user_prefs/tracked/tracked_preference_histogram_names.h:9: 
On 2015/12/14 19:29:27, gab wrote:
> no empty line between consecutive nested namespaces (same at bottom when
closing
> and in .cc)

Done.

https://codereview.chromium.org/1519043002/diff/90001/components/user_prefs/t...
components/user_prefs/tracked/tracked_preference_histogram_names.h:22: }
On 2015/12/14 17:32:37, Bernhard Bauer wrote:
> Empty line before this to match the opening namespace declaration. Also, add a
> comment // namespace tracked.

Done.

https://codereview.chromium.org/1519043002/diff/90001/components/user_prefs/t...
components/user_prefs/tracked/tracked_preference_histogram_names.h:24: }  //
namespace tracked_pref_histogram_names
On 2015/12/14 17:32:37, Bernhard Bauer wrote:
> namespace user_prefs

Done.

[Bernhard Bauer, 2015-12-15 11:19:24]

On 2015/12/14 19:29:27, gab wrote:
> Is there a global effort to extract histogram names to their own files? First
> time I see this paradigm for histograms and the bug doesn't link to any such
> effort..?

Usually we only use a given histogram in a single place, so there isn't really a
need.

[Bernhard Bauer, 2015-12-15 11:19:55]

https://codereview.chromium.org/1519043002/diff/110001/components/user_prefs/...
File components/user_prefs/tracked/tracked_preference_histogram_names.h (right):

https://codereview.chromium.org/1519043002/diff/110001/components/user_prefs/...
components/user_prefs/tracked/tracked_preference_histogram_names.h:22: }  //
namespace tracked.
No period at the end. Essentially, the comment should exactly mirror the
statement the opened the namespace.

[Deepak, 2015-12-15 11:28:28]

@Bernhard
PTAL

https://codereview.chromium.org/1519043002/diff/110001/components/user_prefs/...
File components/user_prefs/tracked/tracked_preference_histogram_names.h (right):

https://codereview.chromium.org/1519043002/diff/110001/components/user_prefs/...
components/user_prefs/tracked/tracked_preference_histogram_names.h:22: }  //
namespace tracked.
On 2015/12/15 11:19:55, Bernhard Bauer wrote:
> No period at the end. Essentially, the comment should exactly mirror the
> statement the opened the namespace.

Done. Sorry I missed that.

[gab, 2015-12-15 16:56:17]

On 2015/12/15 11:19:24, Bernhard Bauer wrote:
> On 2015/12/14 19:29:27, gab wrote:
> > Is there a global effort to extract histogram names to their own files?
First
> > time I see this paradigm for histograms and the bug doesn't link to any such
> > effort..?
> 
> Usually we only use a given histogram in a single place, so there isn't really
a
> need.

Okay, in this case they're used in browser tests as well. There are multiple
places in the codebase that do this duplication in tests and don't use constants
for it, but I don't see a reason not to so patch set 8 lgtm (reworded bug
accordingly).

[Bernhard Bauer, 2015-12-15 16:58:29]

lgtm

[Deepak, 2015-12-16 03:55:49]

On 2015/12/15 16:58:29, Bernhard Bauer wrote:
> lgtm

Thanks Bernhard & Gab.

[Deepak, 2015-12-16 03:55:56]

The CQ bit was checked by deepak.m1@samsung.com

[commit-bot: I haz the power, 2015-12-16 03:57:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1519043002/130001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1519043002/130001

[commit-bot: I haz the power, 2015-12-16 04:12:08]

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

[commit-bot: I haz the power, 2015-12-16 04:12:09]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[Deepak, 2015-12-16 04:53:47]

The CQ bit was checked by deepak.m1@samsung.com

[Deepak, 2015-12-16 04:53:48]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
gab@chromium.org
Link to the patchset: https://codereview.chromium.org/1519043002/#ps150001
(title: "Fixing New line error for mac.")

[commit-bot: I haz the power, 2015-12-16 04:56:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1519043002/150001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1519043002/150001

[commit-bot: I haz the power, 2015-12-16 05:21:49]

Description was changed from

==========
Putting string names of UMA_HISTOGRAM_ENUMERATION at one place to manage them
effectively and efficiently.

BUG=568998
==========

to

==========
Putting string names of UMA_HISTOGRAM_ENUMERATION at one place to manage them
effectively and efficiently.

BUG=568998
==========

[commit-bot: I haz the power, 2015-12-16 05:21:50]

Committed patchset #9 (id:150001)

[commit-bot: I haz the power, 2015-12-16 05:22:29]

Description was changed from

==========
Putting string names of UMA_HISTOGRAM_ENUMERATION at one place to manage them
effectively and efficiently.

BUG=568998
==========

to

==========
Putting string names of UMA_HISTOGRAM_ENUMERATION at one place to manage them
effectively and efficiently.

BUG=568998

Committed: https://crrev.com/13481c8ba871e04264ebfd0c449df6f0518be6fd
Cr-Commit-Position: refs/heads/master@{#365465}
==========

[commit-bot: I haz the power, 2015-12-16 05:22:30]

Patchset 9 (id:??) landed as
https://crrev.com/13481c8ba871e04264ebfd0c449df6f0518be6fd
Cr-Commit-Position: refs/heads/master@{#365465}