Added support for file URI path wildcards in content settings

Added support for file URI path wildcards in content settings
patterns. I.e. "file:///*" matches all file URIs.

Full/explicit/absolute paths (e.g. "file:///foo/bar.html") are still
supported.

BUG=77149
TEST=ContentSettingsPattern* in unit_tests (added new ones).

[Francois, 2012-01-06 17:22:59]

Hi, render_messages.h had a bunch of cpplint warnings that I fixed; probably
shouldn't have included that with this CL... The only change of note in that
file is on line 133. Thanks!

[Bernhard Bauer, 2012-01-09 13:23:41]

Thanks for the patch! It looks pretty good, I just have some style comments
below.

Also, if you haven't already, please sign the Individual Contributor License
Agreement at http://code.google.com/legal/individual-cla-v1.0.html, and add
yourself to the AUTHORS file (you can do that in this CL).

http://codereview.chromium.org/9133002/diff/1/chrome/common/content_settings_...
File chrome/common/content_settings_pattern.cc (right):

http://codereview.chromium.org/9133002/diff/1/chrome/common/content_settings_...
chrome/common/content_settings_pattern.cc:204: else if (parts.is_path_wildcard)
The |else| is unnecessary if you have a |return| in the if-clause body.

http://codereview.chromium.org/9133002/diff/1/chrome/common/content_settings_...
chrome/common/content_settings_pattern.cc:206: else
Same here.

(If you'd keep the else clause, you should put braces around the bodies, as
they're more than one line; see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit...)

http://codereview.chromium.org/9133002/diff/1/chrome/common/content_settings_...
chrome/common/content_settings_pattern.cc:388: WithPathWildcard();
Nit: I think it's nicer if you align this call with |WithSchemeWildcard()|
above.

http://codereview.chromium.org/9133002/diff/1/chrome/common/content_settings_...
chrome/common/content_settings_pattern.cc:430: // one setting for all file URLs.
So the path should be ignored.
Nit: Can you update this comment?

http://codereview.chromium.org/9133002/diff/1/chrome/common/content_settings_...
File chrome/common/content_settings_pattern_unittest.cc (right):

http://codereview.chromium.org/9133002/diff/1/chrome/common/content_settings_...
chrome/common/content_settings_pattern_unittest.cc:189:
EXPECT_STREQ("file:///*", Pattern("file:///*").ToString().c_str());
You can use EXPECT_EQ and remove the |.c_str()|.

http://codereview.chromium.org/9133002/diff/1/chrome/common/render_messages.h
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/9133002/diff/1/chrome/common/render_messages.h...
chrome/common/render_messages.h:187: size_t /* capacity */);
Don't place semicolons after these macros:
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/ipc/ipc_message_macro...

In general, don't worry about lint errors in these files; they have a special
format (no header guards, for example).

[Francois, 2012-01-09 14:50:49]

Please see my changes and comments. I also signed the Individual Contributor
License. Thanks a lot!

http://codereview.chromium.org/9133002/diff/1/chrome/common/content_settings_...
File chrome/common/content_settings_pattern.cc (right):

http://codereview.chromium.org/9133002/diff/1/chrome/common/content_settings_...
chrome/common/content_settings_pattern.cc:204: else if (parts.is_path_wildcard)
On 2012/01/09 13:23:41, Bernhard Bauer wrote:
> The |else| is unnecessary if you have a |return| in the if-clause body.

Done.

http://codereview.chromium.org/9133002/diff/1/chrome/common/content_settings_...
chrome/common/content_settings_pattern.cc:206: else
On 2012/01/09 13:23:41, Bernhard Bauer wrote:
> Same here.
> 
> (If you'd keep the else clause, you should put braces around the bodies, as
> they're more than one line; see
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit...)

Done.

http://codereview.chromium.org/9133002/diff/1/chrome/common/content_settings_...
chrome/common/content_settings_pattern.cc:388: WithPathWildcard();
On 2012/01/09 13:23:41, Bernhard Bauer wrote:
> Nit: I think it's nicer if you align this call with |WithSchemeWildcard()|
> above.

Done.

