Convert RemoveDataMask from enum to pointers and split it between content and embedder

Convert RemoveDataMask from enum to pointers and split it between content and embedder

As the next step of the migration of browsing_data codebase
to content, we need to split RemoveDataMask into platform
datatypes (defined in content/) and Chrome-specific datatypes
(defined in chrome/).

The datatypes from BrowsingDataRemover::RemoveDataMask are now
represented as global static objects of the BrowsingDataType
type; BrowsingDataRemover and related classes use their
addresses (const BrowsingDataType*) to distinguish them.
To pass a mask consisting of multiple datatypes, we use
std::set<const BrowsingDataType*>.

See the design doc for more details why this approach was
chosen:
https://docs.google.com/document/d/1I6m4QwbTNhG6wdtazamhTnArJN-UMUGqpvwH6InBEaM/

Details (a.k.a. how to review this CL):

1. Platform datatypes are defined in the content namespace
   in the file content/public/browsing_data_types.[h|cc].

2. Chrome datatypes are defined in the default namespace in
   the file c/b/browsing_data/chrome_browsing_data_types.[h|cc];
   this file also imports the platform datatypes to the
   default namespace, since embedder callsites should not
   have to know where a datatype originates.

3. The rest of the CL consists mostly of equivalent replacements;
   this is largely possible thanks to C++11 initializers
   allowing syntax such as std::set<X> x = { x1, x2, x3 };
   to replace what was previously enum bitmask "x1 | x2 | x3";
   bitmasks bitwise operations are more complicated to replace;
   there, we use set operations from the <algorithm> library.

4. The filterability of a datatype is now defined as an
   attribute of the BrowsingDataType object.

5. Other nonequivalent changes suggested in the above doc,
   such as the introduction of a "storage" concept in place
   of "site data" are not a part of this CL.

BUG=668114

[msramek, 2017-02-16 19:58:48]

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

[commit-bot: I haz the power, 2017-02-16 19:59:39]

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-16 20:29:06]

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

[commit-bot: I haz the power, 2017-02-16 20:29:07]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[msramek, 2017-02-16 22:00:11]

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

[commit-bot: I haz the power, 2017-02-16 22:01:15]

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

[msramek, 2017-02-16 22:06:57]

Description was changed from

==========
Convert RemoveDataMask from enum to pointers and split it between content and
embedder

BUG=668114
==========

to

==========
Convert RemoveDataMask from enum to pointers and split it between content and
embedder

As the next step of the migration of browsing_data codebase
to content, we need to split RemoveDataMask into platform
datatypes (defined in content/) and Chrome-specific datatypes
(defined in chrome/).

The datatypes from BrowsingDataRemover::RemoveDataMask are now
represented as global static objects of the BrowsingDataType
type; BrowsingDataRemover and related classes use their
addresses (const BrowsingDataType*) to distinguish them.
To pass a mask consisting of multiple datatypes, we use
std::set<const BrowsingDataType*>.

See the design doc for more details why this approach was
chosen:
https://docs.google.com/document/d/1I6m4QwbTNhG6wdtazamhTnArJN-UMUGqpvwH6InBEaM/

Details (a.k.a. how to review this CL):

1. Platform datatypes are defined in the content namespace
   in the file content/public/browsing_data_types.[h|cc].

2. Chrome datatypes are defined in the default namespace in
   the file c/b/browsing_data/chrome_browsing_data_types.[h|cc];
   this file also imports the platform datatypes to the
   default namespace, since embedder callsites should not
   have to know where a datatype originates.

3. The rest of the CL consists mostly of equivalent replacements;
   this is largely possible thanks to C++11 initializers
   allowing syntax such as std::set<X> x = { x1, x2, x3 };
   to replace what was previously enum bitmask "x1 | x2 | x3";
   bitmasks bitwise operations are more complicated to replace;
   there, we use set operations from the <algorithm> library.

4. The filterability of a datatype is now defined as an
   attribute of the BrowsingDataType object.

5. Other nonequivalent changes suggested in the above doc,
   such as the introduction of a "storage" concept in place
   of "site data" are not a part of this CL.

BUG=668114
==========

[msramek, 2017-02-16 22:20:18]

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

