Add full support for filesystem URLs.

Add full support for filesystem URLs.

BUG=114484
TEST=existing filesystem tests don't break

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

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

[markusheintz_, 2011-12-21 10:36:44]

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

http://codereview.chromium.org/7811006/diff/49001/chrome/common/content_setti...
chrome/common/content_settings_pattern.cc:432: // than those of file:// URLs.
I don't understand this comment. What exactly is wrong?

I think it would be great to just have one setting for the origin file:/// since
they come from the same host.

[Aaron Boodman, 2011-12-21 18:10:06]

Can you please point me to a spec or description of filesystem URLs so that I
can help with the extension bits?

[ericu, 2011-12-21 19:01:08]

Thanks Aaron--the extension stuff's the biggest hole I've got left
right now.  I'd love to have your help.

The spec for the filesystem API is here:
http://dev.w3.org/2009/dap/file-system/file-dir-sys.html.
Unfortunately the URL format isn't yet specced; there's just a note in
the above spec that suggests the format that Chromium happens to be
using right now.  Here are some examples:

filesystem:http://example.com/temporary/path/to/file#ref
filesystem:file:///temporary/path/to/file?query
filesystem:https://example.com/persistent/path/to/file

Basically it's

filesystem:[security origin]/[persistent|temporary|external]/[path][query][ref]

I'd be happy to answer any questions you have; feel free to call me at
my desk or put something in calendar if needed.

On Wed, Dec 21, 2011 at 10:10 AM,  <aa@chromium.org> wrote:
> Can you please point me to a spec or description of filesystem URLs so that
> I
> can help with the extension bits?
>
> http://codereview.chromium.org/7811006/

[Aaron Boodman, 2012-01-19 23:47:36]

LGTM

This change doesn't have any affect on current extension functionality.

Over time it might be useful to make more parts of the extension system aware of
filesystem URLs, but I don't see any great need right now.

http://codereview.chromium.org/7811006/diff/56001/chrome/common/extensions/ur...
File chrome/common/extensions/url_pattern.cc (right):

http://codereview.chromium.org/7811006/diff/56001/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:156: // TODO(ericu): This will fail on
filesystem URLs.  What should it do?
Well, there aren't any places right now where we allow user-provided URLPatterns
to include filesystem:, so it's OK.

If we ever want users to be able to specify filesystem URLPatterns, then we'll
have to figure out what kind of syntax they would support and implement it here.

I guess this TODO can turn into:

TODO(ericu): Implement support for filesystem scheme?

http://codereview.chromium.org/7811006/diff/56001/chrome/renderer/extensions/...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

http://codereview.chromium.org/7811006/diff/56001/chrome/renderer/extensions/...
chrome/renderer/extensions/extension_dispatcher.cc:403: // extension permissions
at all?
Nothing needs to be done here.

http://codereview.chromium.org/7811006/diff/56001/chrome/renderer/extensions/...
File chrome/renderer/extensions/user_script_slave.cc (right):

http://codereview.chromium.org/7811006/diff/56001/chrome/renderer/extensions/...
chrome/renderer/extensions/user_script_slave.cc:88: // extension permissions at
all?
Nothing needs to be done here for now.

[ericu, 2012-01-20 04:34:29]

Awesome.  Thanks!
On Jan 19, 2012 3:47 PM, <aa@chromium.org> wrote:

