`chrome.browsingData` extension API can now remove data from protected origins.

`chrome.browsingData` extension API can now remove data from protected origins.

Currently, installing a hosted application for `example.com` means that we
ignore that origin when removing browsing data. This CL extends the filtering
options available to extension authors using the BrowsingData API to include
removal of browsing data for these "protected" web origins.

BUG=113194
TEST=browser_tests


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

[Mike West, 2012-06-04 11:03:00]

Since r140273 landed, we should add it to the extension API as well. Can you
take a look at the CL, Bernhard? This CL also adds you as an owner for the API
directory, you cool with that? :)

Once you're happy, I'll send it to Aaron and Mihai for API review.

[Bernhard Bauer, 2012-06-04 11:20:16]

https://chromiumcodereview.appspot.com/10522002/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/browsing_data/browsing_data_api.cc (right):

https://chromiumcodereview.appspot.com/10522002/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/browsing_data/browsing_data_api.cc:135: // If
|m_origin_set_mask| isn't set, default it to 'UNPROTECTED_WEB'.
Nit: |origin_set_mask_|

https://chromiumcodereview.appspot.com/10522002/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/browsing_data/browsing_data_api.cc:136:
origin_set_mask_ = 0;
I guess you could directly initialize the value to
BrowsingDataHelper::UNPROTECTED_WEB?

