Move counters for passwords, history and autofill to components

Move counters for passwords, history and autofill to components

Files moved from c/b/browsing_data/ to components/browsing_data/core/counters:

- autofill_counter.*
- passwords_counter.*
- history_counter.*

BUG=620317
Committed: https://crrev.com/4245d2bac2ab4d5092e83bbb3ac34174654646e2
Cr-Commit-Position: refs/heads/master@{#407462}

[ioanap, 2016-07-15 17:02:42]

The CQ bit was checked by ioanap@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-15 17:03:30]

Dry run: 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-07-15 17:16:40]

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

[commit-bot: I haz the power, 2016-07-15 17:16:41]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ioanap, 2016-07-19 10:01:33]

The CQ bit was checked by ioanap@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 10:01:59]

Dry run: 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-07-19 10:51:10]

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

[commit-bot: I haz the power, 2016-07-19 10:51:11]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[ioanap, 2016-07-19 12:02:15]

The CQ bit was checked by ioanap@google.com to run a CQ dry run

[ioanap, 2016-07-19 12:02:19]

The CQ bit was unchecked by ioanap@google.com

[ioanap, 2016-07-19 15:14:01]

The CQ bit was checked by ioanap@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 15:14:15]

Dry run: 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-07-19 16:05:34]

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

[commit-bot: I haz the power, 2016-07-19 16:05:37]

Dry run: This issue passed the CQ dry run.

[ioanap, 2016-07-20 08:47:25]

Description was changed from

==========
Moved autofill_counter, history_counter and passwords_counter to
components/browsing_data/core/counters/











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

to

==========
Move counters for passwords, history and autofill to components

Files moved from c/b/browsing_data/ to components/browsing_data/core/counters:

- autofill_counter.*
- passwords_counter.*
- history_counter.* 

BUG=620317
==========

[ioanap, 2016-07-20 12:17:54]

ioanap@google.com changed reviewers:
+ msramek@chromium.org

[ioanap, 2016-07-20 12:17:55]

Hi Martin,

Please have a look!

Thanks,
Ioana

[msramek, 2016-07-20 13:41:16]

Looks good, I just left a couple of comments.

https://codereview.chromium.org/2153863002/diff/60001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_counter_factory.cc (right):

https://codereview.chromium.org/2153863002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_counter_factory.cc:78:
history::WebHistoryService*
Can we move this to an anonymous namespace?

https://codereview.chromium.org/2153863002/diff/60001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_counter_factory.h (right):

https://codereview.chromium.org/2153863002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_counter_factory.h:36:
DISALLOW_COPY_AND_ASSIGN(BrowsingDataCounterFactory);
nit: Empty line before the macro.

https://codereview.chromium.org/2153863002/diff/60001/chrome/browser/browsing...
File chrome/browser/browsing_data/history_counter_browsertest.cc (right):

https://codereview.chromium.org/2153863002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/history_counter_browsertest.cc:303:
fake_web_history_service->SetupFakeResponse(true /* success */, net::HTTP_OK);
This renaming seems very unnecessary.

https://codereview.chromium.org/2153863002/diff/60001/components/browsing_dat...
File components/browsing_data/core/counters/autofill_counter.cc (right):

https://codereview.chromium.org/2153863002/diff/60001/components/browsing_dat...
components/browsing_data/core/counters/autofill_counter.cc:39: return
pref_name_;
|pref_name_| is now only used in this method, so we can probably get away with
returning browsing_data::prefs::kDeleteFormData directly.

But that also means that we must return it by value, not by reference. In case
that causes any additional refactoring on the callsites, you can leave it as it
is.

