Add origin-based deletion to BrowsingDataRemover

Add origin-based history removal to BrowsingDataRemover

This is the first of what will be many CLs to add origin-based deletion to
BrowsingDataRemover for all the variety of data types it supports. As a first
step, this CL hides origin-based functionality in the private
`BrowsingDataRemover::RemoveImpl()`. Once we're done with all the relevant data
types, we can add a `RemoveOrigin(int, GURL)` method next to `Remove(int)` to
expose a public API.

BUG=113965
TEST=unit_tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=122497

[Mike West, 2012-02-10 14:17:53]

Hey Jochen, Bernhard. How do you feel about this approach to extending the
`browsingData` extension API? This is a just-barely-started strawman, there's no
point in reviewing the code, just the approach:

I basically have two options:

1. Extend BrowsingDataRemover to support origin-based deletion.
2. Rewrite most of BrowsingDataRemover inside `extension_browsing_data_api.cc`.

#1 is preferable to me... not sure how well it fits with the current
understanding of the class.

Alternatives also welcome. :)

Thanks!

[jochen (gone - plz use gerrit), 2012-02-10 18:52:33]

On 2012/02/10 14:17:53, Mike West (chromium) wrote:
> Hey Jochen, Bernhard. How do you feel about this approach to extending the
> `browsingData` extension API? This is a just-barely-started strawman, there's
no
> point in reviewing the code, just the approach:
> 
> I basically have two options:
> 
> 1. Extend BrowsingDataRemover to support origin-based deletion.

I think (1) would be very nice. It would essentially be "forget about this
site", and we might want to surface this functionality at different places.

> 2. Rewrite most of BrowsingDataRemover inside
`extension_browsing_data_api.cc`.
> 
> #1 is preferable to me... not sure how well it fits with the current
> understanding of the class.
> 
> Alternatives also welcome. :)
> 
> Thanks!

[Bernhard Bauer, 2012-02-10 19:50:57]

On 2012/02/10 18:52:33, jochen wrote:
> On 2012/02/10 14:17:53, Mike West (chromium) wrote:
> > Hey Jochen, Bernhard. How do you feel about this approach to extending the
> > `browsingData` extension API? This is a just-barely-started strawman,
there's
> no
> > point in reviewing the code, just the approach:
> > 
> > I basically have two options:
> > 
> > 1. Extend BrowsingDataRemover to support origin-based deletion.
> 
> I think (1) would be very nice. It would essentially be "forget about this
> site", and we might want to surface this functionality at different places.

Yes please :) Some comments below:

http://codereview.chromium.org/9379008/diff/1/chrome/browser/browsing_data_re...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/9379008/diff/1/chrome/browser/browsing_data_re...
chrome/browser/browsing_data_remover.cc:186: // historical information.
This means of course we have to be careful, as there could still be information
about a site somewhere if we don't delete history for all origins.

http://codereview.chromium.org/9379008/diff/1/chrome/browser/browsing_data_re...
File chrome/browser/browsing_data_remover.h (right):

http://codereview.chromium.org/9379008/diff/1/chrome/browser/browsing_data_re...
chrome/browser/browsing_data_remover.h:125: void Remove(int remove_mask, const
GURL& origin);
Name this method RemoveOrigin so that you don't overload Remove?

[Mike West, 2012-02-13 12:50:22]

>
http://codereview.chromium.org/9379008/diff/1/chrome/browser/browsing_data_re...
> chrome/browser/browsing_data_remover.cc:186: // historical information.
> This means of course we have to be careful, as there could still be
information
> about a site somewhere if we don't delete history for all origins.

