Separate state of basic and advanced tab in CBD dialog

Separate state of basic and advanced tab in CBD dialog.
 - Introduce ClearBrowsingDataTab enum
 - Add separate prefs for Basic and Advanced tab
 
Also
 - Change browsing_data::TimePeriod and browsing_data::BrowsingDataType to enum class

TBR=thakis@chromium.org
BUG=681523
Review-Url: https://codereview.chromium.org/2671743002
Cr-Commit-Position: refs/heads/master@{#454567}
Committed: https://chromium.googlesource.com/chromium/src/+/303f444d68f7f76722c98987afbdc6bcb466fd67

[dullweber, 2017-02-02 15:41:18]

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

[commit-bot: I haz the power, 2017-02-02 15:41:33]

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, 2017-02-02 15:45:24]

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

[commit-bot: I haz the power, 2017-02-02 15:45:25]

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

[dullweber, 2017-02-03 09:34:25]

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

[commit-bot: I haz the power, 2017-02-03 09:34:34]

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, 2017-02-03 09:41:50]

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

[commit-bot: I haz the power, 2017-02-03 09:41:51]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[dullweber, 2017-02-03 13:32:08]

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

[commit-bot: I haz the power, 2017-02-03 13:33:02]

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, 2017-02-03 13:49:51]

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

[commit-bot: I haz the power, 2017-02-03 13:49:52]

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

[dullweber, 2017-02-03 13:53:43]

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

[commit-bot: I haz the power, 2017-02-03 13:54:04]

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

[dullweber, 2017-02-03 13:54:17]

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

[commit-bot: I haz the power, 2017-02-03 13:54:41]

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, 2017-02-03 14:01:22]

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

[commit-bot: I haz the power, 2017-02-03 14:01:23]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[dullweber, 2017-02-03 15:11:25]

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

[commit-bot: I haz the power, 2017-02-03 15:11:42]

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

[dullweber, 2017-02-03 15:18:33]

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

[commit-bot: I haz the power, 2017-02-03 15:19:00]

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, 2017-02-03 16:36:34]

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

[commit-bot: I haz the power, 2017-02-03 16:36:34]

Dry run: This issue passed the CQ dry run.

[dullweber, 2017-02-07 19:28:49]

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

[commit-bot: I haz the power, 2017-02-07 19:29:28]

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, 2017-02-07 19:33:06]

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

[commit-bot: I haz the power, 2017-02-07 19:33:07]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dullweber, 2017-02-07 19:39:17]

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

[dullweber, 2017-02-07 19:39:18]

Hi Martin, please take a look at this cl. I added tests for the preference
migration. I'm not sure what would be the best way to check that the state of
basic and advanced tab is actually separate and a click on "delete" deletes the
right things. Is there something like a browser test for android ui?

[msramek, 2017-02-08 10:55:56]

LGTM % small comments.

https://codereview.chromium.org/2671743002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java
(right):

https://codereview.chromium.org/2671743002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:304:
* Returns whether this preference page is a basic or advanced tab in order to
use separate
// This method returns 1 or 2.
int method() { return 0; }

:-)

https://codereview.chromium.org/2671743002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java
(right):

https://codereview.chromium.org/2671743002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java:33:
public ClearBrowsingDataTabsFragment() {
Note that migrating syncable preferences is sometimes difficult; you do the
migration on browser startup when Sync is not running, then Sync kicks in and
overwrites the just migrated preferences with their server values, which are
typically exactly the pre-migration values.

Fortunately, this is not a problem in our case. Since we're just adding new
preferences, the server will have no values for them when the migration first
happens.

It might be worth noting this in a comment.

https://codereview.chromium.org/2671743002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java:34:
PrefServiceBridge.getInstance().migrateBrowsingDataPreferences();
Please add a TODO to remove this code in, let's say, three milestones (that
would be M61 or M62, depending on when this is finished).

We do the migration to provide smooth experience for users who will be using
Chromium when this feature launches. We want to make sure that they won't be
surprised by a sudden change in checkbox configuration overnight.

For users who don't use Chrome for several milestones (e.g. update M57->M62)
this is not as important; and would only add code complexity.

https://codereview.chromium.org/2671743002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java:37:
public static boolean isFeatureEnabled() {
Javadocs for public methods plz :)

https://codereview.chromium.org/2671743002/diff/140001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java
(right):