https://codereview.chromium.org/2153863002/diff/60001/components/browsing_dat...
components/browsing_data/core/counters/autofill_counter.cc:113:
std::count_if(credit_cards.begin(), credit_cards.end(),
Is this git cl format? *grumble* *grumble* it's less readable that way.

https://codereview.chromium.org/2153863002/diff/60001/components/browsing_dat...
File components/browsing_data/core/counters/autofill_counter.h (right):

https://codereview.chromium.org/2153863002/diff/60001/components/browsing_dat...
components/browsing_data/core/counters/autofill_counter.h:94: #endif  //
COMPONENTS_BROWSING_DATA_CORE_COUNTERS_AUTOFILL_COUNTER_H_
nit: Empty line.

https://codereview.chromium.org/2153863002/diff/60001/components/browsing_dat...
File components/browsing_data/core/counters/history_counter.cc (right):

https://codereview.chromium.org/2153863002/diff/60001/components/browsing_dat...
components/browsing_data/core/counters/history_counter.cc:22:
ProfileSyncService* profile_sync_service,
nit: The ordering here is strange. In the order of simplicity and relevance to
this counter, I would use:
1. HistoryService
2. WebHistoryService
3. ProfileSyncService

https://codereview.chromium.org/2153863002/diff/60001/components/browsing_dat...
components/browsing_data/core/counters/history_counter.cc:36: if
(profile_sync_service_)
This is an unnecessary change, since we don't need to distinguish between
profile- and other sync services here...

https://codereview.chromium.org/2153863002/diff/60001/components/browsing_dat...
components/browsing_data/core/counters/history_counter.cc:55: return
profile_sync_service_ && profile_sync_service_->IsSyncActive() &&
The semantics here are that history sync is enabled iff a web history service
exists. Of course, this has the same meaning, but it's a very loose coupling. If
the conditions in WebHistoryFactory changed, this might lead to unexpected
behavior.

I think that in the current setup, we should simply test whether or not the
callback returns null.

https://codereview.chromium.org/2153863002/diff/60001/components/browsing_dat...
File components/browsing_data/core/counters/history_counter.h (right):

https://codereview.chromium.org/2153863002/diff/60001/components/browsing_dat...
components/browsing_data/core/counters/history_counter.h:89: }  // namespace
browsing_data
nit: Empty line.

https://codereview.chromium.org/2153863002/diff/60001/components/browsing_dat...
File components/browsing_data/core/counters/passwords_counter.h (right):

https://codereview.chromium.org/2153863002/diff/60001/components/browsing_dat...
components/browsing_data/core/counters/passwords_counter.h:44: }  // namespace
browsing_data
nit: Empty line

[ioanap, 2016-07-20 17:50:37]

Addressed comments.

Thanks,
Ioana

https://codereview.chromium.org/2153863002/diff/60001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_counter_factory.cc (right):

https://codereview.chromium.org/2153863002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_counter_factory.cc:78:
history::WebHistoryService*
On 2016/07/20 13:41:16, msramek wrote:
> Can we move this to an anonymous namespace?

Done.

https://codereview.chromium.org/2153863002/diff/60001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_counter_factory.h (right):

https://codereview.chromium.org/2153863002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_counter_factory.h:36:
DISALLOW_COPY_AND_ASSIGN(BrowsingDataCounterFactory);
On 2016/07/20 13:41:16, msramek wrote:
> nit: Empty line before the macro.

Done.

https://codereview.chromium.org/2153863002/diff/60001/chrome/browser/browsing...
File chrome/browser/browsing_data/history_counter_browsertest.cc (right):

https://codereview.chromium.org/2153863002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/history_counter_browsertest.cc:303:
fake_web_history_service->SetupFakeResponse(true /* success */, net::HTTP_OK);
On 2016/07/20 13:41:16, msramek wrote:
> This renaming seems very unnecessary.

Is web_history_service okay?

https://codereview.chromium.org/2153863002/diff/60001/components/browsing_dat...
File components/browsing_data/core/counters/autofill_counter.cc (right):

https://codereview.chromium.org/2153863002/diff/60001/components/browsing_dat...
components/browsing_data/core/counters/autofill_counter.cc:39: return
pref_name_;
On 2016/07/20 13:41:16, msramek wrote:
> |pref_name_| is now only used in this method, so we can probably get away with
> returning browsing_data::prefs::kDeleteFormData directly.
> 
> But that also means that we must return it by value, not by reference. In case
> that causes any additional refactoring on the callsites, you can leave it as
it
> is.

True. Changed it to return the pref name directly (here and in the other
counters).

Done.