What would you suggest? We clear speculative networking data, which isn't really
host-specific, auto-generated keywords, recently closed tabs (which certainly
aren't host-specific), the previous session (ditto), and prerendered data.

Should we just clear them all, all the time, even if I'm just "forgetting this
site"?

[Bernhard Bauer, 2012-02-13 13:17:40]

On 2012/02/13 12:50:22, Mike West (chromium) wrote:
> >
>
http://codereview.chromium.org/9379008/diff/1/chrome/browser/browsing_data_re...
> > chrome/browser/browsing_data_remover.cc:186: // historical information.
> > This means of course we have to be careful, as there could still be
> information
> > about a site somewhere if we don't delete history for all origins.
> 
> What would you suggest? We clear speculative networking data, which isn't
really
> host-specific, auto-generated keywords, recently closed tabs (which certainly
> aren't host-specific), the previous session (ditto), and prerendered data.

Why aren't recently closed tabs host-specific? Likewise, speculative networking
data *could* include information like "the user visited site A from site B".

> Should we just clear them all, all the time, even if I'm just "forgetting this
> site"?

I'm not sure actually. What I meant with the comment is that we should at least
make it clear to the user (or the client of the API) that this might not delete
all data related to the site.

[Mike West, 2012-02-13 15:56:04]

On 2012/02/13 13:17:40, Bernhard Bauer wrote:
> On 2012/02/13 12:50:22, Mike West (chromium) wrote:
> > >
> >
>
http://codereview.chromium.org/9379008/diff/1/chrome/browser/browsing_data_re...
> > > chrome/browser/browsing_data_remover.cc:186: // historical information.
> > > This means of course we have to be careful, as there could still be
> > information
> > > about a site somewhere if we don't delete history for all origins.
> > 
> > What would you suggest? We clear speculative networking data, which isn't
> really
> > host-specific, auto-generated keywords, recently closed tabs (which
certainly
> > aren't host-specific), the previous session (ditto), and prerendered data.
> 
> Why aren't recently closed tabs host-specific? Likewise, speculative
networking
> data *could* include information like "the user visited site A from site B".
> 
> > Should we just clear them all, all the time, even if I'm just "forgetting
this
> > site"?
> 
> I'm not sure actually. What I meant with the comment is that we should at
least
> make it clear to the user (or the client of the API) that this might not
delete
> all data related to the site.

How's this for a compromise:

* autogenerated keywords stick around forever. We should clear them out when we
clear an origin's data.

* Network history isn't marked with a time we can use anyway, so we're already
clearing too much. Might as well always clear it out: better safe than sorry.

* Remove prerendered pages all the time: the user never requested them anyway,
so there's no real value in walking through the list, looking for a specific
origin.

* Since recently closed tabs expire in one day, the potential window of
opportunity is small. We should clear them out when we clear all origins, but
not worry about iterating through each looking for a specific origin's data.
This should be called out in the extension documentation when we get to that
point.

* Likewise, we should only remove the last session when clearing all history.

That should be what this CL does in its current state. WDYT?

[Mike West, 2012-02-16 16:29:46]

Ever so friendly ping.

[Bernhard Bauer, 2012-02-16 17:23:30]

Apart from a nit below, LGTM!

On 2012/02/13 15:56:04, Mike West (chromium) wrote:
> On 2012/02/13 13:17:40, Bernhard Bauer wrote:
> > On 2012/02/13 12:50:22, Mike West (chromium) wrote:
> > > >
> > >
> >
>
http://codereview.chromium.org/9379008/diff/1/chrome/browser/browsing_data_re...
> > > > chrome/browser/browsing_data_remover.cc:186: // historical information.
> > > > This means of course we have to be careful, as there could still be
> > > information
> > > > about a site somewhere if we don't delete history for all origins.
> > > 
> > > What would you suggest? We clear speculative networking data, which isn't
> > really
> > > host-specific, auto-generated keywords, recently closed tabs (which
> certainly
> > > aren't host-specific), the previous session (ditto), and prerendered data.
> > 
> > Why aren't recently closed tabs host-specific? Likewise, speculative
> networking
> > data *could* include information like "the user visited site A from site B".
> > 
> > > Should we just clear them all, all the time, even if I'm just "forgetting
> this
> > > site"?
> > 
> > I'm not sure actually. What I meant with the comment is that we should at
> least
> > make it clear to the user (or the client of the API) that this might not
> delete
> > all data related to the site.
> 
> How's this for a compromise:
> 
> * autogenerated keywords stick around forever. We should clear them out when
we
> clear an origin's data.
> 
> * Network history isn't marked with a time we can use anyway, so we're already
> clearing too much. Might as well always clear it out: better safe than sorry.
> 
> * Remove prerendered pages all the time: the user never requested them anyway,
> so there's no real value in walking through the list, looking for a specific
> origin.
> 
> * Since recently closed tabs expire in one day, the potential window of
> opportunity is small. We should clear them out when we clear all origins, but
> not worry about iterating through each looking for a specific origin's data.
> This should be called out in the extension documentation when we get to that
> point.
> 
> * Likewise, we should only remove the last session when clearing all history.
> 
> That should be what this CL does in its current state. WDYT?

 Sounds good.

http://codereview.chromium.org/9379008/diff/13001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_remover_unittest.cc (right):

http://codereview.chromium.org/9379008/diff/13001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover_unittest.cc:55: } // Namespace
Nit: two spaces before //, and namespace in lowercase.

[Mike West, 2012-02-16 17:37:35]

Hi Peter, you won the `chrome/browser/search_engine/OWNERS` coin flip. :)

Would you mind taking a look at this CL's proposed changes to add a
`TemplateURLService::RemoveAutoGeneratedForOriginBetween` method to support
host-based history deletion via `BrowsingDataRemover`?

Thanks!

[Peter Kasting, 2012-02-16 18:16:35]

search_engines LGTM

http://codereview.chromium.org/9379008/diff/19003/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service.cc (right):

http://codereview.chromium.org/9379008/diff/19003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:325: void
TemplateURLService::RemoveAutoGeneratedSince(base::Time created_after) {
Nit: I suggest implementing by chaining the call to the 2-arg version of
RemoveAutoGeneratedBetween() rather than the 3-arg version, which will make
future maintenance a tiny fraction easier.

If you do that it might also be fractionally clearer to reverse the order of
these first two functions here and in the header.

http://codereview.chromium.org/9379008/diff/19003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:332: const GURL& origin) {
Nit: If you're going to name the function this way then put the arguments in the
same order -- first |origin|, then the times.

http://codereview.chromium.org/9379008/diff/19003/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service_unittest.cc (right):

http://codereview.chromium.org/9379008/diff/19003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_unittest.cc:478:
TemplateURLService::GenerateKeyword(GURL("http://blah/foo"), false));
Nit: While here, this line is >80 cols

http://codereview.chromium.org/9379008/diff/19003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_unittest.cc:509:
TEST_F(TemplateURLServiceTest, ClearBrowsingData_Keywords) {
While here, can you also add a test for the 2-arg RemoveAutoGeneratedBetween()? 
This function only tests the "...Since()" functionality.

http://codereview.chromium.org/9379008/diff/19003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_unittest.cc:571:
TEST_F(TemplateURLServiceTest, ClearBrowsingData_KeywordsForOrigin) {
Nit: It might be nice to expand this test to have some cases that check that we
correctly enforce both the origin and date restrictions simultaneously.

[Use mkwst_at_chromium.org plz., 2012-02-16 18:35:02]

Thanks for the (stunningly fast) review!

I'll fix up the nits, with one question remaining.

http://codereview.chromium.org/9379008/diff/19003/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service.cc (right):

http://codereview.chromium.org/9379008/diff/19003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:332: const GURL& origin) {
On 2012/02/16 18:16:35, Peter Kasting wrote:
> Nit: If you're going to name the function this way then put the arguments in
the
> same order -- first |origin|, then the times.

If you have a suggestion for a different name, I'm happy to change it. This was
just the first thing I came up with, not a deeply considered evaluation. :)

[Peter Kasting, 2012-02-16 18:41:39]

On 2012/02/16 18:35:02, mkwst wrote:
> Thanks for the (stunningly fast) review!
> 
> I'll fix up the nits, with one question remaining.
> 
>
http://codereview.chromium.org/9379008/diff/19003/chrome/browser/search_engin...
> File chrome/browser/search_engines/template_url_service.cc (right):
> 
>
http://codereview.chromium.org/9379008/diff/19003/chrome/browser/search_engin...
> chrome/browser/search_engines/template_url_service.cc:332: const GURL& origin)
{
> On 2012/02/16 18:16:35, Peter Kasting wrote:
> > Nit: If you're going to name the function this way then put the arguments in
> the
> > same order -- first |origin|, then the times.
> 
> If you have a suggestion for a different name, I'm happy to change it. This
was
> just the first thing I came up with, not a deeply considered evaluation. :)

I think the name is fine.  I'd just move the |origin| argument in front of the
time arguments to match.

[Use mkwst_at_chromium.org plz., 2012-02-17 10:12:36]

lgtm

Cool. Done. I'll throw it into the queue.

Thanks!

http://codereview.chromium.org/9379008/diff/19003/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service.cc (right):

http://codereview.chromium.org/9379008/diff/19003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:325: void
TemplateURLService::RemoveAutoGeneratedSince(base::Time created_after) {
On 2012/02/16 18:16:35, Peter Kasting wrote:
> Nit: I suggest implementing by chaining the call to the 2-arg version of
> RemoveAutoGeneratedBetween() rather than the 3-arg version, which will make
> future maintenance a tiny fraction easier.
> 
> If you do that it might also be fractionally clearer to reverse the order of
> these first two functions here and in the header.

Done.

http://codereview.chromium.org/9379008/diff/19003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:332: const GURL& origin) {
On 2012/02/16 18:35:03, mkwst wrote:
> On 2012/02/16 18:16:35, Peter Kasting wrote:
> > Nit: If you're going to name the function this way then put the arguments in
> the
> > same order -- first |origin|, then the times.
> 
> If you have a suggestion for a different name, I'm happy to change it. This
was
> just the first thing I came up with, not a deeply considered evaluation. :)

Done.

http://codereview.chromium.org/9379008/diff/19003/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service_unittest.cc (right):

http://codereview.chromium.org/9379008/diff/19003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_unittest.cc:478:
TemplateURLService::GenerateKeyword(GURL("http://blah/foo"), false));
On 2012/02/16 18:16:35, Peter Kasting wrote:
> Nit: While here, this line is >80 cols

Done.

http://codereview.chromium.org/9379008/diff/19003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_unittest.cc:509:
TEST_F(TemplateURLServiceTest, ClearBrowsingData_Keywords) {
On 2012/02/16 18:16:35, Peter Kasting wrote:
> While here, can you also add a test for the 2-arg
RemoveAutoGeneratedBetween()? 
> This function only tests the "...Since()" functionality.

Done.

http://codereview.chromium.org/9379008/diff/19003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_unittest.cc:571:
TEST_F(TemplateURLServiceTest, ClearBrowsingData_KeywordsForOrigin) {
On 2012/02/16 18:16:35, Peter Kasting wrote:
> Nit: It might be nice to expand this test to have some cases that check that
we
> correctly enforce both the origin and date restrictions simultaneously.

Done.

[commit-bot: I haz the power, 2012-02-17 10:12:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/9379008/23001

[commit-bot: I haz the power, 2012-02-17 12:20:19]

Change committed as 122497