Teaching BrowsingDataRemover how to delete application data.

Teaching BrowsingDataRemover how to delete application data.

Adding a parameter to BrowsingDataRemover::Remove in order to support a
"Clear app data" flag in the "Clear Browsing Data UI". Additionally, this CL pulls the implementation of DomStorageContext::DeleteDataModifiedSince out of DomStorageContext, and into BrowsingDataRemover in order to manage the complexities of decision-making in the BDR, rather than exporting them out of chrome and into webkit.

This patch only addresses localStorage/sessionStorage and quota managed data. Cookies should already ignore the special storage policy, but we'll need to examine the remainder of the types that the BDR touches.

BUG=116372
TEST=unit_tests,test_shell_tests

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

[Mike West, 2012-05-23 10:58:39]

Hi Jochen and Bernhard! Can you take a look at this patch?

Once you're happy, I'll loop in the remaining OWNERS I'll need.

[jochen (gone - plz use gerrit), 2012-05-23 11:02:20]

https://chromiumcodereview.appspot.com/10413072/diff/1/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_impl.cc (right):

https://chromiumcodereview.appspot.com/10413072/diff/1/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_impl.cc:136: void
DOMStorageContextImpl::DeleteDataModifiedSince(const base::Time& cutoff,
nit. all arguments should go on their own line

https://chromiumcodereview.appspot.com/10413072/diff/1/webkit/dom_storage/dom...
File webkit/dom_storage/dom_storage_context.cc (right):

https://chromiumcodereview.appspot.com/10413072/diff/1/webkit/dom_storage/dom...
webkit/dom_storage/dom_storage_context.cc:99: bool include_protected_origins) {
nit. all arguments on their own line

https://chromiumcodereview.appspot.com/10413072/diff/1/webkit/dom_storage/dom...
webkit/dom_storage/dom_storage_context.cc:107:
!special_storage_policy_->IsStorageProtected(infos[i].origin)) {
this will also delete data on chrome-extensions://

is that really intended, or do you just want to delete data on http/https
origins that are covered by app extends?

[Bernhard Bauer, 2012-05-23 11:13:11]

https://chromiumcodereview.appspot.com/10413072/diff/1/chrome/browser/browsin...
File chrome/browser/browsing_data_remover.h (right):

https://chromiumcodereview.appspot.com/10413072/diff/1/chrome/browser/browsin...
chrome/browser/browsing_data_remover.h:128: void Remove(int remove_mask,
RemoveOriginSet origin_set);
Is there a reason you're not just passing in a bool |include_protected_origins|?

[Mike West, 2012-05-23 11:23:50]

Thanks to you both.

https://chromiumcodereview.appspot.com/10413072/diff/1/chrome/browser/browsin...
File chrome/browser/browsing_data_remover.h (right):

https://chromiumcodereview.appspot.com/10413072/diff/1/chrome/browser/browsin...
chrome/browser/browsing_data_remover.h:128: void Remove(int remove_mask,
RemoveOriginSet origin_set);
On 2012/05/23 11:13:11, Bernhard Bauer wrote:
> Is there a reason you're not just passing in a bool
|include_protected_origins|?

Magic booleans are confusing in a public API. *shrug*  If you saw
`remover_->remove(true);`, you don't understand the impact as well as you do if
there's something descriptive there.

https://chromiumcodereview.appspot.com/10413072/diff/1/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_impl.cc (right):

https://chromiumcodereview.appspot.com/10413072/diff/1/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_impl.cc:136: void
DOMStorageContextImpl::DeleteDataModifiedSince(const base::Time& cutoff,
On 2012/05/23 11:02:21, jochen wrote:
> nit. all arguments should go on their own line

Done.

https://chromiumcodereview.appspot.com/10413072/diff/1/webkit/dom_storage/dom...
File webkit/dom_storage/dom_storage_context.cc (right):

https://chromiumcodereview.appspot.com/10413072/diff/1/webkit/dom_storage/dom...
webkit/dom_storage/dom_storage_context.cc:99: bool include_protected_origins) {
On 2012/05/23 11:02:21, jochen wrote:
> nit. all arguments on their own line

Done.

https://chromiumcodereview.appspot.com/10413072/diff/1/webkit/dom_storage/dom...
webkit/dom_storage/dom_storage_context.cc:107:
!special_storage_policy_->IsStorageProtected(infos[i].origin)) {
On 2012/05/23 11:02:21, jochen wrote:
> this will also delete data on chrome-extensions://
> 
> is that really intended, or do you just want to delete data on http/https
> origins that are covered by app extends?

My understanding of the feature was that it should kill all application data,
which in my mind includes everything installed in the browser. It kills devtools
data too at the moment; that's intentional on my part, but I guess it's up for
debate whether that's what the checkbox means.

[Bernhard Bauer, 2012-05-23 11:31:40]

https://chromiumcodereview.appspot.com/10413072/diff/1/chrome/browser/browsin...
File chrome/browser/browsing_data_remover.h (right):

https://chromiumcodereview.appspot.com/10413072/diff/1/chrome/browser/browsin...
chrome/browser/browsing_data_remover.h:128: void Remove(int remove_mask,
RemoveOriginSet origin_set);
On 2012/05/23 11:23:51, Mike West (chromium) wrote:
> On 2012/05/23 11:13:11, Bernhard Bauer wrote:
> > Is there a reason you're not just passing in a bool
> |include_protected_origins|?
> 
> Magic booleans are confusing in a public API. *shrug*  If you saw
> `remover_->remove(true);`, you don't understand the impact as well as you do
if
> there's something descriptive there.

Right; what I usually do in that case is something like:

  bool include_all_origins = true;
  remover->remove(include_all_origins);

The advantage of seeing that the value is a single bit is that you immediately
know what the possible values are. If you see |REMOVE_FROM_UNPROTECTED_ORIGINS|,
it's not clear what the alternative values would be (or mean).

[jochen (gone - plz use gerrit), 2012-05-23 11:38:35]

https://chromiumcodereview.appspot.com/10413072/diff/1/webkit/dom_storage/dom...
File webkit/dom_storage/dom_storage_context.cc (right):

https://chromiumcodereview.appspot.com/10413072/diff/1/webkit/dom_storage/dom...
webkit/dom_storage/dom_storage_context.cc:107:
!special_storage_policy_->IsStorageProtected(infos[i].origin)) {
On 2012/05/23 11:23:51, Mike West (chromium) wrote:
> On 2012/05/23 11:02:21, jochen wrote:
> > this will also delete data on chrome-extensions://
> > 
> > is that really intended, or do you just want to delete data on http/https
> > origins that are covered by app extends?
> 
> My understanding of the feature was that it should kill all application data,
> which in my mind includes everything installed in the browser. It kills
devtools
> data too at the moment; that's intentional on my part, but I guess it's up for
> debate whether that's what the checkbox means.

ok, if that's the desired behavior meanwhile. We used to do that in the
beginning in it caused a certain level of drama.

[Mike West, 2012-05-23 11:42:55]

> ok, if that's the desired behavior meanwhile. We used to do that in the
> beginning in it caused a certain level of drama.

I'm assuming that the UI folks will come up with some stunningly brilliant
mechanism of communicating the behavior to users. I certainly don't expect a
"Clear app data" checkbox to be on by default.

[jochen (gone - plz use gerrit), 2012-05-23 11:45:56]

On 2012/05/23 11:42:55, Mike West (chromium) wrote:
> > ok, if that's the desired behavior meanwhile. We used to do that in the
> > beginning in it caused a certain level of drama.
> 
> I'm assuming that the UI folks will come up with some stunningly brilliant
> mechanism of communicating the behavior to users. I certainly don't expect a
> "Clear app data" checkbox to be on by default.

ok, content and webkit look good then :)

[Mike West, 2012-05-23 12:03:18]

> The advantage of seeing that the value is a single bit is that you immediately
> know what the possible values are. If you see
|REMOVE_FROM_UNPROTECTED_ORIGINS|,
> it's not clear what the alternative values would be (or mean).

Hrm. I'm not sure that relying on the API client to document the API is a great
idea, but I don't feel strongly enough about it to argue. :) Enum's dropped in
the latest patch.

[Mike West, 2012-05-23 12:33:28]

Hello, potential reviewers! This CL adds an argument to
BrowsingDataRemover::Remove, and touches a bit of code in your respective
directories in order to do it.

michaeln, would you mind taking a look at the changes to DomStorageContext?

estade, can you evaluate the changes to webui/*? They're fairly trivial.

avi, I'd appreciate you taking a look at the content/* changes. They're equally
trivial, for the same reasons.

Thanks!

-mike

[Mike West, 2012-05-23 12:33:53]

Actually adding avi@. Hi!

[Avi (use Gerrit), 2012-05-23 15:44:54]

content lgtm

[michaeln, 2012-05-23 20:36:18]

domstorage change lgtm

although you should probably be prepared for some fallout regarding the deletion
of extension and devtool settings and such as a consequence of "Clear app data"

have you guys ever considered a UI that allows you to delete the "old  stuff"
instead of an interfaced aimed at deleting the "new stuff"?

[Evan Stade, 2012-05-23 23:13:27]

https://chromiumcodereview.appspot.com/10413072/diff/12002/chrome/browser/ui/...
File
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.cc
(right):

https://chromiumcodereview.appspot.com/10413072/diff/12002/chrome/browser/ui/...
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.cc:494:
include_protected_origins);
I'd prefer

false);  // include_protected_origins

or at least add const. It's also frowned upon to define a variable earlier than
you need to (i.e. one line above where you need to in this case).

https://chromiumcodereview.appspot.com/10413072/diff/12002/chrome/browser/ui/...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://chromiumcodereview.appspot.com/10413072/diff/12002/chrome/browser/ui/...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1039:
include_protected_origins);
ditto

https://chromiumcodereview.appspot.com/10413072/diff/12002/chrome/browser/ui/...
File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right):

https://chromiumcodereview.appspot.com/10413072/diff/12002/chrome/browser/ui/...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:734:
remover->Remove(BrowsingDataRemover::REMOVE_CACHE, include_protected_origins);
ditto

https://chromiumcodereview.appspot.com/10413072/diff/12002/chrome/browser/ui/...
File chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc (right):

https://chromiumcodereview.appspot.com/10413072/diff/12002/chrome/browser/ui/...
chrome/browser/ui/webui/options2/clear_browser_data_handler2.cc:135:
remover_->Remove(remove_mask, include_protected_origins);
ditto

[Mike West, 2012-05-24 10:54:24]

Shockingly, the task has changed.

Bernhard, Jochen, could you take another look at the BDR implementation (which
is now a mask instead of a flag). In particular, I need some help with the
DomStorageContext tests. I'm pretty sure that the code does what it's supposed
to do, but the test breaks down on `chrome-extension://` origins. I've no idea
why. :(

Value of: infos[0].origin
  Actual: chrome-extension://
Expected: origin
Which is: chrome-extension://abcdefghijklmnopqrstuvwxyz/

[jochen (gone - plz use gerrit), 2012-05-24 11:34:04]

https://chromiumcodereview.appspot.com/10413072/diff/22001/chrome/browser/ui/...
File
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.cc
(right):

https://chromiumcodereview.appspot.com/10413072/diff/22001/chrome/browser/ui/...
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.cc:492:
browsing_data_remover_->Remove(BrowsingDataRemover::REMOVE_SITE_DATA,
all arguments on separate lines, or align them after the (

https://chromiumcodereview.appspot.com/10413072/diff/22001/webkit/quota/speci...
File webkit/quota/special_storage_policy.h (right):

https://chromiumcodereview.appspot.com/10413072/diff/22001/webkit/quota/speci...
webkit/quota/special_storage_policy.h:30: EXTENSION = 1 << 2
stuff in webkit/ can't know about extensions :-/

same applies to your changes to webkit/dom_storage

Maybe call it UNPROTECTED_OTHER and PROTECTED_OTHER and have a callback on
SpecialStoragePolicy that given a URL returns the origin type?

extensionSpecialStoragePolicy can then do the chrome-extension:// magic

[Mike West, 2012-05-24 19:04:38]

Revising this a lot based on Jochen's note. Don't worry about reviewing it until
the next patch, please. :)

[michaeln, 2012-05-24 20:11:58]

On 2012/05/24 19:04:38, Mike West (chromium) wrote:
> Revising this a lot based on Jochen's note. Don't worry about reviewing it
until
> the next patch, please. :)

Ugh is right... 

The handling for the QuotaManaged storage types seems to fall out more easily
since it's the chrome-level logic that gets to decide what to keep and what to
delete in OnGotQuotaManagedOrigins().

Maybe we should invert the control flow for DomStorage similarly. Ask
DomStorageContext for the list of origins and let the BDR decide (based on
whatever impossibly confusing and complicated policy it desires) which of those
should be kept or deleted.

[Mike West, 2012-05-24 20:37:56]

> Maybe we should invert the control flow for DomStorage similarly. Ask
> DomStorageContext for the list of origins and let the BDR decide (based on
> whatever impossibly confusing and complicated policy it desires) which of
those
> should be kept or deleted.

That's more or less the direction I'm heading. I'd like to move the decision
logic to BrowsingDataRemover and BrowsingDataHelper, and drop the
DeleteDataModifiedSince entirely. Should have a patch for that tomorrow.

[Mike West, 2012-05-31 14:19:32]

Circling back to this after a few days of being buried elsewhere. Jochen,
Bernhard, what do you think of this approach?

Michael, are you ok with dropping DomStorageContext::DeleteDataModifiedSince
entirely?

[Bernhard Bauer, 2012-05-31 15:57:30]

https://chromiumcodereview.appspot.com/10413072/diff/24002/chrome/browser/bro...
File chrome/browser/browsing_data_helper.cc (right):

https://chromiumcodereview.appspot.com/10413072/diff/24002/chrome/browser/bro...
chrome/browser/browsing_data_helper.cc:55: if
(!BrowsingDataHelper::HasValidScheme(origin.GetOrigin()) &&
Could you change this check to just `origin.scheme() ==
chrome::kExtensionScheme`?  It feels kinda weird to assume that everything
that's not a valid scheme is an automatically an extension.

Alternatively, now that we do treat extensions as potentially clearable browsing
data, maybe we should change the IsValidScheme implementation?

https://chromiumcodereview.appspot.com/10413072/diff/24002/chrome/browser/bro...
File chrome/browser/browsing_data_helper.h (right):

https://chromiumcodereview.appspot.com/10413072/diff/24002/chrome/browser/bro...
chrome/browser/browsing_data_helper.h:25: UNPROTECTED_WEB = 1 << 0, // drive-by
web.
Nit: Can you line up the comments (and set them at least two spaces apart from
the code)?

https://chromiumcodereview.appspot.com/10413072/diff/24002/chrome/browser/bro...
File chrome/browser/browsing_data_helper_unittest.cc (right):

https://chromiumcodereview.appspot.com/10413072/diff/24002/chrome/browser/bro...
chrome/browser/browsing_data_helper_unittest.cc:87: EXPECT_FALSE(Match(kOrigin1,
kUnprotected, mock_policy));
You might be able to make this a bit easier to test if you add a method that
classifies a given origin as one of (protected, unprotected, extension); then
you can test that method instead of testing all bitmask combinations.

https://chromiumcodereview.appspot.com/10413072/diff/24002/chrome/browser/bro...
File chrome/browser/browsing_data_remover.cc (right):

https://chromiumcodereview.appspot.com/10413072/diff/24002/chrome/browser/bro...
chrome/browser/browsing_data_remover.cc:65: #include
"third_party/WebKit/Source/WebKit/chromium/public/platform/WebCString.h"
Nit: Alphabetize

https://chromiumcodereview.appspot.com/10413072/diff/24002/chrome/browser/bro...
chrome/browser/browsing_data_remover.cc:98:
BrowsingDataRemover::BrowsingDataRemover(Profile* profile,
If you feel like some yak-shaving (and you do have a *lot* of free time,
right?), we should have only one constructor, and two methods that set the time
interval to delete data.

https://chromiumcodereview.appspot.com/10413072/diff/24002/chrome/browser/bro...
chrome/browser/browsing_data_remover.cc:636: file_util::GetFileInfo(file_path,
&file_info);
Wait, are you doing file access on the IO thread?

https://chromiumcodereview.appspot.com/10413072/diff/24002/chrome/browser/bro...
chrome/browser/browsing_data_remover.cc:687: }
Do you need to decrement the count in the other case?

It might also be less fragile if you increase it when posting a task instead of
decreasing it when you don't.

https://chromiumcodereview.appspot.com/10413072/diff/24002/chrome/browser/bro...
chrome/browser/browsing_data_remover.cc:697: if (!remove_origin_.is_empty() &&
remove_origin_ != origin->GetOrigin())
This whole thing looks weird. If |remote_origin_| is non-empty, we iterate over
the whole set, but ignore every entry except for one which we already knew
before? That seems like it could be simplified.

https://chromiumcodereview.appspot.com/10413072/diff/24002/chrome/browser/bro...
File chrome/browser/browsing_data_remover.h (right):

https://chromiumcodereview.appspot.com/10413072/diff/24002/chrome/browser/bro...
chrome/browser/browsing_data_remover.h:126: // the provided |origin_set_mask|
(protected, unprotected, or both).
Update the comment now there's a third bit?

https://chromiumcodereview.appspot.com/10413072/diff/24002/chrome/browser/bro...
chrome/browser/browsing_data_remover.h:292: content::DOMStorageContext*
dom_storage_context_;
Can you add a comment who owns this?

https://chromiumcodereview.appspot.com/10413072/diff/24002/chrome/browser/chr...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/10413072/diff/24002/chrome/browser/chr...
chrome/browser/chrome_content_browser_client.cc:1527: bool
include_protected_origins = false;
Is this implicitly casted to an int? I hate C++.

https://chromiumcodereview.appspot.com/10413072/diff/24002/webkit/dom_storage...
File webkit/dom_storage/dom_storage_context_unittest.cc (right):

https://chromiumcodereview.appspot.com/10413072/diff/24002/webkit/dom_storage...
webkit/dom_storage/dom_storage_context_unittest.cc:61: if (infos.size() > 0)
You could ASSERT instead.

[michaeln, 2012-05-31 22:49:06]

https://chromiumcodereview.appspot.com/10413072/diff/24002/chrome/browser/bro...
File chrome/browser/browsing_data_remover.cc (right):

https://chromiumcodereview.appspot.com/10413072/diff/24002/chrome/browser/bro...
chrome/browser/browsing_data_remover.cc:617:
dom_storage_context_->GetAllStorageFiles(
The return type for this method pains me... a list of files whose names are
encoded just so. Please see the comments in public/browser/dom_storage_context.h
about fixing up this api which would also resolve the blocking io on the io
thread thing brought up by bernhard.

wdyt?

https://chromiumcodereview.appspot.com/10413072/diff/24002/content/browser/do...
File content/browser/dom_storage/dom_storage_context_impl.cc (right):

https://chromiumcodereview.appspot.com/10413072/diff/24002/content/browser/do...
content/browser/dom_storage/dom_storage_context_impl.cc:30: // TODO(michaeln):
Fix the content layer api, FilePaths and
along the lines of this comment

https://chromiumcodereview.appspot.com/10413072/diff/24002/content/public/bro...
File content/public/browser/dom_storage_context.h (right):

https://chromiumcodereview.appspot.com/10413072/diff/24002/content/public/bro...
content/public/browser/dom_storage_context.h:29: GetAllStorageFilesCallback;
Can you add the following to this interface and use them from the BDR instead of
the FilePath and origin_id string based interfaces. I can come back later and
delete other usages of the FilePath/origin_id cruft later.

  typedef base::Callback<void(const std::vector<UsageInfo >&)>
      GetUsageInfoCallback;

  struct UsageInfo {
    GURL origin;
    size_t data_size;
    base::Time last_modified;

    UsageInfo();
    ~UsageInfo();
  };

  virtual void GetUsageInfo(const GetUsageInfoCallback& callback) = 0;
  virtual void DeleteOrigin(const GURL& origin) = 0;

[michaeln, 2012-05-31 23:40:46]

Or just a review https://chromiumcodereview.appspot.com/10449103/ for those
changes

[michaeln, 2012-06-01 03:13:39]

http://codereview.chromium.org/10413072/diff/24002/chrome/browser/browsing_da...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/10413072/diff/24002/chrome/browser/browsing_da...
chrome/browser/browsing_data_remover.cc:328:
base::Bind(&BrowsingDataRemover::ClearLocalStorageOnIOThread,
Also, its fine to call the DSC methods on the UI thread.

[Mike West, 2012-06-01 13:35:16]

This is looking better, thanks for the feedback so far. FYI, this change now
depends on http://codereview.chromium.org/10449103/, which I hope Michael can
land pretty soon. :)

I'd like to get this change landed on Monday if at all possible, to ensure that
it's in before the next dev channel. CCing Jeff, since he's interested in that
as well. :)

Evan, can you take another look at the WebUI stuff? It's much simpler now, I
hope you'll approve.

Michael, thanks for hammering on the domstorage content API, that's made my life
easier here. Can you take another look at the code now that I've rebased it on
top of your (pending) change?

Bernhard, please take another look at the BDR code. I believe I addressed (or
TODO'd) all your comments.

Thanks all!

-mike

http://codereview.chromium.org/10413072/diff/24002/chrome/browser/browsing_da...
File chrome/browser/browsing_data_helper.cc (right):

http://codereview.chromium.org/10413072/diff/24002/chrome/browser/browsing_da...
chrome/browser/browsing_data_helper.cc:55: if
(!BrowsingDataHelper::HasValidScheme(origin.GetOrigin()) &&
On 2012/05/31 15:57:31, Bernhard Bauer wrote:
> Could you change this check to just `origin.scheme() ==
> chrome::kExtensionScheme`?  It feels kinda weird to assume that everything
> that's not a valid scheme is an automatically an extension.
> 
> Alternatively, now that we do treat extensions as potentially clearable
browsing
> data, maybe we should change the IsValidScheme implementation?

Running with `isWebScheme` and `isExtensionScheme` in
http://codereview.chromium.org/10454113/

http://codereview.chromium.org/10413072/diff/24002/chrome/browser/browsing_da...
File chrome/browser/browsing_data_helper.h (right):

http://codereview.chromium.org/10413072/diff/24002/chrome/browser/browsing_da...
chrome/browser/browsing_data_helper.h:25: UNPROTECTED_WEB = 1 << 0, // drive-by
web.
On 2012/05/31 15:57:31, Bernhard Bauer wrote:
> Nit: Can you line up the comments (and set them at least two spaces apart from
> the code)?

Done.

http://codereview.chromium.org/10413072/diff/24002/chrome/browser/browsing_da...
File chrome/browser/browsing_data_helper_unittest.cc (right):

http://codereview.chromium.org/10413072/diff/24002/chrome/browser/browsing_da...
chrome/browser/browsing_data_helper_unittest.cc:87: EXPECT_FALSE(Match(kOrigin1,
kUnprotected, mock_policy));
On 2012/05/31 15:57:31, Bernhard Bauer wrote:
> You might be able to make this a bit easier to test if you add a method that
> classifies a given origin as one of (protected, unprotected, extension); then
> you can test that method instead of testing all bitmask combinations.

Extensions are both protected and extensions. Bookmark applications are both
unprotected and extensions.

I'm not sure how we'd get around that with a single method.

http://codereview.chromium.org/10413072/diff/24002/chrome/browser/browsing_da...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/10413072/diff/24002/chrome/browser/browsing_da...
chrome/browser/browsing_data_remover.cc:65: #include
"third_party/WebKit/Source/WebKit/chromium/public/platform/WebCString.h"
On 2012/05/31 15:57:31, Bernhard Bauer wrote:
> Nit: Alphabetize

Done.

http://codereview.chromium.org/10413072/diff/24002/chrome/browser/browsing_da...
chrome/browser/browsing_data_remover.cc:98:
BrowsingDataRemover::BrowsingDataRemover(Profile* profile,
On 2012/05/31 15:57:31, Bernhard Bauer wrote:
> If you feel like some yak-shaving (and you do have a *lot* of free time,
> right?), we should have only one constructor, and two methods that set the
time
> interval to delete data.

http://crbug.com/130732

http://codereview.chromium.org/10413072/diff/24002/chrome/browser/browsing_da...
chrome/browser/browsing_data_remover.cc:636: file_util::GetFileInfo(file_path,
&file_info);
On 2012/05/31 15:57:31, Bernhard Bauer wrote:
> Wait, are you doing file access on the IO thread?

No! Well, not now anyway.

http://codereview.chromium.org/10413072/diff/24002/chrome/browser/browsing_da...
chrome/browser/browsing_data_remover.cc:687: }
On 2012/05/31 15:57:31, Bernhard Bauer wrote:
> Do you need to decrement the count in the other case?
> 
> It might also be less fragile if you increase it when posting a task instead
of
> decreasing it when you don't.

Makes sense. Done.

http://codereview.chromium.org/10413072/diff/24002/chrome/browser/browsing_da...
chrome/browser/browsing_data_remover.cc:697: if (!remove_origin_.is_empty() &&
remove_origin_ != origin->GetOrigin())
On 2012/05/31 15:57:31, Bernhard Bauer wrote:
> This whole thing looks weird. If |remote_origin_| is non-empty, we iterate
over
> the whole set, but ignore every entry except for one which we already knew
> before? That seems like it could be simplified.

You're entirely correct, and it's certainly something I want to fix before a
"Forget this site" feature launches. Filed http://crbug.com/130732

http://codereview.chromium.org/10413072/diff/24002/chrome/browser/browsing_da...
File chrome/browser/browsing_data_remover.h (right):

http://codereview.chromium.org/10413072/diff/24002/chrome/browser/browsing_da...
chrome/browser/browsing_data_remover.h:126: // the provided |origin_set_mask|
(protected, unprotected, or both).
On 2012/05/31 15:57:31, Bernhard Bauer wrote:
> Update the comment now there's a third bit?

Done.

http://codereview.chromium.org/10413072/diff/24002/chrome/browser/browsing_da...
chrome/browser/browsing_data_remover.h:292: content::DOMStorageContext*
dom_storage_context_;
On 2012/05/31 15:57:31, Bernhard Bauer wrote:
> Can you add a comment who owns this?

Done.

http://codereview.chromium.org/10413072/diff/24002/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/10413072/diff/24002/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:1527: bool
include_protected_origins = false;
On 2012/05/31 15:57:31, Bernhard Bauer wrote:
> Is this implicitly casted to an int? I hate C++.

So ugly. Thanks for catching this.

http://codereview.chromium.org/10413072/diff/24002/content/public/browser/dom...
File content/public/browser/dom_storage_context.h (right):

http://codereview.chromium.org/10413072/diff/24002/content/public/browser/dom...
content/public/browser/dom_storage_context.h:29: GetAllStorageFilesCallback;
You're taking care of this in http://codereview.chromium.org/10449103/, for
which you've become my single most favoritest engineer of the week. :)

http://codereview.chromium.org/10413072/diff/24002/webkit/dom_storage/dom_sto...
File webkit/dom_storage/dom_storage_context_unittest.cc (right):

http://codereview.chromium.org/10413072/diff/24002/webkit/dom_storage/dom_sto...
webkit/dom_storage/dom_storage_context_unittest.cc:61: if (infos.size() > 0)
On 2012/05/31 15:57:31, Bernhard Bauer wrote:
> You could ASSERT instead.

Done.

[Bernhard Bauer, 2012-06-01 13:59:35]

http://codereview.chromium.org/10413072/diff/24002/chrome/browser/browsing_da...
File chrome/browser/browsing_data_helper.h (right):

http://codereview.chromium.org/10413072/diff/24002/chrome/browser/browsing_da...
chrome/browser/browsing_data_helper.h:25: UNPROTECTED_WEB = 1 << 0, // drive-by
web.
On 2012/06/01 13:35:17, Mike West (chromium) wrote:
> On 2012/05/31 15:57:31, Bernhard Bauer wrote:
> > Nit: Can you line up the comments (and set them at least two spaces apart
from
> > the code)?
> 
> Done.

O RLY? ;-)

http://codereview.chromium.org/10413072/diff/24002/chrome/browser/browsing_da...
File chrome/browser/browsing_data_helper_unittest.cc (right):

http://codereview.chromium.org/10413072/diff/24002/chrome/browser/browsing_da...
chrome/browser/browsing_data_helper_unittest.cc:87: EXPECT_FALSE(Match(kOrigin1,
kUnprotected, mock_policy));
On 2012/06/01 13:35:17, Mike West (chromium) wrote:
> On 2012/05/31 15:57:31, Bernhard Bauer wrote:
> > You might be able to make this a bit easier to test if you add a method that
> > classifies a given origin as one of (protected, unprotected, extension);
then
> > you can test that method instead of testing all bitmask combinations.
> 
> Extensions are both protected and extensions. Bookmark applications are both
> unprotected and extensions.
> 
> I'm not sure how we'd get around that with a single method.

We're defining extension as "everything with a chrome-extension:// scheme" here.
In that sense, an extension is not protected web content (as it's not web
content), and a bookmark application is not an extension (as it doesn't contain
anything with a chrome-extension:// scheme). That means that every origin
matches at most one of (protected web, unprotected web, extension). Or am I
missing something here?

http://codereview.chromium.org/10413072/diff/42002/chrome/browser/browsing_da...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/10413072/diff/42002/chrome/browser/browsing_da...
chrome/browser/browsing_data_remover.cc:671:
(BrowsingDataHelper::UNPROTECTED_WEB | BrowsingDataHelper::EXTENSION)) {
I'm sorry, I still don't get it. This means that we don't delete temporary quota
if we want to delete data only for protected origins, right? Why exactly is
that?

[Mike West, 2012-06-01 14:11:51]

Danke!

http://codereview.chromium.org/10413072/diff/24002/chrome/browser/browsing_da...
File chrome/browser/browsing_data_helper.h (right):

http://codereview.chromium.org/10413072/diff/24002/chrome/browser/browsing_da...
chrome/browser/browsing_data_helper.h:25: UNPROTECTED_WEB = 1 << 0, // drive-by
web.
On 2012/06/01 13:59:35, Bernhard Bauer wrote:
> On 2012/06/01 13:35:17, Mike West (chromium) wrote:
> > On 2012/05/31 15:57:31, Bernhard Bauer wrote:
> > > Nit: Can you line up the comments (and set them at least two spaces apart
> from
> > > the code)?
> > 
> > Done.
> 
> O RLY? ;-)

It was done in my head.

Now it's done in code. SRSLY.

http://codereview.chromium.org/10413072/diff/24002/chrome/browser/browsing_da...
File chrome/browser/browsing_data_helper_unittest.cc (right):

http://codereview.chromium.org/10413072/diff/24002/chrome/browser/browsing_da...
chrome/browser/browsing_data_helper_unittest.cc:87: EXPECT_FALSE(Match(kOrigin1,
kUnprotected, mock_policy));
On 2012/06/01 13:59:35, Bernhard Bauer wrote:
> On 2012/06/01 13:35:17, Mike West (chromium) wrote:
> > On 2012/05/31 15:57:31, Bernhard Bauer wrote:
> > > You might be able to make this a bit easier to test if you add a method
that
> > > classifies a given origin as one of (protected, unprotected, extension);
> then
> > > you can test that method instead of testing all bitmask combinations.
> > 
> > Extensions are both protected and extensions. Bookmark applications are both
> > unprotected and extensions.
> > 
> > I'm not sure how we'd get around that with a single method.
> 
> We're defining extension as "everything with a chrome-extension:// scheme"
here.
> In that sense, an extension is not protected web content (as it's not web
> content), and a bookmark application is not an extension (as it doesn't
contain
> anything with a chrome-extension:// scheme). That means that every origin
> matches at most one of (protected web, unprotected web, extension). Or am I
> missing something here?

No, you're right. Again, I successfully confused myself while talking to you
yesterday.

But I still think I need to check all bitmask combinations. :)

http://codereview.chromium.org/10413072/diff/42002/chrome/browser/browsing_da...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/10413072/diff/42002/chrome/browser/browsing_da...
chrome/browser/browsing_data_remover.cc:671:
(BrowsingDataHelper::UNPROTECTED_WEB | BrowsingDataHelper::EXTENSION)) {
On 2012/06/01 13:59:35, Bernhard Bauer wrote:
> I'm sorry, I still don't get it. This means that we don't delete temporary
quota
> if we want to delete data only for protected origins, right? Why exactly is
> that?

You're entirely correct. I confused myself while talking to you yesterday.

We don't need an `if` here at all. The call to DoesOriginMatchMask in
OnGotQuotaManagedOrigins will take care of things.

[Evan Stade, 2012-06-01 17:17:02]

lgtm

http://codereview.chromium.org/10413072/diff/36004/chrome/browser/browsing_da...
File chrome/browser/browsing_data_helper.h (right):

http://codereview.chromium.org/10413072/diff/36004/chrome/browser/browsing_da...
chrome/browser/browsing_data_helper.h:25: UNPROTECTED_WEB = 1 << 0,  // drive-by
web.
nit: Comments should be capitalized. Also I think this one is a bit jargony, at
best I can only guess at what "drive-by" means

http://codereview.chromium.org/10413072/diff/36004/chrome/browser/ui/webui/ch...
File
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.cc
(right):

http://codereview.chromium.org/10413072/diff/36004/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.cc:497:
BrowsingDataHelper::UNPROTECTED_WEB);
can you tab this over to line up with the first argument like you did in the
other files

[michaeln, 2012-06-01 20:59:36]

DSC usage lgtm

https://chromiumcodereview.appspot.com/10413072/diff/36004/chrome/browser/bro...
File chrome/browser/browsing_data_remover.cc (right):

https://chromiumcodereview.appspot.com/10413072/diff/36004/chrome/browser/bro...
chrome/browser/browsing_data_remover.cc:610: void
BrowsingDataRemover::ClearLocalStorageOnIOThread() {
it'd be fine to do this directly on the UI thread too w/o the extra thread hops

https://chromiumcodereview.appspot.com/10413072/diff/36004/chrome/browser/bro...
chrome/browser/browsing_data_remover.cc:620: const
std::vector<content::DOMStorageContext::UsageInfo>& infos) {
fyi: this type is changing in the other interface CL to...
   dom_storage::DomStorageContext::UsageInfo
see https://chromiumcodereview.appspot.com/10449103/

https://chromiumcodereview.appspot.com/10413072/diff/36004/chrome/browser/bro...
chrome/browser/browsing_data_remover.cc:685: // TODO(mkwst): Clean this up, it's
slow. http://crbug.com/130746
the bug refers to memory alignment?

[Mike West, 2012-06-04 06:42:08]

Bernhard, if I address Michael's two remaining points (switch to the UI thread,
and change the type), are you happy with the BDR changes? I'd like to land this
quickly so that Dominic can rebase and land his patch.

Thanks!

https://chromiumcodereview.appspot.com/10413072/diff/36004/chrome/browser/bro...
File chrome/browser/browsing_data_remover.cc (right):

https://chromiumcodereview.appspot.com/10413072/diff/36004/chrome/browser/bro...
chrome/browser/browsing_data_remover.cc:685: // TODO(mkwst): Clean this up, it's
slow. http://crbug.com/130746
On 2012/06/01 20:59:36, michaeln wrote:
> the bug refers to memory alignment?

No, simpler: If the caller wants to delete all data from a single origin, I'm
currently grabbing all the origins, and walking through them to find the one I'm
looking for. This can be vastly improved by just deleting that origin's data
without grabbing data for all other origins.

[Bernhard Bauer, 2012-06-04 09:06:43]

LGTM

https://chromiumcodereview.appspot.com/10413072/diff/24002/chrome/browser/bro...
File chrome/browser/browsing_data_helper_unittest.cc (right):

https://chromiumcodereview.appspot.com/10413072/diff/24002/chrome/browser/bro...
chrome/browser/browsing_data_helper_unittest.cc:87: EXPECT_FALSE(Match(kOrigin1,
kUnprotected, mock_policy));
On 2012/06/01 14:11:53, Mike West (chromium) wrote:
> On 2012/06/01 13:59:35, Bernhard Bauer wrote:
> > On 2012/06/01 13:35:17, Mike West (chromium) wrote:
> > > On 2012/05/31 15:57:31, Bernhard Bauer wrote:
> > > > You might be able to make this a bit easier to test if you add a method
> that
> > > > classifies a given origin as one of (protected, unprotected, extension);
> > then
> > > > you can test that method instead of testing all bitmask combinations.
> > > 
> > > Extensions are both protected and extensions. Bookmark applications are
both
> > > unprotected and extensions.
> > > 
> > > I'm not sure how we'd get around that with a single method.
> > 
> > We're defining extension as "everything with a chrome-extension:// scheme"
> here.
> > In that sense, an extension is not protected web content (as it's not web
> > content), and a bookmark application is not an extension (as it doesn't
> contain
> > anything with a chrome-extension:// scheme). That means that every origin
> > matches at most one of (protected web, unprotected web, extension). Or am I
> > missing something here?
> 
> No, you're right. Again, I successfully confused myself while talking to you
> yesterday.
> 
> But I still think I need to check all bitmask combinations. :)

In that case I feel obligated to point out that you're missing kUnprotected |
kExtension here ;-)

(If you would for example enumerate all the bitmask values in a loop, you could
avoid that. Just sayin' ;-)

https://chromiumcodereview.appspot.com/10413072/diff/51001/chrome/browser/bro...
File chrome/browser/browsing_data_helper_unittest.cc (right):

https://chromiumcodereview.appspot.com/10413072/diff/51001/chrome/browser/bro...
chrome/browser/browsing_data_helper_unittest.cc:115: new
MockExtensionSpecialStoragePolicy;
Nit: Indentation.

https://chromiumcodereview.appspot.com/10413072/diff/51001/chrome/browser/bro...
chrome/browser/browsing_data_helper_unittest.cc:145: mock_policy));
Nit: Indentation.

[commit-bot: I haz the power, 2012-06-04 09:35:09]

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

[commit-bot: I haz the power, 2012-06-04 10:41:11]

Change committed as 140273