chrome.clear: Increasing granularity of public API

chrome.clear: Increasing granularity of public API

http://codereview.chromium.org/7717023 added more granular options to
BrowsingDataRemover. This CL exposes those options to the chrome.clear
extension API. Among other things, this means that chrome.clear.cookies()
will _only_ clear cookies, not cookies and site data.

At the moment, clearing any quota managed data type will clear them all.
That is being addressed in http://codereview.chromium.org/7839029/
but is independent from changing the public interface.

BUG=94334
TEST=browser_tests

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

[Mike West, 2011-09-23 11:28:38]

Hello Jochen and Bernhard!

When you have a few free moments, please take a look at this CL. Thanks!

-Mike

[Bernhard Bauer, 2011-09-23 12:09:06]

LGTM.

[commit-bot: I haz the power, 2011-09-23 16:34:11]

CQ is trying tha patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/8008012/1

[commit-bot: I haz the power, 2011-09-23 16:34:18]

Presubmit check for 8008012-1 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: chrome/common/extensions/api/extension_api.json

Presubmit checks took 1.9s to calculate.

[Mike West, 2011-09-23 16:52:17]

Fair enough. :)

Mihai, would you mind taking a look at the extension to the extension API?

-Mike

[Mike West, 2011-10-10 18:34:08]

Friendly ping, Mihai. :)

If you don't have time, can you find someone else on the OWNERS list that could
look at the extension_api.json change?

Thanks!

[Aaron Boodman, 2011-10-12 08:20:32]

http://codereview.chromium.org/8008012/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_clear_api_constants.h (right):

http://codereview.chromium.org/8008012/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_clear_api_constants.h:7: #ifndef
CHROME_BROWSER_EXTENSIONS_EXTENSION_CLEAR_API_CONSTANTS_H_
We don't really need the separate 'constants' files anymore. You can just put
these in the api.cc file.

http://codereview.chromium.org/8008012/diff/1/chrome/common/extensions/api/ex...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/8008012/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.json:8323: "namespace":
"experimental.clear",
"clear" is kind of a weird package name.

http://codereview.chromium.org/8008012/diff/1/chrome/test/data/extensions/api...
File chrome/test/data/extensions/api_test/clear/api/background.html (right):

http://codereview.chromium.org/8008012/diff/1/chrome/test/data/extensions/api...
chrome/test/data/extensions/api_test/clear/api/background.html:3: appcache:
true,
API tests are really flakey and hard to work with.

Consider converting this to an 'extension test' at some point. See
chrome/browser/extensions/extension_tabs_test.cc for an example. The test can be
based on either InProcessBrowserTest or a normal unit test, depending on the
needs of your ExtensionFunction implementation.

[Mihai Parparita -not on Chrome, 2011-10-13 02:01:33]

On Mon, Oct 10, 2011 at 11:34 AM, <mkwst@chromium.org> wrote:

> Friendly ping, Mihai. :)
>
> If you don't have time, can you find someone else on the OWNERS list that
> could
> look at the extension_api.json change?
>

Oops, sorry for letting this get buried in my inbox. Aaron, thanks for
stepping in.

Mihai

[Aaron Boodman, 2011-10-13 02:19:18]

http://codereview.chromium.org/8008012/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_clear_api.cc (right):

http://codereview.chromium.org/8008012/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_clear_api.cc:137: return
BrowsingDataRemover::REMOVE_APPCACHE;
Idea: Did you know that you can get the function name that was called from the
ExtensionFunction base class (via the name() method) ?

If the only thing you're doing is essentially mapping the function name to a
constant, perhaps you could have one ExtensionFunction implementation and a
global map of name->constant.

It looks like you will need to add FactoryRegistry::Register<T>(const
std::string& name) in ExtensionFunctionDispatcher but that is pretty simple.

[Mike West, 2011-12-13 11:27:23]

Reviving this patch from the dead after a long stretch of working on everything
else ever. :)

Jochen/Bernhard: Can you rereview the BrowsingDataRemover changes, since they're
now rebased on top of 10 weeks of new work?

Aaron: Can you take another look at the extension bits? I've reworked the API
tests into an 'extension test', but I'd like to delay some of your other
suggestions (specifically: ripping out all the subclasses) until after we decide
how this API really needs to look. I hope we can hammer that out at the meeting
tomorrow.

This CL depends on http://codereview.chromium.org/8907015/ so please don't
accidentally drop it into the CQ until that patch lands. :)

Thanks!

-Mike

http://codereview.chromium.org/8008012/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_clear_api.cc (right):

http://codereview.chromium.org/8008012/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_clear_api.cc:137: return
BrowsingDataRemover::REMOVE_APPCACHE;
On 2011/10/13 02:19:19, Aaron Boodman wrote:
> Idea: Did you know that you can get the function name that was called from the
> ExtensionFunction base class (via the name() method) ?