https://codereview.chromium.org/2671743002/diff/140001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:184:
for (int i = 0; i < screen.getPreferenceCount(); ++i) {
You were asking about tests on Android - this file is an example of the so
called instrumentation test.

On this line, we iterate over checkboxes, make sure they're all checked, then
programatically click the "Clear" button. You might want to use the same
principle to test the UI; For example, check the checkboxes in one tab, switch
to the other tab, check that the configuration didn't carry over etc.

Regarding integration tests:
-> For data that are handled by Android code, we have
BrowsingDataRemoverIntegrationTest.
-> For data that are handled by the native code, we currently rely on unittests
in c/b/browsing_data and this instrumentation test. However, a bug in
PrefServiceBridge could potentially go unnoticed. I would love for you to
improve that, but that's a larger effort that does not belong to this CL.

Basically, to write a proper integration test between the Android UI and the
backend, you need an instrumentation test which populates the data storage
backends, runs the deletion, and inspects that the storage backends are empty.
That means adding a lot of methods to *Bridge for the population and inspection.
If we proceed to do that, I would start by splitting of browsing_data stuff from
PrefServiceBridge to our own BrowsingDataBridge.

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

https://codereview.chromium.org/2671743002/diff/140001/chrome/browser/android...
chrome/browser/android/browsing_data/browsing_data_counter_bridge.cc:25:
DCHECK_LT(data_type, browsing_data::NUM_TYPES);
While you're here, can you please change this enum to an enum class as well?
There is no actual namespace collision with the new enum class that you added,
but still - if we have two enums which both have NUM_TYPES, let's make sure they
both always use full namespace.

https://codereview.chromium.org/2671743002/diff/140001/chrome/browser/android...
chrome/browser/android/browsing_data/browsing_data_counter_bridge.cc:26:
DCHECK_GE(pref_type_int, 0);
Please also add the other DCHECK.

https://codereview.chromium.org/2671743002/diff/140001/chrome/browser/policy/...
File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right):

https://codereview.chromium.org/2671743002/diff/140001/chrome/browser/policy/...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:736:
browsing_data::prefs::kDeleteBrowsingHistoryBasic, false);
Good catch!

https://codereview.chromium.org/2671743002/diff/140001/components/browsing_da...
File components/browsing_data/core/browsing_data_enums.h (right):

https://codereview.chromium.org/2671743002/diff/140001/components/browsing_da...
components/browsing_data/core/browsing_data_enums.h:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
nit: 2017

https://codereview.chromium.org/2671743002/diff/140001/components/browsing_da...
components/browsing_data/core/browsing_data_enums.h:17: enum class
ClearBrowsingDataPreferenceType {
All methods manipulating this enum are in utils.h. All consumers of this enum
already include utils.h. The extraction to a separate file seems artificial to
me. Is there a reason for that?

https://codereview.chromium.org/2671743002/diff/140001/components/browsing_da...
components/browsing_data/core/browsing_data_enums.h:18: DEFAULT,
I'm wondering if we need the concept of DEFAULT; we could treat the Desktop and
iOS dialog as only having the "Advanced" tab. Wouldn't that make the code
simpler?

https://codereview.chromium.org/2671743002/diff/140001/components/browsing_da...
File components/browsing_data/core/counters/browsing_data_counter.cc (right):

https://codereview.chromium.org/2671743002/diff/140001/components/browsing_da...
components/browsing_data/core/counters/browsing_data_counter.cc:59: return
GetPrefType() == ClearBrowsingDataPreferenceType::BASIC
This is already defined as
browsing_data::GetTimePeriodPreferenceName(ClearBrowsingDataPreferenceType) in
utils.h. We should call that here to make sure that the definitions don't
diverge.

[dullweber, 2017-02-08 23:02:29]

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

[commit-bot: I haz the power, 2017-02-08 23:03:12]

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

[dullweber, 2017-02-08 23:03:19]

Thanks, I fixed most comments. I still need to add the test

https://codereview.chromium.org/2671743002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java
(right):

https://codereview.chromium.org/2671743002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:304:
* Returns whether this preference page is a basic or advanced tab in order to
use separate
On 2017/02/08 10:55:55, msramek wrote:
> // This method returns 1 or 2.
> int method() { return 0; }
> 
> :-)

ahh right, that's stupid :D Should be solved now, that there is no default value
anymore.

https://codereview.chromium.org/2671743002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java
(right):

