Moved BrowsingDataCounter and part of BrowsingDataCounterUtils to components.

Moved BrowsingDataCounter and part of BrowsingDataCounterUtils to components.

Changes:
   - BrowsingDataCounter now resides in components/browsing_data/counters/
   - Moved enums and TimePeriod utils to components/browsing_data/browsing_data_utils[.h/.cc]
   - Changes to the counters that still reside in chrome/browser/browsing_data/ are temporary, meant to allow this partial change.
     They will be moved to components in a subsequent CL

TBR=dbeam@chromium.org
BUG=620317
Committed: https://crrev.com/c776d1065425025fdcded9885505ea4637158929
Cr-Commit-Position: refs/heads/master@{#403882}

[ioanap, 2016-06-21 12:04:18]

Description was changed from

==========
Moved BrowsingDataCounter and part of BrowsingDataCounterUtils to components.

Changes:
   - BrowsingDataCounter now resides in components/browsing_data/counters/
   - Moved enums and TimePeriod utils to
components/browsing_data/browsing_data_utils[.h/.cc]
   - Changes to the counters that still reside in chrome/browser/browsing_data/
are temporary, meant to allow this partial change.
     They will be moved to components in a subsequent CL

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

to

==========
Moved BrowsingDataCounter and part of BrowsingDataCounterUtils to components.

Changes:
   - BrowsingDataCounter now resides in components/browsing_data/counters/
   - Moved enums and TimePeriod utils to
components/browsing_data/browsing_data_utils[.h/.cc]
   - Changes to the counters that still reside in chrome/browser/browsing_data/
are temporary, meant to allow this partial change.
     They will be moved to components in a subsequent CL

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

[ioanap, 2016-06-21 12:04:18]

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

[ioanap, 2016-06-21 12:06:10]

Hi,

Please have a look!

Thank you,
Ioana

[ioanap, 2016-06-21 12:06:36]

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

[commit-bot: I haz the power, 2016-06-21 12:07:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2084903002/20001

[commit-bot: I haz the power, 2016-06-21 12:10:33]

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

[commit-bot: I haz the power, 2016-06-21 12:10:35]

Dry run: 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_...)

[ioanap, 2016-06-21 12:57:58]

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

[commit-bot: I haz the power, 2016-06-21 12:58:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2084903002/40001

[commit-bot: I haz the power, 2016-06-21 13:12:04]

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

[commit-bot: I haz the power, 2016-06-21 13:12:06]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[ioanap, 2016-06-21 14:05:34]

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

[commit-bot: I haz the power, 2016-06-21 14:05:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2084903002/60001

[commit-bot: I haz the power, 2016-06-21 14:20:08]

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

[commit-bot: I haz the power, 2016-06-21 14:20:09]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[msramek, 2016-06-21 14:43:56]

Looks generally good :)

I left the first round of comments (in Patchset 3 which was the last when I
started reading). As discussed offline, the most important change is to
introduce namespaces.

https://codereview.chromium.org/2084903002/diff/40001/chrome/browser/browsing...
File chrome/browser/browsing_data/passwords_counter.h (right):

https://codereview.chromium.org/2084903002/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/passwords_counter.h:8: #include
"chrome/browser/profiles/profile.h"
As mentioned elsewhere, Profile should be forward-declared here and included in
the corresponding .cc file.

Similarly in *counter.h files.

https://codereview.chromium.org/2084903002/diff/40001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/2084903002/diff/40001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:4127: 'source_file':
'components/browsing_data/browsing_data_utils.h',
This is the same file as below, so we don't need two targets. Can we join them
to e.g. 'browsing_data_utils_java'?

Also, this references files that do not belong to //chrome/browser, so we should
move it to the component's build file.

See, 'autocomplete_match_type_java' for an example how to.

https://codereview.chromium.org/2084903002/diff/40001/components/browsing_dat...
File components/browsing_data/browsing_data_utils.cc (right):

https://codereview.chromium.org/2084903002/diff/40001/components/browsing_dat...
components/browsing_data/browsing_data_utils.cc:8: using
base::UserMetricsAction;
This is typically discouraged. Please inline base::UserMetricsAction in this
file.

https://codereview.chromium.org/2084903002/diff/40001/components/browsing_dat...
File components/browsing_data/counters/browsing_data_counter.h (right):

https://codereview.chromium.org/2084903002/diff/40001/components/browsing_dat...
components/browsing_data/counters/browsing_data_counter.h:1: // Copyright (c)
2016 The Chromium Authors. All rights reserved.
I think you can keep the year, as the class was written then. But you can remove
the "(c)", since we no longer use that.

https://codereview.chromium.org/2084903002/diff/40001/components/browsing_dat...
components/browsing_data/counters/browsing_data_counter.h:14: #include
"components/prefs/pref_service.h"
Prefer forward declarations before includes in .h files. That means:

class PrefService;

here, and then

#include "pref_service.h"

in the .cc file.

https://codereview.chromium.org/2084903002/diff/40001/components/browsing_dat...
components/browsing_data/counters/browsing_data_counter.h:15: 
In //chrome/browser, we usually don't use namespaces, but in //components, we
always use the name of the component. Please wrap these declarations in:


namespace browsing_data {
}  // namespace browsing_data

https://codereview.chromium.org/2084903002/diff/40001/components/browsing_dat...
components/browsing_data/counters/browsing_data_counter.h:63: const std::string&
delete_time_period_pref,
This should ideally also be defined in the component, so we don't have to pass
it here, e.g. in //components/browsing_data/pref_names.[cc|h].

See e.g.
https://cs.chromium.org/chromium/src/components/content_settings/core/common/...

[ioanap, 2016-06-22 14:44:57]

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

[commit-bot: I haz the power, 2016-06-22 14:45:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2084903002/80001

[commit-bot: I haz the power, 2016-06-22 14:47:33]

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

[commit-bot: I haz the power, 2016-06-22 14:47:34]

Dry run: Try jobs failed on following builders:
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[ioanap, 2016-06-22 16:47:04]

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

[commit-bot: I haz the power, 2016-06-22 16:47:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2084903002/100001

[commit-bot: I haz the power, 2016-06-22 16:53:23]

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

[commit-bot: I haz the power, 2016-06-22 16:53:25]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[ioanap, 2016-06-23 14:56:28]

I made the necessary changes.

Thank you,
Ioana

https://codereview.chromium.org/2084903002/diff/40001/chrome/browser/browsing...
File chrome/browser/browsing_data/passwords_counter.h (right):

https://codereview.chromium.org/2084903002/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/passwords_counter.h:8: #include
"chrome/browser/profiles/profile.h"
On 2016/06/21 14:43:56, msramek wrote:
> As mentioned elsewhere, Profile should be forward-declared here and included
in
> the corresponding .cc file.
> 
> Similarly in *counter.h files.

Done.

https://codereview.chromium.org/2084903002/diff/40001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/2084903002/diff/40001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:4127: 'source_file':
'components/browsing_data/browsing_data_utils.h',
On 2016/06/21 14:43:56, msramek wrote:
> This is the same file as below, so we don't need two targets. Can we join them
> to e.g. 'browsing_data_utils_java'?
> 
> Also, this references files that do not belong to //chrome/browser, so we
should
> move it to the component's build file.
> 
> See, 'autocomplete_match_type_java' for an example how to.

Done.

https://codereview.chromium.org/2084903002/diff/40001/components/browsing_dat...
File components/browsing_data/browsing_data_utils.cc (right):

https://codereview.chromium.org/2084903002/diff/40001/components/browsing_dat...
components/browsing_data/browsing_data_utils.cc:8: using
base::UserMetricsAction;
On 2016/06/21 14:43:56, msramek wrote:
> This is typically discouraged. Please inline base::UserMetricsAction in this
> file.

Noted. I removed it completely, since the method that was using it is now back
in BrowsingDataRemover.

https://codereview.chromium.org/2084903002/diff/40001/components/browsing_dat...
File components/browsing_data/counters/browsing_data_counter.h (right):

https://codereview.chromium.org/2084903002/diff/40001/components/browsing_dat...
components/browsing_data/counters/browsing_data_counter.h:1: // Copyright (c)
2016 The Chromium Authors. All rights reserved.
On 2016/06/21 14:43:56, msramek wrote:
> I think you can keep the year, as the class was written then. But you can
remove
> the "(c)", since we no longer use that.

Done.

https://codereview.chromium.org/2084903002/diff/40001/components/browsing_dat...
components/browsing_data/counters/browsing_data_counter.h:14: #include
"components/prefs/pref_service.h"
On 2016/06/21 14:43:56, msramek wrote:
> Prefer forward declarations before includes in .h files. That means:
> 
> class PrefService;
> 
> here, and then
> 
> #include "pref_service.h"
> 
> in the .cc file.

Done.

https://codereview.chromium.org/2084903002/diff/40001/components/browsing_dat...
components/browsing_data/counters/browsing_data_counter.h:15: 
On 2016/06/21 14:43:56, msramek wrote:
> In //chrome/browser, we usually don't use namespaces, but in //components, we
> always use the name of the component. Please wrap these declarations in:
> 
> 
> namespace browsing_data {
> }  // namespace browsing_data

Done.

https://codereview.chromium.org/2084903002/diff/40001/components/browsing_dat...
components/browsing_data/counters/browsing_data_counter.h:63: const std::string&
delete_time_period_pref,
On 2016/06/21 14:43:56, msramek wrote:
> This should ideally also be defined in the component, so we don't have to pass
> it here, e.g. in //components/browsing_data/pref_names.[cc|h].
> 
> See e.g.
>
https://cs.chromium.org/chromium/src/components/content_settings/core/common/...

Done.

[ioanap, 2016-06-23 14:56:34]

I made the necessary changes.

Thank you,
Ioana

[ioanap, 2016-06-23 14:56:45]

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

[commit-bot: I haz the power, 2016-06-23 14:56:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2084903002/220001

[commit-bot: I haz the power, 2016-06-23 16:08:37]

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

[commit-bot: I haz the power, 2016-06-23 16:08:38]

Dry run: This issue passed the CQ dry run.

[msramek, 2016-06-27 12:46:19]

This is looking quite good now. I left a few comments.

Please rebase this to update the callsites in the MD UI, and I'll take the final
look :)

https://codereview.chromium.org/2084903002/diff/220001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_counter_utils.cc (right):

https://codereview.chromium.org/2084903002/diff/220001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_counter_utils.cc:62: text =
l10n_util::GetStringUTF16(IDS_CLEAR_BROWSING_DATA_CALCULATING);
Can you please re-add the empty line below? I had it there to help readability
(empty line before each "else if").

https://codereview.chromium.org/2084903002/diff/220001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_counter_utils_unittest.cc
(right):

https://codereview.chromium.org/2084903002/diff/220001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_counter_utils_unittest.cc:43: //
std::unique_ptr<Profile> profile_(new TestingProfile());
Please remove the debugging comment.

https://codereview.chromium.org/2084903002/diff/220001/chrome/browser/browsin...
File chrome/browser/browsing_data/downloads_counter.h (right):

https://codereview.chromium.org/2084903002/diff/220001/chrome/browser/browsin...
chrome/browser/browsing_data/downloads_counter.h:10: class Profile;
nit: Empty line below this one.

https://codereview.chromium.org/2084903002/diff/220001/chrome/browser/browsin...
chrome/browser/browsing_data/downloads_counter.h:19: Profile* profile_;
style nit: attributes under methods

This is already wrong in several counters, where methods are under attributes
(that seems to be one mistake that I copy-pasted everywhere). We don't have to
fix that in this CL, but let's at least not mix the two.

https://codereview.chromium.org/2084903002/diff/220001/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/2084903002/diff/220001/chrome/chrome_browser....
chrome/chrome_browser.gypi:4147: 'source_file':
'components/browsing_data/browsing_data_utils.h',
Please remove this. You have already defined it in
components/browsing_data.gypi, and it doesn't belong here.

https://codereview.chromium.org/2084903002/diff/220001/components/browsing_da...
File components/browsing_data/pref_names.h (right):

https://codereview.chromium.org/2084903002/diff/220001/components/browsing_da...
components/browsing_data/pref_names.h:5: #ifndef
COMPONENTS_BROWSING_DATA_PREF_NAMES_H
Please add "_" here and to the next line.

[ioanap, 2016-06-27 14:19:13]

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

[commit-bot: I haz the power, 2016-06-27 14:19:30]

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

[ioanap, 2016-06-27 14:50:43]

Done with rebasing and with the 2nd round of comments!

Thank you!

https://codereview.chromium.org/2084903002/diff/220001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_counter_utils.cc (right):

https://codereview.chromium.org/2084903002/diff/220001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_counter_utils.cc:62: text =
l10n_util::GetStringUTF16(IDS_CLEAR_BROWSING_DATA_CALCULATING);
On 2016/06/27 12:46:18, msramek wrote:
> Can you please re-add the empty line below? I had it there to help readability
> (empty line before each "else if").

Done.

https://codereview.chromium.org/2084903002/diff/220001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_counter_utils_unittest.cc
(right):

https://codereview.chromium.org/2084903002/diff/220001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_counter_utils_unittest.cc:43: //
std::unique_ptr<Profile> profile_(new TestingProfile());
On 2016/06/27 12:46:18, msramek wrote:
> Please remove the debugging comment.

Done.

https://codereview.chromium.org/2084903002/diff/220001/chrome/browser/browsin...
File chrome/browser/browsing_data/downloads_counter.h (right):

https://codereview.chromium.org/2084903002/diff/220001/chrome/browser/browsin...
chrome/browser/browsing_data/downloads_counter.h:10: class Profile;
On 2016/06/27 12:46:18, msramek wrote:
> nit: Empty line below this one.

Done.

https://codereview.chromium.org/2084903002/diff/220001/chrome/browser/browsin...
chrome/browser/browsing_data/downloads_counter.h:19: Profile* profile_;
On 2016/06/27 12:46:18, msramek wrote:
> style nit: attributes under methods
> 
> This is already wrong in several counters, where methods are under attributes
> (that seems to be one mistake that I copy-pasted everywhere). We don't have to
> fix that in this CL, but let's at least not mix the two.

Done.

https://codereview.chromium.org/2084903002/diff/220001/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/2084903002/diff/220001/chrome/chrome_browser....
chrome/chrome_browser.gypi:4147: 'source_file':
'components/browsing_data/browsing_data_utils.h',
On 2016/06/27 12:46:18, msramek wrote:
> Please remove this. You have already defined it in
> components/browsing_data.gypi, and it doesn't belong here.

Thank you for catching this!

Done.

https://codereview.chromium.org/2084903002/diff/220001/components/browsing_da...
File components/browsing_data/pref_names.h (right):

https://codereview.chromium.org/2084903002/diff/220001/components/browsing_da...
components/browsing_data/pref_names.h:5: #ifndef
COMPONENTS_BROWSING_DATA_PREF_NAMES_H
On 2016/06/27 12:46:18, msramek wrote:
> Please add "_" here and to the next line.

Done.

[msramek, 2016-06-27 14:59:03]

Thanks Ioana! I think this is in a good shape now. LGTM.

[msramek, 2016-06-27 15:01:14]

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

[msramek, 2016-06-27 15:01:15]

+Bernhard, please have a look at chrome/browser/android and
chrome/browser/extensions/api.

But feel free to sanity check browsing_data/ as well, since this is a large
change. (For context: Ioana is going to implement the counters for iOS, so we're
componentizing).

[commit-bot: I haz the power, 2016-06-27 15:28:34]

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

[commit-bot: I haz the power, 2016-06-27 15:28:35]

Dry run: This issue passed the CQ dry run.

[ioanap, 2016-06-27 15:32:07]

ioanap@google.com changed reviewers:
+ dbeam@chromium.org, jochen@chromium.org

[ioanap, 2016-06-27 15:32:09]

dbeam@chromium.org: Please review changes in 

chrome/browser/ui/webui/

Thank you,
Ioana

[ioanap, 2016-06-27 15:32:11]

dbeam@chromium.org: Please review changes in 

chrome/browser/ui/webui/

Thank you,
Ioana

[ioanap, 2016-06-27 15:36:04]

Hi Jochen,

Could you please review

chrome/chrome.gyp

and some small changes in:

chrome/browser/net/sdch_browsertest.cc
chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc
chrome/browser/ui/browser_ui_prefs.cc

Thank you,
Ioana

[Bernhard Bauer, 2016-06-27 17:09:46]

Looks pretty good. I found some preexisting issues, but as it turns out, Martin
is already a on this review, so hey! :-D

https://codereview.chromium.org/2084903002/diff/260001/chrome/browser/android...
File chrome/browser/android/browsing_data/browsing_data_counter_bridge.cc
(right):

https://codereview.chromium.org/2084903002/diff/260001/chrome/browser/android...
chrome/browser/android/browsing_data/browsing_data_counter_bridge.cc:28:
ProfileManager::GetActiveUserProfile()->GetOriginalProfile();
I know you just moved this from existing code, but is the GetOriginalProfile()
necessary? Do we every try to clear browsing data for an incognito profile?

https://codereview.chromium.org/2084903002/diff/260001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_counter_utils_unittest.cc
(right):

https://codereview.chromium.org/2084903002/diff/260001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_counter_utils_unittest.cc:31:
base::MessageLoop::current()->RunUntilIdle();
Is there a particular reason why this is necessary? (I don't think it's unlikely
that it is, but it would be at least worth checking).

In any case, use RunLoop to run the current message loop, not MessageLoop
directly.

https://codereview.chromium.org/2084903002/diff/260001/chrome/browser/browsin...
File chrome/browser/browsing_data/passwords_counter.cc (right):

https://codereview.chromium.org/2084903002/diff/260001/chrome/browser/browsin...
chrome/browser/browsing_data/passwords_counter.cc:20: store_ =
PasswordStoreFactory::GetForProfile(
Also preexisting code, but: PasswordStoreFactory::GetForProfile() returns a
scoped_refptr, so storing a raw pointer is pretty ick.

https://codereview.chromium.org/2084903002/diff/260001/chrome/browser/browsin...
chrome/browser/browsing_data/passwords_counter.cc:26: LOG(ERROR) << "No password
store! Cannot count passwords.";
This is also a bit strange... Under which circumstances would this happen? If
it's something like only in a testing profile, that's perfectly legitimate and a
LOG(ERROR) is just going to produce spam. If we _think_ it can't happen, it's
better to DCHECK it anyway, so we'd crash if the assumption turns out to be
wrong.

[jochen (gone - plz use gerrit), 2016-06-28 13:16:29]

will rubberstamp once the main reviewers approved

https://codereview.chromium.org/2084903002/diff/280001/components/browsing_da...
File components/browsing_data/DEPS (right):

https://codereview.chromium.org/2084903002/diff/280001/components/browsing_da...
components/browsing_data/DEPS:3: "+content/public/browser",
if you plan to use this component on ios, you'll need to get rid of this dep

[msramek, 2016-06-28 14:06:19]

https://codereview.chromium.org/2084903002/diff/260001/chrome/browser/android...
File chrome/browser/android/browsing_data/browsing_data_counter_bridge.cc
(right):

https://codereview.chromium.org/2084903002/diff/260001/chrome/browser/android...
chrome/browser/android/browsing_data/browsing_data_counter_bridge.cc:28:
ProfileManager::GetActiveUserProfile()->GetOriginalProfile();
On 2016/06/27 17:09:45, Bernhard Bauer wrote:
> I know you just moved this from existing code, but is the GetOriginalProfile()
> necessary? Do we every try to clear browsing data for an incognito profile?

I'll respond, since this is my code :)

I would expect that it's not necessary, and that on Android
GetActiveUserProfile() will actually be the original profile. However, I
extracted this code from pref_service_bridge.cc which also diligently uses
GetOriginalProfile() wherever it needs a profile, so I wasn't sure about it.

I would prefer to play it safe and keep it, but if you're convinced it can never
be off the record, I'm fine with adding a DCHECK instead.

[ioanap, 2016-06-28 14:12:36]

Thank you for reviewing this!

https://codereview.chromium.org/2084903002/diff/260001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_counter_utils_unittest.cc
(right):

https://codereview.chromium.org/2084903002/diff/260001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_counter_utils_unittest.cc:31:
base::MessageLoop::current()->RunUntilIdle();
On 2016/06/27 17:09:45, Bernhard Bauer wrote:
> Is there a particular reason why this is necessary? (I don't think it's
unlikely
> that it is, but it would be at least worth checking).
> 
> In any case, use RunLoop to run the current message loop, not MessageLoop
> directly.


It would seem like it is not necessary, indeed.

Done.

https://codereview.chromium.org/2084903002/diff/260001/chrome/browser/browsin...
File chrome/browser/browsing_data/passwords_counter.cc (right):

https://codereview.chromium.org/2084903002/diff/260001/chrome/browser/browsin...
chrome/browser/browsing_data/passwords_counter.cc:20: store_ =
PasswordStoreFactory::GetForProfile(
On 2016/06/27 17:09:45, Bernhard Bauer wrote:
> Also preexisting code, but: PasswordStoreFactory::GetForProfile() returns a
> scoped_refptr, so storing a raw pointer is pretty ick.

Done.

https://codereview.chromium.org/2084903002/diff/260001/chrome/browser/browsin...
chrome/browser/browsing_data/passwords_counter.cc:26: LOG(ERROR) << "No password
store! Cannot count passwords.";
On 2016/06/27 17:09:45, Bernhard Bauer wrote:
> This is also a bit strange... Under which circumstances would this happen? If
> it's something like only in a testing profile, that's perfectly legitimate and
a
> LOG(ERROR) is just going to produce spam. If we _think_ it can't happen, it's
> better to DCHECK it anyway, so we'd crash if the assumption turns out to be
> wrong.

Added the DCHECK since it looks like it can't happen.

https://codereview.chromium.org/2084903002/diff/280001/components/browsing_da...
File components/browsing_data/DEPS (right):

https://codereview.chromium.org/2084903002/diff/280001/components/browsing_da...
components/browsing_data/DEPS:3: "+content/public/browser",
On 2016/06/28 13:16:29, jochen wrote:
> if you plan to use this component on ios, you'll need to get rid of this dep

I'll have a look at this and make the necessary changes in a follow-up CL.

[Bernhard Bauer, 2016-06-28 14:29:39]

LGTM with some more simplifications:

https://codereview.chromium.org/2084903002/diff/260001/chrome/browser/android...
File chrome/browser/android/browsing_data/browsing_data_counter_bridge.cc
(right):

https://codereview.chromium.org/2084903002/diff/260001/chrome/browser/android...
chrome/browser/android/browsing_data/browsing_data_counter_bridge.cc:28:
ProfileManager::GetActiveUserProfile()->GetOriginalProfile();
On 2016/06/28 14:06:19, msramek wrote:
> On 2016/06/27 17:09:45, Bernhard Bauer wrote:
> > I know you just moved this from existing code, but is the
GetOriginalProfile()
> > necessary? Do we every try to clear browsing data for an incognito profile?
> 
> I'll respond, since this is my code :)
> 
> I would expect that it's not necessary, and that on Android
> GetActiveUserProfile() will actually be the original profile. However, I
> extracted this code from pref_service_bridge.cc which also diligently uses
> GetOriginalProfile() wherever it needs a profile, so I wasn't sure about it.
> 
> I would prefer to play it safe and keep it, but if you're convinced it can
never
> be off the record, I'm fine with adding a DCHECK instead.

Yeah, I think the ProfileManager always only looks at non-incognito (cognito? 😃)
profiles, so I think we could do the DCHECK, but I don't feel strongly enough to
make someone shave this particular yak.

https://codereview.chromium.org/2084903002/diff/260001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_counter_utils_unittest.cc
(right):

https://codereview.chromium.org/2084903002/diff/260001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_counter_utils_unittest.cc:31:
base::MessageLoop::current()->RunUntilIdle();
On 2016/06/28 14:12:35, ioanap wrote:
> On 2016/06/27 17:09:45, Bernhard Bauer wrote:
> > Is there a particular reason why this is necessary? (I don't think it's
> unlikely
> > that it is, but it would be at least worth checking).
> > 
> > In any case, use RunLoop to run the current message loop, not MessageLoop
> > directly.
> 
> 
> It would seem like it is not necessary, indeed.
> 
> Done.

Cool. In this case, you could even get rid of the unique_ptr and make the
TestingProfile a direct member that will be destroyed automatically :)

https://codereview.chromium.org/2084903002/diff/280001/chrome/browser/browsin...
File chrome/browser/browsing_data/passwords_counter.cc (right):

https://codereview.chromium.org/2084903002/diff/280001/chrome/browser/browsin...
chrome/browser/browsing_data/passwords_counter.cc:15: if (store_)
Now that we're asserting that |store_| can never be null, remove these checks as
well?

[ioanap, 2016-06-29 09:02:38]

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

[commit-bot: I haz the power, 2016-06-29 09:03:11]

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-06-29 10:06:03]

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

[commit-bot: I haz the power, 2016-06-29 10:06:05]

Dry run: This issue passed the CQ dry run.

[ioanap, 2016-06-29 10:55:20]

Fixed the small things as well!

Thank you,
Ioana

https://codereview.chromium.org/2084903002/diff/260001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_counter_utils_unittest.cc
(right):

https://codereview.chromium.org/2084903002/diff/260001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_counter_utils_unittest.cc:31:
base::MessageLoop::current()->RunUntilIdle();
On 2016/06/28 14:29:38, Bernhard Bauer wrote:
> On 2016/06/28 14:12:35, ioanap wrote:
> > On 2016/06/27 17:09:45, Bernhard Bauer wrote:
> > > Is there a particular reason why this is necessary? (I don't think it's
> > unlikely
> > > that it is, but it would be at least worth checking).
> > > 
> > > In any case, use RunLoop to run the current message loop, not MessageLoop
> > > directly.
> > 
> > 
> > It would seem like it is not necessary, indeed.
> > 
> > Done.
> 
> Cool. In this case, you could even get rid of the unique_ptr and make the
> TestingProfile a direct member that will be destroyed automatically :)

Done.

https://codereview.chromium.org/2084903002/diff/280001/chrome/browser/browsin...
File chrome/browser/browsing_data/passwords_counter.cc (right):

https://codereview.chromium.org/2084903002/diff/280001/chrome/browser/browsin...
chrome/browser/browsing_data/passwords_counter.cc:15: if (store_)
On 2016/06/28 14:29:39, Bernhard Bauer wrote:
> Now that we're asserting that |store_| can never be null, remove these checks
as
> well?

Done.

[jochen (gone - plz use gerrit), 2016-06-29 15:23:05]

lgtm

[ioanap, 2016-06-29 15:37:13]

dbeam@: friendly ping!

Thanks,
Ioana

[ioanap, 2016-06-30 12:25:54]

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

[commit-bot: I haz the power, 2016-06-30 12:26: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-06-30 13:24:51]

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

[commit-bot: I haz the power, 2016-06-30 13:24:53]

Dry run: This issue passed the CQ dry run.

[ioanap, 2016-07-05 13:12:45]

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

[commit-bot: I haz the power, 2016-07-05 13:13:01]

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-05 14:22:00]

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

[commit-bot: I haz the power, 2016-07-05 14:22:02]

Dry run: This issue passed the CQ dry run.

[ioanap, 2016-07-05 16:00:01]

@dbeam: friendly ping! :)

Please review some small changes in:

chrome/browser/ui/webui/

Thank you,
Ioana

[ioanap, 2016-07-05 16:00:05]

@dbeam: friendly ping! :)

