Make net::DataURL's MIME string check stricter

Make net::DataURL's MIME string check stricter

Existing IsMimeType is replaced with two methods:
- ParseMimeTypeWithoutParameter: Checks grammar and parses given MIME
  type string
- IsValidTopLevelMimeType: Checks if the given top level MIME type is
  valid one or not

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

[sof, 2013-10-31 08:19:31]

https://codereview.chromium.org/54233002/diff/1/webkit/child/weburlloader_imp...
File webkit/child/weburlloader_impl.cc (right):

https://codereview.chromium.org/54233002/diff/1/webkit/child/weburlloader_imp...
webkit/child/weburlloader_impl.cc:136: std::string raw_header = "HTTP/1.1 200
OK";
Possible to bring in the use of net::HttpUtil::AssembleRawHeaders() here?

[tyoshino (SeeGerritForStatus), 2013-10-31 08:53:20]

https://codereview.chromium.org/54233002/diff/1/webkit/child/weburlloader_imp...
File webkit/child/weburlloader_impl.cc (right):

https://codereview.chromium.org/54233002/diff/1/webkit/child/weburlloader_imp...
webkit/child/weburlloader_impl.cc:136: std::string raw_header = "HTTP/1.1 200
OK";
On 2013/10/31 08:19:32, sof wrote:
> Possible to bring in the use of net::HttpUtil::AssembleRawHeaders() here?

Thanks for suggestion. But it'll be:

    std::string raw_header = "HTTP/1.1 200 OK\r\nContent-Type: " + mime_type;
    if (!charset.empty()) {
      raw_header += ";charset=";
      raw_header += charset;
    }
    raw_header += "\r\n\r\n";

    info->headers = new net::HttpResponseHeaders(
        net::HttpUtil::AssembleRawHeaders(
            raw_header.data(), raw_header.size()));

Not so simpler than current. WDYT?

[sof, 2013-10-31 15:26:59]

https://codereview.chromium.org/54233002/diff/1/webkit/child/weburlloader_imp...
File webkit/child/weburlloader_impl.cc (right):

https://codereview.chromium.org/54233002/diff/1/webkit/child/weburlloader_imp...
webkit/child/weburlloader_impl.cc:136: std::string raw_header = "HTTP/1.1 200
OK";
On 2013/10/31 08:53:20, tyoshino wrote:
> On 2013/10/31 08:19:32, sof wrote:
> > Possible to bring in the use of net::HttpUtil::AssembleRawHeaders() here?
> 
> Thanks for suggestion. But it'll be:
> 
>     std::string raw_header = "HTTP/1.1 200 OK\r\nContent-Type: " + mime_type;
>     if (!charset.empty()) {
>       raw_header += ";charset=";
>       raw_header += charset;
>     }
>     raw_header += "\r\n\r\n";
> 
>     info->headers = new net::HttpResponseHeaders(
>         net::HttpUtil::AssembleRawHeaders(
>             raw_header.data(), raw_header.size()));
> 
> Not so simpler than current. WDYT?

Yes, that wouldn't avoid the explicit formatting of
the response after all. An alternative is to create an empty
response and use AddHeader().

[abarth-chromium, 2013-11-01 16:48:55]

https://codereview.chromium.org/54233002/diff/70001/webkit/child/weburlloader...
File webkit/child/weburlloader_impl.cc (right):

https://codereview.chromium.org/54233002/diff/70001/webkit/child/weburlloader...
webkit/child/weburlloader_impl.cc:147: info->headers = new
net::HttpResponseHeaders(raw_header);
It seems like there should be a higher-level API for this sort of thing.  I
think there's also an implementation in the browser process to update.

[tyoshino (SeeGerritForStatus), 2013-11-01 17:02:28]

Changed to use AddHeader() and ReplaceStatusLine().

> I think there's also an implementation in the browser process to update.

Will check

[sof, 2013-11-08 13:34:33]