https://chromiumcodereview.appspot.com/10522002/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/browsing_data/browsing_data_api.cc:138: if
(options->GetList(extension_browsing_data_api_constants::kOriginTypesKey,
You could be more aggressive in enforcing parameter validity here, i.e.
EXTENSION_FUNCTION_VALIDATE instead of silently ignoring invalid values.

https://chromiumcodereview.appspot.com/10522002/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/browsing_data/browsing_data_api.cc:149: else if
(value == extension_browsing_data_api_constants::kExtensionsKey)
This value isn't in the API definition.

https://chromiumcodereview.appspot.com/10522002/diff/1/chrome/common/extensio...
File chrome/common/extensions/api/browsing_data.json (right):

https://chromiumcodereview.appspot.com/10522002/diff/1/chrome/common/extensio...
chrome/common/extensions/api/browsing_data.json:20: "type": "array",
Would it make sense to have this as an object with keys "unprotected_web" and
"protected_web" like the dataToRemove parameter below?

https://chromiumcodereview.appspot.com/10522002/diff/1/chrome/common/extensio...
chrome/common/extensions/api/browsing_data.json:22: "description": "Array of
origin types that should be effected: 'unprotected_web' refers to normal
websites, 'protected_web' refers to websites that have been installed as hosted
applications. This setting defaults to ['unprotected_web']. Please ensure that
you <em>really</em> want to remove application data before adding
'protected_web'.",
Nit: "affected"

[Mike West, 2012-06-04 12:23:38]

Thanks Bernhard. I've addressed your comments (and ended up extracting the logic
out into a separate method). WDYT?

Aaron, Mihai, can you take a look at the proposed change to the `browsingData`
API, and the addition of the OWNERS file for the API implementation?

Thanks!

http://codereview.chromium.org/10522002/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/browsing_data/browsing_data_api.cc (right):

http://codereview.chromium.org/10522002/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/browsing_data/browsing_data_api.cc:135: // If
|m_origin_set_mask| isn't set, default it to 'UNPROTECTED_WEB'.
On 2012/06/04 11:20:16, Bernhard Bauer wrote:
> Nit: |origin_set_mask_|

Done.

http://codereview.chromium.org/10522002/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/browsing_data/browsing_data_api.cc:136:
origin_set_mask_ = 0;
On 2012/06/04 11:20:16, Bernhard Bauer wrote:
> I guess you could directly initialize the value to
> BrowsingDataHelper::UNPROTECTED_WEB?

I'm not sure that would save me any work: I'd still have to set it to 0 inside
the `if` block (the developer might specify only `["protected_web"]`). But I
guess it's slightly faster when there's no `origin_type` specified...

http://codereview.chromium.org/10522002/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/browsing_data/browsing_data_api.cc:138: if
(options->GetList(extension_browsing_data_api_constants::kOriginTypesKey,
On 2012/06/04 11:20:16, Bernhard Bauer wrote:
> You could be more aggressive in enforcing parameter validity here, i.e.
> EXTENSION_FUNCTION_VALIDATE instead of silently ignoring invalid values.

Done.

http://codereview.chromium.org/10522002/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/browsing_data/browsing_data_api.cc:149: else if
(value == extension_browsing_data_api_constants::kExtensionsKey)
On 2012/06/04 11:20:16, Bernhard Bauer wrote:
> This value isn't in the API definition.

That's intentional. I'm not sure we want to open up that particular pandora's
box. But I'll drop a quick line to our group to see what other people think.

http://codereview.chromium.org/10522002/diff/1/chrome/common/extensions/api/b...
File chrome/common/extensions/api/browsing_data.json (right):

http://codereview.chromium.org/10522002/diff/1/chrome/common/extensions/api/b...
chrome/common/extensions/api/browsing_data.json:20: "type": "array",
On 2012/06/04 11:20:16, Bernhard Bauer wrote:
> Would it make sense to have this as an object with keys "unprotected_web" and
> "protected_web" like the dataToRemove parameter below?

It might. It probably does, actually.

I should have read this before I addressed your other points in the other file.
:)

http://codereview.chromium.org/10522002/diff/1/chrome/common/extensions/api/b...
chrome/common/extensions/api/browsing_data.json:22: "description": "Array of
origin types that should be effected: 'unprotected_web' refers to normal
websites, 'protected_web' refers to websites that have been installed as hosted
applications. This setting defaults to ['unprotected_web']. Please ensure that
you <em>really</em> want to remove application data before adding
'protected_web'.",
On 2012/06/04 11:20:16, Bernhard Bauer wrote:
> Nit: "affected"

Done.

[Bernhard Bauer, 2012-06-04 12:42:17]

http://codereview.chromium.org/10522002/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/browsing_data/browsing_data_api.cc (right):

http://codereview.chromium.org/10522002/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/browsing_data/browsing_data_api.cc:136:
origin_set_mask_ = 0;
On 2012/06/04 12:23:39, Mike West (chromium) wrote:
> On 2012/06/04 11:20:16, Bernhard Bauer wrote:
> > I guess you could directly initialize the value to
> > BrowsingDataHelper::UNPROTECTED_WEB?
> 
> I'm not sure that would save me any work: I'd still have to set it to 0 inside
> the `if` block (the developer might specify only `["protected_web"]`). But I
> guess it's slightly faster when there's no `origin_type` specified...

Ah, true. Just do what's simpler then.

http://codereview.chromium.org/10522002/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/browsing_data/browsing_data_api.cc:149: else if
(value == extension_browsing_data_api_constants::kExtensionsKey)
On 2012/06/04 12:23:39, Mike West (chromium) wrote:
> On 2012/06/04 11:20:16, Bernhard Bauer wrote:
> > This value isn't in the API definition.
> 
> That's intentional. I'm not sure we want to open up that particular pandora's
> box. But I'll drop a quick line to our group to see what other people think.

Fair enough, but then you shouldn't accept it here either; otherwise a malicious
extension / compromised renderer could bypass the JS-side validation and clear
extension data.

http://codereview.chromium.org/10522002/diff/11002/chrome/browser/extensions/...
File chrome/browser/extensions/api/browsing_data/browsing_data_api.cc (right):

http://codereview.chromium.org/10522002/diff/11002/chrome/browser/extensions/...
chrome/browser/extensions/api/browsing_data/browsing_data_api.cc:222: // Default
to UNPROTECTED_WEB if the developer doesn't specify anything.
Note that the documentation says "If this object isn't specified", not "if none
of the values are true". If I specified { 'protected_web': false,
'unprotected_web': false }, I'd expect nothing to be cleared (even if that's not
very useful).

[Mike West, 2012-06-04 13:20:53]

Thanks, Bernhard. :)

http://codereview.chromium.org/10522002/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/browsing_data/browsing_data_api.cc (right):

http://codereview.chromium.org/10522002/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/browsing_data/browsing_data_api.cc:149: else if
(value == extension_browsing_data_api_constants::kExtensionsKey)
On 2012/06/04 12:42:17, Bernhard Bauer wrote:
> On 2012/06/04 12:23:39, Mike West (chromium) wrote:
> > On 2012/06/04 11:20:16, Bernhard Bauer wrote:
> > > This value isn't in the API definition.
> > 
> > That's intentional. I'm not sure we want to open up that particular
pandora's
> > box. But I'll drop a quick line to our group to see what other people think.
> 
> Fair enough, but then you shouldn't accept it here either; otherwise a
malicious
> extension / compromised renderer could bypass the JS-side validation and clear
> extension data.