https://codereview.chromium.org/2671743002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java:33:
public ClearBrowsingDataTabsFragment() {
On 2017/02/08 10:55:56, msramek wrote:
> Note that migrating syncable preferences is sometimes difficult; you do the
> migration on browser startup when Sync is not running, then Sync kicks in and
> overwrites the just migrated preferences with their server values, which are
> typically exactly the pre-migration values.
> 
> Fortunately, this is not a problem in our case. Since we're just adding new
> preferences, the server will have no values for them when the migration first
> happens.
> 
> It might be worth noting this in a comment.

This code also isn't executed when chrome starts but only when you open the new
dialog for the first time.

https://codereview.chromium.org/2671743002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java:34:
PrefServiceBridge.getInstance().migrateBrowsingDataPreferences();
On 2017/02/08 10:55:56, msramek wrote:
> Please add a TODO to remove this code in, let's say, three milestones (that
> would be M61 or M62, depending on when this is finished).
> 
> We do the migration to provide smooth experience for users who will be using
> Chromium when this feature launches. We want to make sure that they won't be
> surprised by a sudden change in checkbox configuration overnight.
> 
> For users who don't use Chrome for several milestones (e.g. update M57->M62)
> this is not as important; and would only add code complexity.

Done.

https://codereview.chromium.org/2671743002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java:37:
public static boolean isFeatureEnabled() {
On 2017/02/08 10:55:55, msramek wrote:
> Javadocs for public methods plz :)

Done.

https://codereview.chromium.org/2671743002/diff/140001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java
(right):

https://codereview.chromium.org/2671743002/diff/140001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:184:
for (int i = 0; i < screen.getPreferenceCount(); ++i) {
On 2017/02/08 10:55:56, msramek wrote:
> You were asking about tests on Android - this file is an example of the so
> called instrumentation test.
> 
> On this line, we iterate over checkboxes, make sure they're all checked, then
> programatically click the "Clear" button. You might want to use the same
> principle to test the UI; For example, check the checkboxes in one tab, switch
> to the other tab, check that the configuration didn't carry over etc.
> 
> Regarding integration tests:
> -> For data that are handled by Android code, we have
> BrowsingDataRemoverIntegrationTest.
> -> For data that are handled by the native code, we currently rely on
unittests
> in c/b/browsing_data and this instrumentation test. However, a bug in
> PrefServiceBridge could potentially go unnoticed. I would love for you to
> improve that, but that's a larger effort that does not belong to this CL.
> 
> Basically, to write a proper integration test between the Android UI and the
> backend, you need an instrumentation test which populates the data storage
> backends, runs the deletion, and inspects that the storage backends are empty.
> That means adding a lot of methods to *Bridge for the population and
inspection.
> If we proceed to do that, I would start by splitting of browsing_data stuff
from
> PrefServiceBridge to our own BrowsingDataBridge.

Ok, I will try to add a test here. Maybe it would be helpful if we could
redirect the call to a mock browsing data remover and only check that the right
remove mask is used to avoid creating all the different data types from java?
The actual deletion should already be tested in many unit tests. The browsing
data remover factory looks like a good point to insert a different remover that
just remembers the last deletion mask.

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

https://codereview.chromium.org/2671743002/diff/140001/chrome/browser/android...
chrome/browser/android/browsing_data/browsing_data_counter_bridge.cc:25:
DCHECK_LT(data_type, browsing_data::NUM_TYPES);
On 2017/02/08 10:55:56, msramek wrote:
> While you're here, can you please change this enum to an enum class as well?
> There is no actual namespace collision with the new enum class that you added,
> but still - if we have two enums which both have NUM_TYPES, let's make sure
they
> both always use full namespace.

I changed BrowsingDataType and also TimePeriod to an enum class as it was in the
same header.

https://codereview.chromium.org/2671743002/diff/140001/chrome/browser/android...
chrome/browser/android/browsing_data/browsing_data_counter_bridge.cc:26:
DCHECK_GE(pref_type_int, 0);
On 2017/02/08 10:55:56, msramek wrote:
> Please also add the other DCHECK.

Done.

https://codereview.chromium.org/2671743002/diff/140001/chrome/browser/policy/...
File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right):

https://codereview.chromium.org/2671743002/diff/140001/chrome/browser/policy/...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:736:
browsing_data::prefs::kDeleteBrowsingHistoryBasic, false);
On 2017/02/08 10:55:56, msramek wrote:
> Good catch!

