Support requesting subsets of host permissions using the permissions API.

Support requesting subsets of host permissions using the permissions API.


BUG=158004
TBR=ben@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182287

[not at google - send to devlin, 2013-02-11 23:44:21]

Matt/Yoz, not sure which of you is more comfortable reviewing this.

I also plan on updating the documentation for the optional permissions API,
since this is a little bit subtle, but that's not critical right now (I can do
that after branch point, would be nice to get this feature in, it goes well with
activeTab).

[Matt Perry, 2013-02-12 01:14:47]

https://codereview.chromium.org/12209094/diff/5001/extensions/common/url_patt...
File extensions/common/url_pattern_set.cc (right):

https://codereview.chromium.org/12209094/diff/5001/extensions/common/url_patt...
extensions/common/url_pattern_set.cc:243: if (needs_comma)
could this just be "it != set.begin()" ?

https://codereview.chromium.org/12209094/diff/5001/extensions/common/url_patt...
File extensions/common/url_pattern_set.h (right):

https://codereview.chromium.org/12209094/diff/5001/extensions/common/url_patt...
extensions/common/url_pattern_set.h:75: // {google.com}. For that, use
Encompasses.
This is a strange distinction. Why not just fix Contains to do what Encompasses
does? Are you worried that there are other uses of Contains that do not want to
match <all_urls>?

[not at google - send to devlin, 2013-02-12 01:49:36]

Verbosity aside - done.

https://codereview.chromium.org/12209094/diff/5001/extensions/common/url_patt...
File extensions/common/url_pattern_set.cc (right):

https://codereview.chromium.org/12209094/diff/5001/extensions/common/url_patt...
extensions/common/url_pattern_set.cc:243: if (needs_comma)
On 2013/02/12 01:14:47, Matt Perry wrote:
> could this just be "it != set.begin()" ?

Done.

https://codereview.chromium.org/12209094/diff/5001/extensions/common/url_patt...
File extensions/common/url_pattern_set.h (right):

https://codereview.chromium.org/12209094/diff/5001/extensions/common/url_patt...
extensions/common/url_pattern_set.h:75: // {google.com}. For that, use
Encompasses.
On 2013/02/12 01:14:47, Matt Perry wrote:
> This is a strange distinction. Why not just fix Contains to do what
Encompasses
> does? Are you worried that there are other uses of Contains that do not want
to
> match <all_urls>?