The change looks fine to me; just a minor code comment.

https://codereview.chromium.org/54233002/diff/110001/webkit/child/weburlloade...
File webkit/child/weburlloader_impl.cc (right):

https://codereview.chromium.org/54233002/diff/110001/webkit/child/weburlloade...
webkit/child/weburlloader_impl.cc:139: std::string content_type_header =
"Content-Type: " + mime_type;
Slight inconsistency in assumptions here; you don't assume that mime_type can be
empty, but do for charset. The MIME type parser will default both, so I believe
you can simplify this ever so slightly.

[tyoshino (SeeGerritForStatus), 2013-11-12 05:47:26]

https://codereview.chromium.org/54233002/diff/110001/webkit/child/weburlloade...
File webkit/child/weburlloader_impl.cc (right):

https://codereview.chromium.org/54233002/diff/110001/webkit/child/weburlloade...
webkit/child/weburlloader_impl.cc:139: std::string content_type_header =
"Content-Type: " + mime_type;
On 2013/11/08 13:34:34, sof wrote:
> Slight inconsistency in assumptions here; you don't assume that mime_type can
be
> empty, but do for charset. The MIME type parser will default both, so I
believe
> you can simplify this ever so slightly.

Thanks. I see. It's well documented in the comment of net::DataURL::Parse().
Replaced L140 and added DCHECKs instead

[sof, 2013-11-12 07:05:23]

On 2013/11/12 05:47:26, tyoshino wrote:
..
> 
> Thanks. I see. It's well documented in the comment of net::DataURL::Parse().
> Replaced L140 and added DCHECKs instead

looks good

[sof, 2013-11-22 21:42:35]

https://codereview.chromium.org/54233002/diff/180001/webkit/child/weburlloade...
File webkit/child/weburlloader_impl.cc (right):

https://codereview.chromium.org/54233002/diff/180001/webkit/child/weburlloade...
webkit/child/weburlloader_impl.cc:144:
headers->AddHeader("Access-Control-Allow-Origin: *");
Thinking about this a bit more, this will work as a device for getting past the
resource sharing check for anonymous CORS requests. It won't work for
credentialled requests.

[tyoshino (SeeGerritForStatus), 2014-03-11 06:40:37]

https://codereview.chromium.org/54233002/diff/180001/webkit/child/weburlloade...
File webkit/child/weburlloader_impl.cc (right):

https://codereview.chromium.org/54233002/diff/180001/webkit/child/weburlloade...
webkit/child/weburlloader_impl.cc:144:
headers->AddHeader("Access-Control-Allow-Origin: *");
On 2013/11/22 21:42:35, sof wrote:
> Thinking about this a bit more, this will work as a device for getting past
the
> resource sharing check for anonymous CORS requests. It won't work for
> credentialled requests.

Right. When withCredentials is set, the response must be with the
Access-Control-Allow-Origin header with a value of the origin of the request.

I don't come up with any use case (or some case where it's convenient) of XHR
with data URL and with withCredentials set. So, I'd like to just adopt this
method. WDYT?

[sof, 2014-03-11 06:44:50]

On 2014/03/11 06:40:37, tyoshino wrote:
>
https://codereview.chromium.org/54233002/diff/180001/webkit/child/weburlloade...
> File webkit/child/weburlloader_impl.cc (right):
> 
>
https://codereview.chromium.org/54233002/diff/180001/webkit/child/weburlloade...
> webkit/child/weburlloader_impl.cc:144:
> headers->AddHeader("Access-Control-Allow-Origin: *");
> On 2013/11/22 21:42:35, sof wrote:
> > Thinking about this a bit more, this will work as a device for getting past
> the
> > resource sharing check for anonymous CORS requests. It won't work for
> > credentialled requests.
> 
> Right. When withCredentials is set, the response must be with the
> Access-Control-Allow-Origin header with a value of the origin of the request.
> 
> I don't come up with any use case (or some case where it's convenient) of XHR
> with data URL and with withCredentials set. So, I'd like to just adopt this
> method. WDYT?