thanks, I tried to find all usages of kDeleteBrowsingHistory and the other
preferences. I hope I have all of them :D

https://codereview.chromium.org/2671743002/diff/140001/components/browsing_da...
File components/browsing_data/core/browsing_data_enums.h (right):

https://codereview.chromium.org/2671743002/diff/140001/components/browsing_da...
components/browsing_data/core/browsing_data_enums.h:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
On 2017/02/08 10:55:56, msramek wrote:
> nit: 2017

Done.

https://codereview.chromium.org/2671743002/diff/140001/components/browsing_da...
components/browsing_data/core/browsing_data_enums.h:17: enum class
ClearBrowsingDataPreferenceType {
On 2017/02/08 10:55:56, msramek wrote:
> All methods manipulating this enum are in utils.h. All consumers of this enum
> already include utils.h. The extraction to a separate file seems artificial to
> me. Is there a reason for that?

The problem was that adding this enum to utils.h creates a circular dependency
between utils.h and counter.h because utils needs BrowsingDataCounter::Result*
(can't be forward declared because it is an inner class) and the counter needs
this new enum. I don't know if there is another way to solve this?

https://codereview.chromium.org/2671743002/diff/140001/components/browsing_da...
components/browsing_data/core/browsing_data_enums.h:18: DEFAULT,
On 2017/02/08 10:55:56, msramek wrote:
> I'm wondering if we need the concept of DEFAULT; we could treat the Desktop
and
> iOS dialog as only having the "Advanced" tab. Wouldn't that make the code
> simpler?

That works as well. I will add a comment that the old code will use advanced.

https://codereview.chromium.org/2671743002/diff/140001/components/browsing_da...
File components/browsing_data/core/counters/browsing_data_counter.cc (right):

https://codereview.chromium.org/2671743002/diff/140001/components/browsing_da...
components/browsing_data/core/counters/browsing_data_counter.cc:59: return
GetPrefType() == ClearBrowsingDataPreferenceType::BASIC
On 2017/02/08 10:55:56, msramek wrote:
> This is already defined as
> browsing_data::GetTimePeriodPreferenceName(ClearBrowsingDataPreferenceType) in
> utils.h. We should call that here to make sure that the definitions don't
> diverge.

Thanks, I removed this method.

[commit-bot: I haz the power, 2017-02-08 23:06:34]

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

[commit-bot: I haz the power, 2017-02-08 23:06:35]

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

[dullweber, 2017-02-08 23:46:57]

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

[commit-bot: I haz the power, 2017-02-08 23:47:55]

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, 2017-02-08 23:58:03]

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

[commit-bot: I haz the power, 2017-02-08 23:58:04]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[dullweber, 2017-02-09 00:27:59]

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

[commit-bot: I haz the power, 2017-02-09 00:28:41]

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, 2017-02-09 00:37:48]

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

[commit-bot: I haz the power, 2017-02-09 00:37:49]

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

[dullweber, 2017-02-09 00:43:56]

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

[commit-bot: I haz the power, 2017-02-09 00:44:24]

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, 2017-02-09 00:54:59]

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

[commit-bot: I haz the power, 2017-02-09 00:55:00]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[dullweber, 2017-02-09 00:59:03]

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

[commit-bot: I haz the power, 2017-02-09 00:59:41]

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, 2017-02-09 02:04:41]

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

[commit-bot: I haz the power, 2017-02-09 02:04:42]

Dry run: This issue passed the CQ dry run.

[msramek, 2017-02-09 18:16:48]

Still LGTM % comment.

Please also update the CL description to mention the secondary changes you made
(enum->enum class etc.).

https://codereview.chromium.org/2671743002/diff/140001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java
(right):