https://codereview.chromium.org/2153863002/diff/60001/components/browsing_dat...
components/browsing_data/core/counters/autofill_counter.cc:113:
std::count_if(credit_cards.begin(), credit_cards.end(),
On 2016/07/20 13:41:16, msramek wrote:
> Is this git cl format? *grumble* *grumble* it's less readable that way.
Oups, I didn't see this one. Yes, it's because of git cl format. Thank you for
spotting it!

https://codereview.chromium.org/2153863002/diff/60001/components/browsing_dat...
File components/browsing_data/core/counters/autofill_counter.h (right):

https://codereview.chromium.org/2153863002/diff/60001/components/browsing_dat...
components/browsing_data/core/counters/autofill_counter.h:94: #endif  //
COMPONENTS_BROWSING_DATA_CORE_COUNTERS_AUTOFILL_COUNTER_H_
On 2016/07/20 13:41:16, msramek wrote:
> nit: Empty line.

Done.

https://codereview.chromium.org/2153863002/diff/60001/components/browsing_dat...
File components/browsing_data/core/counters/history_counter.cc (right):

https://codereview.chromium.org/2153863002/diff/60001/components/browsing_dat...
components/browsing_data/core/counters/history_counter.cc:22:
ProfileSyncService* profile_sync_service,
On 2016/07/20 13:41:16, msramek wrote:
> nit: The ordering here is strange. In the order of simplicity and relevance to
> this counter, I would use:
> 1. HistoryService
> 2. WebHistoryService
> 3. ProfileSyncService

Done.

https://codereview.chromium.org/2153863002/diff/60001/components/browsing_dat...
components/browsing_data/core/counters/history_counter.cc:36: if
(profile_sync_service_)
On 2016/07/20 13:41:16, msramek wrote:
> This is an unnecessary change, since we don't need to distinguish between
> profile- and other sync services here...

Done.

https://codereview.chromium.org/2153863002/diff/60001/components/browsing_dat...
components/browsing_data/core/counters/history_counter.cc:55: return
profile_sync_service_ && profile_sync_service_->IsSyncActive() &&
On 2016/07/20 13:41:16, msramek wrote:
> The semantics here are that history sync is enabled iff a web history service
> exists. Of course, this has the same meaning, but it's a very loose coupling.
If
> the conditions in WebHistoryFactory changed, this might lead to unexpected
> behavior.
> 
> I think that in the current setup, we should simply test whether or not the
> callback returns null.

I see... I added this before I had the callback in place. I changed it now to
check the return value of the callback.

Done.

https://codereview.chromium.org/2153863002/diff/60001/components/browsing_dat...
File components/browsing_data/core/counters/history_counter.h (right):

https://codereview.chromium.org/2153863002/diff/60001/components/browsing_dat...
components/browsing_data/core/counters/history_counter.h:89: }  // namespace
browsing_data
On 2016/07/20 13:41:16, msramek wrote:
> nit: Empty line.

Done.

https://codereview.chromium.org/2153863002/diff/60001/components/browsing_dat...
File components/browsing_data/core/counters/passwords_counter.h (right):

https://codereview.chromium.org/2153863002/diff/60001/components/browsing_dat...
components/browsing_data/core/counters/passwords_counter.h:44: }  // namespace
browsing_data
On 2016/07/20 13:41:16, msramek wrote:
> nit: Empty line

Done.

[msramek, 2016-07-20 18:26:13]

LGTM with a comment and a request.

https://codereview.chromium.org/2153863002/diff/60001/chrome/browser/browsing...
File chrome/browser/browsing_data/history_counter_browsertest.cc (right):

https://codereview.chromium.org/2153863002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/history_counter_browsertest.cc:303:
fake_web_history_service->SetupFakeResponse(true /* success */, net::HTTP_OK);
On 2016/07/20 17:50:36, ioanap wrote:
> On 2016/07/20 13:41:16, msramek wrote:
> > This renaming seems very unnecessary.
> 
> Is web_history_service okay?

Hm. This seems to be a misunderstanding.

