Move static RegistrableDomainFilterBuilder methods to an anonymous namespace

Move static RegistrableDomainFilterBuilder methods to an anonymous namespace

The static methods that BrowsingDataFilterBuilder wraps in base::Callback
and passes as filters do not need to be members of the class. This CL
moves them to an anonymous namespace.

Furthermore, we replace the naked pointers which are passed to these methods
with std::move() of a locally allocated object.

This class is extensively used in BrowsingDataRemoverUnittest, which
provides the test coverage for this change.

BUG=None

Committed: https://crrev.com/939972b08603666c25471a1d5e68a2de298e3fad
Cr-Commit-Position: refs/heads/master@{#436948}

[msramek, 2016-08-19 10:58:11]

Description was changed from

==========
Move static RegistrableDomainFilterBuilder methods to an anonymous namespace

The static methods that BrowsingDataFilterBuilder wraps in base::Callback
and passes as filters do not need to be members of the class. This CL
moves them to an anonymous namespace.

Furthermore, we replace the naked pointers which are passed to these methods
with std::move(std::unique_ptr).

This class is extensively used in BrowsingDataRemoverUnittest, which
provides the test coverage for this change.

BUG=None
==========

to

==========
Move static RegistrableDomainFilterBuilder methods to an anonymous namespace

The static methods that BrowsingDataFilterBuilder wraps in base::Callback
and passes as filters do not need to be members of the class. This CL
moves them to an anonymous namespace.

Furthermore, we replace the naked pointers which are passed to these methods
with std::move() of a locally allocated object.

This class is extensively used in BrowsingDataRemoverUnittest, which
provides the test coverage for this change.

BUG=None
==========

[msramek, 2016-08-19 10:58:17]

Patchset #1 (id:1) has been deleted

[msramek, 2016-08-19 10:58:21]

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

[commit-bot: I haz the power, 2016-08-19 10:58: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, 2016-08-19 11:39:52]

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

[commit-bot: I haz the power, 2016-08-19 11:39:53]

Dry run: This issue passed the CQ dry run.

[msramek, 2016-09-13 15:06:49]

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

[msramek, 2016-09-13 15:06:49]

Hi Bernhard!

I extracted these changes from CL https://codereview.chromium.org/2248403002/
where you requested them during the review, but never actually sent it to you...

Please review at your leisure :) (this is totally low priority; especially since
it's perf time...).

Thanks,
Martin

[msramek, 2016-11-23 14:18:42]

Hi Bernhard!

Mind looking at this super-old CL? :-) It's still low priority etc., but we
might want to land it before we start the content migration of
BrowsingDataRemover.

Thanks!
Martin

[Bernhard Bauer, 2016-11-23 15:53:07]

Oops, turns out I had an old draft comment... :)

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

https://codereview.chromium.org/2261673002/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/registrable_domain_filter_builder.cc:139: return
base::Bind(MatchesURL, domains, mode());
This will copy the set. That could be fine if you're okay with it, but if you
want to avoid the copy, I think you'll need base::Passed(std::move(domains))
(the std::move() to move it into the callback, and base::Passed() to have the
callback move it when running).

[msramek, 2016-11-24 17:26:19]

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

[commit-bot: I haz the power, 2016-11-24 17:26:44]

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

[msramek, 2016-11-24 17:27:25]

Patchset #4 (id:80001) has been deleted

[commit-bot: I haz the power, 2016-11-24 17:29:14]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[msramek, 2016-11-24 17:31:40]

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

[commit-bot: I haz the power, 2016-11-24 17:32:36]

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

[msramek, 2016-11-24 17:38:08]

Heh :) I wonder if I could have discovered that from the "Last updated time"
(which does reflect all sorts of changes, I'm not sure if also added and
unpublished drafts).

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

https://codereview.chromium.org/2261673002/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/registrable_domain_filter_builder.cc:139: return
base::Bind(MatchesURL, domains, mode());
On 2016/11/23 15:53:07, Bernhard Bauer wrote:
> This will copy the set. That could be fine if you're okay with it, but if you
> want to avoid the copy, I think you'll need base::Passed(std::move(domains))
> (the std::move() to move it into the callback, and base::Passed() to have the
> callback move it when running).

The copy on the previous line (|domain_list_|->|domains|) should be a copy - the
Build*() methods should not destroy the Builder's internal state, as that would
be unexpected for the builder design pattern.

The copy of |domains| on this line can be a move, since |domains| is just a
local variable that's not used later.

We should not use base::Passed(), as the callback would let go of |domains| and
we could not call ::Run() twice, which we definitely do. But that's not a
problem - you can see that |domains| are bound to "const std::set<std::string>&"
in ::MatchesURL, so that's not an additional copy.

