Check whether clearing LSO data is supported in clients of the BrowsingDataRemover.

Check whether clearing LSO data is supported in clients of the BrowsingDataRemover.

BUG=99505
TEST=browser_tests


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

[Bernhard Bauer, 2011-10-10 16:59:42]

Mike, if you have time could you review this?

Thanks :)

[Mike West, 2011-10-10 18:41:11]

Didn't we just move this out of the clients in http://crrev.com/99753? :)

What's the background? I'd really prefer to handle the complexity inside the
BrowsingDataRemover if possible. Exporting the complexity out to the clients
seems like too much work in areas that are least likely to be familiar with the
plugin code in question.

The fact that I apparently broke something in the clear API without realizing it
is, in my opinion, an argument in favor of keeping the interface as dumb as
possible. ;)

All that said, I only see one real issue, noted below.

http://codereview.chromium.org/8223004/diff/3001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_clear_api.cc (right):

http://codereview.chromium.org/8223004/diff/3001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_clear_api.cc:105: if (removal_mask_ &
~BrowsingDataRemover::REMOVE_LSO_DATA) {
I think taking the one's complement here is backwards. You want to check if the
bit is set, right?

[Mike West, 2011-10-10 18:50:23]

One meor (small) thing.

http://codereview.chromium.org/8223004/diff/3001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_clear_api.cc (right):

http://codereview.chromium.org/8223004/diff/3001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_clear_api.cc:126: removal_mask_ &=
~BrowsingDataRemover::REMOVE_LSO_DATA;
XOR might be clearer? I had to think about what this meant, though that's likely
just personal preference.

http://codereview.chromium.org/8223004/diff/3001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/clear_browser_data_handler.cc (right):

http://codereview.chromium.org/8223004/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/clear_browser_data_handler.cc:110:
site_data_mask &= ~BrowsingDataRemover::REMOVE_LSO_DATA;
Here too.

[Bernhard Bauer, 2011-10-11 13:52:58]

On 2011/10/10 18:41:11, Mike West (chromium) wrote:
> Didn't we just move this out of the clients in http://crrev.com/99753? :)
> 
> What's the background? I'd really prefer to handle the complexity inside the
> BrowsingDataRemover if possible. Exporting the complexity out to the clients
> seems like too much work in areas that are least likely to be familiar with
the
> plugin code in question.
> 
> The fact that I apparently broke something in the clear API without realizing
it
> is, in my opinion, an argument in favor of keeping the interface as dumb as
> possible. ;)

The background is that the preference stating whether clearing LSO data is
supported was set to the wrong value (we guess the initial value, and that guess
turns out to be wrong on the buildbots). To get a non-stale value, we have to
call PluginDataRemover::IsSupported on the FILE thread.

At first I had the call in BrowsingDataRemover, but it seemed a bit unnecessary
to do it when we're being called from ClearBrowserDataHandler, as that already
knows whether clearing LSO data is supported (to show the correct message).

I guess my idea here is that we should be explicit about what we want
BrowsingDataRemover to do, so that when it doesn't work we can fail instead of
silently ignoring it, which could mask actual problems.

http://codereview.chromium.org/8223004/diff/3001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_clear_api.cc (right):

http://codereview.chromium.org/8223004/diff/3001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_clear_api.cc:105: if (removal_mask_ &
~BrowsingDataRemover::REMOVE_LSO_DATA) {
On 2011/10/10 18:41:11, Mike West (chromium) wrote:
> I think taking the one's complement here is backwards. You want to check if
the
> bit is set, right?

Indeed, that was a copy-and-paste from below :) Thanks for catching it!

http://codereview.chromium.org/8223004/diff/3001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_clear_api.cc:126: removal_mask_ &=
~BrowsingDataRemover::REMOVE_LSO_DATA;
On 2011/10/10 18:50:23, Mike West (chromium) wrote:
> XOR might be clearer? I had to think about what this meant, though that's
likely
> just personal preference.

I think a bitwise "and" is the usual way to flip a bit to off, and there is no
way, that it can accidentally turn it on instead :)

[Mike West, 2011-10-11 17:04:13]

Ok, fair enough. I'm not _totally_ totally convinced that it's the right thing
to do, but your argument makes sense (and I can argue with you about it when I
get back into the office). So let's get the flakey test fixed. :)

LGTM.

[commit-bot: I haz the power, 2011-10-11 17:08:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/8223004/7001

[commit-bot: I haz the power, 2011-10-11 18:45:11]

Change committed as 104928