https://codereview.chromium.org/2671743002/diff/140001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:184:
for (int i = 0; i < screen.getPreferenceCount(); ++i) {
On 2017/02/08 23:03:18, dullweber wrote:
> On 2017/02/08 10:55:56, msramek wrote:
> > You were asking about tests on Android - this file is an example of the so
> > called instrumentation test.
> > 
> > On this line, we iterate over checkboxes, make sure they're all checked,
then
> > programatically click the "Clear" button. You might want to use the same
> > principle to test the UI; For example, check the checkboxes in one tab,
switch
> > to the other tab, check that the configuration didn't carry over etc.
> > 
> > Regarding integration tests:
> > -> For data that are handled by Android code, we have
> > BrowsingDataRemoverIntegrationTest.
> > -> For data that are handled by the native code, we currently rely on
> unittests
> > in c/b/browsing_data and this instrumentation test. However, a bug in
> > PrefServiceBridge could potentially go unnoticed. I would love for you to
> > improve that, but that's a larger effort that does not belong to this CL.
> > 
> > Basically, to write a proper integration test between the Android UI and the
> > backend, you need an instrumentation test which populates the data storage
> > backends, runs the deletion, and inspects that the storage backends are
empty.
> > That means adding a lot of methods to *Bridge for the population and
> inspection.
> > If we proceed to do that, I would start by splitting of browsing_data stuff
> from
> > PrefServiceBridge to our own BrowsingDataBridge.
> 
> Ok, I will try to add a test here. Maybe it would be helpful if we could
> redirect the call to a mock browsing data remover and only check that the
right
> remove mask is used to avoid creating all the different data types from java?
> The actual deletion should already be tested in many unit tests. The browsing
> data remover factory looks like a good point to insert a different remover
that
> just remembers the last deletion mask.

Yes, that's a possibility. We could use
BrowsingDataRemoverFactory::SetTestingFactory() to force the profile to produce
a mock. Then we would call something like testDeletion(datatypes,
expected_mask), which passes the |datatypes| through PrefServiceBridge and
verifies that the mock has seen a call with |expected_mask|. That would cost us
only 2 new methods in the bridge.

https://codereview.chromium.org/2671743002/diff/140001/components/browsing_da...
File components/browsing_data/core/browsing_data_enums.h (right):

https://codereview.chromium.org/2671743002/diff/140001/components/browsing_da...
components/browsing_data/core/browsing_data_enums.h:17: enum class
ClearBrowsingDataPreferenceType {
On 2017/02/08 23:03:18, dullweber wrote:
> On 2017/02/08 10:55:56, msramek wrote:
> > All methods manipulating this enum are in utils.h. All consumers of this
enum
> > already include utils.h. The extraction to a separate file seems artificial
to
> > me. Is there a reason for that?
> 
> The problem was that adding this enum to utils.h creates a circular dependency
> between utils.h and counter.h because utils needs BrowsingDataCounter::Result*
> (can't be forward declared because it is an inner class) and the counter needs
> this new enum. I don't know if there is another way to solve this?

Right... yeah, I don't think there's a workaround. Probably the cleanest
solution is to extract Result as a standalone class; you could argue that this
is the right thing to do, since utils only need to interpret the result, they
don't care that it came from a counter. We'll probably run into the same problem
with Counter subclasses and their inner classes which are Result subclasses; so
the file with the Result could also contain the other subclasses.

But again, that's for another CL.

Then we should at least rename this file. It does not contain all browsing_data
enums, just one of them. And so it should be named after that enum.
Additionally, that enum should be named better as well. This enum is a key for
the tabs in the UI, the fact that we store the checkbox selections in
preferences is an implementation detail. So maybe ClearBrowsingDataDialogTab or
BrowsingDataTab if you think the former is too long.

(Sorry for causing mass renaming, but I think these things tend to get confusing
over time; looking at 'BrowsingDataType' and 'ClearBrowsingDataPreferenceType',
it's not exactly clear to me what's the difference).

[dullweber, 2017-02-22 00:53:22]

Description was changed from

==========
Separate state of basic and advanced tab in CBD dialog

BUG=681523
==========

to

==========
Separate state of basic and advanced tab in CBD dialog.
 - Introduce ClearBrowsingDataTab enum
 - Add separate prefs for Basic and Advanced tab
 
Also
 - Change browsing_data::TimePeriod and browsing_data::BrowsingDataType to enum
class

BUG=681523
==========

[dullweber, 2017-02-22 00:54:08]

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

[commit-bot: I haz the power, 2017-02-22 00:55:37]

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, 2017-02-22 01:13:33]

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

[commit-bot: I haz the power, 2017-02-22 01:13:34]

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

[dullweber, 2017-02-22 01:17:40]

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

[commit-bot: I haz the power, 2017-02-22 01:18:56]

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, 2017-02-22 03:23:07]

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

[commit-bot: I haz the power, 2017-02-22 03:23:08]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[dullweber, 2017-02-28 09:41:46]

dullweber@chromium.org changed reviewers:
+ twellington@chromium.org

[dullweber, 2017-02-28 09:41:48]

twellington@chromium.org: Please review changes in M	
chrome/android/java/src/org/chromium/chrome/browser/preferences

[dullweber, 2017-02-28 09:47:02]

dullweber@chromium.org changed reviewers:
+ sdefresne@chromium.org

[dullweber, 2017-02-28 09:47:03]

sdefresne@chromium.org: Please review these small changes in ios/

[dullweber, 2017-02-28 09:55:21]

dullweber@chromium.org changed reviewers:
+ emaxx@chromium.org

[dullweber, 2017-02-28 09:55:22]

emaxx@chromium.org: Please review the change in
chrome/browser/policy/configuration_policy_handler_list_factory.cc

[emaxx, 2017-02-28 14:16:14]

https://codereview.chromium.org/2671743002/diff/360001/chrome/browser/policy/...
File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right):