What I meant is that renaming this local variable (|service|) is kinda unrelated
to this CL and increases noise. This CL didn't change the meaning of the
variable, so we didn't have to change the name. And there was a recent ToTT
article about not adding unnecessary context to variable names (in this case,
it's clear from the context what kind of service this is). But nevermind - this
is more of a nitpicking now.

More importantly - I liked that you took advantage of the callback function to
pass TestingWebHistoryService in tests. These testing functions are always very
hacky, and that got rid of it in a natural way. I assume that you reverted that
part because of this comment, but that's not at all what I meant - can you
please change that back?

https://codereview.chromium.org/2153863002/diff/90001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_counter_factory.cc (right):

https://codereview.chromium.org/2153863002/diff/90001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_counter_factory.cc:33:
history::WebHistoryService* GetUpdatedWebHistoryService(Profile* profile) {
No offset within namespace. Empty lines between the namespace braces and the
function.

[ioanap, 2016-07-21 11:47:36]

Addressed comments.

Could you please have a look at the modified callback and tell me what you think
?

Thanks,
Ioana

https://codereview.chromium.org/2153863002/diff/60001/chrome/browser/browsing...
File chrome/browser/browsing_data/history_counter_browsertest.cc (right):

https://codereview.chromium.org/2153863002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/history_counter_browsertest.cc:303:
fake_web_history_service->SetupFakeResponse(true /* success */, net::HTTP_OK);
On 2016/07/20 18:26:12, msramek wrote:
> On 2016/07/20 17:50:36, ioanap wrote:
> > On 2016/07/20 13:41:16, msramek wrote:
> > > This renaming seems very unnecessary.
> > 
> > Is web_history_service okay?
> 
> Hm. This seems to be a misunderstanding.
> 
> What I meant is that renaming this local variable (|service|) is kinda
unrelated
> to this CL and increases noise. This CL didn't change the meaning of the
> variable, so we didn't have to change the name. And there was a recent ToTT
> article about not adding unnecessary context to variable names (in this case,
> it's clear from the context what kind of service this is). But nevermind -
this
> is more of a nitpicking now.
> 
> More importantly - I liked that you took advantage of the callback function to
> pass TestingWebHistoryService in tests. These testing functions are always
very
> hacky, and that got rid of it in a natural way. I assume that you reverted
that
> part because of this comment, but that's not at all what I meant - can you
> please change that back?

Changed it back to |service|.

The reason I took out the callback was because checking if the
WebHistoryServiceFactory returns null, instead of "manually" checking if history
sync is enabled led to some weirdness in the tests. The RestartOnSyncChange
test(disabled) operates with the FakeWebHistoryService but relies on the check
within the actual factory, while the Synced test adds synced entries to the fake
service, but was built to bypass the check within the factory. 

I changed the GetFakeWebHistoryService callback to take those considerations
into account.

https://codereview.chromium.org/2153863002/diff/90001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_counter_factory.cc (right):

https://codereview.chromium.org/2153863002/diff/90001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_counter_factory.cc:33:
history::WebHistoryService* GetUpdatedWebHistoryService(Profile* profile) {
On 2016/07/20 18:26:13, msramek wrote:
> No offset within namespace. Empty lines between the namespace braces and the
> function.

Done.

[msramek, 2016-07-21 12:17:11]

I understand what was the problem now, thanks for working around it!

Still LGTM % nits.

https://codereview.chromium.org/2153863002/diff/60001/chrome/browser/browsing...
File chrome/browser/browsing_data/history_counter_browsertest.cc (right):

https://codereview.chromium.org/2153863002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/history_counter_browsertest.cc:303:
fake_web_history_service->SetupFakeResponse(true /* success */, net::HTTP_OK);
On 2016/07/21 11:47:36, ioanap wrote:
> On 2016/07/20 18:26:12, msramek wrote:
> > On 2016/07/20 17:50:36, ioanap wrote:
> > > On 2016/07/20 13:41:16, msramek wrote:
> > > > This renaming seems very unnecessary.
> > > 
> > > Is web_history_service okay?
> > 
> > Hm. This seems to be a misunderstanding.
> > 
> > What I meant is that renaming this local variable (|service|) is kinda
> unrelated
> > to this CL and increases noise. This CL didn't change the meaning of the
> > variable, so we didn't have to change the name. And there was a recent ToTT
> > article about not adding unnecessary context to variable names (in this
case,
> > it's clear from the context what kind of service this is). But nevermind -
> this
> > is more of a nitpicking now.
> > 
> > More importantly - I liked that you took advantage of the callback function
to
> > pass TestingWebHistoryService in tests. These testing functions are always
> very
> > hacky, and that got rid of it in a natural way. I assume that you reverted
> that
> > part because of this comment, but that's not at all what I meant - can you
> > please change that back?
> 
> Changed it back to |service|.
> 
> The reason I took out the callback was because checking if the
> WebHistoryServiceFactory returns null, instead of "manually" checking if
history
> sync is enabled led to some weirdness in the tests. The RestartOnSyncChange
> test(disabled) operates with the FakeWebHistoryService but relies on the check
> within the actual factory, while the Synced test adds synced entries to the
fake
> service, but was built to bypass the check within the factory. 
> 
> I changed the GetFakeWebHistoryService callback to take those considerations
> into account. 

I see it now. Yes, that bypass is a bit weird. One thing that could be done is
to simply initialize the Sync service in this test instead of setting
check_sync_status = false. However, I'm pretty sure that the other test is
disabled because something was flaky in the Sync interaction, so let's rather
not. I like this solution where we hide the details about Sync inside the
callback.

On a related note - can you please check if the last disabled test still passes
locally at your machine? It's really only disabled because of some flakiness,
but it should, in principle, work.

https://codereview.chromium.org/2153863002/diff/110001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_counter_factory.cc (right):

https://codereview.chromium.org/2153863002/diff/110001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_counter_factory.cc:38: } // namespace
nit: two spaces before inline comments

https://codereview.chromium.org/2153863002/diff/110001/chrome/browser/browsin...
File chrome/browser/browsing_data/history_counter_browsertest.cc (right):

https://codereview.chromium.org/2153863002/diff/110001/chrome/browser/browsin...
chrome/browser/browsing_data/history_counter_browsertest.cc:124: bool
check_sync_status) {
nit: alignment

https://codereview.chromium.org/2153863002/diff/110001/chrome/browser/browsin...
chrome/browser/browsing_data/history_counter_browsertest.cc:128: ||
WebHistoryServiceFactory::GetForProfile(profile)) {
style nit: operator || on the previous line

[ioanap, 2016-07-21 13:27:13]

ioanap@google.com changed reviewers:
+ blundell@chromium.org

[ioanap, 2016-07-21 13:27:14]

blundell@chromium.org: Please review changes in 

components/browsing_data.gypi

Thanks,
Ioana

https://codereview.chromium.org/2153863002/diff/110001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_counter_factory.cc (right):

https://codereview.chromium.org/2153863002/diff/110001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_counter_factory.cc:38: } // namespace
On 2016/07/21 12:17:11, msramek wrote:
> nit: two spaces before inline comments

Done.

https://codereview.chromium.org/2153863002/diff/110001/chrome/browser/browsin...
File chrome/browser/browsing_data/history_counter_browsertest.cc (right):

https://codereview.chromium.org/2153863002/diff/110001/chrome/browser/browsin...
chrome/browser/browsing_data/history_counter_browsertest.cc:124: bool
check_sync_status) {
On 2016/07/21 12:17:11, msramek wrote:
> nit: alignment

Done.

https://codereview.chromium.org/2153863002/diff/110001/chrome/browser/browsin...
chrome/browser/browsing_data/history_counter_browsertest.cc:128: ||
WebHistoryServiceFactory::GetForProfile(profile)) {
On 2016/07/21 12:17:11, msramek wrote:
> style nit: operator || on the previous line

Done.

[blundell, 2016-07-21 13:30:40]

I don't think you need me based on the browsing_data line in
//components/OWNERS.

[msramek, 2016-07-21 13:45:41]

Still LGTM.

On 2016/07/21 13:30:40, blundell wrote:
> I don't think you need me based on the browsing_data line in
> //components/OWNERS.

Yeah, that's a problem with the Chromite Butler extension - it doesn't
understand when an OWNERS file contains a reference to another OWNERS file. The
.gypi still shows up red (and the latter OWNERS file is offered in the list of
reviewers).

You can put this to CQ, Ioana :)

[ioanap, 2016-07-21 13:56:11]

Ah, you're right, that's what happened. Sorry!

[ioanap, 2016-07-21 14:27:17]

ioanap@google.com changed reviewers:
+ maxbogue@chromium.org, pkasting@chromium.org, sdefresne@chromium.org,
vabr@chromium.org
- blundell@chromium.org

[ioanap, 2016-07-21 14:27:18]

+vabr for DEPS on:
   - components/autofill/core/browser
   - components/password_manager/core/browser

+maxbogue for DEPS on:
   -  components/browser_sync/browser
   -  components/sync_driver

+pkasting for DEPS on:
  - components/webdata/common

+sdefresne for DEPS on:
  - components/history/core/browser

Thanks,
Ioana

[vabr (Chromium), 2016-07-21 14:31:51]

On 2016/07/21 14:27:18, ioanap wrote:
> +vabr for DEPS on:
>    - components/autofill/core/browser
>    - components/password_manager/core/browser
> 

browsing_data depending on autofill and password_manager LGTM. In particular,
nothing in components seems to depend on browsing_data at the moment, so we are
not introducing a circle, and it is reasonable to expect that neither autofill
nor password_manager would ever need to depend on browsing_data.

Cheers,
Vaclav

[maxbogue, 2016-07-21 16:03:05]

https://codereview.chromium.org/2153863002/diff/130001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_counter_factory.h (right):

https://codereview.chromium.org/2153863002/diff/130001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_counter_factory.h:20: class
WebHistoryService;
Why is this here? I don't see any reference in this file to it.

https://codereview.chromium.org/2153863002/diff/130001/components/browsing_da...
File components/browsing_data/core/counters/history_counter.h (right):

https://codereview.chromium.org/2153863002/diff/130001/components/browsing_da...
components/browsing_data/core/counters/history_counter.h:39: ProfileSyncService*
profile_sync_service);
It looks to me like you can just change this to sync_driver::SyncService and
remove the browser_sync DEP.

[Peter Kasting, 2016-07-21 19:11:48]

LGTM

[ioanap, 2016-07-22 09:15:39]

Patchset #9 (id:150001) has been deleted

[ioanap, 2016-07-22 09:59:25]

Thank you for the reviews!

maxbogue@: I removed the dependency on browser_sync.

https://codereview.chromium.org/2153863002/diff/130001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_counter_factory.h (right):

https://codereview.chromium.org/2153863002/diff/130001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_counter_factory.h:20: class
WebHistoryService;
On 2016/07/21 16:03:05, maxbogue wrote:
> Why is this here? I don't see any reference in this file to it.

It was used before. Removed it now. Thank you!

Done.

https://codereview.chromium.org/2153863002/diff/130001/components/browsing_da...
File components/browsing_data/core/counters/history_counter.h (right):

https://codereview.chromium.org/2153863002/diff/130001/components/browsing_da...
components/browsing_data/core/counters/history_counter.h:39: ProfileSyncService*
profile_sync_service);
On 2016/07/21 16:03:05, maxbogue wrote:
> It looks to me like you can just change this to sync_driver::SyncService and
> remove the browser_sync DEP.

Done.

[maxbogue, 2016-07-22 16:50:01]

lgtm for the sync_driver DEP, thanks!

[ioanap, 2016-07-25 08:53:07]

sdefresne@: friendly ping! :)

(for DEPS on components/history/core/browser)

[sdefresne, 2016-07-25 12:19:14]

lgtm

[ioanap, 2016-07-25 12:23:02]

The CQ bit was checked by ioanap@google.com

[ioanap, 2016-07-25 12:23:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
msramek@chromium.org, vabr@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2153863002/#ps170001
(title: "Addressed comments")

[commit-bot: I haz the power, 2016-07-25 12:23:18]

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-07-25 13:28:25]

Description was changed from

==========
Move counters for passwords, history and autofill to components

Files moved from c/b/browsing_data/ to components/browsing_data/core/counters:

- autofill_counter.*
- passwords_counter.*
- history_counter.* 

BUG=620317
==========

to

==========
Move counters for passwords, history and autofill to components

Files moved from c/b/browsing_data/ to components/browsing_data/core/counters:

- autofill_counter.*
- passwords_counter.*
- history_counter.* 

BUG=620317
==========

[commit-bot: I haz the power, 2016-07-25 13:28:27]

Committed patchset #9 (id:170001)

[commit-bot: I haz the power, 2016-07-25 13:30:13]

Description was changed from

==========
Move counters for passwords, history and autofill to components

Files moved from c/b/browsing_data/ to components/browsing_data/core/counters:

- autofill_counter.*
- passwords_counter.*
- history_counter.* 

BUG=620317
==========

to

==========
Move counters for passwords, history and autofill to components

Files moved from c/b/browsing_data/ to components/browsing_data/core/counters:

- autofill_counter.*
- passwords_counter.*
- history_counter.*

BUG=620317

Committed: https://crrev.com/4245d2bac2ab4d5092e83bbb3ac34174654646e2
Cr-Commit-Position: refs/heads/master@{#407462}
==========

[commit-bot: I haz the power, 2016-07-25 13:30:15]

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