This is a good idea, but I'd like to do it in a separate CL if you're ok with
that.

I filed http://crbug.com/100155 to keep track of the refactoring.

http://codereview.chromium.org/8008012/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_clear_api_constants.h (right):

http://codereview.chromium.org/8008012/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_clear_api_constants.h:7: #ifndef
CHROME_BROWSER_EXTENSIONS_EXTENSION_CLEAR_API_CONSTANTS_H_
On 2011/10/12 08:20:32, Aaron Boodman wrote:
> We don't really need the separate 'constants' files anymore. You can just put
> these in the api.cc file.

Done.

http://codereview.chromium.org/8008012/diff/1/chrome/common/extensions/api/ex...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/8008012/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.json:8323: "namespace":
"experimental.clear",
On 2011/10/12 08:20:32, Aaron Boodman wrote:
> "clear" is kind of a weird package name.

Yeah. I had this exact discussion with Kathy and Mihai when I first landed it.
That's one of the things I'd like to address in an extension-team session
sometime before the branch point. Happily, that's scheduled for tomorrow. :)

http://codereview.chromium.org/8008012/diff/1/chrome/test/data/extensions/api...
File chrome/test/data/extensions/api_test/clear/api/background.html (right):

http://codereview.chromium.org/8008012/diff/1/chrome/test/data/extensions/api...
chrome/test/data/extensions/api_test/clear/api/background.html:3: appcache:
true,
On 2011/10/12 08:20:32, Aaron Boodman wrote:
> API tests are really flakey and hard to work with.
> 
> Consider converting this to an 'extension test' at some point. See
> chrome/browser/extensions/extension_tabs_test.cc for an example. The test can
be
> based on either InProcessBrowserTest or a normal unit test, depending on the
> needs of your ExtensionFunction implementation.

Done.

[jochen (gone - plz use gerrit), 2011-12-13 13:26:18]

http://codereview.chromium.org/8008012/diff/15001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/8008012/diff/15001/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:2516: 'browser/extensions/extension_clear_test.cc',
it's a browsertest, so it should be called _browsertest.cc

http://codereview.chromium.org/8008012/diff/15001/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/8008012/diff/15001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:8677: "enum": ["last_hour",
"last_day", "last_week", "last_month", "everything"],
why restrict this to the fixed periods instead of letting the extension specify
time points (start/end), or at least a timespan, so (now - timespan, now] is
deleted?

http://codereview.chromium.org/8008012/diff/15001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:8741: "lsoData": {
lsoData sounds strange, what about localSharedObjects or pluginData. Flash for
example also kills history and cache, not only lsos

http://codereview.chromium.org/8008012/diff/15001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:8751: "webSQL": {
I think the official name is web databases?

[Mike West, 2011-12-13 15:28:47]

Thanks Jochen, responses inline.

http://codereview.chromium.org/8008012/diff/15001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/8008012/diff/15001/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:2516: 'browser/extensions/extension_clear_test.cc',
On 2011/12/13 13:26:18, jochen wrote:
> it's a browsertest, so it should be called _browsertest.cc

I was following aa@'s lead for extension tests (see `extension_tabs_test.cc`). I
don't have strong feeling either way. Aaron?

http://codereview.chromium.org/8008012/diff/15001/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/8008012/diff/15001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:8677: "enum": ["last_hour",
"last_day", "last_week", "last_month", "everything"],
On 2011/12/13 13:26:18, jochen wrote:
> why restrict this to the fixed periods instead of letting the extension
specify
> time points (start/end), or at least a timespan, so (now - timespan, now] is
> deleted?

No good reason. I think it's worthwhile to expand this to arbitrary timelines

However, we probably can't offer start/end timeframes since some of the removal
mechanisms don't support arbitrary ending points.

http://codereview.chromium.org/8008012/diff/15001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:8741: "lsoData": {
On 2011/12/13 13:26:18, jochen wrote:
> lsoData sounds strange, what about localSharedObjects or pluginData. Flash for
> example also kills history and cache, not only lsos

pluginData makes sense, and better matches what we're actually doing. We should
change BDR's external interface to use this term to match
content::PluginDataRemover. Pulling part of this out into
http://crbug.com/107355, for which a CL is in-flight; the rest is in the newly
uploaded patch.

http://codereview.chromium.org/8008012/diff/15001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:8751: "webSQL": {
On 2011/12/13 13:26:18, jochen wrote:
> I think the official name is web databases?

The official name, unfortunately, seems to be "Web SQL Databases".
http://www.w3.org/TR/webdatabase/  I've been calling it WebSQL forever, but
that's no promise that it's actually common usage. CCing Kathy/Meggin for input
on this.

[jochen (gone - plz use gerrit), 2011-12-14 19:30:57]

browser data remover bits look good