Since the first two operations are copy + move, I just got rid of the
intermediate step and pass |domain_list_| directly into base::Bind().

And since I am now confused why it's even called |domain_list_| when it's a set,
I renamed it to |domains_|.

[commit-bot: I haz the power, 2016-11-24 18:36:03]

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

[commit-bot: I haz the power, 2016-11-24 18:36:04]

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

[Bernhard Bauer, 2016-11-25 13:53:20]

LGTM

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

https://codereview.chromium.org/2261673002/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/registrable_domain_filter_builder.cc:139: return
base::Bind(MatchesURL, domains, mode());
On 2016/11/24 17:38:08, msramek wrote:
> On 2016/11/23 15:53:07, Bernhard Bauer wrote:
> > This will copy the set. That could be fine if you're okay with it, but if
you
> > want to avoid the copy, I think you'll need base::Passed(std::move(domains))
> > (the std::move() to move it into the callback, and base::Passed() to have
the
> > callback move it when running).
> 
> The copy on the previous line (|domain_list_|->|domains|) should be a copy -
the
> Build*() methods should not destroy the Builder's internal state, as that
would
> be unexpected for the builder design pattern.
> 
> The copy of |domains| on this line can be a move, since |domains| is just a
> local variable that's not used later.
> 
> We should not use base::Passed(), as the callback would let go of |domains|
and
> we could not call ::Run() twice, which we definitely do. But that's not a
> problem - you can see that |domains| are bound to "const
std::set<std::string>&"
> in ::MatchesURL, so that's not an additional copy.
> 
> Since the first two operations are copy + move, I just got rid of the
> intermediate step and pass |domain_list_| directly into base::Bind().
> 
> And since I am now confused why it's even called |domain_list_| when it's a
set,
> I renamed it to |domains_|.

Ok, that makes sense. Thanks!

Actually, now that you mention calling the callback repeatedly: should we go
ahead and use base::RepeatingCallback?

[msramek, 2016-11-25 17:03:43]

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

https://codereview.chromium.org/2261673002/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/registrable_domain_filter_builder.cc:139: return
base::Bind(MatchesURL, domains, mode());
On 2016/11/25 13:53:19, Bernhard Bauer wrote:
> On 2016/11/24 17:38:08, msramek wrote:
> > On 2016/11/23 15:53:07, Bernhard Bauer wrote:
> > > This will copy the set. That could be fine if you're okay with it, but if
> you
> > > want to avoid the copy, I think you'll need
base::Passed(std::move(domains))
> > > (the std::move() to move it into the callback, and base::Passed() to have
> the
> > > callback move it when running).
> > 
> > The copy on the previous line (|domain_list_|->|domains|) should be a copy -
> the
> > Build*() methods should not destroy the Builder's internal state, as that
> would
> > be unexpected for the builder design pattern.
> > 
> > The copy of |domains| on this line can be a move, since |domains| is just a
> > local variable that's not used later.
> > 
> > We should not use base::Passed(), as the callback would let go of |domains|
> and
> > we could not call ::Run() twice, which we definitely do. But that's not a
> > problem - you can see that |domains| are bound to "const
> std::set<std::string>&"
> > in ::MatchesURL, so that's not an additional copy.
> > 
> > Since the first two operations are copy + move, I just got rid of the
> > intermediate step and pass |domain_list_| directly into base::Bind().
> > 
> > And since I am now confused why it's even called |domain_list_| when it's a
> set,
> > I renamed it to |domains_|.
> 
> Ok, that makes sense. Thanks!
> 
> Actually, now that you mention calling the callback repeatedly: should we go
> ahead and use base::RepeatingCallback?

Well, I see three options:
1. Status quo. Don't be explicit, just rely on the fact that base::Callback and
base::RepeatingCallback are the same thing.
2. Change this to base::RepeatingCallback here, in BrowsingDataRemover, and all
BrowsingDataRemover's backends and their subclasses and modules and... could be
on the order of hundred callsites.
3. Change it to a RepeatingCallback just here.

I don't want to do #2, at least not now. According to
https://cs.chromium.org/chromium/src/base/bind.h?q=bind.h&sq=package:chromium...
it might be desirable at some point though.

From #1 and #3, #1 (status quo) seems better to me, because we're at least
consistently using the same type.

Although I can see an argument that if base::Callback ever changed in the future
not to be equal to base::RepeatingCallback, #3 would not compile, which might be
a good thing. So if you prefer, I can do that.

[Bernhard Bauer, 2016-11-25 17:18:31]

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