http://codereview.chromium.org/9133002/diff/1/chrome/common/content_settings_...
chrome/common/content_settings_pattern.cc:430: // one setting for all file URLs.
So the path should be ignored.
On 2012/01/09 13:23:41, Bernhard Bauer wrote:
> Nit: Can you update this comment?

Done, but not sure whether the TODO needs to change as well? Are file paths
going to be supported for the foreseeable future?

http://codereview.chromium.org/9133002/diff/1/chrome/common/content_settings_...
File chrome/common/content_settings_pattern_unittest.cc (right):

http://codereview.chromium.org/9133002/diff/1/chrome/common/content_settings_...
chrome/common/content_settings_pattern_unittest.cc:189:
EXPECT_STREQ("file:///*", Pattern("file:///*").ToString().c_str());
On 2012/01/09 13:23:41, Bernhard Bauer wrote:
> You can use EXPECT_EQ and remove the |.c_str()|.

Done.

http://codereview.chromium.org/9133002/diff/1/chrome/common/render_messages.h
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/9133002/diff/1/chrome/common/render_messages.h...
chrome/common/render_messages.h:187: size_t /* capacity */);
On 2012/01/09 13:23:41, Bernhard Bauer wrote:
> Don't place semicolons after these macros:
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/ipc/ipc_message_macro...
> 
> In general, don't worry about lint errors in these files; they have a special
> format (no header guards, for example).

Done.

[markusheintz_, 2012-01-09 15:00:59]

Thanks a lot for contributing this CL :). That's great!

I only have a few minor comments.

http://codereview.chromium.org/9133002/diff/1/chrome/common/content_settings_...
File chrome/common/content_settings_pattern.cc (right):

http://codereview.chromium.org/9133002/diff/1/chrome/common/content_settings_...
chrome/common/content_settings_pattern.cc:430: // one setting for all file URLs.
So the path should be ignored.
Please also remove the TODO completely. Thanks

http://codereview.chromium.org/9133002/diff/1/chrome/common/content_settings_...
chrome/common/content_settings_pattern.cc:433: if (parts_.is_path_wildcard ||
parts_.path == std::string(url.path()))
Nit: You could get rid of the if. Just 
" return parts_.is_path_wildcard || parts_.path == std::string(url.path());"

http://codereview.chromium.org/9133002/diff/1/chrome/common/content_settings_...
File chrome/common/content_settings_pattern_parser.cc (right):

http://codereview.chromium.org/9133002/diff/1/chrome/common/content_settings_...
chrome/common/content_settings_pattern_parser.cc:45: const char*
PatternParser::kPathWildcard = "/*";
I would prefer that the path wildcard is '*' only, since '/' is the path
separator. (see line 15).

Please change to:
const char* PatternParser::kPathWildcard = "*";

http://codereview.chromium.org/9133002/diff/1/chrome/common/content_settings_...
chrome/common/content_settings_pattern_parser.cc:182: if (path == kPathWildcard)
see comment above.

change to:
if(path.substr(1) == kPathWildcard())

http://codereview.chromium.org/9133002/diff/1/chrome/common/content_settings_...
chrome/common/content_settings_pattern_parser.cc:206: return str +
kPathWildcard;
see comment above.

pls change to;
return str + kUrlPathSeparator + kPathWildcard;

http://codereview.chromium.org/9133002/diff/1/chrome/common/content_settings_...
File chrome/common/content_settings_pattern_unittest.cc (right):

http://codereview.chromium.org/9133002/diff/1/chrome/common/content_settings_...
chrome/common/content_settings_pattern_unittest.cc:184:
EXPECT_FALSE(Pattern("file://*/foo/bar/test.html").IsValid());
Maybe we could also add: 
EXPECT_FALSE(Pattern("file:///foo/*").IsValid());

[markusheintz_, 2012-01-09 15:24:36]

Adding Eric U; FYI

[Francois, 2012-01-09 15:33:00]

Thanks a lot Markus, please see my changes and comments.

http://codereview.chromium.org/9133002/diff/1/chrome/common/content_settings_...
File chrome/common/content_settings_pattern.cc (right):

http://codereview.chromium.org/9133002/diff/1/chrome/common/content_settings_...
chrome/common/content_settings_pattern.cc:430: // one setting for all file URLs.
So the path should be ignored.
On 2012/01/09 15:01:00, markusheintz_ wrote:
> Please also remove the TODO completely. Thanks

