Componentize //chrome/browser/prefs/tracked.

Componentize //chrome/browser/prefs/tracked.

Componentize //chrome/browser/prefs/tracked and
updated corresponding .gypi files for same.

Moved the files from //chrome/browser/prefs/tracked
to //components/user_prefs/tracked.
Also added new //components/user_prefs/pref_names.* file and added BUILD.gn for same

BUG=507660
Committed: https://crrev.com/94c0b9648ae7c0d72c3c280e014d83866eeef252
Cr-Commit-Position: refs/heads/master@{#342588}

[Jitu( very slow this week), 2015-07-20 11:14:13]

jitendra.ks@samsung.com changed reviewers:
+ vivek.vg@samsung.com, vivekg@chromium.org

[Jitu( very slow this week), 2015-07-20 11:14:14]

PTAL ...

[blundell, 2015-07-22 12:00:38]

blundell@chromium.org changed reviewers:
+ blundell@chromium.org

[blundell, 2015-07-22 12:00:39]

I think prefs_tracker would be a good name for the component. tracked is a
little generic. Let me know when you'd like me to do a reviewing pass.

[Jitu( very slow this week), 2015-07-23 13:43:56]

Dear blundell,

Moved to components/prefs_tracker as per comments.

PTAL...

[blundell, 2015-07-23 13:50:43]

Looks good in general, thanks!

https://codereview.chromium.org/1227973003/diff/40001/chrome/browser/prefs/tr...
File chrome/browser/prefs/tracked/device_id_stub.cc (left):

https://codereview.chromium.org/1227973003/diff/40001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/device_id_stub.cc:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
This file should be moved, right?

https://codereview.chromium.org/1227973003/diff/40001/chrome/browser/prefs/tr...
File chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc
(left):

https://codereview.chromium.org/1227973003/diff/40001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
and this one?

https://codereview.chromium.org/1227973003/diff/40001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/1227973003/diff/40001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:572:
'../components/prefs_tracker/device_id_unittest.cc',
These unit tests should be run as part of components_unittests now, not
unit_tests.

https://codereview.chromium.org/1227973003/diff/40001/components/prefs_tracke...
File components/prefs_tracker.gypi (right):

https://codereview.chromium.org/1227973003/diff/40001/components/prefs_tracke...
components/prefs_tracker.gypi:8: 'target_name': 'prefs_tracker_browser',
s/prefs_tracker_browser/prefs_tracker

https://codereview.chromium.org/1227973003/diff/40001/components/prefs_tracke...
File components/prefs_tracker/pref_names.h (right):

https://codereview.chromium.org/1227973003/diff/40001/components/prefs_tracke...
components/prefs_tracker/pref_names.h:8: namespace prefs {
Change this namespace to prefs_tracker_prefs.

[gab, 2015-07-23 21:24:34]

On 2015/07/23 13:43:56, jitu wrote:
> Dear blundell,
> 
> Moved to components/prefs_tracker as per comments.

Agreed that prefs_tracker is a better name for the component, please update the
description.

Also, I was pretty sure we had added OWNERS of chrome/browser/prefs/tracked but
it looks like we never got around to it (probably because the main owner,
myself, owns c/b/prefs/ already).

Please add components/prefs_tracker/OWNERS with gab@ and bauerb@.

> 
> PTAL...

[gab, 2015-07-23 21:41:15]

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

[gab, 2015-07-23 21:41:16]

Took a quick look before heading home (east coast), curious what's the
motivation behind this move? Other than that, a few quick comments below.

https://codereview.chromium.org/1227973003/diff/40001/chrome/browser/prefs/tr...
File chrome/browser/prefs/tracked/device_id_stub.cc (left):

https://codereview.chromium.org/1227973003/diff/40001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/device_id_stub.cc:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2015/07/23 13:50:43, blundell wrote:
> This file should be moved, right?

Yes.

https://codereview.chromium.org/1227973003/diff/40001/chrome/browser/prefs/tr...
File chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc
(left):

https://codereview.chromium.org/1227973003/diff/40001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
On 2015/07/23 13:50:43, blundell wrote:
> and this one?

Yes, though I assume the dependency on ProfilePrefStoreManager will be
problematic.

https://codereview.chromium.org/1227973003/diff/40001/components/prefs_tracke...
File components/prefs_tracker/pref_hash_filter.cc (right):

https://codereview.chromium.org/1227973003/diff/40001/components/prefs_tracke...
components/prefs_tracker/pref_hash_filter.cc:31: static const char* const
kDeprecatedTrackedPreferences[] = {
These used to move from c/b/prefs/chrome_pref_service_factory.cc to here in
c/b/prefs/tracked/pref_hash_filter.cc upon deprecation, this path is now a lot
more blurry...

https://codereview.chromium.org/1227973003/diff/40001/components/prefs_tracke...
File components/prefs_tracker/tracked_preference_helper.cc (left):

https://codereview.chromium.org/1227973003/diff/40001/components/prefs_tracke...
components/prefs_tracker/tracked_preference_helper.cc:111:
base::LinearHistogram::FactoryGet(
This was easier to read IMO and is how it's written pretty much everywhere I've
seen it in the codebase.

https://codereview.chromium.org/1227973003/diff/40001/components/prefs_tracke...
File components/prefs_tracker/tracked_preferences_migration.cc (left):

https://codereview.chromium.org/1227973003/diff/40001/components/prefs_tracke...
components/prefs_tracker/tracked_preferences_migration.cc:284:
MigratePrefsFromOldToNewStore(protected_pref_names_,
I think this is also easier to read (easier to see the difference between the
two calls than the pack of arguments).

[blundell, 2015-07-23 21:48:39]

The motivation is that this code is to share this code with iOS, which can't use
//chrome
(https://www.chromium.org/developers/design-documents/layered-components-design
for some details if you're interested :).

[blundell, 2015-07-24 08:26:19]

https://codereview.chromium.org/1227973003/diff/40001/components/prefs_tracke...
File components/prefs_tracker/pref_hash_filter.cc (right):

https://codereview.chromium.org/1227973003/diff/40001/components/prefs_tracke...
components/prefs_tracker/pref_hash_filter.cc:31: static const char* const
kDeprecatedTrackedPreferences[] = {
On 2015/07/23 21:41:16, gab wrote:
> These used to move from c/b/prefs/chrome_pref_service_factory.cc to here in
> c/b/prefs/tracked/pref_hash_filter.cc upon deprecation, this path is now a lot
> more blurry...

Could you elaborate a bit? Are you saying more blurry because now that path is
crossing the line from the Chrome embedder into the component?

[blundell, 2015-07-24 08:28:52]

https://codereview.chromium.org/1227973003/diff/40001/chrome/browser/prefs/tr...
File chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc
(left):

https://codereview.chromium.org/1227973003/diff/40001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
On 2015/07/23 21:41:16, gab wrote:
> On 2015/07/23 13:50:43, blundell wrote:
> > and this one?
> 
> Yes, though I assume the dependency on ProfilePrefStoreManager will be
> problematic.

Good catch. That dependence is just to call
ProfilePrefStoreManager::RegisterPrefs(), which in turn just calls
PrefServiceHashStoreContents::RegisterPrefs(). The latter dependence is a
//components-level one, so the indirection through ProfilePrefStoreManager can
presumably just be eliminated here.

[Jitu( very slow this week), 2015-07-24 09:09:23]

Modified as per comments

PTAL

https://codereview.chromium.org/1227973003/diff/40001/chrome/browser/prefs/tr...
File chrome/browser/prefs/tracked/device_id_stub.cc (left):

https://codereview.chromium.org/1227973003/diff/40001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/device_id_stub.cc:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2015/07/23 21:41:16, gab wrote:
> On 2015/07/23 13:50:43, blundell wrote:
> > This file should be moved, right?
> 
> Yes.

Done.

https://codereview.chromium.org/1227973003/diff/40001/chrome/browser/prefs/tr...
File chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc
(left):

https://codereview.chromium.org/1227973003/diff/40001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
On 2015/07/23 21:41:16, gab wrote:
> On 2015/07/23 13:50:43, blundell wrote:
> > and this one?
> 
> Yes, though I assume the dependency on ProfilePrefStoreManager will be
> problematic.

Done.

https://codereview.chromium.org/1227973003/diff/40001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/1227973003/diff/40001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:572:
'../components/prefs_tracker/device_id_unittest.cc',
On 2015/07/23 13:50:43, blundell wrote:
> These unit tests should be run as part of components_unittests now, not
> unit_tests.

mock_validation_delegate is used by profile_pref_store_manager_unittest.cc, so i
kept it here.

Others moved to components_unittests.

https://codereview.chromium.org/1227973003/diff/40001/components/prefs_tracke...
File components/prefs_tracker.gypi (right):

https://codereview.chromium.org/1227973003/diff/40001/components/prefs_tracke...
components/prefs_tracker.gypi:8: 'target_name': 'prefs_tracker_browser',
On 2015/07/23 13:50:43, blundell wrote:
> s/prefs_tracker_browser/prefs_tracker

Done.

https://codereview.chromium.org/1227973003/diff/40001/components/prefs_tracke...
File components/prefs_tracker/pref_hash_filter.cc (right):

https://codereview.chromium.org/1227973003/diff/40001/components/prefs_tracke...
components/prefs_tracker/pref_hash_filter.cc:31: static const char* const
kDeprecatedTrackedPreferences[] = {
On 2015/07/23 21:41:16, gab wrote:
> These used to move from c/b/prefs/chrome_pref_service_factory.cc to here in
> c/b/prefs/tracked/pref_hash_filter.cc upon deprecation, this path is now a lot
> more blurry...

sorry i couldn't get this. Can you please elaborate more on this.

https://codereview.chromium.org/1227973003/diff/40001/components/prefs_tracke...
File components/prefs_tracker/pref_names.h (right):

https://codereview.chromium.org/1227973003/diff/40001/components/prefs_tracke...
components/prefs_tracker/pref_names.h:8: namespace prefs {
On 2015/07/23 13:50:43, blundell wrote:
> Change this namespace to prefs_tracker_prefs.

Done.

https://codereview.chromium.org/1227973003/diff/40001/components/prefs_tracke...
File components/prefs_tracker/tracked_preference_helper.cc (left):

https://codereview.chromium.org/1227973003/diff/40001/components/prefs_tracke...
components/prefs_tracker/tracked_preference_helper.cc:111:
base::LinearHistogram::FactoryGet(
On 2015/07/23 21:41:16, gab wrote:
> This was easier to read IMO and is how it's written pretty much everywhere
I've
> seen it in the codebase.

Done.

As i used git format cl command this piece is automatically changed.

https://codereview.chromium.org/1227973003/diff/40001/components/prefs_tracke...
File components/prefs_tracker/tracked_preferences_migration.cc (left):

https://codereview.chromium.org/1227973003/diff/40001/components/prefs_tracke...
components/prefs_tracker/tracked_preferences_migration.cc:284:
MigratePrefsFromOldToNewStore(protected_pref_names_,
On 2015/07/23 21:41:16, gab wrote:
> I think this is also easier to read (easier to see the difference between the
> two calls than the pack of arguments).

Done.

[blundell, 2015-07-24 09:44:48]

Thanks! Just buildfile changes left for me.

In addition to the comment, you should bring up this target in the GN build,
i.e., add //components/prefs_tracker/BUILD.gn.

https://codereview.chromium.org/1227973003/diff/60001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/1227973003/diff/60001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:571:
'../components/prefs_tracker/mock_validation_delegate.cc',
This file should go into a prefs_tracker_test_support target in
prefs_tracker.gypi, which this target should depend on.

[grt (UTC plus 2), 2015-07-24 14:03:34]

On 2015/07/23 21:48:39, blundell wrote:
> The motivation is that this code is to share this code with iOS, which can't
use
> //chrome
>
(https://www.chromium.org/developers/design-documents/layered-components-design
> for some details if you're interested :).

Is there a need for iOS to have the tracked preferences code?

[blundell, 2015-07-24 14:14:59]

On 2015/07/24 14:03:34, grt wrote:
> On 2015/07/23 21:48:39, blundell wrote:
> > The motivation is that this code is to share this code with iOS, which can't
> use
> > //chrome
> >
>
(https://www.chromium.org/developers/design-documents/layered-components-design
> > for some details if you're interested :).
> 
> Is there a need for iOS to have the tracked preferences code?

Good question. It's built on iOS downstream; I admit that I didn't look to see
whether the building of this code was unnecessary as the componentization looked
so straightforward.

Looking now, kPlatformSupportsPreferenceTracking in
profile_prefs_store_manager.cc is true on iOS.

[grt (UTC plus 2), 2015-07-27 14:55:37]

On 2015/07/24 14:14:59, blundell wrote:
> On 2015/07/24 14:03:34, grt wrote:
> > On 2015/07/23 21:48:39, blundell wrote:
> > > The motivation is that this code is to share this code with iOS, which
can't
> > use
> > > //chrome
> > >
> >
>
(https://www.chromium.org/developers/design-documents/layered-components-design
> > > for some details if you're interested :).
> > 
> > Is there a need for iOS to have the tracked preferences code?
> 
> Good question. It's built on iOS downstream; I admit that I didn't look to see
> whether the building of this code was unnecessary as the componentization
looked
> so straightforward.
> 
> Looking now, kPlatformSupportsPreferenceTracking in
> profile_prefs_store_manager.cc is true on iOS.

gab and erikwright: is this intentional? Does iOS need tracked prefs?

[erikwright (departed), 2015-07-27 15:00:26]

On 2015/07/27 14:55:37, grt wrote:
> On 2015/07/24 14:14:59, blundell wrote:
> > On 2015/07/24 14:03:34, grt wrote:
> > > On 2015/07/23 21:48:39, blundell wrote:
> > > > The motivation is that this code is to share this code with iOS, which
> can't
> > > use
> > > > //chrome
> > > >
> > >
> >
>
(https://www.chromium.org/developers/design-documents/layered-components-design
> > > > for some details if you're interested :).
> > > 
> > > Is there a need for iOS to have the tracked preferences code?
> > 
> > Good question. It's built on iOS downstream; I admit that I didn't look to
see
> > whether the building of this code was unnecessary as the componentization
> looked
> > so straightforward.
> > 
> > Looking now, kPlatformSupportsPreferenceTracking in
> > profile_prefs_store_manager.cc is true on iOS.
> 
> gab and erikwright: is this intentional? Does iOS need tracked prefs?

There's tracked, and then there's protected. Most platforms, IIRC, support
"tracked".

This gives us metrics for preference tampering on those platforms, without
actually implementing any kind of countermeasure. I can't think of a good reason
not to do it.

[grt (UTC plus 2), 2015-07-27 15:01:34]

On 2015/07/27 15:00:26, erikwright wrote:
> On 2015/07/27 14:55:37, grt wrote:
> > On 2015/07/24 14:14:59, blundell wrote:
> > > On 2015/07/24 14:03:34, grt wrote:
> > > > On 2015/07/23 21:48:39, blundell wrote:
> > > > > The motivation is that this code is to share this code with iOS, which
> > can't
> > > > use
> > > > > //chrome
> > > > >
> > > >
> > >
> >
>
(https://www.chromium.org/developers/design-documents/layered-components-design
> > > > > for some details if you're interested :).
> > > > 
> > > > Is there a need for iOS to have the tracked preferences code?
> > > 
> > > Good question. It's built on iOS downstream; I admit that I didn't look to
> see
> > > whether the building of this code was unnecessary as the componentization
> > looked
> > > so straightforward.
> > > 
> > > Looking now, kPlatformSupportsPreferenceTracking in
> > > profile_prefs_store_manager.cc is true on iOS.
> > 
> > gab and erikwright: is this intentional? Does iOS need tracked prefs?
> 
> There's tracked, and then there's protected. Most platforms, IIRC, support
> "tracked".
> 
> This gives us metrics for preference tampering on those platforms, without
> actually implementing any kind of countermeasure. I can't think of a good
reason
> not to do it.

Does the platform's security model makes it impossible for prefs to be modified
outside of Chrome?

[gab, 2015-07-27 15:04:34]

On 2015/07/27 14:55:37, grt wrote:
> On 2015/07/24 14:14:59, blundell wrote:
> > On 2015/07/24 14:03:34, grt wrote:
> > > On 2015/07/23 21:48:39, blundell wrote:
> > > > The motivation is that this code is to share this code with iOS, which
> can't
> > > use
> > > > //chrome
> > > >
> > >
> >
>
(https://www.chromium.org/developers/design-documents/layered-components-design
> > > > for some details if you're interested :).
> > > 
> > > Is there a need for iOS to have the tracked preferences code?
> > 
> > Good question. It's built on iOS downstream; I admit that I didn't look to
see
> > whether the building of this code was unnecessary as the componentization
> looked
> > so straightforward.
> > 
> > Looking now, kPlatformSupportsPreferenceTracking in
> > profile_prefs_store_manager.cc is true on iOS.
> 
> gab and erikwright: is this intentional? Does iOS need tracked prefs?

At first the goal was to support it everywhere, hence it was only disabled on
platforms with failing tests with TODOs.

Now we've realized however preference protection doesn't really make sense on
OSes where the file system is private to the application (i.e. CHROME_OS,
OS_ANDROID, and OS_IOS).

So I think the TODOs should be dropped and OS_IOS should be added to the OSes
that don't support preference tracking (I think the reason this was never added
is that iOS doesn't even compile this code IIUC).

However, I'm guessing the ultimate goal is to migrate prefs to components? If so
you'll still need to migrate c/b/prefs/tracked/ to components as c/b/prefs/
needs it.

Will do another pass once we've resolved whether we should do it or not (in the
mean time I'll put up a CL to add iOS to the unsupported platforms).

Thanks,
Gab

[blundell, 2015-07-27 15:15:59]

On 2015/07/27 15:04:34, gab wrote:
> On 2015/07/27 14:55:37, grt wrote:
> > On 2015/07/24 14:14:59, blundell wrote:
> > > On 2015/07/24 14:03:34, grt wrote:
> > > > On 2015/07/23 21:48:39, blundell wrote:
> > > > > The motivation is that this code is to share this code with iOS, which
> > can't
> > > > use
> > > > > //chrome
> > > > >
> > > >
> > >
> >
>
(https://www.chromium.org/developers/design-documents/layered-components-design
> > > > > for some details if you're interested :).
> > > > 
> > > > Is there a need for iOS to have the tracked preferences code?
> > > 
> > > Good question. It's built on iOS downstream; I admit that I didn't look to
> see
> > > whether the building of this code was unnecessary as the componentization
> > looked
> > > so straightforward.
> > > 
> > > Looking now, kPlatformSupportsPreferenceTracking in
> > > profile_prefs_store_manager.cc is true on iOS.
> > 
> > gab and erikwright: is this intentional? Does iOS need tracked prefs?
> 
> At first the goal was to support it everywhere, hence it was only disabled on
> platforms with failing tests with TODOs.
> 
> Now we've realized however preference protection doesn't really make sense on
> OSes where the file system is private to the application (i.e. CHROME_OS,
> OS_ANDROID, and OS_IOS).
> 
> So I think the TODOs should be dropped and OS_IOS should be added to the OSes
> that don't support preference tracking (I think the reason this was never
added
> is that iOS doesn't even compile this code IIUC).
> 

The code is compiled downstream but not upstream (which is the case for much of
the code iOS builds in //chrome).

> However, I'm guessing the ultimate goal is to migrate prefs to components? If
so
> you'll still need to migrate c/b/prefs/tracked/ to components as c/b/prefs/
> needs it.
>

The ultimate goal is indeed to migrate //chrome/browser/prefs to components
(this quarter).
 
> Will do another pass once we've resolved whether we should do it or not (in
the
> mean time I'll put up a CL to add iOS to the unsupported platforms).
> 
> Thanks,
> Gab

[Jitu( very slow this week), 2015-07-27 15:49:30]

Fixed the review comments and also added BUILD.gn for same.

PTAL...

Thanks

https://codereview.chromium.org/1227973003/diff/60001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/1227973003/diff/60001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:571:
'../components/prefs_tracker/mock_validation_delegate.cc',
On 2015/07/24 09:44:47, blundell wrote:
> This file should go into a prefs_tracker_test_support target in
> prefs_tracker.gypi, which this target should depend on.

Done.

[gab, 2015-07-27 16:47:42]

On 2015/07/27 15:15:59, blundell wrote:
> On 2015/07/27 15:04:34, gab wrote:
> > On 2015/07/27 14:55:37, grt wrote:
> > > On 2015/07/24 14:14:59, blundell wrote:
> > > > On 2015/07/24 14:03:34, grt wrote:
> > > > > On 2015/07/23 21:48:39, blundell wrote:
> > > > > > The motivation is that this code is to share this code with iOS,
which
> > > can't
> > > > > use
> > > > > > //chrome
> > > > > >
> > > > >
> > > >
> > >
> >
>
(https://www.chromium.org/developers/design-documents/layered-components-design
> > > > > > for some details if you're interested :).
> > > > > 
> > > > > Is there a need for iOS to have the tracked preferences code?
> > > > 
> > > > Good question. It's built on iOS downstream; I admit that I didn't look
to
> > see
> > > > whether the building of this code was unnecessary as the
componentization
> > > looked
> > > > so straightforward.
> > > > 
> > > > Looking now, kPlatformSupportsPreferenceTracking in
> > > > profile_prefs_store_manager.cc is true on iOS.
> > > 
> > > gab and erikwright: is this intentional? Does iOS need tracked prefs?
> > 
> > At first the goal was to support it everywhere, hence it was only disabled
on
> > platforms with failing tests with TODOs.
> > 
> > Now we've realized however preference protection doesn't really make sense
on
> > OSes where the file system is private to the application (i.e. CHROME_OS,
> > OS_ANDROID, and OS_IOS).
> > 
> > So I think the TODOs should be dropped and OS_IOS should be added to the
OSes
> > that don't support preference tracking (I think the reason this was never
> added
> > is that iOS doesn't even compile this code IIUC).
> > 
> 
> The code is compiled downstream but not upstream (which is the case for much
of
> the code iOS builds in //chrome).
> 
> > However, I'm guessing the ultimate goal is to migrate prefs to components?
If
> so
> > you'll still need to migrate c/b/prefs/tracked/ to components as c/b/prefs/
> > needs it.
> >
> 
> The ultimate goal is indeed to migrate //chrome/browser/prefs to components
> (this quarter).

Okay thanks for the info, then would it make sense to have
components/prefs/tracked/ for now as a first step and eventually fill up
components/prefs/?

I don't really like having it end up as a top-level component when it mostly
only is a subcomponent of prefs.

>  
> > Will do another pass once we've resolved whether we should do it or not (in
> the
> > mean time I'll put up a CL to add iOS to the unsupported platforms).
> > 
> > Thanks,
> > Gab

[blundell, 2015-07-28 08:59:07]

On 2015/07/27 16:47:42, gab wrote:
> On 2015/07/27 15:15:59, blundell wrote:
> > On 2015/07/27 15:04:34, gab wrote:
> > > On 2015/07/27 14:55:37, grt wrote:
> > > > On 2015/07/24 14:14:59, blundell wrote:
> > > > > On 2015/07/24 14:03:34, grt wrote:
> > > > > > On 2015/07/23 21:48:39, blundell wrote:
> > > > > > > The motivation is that this code is to share this code with iOS,
> which
> > > > can't
> > > > > > use
> > > > > > > //chrome
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
(https://www.chromium.org/developers/design-documents/layered-components-design
> > > > > > > for some details if you're interested :).
> > > > > > 
> > > > > > Is there a need for iOS to have the tracked preferences code?
> > > > > 
> > > > > Good question. It's built on iOS downstream; I admit that I didn't
look
> to
> > > see
> > > > > whether the building of this code was unnecessary as the
> componentization
> > > > looked
> > > > > so straightforward.
> > > > > 
> > > > > Looking now, kPlatformSupportsPreferenceTracking in
> > > > > profile_prefs_store_manager.cc is true on iOS.
> > > > 
> > > > gab and erikwright: is this intentional? Does iOS need tracked prefs?
> > > 
> > > At first the goal was to support it everywhere, hence it was only disabled
> on
> > > platforms with failing tests with TODOs.
> > > 
> > > Now we've realized however preference protection doesn't really make sense
> on
> > > OSes where the file system is private to the application (i.e. CHROME_OS,
> > > OS_ANDROID, and OS_IOS).
> > > 
> > > So I think the TODOs should be dropped and OS_IOS should be added to the
> OSes
> > > that don't support preference tracking (I think the reason this was never
> > added
> > > is that iOS doesn't even compile this code IIUC).
> > > 
> > 
> > The code is compiled downstream but not upstream (which is the case for much
> of
> > the code iOS builds in //chrome).
> > 
> > > However, I'm guessing the ultimate goal is to migrate prefs to components?
> If
> > so
> > > you'll still need to migrate c/b/prefs/tracked/ to components as
c/b/prefs/
> > > needs it.
> > >
> > 
> > The ultimate goal is indeed to migrate //chrome/browser/prefs to components
> > (this quarter).
> 
> Okay thanks for the info, then would it make sense to have
> components/prefs/tracked/ for now as a first step and eventually fill up
> components/prefs/?
> 

Good idea! In fact, //components/user_prefs already exists, and is where we'll
put any further
componentized code from //chrome/browser/prefs. So let's put this code in
//components/user_prefs/tracked.

Jitendra, apologies for the churn!

> I don't really like having it end up as a top-level component when it mostly
> only is a subcomponent of prefs.
> 
> >  
> > > Will do another pass once we've resolved whether we should do it or not
(in
> > the
> > > mean time I'll put up a CL to add iOS to the unsupported platforms).
> > > 
> > > Thanks,
> > > Gab

[Jitu( very slow this week), 2015-07-28 09:04:22]

On 2015/07/28 08:59:07, blundell wrote:
> On 2015/07/27 16:47:42, gab wrote:
> > On 2015/07/27 15:15:59, blundell wrote:
> > > On 2015/07/27 15:04:34, gab wrote:
> > > > On 2015/07/27 14:55:37, grt wrote:
> > > > > On 2015/07/24 14:14:59, blundell wrote:
> > > > > > On 2015/07/24 14:03:34, grt wrote:
> > > > > > > On 2015/07/23 21:48:39, blundell wrote:
> > > > > > > > The motivation is that this code is to share this code with iOS,
> > which
> > > > > can't
> > > > > > > use
> > > > > > > > //chrome
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
(https://www.chromium.org/developers/design-documents/layered-components-design
> > > > > > > > for some details if you're interested :).
> > > > > > > 
> > > > > > > Is there a need for iOS to have the tracked preferences code?
> > > > > > 
> > > > > > Good question. It's built on iOS downstream; I admit that I didn't
> look
> > to
> > > > see
> > > > > > whether the building of this code was unnecessary as the
> > componentization
> > > > > looked
> > > > > > so straightforward.
> > > > > > 
> > > > > > Looking now, kPlatformSupportsPreferenceTracking in
> > > > > > profile_prefs_store_manager.cc is true on iOS.
> > > > > 
> > > > > gab and erikwright: is this intentional? Does iOS need tracked prefs?
> > > > 
> > > > At first the goal was to support it everywhere, hence it was only
disabled
> > on
> > > > platforms with failing tests with TODOs.
> > > > 
> > > > Now we've realized however preference protection doesn't really make
sense
> > on
> > > > OSes where the file system is private to the application (i.e.
CHROME_OS,
> > > > OS_ANDROID, and OS_IOS).
> > > > 
> > > > So I think the TODOs should be dropped and OS_IOS should be added to the
> > OSes
> > > > that don't support preference tracking (I think the reason this was
never
> > > added
> > > > is that iOS doesn't even compile this code IIUC).
> > > > 
> > > 
> > > The code is compiled downstream but not upstream (which is the case for
much
> > of
> > > the code iOS builds in //chrome).
> > > 
> > > > However, I'm guessing the ultimate goal is to migrate prefs to
components?
> > If
> > > so
> > > > you'll still need to migrate c/b/prefs/tracked/ to components as
> c/b/prefs/
> > > > needs it.
> > > >
> > > 
> > > The ultimate goal is indeed to migrate //chrome/browser/prefs to
components
> > > (this quarter).
> > 
> > Okay thanks for the info, then would it make sense to have
> > components/prefs/tracked/ for now as a first step and eventually fill up
> > components/prefs/?
> > 
> 
> Good idea! In fact, //components/user_prefs already exists, and is where we'll
> put any further
> componentized code from //chrome/browser/prefs. So let's put this code in
> //components/user_prefs/tracked.
> 
> Jitendra, apologies for the churn!
> 
> > I don't really like having it end up as a top-level component when it mostly
> > only is a subcomponent of prefs.
> > 
> > >  
> > > > Will do another pass once we've resolved whether we should do it or not
> (in
> > > the
> > > > mean time I'll put up a CL to add iOS to the unsupported platforms).
> > > > 
> > > > Thanks,
> > > > Gab

So as per my understanding now we need to move the code to
//components/user_prefs/tracked, rite ?

[blundell, 2015-07-28 09:12:14]

On 2015/07/28 09:04:22, jitu wrote:
> On 2015/07/28 08:59:07, blundell wrote:
> > On 2015/07/27 16:47:42, gab wrote:
> > > On 2015/07/27 15:15:59, blundell wrote:
> > > > On 2015/07/27 15:04:34, gab wrote:
> > > > > On 2015/07/27 14:55:37, grt wrote:
> > > > > > On 2015/07/24 14:14:59, blundell wrote:
> > > > > > > On 2015/07/24 14:03:34, grt wrote:
> > > > > > > > On 2015/07/23 21:48:39, blundell wrote:
> > > > > > > > > The motivation is that this code is to share this code with
iOS,
> > > which
> > > > > > can't
> > > > > > > > use
> > > > > > > > > //chrome
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
(https://www.chromium.org/developers/design-documents/layered-components-design
> > > > > > > > > for some details if you're interested :).
> > > > > > > > 
> > > > > > > > Is there a need for iOS to have the tracked preferences code?
> > > > > > > 
> > > > > > > Good question. It's built on iOS downstream; I admit that I didn't
> > look
> > > to
> > > > > see
> > > > > > > whether the building of this code was unnecessary as the
> > > componentization
> > > > > > looked
> > > > > > > so straightforward.
> > > > > > > 
> > > > > > > Looking now, kPlatformSupportsPreferenceTracking in
> > > > > > > profile_prefs_store_manager.cc is true on iOS.
> > > > > > 
> > > > > > gab and erikwright: is this intentional? Does iOS need tracked
prefs?
> > > > > 
> > > > > At first the goal was to support it everywhere, hence it was only
> disabled
> > > on
> > > > > platforms with failing tests with TODOs.
> > > > > 
> > > > > Now we've realized however preference protection doesn't really make
> sense
> > > on
> > > > > OSes where the file system is private to the application (i.e.
> CHROME_OS,
> > > > > OS_ANDROID, and OS_IOS).
> > > > > 
> > > > > So I think the TODOs should be dropped and OS_IOS should be added to
the
> > > OSes
> > > > > that don't support preference tracking (I think the reason this was
> never
> > > > added
> > > > > is that iOS doesn't even compile this code IIUC).
> > > > > 
> > > > 
> > > > The code is compiled downstream but not upstream (which is the case for
> much
> > > of
> > > > the code iOS builds in //chrome).
> > > > 
> > > > > However, I'm guessing the ultimate goal is to migrate prefs to
> components?
> > > If
> > > > so
> > > > > you'll still need to migrate c/b/prefs/tracked/ to components as
> > c/b/prefs/
> > > > > needs it.
> > > > >
> > > > 
> > > > The ultimate goal is indeed to migrate //chrome/browser/prefs to
> components
> > > > (this quarter).
> > > 
> > > Okay thanks for the info, then would it make sense to have
> > > components/prefs/tracked/ for now as a first step and eventually fill up
> > > components/prefs/?
> > > 
> > 
> > Good idea! In fact, //components/user_prefs already exists, and is where
we'll
> > put any further
> > componentized code from //chrome/browser/prefs. So let's put this code in
> > //components/user_prefs/tracked.
> > 
> > Jitendra, apologies for the churn!
> > 
> > > I don't really like having it end up as a top-level component when it
mostly
> > > only is a subcomponent of prefs.
> > > 
> > > >  
> > > > > Will do another pass once we've resolved whether we should do it or
not
> > (in
> > > > the
> > > > > mean time I'll put up a CL to add iOS to the unsupported platforms).
> > > > > 
> > > > > Thanks,
> > > > > Gab
> 
> So as per my understanding now we need to move the code to
> //components/user_prefs/tracked, rite ?

That's right.

[Jitu( very slow this week), 2015-07-29 07:03:40]

PTAL...

[blundell, 2015-07-29 09:40:32]

LGTM with changes, thanks!

https://codereview.chromium.org/1227973003/diff/120001/components/components_...
File components/components_tests.gyp (right):

https://codereview.chromium.org/1227973003/diff/120001/components/components_...
components/components_tests.gyp:453: 'tracked_unittest_sources': [
I would just call this user_prefs_unittest_sources. If there are eventually
other user_prefs unittests, they could also go in this list.

https://codereview.chromium.org/1227973003/diff/120001/components/tracked.gypi
File components/tracked.gypi (right):

https://codereview.chromium.org/1227973003/diff/120001/components/tracked.gyp...
components/tracked.gypi:8: 'target_name': 'tracked',
This file doesn't need to exist: these targets should be in
//components/user_prefs.gypi as user_prefs_tracked and
user_prefs_tracked_test_support respectively.

https://codereview.chromium.org/1227973003/diff/120001/components/user_prefs/...
File components/user_prefs/tracked/BUILD.gn (right):

https://codereview.chromium.org/1227973003/diff/120001/components/user_prefs/...
components/user_prefs/tracked/BUILD.gn:5: # GYP version:
components/prefs_tracker.gyp:prefs_tracker
out of date.

https://codereview.chromium.org/1227973003/diff/120001/components/user_prefs/...
File components/user_prefs/tracked/pref_names.h (right):

https://codereview.chromium.org/1227973003/diff/120001/components/user_prefs/...
components/user_prefs/tracked/pref_names.h:8: namespace prefs_tracker_prefs {
This namespace should now be user_prefs_prefs.

[Jitu( very slow this week), 2015-07-29 13:32:00]

Dear blundell,

Changes as suggested.
PTAL...

https://codereview.chromium.org/1227973003/diff/120001/components/components_...
File components/components_tests.gyp (right):

https://codereview.chromium.org/1227973003/diff/120001/components/components_...
components/components_tests.gyp:453: 'tracked_unittest_sources': [
On 2015/07/29 09:40:32, blundell wrote:
> I would just call this user_prefs_unittest_sources. If there are eventually
> other user_prefs unittests, they could also go in this list.

Done.

https://codereview.chromium.org/1227973003/diff/120001/components/tracked.gypi
File components/tracked.gypi (right):

https://codereview.chromium.org/1227973003/diff/120001/components/tracked.gyp...
components/tracked.gypi:8: 'target_name': 'tracked',
On 2015/07/29 09:40:32, blundell wrote:
> This file doesn't need to exist: these targets should be in
> //components/user_prefs.gypi as user_prefs_tracked and
> user_prefs_tracked_test_support respectively.

Done.

https://codereview.chromium.org/1227973003/diff/120001/components/user_prefs/...
File components/user_prefs/tracked/BUILD.gn (right):

https://codereview.chromium.org/1227973003/diff/120001/components/user_prefs/...
components/user_prefs/tracked/BUILD.gn:5: # GYP version:
components/prefs_tracker.gyp:prefs_tracker
On 2015/07/29 09:40:32, blundell wrote:
> out of date.

Removed...

https://codereview.chromium.org/1227973003/diff/120001/components/user_prefs/...
File components/user_prefs/tracked/pref_names.h (right):

https://codereview.chromium.org/1227973003/diff/120001/components/user_prefs/...
components/user_prefs/tracked/pref_names.h:8: namespace prefs_tracker_prefs {
On 2015/07/29 09:40:32, blundell wrote:
> This namespace should now be user_prefs_prefs.

Done.

[blundell, 2015-07-29 14:08:53]

SLGTM

One last nit about GN.

https://codereview.chromium.org/1227973003/diff/140001/components/user_prefs/...
File components/user_prefs/BUILD.gn (right):

https://codereview.chromium.org/1227973003/diff/140001/components/user_prefs/...
components/user_prefs/BUILD.gn:20: static_library("user_prefs_tracked") {
The structure you had with GN was correct in the previous patchset, since the
convention with GN is indeed to place the BUILD.gn file in the directory of the
code that it corresponds to.

[gab, 2015-07-29 18:12:14]

lgtm % comments.

https://codereview.chromium.org/1227973003/diff/140001/components/components_...
File components/components_tests.gyp (right):

https://codereview.chromium.org/1227973003/diff/140001/components/components_...
components/components_tests.gyp:456:
'user_prefs/tracked/mock_validation_delegate.h',
SHouldn't this depend on user_prefs_tracked_test_support instead of re-including
mock_validation_delegate.(cc|h) ?

https://codereview.chromium.org/1227973003/diff/140001/components/user_prefs....
File components/user_prefs.gypi (right):

https://codereview.chromium.org/1227973003/diff/140001/components/user_prefs....
components/user_prefs.gypi:36: 'user_prefs/tracked/device_id_stub.cc',
device_id_stub.cc needs to be excluded on Windows, how is this done here?

https://codereview.chromium.org/1227973003/diff/140001/components/user_prefs/...
File components/user_prefs/tracked/pref_names.h (right):

https://codereview.chromium.org/1227973003/diff/140001/components/user_prefs/...
components/user_prefs/tracked/pref_names.h:8: namespace user_prefs_prefs {
user_prefs_prefs sounds very weird to me... can you think of something better?

https://codereview.chromium.org/1227973003/diff/140001/components/user_prefs/...
File components/user_prefs/tracked/tracked_preference_helper.cc (left):

https://codereview.chromium.org/1227973003/diff/140001/components/user_prefs/...
components/user_prefs/tracked/tracked_preference_helper.cc:116:
base::HistogramBase::kUmaTargetedHistogramFlag);
As mentioned before, please keep this on its own line as everywhere in the
codebase.

[Jitu( very slow this week), 2015-07-30 08:06:13]

Changes done as suggested...

PTAL

https://codereview.chromium.org/1227973003/diff/140001/components/components_...
File components/components_tests.gyp (right):

https://codereview.chromium.org/1227973003/diff/140001/components/components_...
components/components_tests.gyp:456:
'user_prefs/tracked/mock_validation_delegate.h',
On 2015/07/29 18:12:14, gab wrote:
> SHouldn't this depend on user_prefs_tracked_test_support instead of
re-including
> mock_validation_delegate.(cc|h) ?

Nice catch, thanks

Done ...

https://codereview.chromium.org/1227973003/diff/140001/components/user_prefs....
File components/user_prefs.gypi (right):

https://codereview.chromium.org/1227973003/diff/140001/components/user_prefs....
components/user_prefs.gypi:36: 'user_prefs/tracked/device_id_stub.cc',
On 2015/07/29 18:12:14, gab wrote:
> device_id_stub.cc needs to be excluded on Windows, how is this done here?

Done.

https://codereview.chromium.org/1227973003/diff/140001/components/user_prefs/...
File components/user_prefs/BUILD.gn (right):

https://codereview.chromium.org/1227973003/diff/140001/components/user_prefs/...
components/user_prefs/BUILD.gn:20: static_library("user_prefs_tracked") {
On 2015/07/29 14:08:52, blundell wrote:
> The structure you had with GN was correct in the previous patchset, since the
> convention with GN is indeed to place the BUILD.gn file in the directory of
the
> code that it corresponds to.

Ok Done

https://codereview.chromium.org/1227973003/diff/140001/components/user_prefs/...
File components/user_prefs/tracked/pref_names.h (right):

https://codereview.chromium.org/1227973003/diff/140001/components/user_prefs/...
components/user_prefs/tracked/pref_names.h:8: namespace user_prefs_prefs {
On 2015/07/29 18:12:14, gab wrote:
> user_prefs_prefs sounds very weird to me... can you think of something better?

How about 
-- prefs ( simple)
-- user_prefs
-- user_prefs_tracked

https://codereview.chromium.org/1227973003/diff/140001/components/user_prefs/...
File components/user_prefs/tracked/tracked_preference_helper.cc (left):

https://codereview.chromium.org/1227973003/diff/140001/components/user_prefs/...
components/user_prefs/tracked/tracked_preference_helper.cc:116:
base::HistogramBase::kUmaTargetedHistogramFlag);
On 2015/07/29 18:12:14, gab wrote:
> As mentioned before, please keep this on its own line as everywhere in the
> codebase.

Done.

[gab, 2015-07-30 15:38:33]

lgtm w/ 2 last nits

https://codereview.chromium.org/1227973003/diff/140001/components/user_prefs/...
File components/user_prefs/tracked/pref_names.h (right):

https://codereview.chromium.org/1227973003/diff/140001/components/user_prefs/...
components/user_prefs/tracked/pref_names.h:8: namespace user_prefs_prefs {
On 2015/07/30 08:06:13, jitu wrote:
> On 2015/07/29 18:12:14, gab wrote:
> > user_prefs_prefs sounds very weird to me... can you think of something
better?
> 
> How about 
> -- prefs ( simple)
> -- user_prefs
> -- user_prefs_tracked

Looking at the trend @ [1], looks like either just use "prefs" or a "prefs"
subnamespace inside the component's namespace, e.g. user_prefs::prefs::.

I think I prefer the former, simpler form.

[1]
https://code.google.com/p/chromium/codesearch#search/&q=components/.*pref_nam...

https://codereview.chromium.org/1227973003/diff/160001/components/user_prefs....
File components/user_prefs.gypi (right):

https://codereview.chromium.org/1227973003/diff/160001/components/user_prefs....
components/user_prefs.gypi:74: ],
This indentation looks wrong.

[blundell, 2015-07-30 15:40:28]

On 2015/07/30 15:38:33, gab wrote:
> lgtm w/ 2 last nits
> 
>
https://codereview.chromium.org/1227973003/diff/140001/components/user_prefs/...
> File components/user_prefs/tracked/pref_names.h (right):
> 
>
https://codereview.chromium.org/1227973003/diff/140001/components/user_prefs/...
> components/user_prefs/tracked/pref_names.h:8: namespace user_prefs_prefs {
> On 2015/07/30 08:06:13, jitu wrote:
> > On 2015/07/29 18:12:14, gab wrote:
> > > user_prefs_prefs sounds very weird to me... can you think of something
> better?
> > 
> > How about 
> > -- prefs ( simple)
> > -- user_prefs
> > -- user_prefs_tracked
> 
> Looking at the trend @ [1], looks like either just use "prefs" or a "prefs"
> subnamespace inside the component's namespace, e.g. user_prefs::prefs::.
> 
> I think I prefer the former, simpler form.

I think of ::prefs as being reserved for //chrome (and those examples in
components bugs), so would prefer user_prefs::prefs unless you feel strongly
about it.

> 
> [1]
>
https://code.google.com/p/chromium/codesearch#search/&q=components/.*pref_nam...
> 
>
https://codereview.chromium.org/1227973003/diff/160001/components/user_prefs....
> File components/user_prefs.gypi (right):
> 
>
https://codereview.chromium.org/1227973003/diff/160001/components/user_prefs....
> components/user_prefs.gypi:74: ],
> This indentation looks wrong.

[gab, 2015-07-30 19:42:36]

On 2015/07/30 15:40:28, blundell wrote:
> On 2015/07/30 15:38:33, gab wrote:
> > lgtm w/ 2 last nits
> > 
> >
>
https://codereview.chromium.org/1227973003/diff/140001/components/user_prefs/...
> > File components/user_prefs/tracked/pref_names.h (right):
> > 
> >
>
https://codereview.chromium.org/1227973003/diff/140001/components/user_prefs/...
> > components/user_prefs/tracked/pref_names.h:8: namespace user_prefs_prefs {
> > On 2015/07/30 08:06:13, jitu wrote:
> > > On 2015/07/29 18:12:14, gab wrote:
> > > > user_prefs_prefs sounds very weird to me... can you think of something
> > better?
> > > 
> > > How about 
> > > -- prefs ( simple)
> > > -- user_prefs
> > > -- user_prefs_tracked
> > 
> > Looking at the trend @ [1], looks like either just use "prefs" or a "prefs"
> > subnamespace inside the component's namespace, e.g. user_prefs::prefs::.
> > 
> > I think I prefer the former, simpler form.
> 
> I think of ::prefs as being reserved for //chrome (and those examples in
> components bugs), so would prefer user_prefs::prefs unless you feel strongly
> about it.
> 

Okay.

> > 
> > [1]
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=components/.*pref_nam...
> > 
> >
>
https://codereview.chromium.org/1227973003/diff/160001/components/user_prefs....
> > File components/user_prefs.gypi (right):
> > 
> >
>
https://codereview.chromium.org/1227973003/diff/160001/components/user_prefs....
> > components/user_prefs.gypi:74: ],
> > This indentation looks wrong.

[Jitu( very slow this week), 2015-08-03 07:55:13]

Changes done...

PTAL

https://codereview.chromium.org/1227973003/diff/140001/components/user_prefs/...
File components/user_prefs/tracked/pref_names.h (right):

https://codereview.chromium.org/1227973003/diff/140001/components/user_prefs/...
components/user_prefs/tracked/pref_names.h:8: namespace user_prefs_prefs {
On 2015/07/30 15:38:33, gab wrote:
> On 2015/07/30 08:06:13, jitu wrote:
> > On 2015/07/29 18:12:14, gab wrote:
> > > user_prefs_prefs sounds very weird to me... can you think of something
> better?
> > 
> > How about 
> > -- prefs ( simple)
> > -- user_prefs
> > -- user_prefs_tracked
> 
> Looking at the trend @ [1], looks like either just use "prefs" or a "prefs"
> subnamespace inside the component's namespace, e.g. user_prefs::prefs::.
> 
> I think I prefer the former, simpler form.
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#search/&q=components/.*pref_nam...

Done.

https://codereview.chromium.org/1227973003/diff/160001/components/user_prefs....
File components/user_prefs.gypi (right):

https://codereview.chromium.org/1227973003/diff/160001/components/user_prefs....
components/user_prefs.gypi:74: ],
On 2015/07/30 15:38:33, gab wrote:
> This indentation looks wrong.

Done.

[Jitu( very slow this week), 2015-08-03 09:21:49]

On 2015/08/03 07:55:13, jitu wrote:
> Changes done...
> 
> PTAL
> 
>
https://codereview.chromium.org/1227973003/diff/140001/components/user_prefs/...
> File components/user_prefs/tracked/pref_names.h (right):
> 
>
https://codereview.chromium.org/1227973003/diff/140001/components/user_prefs/...
> components/user_prefs/tracked/pref_names.h:8: namespace user_prefs_prefs {
> On 2015/07/30 15:38:33, gab wrote:
> > On 2015/07/30 08:06:13, jitu wrote:
> > > On 2015/07/29 18:12:14, gab wrote:
> > > > user_prefs_prefs sounds very weird to me... can you think of something
> > better?
> > > 
> > > How about 
> > > -- prefs ( simple)
> > > -- user_prefs
> > > -- user_prefs_tracked
> > 
> > Looking at the trend @ [1], looks like either just use "prefs" or a "prefs"
> > subnamespace inside the component's namespace, e.g. user_prefs::prefs::.
> > 
> > I think I prefer the former, simpler form.
> > 
> > [1]
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=components/.*pref_nam...
> 
> Done.
> 
>
https://codereview.chromium.org/1227973003/diff/160001/components/user_prefs....
> File components/user_prefs.gypi (right):
> 
>
https://codereview.chromium.org/1227973003/diff/160001/components/user_prefs....
> components/user_prefs.gypi:74: ],
> On 2015/07/30 15:38:33, gab wrote:
> > This indentation looks wrong.
> 
> Done.

Getting some error while building in trybots
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...

Error
=====
In file included from
../../components/user_prefs/tracked/device_id_unittest.cc:7:
../../testing/gtest/include/gtest/gtest.h:58:10: fatal error:
'gtest/internal/gtest-internal.h' file not found
#include "gtest/internal/gtest-internal.h"

Any idea ??

[blundell, 2015-08-03 09:24:17]

On 2015/08/03 09:21:49, jitu wrote:
> On 2015/08/03 07:55:13, jitu wrote:
> > Changes done...
> > 
> > PTAL
> > 
> >
>
https://codereview.chromium.org/1227973003/diff/140001/components/user_prefs/...
> > File components/user_prefs/tracked/pref_names.h (right):
> > 
> >
>
https://codereview.chromium.org/1227973003/diff/140001/components/user_prefs/...
> > components/user_prefs/tracked/pref_names.h:8: namespace user_prefs_prefs {
> > On 2015/07/30 15:38:33, gab wrote:
> > > On 2015/07/30 08:06:13, jitu wrote:
> > > > On 2015/07/29 18:12:14, gab wrote:
> > > > > user_prefs_prefs sounds very weird to me... can you think of something
> > > better?
> > > > 
> > > > How about 
> > > > -- prefs ( simple)
> > > > -- user_prefs
> > > > -- user_prefs_tracked
> > > 
> > > Looking at the trend @ [1], looks like either just use "prefs" or a
"prefs"
> > > subnamespace inside the component's namespace, e.g. user_prefs::prefs::.
> > > 
> > > I think I prefer the former, simpler form.
> > > 
> > > [1]
> > >
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=components/.*pref_nam...
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1227973003/diff/160001/components/user_prefs....
> > File components/user_prefs.gypi (right):
> > 
> >
>
https://codereview.chromium.org/1227973003/diff/160001/components/user_prefs....
> > components/user_prefs.gypi:74: ],
> > On 2015/07/30 15:38:33, gab wrote:
> > > This indentation looks wrong.
> > 
> > Done.
> 
> Getting some error while building in trybots
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> 
> Error
> =====
> In file included from
> ../../components/user_prefs/tracked/device_id_unittest.cc:7:
> ../../testing/gtest/include/gtest/gtest.h:58:10: fatal error:
> 'gtest/internal/gtest-internal.h' file not found
> #include "gtest/internal/gtest-internal.h"
> 
> Any idea ??

I assume this is a GN bot? You need to add a dependency on //testing/gtest in
//components/user_prefs/tracked:unit_tests.

[Jitu( very slow this week), 2015-08-03 13:19:03]

On 2015/08/03 09:24:17, blundell wrote:
> On 2015/08/03 09:21:49, jitu wrote:
> > On 2015/08/03 07:55:13, jitu wrote:
> > > Changes done...
> > > 
> > > PTAL
> > > 
> > >
> >
>
https://codereview.chromium.org/1227973003/diff/140001/components/user_prefs/...
> > > File components/user_prefs/tracked/pref_names.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1227973003/diff/140001/components/user_prefs/...
> > > components/user_prefs/tracked/pref_names.h:8: namespace user_prefs_prefs {
> > > On 2015/07/30 15:38:33, gab wrote:
> > > > On 2015/07/30 08:06:13, jitu wrote:
> > > > > On 2015/07/29 18:12:14, gab wrote:
> > > > > > user_prefs_prefs sounds very weird to me... can you think of
something
> > > > better?
> > > > > 
> > > > > How about 
> > > > > -- prefs ( simple)
> > > > > -- user_prefs
> > > > > -- user_prefs_tracked
> > > > 
> > > > Looking at the trend @ [1], looks like either just use "prefs" or a
> "prefs"
> > > > subnamespace inside the component's namespace, e.g. user_prefs::prefs::.
> > > > 
> > > > I think I prefer the former, simpler form.
> > > > 
> > > > [1]
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=components/.*pref_nam...
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/1227973003/diff/160001/components/user_prefs....
> > > File components/user_prefs.gypi (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1227973003/diff/160001/components/user_prefs....
> > > components/user_prefs.gypi:74: ],
> > > On 2015/07/30 15:38:33, gab wrote:
> > > > This indentation looks wrong.
> > > 
> > > Done.
> > 
> > Getting some error while building in trybots
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> > 
> > Error
> > =====
> > In file included from
> > ../../components/user_prefs/tracked/device_id_unittest.cc:7:
> > ../../testing/gtest/include/gtest/gtest.h:58:10: fatal error:
> > 'gtest/internal/gtest-internal.h' file not found
> > #include "gtest/internal/gtest-internal.h"
> > 
> > Any idea ??
> 
> I assume this is a GN bot? You need to add a dependency on //testing/gtest in
> //components/user_prefs/tracked:unit_tests.

Thanks it solved :)

[Jitu( very slow this week), 2015-08-03 13:41:30]

jitendra.ks@samsung.com changed reviewers:
+ anthonyvd@chromium.org, mattm@chromium.org

[Jitu( very slow this week), 2015-08-03 13:41:32]

Dear mattm :: Can you please do an OWNERS review of
     --- chrome/browser/safe_browsing/*

Dear anthonyvd :: Can you please do an OWNERS review of
     --- chrome/browser/profiles/*

[anthonyvd, 2015-08-03 13:44:59]

chrome/browser/profiles/ lgtm

[mattm, 2015-08-03 23:12:06]

safe_browsing lgtm

[Jitu( very slow this week), 2015-08-04 07:07:44]

The CQ bit was checked by jitendra.ks@samsung.com

[Jitu( very slow this week), 2015-08-04 07:07:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
blundell@chromium.org, gab@chromium.org
Link to the patchset: https://codereview.chromium.org/1227973003/#ps220001
(title: "fix Build error in GN file")

[commit-bot: I haz the power, 2015-08-04 07:07:49]

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

[Jitu( very slow this week), 2015-08-04 07:08:12]

The CQ bit was unchecked by jitendra.ks@samsung.com

[Jitu( very slow this week), 2015-08-04 07:18:37]

jitendra.ks@samsung.com changed reviewers:
+ bauerb@chromium.org

[Jitu( very slow this week), 2015-08-04 07:18:38]

Dear Lei Zhang,

Can you please do the owner review of below files

--- chrome/browser/BUILD.gn 
--- chrome/browser/DEPS

[Jitu( very slow this week), 2015-08-04 07:20:57]

jitendra.ks@samsung.com changed reviewers:
+ thestig@chromium.org
- bauerb@chromium.org

[Lei Zhang, 2015-08-04 08:26:53]

https://codereview.chromium.org/1227973003/diff/220001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/1227973003/diff/220001/chrome/browser/DEPS#ne...
chrome/browser/DEPS:100: "+components/user_prefs/tracked",
Is this needed? Doesn't the line above cover this?

[Jitu( very slow this week), 2015-08-04 10:44:16]

Dear Lei Zhang,

PTAL ...

https://codereview.chromium.org/1227973003/diff/220001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/1227973003/diff/220001/chrome/browser/DEPS#ne...
chrome/browser/DEPS:100: "+components/user_prefs/tracked",
On 2015/08/04 08:26:53, Lei Zhang wrote:
> Is this needed? Doesn't the line above cover this?

Its not required.
Thanks.

[Lei Zhang, 2015-08-04 16:32:25]

chrome/ lgtm

[Lei Zhang, 2015-08-04 18:24:45]

https://codereview.chromium.org/1227973003/diff/240001/components/user_prefs/...
File components/user_prefs/tracked/BUILD.gn (right):

https://codereview.chromium.org/1227973003/diff/240001/components/user_prefs/...
components/user_prefs/tracked/BUILD.gn:46: "//components/pref_registry",
You probably need to depend on //base here?

[Jitu( very slow this week), 2015-08-05 10:36:40]

Dear Lei Zhang,

Added the dependency in GN build.
PTAL ...

https://codereview.chromium.org/1227973003/diff/240001/components/user_prefs/...
File components/user_prefs/tracked/BUILD.gn (right):

https://codereview.chromium.org/1227973003/diff/240001/components/user_prefs/...
components/user_prefs/tracked/BUILD.gn:46: "//components/pref_registry",
On 2015/08/04 18:24:45, Lei Zhang wrote:
> You probably need to depend on //base here?

Yes, added.

When i build in my local machine i didn't got any error so it was missed.
But it shows in TryBots.

Thanks...

[Jitu( very slow this week), 2015-08-05 11:10:34]

Patchset #15 (id:280001) has been deleted

[Jitu( very slow this week), 2015-08-05 11:53:45]

Patchset #15 (id:300001) has been deleted

[Jitu( very slow this week), 2015-08-05 12:24:23]

Patchset #15 (id:320001) has been deleted

[Jitu( very slow this week), 2015-08-05 12:47:25]

Patchset #15 (id:330001) has been deleted

[Jitu( very slow this week), 2015-08-06 06:49:31]

On 2015/08/05 10:36:40, jitu wrote:
> Dear Lei Zhang,
> 
> Added the dependency in GN build.
> PTAL ...
> 
>
https://codereview.chromium.org/1227973003/diff/240001/components/user_prefs/...
> File components/user_prefs/tracked/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1227973003/diff/240001/components/user_prefs/...
> components/user_prefs/tracked/BUILD.gn:46: "//components/pref_registry",
> On 2015/08/04 18:24:45, Lei Zhang wrote:
> > You probably need to depend on //base here?
> 
> Yes, added.
> 
> When i build in my local machine i didn't got any error so it was missed.
> But it shows in TryBots.
> 
> Thanks...

Getting some error in win_chromium_gn_x64_rel build.

:\b\build\slave\win_gn\build\src\components\user_prefs\tracked\tracked_preference_helper.cc(57)
: error C2220: warning treated as error - no 'object' file generated

Please help me how to check it further!

[blundell, 2015-08-06 09:22:35]

On 2015/08/06 06:49:31, jitu wrote:
> On 2015/08/05 10:36:40, jitu wrote:
> > Dear Lei Zhang,
> > 
> > Added the dependency in GN build.
> > PTAL ...
> > 
> >
>
https://codereview.chromium.org/1227973003/diff/240001/components/user_prefs/...
> > File components/user_prefs/tracked/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/1227973003/diff/240001/components/user_prefs/...
> > components/user_prefs/tracked/BUILD.gn:46: "//components/pref_registry",
> > On 2015/08/04 18:24:45, Lei Zhang wrote:
> > > You probably need to depend on //base here?
> > 
> > Yes, added.
> > 
> > When i build in my local machine i didn't got any error so it was missed.
> > But it shows in TryBots.
> > 
> > Thanks...
> 
> Getting some error in win_chromium_gn_x64_rel build.
> 
>
:\b\build\slave\win_gn\build\src\components\user_prefs\tracked\tracked_preference_helper.cc(57)
> : error C2220: warning treated as error - no 'object' file generated
> 
> Please help me how to check it further!

You need to disable that warning (4267) for this target in GYP and GN. See
//components/url_fixer.gypi and //components/url_fixer/BUILD.gn for examples of
each (search for "TODO(jschuh)" and just add the same lines to your GYP and GN
files for the new target.

[Jitu( very slow this week), 2015-08-06 14:12:25]

On 2015/08/06 09:22:35, blundell wrote:
> On 2015/08/06 06:49:31, jitu wrote:
> > On 2015/08/05 10:36:40, jitu wrote:
> > > Dear Lei Zhang,
> > > 
> > > Added the dependency in GN build.
> > > PTAL ...
> > > 
> > >
> >
>
https://codereview.chromium.org/1227973003/diff/240001/components/user_prefs/...
> > > File components/user_prefs/tracked/BUILD.gn (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1227973003/diff/240001/components/user_prefs/...
> > > components/user_prefs/tracked/BUILD.gn:46: "//components/pref_registry",
> > > On 2015/08/04 18:24:45, Lei Zhang wrote:
> > > > You probably need to depend on //base here?
> > > 
> > > Yes, added.
> > > 
> > > When i build in my local machine i didn't got any error so it was missed.
> > > But it shows in TryBots.
> > > 
> > > Thanks...
> > 
> > Getting some error in win_chromium_gn_x64_rel build.
> > 
> >
>
:\b\build\slave\win_gn\build\src\components\user_prefs\tracked\tracked_preference_helper.cc(57)
> > : error C2220: warning treated as error - no 'object' file generated
> > 
> > Please help me how to check it further!
> 
> You need to disable that warning (4267) for this target in GYP and GN. See
> //components/url_fixer.gypi and //components/url_fixer/BUILD.gn for examples
of
> each (search for "TODO(jschuh)" and just add the same lines to your GYP and GN
> files for the new target.

Thanks it got resolved ...

PTAL weather we can land this patch.

[blundell, 2015-08-07 11:39:36]

go for it with the comment addressed

https://codereview.chromium.org/1227973003/diff/370001/components/user_prefs....
File components/user_prefs.gypi (right):

https://codereview.chromium.org/1227973003/diff/370001/components/user_prefs....
components/user_prefs.gypi:76: # Disable the warning(4267) treated as error
Here and in the gn file, add the same comment as in other places:

"# TODO(jschuh): crbug.com/167187 fix size_t to int truncations."

[Jitu( very slow this week), 2015-08-07 11:47:25]

The CQ bit was checked by jitendra.ks@samsung.com

[Jitu( very slow this week), 2015-08-07 11:47:25]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
anthonyvd@chromium.org, mattm@chromium.org, blundell@chromium.org,
thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1227973003/#ps410001
(title: "added comments")

[commit-bot: I haz the power, 2015-08-07 11:47:42]

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

[commit-bot: I haz the power, 2015-08-07 11:56:05]

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

[commit-bot: I haz the power, 2015-08-07 11:56:07]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Jitu( very slow this week), 2015-08-07 12:04:32]

jitendra.ks@samsung.com changed reviewers:
+ rsleevi@chromium.org

[Jitu( very slow this week), 2015-08-07 12:04:34]

Dear Ryan,
Can you please do the owner review for
--- components/user_prefs/tracked/DEPS

Getting some error like below::
Missing LGTM from OWNERS of dependencies added to DEPS:
    '+crypto/hmac.h',

[Ryan Sleevi, 2015-08-07 20:03:35]

Reviewed the crypto, DEPS LGTM.

One note below about the code design, but *shrug*

https://codereview.chromium.org/1227973003/diff/410001/chrome/browser/prefs/c...
File chrome/browser/prefs/chrome_pref_service_factory.cc (right):

https://codereview.chromium.org/1227973003/diff/410001/chrome/browser/prefs/c...
chrome/browser/prefs/chrome_pref_service_factory.cc:410: seed =
ResourceBundle::GetSharedInstance().GetRawDataResource(
Why put this behind an ifdef? Wouldn't it work just to do this unconditionally,
and leave the resource value as empty for existing bits?

|seed| here becomes the HMAC key for preference modifications (PrefHashStoreImpl
-> PrefHashCalculator -> GetDigestString/VerifyDigestString), and it doesn't
seem this is particularly kept secret (since, as noted, it's readily available
in the resource bundle, and it's kept constant across all builds, it would
seem), but even still, it'd be one less #ifdef in code, since it'd "Just work"
for Chromium builds as it does today, with an empty seed.

[gab, 2015-08-07 20:12:36]

https://codereview.chromium.org/1227973003/diff/410001/chrome/browser/prefs/c...
File chrome/browser/prefs/chrome_pref_service_factory.cc (right):

https://codereview.chromium.org/1227973003/diff/410001/chrome/browser/prefs/c...
chrome/browser/prefs/chrome_pref_service_factory.cc:410: seed =
ResourceBundle::GetSharedInstance().GetRawDataResource(
On 2015/08/07 20:03:34, Ryan Sleevi wrote:
> Why put this behind an ifdef? Wouldn't it work just to do this
unconditionally,
> and leave the resource value as empty for existing bits?
> 
> |seed| here becomes the HMAC key for preference modifications
(PrefHashStoreImpl
> -> PrefHashCalculator -> GetDigestString/VerifyDigestString), and it doesn't
> seem this is particularly kept secret (since, as noted, it's readily available
> in the resource bundle, and it's kept constant across all builds, it would
> seem), but even still, it'd be one less #ifdef in code, since it'd "Just work"
> for Chromium builds as it does today, with an empty seed.

Hey Ryan, I own this code so I'll answer.

That's a very good point, I think you're right that this ifdef is overkill.

I don't think a migration CL is the right place to modify code, but I'll try to
remember to clean it up after this lands (or feel free to send me a CL if I
forget).

[Jitu( very slow this week), 2015-08-08 05:49:29]

The CQ bit was checked by jitendra.ks@samsung.com

[commit-bot: I haz the power, 2015-08-08 05:49:36]

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

[commit-bot: I haz the power, 2015-08-08 05:53:12]

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

[commit-bot: I haz the power, 2015-08-08 05:53:14]

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

[Jitu( very slow this week), 2015-08-10 08:18:56]

The CQ bit was checked by jitendra.ks@samsung.com

[Jitu( very slow this week), 2015-08-10 08:18:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
rsleevi@chromium.org, thestig@chromium.org, gab@chromium.org,
anthonyvd@chromium.org, mattm@chromium.org, blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/1227973003/#ps430001
(title: "Rebased")

[commit-bot: I haz the power, 2015-08-10 08:19:11]

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

[commit-bot: I haz the power, 2015-08-10 08:24:20]

Committed patchset #19 (id:430001)

[commit-bot: I haz the power, 2015-08-10 08:25:03]

Patchset 19 (id:??) landed as
https://crrev.com/94c0b9648ae7c0d72c3c280e014d83866eeef252
Cr-Commit-Position: refs/heads/master@{#342588}