https://codereview.chromium.org/2261673002/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/registrable_domain_filter_builder.cc:139: return
base::Bind(MatchesURL, domains, mode());
On 2016/11/25 17:03:43, msramek wrote:
> On 2016/11/25 13:53:19, Bernhard Bauer wrote:
> > On 2016/11/24 17:38:08, msramek wrote:
> > > On 2016/11/23 15:53:07, Bernhard Bauer wrote:
> > > > This will copy the set. That could be fine if you're okay with it, but
if
> > you
> > > > want to avoid the copy, I think you'll need
> base::Passed(std::move(domains))
> > > > (the std::move() to move it into the callback, and base::Passed() to
have
> > the
> > > > callback move it when running).
> > > 
> > > The copy on the previous line (|domain_list_|->|domains|) should be a copy
-
> > the
> > > Build*() methods should not destroy the Builder's internal state, as that
> > would
> > > be unexpected for the builder design pattern.
> > > 
> > > The copy of |domains| on this line can be a move, since |domains| is just
a
> > > local variable that's not used later.
> > > 
> > > We should not use base::Passed(), as the callback would let go of
|domains|
> > and
> > > we could not call ::Run() twice, which we definitely do. But that's not a
> > > problem - you can see that |domains| are bound to "const
> > std::set<std::string>&"
> > > in ::MatchesURL, so that's not an additional copy.
> > > 
> > > Since the first two operations are copy + move, I just got rid of the
> > > intermediate step and pass |domain_list_| directly into base::Bind().
> > > 
> > > And since I am now confused why it's even called |domain_list_| when it's
a
> > set,
> > > I renamed it to |domains_|.
> > 
> > Ok, that makes sense. Thanks!
> > 
> > Actually, now that you mention calling the callback repeatedly: should we go
> > ahead and use base::RepeatingCallback?
> 
> Well, I see three options:
> 1. Status quo. Don't be explicit, just rely on the fact that base::Callback
and
> base::RepeatingCallback are the same thing.
> 2. Change this to base::RepeatingCallback here, in BrowsingDataRemover, and
all
> BrowsingDataRemover's backends and their subclasses and modules and... could
be
> on the order of hundred callsites.
> 3. Change it to a RepeatingCallback just here.
> 
> I don't want to do #2, at least not now. According to
>
https://cs.chromium.org/chromium/src/base/bind.h?q=bind.h&sq=package:chromium...
> it might be desirable at some point though.
> 
> From #1 and #3, #1 (status quo) seems better to me, because we're at least
> consistently using the same type.
> 
> Although I can see an argument that if base::Callback ever changed in the
future
> not to be equal to base::RepeatingCallback, #3 would not compile, which might
be
> a good thing. So if you prefer, I can do that.

Yes, that's what I understand from
https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md.

I mean, you don't have to shave this particular yak in this CL, but all these
will have to be changed to RepeatingCallback eventually, so we might as well
start now.

[msramek, 2016-12-07 13:21:34]

Argh, I completely forgot about this CL, sorry.

I made the largest possible substitution that didn't explode the size of this
CL, and will proceed to land it in this state.

I'll send you a followup which does more replacements after.

Thanks!

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

https://codereview.chromium.org/2261673002/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/registrable_domain_filter_builder.cc:139: return
base::Bind(MatchesURL, domains, mode());
On 2016/11/25 17:18:31, Bernhard Bauer wrote:
> On 2016/11/25 17:03:43, msramek wrote:
> > On 2016/11/25 13:53:19, Bernhard Bauer wrote:
> > > On 2016/11/24 17:38:08, msramek wrote:
> > > > On 2016/11/23 15:53:07, Bernhard Bauer wrote:
> > > > > This will copy the set. That could be fine if you're okay with it, but
> if
> > > you
> > > > > want to avoid the copy, I think you'll need
> > base::Passed(std::move(domains))
> > > > > (the std::move() to move it into the callback, and base::Passed() to
> have
> > > the
> > > > > callback move it when running).
> > > > 
> > > > The copy on the previous line (|domain_list_|->|domains|) should be a
copy
> -
> > > the
> > > > Build*() methods should not destroy the Builder's internal state, as
that
> > > would
> > > > be unexpected for the builder design pattern.
> > > > 
> > > > The copy of |domains| on this line can be a move, since |domains| is
just
> a
> > > > local variable that's not used later.
> > > > 
> > > > We should not use base::Passed(), as the callback would let go of
> |domains|
> > > and
> > > > we could not call ::Run() twice, which we definitely do. But that's not
a
> > > > problem - you can see that |domains| are bound to "const
> > > std::set<std::string>&"
> > > > in ::MatchesURL, so that's not an additional copy.
> > > > 
> > > > Since the first two operations are copy + move, I just got rid of the
> > > > intermediate step and pass |domain_list_| directly into base::Bind().
> > > > 
> > > > And since I am now confused why it's even called |domain_list_| when
it's
> a
> > > set,
> > > > I renamed it to |domains_|.
> > > 
> > > Ok, that makes sense. Thanks!
> > > 
> > > Actually, now that you mention calling the callback repeatedly: should we
go
> > > ahead and use base::RepeatingCallback?
> > 
> > Well, I see three options:
> > 1. Status quo. Don't be explicit, just rely on the fact that base::Callback
> and
> > base::RepeatingCallback are the same thing.
> > 2. Change this to base::RepeatingCallback here, in BrowsingDataRemover, and
> all
> > BrowsingDataRemover's backends and their subclasses and modules and... could
> be
> > on the order of hundred callsites.
> > 3. Change it to a RepeatingCallback just here.
> > 
> > I don't want to do #2, at least not now. According to
> >
>
https://cs.chromium.org/chromium/src/base/bind.h?q=bind.h&sq=package:chromium...
> > it might be desirable at some point though.
> > 
> > From #1 and #3, #1 (status quo) seems better to me, because we're at least
> > consistently using the same type.
> > 
> > Although I can see an argument that if base::Callback ever changed in the
> future
> > not to be equal to base::RepeatingCallback, #3 would not compile, which
might
> be
> > a good thing. So if you prefer, I can do that.
> 
> Yes, that's what I understand from
> https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md.
> 
> I mean, you don't have to shave this particular yak in this CL, but all these
> will have to be changed to RepeatingCallback eventually, so we might as well
> start now.