Wouldn't that break a generic XHR loading script method that unconditionally
sets withCredentials=true given a URL?

[tyoshino (SeeGerritForStatus), 2014-03-11 06:47:22]

On 2013/11/01 17:02:28, tyoshino wrote:
> > I think there's also an implementation in the browser process to update.
> 
> Will check

Adam, I looked for code where DataURL::Parse is used. There're a lot of such
places but seem to be just using it as a decoder. I.e. not passing it to any web
platform API. Could you please give me a hint which you have in your mind might
need to be changed as well as WebUrlLoader?

[tyoshino (SeeGerritForStatus), 2014-03-11 06:56:28]

On 2014/03/11 06:44:50, sof wrote:
> On 2014/03/11 06:40:37, tyoshino wrote:
> >
>
https://codereview.chromium.org/54233002/diff/180001/webkit/child/weburlloade...
> > File webkit/child/weburlloader_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/54233002/diff/180001/webkit/child/weburlloade...
> > webkit/child/weburlloader_impl.cc:144:
> > headers->AddHeader("Access-Control-Allow-Origin: *");
> > On 2013/11/22 21:42:35, sof wrote:
> > > Thinking about this a bit more, this will work as a device for getting
past
> > the
> > > resource sharing check for anonymous CORS requests. It won't work for
> > > credentialled requests.
> > 
> > Right. When withCredentials is set, the response must be with the
> > Access-Control-Allow-Origin header with a value of the origin of the
request.
> > 
> > I don't come up with any use case (or some case where it's convenient) of
XHR
> > with data URL and with withCredentials set. So, I'd like to just adopt this
> > method. WDYT?
> 
> Wouldn't that break a generic XHR loading script method that unconditionally
> sets withCredentials=true given a URL?

OK, but As such a loader have been not working for data URLs, I think it's ok to
land this first.

Hmm, maybe to allow that, we need to check bypass CORS check for data URLs while
keeping it handled as cross origin request.

[sof, 2014-03-11 07:09:19]

On 2014/03/11 06:56:28, tyoshino wrote:
> On 2014/03/11 06:44:50, sof wrote:
> > On 2014/03/11 06:40:37, tyoshino wrote:
> > >
> >
>
https://codereview.chromium.org/54233002/diff/180001/webkit/child/weburlloade...
> > > File webkit/child/weburlloader_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/54233002/diff/180001/webkit/child/weburlloade...
> > > webkit/child/weburlloader_impl.cc:144:
> > > headers->AddHeader("Access-Control-Allow-Origin: *");
> > > On 2013/11/22 21:42:35, sof wrote:
> > > > Thinking about this a bit more, this will work as a device for getting
> past
> > > the
> > > > resource sharing check for anonymous CORS requests. It won't work for
> > > > credentialled requests.
> > > 
> > > Right. When withCredentials is set, the response must be with the
> > > Access-Control-Allow-Origin header with a value of the origin of the
> request.
> > > 
> > > I don't come up with any use case (or some case where it's convenient) of
> XHR
> > > with data URL and with withCredentials set. So, I'd like to just adopt
this
> > > method. WDYT?
> > 
> > Wouldn't that break a generic XHR loading script method that unconditionally
> > sets withCredentials=true given a URL?
> 
> OK, but As such a loader have been not working for data URLs, I think it's ok
to
> land this first.
> 
> Hmm, maybe to allow that, we need to check bypass CORS check for data URLs
while
> keeping it handled as cross origin request.

Yes, I don't think there is a way around having the fetch CORS-enabled access
check know about data: -- just like the spec considers them a separate case.
If so, why do this annotation of data: URLs?

[tyoshino (SeeGerritForStatus), 2014-03-11 07:27:01]

I rebased this and did manual test again. It turned out that this CL requires a
Blink-side change that is adding "data" to
WebCore::SchemeRegistry::shouldTreatURLSchemeAsCORSEnabled.

