Fix issues related to <all_urls> in extensions.

Fix issues related to <all_urls> in extensions.

This fixes the order effective host permissions are initialized in ExtensionPermissionSet. This also fixes an issue in URLPattern where the the pattern for <all_urls> differs depending on if you make the pattern by parsing <all_urls> or by calling SetMatchAllURLs.

BUG=98418
TEST=ExtensionApiTest.CrossOriginXHR*


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

[jstritar, 2011-10-10 20:23:24]

http://codereview.chromium.org/8138004/diff/6021/chrome/common/extensions/url...
File chrome/common/extensions/url_pattern.cc (right):

http://codereview.chromium.org/8138004/diff/6021/chrome/common/extensions/url...
chrome/common/extensions/url_pattern.cc:263: }
This was the real bug- the <all_urls> URLPatterns were different depending on
how you create them. This method feels strange. What should happen to the other
variables if calling it with false? I wonder if URLPattern should be immutable,
or make SetMatchAllURLs private?

[Matt Perry, 2011-10-10 23:22:39]

http://codereview.chromium.org/8138004/diff/6021/chrome/common/extensions/url...
File chrome/common/extensions/url_pattern.cc (right):

http://codereview.chromium.org/8138004/diff/6021/chrome/common/extensions/url...
chrome/common/extensions/url_pattern.cc:263: }
On 2011/10/10 20:23:24, jstritar wrote:
> This was the real bug- the <all_urls> URLPatterns were different depending on
> how you create them. This method feels strange. What should happen to the
other
> variables if calling it with false? I wonder if URLPattern should be
immutable,
> or make SetMatchAllURLs private? 

Hmm.. I think the real bug is that the caller didn't call Parse. Without calling
Parse(), the URLPattern is in an invalid state. The caller should be changed to
do URLPattern pattern(SCHEMES_ALL, kAllUrlsPattern);

I think your change here is good, though. Toggling match_all_urls_ off should
leave the rest of the state unchanged (since this stuff would be set in a
particular state if match_all_urls_ were true anyway).

http://codereview.chromium.org/8138004/diff/6021/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/cross_origin_xhr/all_urls/manifest.json
(right):

http://codereview.chromium.org/8138004/diff/6021/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/cross_origin_xhr/all_urls/manifest.json:13:
{"path": "plugin.dll", "public": true},
nit: you can just leave 1 plugin in the list here

http://codereview.chromium.org/8138004/diff/6021/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/cross_origin_xhr/all_urls/test.html
(right):

http://codereview.chromium.org/8138004/diff/6021/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/cross_origin_xhr/all_urls/test.html:14:
chrome.tabs.sendRequest(testTabId, url, function(response) {
wrap this callback in chrome.test.callbackPass instead of manually calling
runNextTest.

[jstritar, 2011-10-11 15:13:38]

http://codereview.chromium.org/8138004/diff/6021/chrome/common/extensions/url...
File chrome/common/extensions/url_pattern.cc (right):

http://codereview.chromium.org/8138004/diff/6021/chrome/common/extensions/url...
chrome/common/extensions/url_pattern.cc:263: }
On 2011/10/10 23:22:39, Matt Perry wrote:
> On 2011/10/10 20:23:24, jstritar wrote:
> > This was the real bug- the <all_urls> URLPatterns were different depending
on
> > how you create them. This method feels strange. What should happen to the
> other
> > variables if calling it with false? I wonder if URLPattern should be
> immutable,
> > or make SetMatchAllURLs private? 
> 
> Hmm.. I think the real bug is that the caller didn't call Parse. Without
calling
> Parse(), the URLPattern is in an invalid state. The caller should be changed
to
> do URLPattern pattern(SCHEMES_ALL, kAllUrlsPattern);
> 
> I think your change here is good, though. Toggling match_all_urls_ off should
> leave the rest of the state unchanged (since this stuff would be set in a
> particular state if match_all_urls_ were true anyway).

Ah, I see... you're supposed to always call Parse. I updated
ExtensionPermissionSet::InitEffectiveHosts to initialize based on
kAllUrlsPattern.

http://codereview.chromium.org/8138004/diff/6021/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/cross_origin_xhr/all_urls/manifest.json
(right):

http://codereview.chromium.org/8138004/diff/6021/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/cross_origin_xhr/all_urls/manifest.json:13:
{"path": "plugin.dll", "public": true},
On 2011/10/10 23:22:39, Matt Perry wrote:
> nit: you can just leave 1 plugin in the list here

Done.

http://codereview.chromium.org/8138004/diff/6021/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/cross_origin_xhr/all_urls/test.html
(right):

http://codereview.chromium.org/8138004/diff/6021/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/cross_origin_xhr/all_urls/test.html:14:
chrome.tabs.sendRequest(testTabId, url, function(response) {
On 2011/10/10 23:22:39, Matt Perry wrote:
> wrap this callback in chrome.test.callbackPass instead of manually calling
> runNextTest.

Done.

[jstritar, 2011-10-11 17:26:06]

Actually you can hold off reviewing again... looking at trybot failures

[jstritar, 2011-10-13 19:30:02]

Okay, I think I fixed the test. 

I removed the part from ExtensionPermissionSet::InitEffectiveHosts that sets
<all_urls> if the set HasEffectiveAccessToAllHosts because the <all_urls> we
insert there may have different schemes than the all host patterns present in
user scripts or host permissions. Instead, the effective hosts will always be
the union of script and explicit hosts.

[Matt Perry, 2011-10-13 20:20:15]

lgtm

[commit-bot: I haz the power, 2011-10-13 22:58:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jstritar@chromium.org/8138004/21001

[commit-bot: I haz the power, 2011-10-14 00:19:47]

Change committed as 105422