Please review some small changes in:

chrome/browser/ui/webui/

Thank you,
Ioana

[Bernhard Bauer, 2016-07-05 16:15:50]

On 2016/07/05 16:00:05, ioanap wrote:
> @dbeam: friendly ping! :)
> 
> Please review some small changes in:
> 
> chrome/browser/ui/webui/
> 
> Thank you,
> Ioana

(Today is a US holiday, so I wouldn't expect a reply. As the changes to
chrome/browser/ui/webui/options/ are just updating call sites after a
refactoring and the overall change has been approved, TBRing them would also be
acceptable.)

[ioanap, 2016-07-06 11:02:04]

Description was changed from

==========
Moved BrowsingDataCounter and part of BrowsingDataCounterUtils to components.

Changes:
   - BrowsingDataCounter now resides in components/browsing_data/counters/
   - Moved enums and TimePeriod utils to
components/browsing_data/browsing_data_utils[.h/.cc]
   - Changes to the counters that still reside in chrome/browser/browsing_data/
are temporary, meant to allow this partial change.
     They will be moved to components in a subsequent CL

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

to

==========
Moved BrowsingDataCounter and part of BrowsingDataCounterUtils to components.

Changes:
   - BrowsingDataCounter now resides in components/browsing_data/counters/
   - Moved enums and TimePeriod utils to