In Anne's fetch spec, data is specified to use basic fetch. We're going to skip
it and
- add "data" to the step "url's scheme is not one of "http" and "https""
- proceed to "CORS fetch with preflight" or "basic fetch with CORS flag set".

[abarth-chromium, 2014-03-11 07:34:34]

Test?

I'm not sure whether you've addressed sof's questions...

[sof, 2014-03-11 07:51:12]

On 2014/03/11 07:34:34, abarth wrote:
> Test?
> 
> I'm not sure whether you've addressed sof's questions...

If annotating the data: URL helps reduce overall plumbing needed to provide the
required access control for data: resources, then please do it here. But it
wouldn't be complete without considering data: in the CORS resource sharing
check.

That was my main concern. I think. :)

[tyoshino (SeeGerritForStatus), 2014-03-11 08:20:57]

On 2014/03/11 07:51:12, sof wrote:
> On 2014/03/11 07:34:34, abarth wrote:
> > Test?

I'll add a layout test by https://codereview.chromium.org/54173002/.

It also included necessary change in Blink.

> > I'm not sure whether you've addressed sof's questions...
> 
> If annotating the data: URL helps reduce overall plumbing needed to provide
the
> required access control for data: resources, then please do it here. But it
> wouldn't be complete without considering data: in the CORS resource sharing
> check.
> 
> That was my main concern. I think. :)

[tyoshino (SeeGerritForStatus), 2014-03-12 07:31:34]

On 2014/03/11 08:20:57, tyoshino wrote:
> On 2014/03/11 07:51:12, sof wrote:
> > On 2014/03/11 07:34:34, abarth wrote:
> > > Test?
> 
> I'll add a layout test by https://codereview.chromium.org/54173002/.
> 
> It also included necessary change in Blink.
> 
> > > I'm not sure whether you've addressed sof's questions...
> > 
> > If annotating the data: URL helps reduce overall plumbing needed to provide
> the
> > required access control for data: resources, then please do it here. But it
> > wouldn't be complete without considering data: in the CORS resource sharing
> > check.
> > 
> > That was my main concern. I think. :)

Can we start with this patch? Modification on DocumentThreadableLoader affects
lots of components and so I want to work on it carefully.

[sof, 2014-03-12 07:54:14]

On 2014/03/12 07:31:34, tyoshino wrote:
> On 2014/03/11 08:20:57, tyoshino wrote:
> > On 2014/03/11 07:51:12, sof wrote:
> > > On 2014/03/11 07:34:34, abarth wrote:
> > > > Test?
> > 
> > I'll add a layout test by https://codereview.chromium.org/54173002/.
> > 
> > It also included necessary change in Blink.
> > 
> > > > I'm not sure whether you've addressed sof's questions...
> > > 
> > > If annotating the data: URL helps reduce overall plumbing needed to
provide
> > the
> > > required access control for data: resources, then please do it here. But
it
> > > wouldn't be complete without considering data: in the CORS resource
sharing
> > > check.
> > > 
> > > That was my main concern. I think. :)
> 
> Can we start with this patch? Modification on DocumentThreadableLoader affects
> lots of components and so I want to work on it carefully.

yes, sounds good to me.

[robwu, 2014-03-17 20:26:53]

tyoshino:
withCredentials for wildcard origins is supported for non-http(s) URLs since
https://codereview.chromium.org/192573011/.

Could you add the following line to fix the withCredentials issue?
    headers->AddHeader("Access-Control-Allow-Credentials: true");

Do not forget to update the tests at https://codereview.chromium.org/54173002/.

[tyoshino (SeeGerritForStatus), 2014-03-18 13:22:45]

On 2014/03/17 20:26:53, robwu wrote:
> tyoshino:
> withCredentials for wildcard origins is supported for non-http(s) URLs since
> https://codereview.chromium.org/192573011/.