https://codereview.chromium.org/2671743002/diff/360001/chrome/browser/policy/...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:738:
prefs->SetBoolean(browsing_data::prefs::kDeleteBrowsingHistoryBasic,
Looks like there is no actual browser test for AllowDeletingBrowserHistory
policy that covers this logic.

From the discussion above, it seems that the complete end-to-end test (e.g. set
up the policy and check that the UI is disabled and the deletion is not
possible) is quite difficult to write.

But would you mind adding a browser test that covers this logic of mapping the
policy to prefs?
I think the chrome/browser/policy/policy_browsertest.cc file would be a relevant
place for this test. For an example of the test, you could look at
PolicyTest.BookmarkBarEnabled:
https://cs.chromium.org/chromium/src/chrome/browser/policy/policy_browsertest...

[sdefresne, 2017-02-28 17:45:25]

ios/ lgtm (with a formatting comment)

https://codereview.chromium.org/2671743002/diff/360001/ios/chrome/browser/ui/...
File
ios/chrome/browser/ui/settings/clear_browsing_data_collection_view_controller.mm
(right):

https://codereview.chromium.org/2671743002/diff/360001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/settings/clear_browsing_data_collection_view_controller.mm:195:
if (prefValue >
Can you use a local variable to store
"static_cast<int>(browsing_data::TimePeriod::TIME_PERIOD_LAST)" so that
formatting is better?

  const int maxValue =
static_cast<int>(browsing_data::TimePeriod::TIME_PERIOD_LAST);
  prefValue = std::min(maxValue, std::max(0, prefValue));

[dullweber, 2017-03-01 13:03:09]

improved ios formatting

https://codereview.chromium.org/2671743002/diff/360001/ios/chrome/browser/ui/...
File
ios/chrome/browser/ui/settings/clear_browsing_data_collection_view_controller.mm
(right):

https://codereview.chromium.org/2671743002/diff/360001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/settings/clear_browsing_data_collection_view_controller.mm:195:
if (prefValue >
On 2017/02/28 17:45:24, sdefresne wrote:
> Can you use a local variable to store
> "static_cast<int>(browsing_data::TimePeriod::TIME_PERIOD_LAST)" so that
> formatting is better?
> 
>   const int maxValue =
> static_cast<int>(browsing_data::TimePeriod::TIME_PERIOD_LAST);
>   prefValue = std::min(maxValue, std::max(0, prefValue));

sure, fixed

[Theresa, 2017-03-01 17:45:46]

lgtm % nits

https://codereview.chromium.org/2671743002/diff/380001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java
(right):

https://codereview.chromium.org/2671743002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:769:
* with tabs is visited.
nit: Does this fit on the line above?

https://codereview.chromium.org/2671743002/diff/380001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java
(right):

https://codereview.chromium.org/2671743002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java:34:
// TODO(dullweber): Remove this migration after after three milestones (probably
M61)
Typically migration paths cannot be removed after 3 milestones because users may
take a long time to upgrade. If the user is upgrading from an old version and
the migration is skipped, what happens?

https://codereview.chromium.org/2671743002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java:39:
* @return boolean Returns if the CBD dialog with tabs is enabled.
nit: "Returns whether the..."

We also typically don't start the @return JavaDoc with the type. The description
makes it clear that it's returning a boolean.

[dullweber, 2017-03-02 09:24:36]

dullweber@chromium.org changed reviewers:
+ tedchoc@chromium.org

[dullweber, 2017-03-02 09:24:38]

tedchoc@chromium.org: Please review changes in chrome/browser/android/

https://codereview.chromium.org/2671743002/diff/380001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java
(right):

https://codereview.chromium.org/2671743002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:769:
* with tabs is visited.
On 2017/03/01 17:45:45, Theresa wrote:
> nit: Does this fit on the line above?

not completely but I moved some words up

https://codereview.chromium.org/2671743002/diff/380001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java
(right):

https://codereview.chromium.org/2671743002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java:34:
// TODO(dullweber): Remove this migration after after three milestones (probably
M61)
On 2017/03/01 17:45:45, Theresa wrote:
> Typically migration paths cannot be removed after 3 milestones because users
may
> take a long time to upgrade. If the user is upgrading from an old version and
> the migration is skipped, what happens?

The migration only copies the prefs from the currently selected checkboxes to
the new prefs for the basic tab, so that they will initially be the same.
Nothing will break if this is not done, so we thought that this code is not
really needed in the long term.

https://codereview.chromium.org/2671743002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataTabsFragment.java:39:
* @return boolean Returns if the CBD dialog with tabs is enabled.
On 2017/03/01 17:45:45, Theresa wrote:
> nit: "Returns whether the..."
> 
> We also typically don't start the @return JavaDoc with the type. The
description
> makes it clear that it's returning a boolean.

Done.

[dullweber, 2017-03-02 09:31:16]

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

[commit-bot: I haz the power, 2017-03-02 09:31:32]

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

[dullweber, 2017-03-02 10:10:00]

@emaxx@chromium.org: I added a test for the policy

https://codereview.chromium.org/2671743002/diff/360001/chrome/browser/policy/...
File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right):

