Separate state of basic and advanced tab in CBD dialog

components/browsing_data/core/browsing_data_enums.h

+// Copyright 2016 The Chromium Authors. All rights reserved.

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

nit: 2017

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

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_BROWSING_DATA_CORE_BROWSING_DATA_ENUMS_H_
+#define COMPONENTS_BROWSING_DATA_CORE_BROWSING_DATA_ENUMS_H_
+
+// Enums shared between browsing_data_counter and browsing_data_utils.
+namespace browsing_data {
+
+// This enum is used to differentiate CBD preferences from the basic and
+// advanced tab and manage their state separately. It is important that all
+// preferences and the timeperiod selection have the same type.
+//
+// A Java counterpart will be generated for this enum.
+// GENERATED_JAVA_ENUM_PACKAGE: org.chromium.chrome.browser.browsing_data
+enum class ClearBrowsingDataPreferenceType {

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

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?

[dullweber, 2017/02/08 23:03:18]

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?

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

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

+  DEFAULT,

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

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?

[dullweber, 2017/02/08 23:03:18]

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

+  BASIC,
+  ADVANCED,
+  NUM_TYPES
+};
+
+}  // namespace browsing_data
+
+#endif  // COMPONENTS_BROWSING_DATA_CORE_BROWSING_DATA_ENUMS_H_