[msramek, 2017-02-16 22:20:19]

Hi Bernhard and Mike,

Here's the result of our discussion about RemoveDataMask from the doc. Can you
please both take a pass on this? This is a large change, so I want to make sure
that everyone is on board :)

Some boilerplate code was still necessary to get this working, and if it wasn't
for C++11 static initializers, there would be much more. But I think the final
result is more understandable than other options... once you get used to it.

We might want to tweak the namespaces (e.g. introduce browsing_data_type::)
instead of prefixing every item with kBrowsingDataType and keeping them in
standard namespaces. I'm also not sure about handling the content/public
interface; technically, we break the requirements, but since we're only defining
a C-style struct datatype with no methods, I hope it's acceptable to have an
implementation there.

Anyways... please have a look!

Thanks,
Martin

[commit-bot: I haz the power, 2017-02-16 22:36:14]

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

[commit-bot: I haz the power, 2017-02-16 22:36:15]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[msramek, 2017-02-17 09:50:53]

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

[commit-bot: I haz the power, 2017-02-17 09:51:19]

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-17 10:18:33]

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

[commit-bot: I haz the power, 2017-02-17 10:18:34]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[msramek, 2017-02-17 11:01:37]

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

[commit-bot: I haz the power, 2017-02-17 11:02:11]

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

[Bernhard Bauer, 2017-02-17 11:10:44]

Nice!

https://codereview.chromium.org/2697123004/diff/40001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover_impl.cc (right):

https://codereview.chromium.org/2697123004/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_impl.cc:123: if (data_type &&
!data_type->filterable)
Why the null check?

https://codereview.chromium.org/2697123004/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_impl.cc:130: inline bool
MaskContains(const std::set<const BrowsingDataType*>& remove_mask,
You could use base::ContainsValue (from base/stl_util.h) instead of this.

https://codereview.chromium.org/2697123004/diff/40001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover_impl_unittest.cc
(right):

https://codereview.chromium.org/2697123004/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_impl_unittest.cc:952: {
&content::kBrowsingDataTypeDownloads,
Is this what clang-format produces? Sigh...

https://codereview.chromium.org/2697123004/diff/40001/chrome/browser/browsing...
File chrome/browser/browsing_data/chrome_browsing_data_types.cc (right):

https://codereview.chromium.org/2697123004/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/chrome_browsing_data_types.cc:98: const
std::set<const content::BrowsingDataType*>& kBrowsingDataTypeSetSiteData =
So, what you are saying is that this does not create static initializer code due
to the magic of C++11?

https://codereview.chromium.org/2697123004/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/browsing_data/browsing_data_api.cc (right):

https://codereview.chromium.org/2697123004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/browsing_data/browsing_data_api.cc:287:
EXTENSION_FUNCTION_VALIDATE(GetRemovalMask(&removal_mask_));
You could clear the mask beforehand (unless it already is empty anyway at that
point, if the function was freshly created?), then you don't need that in every
one of the implementations.

https://codereview.chromium.org/2697123004/diff/40001/content/public/browser/...
File content/public/browser/browsing_data_types.cc (right):

https://codereview.chromium.org/2697123004/diff/40001/content/public/browser/...
content/public/browser/browsing_data_types.cc:5: #include
"content/public/browser/browsing_data_types.h"
Hm... would it work to have the definition of these outside of the public
interface and only declare them in the header, or would that create a problem
with component builds? I want to say it should work, because `extern` makes the
symbol not different from e.g. a method definition, and those can definitely be
declared in a public header but implemented only in non-public files.

[commit-bot: I haz the power, 2017-02-17 11:28:45]

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

[commit-bot: I haz the power, 2017-02-17 11:28:48]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[msramek, 2017-02-17 17:45:12]

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

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

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

[msramek, 2017-02-17 17:46:18]

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

[commit-bot: I haz the power, 2017-02-17 17:46:58]

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

[msramek, 2017-02-17 18:00:20]

https://codereview.chromium.org/2697123004/diff/40001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover_impl.cc (right):

https://codereview.chromium.org/2697123004/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_impl.cc:123: if (data_type &&
!data_type->filterable)
On 2017/02/17 11:10:43, Bernhard Bauer wrote:
> Why the null check?