Oh, Thanks.

> Could you add the following line to fix the withCredentials issue?
>     headers->AddHeader("Access-Control-Allow-Credentials: true");

Done!

> Do not forget to update the tests at
https://codereview.chromium.org/54173002/.

Sure, done!

[tyoshino (SeeGerritForStatus), 2014-03-24 12:35:16]

abarth: ping

[abarth-chromium, 2014-03-24 16:40:30]

This CL is missing a test.

https://codereview.chromium.org/54233002/diff/260001/content/child/web_url_lo...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/54233002/diff/260001/content/child/web_url_lo...
content/child/web_url_loader_impl.cc:147: "Content-Type: " + mime_type +
";charset=" + charset;
Do we know that mime_type and charset are such that they're not going to break
the grammar of the Content-Type header?

https://codereview.chromium.org/54233002/diff/260001/content/child/web_url_lo...
content/child/web_url_loader_impl.cc:150:
headers->AddHeader("Access-Control-Allow-Credentials: true");
I don't think you can combine Access-Control-Allow-Origin: * with
Access-Control-Allow-Credentials: true.  That's forbidden by the CORS spec.

[tyoshino (SeeGerritForStatus), 2014-03-24 19:36:57]

https://codereview.chromium.org/54233002/diff/260001/content/child/web_url_lo...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/54233002/diff/260001/content/child/web_url_lo...
content/child/web_url_loader_impl.cc:150:
headers->AddHeader("Access-Control-Allow-Credentials: true");
On 2014/03/24 16:40:30, abarth wrote:
> I don't think you can combine Access-Control-Allow-Origin: * with
> Access-Control-Allow-Credentials: true.  That's forbidden by the CORS spec.

robwu@ suggested this change in #21. With
https://codereview.chromium.org/192573011/ and this, Blink can parse data URI
even with withCredentials=true. Otherwise, we need to make some change in Blink
side.