components/browsing_data/browsing_data_utils[.h/.cc]
   - Changes to the counters that still reside in chrome/browser/browsing_data/
are temporary, meant to allow this partial change.
     They will be moved to components in a subsequent CL

TBR=dbeam@chromium.org
BUG=620317
==========

[ioanap, 2016-07-06 11:03:35]

Hi Dan,

Since this CL only makes a small change to the files in chrome/browser/ui/webui,
I thought it okay to TBR you. I hope this is alright. Please feel free to look
at it and comment where necessary!

Thank you, 
Ioana

[ioanap, 2016-07-06 11:04:19]

The CQ bit was checked by ioanap@google.com

[ioanap, 2016-07-06 11:04:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
msramek@chromium.org, bauerb@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2084903002/#ps360001
(title: "Rebase")

[commit-bot: I haz the power, 2016-07-06 11:04:31]

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-06 11:09:52]

Description was changed from

==========
Moved BrowsingDataCounter and part of BrowsingDataCounterUtils to components.

Changes:
   - BrowsingDataCounter now resides in components/browsing_data/counters/
   - Moved enums and TimePeriod utils to
components/browsing_data/browsing_data_utils[.h/.cc]
   - Changes to the counters that still reside in chrome/browser/browsing_data/
are temporary, meant to allow this partial change.
     They will be moved to components in a subsequent CL