Alright! I changed all base::Bind to base::BindRepeating, so that they create a
base::RepeatingCallback. However, the methods override the ones from
BrowsingDataFilterBuilder, so I can't change the signature without changing it
in at least 3 additional files as well, which I think doesn't belong to this CL.
I'll send a followup soon.

[msramek, 2016-12-07 13:21:38]

The CQ bit was checked by msramek@chromium.org

[msramek, 2016-12-07 13:21:39]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2261673002/#ps120001
(title: "BindRepeating")

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

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-12-07 14:46:01]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1481116898614930,
"parent_rev": "7646acd51017a8040a3af3699e78c2a133561bf0", "commit_rev":
"f320064fe9021ebb351cc37614b0ff2df952f839"}

[commit-bot: I haz the power, 2016-12-07 14:46:28]

Description was changed from

==========
Move static RegistrableDomainFilterBuilder methods to an anonymous namespace

The static methods that BrowsingDataFilterBuilder wraps in base::Callback
and passes as filters do not need to be members of the class. This CL
moves them to an anonymous namespace.

Furthermore, we replace the naked pointers which are passed to these methods
with std::move() of a locally allocated object.

This class is extensively used in BrowsingDataRemoverUnittest, which
provides the test coverage for this change.

BUG=None
==========

to

==========
Move static RegistrableDomainFilterBuilder methods to an anonymous namespace

The static methods that BrowsingDataFilterBuilder wraps in base::Callback
and passes as filters do not need to be members of the class. This CL
moves them to an anonymous namespace.

Furthermore, we replace the naked pointers which are passed to these methods
with std::move() of a locally allocated object.

This class is extensively used in BrowsingDataRemoverUnittest, which
provides the test coverage for this change.

BUG=None
==========

[commit-bot: I haz the power, 2016-12-07 14:46:29]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-12-07 14:48:30]

Description was changed from

==========
Move static RegistrableDomainFilterBuilder methods to an anonymous namespace

The static methods that BrowsingDataFilterBuilder wraps in base::Callback
and passes as filters do not need to be members of the class. This CL
moves them to an anonymous namespace.

Furthermore, we replace the naked pointers which are passed to these methods
with std::move() of a locally allocated object.

This class is extensively used in BrowsingDataRemoverUnittest, which
provides the test coverage for this change.

BUG=None
==========

to

==========
Move static RegistrableDomainFilterBuilder methods to an anonymous namespace

The static methods that BrowsingDataFilterBuilder wraps in base::Callback
and passes as filters do not need to be members of the class. This CL
moves them to an anonymous namespace.

Furthermore, we replace the naked pointers which are passed to these methods
with std::move() of a locally allocated object.

This class is extensively used in BrowsingDataRemoverUnittest, which
provides the test coverage for this change.

BUG=None

Committed: https://crrev.com/939972b08603666c25471a1d5e68a2de298e3fad
Cr-Commit-Position: refs/heads/master@{#436948}
==========

[commit-bot: I haz the power, 2016-12-07 14:48:30]

Patchset 5 (id:??) landed as
https://crrev.com/939972b08603666c25471a1d5e68a2de298e3fad
Cr-Commit-Position: refs/heads/master@{#436948}