https://codereview.chromium.org/2671743002/diff/360001/chrome/browser/policy/...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:738:
prefs->SetBoolean(browsing_data::prefs::kDeleteBrowsingHistoryBasic,
On 2017/02/28 14:16:14, emaxx wrote:
> Looks like there is no actual browser test for AllowDeletingBrowserHistory
> policy that covers this logic.
> 
> From the discussion above, it seems that the complete end-to-end test (e.g.
set
> up the policy and check that the UI is disabled and the deletion is not
> possible) is quite difficult to write.
> 
> But would you mind adding a browser test that covers this logic of mapping the
> policy to prefs?
> I think the chrome/browser/policy/policy_browsertest.cc file would be a
relevant
> place for this test. For an example of the test, you could look at
> PolicyTest.BookmarkBarEnabled:
>
https://cs.chromium.org/chromium/src/chrome/browser/policy/policy_browsertest...

I added a test to verify that the prefs are set correctly when activating the
policy.

[dullweber, 2017-03-02 10:10:54]

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

[commit-bot: I haz the power, 2017-03-02 10:11: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, 2017-03-02 11:34:36]

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

[commit-bot: I haz the power, 2017-03-02 11:34:37]

Dry run: This issue passed the CQ dry run.

[emaxx, 2017-03-02 12:28:49]

On 2017/03/02 10:10:00, dullweber wrote:
> I added a test to verify that the prefs are set correctly when activating the
> policy.

LGTM. Thanks for adding this test!

[Ted C, 2017-03-02 22:06:15]

lgtm

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

https://codereview.chromium.org/2671743002/diff/420001/chrome/browser/android...
chrome/browser/android/browsing_data/browsing_data_counter_bridge.cc:22: jint
clear_browsing_data_tab_int)
I would try to keep this in sync with the header.  When we want a different name
from java to C++, we will use the naming scheme jclear_browsing_data_tab

https://codereview.chromium.org/2671743002/diff/420001/chrome/browser/android...
File chrome/browser/android/preferences/pref_service_bridge.cc (right):

https://codereview.chromium.org/2671743002/diff/420001/chrome/browser/android...
chrome/browser/android/preferences/pref_service_bridge.cc:627:
browsing_data::MigratePreferencesToBasic(GetOriginalProfile()->GetPrefs());
Is Android the only place that this method needs to be called?  I tried to see
if we could pull it to a more common Chrome codepath, but didn't see any other
callers.

[dullweber, 2017-03-03 10:14:19]

I fixed the variable name, thanks

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

https://codereview.chromium.org/2671743002/diff/420001/chrome/browser/android...
chrome/browser/android/browsing_data/browsing_data_counter_bridge.cc:22: jint
clear_browsing_data_tab_int)
On 2017/03/02 22:06:15, Ted C wrote:
> I would try to keep this in sync with the header.  When we want a different
name
> from java to C++, we will use the naming scheme jclear_browsing_data_tab

Done.