Done.

http://codereview.chromium.org/9133002/diff/1/chrome/common/content_settings_...
chrome/common/content_settings_pattern.cc:433: if (parts_.is_path_wildcard ||
parts_.path == std::string(url.path()))
On 2012/01/09 15:01:00, markusheintz_ wrote:
> Nit: You could get rid of the if. Just 
> " return parts_.is_path_wildcard || parts_.path == std::string(url.path());"

Done.

http://codereview.chromium.org/9133002/diff/1/chrome/common/content_settings_...
File chrome/common/content_settings_pattern_parser.cc (right):

http://codereview.chromium.org/9133002/diff/1/chrome/common/content_settings_...
chrome/common/content_settings_pattern_parser.cc:45: const char*
PatternParser::kPathWildcard = "/*";
On 2012/01/09 15:01:00, markusheintz_ wrote:
> I would prefer that the path wildcard is '*' only, since '/' is the path
> separator. (see line 15).
> 
> Please change to:
> const char* PatternParser::kPathWildcard = "*";  

Done.

http://codereview.chromium.org/9133002/diff/1/chrome/common/content_settings_...
chrome/common/content_settings_pattern_parser.cc:182: if (path == kPathWildcard)
On 2012/01/09 15:01:00, markusheintz_ wrote:
> see comment above.
> 
> change to:
> if(path.substr(1) == kPathWildcard())

Done.

http://codereview.chromium.org/9133002/diff/1/chrome/common/content_settings_...
chrome/common/content_settings_pattern_parser.cc:206: return str +
kPathWildcard;
On 2012/01/09 15:01:00, markusheintz_ wrote:
> see comment above.
> 
> pls change to;
> return str + kUrlPathSeparator + kPathWildcard;

Done.

http://codereview.chromium.org/9133002/diff/1/chrome/common/content_settings_...
File chrome/common/content_settings_pattern_unittest.cc (right):

http://codereview.chromium.org/9133002/diff/1/chrome/common/content_settings_...
chrome/common/content_settings_pattern_unittest.cc:184:
EXPECT_FALSE(Pattern("file://*/foo/bar/test.html").IsValid());
On 2012/01/09 15:01:00, markusheintz_ wrote:
> Maybe we could also add: 
> EXPECT_FALSE(Pattern("file:///foo/*").IsValid());

Done, but added it a few lines further down.

[Francois, 2012-01-13 06:29:02]

Hi, would one of you please take another look at this? Thanks!

[markusheintz_, 2012-01-13 16:15:38]

From my side this looks good.
@bauerb: Do you have any more comments?

Did you run it through the trybot again?

[Bernhard Bauer, 2012-01-13 16:21:29]

On 2012/01/13 16:15:38, markusheintz_ wrote:
> From my side this looks good.
> @bauerb: Do you have any more comments?
> 
> Did you run it through the trybot again?

LGTM.

I can also immediately check the commit box, then it'll run through the trybots
anyway.

[markusheintz_, 2012-01-13 16:43:46]

LGTM! Let's commit :)

On 2012/01/13 16:21:29, Bernhard Bauer wrote:
> On 2012/01/13 16:15:38, markusheintz_ wrote:
> > From my side this looks good.
> > @bauerb: Do you have any more comments?
> > 
> > Did you run it through the trybot again?
> 
> LGTM.
> 
> I can also immediately check the commit box, then it'll run through the
trybots
> anyway.