In browsing_data_api.cc, there is one place where we need a "datatype or
nothing", which I replaced with "BrowsingDataType non-null or null".

I rewrote that part a bit so that we can DCHECK here.

https://codereview.chromium.org/2697123004/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_impl.cc:130: inline bool
MaskContains(const std::set<const BrowsingDataType*>& remove_mask,
On 2017/02/17 11:10:43, Bernhard Bauer wrote:
> You could use base::ContainsValue (from base/stl_util.h) instead of this.

Done. Thanks!

https://codereview.chromium.org/2697123004/diff/40001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover_impl_unittest.cc
(right):

https://codereview.chromium.org/2697123004/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_impl_unittest.cc:952: {
&content::kBrowsingDataTypeDownloads,
On 2017/02/17 11:10:43, Bernhard Bauer wrote:
> Is this what clang-format produces? Sigh...

*cough* that might have been me *cough*

Sorry for that. I remembered multiline initializers being offset on the second
and next line somwehere (which might have been the behavior of clang-format or
unformatted code) and I reproduced it here. I planned to take another pass on
these things before uploading the CL, but I forgot. I was reluctant to run
clang-format in a CL like this due to it touching some unrelated and correctly
formatted code, and increasing the probability of merge conflicts.

I ran it now. It fixed the style in many places. It ruined the style in other
places. But the delta seems to be positive, so I'll keep it.

https://codereview.chromium.org/2697123004/diff/40001/chrome/browser/browsing...
File chrome/browser/browsing_data/chrome_browsing_data_types.cc (right):

https://codereview.chromium.org/2697123004/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/chrome_browsing_data_types.cc:98: const
std::set<const content::BrowsingDataType*>& kBrowsingDataTypeSetSiteData =
On 2017/02/17 11:10:43, Bernhard Bauer wrote:
> So, what you are saying is that this does not create static initializer code
due
> to the magic of C++11?

No, I totally misspoke here, sorry.

I wanted to praise C++11 for its list initializers, not static initializers,
i.e. the ability to use braces to construct a set. I updated the CL description.

These things here are definitely static. Note also that I'm using LazyInstance
pretty much only for code style purposes. I considered these approaches:

a) CR_DEFINE_STATIC_LOCAL should not be used because this is not local, and it
also doesn't allow complicated (non-constexpr) definitions.

b)

namespace {
std::set<>* all = new ...;
}
kBrowsingDataTypeAll = *all;

But random "new" operators also don't seem to be in line with the code style and
sanitizers are probably not going to like them.

c) base::LazyInstance seems to be the preferred way to go about static
initializations, but in our case it's not lazy; Get() is called right below.

I'm happy to hear your thoughts if there's a better way :) I don't have much
experience with this.

https://codereview.chromium.org/2697123004/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/browsing_data/browsing_data_api.cc (right):

https://codereview.chromium.org/2697123004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/browsing_data/browsing_data_api.cc:287:
EXTENSION_FUNCTION_VALIDATE(GetRemovalMask(&removal_mask_));
On 2017/02/17 11:10:44, Bernhard Bauer wrote:
> You could clear the mask beforehand (unless it already is empty anyway at that
> point, if the function was freshly created?), then you don't need that in
every
> one of the implementations.

Done. Good point.

When I have some spare cycles™, I'll want to clean this file up a bit, since
there is a lot of repetitive code.

https://codereview.chromium.org/2697123004/diff/40001/content/public/browser/...
File content/public/browser/browsing_data_types.cc (right):

https://codereview.chromium.org/2697123004/diff/40001/content/public/browser/...
content/public/browser/browsing_data_types.cc:5: #include
"content/public/browser/browsing_data_types.h"
On 2017/02/17 11:10:44, Bernhard Bauer wrote:
> Hm... would it work to have the definition of these outside of the public
> interface and only declare them in the header, or would that create a problem
> with component builds? I want to say it should work, because `extern` makes
the
> symbol not different from e.g. a method definition, and those can definitely
be
> declared in a public header but implemented only in non-public files.

Yes, I think you're right.