> LGTM
>
> This change doesn't have any affect on current extension functionality.
>
> Over time it might be useful to make more parts of the extension system
> aware of
> filesystem URLs, but I don't see any great need right now.
>
>
> http://codereview.chromium.**org/7811006/diff/56001/chrome/**
>
common/extensions/url_pattern.**cc<http://codereview.chromium.org/7811006/diff/56001/chrome/common/extensions/url_pattern.cc>
> File chrome/common/extensions/url_**pattern.cc (right):
>
> http://codereview.chromium.**org/7811006/diff/56001/chrome/**
>
common/extensions/url_pattern.**cc#newcode156<http://codereview.chromium.org/7811006/diff/56001/chrome/common/extensions/url_pattern.cc#newcode156>
> chrome/common/extensions/url_**pattern.cc:156: // TODO(ericu): This will
> fail on filesystem URLs.  What should it do?
> Well, there aren't any places right now where we allow user-provided
> URLPatterns to include filesystem:, so it's OK.
>
> If we ever want users to be able to specify filesystem URLPatterns, then
> we'll have to figure out what kind of syntax they would support and
> implement it here.
>
> I guess this TODO can turn into:
>
> TODO(ericu): Implement support for filesystem scheme?
>
> http://codereview.chromium.**org/7811006/diff/56001/chrome/**
>
renderer/extensions/extension_**dispatcher.cc<http://codereview.chromium.org/7811006/diff/56001/chrome/renderer/extensions/extension_dispatcher.cc>
> File chrome/renderer/extensions/**extension_dispatcher.cc (right):
>
> http://codereview.chromium.**org/7811006/diff/56001/chrome/**
>
renderer/extensions/extension_**dispatcher.cc#newcode403<http://codereview.chromium.org/7811006/diff/56001/chrome/renderer/extensions/extension_dispatcher.cc#newcode403>
> chrome/renderer/extensions/**extension_dispatcher.cc:403: // extension
> permissions at all?
> Nothing needs to be done here.
>
> http://codereview.chromium.**org/7811006/diff/56001/chrome/**
>
renderer/extensions/user_**script_slave.cc<http://codereview.chromium.org/7811006/diff/56001/chrome/renderer/extensions/user_script_slave.cc>
> File chrome/renderer/extensions/**user_script_slave.cc (right):
>
> http://codereview.chromium.**org/7811006/diff/56001/chrome/**
>
renderer/extensions/user_**script_slave.cc#newcode88<http://codereview.chromium.org/7811006/diff/56001/chrome/renderer/extensions/user_script_slave.cc#newcode88>
> chrome/renderer/extensions/**user_script_slave.cc:88: // extension
> permissions at all?
> Nothing needs to be done here for now.
>
>
http://codereview.chromium.**org/7811006/<http://codereview.chromium.org/7811...
>

[ericu, 2012-02-03 00:18:36]

On Thu, Jan 19, 2012 at 3:47 PM,  <aa@chromium.org> wrote:
> LGTM
>
> This change doesn't have any affect on current extension functionality.
>
> Over time it might be useful to make more parts of the extension system
> aware of
> filesystem URLs, but I don't see any great need right now.
>
>
>
http://codereview.chromium.org/7811006/diff/56001/chrome/common/extensions/ur...
> File chrome/common/extensions/url_pattern.cc (right):
>
>
http://codereview.chromium.org/7811006/diff/56001/chrome/common/extensions/ur...
> chrome/common/extensions/url_pattern.cc:156: // TODO(ericu): This will
> fail on filesystem URLs.  What should it do?
> Well, there aren't any places right now where we allow user-provided
> URLPatterns to include filesystem:, so it's OK.
>
> If we ever want users to be able to specify filesystem URLPatterns, then
> we'll have to figure out what kind of syntax they would support and
> implement it here.
>
> I guess this TODO can turn into:
>
> TODO(ericu): Implement support for filesystem scheme?

It seems this is needed sooner rather than later.

ExtensionManifestTest.FileBrowserHandlers validates that
"filesystem:*.txt" successfully matches
GURL("filesystem:chrome-extension://foo/local/test.txt").  It looks
like that's there for ChromeOS's file browser.

That wildcard is rather broad; I'm wondering if we should perhaps
require that we have a full standard scheme and separator following
the "filesystem:", e.g. "filesystem:http://".  We don't seem to allow
"http:*", but require "http://" instead.  However, I don't know what
ChromeOS is using right now, and I don't want to accidentally break
them.  I'd be happy to patch them up at the same time, if the code's
in this repo.

Zelidrag, can you comment on what ChromeOS needs here?

Aaron, can you comment on what the right restrictions are here?

Other than that, we're just about ready to go with this change; the
GoogleURL and Webkit parts have already landed.

Thanks.

	Eric

[ericu, 2012-02-03 00:19:25]

This time really adding Zelidrag.

On Thu, Feb 2, 2012 at 4:18 PM, Eric Uhrhane <ericu@chromium.org> wrote:
> On Thu, Jan 19, 2012 at 3:47 PM,  <aa@chromium.org> wrote:
>> LGTM
>>
>> This change doesn't have any affect on current extension functionality.
>>
>> Over time it might be useful to make more parts of the extension system
>> aware of
>> filesystem URLs, but I don't see any great need right now.
>>
>>
>>
http://codereview.chromium.org/7811006/diff/56001/chrome/common/extensions/ur...
>> File chrome/common/extensions/url_pattern.cc (right):
>>
>>
http://codereview.chromium.org/7811006/diff/56001/chrome/common/extensions/ur...
>> chrome/common/extensions/url_pattern.cc:156: // TODO(ericu): This will
>> fail on filesystem URLs.  What should it do?
>> Well, there aren't any places right now where we allow user-provided
>> URLPatterns to include filesystem:, so it's OK.
>>
>> If we ever want users to be able to specify filesystem URLPatterns, then
>> we'll have to figure out what kind of syntax they would support and
>> implement it here.
>>
>> I guess this TODO can turn into:
>>
>> TODO(ericu): Implement support for filesystem scheme?
>
> It seems this is needed sooner rather than later.
>
> ExtensionManifestTest.FileBrowserHandlers validates that
> "filesystem:*.txt" successfully matches
> GURL("filesystem:chrome-extension://foo/local/test.txt").  It looks
> like that's there for ChromeOS's file browser.
>
> That wildcard is rather broad; I'm wondering if we should perhaps
> require that we have a full standard scheme and separator following
> the "filesystem:", e.g. "filesystem:http://".  We don't seem to allow
> "http:*", but require "http://" instead.  However, I don't know what
> ChromeOS is using right now, and I don't want to accidentally break
> them.  I'd be happy to patch them up at the same time, if the code's
> in this repo.
>
> Zelidrag, can you comment on what ChromeOS needs here?
>
> Aaron, can you comment on what the right restrictions are here?
>
> Other than that, we're just about ready to go with this change; the
> GoogleURL and Webkit parts have already landed.
>
> Thanks.
>
>        Eric

[ericu, 2012-02-07 00:49:43]

On Thu, Feb 2, 2012 at 4:19 PM, Eric Uhrhane <ericu@chromium.org> wrote:
> This time really adding Zelidrag.
>
> On Thu, Feb 2, 2012 at 4:18 PM, Eric Uhrhane <ericu@chromium.org> wrote:
>> On Thu, Jan 19, 2012 at 3:47 PM,  <aa@chromium.org> wrote:
>>> LGTM
>>>
>>> This change doesn't have any affect on current extension functionality.
>>>
>>> Over time it might be useful to make more parts of the extension system
>>> aware of
>>> filesystem URLs, but I don't see any great need right now.
>>>
>>>
>>>
http://codereview.chromium.org/7811006/diff/56001/chrome/common/extensions/ur...
>>> File chrome/common/extensions/url_pattern.cc (right):
>>>
>>>
http://codereview.chromium.org/7811006/diff/56001/chrome/common/extensions/ur...
>>> chrome/common/extensions/url_pattern.cc:156: // TODO(ericu): This will
>>> fail on filesystem URLs.  What should it do?
>>> Well, there aren't any places right now where we allow user-provided
>>> URLPatterns to include filesystem:, so it's OK.
>>>
>>> If we ever want users to be able to specify filesystem URLPatterns, then
>>> we'll have to figure out what kind of syntax they would support and
>>> implement it here.
>>>
>>> I guess this TODO can turn into:
>>>
>>> TODO(ericu): Implement support for filesystem scheme?
>>
>> It seems this is needed sooner rather than later.
>>
>> ExtensionManifestTest.FileBrowserHandlers validates that
>> "filesystem:*.txt" successfully matches
>> GURL("filesystem:chrome-extension://foo/local/test.txt").  It looks
>> like that's there for ChromeOS's file browser.
>>
>> That wildcard is rather broad; I'm wondering if we should perhaps
>> require that we have a full standard scheme and separator following
>> the "filesystem:", e.g. "filesystem:http://".  We don't seem to allow
>> "http:*", but require "http://" instead.  However, I don't know what
>> ChromeOS is using right now, and I don't want to accidentally break
>> them.  I'd be happy to patch them up at the same time, if the code's
>> in this repo.
>>
>> Zelidrag, can you comment on what ChromeOS needs here?

That is, where's the code that's invoking this, and what patterns are
you using?  Is the file browser using filesystem:*, or something more
specific?  If it's using filesystem:*, and we change that to, say,
filesystem:chrome-extension:*, would that break anything?

>> Aaron, can you comment on what the right restrictions are here?

Specifically, how is this going to be used?  Is this for security or
access control of some kind?

According to the comments in url_pattern.h, the pattern used in the
test ["filesystem:*.txt"] isn't actually legal, as it lacks the "://"
separator.  But URLPattern:Parse accepts bare ":" as a separator as
well, for nonstandard schemes.  That's why this worked before, but now
filesystem is becoming a standard scheme.

The code assumes that a URL has only one scheme, so it just stores and
checks scheme, host, port, and path.  But if the scheme is filesystem,
it may have an inner scheme of file, chrome-extension, http, or https.
 Do we want to store + check more info in every pattern, which would
require a bunch of filesystem-specific code here, or is usage of
URLPattern limited enough that we can get by with something simpler?
For example, if the only legal use of filesystem in URLPattern is
filesystem:chrome-extension, then we can just assume that
filesystem:*.txt means filesystem:chrome-extension://*.txt, and that
there shouldn't be a host or port, etc.

Thanks,

	Eric

[ericu, 2012-02-08 01:05:58]

Update: I'm prototyping a mostly-full filesystem URL parser for
URLPattern.  I'm banging the bugs out of it right now with some new
tests, and expect to post the code tomorrow.  Zelidrag, how can I
verify that I'm not going to break CrOS?

On Mon, Feb 6, 2012 at 4:49 PM, Eric Uhrhane <ericu@chromium.org> wrote:
> On Thu, Feb 2, 2012 at 4:19 PM, Eric Uhrhane <ericu@chromium.org> wrote:
>> This time really adding Zelidrag.
>>
>> On Thu, Feb 2, 2012 at 4:18 PM, Eric Uhrhane <ericu@chromium.org> wrote:
>>> On Thu, Jan 19, 2012 at 3:47 PM,  <aa@chromium.org> wrote:
>>>> LGTM
>>>>
>>>> This change doesn't have any affect on current extension functionality.
>>>>
>>>> Over time it might be useful to make more parts of the extension system
>>>> aware of
>>>> filesystem URLs, but I don't see any great need right now.
>>>>
>>>>
>>>>
http://codereview.chromium.org/7811006/diff/56001/chrome/common/extensions/ur...
>>>> File chrome/common/extensions/url_pattern.cc (right):
>>>>
>>>>
http://codereview.chromium.org/7811006/diff/56001/chrome/common/extensions/ur...
>>>> chrome/common/extensions/url_pattern.cc:156: // TODO(ericu): This will
>>>> fail on filesystem URLs.  What should it do?
>>>> Well, there aren't any places right now where we allow user-provided
>>>> URLPatterns to include filesystem:, so it's OK.
>>>>
>>>> If we ever want users to be able to specify filesystem URLPatterns, then
>>>> we'll have to figure out what kind of syntax they would support and
>>>> implement it here.
>>>>
>>>> I guess this TODO can turn into:
>>>>
>>>> TODO(ericu): Implement support for filesystem scheme?
>>>
>>> It seems this is needed sooner rather than later.
>>>
>>> ExtensionManifestTest.FileBrowserHandlers validates that
>>> "filesystem:*.txt" successfully matches
>>> GURL("filesystem:chrome-extension://foo/local/test.txt").  It looks
>>> like that's there for ChromeOS's file browser.
>>>
>>> That wildcard is rather broad; I'm wondering if we should perhaps
>>> require that we have a full standard scheme and separator following
>>> the "filesystem:", e.g. "filesystem:http://".  We don't seem to allow
>>> "http:*", but require "http://" instead.  However, I don't know what
>>> ChromeOS is using right now, and I don't want to accidentally break
>>> them.  I'd be happy to patch them up at the same time, if the code's
>>> in this repo.
>>>
>>> Zelidrag, can you comment on what ChromeOS needs here?
>
> That is, where's the code that's invoking this, and what patterns are
> you using?  Is the file browser using filesystem:*, or something more
> specific?  If it's using filesystem:*, and we change that to, say,
> filesystem:chrome-extension:*, would that break anything?
>
>>> Aaron, can you comment on what the right restrictions are here?
>
> Specifically, how is this going to be used?  Is this for security or
> access control of some kind?
>
> According to the comments in url_pattern.h, the pattern used in the
> test ["filesystem:*.txt"] isn't actually legal, as it lacks the "://"
> separator.  But URLPattern:Parse accepts bare ":" as a separator as
> well, for nonstandard schemes.  That's why this worked before, but now
> filesystem is becoming a standard scheme.
>
> The code assumes that a URL has only one scheme, so it just stores and
> checks scheme, host, port, and path.  But if the scheme is filesystem,
> it may have an inner scheme of file, chrome-extension, http, or https.
>  Do we want to store + check more info in every pattern, which would
> require a bunch of filesystem-specific code here, or is usage of
> URLPattern limited enough that we can get by with something simpler?
> For example, if the only legal use of filesystem in URLPattern is
> filesystem:chrome-extension, then we can just assume that
> filesystem:*.txt means filesystem:chrome-extension://*.txt, and that
> there shouldn't be a host or port, etc.
>
> Thanks,
>
>        Eric

[ericu, 2012-02-15 02:49:38]

I'd like to request a code review from each of you for this change.  It's a lot
of files, but in most cases the changes are quite small.  I've tried to find
someone who's an expert on each significant change as a reviewer, and of course
in many cases I need OWNER approval.

Adam, since you know about this change, could you take a quick look over
everything, including the few files with trivial changes?  Everyone else, I've
listed the files I'd like to ask for your help on below.

Thanks all.

	Eric


abarth: These files have only trivial dead code deletion or build changes.
chrome/browser/browsing_data_database_helper.h
chrome/browser/browsing_data_database_helper.cc
chrome/browser/browsing_data_indexed_db_helper.h
chrome/browser/browsing_data_local_storage_helper.h
build/temp_gyp/googleurl.gyp

aa:
chrome/browser/extensions/api/webrequest/webrequest_api.cc
chrome/browser/extensions/extension_webnavigation_api.cc
chrome/browser/web_applications/web_app.cc
chrome/common/extensions/extension.cc
chrome/common/extensions/extension_manifests_unittest.cc
chrome/common/extensions/url_pattern.cc
chrome/common/extensions/url_pattern.h
chrome/common/extensions/url_pattern_unittest.cc
chrome/test/data/extensions/api_test/filebrowser_component/main.js

markusheintz_:
chrome/common/content_settings_pattern.cc
chrome/common/content_settings_pattern_unittest.cc

avi:
content/public/common/url_constants.cc

eroman:
net/base/mime_sniffer.cc
net/base/net_util.cc
chrome/browser/net/url_fixer_upper.cc

sky:
ui/base/text/text_elider.cc
chrome/browser/autocomplete/autocomplete.cc
content/browser/browser_url_handler.cc
chrome/browser/history/url_index_private_data.cc

michaeln:
webkit/appcache/appcache_interfaces.cc

kinuko:
webkit/fileapi/file_system_util.cc
webkit/fileapi/sandbox_mount_point_provider.cc

[Avi (use Gerrit), 2012-02-15 03:03:05]

Neither the CL description nor your review request provides any information on
what a "filesystem" scheme is, and there is no linked bug. The fact that there's
one file that would go in content isn't saying good things about content needing
to know about this scheme, and I have no information to go by otherwise.

Please at least file a bug and link it.

[eroman, 2012-02-15 03:12:59]

It looks like Eric has a little more background in comment #3 of the
codereview.

On Tue, Feb 14, 2012 at 7:03 PM, <avi@chromium.org> wrote:

> Neither the CL description nor your review request provides any
> information on
> what a "filesystem" scheme is, and there is no linked bug. The fact that
> there's
> one file that would go in content isn't saying good things about content
> needing
> to know about this scheme, and I have no information to go by otherwise.
>
> Please at least file a bug and link it.
>
>
http://codereview.chromium.**org/7811006/<http://codereview.chromium.org/7811...
>

[Avi (use Gerrit), 2012-02-15 04:23:04]

Ah, I see. This still needs a bug linked to it, even if it only says "Implement
W3 filesystem API".

[Avi (use Gerrit), 2012-02-15 04:23:41]

Content LGTM

[eroman, 2012-02-15 06:07:12]

lgtm for net/

http://codereview.chromium.org/7811006/diff/99002/net/base/mime_sniffer.cc
File net/base/mime_sniffer.cc (right):

http://codereview.chromium.org/7811006/diff/99002/net/base/mime_sniffer.cc#ne...
net/base/mime_sniffer.cc:570: // We are willing to sniff the mime type for HTTP,
HTTPS, and FTP
You should probably update this comment to match. Or even better, change it to
not repeat what the code is already saying.

http://codereview.chromium.org/7811006/diff/99002/net/base/net_util.cc
File net/base/net_util.cc (right):

http://codereview.chromium.org/7811006/diff/99002/net/base/net_util.cc#newcod...
net/base/net_util.cc:1902: !url.SchemeIsFileSystem() && !url.has_query() &&
!url.has_ref()
I see, I think I understand a bit better what this change is about now:

You have introduced SchemeIsFileSystem() into GURL. However existing code may
not expect that, and may make assumption on stuff like what (url.IsStandard() &&
!url.SchemeIsFile())  entails.

Did you create this changelist by doing a search over Chrome codebase for all
occurrences of things like "IsStandard", "SchemeIsFile"?

Hmm that is tough. There could be other harder to find callers testing the
scheme string directly for instance.

[abarth-chromium, 2012-02-15 08:43:20]

chrome/browser/browsing_data_* LGTM

http://codereview.chromium.org/7811006/diff/99002/net/base/mime_sniffer.cc
File net/base/mime_sniffer.cc (right):

http://codereview.chromium.org/7811006/diff/99002/net/base/mime_sniffer.cc#ne...
net/base/mime_sniffer.cc:570: // We are willing to sniff the mime type for HTTP,
HTTPS, and FTP
On 2012/02/15 06:07:13, eroman wrote:
> You should probably update this comment to match. Or even better, change it to
> not repeat what the code is already saying.

Or just remove the comment.  I'm sorry to have added it since it's redundant
with the code below.  (I'd also change the code below to use the scheme
constants, which have existed now for a long time.)

http://codereview.chromium.org/7811006/diff/99002/webkit/fileapi/sandbox_moun...
File webkit/fileapi/sandbox_mount_point_provider.cc (right):

http://codereview.chromium.org/7811006/diff/99002/webkit/fileapi/sandbox_moun...
webkit/fileapi/sandbox_mount_point_provider.cc:639: return url.inner_url() &&
IsAllowedScheme(*url.inner_url());
I wonder if places like this should be able to check more generally for the
presence of an inner URL rather than assuming that we should consult the inner
URL iff the scheme if FileSystem.

[sky, 2012-02-15 16:17:27]

I'm adding Brett on to this. He doesn't have to review it, but I want to make
sure he's aware of these changes (I'm sure he is because you changed GURL, but
none-the-less).

http://codereview.chromium.org/7811006/diff/99002/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete.cc (right):

http://codereview.chromium.org/7811006/diff/99002/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete.cc:165: if
(LowerCaseEqualsASCII(parsed_scheme, chrome::kFileSystemScheme)) {
Can you add test coverage for these cases?

http://codereview.chromium.org/7811006/diff/99002/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete.cc:464: parts.inner_parsed() &&
parts.inner_parsed()->scheme.is_valid()) {
spacing is off.

http://codereview.chromium.org/7811006/diff/99002/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (left):

http://codereview.chromium.org/7811006/diff/99002/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:175: if (!(url.SchemeIsFile() || url.IsStandard()))
nit: I prefer !foo && !foo. Easier to parse.

[Aaron Boodman, 2012-02-15 21:31:17]

Here is a first wave of comments. URLPattern::MatchesURL() is currently blowing
my mind, but I'm hoping that is because I was up too late, and it will make more
sense on the second pass.

http://codereview.chromium.org/7811006/diff/99002/chrome/common/extensions/ur...
File chrome/common/extensions/url_pattern.cc (right):

http://codereview.chromium.org/7811006/diff/99002/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:159: size_t scheme_end_pos;
Please always initialize primitives. I wish we had a clang plugin for this.

http://codereview.chromium.org/7811006/diff/99002/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:159: size_t scheme_end_pos;
So is it true at this point that has_simple_scheme_separator and
has_standard_scheme_separator are exclusive? If so, it would be nice to put a
CHECK to that effect to remind the reader.

http://codereview.chromium.org/7811006/diff/99002/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:168: if (!SetScheme(pattern.substr(0,
scheme_end_pos)))
Would it be simpler to restructure parsing the scheme bit like this (in
psuedo-code):

scheme_separator_start = input.find(':');
scheme_separator_end = scheme_separator_start + 1;
if (input.length > (scheme_separator_start + 2) &&
    input[scheme_separator_start + 1] == '/' &&
    input[scheme_separator_start + 2] == '/')) {
  scheme_separator_end = scheme_separator_start + 3;
}

scheme = input.substr(0, scheme_separator_start);
scheme_separator = input.substr(scheme_separator_start, scheme_separator_end);

< validate legit scheme/separator combinations >

< if outer scheme is filesystem, parse inner scheme >

http://codereview.chromium.org/7811006/diff/99002/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:171: if (!has_simple_scheme_separator &&
has_standard_scheme_separator &&
This method is getting pretty hairy. Is it possible to break it up into parts?

http://codereview.chromium.org/7811006/diff/99002/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:307: bool
URLPattern::SetInnerScheme(const std::string& scheme) {
It would be more clear to name this param "inner_scheme".

http://codereview.chromium.org/7811006/diff/99002/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:311: valid_inner_schemes_ = SCHEME_HTTP
| SCHEME_HTTPS | SCHEME_FTP |
This looks weird. In your travels, did you come to understand why SetScheme()
modifies valid_schemes_?

http://codereview.chromium.org/7811006/diff/99002/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:359: std::string scheme = scheme_;
More clear to use a ternary operator here?

http://codereview.chromium.org/7811006/diff/99002/chrome/common/extensions/ur...
File chrome/common/extensions/url_pattern.h (right):

http://codereview.chromium.org/7811006/diff/99002/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.h:102: // Sets the bitmask of valid schemes
and inner_schemes.
Please put an empty line before each of these comments.

http://codereview.chromium.org/7811006/diff/99002/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.h:132: // patterns whose scheme has already
been set to "filesystem". This can be a
> has already been set to "filesystem".

Why make this restriction? It seems easier to just ignore it if the outer scheme
doesn't support nested schemes.

http://codereview.chromium.org/7811006/diff/99002/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.h:137: // Note: You should use
MatchesScheme() instead of this getter unless you
In retrospect, these warnings are overly scary. Maybe something like:

Returns the scheme component of the pattern. Note that it is usually better to
call MatchesScheme(), since that will also handle the case where the scheme
component is a wildcard.

http://codereview.chromium.org/7811006/diff/99002/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.h:148: // Returns true if the specified
scheme can be used in this URL pattern, and
.replace("specified scheme", "specified inner scheme");

http://codereview.chromium.org/7811006/diff/99002/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.h:225: // A bitmask containing the inner
schemes which are considered valid for this
.insertBefore("\n");

[ericu, 2012-02-15 22:06:37]

On 2012/02/15 04:23:04, Avi wrote:
> Ah, I see. This still needs a bug linked to it, even if it only says
"Implement
> W3 filesystem API".

Right you are--sorry about that.  I've logged an overall bug for this change,
and linked the manifestations that people have already complained about to the
master bug.

[ericu, 2012-02-15 22:09:40]

On 2012/02/15 06:07:12, eroman wrote:
> lgtm for net/
> 
> http://codereview.chromium.org/7811006/diff/99002/net/base/mime_sniffer.cc
> File net/base/mime_sniffer.cc (right):
> 
>
http://codereview.chromium.org/7811006/diff/99002/net/base/mime_sniffer.cc#ne...
> net/base/mime_sniffer.cc:570: // We are willing to sniff the mime type for
HTTP,
> HTTPS, and FTP
> You should probably update this comment to match. Or even better, change it to
> not repeat what the code is already saying.
> 
> http://codereview.chromium.org/7811006/diff/99002/net/base/net_util.cc
> File net/base/net_util.cc (right):
> 
>
http://codereview.chromium.org/7811006/diff/99002/net/base/net_util.cc#newcod...
> net/base/net_util.cc:1902: !url.SchemeIsFileSystem() && !url.has_query() &&
> !url.has_ref()
> I see, I think I understand a bit better what this change is about now:
> 
> You have introduced SchemeIsFileSystem() into GURL. However existing code may
> not expect that, and may make assumption on stuff like what (url.IsStandard()
&&
> !url.SchemeIsFile())  entails.
> 
> Did you create this changelist by doing a search over Chrome codebase for all
> occurrences of things like "IsStandard", "SchemeIsFile"?

SchemeIsFile, yes--that's pretty much where I started.  I'll take another look
for IsStandard and see if there are any suprises.

> Hmm that is tough. There could be other harder to find callers testing the
> scheme string directly for instance.

They probably shouldn't be doing that, should they?  That's part of the point of
the googleurl library.  Of course, I was surprised by how much other URL parsing
code there still is...

[ericu, 2012-02-15 22:11:47]

On 2012/02/15 16:17:27, sky wrote:
> I'm adding Brett on to this. He doesn't have to review it, but I want to make
> sure he's aware of these changes (I'm sure he is because you changed GURL, but
> none-the-less).

Thanks for that--I'd meant to CC him, of course, but missed the address in my
hurry.

>
http://codereview.chromium.org/7811006/diff/99002/chrome/browser/autocomplete...
> File chrome/browser/autocomplete/autocomplete.cc (right):
> 
>
http://codereview.chromium.org/7811006/diff/99002/chrome/browser/autocomplete...
> chrome/browser/autocomplete/autocomplete.cc:165: if
> (LowerCaseEqualsASCII(parsed_scheme, chrome::kFileSystemScheme)) {
> Can you add test coverage for these cases?

Will do.

>
http://codereview.chromium.org/7811006/diff/99002/chrome/browser/autocomplete...
> chrome/browser/autocomplete/autocomplete.cc:464: parts.inner_parsed() &&
> parts.inner_parsed()->scheme.is_valid()) {
> spacing is off.
> 
> http://codereview.chromium.org/7811006/diff/99002/ui/base/text/text_elider.cc
> File ui/base/text/text_elider.cc (left):
> 
>
http://codereview.chromium.org/7811006/diff/99002/ui/base/text/text_elider.cc...
> ui/base/text/text_elider.cc:175: if (!(url.SchemeIsFile() ||
url.IsStandard()))
> nit: I prefer !foo && !foo. Easier to parse.

I find it easier the other way, but I'm easy.

[ericu, 2012-02-15 22:32:10]

http://codereview.chromium.org/7811006/diff/99002/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (left):

http://codereview.chromium.org/7811006/diff/99002/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:175: if (!(url.SchemeIsFile() || url.IsStandard()))
On 2012/02/15 16:17:28, sky wrote:
> nit: I prefer !foo && !foo. Easier to parse.

Actually, you're commenting on the "before" code, not the "after".  The only
reason I made this change is that the old code was misleading.  The new code is
equivalent, since file URLs are standard URLs.

[ericu, 2012-02-15 22:48:09]

http://codereview.chromium.org/7811006/diff/99002/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete.cc (right):

http://codereview.chromium.org/7811006/diff/99002/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete.cc:464: parts.inner_parsed() &&
parts.inner_parsed()->scheme.is_valid()) {
On 2012/02/15 16:17:28, sky wrote:
> spacing is off.

Got it.

http://codereview.chromium.org/7811006/diff/99002/net/base/mime_sniffer.cc
File net/base/mime_sniffer.cc (right):

http://codereview.chromium.org/7811006/diff/99002/net/base/mime_sniffer.cc#ne...
net/base/mime_sniffer.cc:570: // We are willing to sniff the mime type for HTTP,
HTTPS, and FTP
On 2012/02/15 08:43:20, abarth wrote:
> On 2012/02/15 06:07:13, eroman wrote:
> > You should probably update this comment to match. Or even better, change it
to
> > not repeat what the code is already saying.
> 
> Or just remove the comment.  I'm sorry to have added it since it's redundant
> with the code below.  (I'd also change the code below to use the scheme
> constants, which have existed now for a long time.)

I'll kill the comment and convert to the constants.

http://codereview.chromium.org/7811006/diff/99002/webkit/fileapi/sandbox_moun...
File webkit/fileapi/sandbox_mount_point_provider.cc (right):

http://codereview.chromium.org/7811006/diff/99002/webkit/fileapi/sandbox_moun...
webkit/fileapi/sandbox_mount_point_provider.cc:639: return url.inner_url() &&
IsAllowedScheme(*url.inner_url());
On 2012/02/15 08:43:20, abarth wrote:
> I wonder if places like this should be able to check more generally for the
> presence of an inner URL rather than assuming that we should consult the inner
> URL iff the scheme if FileSystem.

Eventually, probably...once we decide what all we might use inner_url for.  It's
a clear possibility for view-source, but we probably don't want to implement
full generality without some idea of if we'll ever use it or not, since it will
complicate a bunch of code.

[ericu, 2012-02-16 01:42:56]

I've addressed all current comments with the exception of those in
url_pattern.h; I'll make those changes on Monday.

http://codereview.chromium.org/7811006/diff/99002/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete.cc (right):

http://codereview.chromium.org/7811006/diff/99002/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete.cc:165: if
(LowerCaseEqualsASCII(parsed_scheme, chrome::kFileSystemScheme)) {
On 2012/02/15 16:17:28, sky wrote:
> Can you add test coverage for these cases?

Done.

http://codereview.chromium.org/7811006/diff/99002/chrome/common/extensions/ur...
File chrome/common/extensions/url_pattern.cc (right):

http://codereview.chromium.org/7811006/diff/99002/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:159: size_t scheme_end_pos;
On 2012/02/15 21:31:18, Aaron Boodman wrote:
> Please always initialize primitives. I wish we had a clang plugin for this.

Done.

http://codereview.chromium.org/7811006/diff/99002/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:159: size_t scheme_end_pos;
On 2012/02/15 21:31:18, Aaron Boodman wrote:
> So is it true at this point that has_simple_scheme_separator and
> has_standard_scheme_separator are exclusive? If so, it would be nice to put a
> CHECK to that effect to remind the reader.

No.  It used to be true, but now we have "filesystem:http://" which tests
positive for both.  As long as the ':' comes before a '://', it counts.

http://codereview.chromium.org/7811006/diff/99002/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:168: if (!SetScheme(pattern.substr(0,
scheme_end_pos)))
On 2012/02/15 21:31:18, Aaron Boodman wrote:
> Would it be simpler to restructure parsing the scheme bit like this (in
> psuedo-code):
> 
> scheme_separator_start = input.find(':');
> scheme_separator_end = scheme_separator_start + 1;
> if (input.length > (scheme_separator_start + 2) &&
>     input[scheme_separator_start + 1] == '/' &&
>     input[scheme_separator_start + 2] == '/')) {
>   scheme_separator_end = scheme_separator_start + 3;
> }
> 
> scheme = input.substr(0, scheme_separator_start);
> scheme_separator = input.substr(scheme_separator_start, scheme_separator_end);
>
> < validate legit scheme/separator combinations >
> 
> < if outer scheme is filesystem, parse inner scheme >

Thanks for the suggestion; I think it's a lot clearer now.  Some of the
complexity got in there when I was trying to keep supporting CrOS's
"filesystem:*" wildcards here, instead of converting them off in extension-land
before calling in.

http://codereview.chromium.org/7811006/diff/99002/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:171: if (!has_simple_scheme_separator &&
has_standard_scheme_separator &&
On 2012/02/15 21:31:18, Aaron Boodman wrote:
> This method is getting pretty hairy. Is it possible to break it up into parts?

Do you still think it's necessary after the rewrite?

http://codereview.chromium.org/7811006/diff/99002/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:307: bool
URLPattern::SetInnerScheme(const std::string& scheme) {
On 2012/02/15 21:31:18, Aaron Boodman wrote:
> It would be more clear to name this param "inner_scheme".

Done.

http://codereview.chromium.org/7811006/diff/99002/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:311: valid_inner_schemes_ = SCHEME_HTTP
| SCHEME_HTTPS | SCHEME_FTP |
On 2012/02/15 21:31:18, Aaron Boodman wrote:
> This looks weird. In your travels, did you come to understand why SetScheme()
> modifies valid_schemes_?

It only does so if you're putting in a wildcard.  The comment in the header says
"The '*' scheme pattern excludes file URLs.".  The code for SetScheme also rules
out ftp; I wasn't sure if that was a bug, but it disagrees with the comments.  I
left FTP in, but they should probably match.  Which way should we go?

http://codereview.chromium.org/7811006/diff/99002/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:359: std::string scheme = scheme_;
On 2012/02/15 21:31:18, Aaron Boodman wrote:
> More clear to use a ternary operator here?

Done.

http://codereview.chromium.org/7811006/diff/99002/chrome/common/extensions/ur...
File chrome/common/extensions/url_pattern.h (right):

http://codereview.chromium.org/7811006/diff/99002/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.h:132: // patterns whose scheme has already
been set to "filesystem". This can be a
On 2012/02/15 21:31:18, Aaron Boodman wrote:
> > has already been set to "filesystem".
> 
> Why make this restriction? It seems easier to just ignore it if the outer
scheme
> doesn't support nested schemes.

If you're doing something that's obviously wrong, I'd much rather be able to
DCHECK so that you find out about it.

http://codereview.chromium.org/7811006/diff/99002/net/base/mime_sniffer.cc
File net/base/mime_sniffer.cc (right):

http://codereview.chromium.org/7811006/diff/99002/net/base/mime_sniffer.cc#ne...
net/base/mime_sniffer.cc:570: // We are willing to sniff the mime type for HTTP,
HTTPS, and FTP
On 2012/02/15 22:48:09, ericu wrote:
> On 2012/02/15 08:43:20, abarth wrote:
> > On 2012/02/15 06:07:13, eroman wrote:
> > > You should probably update this comment to match. Or even better, change
it
> to
> > > not repeat what the code is already saying.
> > 
> > Or just remove the comment.  I'm sorry to have added it since it's redundant
> > with the code below.  (I'd also change the code below to use the scheme
> > constants, which have existed now for a long time.)
> 
> I'll kill the comment and convert to the constants.

Actually, I don't think the constants I've been using are accessible from
/net/base.  Do you know of any that are?

http://codereview.chromium.org/7811006/diff/99002/net/base/net_util.cc
File net/base/net_util.cc (right):

http://codereview.chromium.org/7811006/diff/99002/net/base/net_util.cc#newcod...
net/base/net_util.cc:1902: !url.SchemeIsFileSystem() && !url.has_query() &&
!url.has_ref()
On 2012/02/15 06:07:13, eroman wrote:
> I see, I think I understand a bit better what this change is about now:
> 
> You have introduced SchemeIsFileSystem() into GURL. However existing code may
> not expect that, and may make assumption on stuff like what (url.IsStandard()
&&
> !url.SchemeIsFile())  entails.
> 
> Did you create this changelist by doing a search over Chrome codebase for all
> occurrences of things like "IsStandard", "SchemeIsFile"?

Having looked through again, I found two files that are probably OK as they are,
but would be better with a bit of filesystem-specific code.  I'll either add
them to the patch next week [I'm out until Monday] or log bugs for later.

> Hmm that is tough. There could be other harder to find callers testing the
> scheme string directly for instance.

http://codereview.chromium.org/7811006/diff/105001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/autocomplete_unittest.cc (right):

http://codereview.chromium.org/7811006/diff/105001/chrome/browser/autocomplet...
chrome/browser/autocomplete/autocomplete_unittest.cc:386: SCOPED_TRACE(i);
This was debugging code I accidentally uploaded.  I'll take it back out.

[michaeln, 2012-02-16 01:46:21]

> michaeln:
> webkit/appcache/appcache_interfaces.cc

lgtm (note: didn't really look at the meat of this change)

[brettw, 2012-02-16 05:06:23]

I only spot checked this but it was basically what I expected. (LGTM)

[markusheintz_, 2012-02-16 09:55:37]

http://codereview.chromium.org/7811006/diff/105001/chrome/common/content_sett...
File chrome/common/content_settings_pattern_unittest.cc (right):

http://codereview.chromium.org/7811006/diff/105001/chrome/common/content_sett...
chrome/common/content_settings_pattern_unittest.cc:128:
EXPECT_TRUE(pattern.Matches(GURL("file:///temporary")));
That means a content settings pattern for "filesystem:file:///temporary/foo.txt"
would also match a file url for the file "/temporary" on the local file system
("file:///temporary"). Same for any other 

Just to refresh my memory:
filesystem:file:///temporary/foo.txt are two separate files.
file:///temporary/foo.txt are two different files

http://codereview.chromium.org/7811006/diff/105001/chrome/common/extensions/u...
File chrome/common/extensions/url_pattern_unittest.cc (right):

http://codereview.chromium.org/7811006/diff/105001/chrome/common/extensions/u...
chrome/common/extensions/url_pattern_unittest.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
nit: Please change to 2012

[markusheintz_, 2012-02-16 10:02:57]

Only file header nits.

http://codereview.chromium.org/7811006/diff/105001/chrome/browser/browsing_da...
File chrome/browser/browsing_data_database_helper.cc (right):

http://codereview.chromium.org/7811006/diff/105001/chrome/browser/browsing_da...
chrome/browser/browsing_data_database_helper.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
nit: s/2011/2012

http://codereview.chromium.org/7811006/diff/105001/chrome/browser/browsing_da...
File chrome/browser/browsing_data_database_helper.h (right):

http://codereview.chromium.org/7811006/diff/105001/chrome/browser/browsing_da...
chrome/browser/browsing_data_database_helper.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
nit: s/2011/2012

http://codereview.chromium.org/7811006/diff/105001/chrome/browser/browsing_da...
File chrome/browser/browsing_data_indexed_db_helper.h (right):

http://codereview.chromium.org/7811006/diff/105001/chrome/browser/browsing_da...
chrome/browser/browsing_data_indexed_db_helper.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
nit: s/2011/2012

http://codereview.chromium.org/7811006/diff/105001/chrome/browser/browsing_da...
File chrome/browser/browsing_data_local_storage_helper.h (right):

http://codereview.chromium.org/7811006/diff/105001/chrome/browser/browsing_da...
chrome/browser/browsing_data_local_storage_helper.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
nit: s/2011/2012

http://codereview.chromium.org/7811006/diff/105001/chrome/browser/net/url_fix...
File chrome/browser/net/url_fixer_upper.cc (right):

http://codereview.chromium.org/7811006/diff/105001/chrome/browser/net/url_fix...
chrome/browser/net/url_fixer_upper.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
nit: s/2011/2012

http://codereview.chromium.org/7811006/diff/105001/chrome/common/extensions/u...
File chrome/common/extensions/url_pattern.h (right):

http://codereview.chromium.org/7811006/diff/105001/chrome/common/extensions/u...
chrome/common/extensions/url_pattern.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
nit: s/2011/2012

http://codereview.chromium.org/7811006/diff/105001/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/filebrowser_component/main.js (right):

http://codereview.chromium.org/7811006/diff/105001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/filebrowser_component/main.js:1: //
Copyright (c) 2011 The Chromium Authors. All rights reserved.
nit: s/2011/2012

http://codereview.chromium.org/7811006/diff/105001/content/browser/browser_ur...
File content/browser/browser_url_handler.cc (right):

http://codereview.chromium.org/7811006/diff/105001/content/browser/browser_ur...
content/browser/browser_url_handler.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
nit: s/2011/2012

http://codereview.chromium.org/7811006/diff/105001/content/public/common/url_...
File content/public/common/url_constants.cc (right):

http://codereview.chromium.org/7811006/diff/105001/content/public/common/url_...
content/public/common/url_constants.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
nit: s/2011/2012

http://codereview.chromium.org/7811006/diff/105001/net/base/mime_sniffer.cc
File net/base/mime_sniffer.cc (right):

http://codereview.chromium.org/7811006/diff/105001/net/base/mime_sniffer.cc#n...
net/base/mime_sniffer.cc:1: // Copyright (c) 2011 The Chromium Authors. All
rights reserved.
nit: s/2011/2012

http://codereview.chromium.org/7811006/diff/105001/webkit/appcache/appcache_i...
File webkit/appcache/appcache_interfaces.cc (right):

http://codereview.chromium.org/7811006/diff/105001/webkit/appcache/appcache_i...
webkit/appcache/appcache_interfaces.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
nit: s/2011/2012

[kinuko, 2012-02-16 14:31:35]

webkit/fileapi changes lgtm.

[sky, 2012-02-16 15:43:25]

http://codereview.chromium.org/7811006/diff/105001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/autocomplete.cc (right):

http://codereview.chromium.org/7811006/diff/105001/chrome/browser/autocomplet...
chrome/browser/autocomplete/autocomplete.cc:167: ParseFileSystemURL(text.data(),
text.length(), parts);
Is it really safe to reuse parts here?

[kinuko, 2012-02-17 23:47:37]

http://codereview.chromium.org/7811006/diff/105001/webkit/fileapi/file_system...
File webkit/fileapi/file_system_util.cc (right):

http://codereview.chromium.org/7811006/diff/105001/webkit/fileapi/file_system...
webkit/fileapi/file_system_util.cc:40: if (inner_path.compare(0,
strlen(kPersistentDir), kPersistentDir) == 0) {
nit: I think we can use arraysize here

[Aaron Boodman, 2012-02-18 11:17:12]

I'm out for a week. Matt, can you take over for me on this review?

[ericu, 2012-02-22 00:00:50]

I've rolled in all the feedback.

Markus, I'm unsure what the correct behavior is w.r.t. file paths; see my
comment.

http://codereview.chromium.org/7811006/diff/99002/chrome/common/extensions/ur...
File chrome/common/extensions/url_pattern.h (right):

http://codereview.chromium.org/7811006/diff/99002/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.h:137: // Note: You should use
MatchesScheme() instead of this getter unless you
On 2012/02/15 21:31:18, Aaron Boodman wrote:
> In retrospect, these warnings are overly scary. Maybe something like:
> 
> Returns the scheme component of the pattern. Note that it is usually better to
> call MatchesScheme(), since that will also handle the case where the scheme
> component is a wildcard.

Done.

http://codereview.chromium.org/7811006/diff/99002/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.h:148: // Returns true if the specified
scheme can be used in this URL pattern, and
On 2012/02/15 21:31:18, Aaron Boodman wrote:
> .replace("specified scheme", "specified inner scheme");

Done.

http://codereview.chromium.org/7811006/diff/99002/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.h:225: // A bitmask containing the inner
schemes which are considered valid for this
On 2012/02/15 21:31:18, Aaron Boodman wrote:
> .insertBefore("\n");

Done.

http://codereview.chromium.org/7811006/diff/105001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/autocomplete.cc (right):

http://codereview.chromium.org/7811006/diff/105001/chrome/browser/autocomplet...
chrome/browser/autocomplete/autocomplete.cc:167: ParseFileSystemURL(text.data(),
text.length(), parts);
On 2012/02/16 15:43:26, sky wrote:
> Is it really safe to reuse parts here?

It would be if I'd remembered to clear out inner_parsed in DoParseFileSystemURL;
I'll have to do that in another CL.

However, URLFixerUpper::SegmentURL is already calling into ParseFileSystemURL,
so we can skip this call entirely.

http://codereview.chromium.org/7811006/diff/105001/chrome/browser/browsing_da...
File chrome/browser/browsing_data_database_helper.cc (right):

http://codereview.chromium.org/7811006/diff/105001/chrome/browser/browsing_da...
chrome/browser/browsing_data_database_helper.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/02/16 10:02:57, markusheintz_ wrote:
> nit: s/2011/2012

Done.

http://codereview.chromium.org/7811006/diff/105001/chrome/browser/browsing_da...
File chrome/browser/browsing_data_database_helper.h (right):

http://codereview.chromium.org/7811006/diff/105001/chrome/browser/browsing_da...
chrome/browser/browsing_data_database_helper.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/02/16 10:02:57, markusheintz_ wrote:
> nit: s/2011/2012

Done.

http://codereview.chromium.org/7811006/diff/105001/chrome/browser/browsing_da...
File chrome/browser/browsing_data_indexed_db_helper.h (right):

http://codereview.chromium.org/7811006/diff/105001/chrome/browser/browsing_da...
chrome/browser/browsing_data_indexed_db_helper.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/02/16 10:02:57, markusheintz_ wrote:
> nit: s/2011/2012

Done.

http://codereview.chromium.org/7811006/diff/105001/chrome/browser/browsing_da...
File chrome/browser/browsing_data_local_storage_helper.h (right):

http://codereview.chromium.org/7811006/diff/105001/chrome/browser/browsing_da...
chrome/browser/browsing_data_local_storage_helper.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/02/16 10:02:57, markusheintz_ wrote:
> nit: s/2011/2012

Done.

http://codereview.chromium.org/7811006/diff/105001/chrome/browser/net/url_fix...
File chrome/browser/net/url_fixer_upper.cc (right):

http://codereview.chromium.org/7811006/diff/105001/chrome/browser/net/url_fix...
chrome/browser/net/url_fixer_upper.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2012/02/16 10:02:57, markusheintz_ wrote:
> nit: s/2011/2012

Done.

http://codereview.chromium.org/7811006/diff/105001/chrome/common/content_sett...
File chrome/common/content_settings_pattern_unittest.cc (right):

http://codereview.chromium.org/7811006/diff/105001/chrome/common/content_sett...
chrome/common/content_settings_pattern_unittest.cc:128:
EXPECT_TRUE(pattern.Matches(GURL("file:///temporary")));
On 2012/02/16 09:55:37, markusheintz_ wrote:
> That means a content settings pattern for
"filesystem:file:///temporary/foo.txt"
> would also match a file url for the file "/temporary" on the local file system
> ("file:///temporary"). Same for any other 
> 
> Just to refresh my memory:
> filesystem:file:///temporary/foo.txt are two separate files.
> file:///temporary/foo.txt are two different files

Yes, they're different files.  What do we want to happen here?  Generally if you
have access to anything in file://, you should have access to any
filesystem:file://.  Paths kind of mess that up, though.  Is there any point in
path-based restrictions on filesystem:file URLs?  Do we enforce specific
path-based restrictions on file: URLs, or are they really all the same [or all
unique]?

http://codereview.chromium.org/7811006/diff/105001/chrome/common/extensions/u...
File chrome/common/extensions/url_pattern.h (right):

http://codereview.chromium.org/7811006/diff/105001/chrome/common/extensions/u...
chrome/common/extensions/url_pattern.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2012/02/16 10:02:57, markusheintz_ wrote:
> nit: s/2011/2012

Done.

http://codereview.chromium.org/7811006/diff/105001/chrome/common/extensions/u...
File chrome/common/extensions/url_pattern_unittest.cc (right):

http://codereview.chromium.org/7811006/diff/105001/chrome/common/extensions/u...
chrome/common/extensions/url_pattern_unittest.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/02/16 09:55:37, markusheintz_ wrote:
> nit: Please change to 2012

Done.  Must have been a bad merge on my part.

http://codereview.chromium.org/7811006/diff/105001/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/filebrowser_component/main.js (right):

http://codereview.chromium.org/7811006/diff/105001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/filebrowser_component/main.js:1: //
Copyright (c) 2011 The Chromium Authors. All rights reserved.
On 2012/02/16 10:02:57, markusheintz_ wrote:
> nit: s/2011/2012

Done.

http://codereview.chromium.org/7811006/diff/105001/content/browser/browser_ur...
File content/browser/browser_url_handler.cc (right):

http://codereview.chromium.org/7811006/diff/105001/content/browser/browser_ur...
content/browser/browser_url_handler.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2012/02/16 10:02:57, markusheintz_ wrote:
> nit: s/2011/2012

Done.

http://codereview.chromium.org/7811006/diff/105001/content/public/common/url_...
File content/public/common/url_constants.cc (right):

http://codereview.chromium.org/7811006/diff/105001/content/public/common/url_...
content/public/common/url_constants.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2012/02/16 10:02:57, markusheintz_ wrote:
> nit: s/2011/2012

Done.

http://codereview.chromium.org/7811006/diff/105001/net/base/mime_sniffer.cc
File net/base/mime_sniffer.cc (right):

http://codereview.chromium.org/7811006/diff/105001/net/base/mime_sniffer.cc#n...
net/base/mime_sniffer.cc:1: // Copyright (c) 2011 The Chromium Authors. All
rights reserved.
On 2012/02/16 10:02:57, markusheintz_ wrote:
> nit: s/2011/2012

Done.

http://codereview.chromium.org/7811006/diff/105001/webkit/appcache/appcache_i...
File webkit/appcache/appcache_interfaces.cc (right):

http://codereview.chromium.org/7811006/diff/105001/webkit/appcache/appcache_i...
webkit/appcache/appcache_interfaces.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2012/02/16 10:02:57, markusheintz_ wrote:
> nit: s/2011/2012

Done.

http://codereview.chromium.org/7811006/diff/105001/webkit/fileapi/file_system...
File webkit/fileapi/file_system_util.cc (right):

http://codereview.chromium.org/7811006/diff/105001/webkit/fileapi/file_system...
webkit/fileapi/file_system_util.cc:40: if (inner_path.compare(0,
strlen(kPersistentDir), kPersistentDir) == 0) {
On 2012/02/17 23:47:37, kinuko wrote:
> nit: I think we can use arraysize here

Done.

[ericu, 2012-02-23 01:39:13]

http://codereview.chromium.org/7811006/diff/117005/chrome/common/extensions/u...
File chrome/common/extensions/url_pattern_unittest.cc (left):

http://codereview.chromium.org/7811006/diff/117005/chrome/common/extensions/u...
chrome/common/extensions/url_pattern_unittest.cc:625:
TEST(ExtensionURLPatternTest, CanReusePatternWithParse) {
This looks like a merge botch; I'll put it back.

[ericu, 2012-02-27 21:24:05]

On 2012/02/23 01:39:13, ericu wrote:
>
http://codereview.chromium.org/7811006/diff/117005/chrome/common/extensions/u...
> File chrome/common/extensions/url_pattern_unittest.cc (left):
> 
>
http://codereview.chromium.org/7811006/diff/117005/chrome/common/extensions/u...
> chrome/common/extensions/url_pattern_unittest.cc:625:
> TEST(ExtensionURLPatternTest, CanReusePatternWithParse) {
> This looks like a merge botch; I'll put it back.

Done.

Ping: Markus, Sky, Aaron-or-Matt--how does it look?

[sky, 2012-02-27 21:59:33]

LGTM

[markusheintz_, 2012-02-28 23:20:08]

LGTM

http://codereview.chromium.org/7811006/diff/105001/chrome/common/content_sett...
File chrome/common/content_settings_pattern_unittest.cc (right):

http://codereview.chromium.org/7811006/diff/105001/chrome/common/content_sett...
chrome/common/content_settings_pattern_unittest.cc:128:
EXPECT_TRUE(pattern.Matches(GURL("file:///temporary")));
Unfortunalty we still support path for file content settings pattern. But IMO it
does not make much sense to set content settings based on a file path. This is
just legacy stuff and  should/will go away.

On 2012/02/22 00:00:51, ericu wrote:
> On 2012/02/16 09:55:37, markusheintz_ wrote:
> > That means a content settings pattern for
> "filesystem:file:///temporary/foo.txt"
> > would also match a file url for the file "/temporary" on the local file
system
> > ("file:///temporary"). Same for any other 
> > 
> > Just to refresh my memory:
> > filesystem:file:///temporary/foo.txt are two separate files.
> > file:///temporary/foo.txt are two different files
> 
> Yes, they're different files.  What do we want to happen here?  Generally if
you
> have access to anything in file://, you should have access to any
> filesystem:file://.  Paths kind of mess that up, though.  Is there any point
in
> path-based restrictions on filesystem:file URLs?  Do we enforce specific
> path-based restrictions on file: URLs, or are they really all the same [or all
> unique]?

http://codereview.chromium.org/7811006/diff/123001/chrome/common/content_sett...
File chrome/common/content_settings_pattern.cc (right):

http://codereview.chromium.org/7811006/diff/123001/chrome/common/content_sett...
chrome/common/content_settings_pattern.cc:443: //
filesystem:file:///temporary/... are equivalent.
Please add a TODO for me here:
TODO(markusheintz): Content settings should be defined for all files on a
machine. Unless there is a good use case for supporting paths for file patterns,
stop supporting path for file patterns.

[ericu, 2012-02-28 23:27:15]

http://codereview.chromium.org/7811006/diff/123001/chrome/common/content_sett...
File chrome/common/content_settings_pattern.cc (right):

http://codereview.chromium.org/7811006/diff/123001/chrome/common/content_sett...
chrome/common/content_settings_pattern.cc:443: //
filesystem:file:///temporary/... are equivalent.
On 2012/02/28 23:20:09, markusheintz_ wrote:
> Please add a TODO for me here:
> TODO(markusheintz): Content settings should be defined for all files on a
> machine. Unless there is a good use case for supporting paths for file
patterns,
> stop supporting path for file patterns.

Done, thanks.

[ericu, 2012-03-07 22:56:57]

aa, mpcomplete: ping?

	Eric

[Aaron Boodman, 2012-03-12 23:17:13]

Here are some comments, but I still feel uncomfortable with this change.

Is there any way we can do something smaller for the extension parts? It seems
like we only need to change URLPattern at all because of the file system filters
feature. And that doesn't use the majority of the power of URLPattern. Could we
change that feature so that it doesn't use URLPattern?

I realize this is more of a question for zel. I will ping him.

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/e...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:838: if (filter.substr(0, 10) !=
chrome::kFileSystemScheme ||
Can you use StartsWith()?

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
File chrome/common/extensions/url_pattern.cc (right):

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
chrome/common/extensions/url_pattern.cc:163: if (scheme_ ==
chrome::kFileSystemScheme) {
Yeah, would prefer this was something like if (SupportsInnerSchemes(scheme_)) {
...

here.

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
chrome/common/extensions/url_pattern.cc:173: return PARSE_ERROR_INVALID_SCHEME;
PARSE_ERROR_INVALID_INNER_SCHEME seems better here.

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
chrome/common/extensions/url_pattern.cc:177: if (standard_scheme !=
has_standard_scheme_separator)
I still think this method has gotten too long and warrants splitting up. It
doesn't seem that hard to do while you are in here.

I am imagining methods like:

ParseResult ParseScheme(const std::string& input, int* end_pos);
ParseResult ParseInnerScheme(const std::string& input, int* end_pos);

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
chrome/common/extensions/url_pattern.cc:302: valid_inner_schemes_ = SCHEME_HTTP
| SCHEME_HTTPS | SCHEME_FTP |
I am not sure why SetScheme modifies the valid schemes. That does not make much
sense to me. Seems like it should be controlled separately. Same comment here.

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
chrome/common/extensions/url_pattern.cc:302: valid_inner_schemes_ = SCHEME_HTTP
| SCHEME_HTTPS | SCHEME_FTP |
Why this particular interpretation of "*"? Note that it is different than the
one in SetScheme().

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
chrome/common/extensions/url_pattern.cc:369:
!MatchesInnerScheme(test.inner_url()->scheme())))
Style guide requires curly braces here.

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
chrome/common/extensions/url_pattern.cc:469: bool standard_scheme =
IsStandardScheme(scheme_);
Can you rename this to 'is_standard_scheme' while you're in here?

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
File chrome/common/extensions/url_pattern.h (right):

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
chrome/common/extensions/url_pattern.h:107: void SetValidSchemes(int
valid_schemes);
It's surprising that this would also set the inner schemes. Why is that?

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
chrome/common/extensions/url_pattern.h:137: // Sets the inner scheme for pattern
matches; this can only be used for
Add blank line before this comment block.

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
chrome/common/extensions/url_pattern.h:138: // patterns whose scheme has already
been set to "filesystem". This can be a
"with a scheme that supports inner schemes."

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
chrome/common/extensions/url_pattern.h:248: // The inner scheme for the pattern,
used only when scheme_ is "filesystem".
It would be better to create an array of schemes that support inner schemes, and
just refer to those. Even if it only has one item currently.

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
File chrome/common/extensions/url_pattern_unittest.cc (right):

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
chrome/common/extensions/url_pattern_unittest.cc:442: 
What about patterns like file-system:http://*.google.com/*?

Are they allowed?

[Aaron Boodman, 2012-03-12 23:21:45]

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/e...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:844: filter.replace(11, 0,
"chrome-extension://*/");
Why?