[commit-bot: I haz the power, 2012-01-17 09:42:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/francoisk777@gmail.com/9133002/18002

[commit-bot: I haz the power, 2012-01-17 09:42:33]

Can't apply patch for file AUTHORS.
While running patch -p0 --forward --force;
patching file AUTHORS
Hunk #1 FAILED at 156.
1 out of 1 hunk FAILED -- saving rejects to file AUTHORS.rej

[markusheintz_, 2012-01-17 09:52:57]

Hey Francois

I just noticed that we haven't submitted your CL yet. So I'm kind of fortunate
because I have some last comments about one thing I didn't notice yet. 

Could you please address the comments? That would be great.

Also please sync you client.

I will then commit the CL for you.

Thanks a lot.

http://codereview.chromium.org/9133002/diff/18002/chrome/common/content_setti...
File chrome/common/content_settings_pattern.cc (right):

http://codereview.chromium.org/9133002/diff/18002/chrome/common/content_setti...
chrome/common/content_settings_pattern.cc:319:
builder->WithScheme(url.scheme())->WithPath(url.path());
Please replace 319 with:

if (url.path().empty()) {
  builder->WithPath(url.path());
} else {
  builder->WithPathWildcard();
}

http://codereview.chromium.org/9133002/diff/18002/chrome/common/content_setti...
File chrome/common/content_settings_pattern_unittest.cc (right):

http://codereview.chromium.org/9133002/diff/18002/chrome/common/content_setti...
chrome/common/content_settings_pattern_unittest.cc:89:
EXPECT_STREQ("file:///foo/bar.html", pattern.ToString().c_str());
Please add here:

pattern = ContentSettingsPattern::FromURL(GURL("file:///"));
  EXPECT_TRUE(pattern.IsValid());

[Francois, 2012-01-18 15:34:45]

Hi Markus

Please find my latest changes in Patch Set 4.

I wasn't quite sure what the intention of your changes were. From the test case
you added it looks like you simply want "file:///" to now be a valid file
pattern (with path "/"), but from your change to
ContentSettingsPattern::FromURL() it looks like you may have intended for
"file:///" to be taken as another form of the wildcard.

I decided to submit changes based on the former assumption ("file:///" becomes a
valid pattern with path "/") instead of asking first because it seemed the most
likely. However, this required a change to ContentSettingsPattern::Validate() to
no longer reject file patterns with a path of "/". It also required the test
case which asserts that "file:///" is an invalid pattern to be removed from
content_settings_pattern_unittest.cc.

Thanks,
Francois

http://codereview.chromium.org/9133002/diff/18002/chrome/common/content_setti...
File chrome/common/content_settings_pattern.cc (right):

http://codereview.chromium.org/9133002/diff/18002/chrome/common/content_setti...
chrome/common/content_settings_pattern.cc:209: }
Please note that my new CL (Patch Set 4) removes the rejection of "/".

http://codereview.chromium.org/9133002/diff/18002/chrome/common/content_setti...
chrome/common/content_settings_pattern.cc:319:
builder->WithScheme(url.scheme())->WithPath(url.path());
On 2012/01/17 09:52:58, markusheintz_ wrote:
> Please replace 319 with:
> 
> if (url.path().empty()) {
>   builder->WithPath(url.path());
> } else {
>   builder->WithPathWildcard();
> }

Done, but I also set the scheme, which I hope is correct.

http://codereview.chromium.org/9133002/diff/18002/chrome/common/content_setti...
File chrome/common/content_settings_pattern_unittest.cc (right):

http://codereview.chromium.org/9133002/diff/18002/chrome/common/content_setti...
chrome/common/content_settings_pattern_unittest.cc:89:
EXPECT_STREQ("file:///foo/bar.html", pattern.ToString().c_str());
On 2012/01/17 09:52:58, markusheintz_ wrote:
> Please add here:
> 
> pattern = ContentSettingsPattern::FromURL(GURL("file:///"));
>   EXPECT_TRUE(pattern.IsValid());

Done.

http://codereview.chromium.org/9133002/diff/18002/chrome/common/content_setti...
chrome/common/content_settings_pattern_unittest.cc:176:
EXPECT_FALSE(Pattern("file:///").IsValid());
Please note that my new CL (Patch Set 4) removes this assertion.

[markusheintz_, 2012-01-18 19:45:24]

Sorry. I should have been more precise. 

I actually wanted to use the URL "file:///" as a (short form) way to express a
path wildcard. The reason for this is , if you get the origin of a file url you
loose the path component. I have one situation where I create a content settings
pattern from a URL that contains only the origin of a file URL so essentially
only the file - scheme and no path

So I would love to use file:/// as shortcut for file:///*.

Sorry for the confusion.

http://codereview.chromium.org/9133002/diff/18002/chrome/common/content_setti...
File chrome/common/content_settings_pattern.cc (right):