The problem with content/public interface that I mentioned had to with the
standard pattern:
content/public/*.h -> pure virtual
content/*impl.[h|cc] -> implementation

If we broke BrowsingDataType into a pure virtual BrowsingDataType and
BrowsingDataTypeImpl, things would get a bit more complicated.

But moving just the .cc file should be fine.

[commit-bot: I haz the power, 2017-02-17 19:00:01]

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

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

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

[msramek, 2017-02-20 14:12:10]

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

[commit-bot: I haz the power, 2017-02-20 14:12:21]

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-20 14:14:41]

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

[commit-bot: I haz the power, 2017-02-20 14:14:42]

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

[Bernhard Bauer, 2017-02-20 14:19:16]

https://codereview.chromium.org/2697123004/diff/40001/chrome/browser/browsing...
File chrome/browser/browsing_data/chrome_browsing_data_types.cc (right):

https://codereview.chromium.org/2697123004/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/chrome_browsing_data_types.cc:98: const
std::set<const content::BrowsingDataType*>& kBrowsingDataTypeSetSiteData =
On 2017/02/17 18:00:20, msramek wrote:
> On 2017/02/17 11:10:43, Bernhard Bauer wrote:
> > So, what you are saying is that this does not create static initializer code
> due
> > to the magic of C++11?
> 
> No, I totally misspoke here, sorry.
> 
> I wanted to praise C++11 for its list initializers, not static initializers,
> i.e. the ability to use braces to construct a set. I updated the CL
description.
> 
> These things here are definitely static. Note also that I'm using LazyInstance
> pretty much only for code style purposes. I considered these approaches:
> 
> a) CR_DEFINE_STATIC_LOCAL should not be used because this is not local, and it
> also doesn't allow complicated (non-constexpr) definitions.
> 
> b)
> 
> namespace {
> std::set<>* all = new ...;
> }
> kBrowsingDataTypeAll = *all;
> 
> But random "new" operators also don't seem to be in line with the code style
and
> sanitizers are probably not going to like them.
> 
> c) base::LazyInstance seems to be the preferred way to go about static
> initializations, but in our case it's not lazy; Get() is called right below.
> 
> I'm happy to hear your thoughts if there's a better way :) I don't have much
> experience with this.

So, primarily my problem with this right now is that it does create static
initializer code, which is disallowed, and AFAIK checked by at least one bot
(not all of them though, as it requires instrumentation or fiddling with clang
or something, so this might not show up immediately). I think there is going to
be no way to get a static instance of a std::set that you can just access as a
constant without the compiler creating these initializers. Your options are:
1) Writing methods that return these -- essentially, wrap these lines into a
method each, which will take care of the initialization the first time they're
called.
2) Only expose the LazyInstance to clients and have them get the sets from
there. You could also use a separate LazyInstance for each of them.

Out of those options, I would probably prefer the first one.

[msramek, 2017-02-20 15:37:20]

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

[commit-bot: I haz the power, 2017-02-20 15:37: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-20 15:48:15]

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

[commit-bot: I haz the power, 2017-02-20 15:48:16]

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

[msramek, 2017-02-20 16:01:40]

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

[commit-bot: I haz the power, 2017-02-20 16:01:52]

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-20 16:33:18]

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

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

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[msramek, 2017-02-20 17:00:27]

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

[commit-bot: I haz the power, 2017-02-20 17:00:44]

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

[msramek, 2017-02-20 17:11:17]

I fixed a few more red tests, rebased (merge conflict), and addressed your
comment :)

https://codereview.chromium.org/2697123004/diff/40001/chrome/browser/browsing...
File chrome/browser/browsing_data/chrome_browsing_data_types.cc (right):

https://codereview.chromium.org/2697123004/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/chrome_browsing_data_types.cc:98: const
std::set<const content::BrowsingDataType*>& kBrowsingDataTypeSetSiteData =
On 2017/02/20 14:19:16, Bernhard Bauer wrote:
> On 2017/02/17 18:00:20, msramek wrote:
> > On 2017/02/17 11:10:43, Bernhard Bauer wrote:
> > > So, what you are saying is that this does not create static initializer
code
> > due
> > > to the magic of C++11?
> > 
> > No, I totally misspoke here, sorry.
> > 
> > I wanted to praise C++11 for its list initializers, not static initializers,
> > i.e. the ability to use braces to construct a set. I updated the CL
> description.
> > 
> > These things here are definitely static. Note also that I'm using
LazyInstance
> > pretty much only for code style purposes. I considered these approaches:
> > 
> > a) CR_DEFINE_STATIC_LOCAL should not be used because this is not local, and
it
> > also doesn't allow complicated (non-constexpr) definitions.
> > 
> > b)
> > 
> > namespace {
> > std::set<>* all = new ...;
> > }
> > kBrowsingDataTypeAll = *all;
> > 
> > But random "new" operators also don't seem to be in line with the code style
> and
> > sanitizers are probably not going to like them.
> > 
> > c) base::LazyInstance seems to be the preferred way to go about static
> > initializations, but in our case it's not lazy; Get() is called right below.
> > 
> > I'm happy to hear your thoughts if there's a better way :) I don't have much
> > experience with this.
> 
> So, primarily my problem with this right now is that it does create static
> initializer code, which is disallowed, and AFAIK checked by at least one bot
> (not all of them though, as it requires instrumentation or fiddling with clang
> or something, so this might not show up immediately). I think there is going
to
> be no way to get a static instance of a std::set that you can just access as a
> constant without the compiler creating these initializers. Your options are:
> 1) Writing methods that return these -- essentially, wrap these lines into a
> method each, which will take care of the initialization the first time they're
> called.
> 2) Only expose the LazyInstance to clients and have them get the sets from
> there. You could also use a separate LazyInstance for each of them.
> 
> Out of those options, I would probably prefer the first one. 

Done. I changed them to methods.

My original goal is to have them as constants so that they're similar to
individual datatypes, but that's actually probably not even a good thing. No
need to copy the old world.

[Bernhard Bauer, 2017-02-20 17:15:18]

LGTM!

[msramek, 2017-02-20 17:22:33]

Wow, that was much faster than I expected with this CL :) Thanks, Bernhard! Also
for your idea!

[msramek, 2017-02-20 17:29:16]

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

[msramek, 2017-02-20 17:29:17]

+jam@ PTAL at content/public interface

+dbeam@ PTAL at options/ change (it's an equivalent replacement, but not exactly
line by line)

[msramek, 2017-02-20 17:30:10]

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

[msramek, 2017-02-20 17:30:11]

+dbeam@ this time really :)

[commit-bot: I haz the power, 2017-02-20 18:10:02]

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

[commit-bot: I haz the power, 2017-02-20 18:10:04]

Dry run: This issue passed the CQ dry run.

[jam, 2017-02-21 16:15:49]

I just read the doc section about extending the enum.

Extending an enum declared in content/ by embedders is something that's common.
e.g. RESULT_CODE_LAST_CODE, NOTIFICATION_CONTENT_END, PROCESS_TYPE_CONTENT_END
off the top of my head (there are more). That seems simpler than this approach.

The cons listed in the doc don't seem like real problems:
-i.e. content provides an _LAST or _END, and the embedder starts their enum from
that, so no static asserts are needed
-don't see that it's a con that they refer to some enum values with content::
while others are chrome::..., this happens with other enums or data types across
different layers of the codebase all the time.

[msramek, 2017-02-21 19:35:32]

On 2017/02/21 16:15:49, jam (OOO) wrote:
> I just read the doc section about extending the enum.
> 
> Extending an enum declared in content/ by embedders is something that's
common.
> e.g. RESULT_CODE_LAST_CODE, NOTIFICATION_CONTENT_END, PROCESS_TYPE_CONTENT_END
> off the top of my head (there are more). That seems simpler than this
approach.
> 
> The cons listed in the doc don't seem like real problems:
> -i.e. content provides an _LAST or _END, and the embedder starts their enum
from
> that, so no static asserts are needed

My worry was that our enum is used for bitmasks, i.e. it takes powers of two as
values. Therefore, we only have 32 values available on 32-bit systems. I'm not
sure if it's allowed to use wider enums in Chromium, I didn't find anything in
CodeSearch:
https://cs.chromium.org/search/?q=enum:.*64+file:.*%5C.(h%7Ccc)$&type=cs

We're already at around 20 datatypes. Even if we're conservative in Chrome and
manage to stay under 32, there could be an embedder that requires more datatypes
and suggests a refactoring.

It could also happen that we add a datatype to content/ and unknowingly shift an
embedder that was at 32 into overflow - that's why I wanted every embedder to
have a static_assert.

> -don't see that it's a con that they refer to some enum values with content::
> while others are chrome::..., this happens with other enums or data types
across
> different layers of the codebase all the time.

I guess this could be a matter of taste. The point of BrowsingDataRemover is
among other things that callers only need to know which datatype they want to
delete, and they don't need to know all the storage backends where it is stored.
In this spirit, I also didn't want them to know that history is a Chrome concept
but cache is a platform concept.

So I think we have a bit different usecase from the enums that you mentioned,
but if you're not convinced, I will of course follow your guidance.

[jam, 2017-02-21 20:50:12]

On 2017/02/21 19:35:32, msramek wrote:
> On 2017/02/21 16:15:49, jam (OOO) wrote:
> > I just read the doc section about extending the enum.
> > 
> > Extending an enum declared in content/ by embedders is something that's
> common.
> > e.g. RESULT_CODE_LAST_CODE, NOTIFICATION_CONTENT_END,
PROCESS_TYPE_CONTENT_END
> > off the top of my head (there are more). That seems simpler than this
> approach.
> > 
> > The cons listed in the doc don't seem like real problems:
> > -i.e. content provides an _LAST or _END, and the embedder starts their enum
> from
> > that, so no static asserts are needed
> 
> My worry was that our enum is used for bitmasks, i.e. it takes powers of two
as
> values. Therefore, we only have 32 values available on 32-bit systems. I'm not
> sure if it's allowed to use wider enums in Chromium, I didn't find anything in
> CodeSearch:
> https://cs.chromium.org/search/?q=enum:.*64+file:.*%5C.(h%7Ccc)$&type=cs
> 
> We're already at around 20 datatypes. Even if we're conservative in Chrome and
> manage to stay under 32, there could be an embedder that requires more
datatypes
> and suggests a refactoring.
> 
> It could also happen that we add a datatype to content/ and unknowingly shift
an
> embedder that was at 32 into overflow - that's why I wanted every embedder to
> have a static_assert.

In cases where we have values in both content and the embedder (i.e. like the
above examples), we use an integer type since otherwise you wouldn't be able to
assign a chrome:: enum to a content:: enum. So you could go with int64_t

Regarding embedders other than src/chrome: in general, src/chrome is the most
complex embedder of content. As long as it's sufficient for chrome, we can
ignore embedders not in our repo. If in the very unlikely chance that this comes
up from an embedder, we can use another data type then, but until that time
going with the simplest solution is what I recommend.

> 
> > -don't see that it's a con that they refer to some enum values with
content::
> > while others are chrome::..., this happens with other enums or data types
> across
> > different layers of the codebase all the time.
> 
> I guess this could be a matter of taste. The point of BrowsingDataRemover is
> among other things that callers only need to know which datatype they want to
> delete, and they don't need to know all the storage backends where it is
stored.
> In this spirit, I also didn't want them to know that history is a Chrome
concept
> but cache is a platform concept.

This seems like an orthogonal issue; the current patchset accomplishes this
through "using" declarations in the chrome headers. That could be accomplished
with two enums as well. But even then, there's only one or two files in chrome/
that deal with both enums, so it seems like not a burden.

> 
> So I think we have a bit different usecase from the enums that you mentioned,
> but if you're not convinced, I will of course follow your guidance.

[Dan Beam, 2017-02-23 06:34:45]

lgtm but using "mask" in variable names that are now sets is weird, IMO

[msramek, 2017-03-08 13:58:32]

Sorry for the long delay.

John - you're right that the question of forwarding content-defined enum values
to the embedder's namespace is mostly orthogonal to the approach that with take
to extend the enum.

I rewrote this CL from scratch, this time using enums again. I bundled it with
an equivalent change to OriginTypeMasks to avoid making two CLs that touch
exactly the same files for the same reasons. The new CL is at
https://codereview.chromium.org/2733393003/ - please have a look!

Dan - thanks, that's a good point. The new approach fortunately uses masks
again, so I didn't have to rename everything :)