Strawman proposal for chrome.experimental.clear.*

Initial implementation of `chrome.clear.*` extension API.

Opens up the `BrowsingDataRemover::Remove()` functionality to extensions: initial implementation, so it's sitting behind the experimental flag to work out kinks in the API/code.

BUG=83530, 54853
TEST=browser_tests


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

[Mike West, 2011-08-12 13:30:48]

Hi Jochen, Mihai!

This is a strawman implementation of browsing data remover APIs based on the
design doc I sent out earlier in the week. In the spirit of review early and
often, I'd like you two to take a look at it now, though I still need to put
together more than a few tests.

Jochen: Can you look at the BrowsingDataRemover implementation?
Mihai: Can you give me some feedback from the extension-team standpoint? Is the
API something you folks can approve of, and is the extension integration done
correctly?

Thanks to you both!

-Mike

[Mike West, 2011-08-16 08:10:51]

Bernhard expressed interest in taking a look at this as well. Adding him to the
reviewer list. Hi Bernhard!

[Bernhard Bauer, 2011-08-16 09:49:36]

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

http://codereview.chromium.org/7551008/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_clear_api.cc:26: bool
DataRemovalRequested(base::DictionaryValue* dict, std::string key) {
Nit: namespaces aren't indented.

http://codereview.chromium.org/7551008/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_clear_api.cc:28: if
(!dict->GetBoolean(keys::kCookiesKey, &value))
Shouldn't you pass |key| in here? :)

http://codereview.chromium.org/7551008/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_clear_api.cc:47: to_return =
BrowsingDataRemover::LAST_HOUR;
Nit: just directly return the value?

http://codereview.chromium.org/7551008/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_clear_api.cc:57: NOTREACHED() << "Bad
timeperiod: " << period;
I think you should return a value specifying whether the passed in string was
valid or not (and then in the extension function below
EXTENSION_FUNCTION_VALIDATE that it was).

http://codereview.chromium.org/7551008/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_clear_api.cc:92: AddRef();
Can you move this below, to after you have validated all input?

http://codereview.chromium.org/7551008/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_clear_api.cc:104: BrowsingDataRemover*
bdr(new BrowsingDataRemover(
Nit: Can you call it |remover| or something? Abbreviations are kinda hard to
parse.

http://codereview.chromium.org/7551008/diff/7001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_clear_api.h (right):

http://codereview.chromium.org/7551008/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_clear_api.h:20: class DictionaryValue;
Nit: no indent.

http://codereview.chromium.org/7551008/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_clear_api.h:63:
DECLARE_EXTENSION_FUNCTION_NAME(
Nit: does this fit on one line?

http://codereview.chromium.org/7551008/diff/7001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_clear_apitest.cc (right):

http://codereview.chromium.org/7551008/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_clear_apitest.cc:12:
switches::kEnableExperimentalExtensionApis);
Nit: indentation.

http://codereview.chromium.org/7551008/diff/7001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/7551008/diff/7001/chrome/chrome_tests.gypi#new...
chrome/chrome_tests.gypi:2490: 'browser/extensions/extension_clear_apitest.cc',
Nit: sorting ("clear" after "chrome").

http://codereview.chromium.org/7551008/diff/7001/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7551008/diff/7001/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:7536: "enum": ["last_hour",
"last_day", "last_week", "last_month", "everything"],
I think you could pull this into a type?

[Mike West, 2011-08-16 11:05:56]

Thanks Bernhard.  I've addressed your feedback, rebased, and regenerated the
docs.

Take another look at BrowsingDataExtensionFunction::ParseTimePeriod if you don't
mind. Everything else was very straightforward.

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

http://codereview.chromium.org/7551008/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_clear_api.cc:26: bool
DataRemovalRequested(base::DictionaryValue* dict, std::string key) {
On 2011/08/16 09:49:36, Bernhard Bauer wrote:
> Nit: namespaces aren't indented.

Done.

http://codereview.chromium.org/7551008/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_clear_api.cc:28: if
(!dict->GetBoolean(keys::kCookiesKey, &value))
On 2011/08/16 09:49:36, Bernhard Bauer wrote:
> Shouldn't you pass |key| in here? :)

Done.

http://codereview.chromium.org/7551008/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_clear_api.cc:47: to_return =
BrowsingDataRemover::LAST_HOUR;
On 2011/08/16 09:49:36, Bernhard Bauer wrote:
> Nit: just directly return the value?

I took your next bit of advice instead.

http://codereview.chromium.org/7551008/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_clear_api.cc:57: NOTREACHED() << "Bad
timeperiod: " << period;
On 2011/08/16 09:49:36, Bernhard Bauer wrote:
> I think you should return a value specifying whether the passed in string was
> valid or not (and then in the extension function below
> EXTENSION_FUNCTION_VALIDATE that it was).

Done.

http://codereview.chromium.org/7551008/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_clear_api.cc:92: AddRef();
On 2011/08/16 09:49:36, Bernhard Bauer wrote:
> Can you move this below, to after you have validated all input?

Done.

http://codereview.chromium.org/7551008/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_clear_api.cc:104: BrowsingDataRemover*
bdr(new BrowsingDataRemover(
On 2011/08/16 09:49:36, Bernhard Bauer wrote:
> Nit: Can you call it |remover| or something? Abbreviations are kinda hard to
> parse.

Done.

http://codereview.chromium.org/7551008/diff/7001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_clear_api.h (right):

http://codereview.chromium.org/7551008/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_clear_api.h:20: class DictionaryValue;
On 2011/08/16 09:49:36, Bernhard Bauer wrote:
> Nit: no indent.

Done.

http://codereview.chromium.org/7551008/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_clear_api.h:63:
DECLARE_EXTENSION_FUNCTION_NAME(
On 2011/08/16 09:49:36, Bernhard Bauer wrote:
> Nit: does this fit on one line?

Done.

http://codereview.chromium.org/7551008/diff/7001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_clear_apitest.cc (right):

http://codereview.chromium.org/7551008/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_clear_apitest.cc:12:
switches::kEnableExperimentalExtensionApis);
On 2011/08/16 09:49:36, Bernhard Bauer wrote:
> Nit: indentation.

Done.

http://codereview.chromium.org/7551008/diff/7001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/7551008/diff/7001/chrome/chrome_tests.gypi#new...
chrome/chrome_tests.gypi:2490: 'browser/extensions/extension_clear_apitest.cc',
On 2011/08/16 09:49:36, Bernhard Bauer wrote:
> Nit: sorting ("clear" after "chrome").

Done.

http://codereview.chromium.org/7551008/diff/7001/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7551008/diff/7001/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:7536: "enum": ["last_hour",
"last_day", "last_week", "last_month", "everything"],
On 2011/08/16 09:49:36, Bernhard Bauer wrote:
> I think you could pull this into a type?

Done.

[Bernhard Bauer, 2011-08-16 11:29:25]

http://codereview.chromium.org/7551008/diff/13001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_clear_api.cc (right):

http://codereview.chromium.org/7551008/diff/13001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_clear_api.cc:42: Release();  // Balanced in
RunImpl.
When clearing browsing data from the UI, we block the UI until everything is
done. Could any Bad Things(tm) happen if the browser (or just the current
profile) is shut down while one of these requests is pending?

http://codereview.chromium.org/7551008/diff/13001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_clear_api.cc:105: BrowsingDataRemover*
remover(new BrowsingDataRemover(
Nit: I think using = for initialization is more common.

[Mike West, 2011-08-16 12:13:41]

http://codereview.chromium.org/7551008/diff/13001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_clear_api.cc (right):

http://codereview.chromium.org/7551008/diff/13001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_clear_api.cc:42: Release();  // Balanced in
RunImpl.
On 2011/08/16 11:29:25, Bernhard Bauer wrote:
> When clearing browsing data from the UI, we block the UI until everything is
> done. Could any Bad Things(tm) happen if the browser (or just the current
> profile) is shut down while one of these requests is pending?

Idunno. Probably? :)

Jochen? Is there anything I can do here to mitigate the risk?

http://codereview.chromium.org/7551008/diff/13001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_clear_api.cc:105: BrowsingDataRemover*
remover(new BrowsingDataRemover(
On 2011/08/16 11:29:25, Bernhard Bauer wrote:
> Nit: I think using = for initialization is more common.

Done.

[jochen (gone - plz use gerrit), 2011-08-16 15:11:44]

http://codereview.chromium.org/7551008/diff/13001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_clear_api.cc (right):

http://codereview.chromium.org/7551008/diff/13001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_clear_api.cc:42: Release();  // Balanced in
RunImpl.
On 2011/08/16 12:13:42, Mike West wrote:
> On 2011/08/16 11:29:25, Bernhard Bauer wrote:
> > When clearing browsing data from the UI, we block the UI until everything is
> > done. Could any Bad Things(tm) happen if the browser (or just the current
> > profile) is shut down while one of these requests is pending?
> 
> Idunno. Probably? :)
> 
> Jochen? Is there anything I can do here to mitigate the risk?

In my opinion, the browsingdataremover should somehow make sure that it doesn't
outlive its profile, and ExtensionFunction should make sure that it doesn't try
to send IPCs to not any more existing render views.

However, both should not require changes here

[Mike West, 2011-08-19 09:49:17]

@Mihai: Friendly ping. :)

I stuck you on as a review because you were the only person on the extension
team to respond to the design doc. If you're swamped, please do suggest a
replacement; I just want to make sure your team signs off on this API.

Thanks!

-Mike

[Mihai Parparita -not on Chrome, 2011-08-23 23:24:13]

http://codereview.chromium.org/7551008/diff/20001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_clear_api.cc (right):

http://codereview.chromium.org/7551008/diff/20001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_clear_api.cc:77: // "Cookies" are cookies +
site data + LSO.
I'm not seeing a mask value corresponding to "site data", which makes this
comment a bit confusing. It looks like BrowsingDataRemover maps REMOVE_COOKIES
to include site data too. Perhaps the comment could be rephrased as:

// "Cookies" gets expanded to cookies and LSO. In turn, BrowsingDataRemover
expands cookies to cookies and site data.

http://codereview.chromium.org/7551008/diff/20001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_clear_api.h (right):

http://codereview.chromium.org/7551008/diff/20001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_clear_api.h:41: virtual int removal_mask()
const = 0;
Since this isn't just a simple getter for a variable, you should call this
GetRemovalMask.

Also, I believe you can just have this method as protected.

http://codereview.chromium.org/7551008/diff/20001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_clear_api.h:47: virtual bool
ParseTimePeriod(const std::string& parse,
Would any subclass need to call this? Seems like it can just be private.

http://codereview.chromium.org/7551008/diff/20001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_clear_api.h:52: virtual int
ParseRemovalMask(base::DictionaryValue* value) const;
It seems like you would only need this for ClearBrowsingDataFunction, can this
just be a private method in that class?

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

http://codereview.chromium.org/7551008/diff/20001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:8138: "type": "boolean",
I'm not seeing this parameter being passed back. Errors for extension APIs are
normally reported via chrome.extension.lastError (setting error_ and returning
false in an ExtensionFunction implementation propagates to that).

http://codereview.chromium.org/7551008/diff/20001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_permission_set.h (right):

http://codereview.chromium.org/7551008/diff/20001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:49: kClear,
We don't add new permissions (or messages) until an API graduates from
experimental (all those APIs are behind the experimental permission).

[Mike West, 2011-08-24 09:10:33]

Thanks.  Callback boolean is the only outstanding issue; the problem is quite
simply that the BrowsingDataRemover doesn't do a very good job of bubbling
errors up to observers. I'm not sure it's possible to fix that without some
serious rework, but I'll take a look today.

http://codereview.chromium.org/7551008/diff/20001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_clear_api.cc (right):

http://codereview.chromium.org/7551008/diff/20001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_clear_api.cc:77: // "Cookies" are cookies +
site data + LSO.
On 2011/08/23 23:24:13, Mihai Parparita wrote:
> I'm not seeing a mask value corresponding to "site data", which makes this
> comment a bit confusing. It looks like BrowsingDataRemover maps REMOVE_COOKIES
> to include site data too. Perhaps the comment could be rephrased as:
> 
> // "Cookies" gets expanded to cookies and LSO. In turn, BrowsingDataRemover
> expands cookies to cookies and site data.

You're right, this was poorly phrased. I've updated the comment to say something
more like what I meant to say. :)

Also, we'll eventually be removing the LSO mask entirely, which will obviate
this comment in any event: see http://crbug.com/92628

http://codereview.chromium.org/7551008/diff/20001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_clear_api.h (right):

http://codereview.chromium.org/7551008/diff/20001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_clear_api.h:41: virtual int removal_mask()
const = 0;
On 2011/08/23 23:24:13, Mihai Parparita wrote:
> Since this isn't just a simple getter for a variable, you should call this
> GetRemovalMask.
> 
> Also, I believe you can just have this method as protected.

Done.

http://codereview.chromium.org/7551008/diff/20001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_clear_api.h:47: virtual bool
ParseTimePeriod(const std::string& parse,
On 2011/08/23 23:24:13, Mihai Parparita wrote:
> Would any subclass need to call this? Seems like it can just be private.

Done.

http://codereview.chromium.org/7551008/diff/20001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_clear_api.h:52: virtual int
ParseRemovalMask(base::DictionaryValue* value) const;
On 2011/08/23 23:24:13, Mihai Parparita wrote:
> It seems like you would only need this for ClearBrowsingDataFunction, can this
> just be a private method in that class?

Conceptually, it seemed like something that belonged to the general class of
BrowsingDataExtensionFunctions, but you're right: it's only used in
ClearBrowsingDataFunction. I've moved it there, and if it ever turns out to be
necessary elsewhere (maybe we'll have new types of browsing data in the future?
or more granular deletion mechanisms? who knows...) we can move it back.

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

http://codereview.chromium.org/7551008/diff/20001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:8138: "type": "boolean",
On 2011/08/23 23:24:13, Mihai Parparita wrote:
> I'm not seeing this parameter being passed back. Errors for extension APIs are
> normally reported via chrome.extension.lastError (setting error_ and returning
> false in an ExtensionFunction implementation propagates to that).

It's hardcoded to `true` at the moment (see
BrowsingDataExtensionFunction::OnBrowsingDataRemoverDone). I'm trying to
determine if it's possible to do anything here that makes sense, but given the
way the BrowsingDataRemover works, I'm not sure what that might be.

http://codereview.chromium.org/7551008/diff/20001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_permission_set.h (right):

http://codereview.chromium.org/7551008/diff/20001/chrome/common/extensions/ex...
chrome/common/extensions/extension_permission_set.h:49: kClear,
On 2011/08/23 23:24:13, Mihai Parparita wrote:
> We don't add new permissions (or messages) until an API graduates from
> experimental (all those APIs are behind the experimental permission).

Done.

[Mihai Parparita -not on Chrome, 2011-08-24 17:15:12]

On Wed, Aug 24, 2011 at 2:10 AM, <mkwst@chromium.org> wrote:

> Thanks.  Callback boolean is the only outstanding issue; the problem is
> quite
> simply that the BrowsingDataRemover doesn't do a very good job of bubbling
> errors up to observers. I'm not sure it's possible to fix that without some
> serious rework, but I'll take a look today.


Even if it's not possible or easy to get a more specific failure reason, you
could still have a generic "removal failed" error message that you set
chrome.extension.lastError to.

Mihai

[Mihai Parparita -not on Chrome, 2011-08-24 17:19:59]

http://codereview.chromium.org/7551008/diff/28007/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_clear_api.cc (right):

http://codereview.chromium.org/7551008/diff/28007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_clear_api.cc:91: int
ClearBrowsingDataFunction::ParseRemovalMask(
Nit: this can actually be static in this file.

[Mike West, 2011-08-24 19:19:54]

On 2011/08/24 17:15:12, Mihai Parparita wrote:
> Even if it's not possible or easy to get a more specific failure reason, you
> could still have a generic "removal failed" error message that you set
> chrome.extension.lastError to.

Yeah. That'd be good enough, but even that's not simple. BrowsingDataRemover
simply doesn't pass on any information about the status of the removal; it
simply calls OnBrowsingDataRemoverDone when all the removal functions have
_run_, regardless of their status. Errors are generally ignored. See
http://codesearch.google.com/#OAMlx_jo-ck/src/chrome/browser/browsing_data_re...
for example.

So, I agree absolutely that we should change the core code to capture errors in
some workable way (or even to capture the fact that an error occurred at all).
I'd prefer to do that in a separate CL, though, as this one's already relatively
large. I've filed http://crbug.com/94130 and set it to block this feature's
metabug.

If you feel strongly about it, I'm happy to address that bug before we continue
with this review. I hope you don't. :)

-Mike

[Mihai Parparita -not on Chrome, 2011-08-24 19:38:09]

On Wed, Aug 24, 2011 at 12:19 PM, <mkwst@chromium.org> wrote:

> If you feel strongly about it, I'm happy to address that bug before we
> continue
> with this review. I hope you don't. :)
>

No, I don't think getting an error out of BrowserDataRemover should block
this review. However, the boolean parameter to the callback should still be
removed either way there's no need for it (right now there's no way to pass
anything but true to it, and once BrowserDataRemover can give us failure
reasons, that'll just populate chrome.extension.lastError).

Mihai

[Mike West, 2011-08-25 09:27:54]

Thanks Mihai, I appreciate the explanation. I've dropped the boolean parameter
from the various callbacks and addressed the nits.

Adding Kathy in the hopes that she can take a look at the docs ("staged" as
before at http://www.corp.google.com/~mkwst/docs/experimental.clear.html ). Hi
Kathy! Thanks!

-Mike

http://codereview.chromium.org/7551008/diff/28007/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_clear_api.cc (right):

http://codereview.chromium.org/7551008/diff/28007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_clear_api.cc:91: int
ClearBrowsingDataFunction::ParseRemovalMask(
On 2011/08/24 17:20:00, Mihai Parparita wrote:
> Nit: this can actually be static in this file.

You're right. In fact, I'll go a step further.

I've moved both this and ParseTimePeriod into the anonymous namespace at the top
of this file; there's really no reason for either of these implementation helper
methods to be externally visible.

[kathyw, 2011-08-25 18:33:14]

I started out nitpicking descriptions. (I like your writing. It's nice and it
flows well, but sometimes it could be more precise -- which, unfortunately,
tends to be more boring.)

Then I started thinking about whether "clear" was the best, clearest verb to
use, and where we should use other verbs such as remove/delete.

Then I realized that the method names weren't verbs, which made me realize that
the namespace *is* a verb, which is a little weird.

I think that if people see a chrome.clear API in the list of noun APIs, it'll be
a little confusing, and they might not be able to guess what it does.
chrome.browsingData is a possibility. Or something like chrome.dataRemoval, if
you want to make very clear that there's only one way to use the data (though
that name doesn't lend itself to good method names).

Anyway, I figured you should see my comments before I go too far down this
trail.

-k-

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

http://codereview.chromium.org/7551008/diff/33002/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:8095: "namespace":
"experimental.clear",
It's a little odd to have a namespace that is a verb. Usually method names are
verbs, and namespaces are nouns.

Maybe rename to experimental.browserData?

(I'm guessing you want to say "clear" to make clear -- haha -- that you can only
delete data, not read or manipulate it. But it's just a weird precedent to set.
Also, "clear" has many meanings and is easy to read as a noun, so it might be
nice to use a different word as the primary verb.)

http://codereview.chromium.org/7551008/diff/33002/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:8101: "description": "The
timeframe inside of which to delete browsing data. Passing 'last_day', for
example, will delete all browsing data that was touched between 24 hours ago and
right now, inclusive."
will delete -> deletes

http://codereview.chromium.org/7551008/diff/33002/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:8106: "name": "browsingData",
browsingData -> clear

or maybe "remove"

or maybe "delete"

http://codereview.chromium.org/7551008/diff/33002/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:8107: "description": "Clears
data generated by browsing within a particular timeframe.",
data generated by -> the specified kinds of data that was the result of
within -> during

http://codereview.chromium.org/7551008/diff/33002/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:8117: "description": "An object
whose properties specify which browsing data types ought to be cleared. You may
set as many or as few as you like in a single call, each is optional (defaulting
to <code>false</code>).",
First sentence could be more straightforward:
 -> An object specifying which browsing data types to clear.
may -> can
few -> few properties
, each is -> ; each property is (or ". Each property is")

Finally, the default value seems as important as the fact that the value is
optional, so I wouldn't use parentheses. Something like:

optional (...) -> optional and has a default value of <code>false</code>

http://codereview.chromium.org/7551008/diff/33002/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:8119: "cache": {
cache -> clearCache

http://codereview.chromium.org/7551008/diff/33002/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:8122: "description": "Should the
browser cache be cleared?"
Interesting construction, having these as questions. I'd say the mapping from
true/false to yes/no isn't perfect, though. Also, you could see these as
philsophical questions with a long-term effect, rather than immediate actions. I
guess I'd use a more usual construction, such as:

Specify <code>true</code> to remove data from the browser cache.

http://codereview.chromium.org/7551008/diff/33002/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:8127: "description": "Should the
browser's cookies be cleared?"
Specify <code>true</code> to remove browser cookies.

[Mike West, 2011-08-25 18:52:35]

I'll address my writings lack of clarity (boringness) once I can log back into
my machine (hooray for filer migrations!). ;)

Regarding the namespace, I agree with you, and I disagree with you. As I
mentioned in our conversation just now, I _like_ the fact that the namespace is
a verb, as it's a nicely clear way of making the action the caller is
undertaking clear (ha!): `chrome.clear.cookies()` tells Chrome to clear its
cookies. `chrome.dataRemoval.removeData()` isn't as cle... um... straightforward
(though I grant you that it's more traditional).

I vaguely with you that `chrome.clear` doesn't have enough inherent meaning.
However, I think it's obvious once you see the method names, and it has the
distinct advantage of allowing us to potentially add things in the future that
_aren't_ browsing data (I'd like to see a mechanism for clearing _application_
data, for instance... and the list of prefetchable DNS entries, and etc.).

Anyway; I'd really like to land this in time for M15 so I can get it in front of
some extension authors to test it out. Would you be averse to landing it with
this namespace as an experiment, and then arguing about the name?

Thanks for your comments!

-Mike

[Kathy Walrath, 2011-08-25 18:58:11]

> Anyway; I'd really like to land this in time for M15 so I can get it in
> front of
> some extension authors to test it out. Would you be averse to landing it
> with
> this namespace as an experiment, and then arguing about the name?

Not averse at all. Do it!

-k-

[Mihai Parparita -not on Chrome, 2011-08-26 06:11:19]

LGTM (code-wise)

[commit-bot: I haz the power, 2011-08-26 16:37:21]

Change committed as 98429