[ericu, 2012-03-12 23:57:20]

On Mon, Mar 12, 2012 at 4:17 PM,  <aa@chromium.org> wrote:
> Here are some comments, but I still feel uncomfortable with this change.
>
> Is there any way we can do something smaller for the extension parts? It
> seems
> like we only need to change URLPattern at all because of the file system
> filters
> feature. And that doesn't use the majority of the power of URLPattern. Could
> we
> change that feature so that it doesn't use URLPattern?

While the fileBrowserHandler could get away with simpler code, I'd
really like to minimize the number of URL and URL pattern parsers we
have in Chrome.  I can't believe how many places I had to change to
add this new URL type.  If we can avoid adding another parser, I'm for
it.

Also, even if this extension is the only user of URLPattern currently
trying to support filesystem URLs, the rest of its current and future
users should be able to handle them without adding custom code.  No
code that parses URLs should be ignorant of a standard URL type
without a very good reason.

>
> I realize this is more of a question for zel. I will ping him.
>
>
>
http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/e...
> File chrome/common/extensions/extension.cc (right):
>
>
http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/e...
> chrome/common/extensions/extension.cc:838: if (filter.substr(0, 10) !=
> chrome::kFileSystemScheme ||
> Can you use StartsWith()?
>
>
http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
> File chrome/common/extensions/url_pattern.cc (right):
>
>
http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
> chrome/common/extensions/url_pattern.cc:163: if (scheme_ ==
> chrome::kFileSystemScheme) {
> Yeah, would prefer this was something like if
> (SupportsInnerSchemes(scheme_)) { ...
>
> here.
>
>
http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
> chrome/common/extensions/url_pattern.cc:173: return
> PARSE_ERROR_INVALID_SCHEME;
> PARSE_ERROR_INVALID_INNER_SCHEME seems better here.
>
>
http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
> chrome/common/extensions/url_pattern.cc:177: if (standard_scheme !=
> has_standard_scheme_separator)
> I still think this method has gotten too long and warrants splitting up.
> It doesn't seem that hard to do while you are in here.
>
> I am imagining methods like:
>
> ParseResult ParseScheme(const std::string& input, int* end_pos);
> ParseResult ParseInnerScheme(const std::string& input, int* end_pos);
>
>
http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
> chrome/common/extensions/url_pattern.cc:302: valid_inner_schemes_ =
> SCHEME_HTTP | SCHEME_HTTPS | SCHEME_FTP |
> I am not sure why SetScheme modifies the valid schemes. That does not
> make much sense to me. Seems like it should be controlled separately.
> Same comment here.
>
>
http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
> chrome/common/extensions/url_pattern.cc:302: valid_inner_schemes_ =
> SCHEME_HTTP | SCHEME_HTTPS | SCHEME_FTP |
> Why this particular interpretation of "*"? Note that it is different
> than the one in SetScheme().
>
>
http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
> chrome/common/extensions/url_pattern.cc:369:
> !MatchesInnerScheme(test.inner_url()->scheme())))
> Style guide requires curly braces here.
>
>
http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
> chrome/common/extensions/url_pattern.cc:469: bool standard_scheme =
> IsStandardScheme(scheme_);
> Can you rename this to 'is_standard_scheme' while you're in here?
>
>
http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
> File chrome/common/extensions/url_pattern.h (right):
>
>
http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
> chrome/common/extensions/url_pattern.h:107: void SetValidSchemes(int
> valid_schemes);
> It's surprising that this would also set the inner schemes. Why is that?
>
>
http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
> chrome/common/extensions/url_pattern.h:137: // Sets the inner scheme for
> pattern matches; this can only be used for
> Add blank line before this comment block.
>
>
http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
> chrome/common/extensions/url_pattern.h:138: // patterns whose scheme has
> already been set to "filesystem". This can be a
> "with a scheme that supports inner schemes."
>
>
http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
> chrome/common/extensions/url_pattern.h:248: // The inner scheme for the
> pattern, used only when scheme_ is "filesystem".
> It would be better to create an array of schemes that support inner
> schemes, and just refer to those. Even if it only has one item
> currently.
>
>
http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
> File chrome/common/extensions/url_pattern_unittest.cc (right):
>
>
http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
> chrome/common/extensions/url_pattern_unittest.cc:442:
> What about patterns like file-system:http://*.google.com/*?
>
> Are they allowed?
>
> http://codereview.chromium.org/7811006/

[ericu, 2012-03-13 21:58:46]

I had a little trouble with one merge, so I expect this to break a CrOS test;
I'll fix it next upload.

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/e...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:838: if (filter.substr(0, 10) !=
chrome::kFileSystemScheme ||
On 2012/03/12 23:17:13, Aaron Boodman wrote:
> Can you use StartsWith()?

Done.

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:844: filter.replace(11, 0,
"chrome-extension://*/");
On 2012/03/12 23:21:45, Aaron Boodman wrote:
> Why?

Added a comment.

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
File chrome/common/extensions/url_pattern.cc (right):

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
chrome/common/extensions/url_pattern.cc:163: if (scheme_ ==
chrome::kFileSystemScheme) {
On 2012/03/12 23:17:13, Aaron Boodman wrote:
> Yeah, would prefer this was something like if (SupportsInnerSchemes(scheme_))
{
> ...
> 
> here.

Done.

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
chrome/common/extensions/url_pattern.cc:173: return PARSE_ERROR_INVALID_SCHEME;
On 2012/03/12 23:17:13, Aaron Boodman wrote:
> PARSE_ERROR_INVALID_INNER_SCHEME seems better here. 

Done.

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
chrome/common/extensions/url_pattern.cc:177: if (standard_scheme !=
has_standard_scheme_separator)
On 2012/03/12 23:17:13, Aaron Boodman wrote:
> I still think this method has gotten too long and warrants splitting up. It
> doesn't seem that hard to do while you are in here.
> 
> I am imagining methods like:
> 
> ParseResult ParseScheme(const std::string& input, int* end_pos);
> ParseResult ParseInnerScheme(const std::string& input, int* end_pos);

Done.

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
chrome/common/extensions/url_pattern.cc:302: valid_inner_schemes_ = SCHEME_HTTP
| SCHEME_HTTPS | SCHEME_FTP |
On 2012/03/12 23:17:13, Aaron Boodman wrote:
> Why this particular interpretation of "*"? Note that it is different than the
> one in SetScheme().

I implemented it based on the comment in url_pattern.h, which contradicts the
code in SetScheme; I could match SetScheme instead and fix the comment if you
think that's better.  Or I could make SetScheme match the comment.

What do you think?

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
chrome/common/extensions/url_pattern.cc:302: valid_inner_schemes_ = SCHEME_HTTP
| SCHEME_HTTPS | SCHEME_FTP |
On 2012/03/12 23:17:13, Aaron Boodman wrote:
> I am not sure why SetScheme modifies the valid schemes. That does not make
much
> sense to me. Seems like it should be controlled separately. Same comment here.

I'm OK with changing the behavior--there aren't all that many callers--but
perhaps it belongs in a different CL?  That's really nothing to do with
supporting filesystem URLs, and this is already not a small change.

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
chrome/common/extensions/url_pattern.cc:369:
!MatchesInnerScheme(test.inner_url()->scheme())))
On 2012/03/12 23:17:13, Aaron Boodman wrote:
> Style guide requires curly braces here.

Done.

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
chrome/common/extensions/url_pattern.cc:469: bool standard_scheme =
IsStandardScheme(scheme_);
On 2012/03/12 23:17:13, Aaron Boodman wrote:
> Can you rename this to 'is_standard_scheme' while you're in here?

Done.

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
File chrome/common/extensions/url_pattern.h (right):

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
chrome/common/extensions/url_pattern.h:107: void SetValidSchemes(int
valid_schemes);
On 2012/03/12 23:17:13, Aaron Boodman wrote:
> It's surprising that this would also set the inner schemes. Why is that?

Generally if you want to match http://google.com, you also want to match
filesystem:http://google.com.

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
chrome/common/extensions/url_pattern.h:137: // Sets the inner scheme for pattern
matches; this can only be used for
On 2012/03/12 23:17:13, Aaron Boodman wrote:
> Add blank line before this comment block.

Done.

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
chrome/common/extensions/url_pattern.h:138: // patterns whose scheme has already
been set to "filesystem". This can be a
On 2012/03/12 23:17:13, Aaron Boodman wrote:
> "with a scheme that supports inner schemes."

Done.

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
chrome/common/extensions/url_pattern.h:248: // The inner scheme for the pattern,
used only when scheme_ is "filesystem".
On 2012/03/12 23:17:13, Aaron Boodman wrote:
> It would be better to create an array of schemes that support inner schemes,
and
> just refer to those. Even if it only has one item currently.

I made a trivial inline function instead of an array for now, just to keep it
simple.

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
File chrome/common/extensions/url_pattern_unittest.cc (right):

http://codereview.chromium.org/7811006/diff/128001/chrome/common/extensions/u...
chrome/common/extensions/url_pattern_unittest.cc:442: 
On 2012/03/12 23:17:13, Aaron Boodman wrote:
> What about patterns like file-system:http://*.google.com/*?
> 
> Are they allowed?

Yup; added tests.

[ericu, 2012-03-15 02:38:27]

I've uploaded a new version, rolling in Aaron's feedback [including some
offline].  This reverts most of the URLPattern changes, and instead replaces
them with code that says that:

* Patterns for a given scheme/host/port also match that trio as an inner_url of
a filesystem URL.
* If you want to match a path other than "/*" to a filesystem URL, you have to
set a flag to tell the pattern that.  If you set it, then it will no longer
patch non-filesystem URLs.

[ericu, 2012-03-20 21:35:00]

On 2012/03/15 02:38:27, ericu wrote:
> I've uploaded a new version, rolling in Aaron's feedback [including some
> offline].  This reverts most of the URLPattern changes, and instead replaces
> them with code that says that:
> 
> * Patterns for a given scheme/host/port also match that trio as an inner_url
of
> a filesystem URL.
> * If you want to match a path other than "/*" to a filesystem URL, you have to
> set a flag to tell the pattern that.  If you set it, then it will no longer
> patch non-filesystem URLs.

Aaron:  The ChromeOS trybot failures appear to be bad/flaky tests; the
consistent failure fails for other folks too.  Windows and Mac pass with clobber
builds.  Please consider this CL ready for review.

[Aaron Boodman, 2012-03-20 21:51:16]

On Tue, Mar 20, 2012 at 2:35 PM,  <ericu@chromium.org> wrote:
> On 2012/03/15 02:38:27, ericu wrote:
>>
>> I've uploaded a new version, rolling in Aaron's feedback [including some
>> offline].  This reverts most of the URLPattern changes, and instead
>> replaces
>> them with code that says that:
>
>
>> * Patterns for a given scheme/host/port also match that trio as an
>> inner_url
>
> of
>>
>> a filesystem URL.
>> * If you want to match a path other than "/*" to a filesystem URL, you
>> have to
>> set a flag to tell the pattern that.  If you set it, then it will no
>> longer
>> patch non-filesystem URLs.
>
>
> Aaron:  The ChromeOS trybot failures appear to be bad/flaky tests; the
> consistent failure fails for other folks too.  Windows and Mac pass with
> clobber
> builds.  Please consider this CL ready for review.

Yeah, the CL is queued. I will try to get to it today, but definitely tomorrow.

- a

[Aaron Boodman, 2012-03-21 23:51:57]

https://chromiumcodereview.appspot.com/7811006/diff/178034/chrome/common/exte...
File chrome/common/extensions/url_pattern.cc (right):

https://chromiumcodereview.appspot.com/7811006/diff/178034/chrome/common/exte...
chrome/common/extensions/url_pattern.cc:70: bool is_standard_scheme(const
std::string& scheme) {
Why rename this? I think the current capitalization is correct given that this
is a nontrivial amount of work.

https://chromiumcodereview.appspot.com/7811006/diff/178034/chrome/common/exte...
chrome/common/extensions/url_pattern.cc:255: void
URLPattern::SetMatchNestedURLPath(bool val) {
This one on the other hand should be set_match_nested_url_path() :-).

https://chromiumcodereview.appspot.com/7811006/diff/178034/chrome/common/exte...
File chrome/common/extensions/url_pattern.h (right):

https://chromiumcodereview.appspot.com/7811006/diff/178034/chrome/common/exte...
chrome/common/extensions/url_pattern.h:124: // If true, a path match other than
'*' will always fail on a non-nested URL.
Can we do something even more specific, just for the one place we need
filesystem support?

How about something like:

set_enable_partial_filesystem_scheme_support()

This would allow <pattern> to match filesystem:<url-that-would-match-<pattern>>,
but only when <pattern> has * for the path component.

This way when we fix the one place that uses this feature, we can easily remove
the support from URLPattern.

[Aaron Boodman, 2012-03-21 23:53:03]

On 2012/03/21 23:51:57, Aaron Boodman wrote:
>
https://chromiumcodereview.appspot.com/7811006/diff/178034/chrome/common/exte...
> File chrome/common/extensions/url_pattern.cc (right):
> 
>
https://chromiumcodereview.appspot.com/7811006/diff/178034/chrome/common/exte...
> chrome/common/extensions/url_pattern.cc:70: bool is_standard_scheme(const
> std::string& scheme) {
> Why rename this? I think the current capitalization is correct given that this
> is a nontrivial amount of work.
> 
>
https://chromiumcodereview.appspot.com/7811006/diff/178034/chrome/common/exte...
> chrome/common/extensions/url_pattern.cc:255: void
> URLPattern::SetMatchNestedURLPath(bool val) {
> This one on the other hand should be set_match_nested_url_path() :-).
> 
>
https://chromiumcodereview.appspot.com/7811006/diff/178034/chrome/common/exte...
> File chrome/common/extensions/url_pattern.h (right):
> 
>
https://chromiumcodereview.appspot.com/7811006/diff/178034/chrome/common/exte...
> chrome/common/extensions/url_pattern.h:124: // If true, a path match other
than
> '*' will always fail on a non-nested URL.
> Can we do something even more specific, just for the one place we need
> filesystem support?
> 
> How about something like:
> 
> set_enable_partial_filesystem_scheme_support()
> 
> This would allow <pattern> to match
filesystem:<url-that-would-match-<pattern>>,
> but only when <pattern> has * for the path component.

(otherwise, the behavior would be unchanged from today)

[ericu, 2012-03-22 00:22:13]

http://codereview.chromium.org/7811006/diff/178034/chrome/common/extensions/u...
File chrome/common/extensions/url_pattern.cc (right):

http://codereview.chromium.org/7811006/diff/178034/chrome/common/extensions/u...
chrome/common/extensions/url_pattern.cc:70: bool is_standard_scheme(const
std::string& scheme) {
On 2012/03/21 23:51:57, Aaron Boodman wrote:
> Why rename this? I think the current capitalization is correct given that this
> is a nontrivial amount of work.

You requested this change in your comments on patch set 40 ;'>.

Reverted.

http://codereview.chromium.org/7811006/diff/178034/chrome/common/extensions/u...
chrome/common/extensions/url_pattern.cc:255: void
URLPattern::SetMatchNestedURLPath(bool val) {
On 2012/03/21 23:51:57, Aaron Boodman wrote:
> This one on the other hand should be set_match_nested_url_path() :-).

Done.

http://codereview.chromium.org/7811006/diff/178034/chrome/common/extensions/u...
File chrome/common/extensions/url_pattern.h (right):

http://codereview.chromium.org/7811006/diff/178034/chrome/common/extensions/u...
chrome/common/extensions/url_pattern.h:124: // If true, a path match other than
'*' will always fail on a non-nested URL.
On 2012/03/21 23:51:57, Aaron Boodman wrote:
> Can we do something even more specific, just for the one place we need
> filesystem support?
> 
> How about something like:
> 
> set_enable_partial_filesystem_scheme_support()
> 
> This would allow <pattern> to match
filesystem:<url-that-would-match-<pattern>>,
> but only when <pattern> has * for the path component.
> 
> This way when we fix the one place that uses this feature, we can easily
remove
> the support from URLPattern.

My goal was actually to support filesystem URLs transparently where it would
make sense.  So if e.g. a userscript should have access to all pages at
www.google.com, the pattern would also just work for pages that that site
creates in the filesystem.  It's only when the pattern is supposed to match
specific files that I want you to have to set a flag, since a specific path on
the server is clearly different than the same path in the filesystem.  The flag
just forces you to tell me whether it's a local path or a server path.

[ericu, 2012-03-22 01:59:23]

As discussed with Aaron offline, I've removed general filesystem URL support
from URLPattern, and replaced it with a temporary hack for ChromeOS.  Once this
CL goes in, we'll talk about what we want full support to look like for
extensions.

[ericu, 2012-03-23 21:11:36]

On 2012/03/22 01:59:23, ericu wrote:
> As discussed with Aaron offline, I've removed general filesystem URL support
> from URLPattern, and replaced it with a temporary hack for ChromeOS.  Once
this
> CL goes in, we'll talk about what we want full support to look like for
> extensions.

The win_rel trybot failures look unrelated, BTW.

[Aaron Boodman, 2012-03-23 21:48:32]

http://codereview.chromium.org/7811006/diff/177038/chrome/common/extensions/u...
File chrome/common/extensions/url_pattern.cc (right):

http://codereview.chromium.org/7811006/diff/177038/chrome/common/extensions/u...
chrome/common/extensions/url_pattern.cc:255: void
URLPattern::set_partial_filesystem_support_hack(bool val) {
Pure setters like this are typically in the .h file in Chrome.

http://codereview.chromium.org/7811006/diff/177038/chrome/common/extensions/u...
chrome/common/extensions/url_pattern.cc:384: bool URLPattern::MatchesPath(const
std::string& test, bool nested_url)
I don't understand why the nested_url param is necessary. Callers will
presumably ensure that they have the entire path before calling this, even in
the case of nested URLs.

[ericu, 2012-03-23 22:47:15]

http://codereview.chromium.org/7811006/diff/177038/chrome/common/extensions/u...
File chrome/common/extensions/url_pattern.cc (right):

http://codereview.chromium.org/7811006/diff/177038/chrome/common/extensions/u...
chrome/common/extensions/url_pattern.cc:255: void
URLPattern::set_partial_filesystem_support_hack(bool val) {
On 2012/03/23 21:48:32, Aaron Boodman wrote:
> Pure setters like this are typically in the .h file in Chrome.

Done.

http://codereview.chromium.org/7811006/diff/177038/chrome/common/extensions/u...
chrome/common/extensions/url_pattern.cc:384: bool URLPattern::MatchesPath(const
std::string& test, bool nested_url)
On 2012/03/23 21:48:32, Aaron Boodman wrote:
> I don't understand why the nested_url param is necessary. Callers will
> presumably ensure that they have the entire path before calling this, even in
> the case of nested URLs.

The caller is telling us where he got the path.  If that wasn't from a
filesystem URL and !partial_filesystem_support_hack_, we need to reject it.  I'm
assuming the calling code wouldn't necessarily be the code that created the
pattern, and wouldn't want to deal with these details itself.  Of course, as it
happens, nobody calls this function outside this file at the moment, so we're
free to decide what we please, but this is the best I could come up with short
of making the function private.

[ericu, 2012-03-23 23:48:46]

http://codereview.chromium.org/7811006/diff/177038/chrome/common/extensions/u...
File chrome/common/extensions/url_pattern.cc (right):

http://codereview.chromium.org/7811006/diff/177038/chrome/common/extensions/u...
chrome/common/extensions/url_pattern.cc:384: bool URLPattern::MatchesPath(const
std::string& test, bool nested_url)
On 2012/03/23 22:47:15, ericu wrote:
> On 2012/03/23 21:48:32, Aaron Boodman wrote:
> > I don't understand why the nested_url param is necessary. Callers will
> > presumably ensure that they have the entire path before calling this, even
in
> > the case of nested URLs.
> 
> The caller is telling us where he got the path.  If that wasn't from a
> filesystem URL and !partial_filesystem_support_hack_, we need to reject it. 
I'm
> assuming the calling code wouldn't necessarily be the code that created the
> pattern, and wouldn't want to deal with these details itself.  Of course, as
it
> happens, nobody calls this function outside this file at the moment, so we're
> free to decide what we please, but this is the best I could come up with short
> of making the function private.

Param removed.

[Aaron Boodman, 2012-03-23 23:57:58]

LGTM

Happy Friday!

[ericu, 2012-03-24 17:58:33]

FYI this was reverted; it reveals a memory leak I put in GURL.  I'm fixing that
and will roll deps before resubmitting this.

[Sergey Ulanov, 2012-04-03 02:22:42]

Reverted because browser_tests were failing on XP:
http://build.chromium.org/p/chromium/buildstatus?builder=Win7%20Tests%20%28db...

And googleurl_unittests were failing on Vista:
http://build.chromium.org/p/chromium/buildstatus?builder=Vista%20Tests%20%281...