http://codereview.chromium.org/9133002/diff/18002/chrome/common/content_setti...
chrome/common/content_settings_pattern.cc:319:
builder->WithScheme(url.scheme())->WithPath(url.path());
On 2012/01/18 15:34:45, Francois wrote:
> On 2012/01/17 09:52:58, markusheintz_ wrote:
> > Please replace 319 with:
> > 
> > if (url.path().empty()) {
> >   builder->WithPath(url.path());
> > } else {
> >   builder->WithPathWildcard();
> > }
> 
> Done, but I also set the scheme, which I hope is correct.

Yes this is absolutely correct. I actually meant it the way to implemented it
:).

http://codereview.chromium.org/9133002/diff/28001/chrome/common/content_setti...
File chrome/common/content_settings_pattern_unittest.cc (right):

http://codereview.chromium.org/9133002/diff/28001/chrome/common/content_setti...
chrome/common/content_settings_pattern_unittest.cc:93:
EXPECT_TRUE(pattern.IsValid());
I should have been more precise here. Sorry.

These lines should be added as well.

EXPECT_TRUE(pattern.Matches(GURL("file:///")));
EXPECT_TRUE(pattern.Matches(GURL("file:///foo")));
EXPECT_TRUE(pattern.Matches(GURL("file:///foo/bar.html")));

[Francois, 2012-01-19 08:43:07]

Hi Markus

Please see my comments and my latest changes in Patch Set 5.

Thanks,
Francois

http://codereview.chromium.org/9133002/diff/18002/chrome/common/content_setti...
File chrome/common/content_settings_pattern.cc (right):

http://codereview.chromium.org/9133002/diff/18002/chrome/common/content_setti...
chrome/common/content_settings_pattern.cc:209: }
On 2012/01/18 15:34:45, Francois wrote:
> Please note that my new CL (Patch Set 4) removes the rejection of "/".

Added back in Patch Set 5.

http://codereview.chromium.org/9133002/diff/18002/chrome/common/content_setti...
chrome/common/content_settings_pattern.cc:319:
builder->WithScheme(url.scheme())->WithPath(url.path());
On 2012/01/18 19:45:24, markusheintz_ wrote:
> On 2012/01/18 15:34:45, Francois wrote:
> > On 2012/01/17 09:52:58, markusheintz_ wrote:
> > > Please replace 319 with:
> > > 
> > > if (url.path().empty()) {
> > >   builder->WithPath(url.path());
> > > } else {
> > >   builder->WithPathWildcard();
> > > }
> > 
> > Done, but I also set the scheme, which I hope is correct.
> 
> Yes this is absolutely correct. I actually meant it the way to implemented it
> :).

I also had to change it so that it sets the wildcard only if url.path() == "/"
(because GURL("file:///").path() == "/"), and the path, otherwise. It now reads:

    if (url.path() == "/") {
      builder->WithPathWildcard();
    } else {
      builder->WithPath(url.path());
    }

http://codereview.chromium.org/9133002/diff/18002/chrome/common/content_setti...
File chrome/common/content_settings_pattern_unittest.cc (right):

http://codereview.chromium.org/9133002/diff/18002/chrome/common/content_setti...
chrome/common/content_settings_pattern_unittest.cc:176:
EXPECT_FALSE(Pattern("file:///").IsValid());
On 2012/01/18 15:34:45, Francois wrote:
> Please note that my new CL (Patch Set 4) removes this assertion.

Added back in Patch Set 5.

http://codereview.chromium.org/9133002/diff/28001/chrome/common/content_setti...
File chrome/common/content_settings_pattern_unittest.cc (right):

http://codereview.chromium.org/9133002/diff/28001/chrome/common/content_setti...
chrome/common/content_settings_pattern_unittest.cc:93:
EXPECT_TRUE(pattern.IsValid());
On 2012/01/18 19:45:24, markusheintz_ wrote:
> I should have been more precise here. Sorry.
> 
> These lines should be added as well.
> 
> EXPECT_TRUE(pattern.Matches(GURL("file:///")));
> EXPECT_TRUE(pattern.Matches(GURL("file:///foo")));
> EXPECT_TRUE(pattern.Matches(GURL("file:///foo/bar.html")));

Done. Also added
  EXPECT_EQ("file:///*", pattern.ToString());