As you suggested that we enable data URI access while keeping it treated as
cross-origin in this comment https://codereview.chromium.org/54173002/#msg2
(when reviewing this patch
https://codereview.chromium.org/54173002/diff/50001/Source/core/loader/Docume...),
I've been trying to address withCredentials issue also by Chromium side
annotation.

Another approach is
- remember that the request was data URI in
DocumentThreadableLoader::loadRequest
- instruct passesAccessControlCheck() to skip credentials check in
DocumentThreadableLoader::didReceiveResponse when the request was data URI

Or it also makes sense, I think, that we rather silently override
allowCredentials of the ThreadableLoaderOptions in XMLHttpRequest::createRequest
when URI is data.

Please tell me you idea about layering.

[tyoshino (SeeGerritForStatus), 2014-03-26 17:34:07]

https://codereview.chromium.org/54233002/diff/260001/content/child/web_url_lo...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/54233002/diff/260001/content/child/web_url_lo...
content/child/web_url_loader_impl.cc:147: "Content-Type: " + mime_type +
";charset=" + charset;
On 2014/03/24 16:40:30, abarth wrote:
> Do we know that mime_type and charset are such that they're not going to break
> the grammar of the Content-Type header?

Added mime_type grammar check in net::DataURL::Parse() and charset grammar check
here respectively because data URL RFC says nothing about charset grammar.

Also added unittests.

[tyoshino (SeeGerritForStatus), 2014-03-26 17:37:59]

+asanka for net/ review

[asanka, 2014-03-26 19:42:08]

https://codereview.chromium.org/54233002/diff/340001/net/base/data_url.cc
File net/base/data_url.cc (right):

https://codereview.chromium.org/54233002/diff/340001/net/base/data_url.cc#new...
net/base/data_url.cc:67: // Check grammar.
Consider using IsMimeType() instead?

[tyoshino (SeeGerritForStatus), 2014-03-26 22:07:00]

https://codereview.chromium.org/54233002/diff/340001/net/base/data_url.cc
File net/base/data_url.cc (right):

https://codereview.chromium.org/54233002/diff/340001/net/base/data_url.cc#new...
net/base/data_url.cc:67: // Check grammar.
On 2014/03/26 19:42:09, asanka wrote:
> Consider using IsMimeType() instead?

I considered that. But IsMimeType() doesn't check token grammar, accepts "*" and
is too strict (allow only IANA registered top level types and x- prefixed ones).
So, I refrained from using that.

I checked again why IsMimeType() is designed like this. It was originally part
of web_intents_registry.cc. Where the code which is now in IsMimeType() worked
as a validation for MIME type pattern to be passed to MatchesMimeType().

https://chromiumcodereview.appspot.com/10448109/diff/23003/chrome/browser/int...

As web_intents_registry.cc has gone and the only user of IsMimeType() is
pepper_file_system_browser_host.cc where it's not expected to check pattern
grammar but just MIME grammar, we could just make IsMimeType():
- stop accepting "*"
- separate registered top-level type validation into a separate method
- validate token grammar on type and subtype

Are you fine with this?

[asanka, 2014-03-26 22:29:42]

https://codereview.chromium.org/54233002/diff/340001/net/base/data_url.cc
File net/base/data_url.cc (right):

https://codereview.chromium.org/54233002/diff/340001/net/base/data_url.cc#new...
net/base/data_url.cc:67: // Check grammar.
On 2014/03/26 22:07:00, tyoshino wrote:
> On 2014/03/26 19:42:09, asanka wrote:
> > Consider using IsMimeType() instead?
> 
> I considered that. But IsMimeType() doesn't check token grammar, accepts "*"
and
> is too strict (allow only IANA registered top level types and x- prefixed
ones).
> So, I refrained from using that.
> 
> I checked again why IsMimeType() is designed like this. It was originally part
> of web_intents_registry.cc. Where the code which is now in IsMimeType() worked
> as a validation for MIME type pattern to be passed to MatchesMimeType().
> 
>
https://chromiumcodereview.appspot.com/10448109/diff/23003/chrome/browser/int...
> 
> As web_intents_registry.cc has gone and the only user of IsMimeType() is
> pepper_file_system_browser_host.cc where it's not expected to check pattern
> grammar but just MIME grammar, we could just make IsMimeType():
> - stop accepting "*"
> - separate registered top-level type validation into a separate method
> - validate token grammar on type and subtype
> 
> Are you fine with this?

SGTM

[tyoshino (SeeGerritForStatus), 2014-03-27 05:48:28]

On 2014/03/26 22:29:42, asanka wrote:
> > As web_intents_registry.cc has gone and the only user of IsMimeType() is
> > pepper_file_system_browser_host.cc where it's not expected to check pattern
> > grammar but just MIME grammar, we could just make IsMimeType():
> > - stop accepting "*"
> > - separate registered top-level type validation into a separate method
> > - validate token grammar on type and subtype
> > 
> > Are you fine with this?
> 
> SGTM

Done. PTAL

[tyoshino (SeeGerritForStatus), 2014-03-27 05:50:05]

Added tzik@ to ensure I'm doing right for pepper_.* file.

[tzik, 2014-03-27 06:22:57]

On 2014/03/27 05:50:05, tyoshino wrote:
> Added tzik@ to ensure I'm doing right for pepper_.* file.

pepper_file_system_browser_host.cc change looks good to me.
I believe the behavior isn't changed in production code,
 since the parameter part of the mimetype is stripped beforehand.

Hiroki-san: Could you double check to this?

[nhiroki, 2014-03-27 06:53:20]

On 2014/03/27 06:22:57, tzik wrote:
> On 2014/03/27 05:50:05, tyoshino wrote:
> > Added tzik@ to ensure I'm doing right for pepper_.* file.
> 
> pepper_file_system_browser_host.cc change looks good to me.
> I believe the behavior isn't changed in production code,
>  since the parameter part of the mimetype is stripped beforehand.
> 
> Hiroki-san: Could you double check to this?

LGTM for pepper_file_system_browser_host.cc.
Let me add dmichael@ for real owner stamp.

[dmichael (off chromium), 2014-03-27 15:53:04]

pepper lgtm

[asanka, 2014-03-27 18:59:51]

net/ lgtm

https://codereview.chromium.org/54233002/diff/390001/net/base/data_url.cc
File net/base/data_url.cc (right):

https://codereview.chromium.org/54233002/diff/390001/net/base/data_url.cc#new...
net/base/data_url.cc:66: } else {
Nit: else if

https://codereview.chromium.org/54233002/diff/390001/net/base/mime_util.h
File net/base/mime_util.h (right):

https://codereview.chromium.org/54233002/diff/390001/net/base/mime_util.h#new...
net/base/mime_util.h:64: // If |subtype| is non-NULL, sets it to parsed subtype
string.
nit: Can you add a comment pointing to the RFC that this grammar is based on?

https://codereview.chromium.org/54233002/diff/390001/net/base/mime_util.h#new...
net/base/mime_util.h:77: NET_EXPORT bool IsValidTopLevelMimeType(const
std::string& type_string);
Nit: I wouldn't object if  this was removed if no-one is using it.

[tyoshino (SeeGerritForStatus), 2014-03-31 13:02:45]

Thanks for review, pepper and net guys.

https://codereview.chromium.org/54233002/diff/390001/net/base/data_url.cc
File net/base/data_url.cc (right):

https://codereview.chromium.org/54233002/diff/390001/net/base/data_url.cc#new...
net/base/data_url.cc:66: } else {
On 2014/03/27 18:59:52, asanka wrote:
> Nit: else if

Done.

https://codereview.chromium.org/54233002/diff/390001/net/base/mime_util.h
File net/base/mime_util.h (right):

https://codereview.chromium.org/54233002/diff/390001/net/base/mime_util.h#new...
net/base/mime_util.h:64: // If |subtype| is non-NULL, sets it to parsed subtype
string.
On 2014/03/27 18:59:52, asanka wrote:
> nit: Can you add a comment pointing to the RFC that this grammar is based on?

Done.

https://codereview.chromium.org/54233002/diff/390001/net/base/mime_util.h#new...
net/base/mime_util.h:77: NET_EXPORT bool IsValidTopLevelMimeType(const
std::string& type_string);
On 2014/03/27 18:59:52, asanka wrote:
> Nit: I wouldn't object if  this was removed if no-one is using it.

OK. For now, I'd like to keep this not to make any behavior change on pepper
code.

[tyoshino (SeeGerritForStatus), 2014-04-08 16:12:34]

Had offline discussion with abarth@. Updated the CL description to
explain decisions we made.

abarth@, please take a look again.

[tyoshino (SeeGerritForStatus), 2014-05-21 09:36:19]

On 2014/04/08 16:12:34, tyoshino wrote:
> Had offline discussion with abarth@. Updated the CL description to
> explain decisions we made.
> 
> abarth@, please take a look again.

Let me commit pepper and net/ changes first as they're useful regardless we add
data: support to XHR or not.

web_url_loader_impl changes will be resent as a new CL.

[tyoshino (SeeGerritForStatus), 2014-05-21 11:21:27]

The CQ bit was checked by tyoshino@chromium.org

[commit-bot: I haz the power, 2014-05-21 11:22:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tyoshino@chromium.org/54233002/510001

[tyoshino (SeeGerritForStatus), 2014-05-21 17:04:44]

The CQ bit was unchecked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2014-05-21 17:05:00]

The CQ bit was checked by tyoshino@chromium.org

[commit-bot: I haz the power, 2014-05-21 19:58:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tyoshino@chromium.org/54233002/510001

[commit-bot: I haz the power, 2014-05-21 23:52:11]

Change committed as 272022