Fair enough. Commented out for the moment. Once we make a decision, I'll either
uncomment it, or remove it entirely.

http://codereview.chromium.org/10522002/diff/11002/chrome/browser/extensions/...
File chrome/browser/extensions/api/browsing_data/browsing_data_api.cc (right):

http://codereview.chromium.org/10522002/diff/11002/chrome/browser/extensions/...
chrome/browser/extensions/api/browsing_data/browsing_data_api.cc:222: // Default
to UNPROTECTED_WEB if the developer doesn't specify anything.
On 2012/06/04 12:42:17, Bernhard Bauer wrote:
> Note that the documentation says "If this object isn't specified", not "if
none
> of the values are true". If I specified { 'protected_web': false,
> 'unprotected_web': false }, I'd expect nothing to be cleared (even if that's
not
> very useful).

Would you expect a warning to pop up, explaining that you're a moron?

*sigh*  Done.

[Bernhard Bauer, 2012-06-04 13:42:14]

LGTM

http://codereview.chromium.org/10522002/diff/11002/chrome/browser/extensions/...
File chrome/browser/extensions/api/browsing_data/browsing_data_api.cc (right):

http://codereview.chromium.org/10522002/diff/11002/chrome/browser/extensions/...
chrome/browser/extensions/api/browsing_data/browsing_data_api.cc:222: // Default
to UNPROTECTED_WEB if the developer doesn't specify anything.
On 2012/06/04 13:20:53, Mike West (chromium) wrote:
> On 2012/06/04 12:42:17, Bernhard Bauer wrote:
> > Note that the documentation says "If this object isn't specified", not "if
> none
> > of the values are true". If I specified { 'protected_web': false,
> > 'unprotected_web': false }, I'd expect nothing to be cleared (even if that's
> not
> > very useful).
> 
> Would you expect a warning to pop up, explaining that you're a moron?

We could return an error in that case (and I'm not completely averse to the
error message containing the word moron), but silently doing something else
seems to violate the principle of least surprise.

> *sigh*  Done.

[Mike West, 2012-06-04 13:44:25]

On 2012/06/04 13:42:14, Bernhard Bauer wrote:
> LGTM
> 
>
http://codereview.chromium.org/10522002/diff/11002/chrome/browser/extensions/...
> File chrome/browser/extensions/api/browsing_data/browsing_data_api.cc (right):
> 
>
http://codereview.chromium.org/10522002/diff/11002/chrome/browser/extensions/...
> chrome/browser/extensions/api/browsing_data/browsing_data_api.cc:222: //
Default
> to UNPROTECTED_WEB if the developer doesn't specify anything.
> On 2012/06/04 13:20:53, Mike West (chromium) wrote:
> > On 2012/06/04 12:42:17, Bernhard Bauer wrote:
> > > Note that the documentation says "If this object isn't specified", not "if
> > none
> > > of the values are true". If I specified { 'protected_web': false,
> > > 'unprotected_web': false }, I'd expect nothing to be cleared (even if
that's
> > not
> > > very useful).
> > 
> > Would you expect a warning to pop up, explaining that you're a moron?
> 
> We could return an error in that case (and I'm not completely averse to the
> error message containing the word moron), but silently doing something else
> seems to violate the principle of least surprise.

No, you're right. I'm not sure it's worth an error, but the behavior was
certainly not as described.

[Mike West, 2012-06-12 12:18:32]

Hi Aaron, Mihai, following up on http://codereview.chromium.org/10502015/ and
https://chromiumcodereview.appspot.com/10413072, I'd like to add deletion of
protected origins to the `chrome.browsingData` API.

Would you mind taking a look at this CL for that change?

[Mihai Parparita -not on Chrome, 2012-06-13 19:48:54]

LGTM

http://codereview.chromium.org/10522002/diff/11004/chrome/browser/extensions/...
File chrome/browser/extensions/api/browsing_data/browsing_data_api.cc (right):

http://codereview.chromium.org/10522002/diff/11004/chrome/browser/extensions/...
chrome/browser/extensions/api/browsing_data/browsing_data_api.cc:219: //
TODO(mkwst): Decide whether we should expose this to the extension API.
Seems reasonable to expose this.

[commit-bot: I haz the power, 2012-06-14 12:06:06]

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

[commit-bot: I haz the power, 2012-06-14 13:21:48]

Change committed as 142133