TBR=dbeam@chromium.org
BUG=620317
==========

to

==========
Moved BrowsingDataCounter and part of BrowsingDataCounterUtils to components.

Changes:
   - BrowsingDataCounter now resides in components/browsing_data/counters/
   - Moved enums and TimePeriod utils to
components/browsing_data/browsing_data_utils[.h/.cc]
   - Changes to the counters that still reside in chrome/browser/browsing_data/
are temporary, meant to allow this partial change.
     They will be moved to components in a subsequent CL

TBR=dbeam@chromium.org
BUG=620317
==========

[commit-bot: I haz the power, 2016-07-06 11:09:54]

Committed patchset #19 (id:360001)

[commit-bot: I haz the power, 2016-07-06 11:10:05]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-06 11:11:17]

Description was changed from

==========
Moved BrowsingDataCounter and part of BrowsingDataCounterUtils to components.

Changes:
   - BrowsingDataCounter now resides in components/browsing_data/counters/
   - Moved enums and TimePeriod utils to
components/browsing_data/browsing_data_utils[.h/.cc]
   - Changes to the counters that still reside in chrome/browser/browsing_data/
are temporary, meant to allow this partial change.
     They will be moved to components in a subsequent CL

TBR=dbeam@chromium.org
BUG=620317
==========

to

==========
Moved BrowsingDataCounter and part of BrowsingDataCounterUtils to components.

Changes:
   - BrowsingDataCounter now resides in components/browsing_data/counters/
   - Moved enums and TimePeriod utils to
components/browsing_data/browsing_data_utils[.h/.cc]
   - Changes to the counters that still reside in chrome/browser/browsing_data/
are temporary, meant to allow this partial change.
     They will be moved to components in a subsequent CL

TBR=dbeam@chromium.org
BUG=620317

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

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

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