http://codereview.chromium.org/8008012/diff/15001/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/8008012/diff/15001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:8751: "webSQL": {
On 2011/12/13 15:28:47, Mike West (chromium) wrote:
> On 2011/12/13 13:26:18, jochen wrote: > I think the official name is web
databases?
> The official name, unfortunately, seems to be "Web SQL Databases".
http://www.w3.org/TR/webdatabase/ I've been calling it WebSQL forever, but
that's no promise that it's actually common usage. CCing Kathy/Meggin for input
on this.

In Chrome, we call it Web Databases (e.g.
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/app/generated_...)

[Aaron Boodman, 2011-12-14 22:23:45]

http://codereview.chromium.org/8008012/diff/17004/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_function_test_utils.cc (right):

http://codereview.chromium.org/8008012/diff/17004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_function_test_utils.cc:142: return
function->GetResultValue()->DeepCopy();
Was the only reason RunFunctionWithoutResult needed because this line crashes if
the function doesn't return a value? If so you could modify it to check
GetResultValue() is not NULL and return NULL if so.

[Mike West, 2011-12-14 22:41:58]

http://codereview.chromium.org/8008012/diff/17004/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_function_test_utils.cc (right):

http://codereview.chromium.org/8008012/diff/17004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_function_test_utils.cc:142: return
function->GetResultValue()->DeepCopy();
On 2011/12/14 22:23:46, Aaron Boodman wrote:
> Was the only reason RunFunctionWithoutResult needed because this line crashes
if
> the function doesn't return a value? If so you could modify it to check
> GetResultValue() is not NULL and return NULL if so.

Yes, that's what I originally did. Having a separate function has, however, the
nice side effect of letting us assert that there's no result (line #158).

I don't have a strong opinion on it, though, If you'd rather stick with the one
function, I'll modify it as you've suggested.

[Aaron Boodman, 2011-12-14 22:44:34]

http://codereview.chromium.org/8008012/diff/17004/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_function_test_utils.cc (right):

http://codereview.chromium.org/8008012/diff/17004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_function_test_utils.cc:142: return
function->GetResultValue()->DeepCopy();
On 2011/12/14 22:41:59, Mike West (chromium) wrote:
> On 2011/12/14 22:23:46, Aaron Boodman wrote:
> > Was the only reason RunFunctionWithoutResult needed because this line
crashes
> if
> > the function doesn't return a value? If so you could modify it to check
> > GetResultValue() is not NULL and return NULL if so.
> 
> Yes, that's what I originally did. Having a separate function has, however,
the
> nice side effect of letting us assert that there's no result (line #158).
> 
> I don't have a strong opinion on it, though, If you'd rather stick with the
one
> function, I'll modify it as you've suggested.

You can just do that assertion at the call site.

In code like this I've worked with in the past, there is a big problem of
combinatorial explosion. I'm trying to keep that to a low roar this time.

So yeah, I'd prefer not having these functions if it works for you.

[Mike West, 2011-12-14 22:59:03]

Is this more or less what you had in mind?

http://codereview.chromium.org/8008012/diff/17004/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_function_test_utils.cc (right):

http://codereview.chromium.org/8008012/diff/17004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_function_test_utils.cc:142: return
function->GetResultValue()->DeepCopy();
On 2011/12/14 22:44:35, Aaron Boodman wrote:
> On 2011/12/14 22:41:59, Mike West (chromium) wrote:
> > On 2011/12/14 22:23:46, Aaron Boodman wrote:
> > > Was the only reason RunFunctionWithoutResult needed because this line
> crashes
> > if
> > > the function doesn't return a value? If so you could modify it to check
> > > GetResultValue() is not NULL and return NULL if so.
> > 
> > Yes, that's what I originally did. Having a separate function has, however,
> the
> > nice side effect of letting us assert that there's no result (line #158).
> > 
> > I don't have a strong opinion on it, though, If you'd rather stick with the
> one
> > function, I'll modify it as you've suggested.
> 
> You can just do that assertion at the call site.
> 
> In code like this I've worked with in the past, there is a big problem of
> combinatorial explosion. I'm trying to keep that to a low roar this time.
> 
> So yeah, I'd prefer not having these functions if it works for you.

Done.

[Aaron Boodman, 2011-12-14 23:36:37]

On 2011/12/14 22:59:03, Mike West (chromium) wrote:
> Is this more or less what you had in mind?

yup, thanks.

lgtm

[commit-bot: I haz the power, 2011-12-15 08:44:53]

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

[commit-bot: I haz the power, 2011-12-15 09:55:31]

Change committed as 114615

[Mike West, 2011-12-15 11:02:33]

On 2011/12/15 09:55:31, I haz the power (commit-bot) wrote:
> Change committed as 114615

Bah. Reverted, as I neglected to remove some files from chrome_browser.gypi

Trying again in http://codereview.chromium.org/8952014/