https://codereview.chromium.org/2671743002/diff/420001/chrome/browser/android...
File chrome/browser/android/preferences/pref_service_bridge.cc (right):

https://codereview.chromium.org/2671743002/diff/420001/chrome/browser/android...
chrome/browser/android/preferences/pref_service_bridge.cc:627:
browsing_data::MigratePreferencesToBasic(GetOriginalProfile()->GetPrefs());
On 2017/03/02 22:06:15, Ted C wrote:
> Is Android the only place that this method needs to be called?  I tried to see
> if we could pull it to a more common Chrome codepath, but didn't see any other
> callers.

This method is currently only used from android when the new CBD dialog is
visited for the first time. At some point it will be necessary on desktop as
well but the UI is not implemented yet.

[dullweber, 2017-03-03 10:20:01]

Description was changed from

==========
Separate state of basic and advanced tab in CBD dialog.
 - Introduce ClearBrowsingDataTab enum
 - Add separate prefs for Basic and Advanced tab
 
Also
 - Change browsing_data::TimePeriod and browsing_data::BrowsingDataType to enum
class

BUG=681523
==========

to

==========
Separate state of basic and advanced tab in CBD dialog.
 - Introduce ClearBrowsingDataTab enum
 - Add separate prefs for Basic and Advanced tab
 
Also
 - Change browsing_data::TimePeriod and browsing_data::BrowsingDataType to enum
class

TBR=thakis@chromium.org
BUG=681523
==========

[dullweber, 2017-03-03 10:22:46]

dullweber@chromium.org changed reviewers:
+ thakis@chromium.org

[dullweber, 2017-03-03 10:22:48]

thakis: adding you as TBR for changes in chrome/browser/ required by converting
enum to enum class

[dullweber, 2017-03-03 10:23:04]

The CQ bit was checked by dullweber@chromium.org

[dullweber, 2017-03-03 10:23:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
msramek@chromium.org, sdefresne@chromium.org, twellington@chromium.org,
tedchoc@chromium.org, emaxx@chromium.org
Link to the patchset: https://codereview.chromium.org/2671743002/#ps440001
(title: "rename clear_browsing_data_tab_int to clear_browsing_data_tab")

[commit-bot: I haz the power, 2017-03-03 10:23:17]

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, 2017-03-03 10:37:42]

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

[commit-bot: I haz the power, 2017-03-03 10:37:44]

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_...)

[dullweber, 2017-03-03 10:48:04]

The CQ bit was checked by dullweber@chromium.org

[dullweber, 2017-03-03 10:48:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
msramek@chromium.org, twellington@chromium.org, tedchoc@chromium.org,
sdefresne@chromium.org, emaxx@chromium.org
Link to the patchset: https://codereview.chromium.org/2671743002/#ps460001
(title: "rebase and fix compilation")

[commit-bot: I haz the power, 2017-03-03 10:48:20]

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, 2017-03-03 11:50:24]

CQ is committing da patch.

Bot data: {"patchset_id": 460001, "attempt_start_ts": 1488538084293400,
"parent_rev": "246ebe8f2a857270c6a256493e2244babf257c00", "commit_rev":
"303f444d68f7f76722c98987afbdc6bcb466fd67"}

[commit-bot: I haz the power, 2017-03-03 11:51:01]

Description was changed from

==========
Separate state of basic and advanced tab in CBD dialog.
 - Introduce ClearBrowsingDataTab enum
 - Add separate prefs for Basic and Advanced tab
 
Also
 - Change browsing_data::TimePeriod and browsing_data::BrowsingDataType to enum
class

TBR=thakis@chromium.org
BUG=681523
==========

to

==========
Separate state of basic and advanced tab in CBD dialog.
 - Introduce ClearBrowsingDataTab enum
 - Add separate prefs for Basic and Advanced tab
 
Also
 - Change browsing_data::TimePeriod and browsing_data::BrowsingDataType to enum
class

TBR=thakis@chromium.org
BUG=681523

Review-Url: https://codereview.chromium.org/2671743002
Cr-Commit-Position: refs/heads/master@{#454567}
Committed:
https://chromium.googlesource.com/chromium/src/+/303f444d68f7f76722c98987afbd...
==========

[commit-bot: I haz the power, 2017-03-03 11:51:03]

Committed patchset #24 (id:460001) as
https://chromium.googlesource.com/chromium/src/+/303f444d68f7f76722c98987afbd...