Yeah, it is strange. I had it like you said - just make Contains be smart - but
it introduced a problem (though I can't think of it right now...).

Hm well here is one, maybe. There are a lot of set-idiom-methods on these
objects (notably difference/intersection/union) which work just off the actual
patterns that have been added, and it may be surprising that Contains (obviously
also a set-idiom-method) behaves differently. Not convinced that's a big deal.

Actually perhaps the problem was that I was worried about all the corner cases,
and it seemed more manageable this way, specifically permissions.remove - I
wanted to make sure only actually-added permissions could be removed - but later
I saw that the function basically always returns "true" so it was moot.

I'll just change Contains and see what happens.

[Yoyo Zhou, 2013-02-12 06:46:25]

https://codereview.chromium.org/12209094/diff/5001/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/permissions/host_subsets/background.js
(right):

https://codereview.chromium.org/12209094/diff/5001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/permissions/host_subsets/background.js:193:
test.runTests([main]);
Not super important, but this doesn't have to be a single test; you have some
independent test cases in there.

[not at google - send to devlin, 2013-02-12 17:06:43]

p.s. fixed failing test due to changing the way that Contains works, but it was
nbd.

https://codereview.chromium.org/12209094/diff/5001/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/permissions/host_subsets/background.js
(right):

https://codereview.chromium.org/12209094/diff/5001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/permissions/host_subsets/background.js:193:
test.runTests([main]);
On 2013/02/12 06:46:25, Yoyo Zhou wrote:
> Not super important, but this doesn't have to be a single test; you have some
> independent test cases in there.

True, but that'd mess up the beautiful chain of continuations.

What is true is that it's a bit hard to see exactly what failed, but you can
kind of tell given the pattern it fails on - which splitting it up into
different tests wouldn't really improve on.

I can split it up I suppose, but yeah, don't see a huge advantage (and I'm not
sure these tests can be meaningfully split up into sections - I could cut it off
before the ftp tests sure, but yeah, it's not all that meaningful).

[Matt Perry, 2013-02-12 19:07:16]

lgtm

https://codereview.chromium.org/12209094/diff/2003/extensions/common/url_patt...
File extensions/common/url_pattern.h (right):

https://codereview.chromium.org/12209094/diff/2003/extensions/common/url_patt...
extensions/common/url_pattern.h:166: bool Encompasses(const URLPattern& other)
const;
rename to Contains?

https://codereview.chromium.org/12209094/diff/2003/extensions/common/url_patt...
File extensions/common/url_pattern_unittest.cc (right):

https://codereview.chromium.org/12209094/diff/2003/extensions/common/url_patt...
extensions/common/url_pattern_unittest.cc:453: testing::AssertionResult
OverlapsWith(const URLPattern& pattern1,
nit: Overlap would be more accurate since it doesn't imply direction

[Yoyo Zhou, 2013-02-12 19:45:04]

https://codereview.chromium.org/12209094/diff/2003/extensions/common/url_patt...
File extensions/common/url_pattern.h (right):

https://codereview.chromium.org/12209094/diff/2003/extensions/common/url_patt...
extensions/common/url_pattern.h:166: bool Encompasses(const URLPattern& other)
const;
On 2013/02/12 19:07:17, Matt Perry wrote:
> rename to Contains?

I thought about this as well, but you'd also want to rename the Encompasses on
URLPatternSet, so you have both

URLPatternSet::Contains(URLPatternSet)
and
URLPatternSet::Contains(URLPattern)

Not sure if this is more or less clear.

[Matt Perry, 2013-02-12 19:50:00]

https://codereview.chromium.org/12209094/diff/2003/extensions/common/url_patt...
File extensions/common/url_pattern.h (right):

https://codereview.chromium.org/12209094/diff/2003/extensions/common/url_patt...
extensions/common/url_pattern.h:166: bool Encompasses(const URLPattern& other)
const;
On 2013/02/12 19:45:04, Yoyo Zhou wrote:
> On 2013/02/12 19:07:17, Matt Perry wrote:
> > rename to Contains?
> 
> I thought about this as well, but you'd also want to rename the Encompasses on
> URLPatternSet, so you have both
> 
> URLPatternSet::Contains(URLPatternSet)
> and
> URLPatternSet::Contains(URLPattern)
> 
> Not sure if this is more or less clear.

Seems better to have Contains and ContainsPattern, then. Encompasses is a
synonym for Contains, so using it doesn't make things clearer.

[Yoyo Zhou, 2013-02-12 19:53:55]

https://codereview.chromium.org/12209094/diff/2003/extensions/common/url_patt...
File extensions/common/url_pattern.h (right):

https://codereview.chromium.org/12209094/diff/2003/extensions/common/url_patt...
extensions/common/url_pattern.h:166: bool Encompasses(const URLPattern& other)
const;
On 2013/02/12 19:50:00, Matt Perry wrote:
> On 2013/02/12 19:45:04, Yoyo Zhou wrote:
> > On 2013/02/12 19:07:17, Matt Perry wrote:
> > > rename to Contains?
> > 
> > I thought about this as well, but you'd also want to rename the Encompasses
on
> > URLPatternSet, so you have both
> > 
> > URLPatternSet::Contains(URLPatternSet)
> > and
> > URLPatternSet::Contains(URLPattern)
> > 
> > Not sure if this is more or less clear.
> 
> Seems better to have Contains and ContainsPattern, then. Encompasses is a
> synonym for Contains, so using it doesn't make things clearer.

Right. You do the dual meaning of containment - this set set-contains a pattern
that pattern-contains foo pattern - but I think that's understandable.

[not at google - send to devlin, 2013-02-12 19:56:20]

https://codereview.chromium.org/12209094/diff/2003/extensions/common/url_patt...
File extensions/common/url_pattern.h (right):

https://codereview.chromium.org/12209094/diff/2003/extensions/common/url_patt...
extensions/common/url_pattern.h:166: bool Encompasses(const URLPattern& other)
const;
On 2013/02/12 19:07:17, Matt Perry wrote:
> rename to Contains?

Contains seemed wrong to me, since it's not a container - I had IsSuperset for a
while but that seemed equally wrong - and like this confusing conversation is
unfolding, one of the reasons I didn't go with Contains is the confusion with
URLPattern vs URLPatternSet.

I'll just make it Contains.

https://codereview.chromium.org/12209094/diff/2003/extensions/common/url_patt...
File extensions/common/url_pattern_unittest.cc (right):

https://codereview.chromium.org/12209094/diff/2003/extensions/common/url_patt...
extensions/common/url_pattern_unittest.cc:453: testing::AssertionResult
OverlapsWith(const URLPattern& pattern1,
On 2013/02/12 19:07:17, Matt Perry wrote:
> nit: Overlap would be more accurate since it doesn't imply direction

Done.

[commit-bot: I haz the power, 2013-02-12 20:15:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/12209094/13003

[commit-bot: I haz the power, 2013-02-12 20:15:33]

Presubmit check for 12209094-13003 failed and returned exit status 1.

INFO:root:Found 13 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/common/extensions/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    

Presubmit checks took 2.1s to calculate.

[not at google - send to devlin, 2013-02-12 20:19:40]

huh?????????

[commit-bot: I haz the power, 2013-02-13 01:28:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/12209094/13003

[commit-bot: I haz the power, 2013-02-13 02:47:34]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-02-13 03:02:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/12209094/13003

[commit-bot: I haz the power, 2013-02-13 03:36:10]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-02-13 03:52:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/12209094/6018

[commit-bot: I haz the power, 2013-02-13 04:46:33]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-02-13 16:15:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/12209094/18001

[commit-bot: I haz the power, 2013-02-13 17:05:46]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win7_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-02-13 17:28:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/12209094/22005

[commit-bot: I haz the power, 2013-02-13 19:31:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/12209094/22005

[commit-bot: I haz the power, 2013-02-13 19:43:49]

Change committed as 182287