Implement URLRequestDataJob

Continue Hayato Ito's work on issue #24846.

Implement URLRequestDataJob class to handle "data:" URLs.

Also no longer attempts to extract a file name suggestion from "about:" and "data:" URLs, since esp. "data:" URLs may contain slashes that would lead to unwanted results.


BUG=24846
TEST=URLRequestTest.DataURLImageTest, NetUtilTest.GetSuggestedFilename

[Paweł Hajdan Jr., 2009-10-19 13:42:18]

http://codereview.chromium.org/297003/diff/1/4
File net/base/net_util_unittest.cc (right):

http://codereview.chromium.org/297003/diff/1/4#newcode976
Line 976: {"about:chrome",
nit: It seems that there should be one less space before it.

http://codereview.chromium.org/297003/diff/1/5
File net/url_request/url_request_data_job.cc (right):

http://codereview.chromium.org/297003/diff/1/5#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
nit: 2009

http://codereview.chromium.org/297003/diff/1/5#newcode22
Line 22: , is_valid_(false)
nit: This is not how it's formatted, please see the style guide or other files
here.

http://codereview.chromium.org/297003/diff/1/5#newcode46
Line 46: if (request_->url().is_valid())
nit: This if should use braces for better readability.

http://codereview.chromium.org/297003/diff/1/5#newcode63
Line 63: int *bytes_read) {
nit: should be int* bytes_read (note the star).

http://codereview.chromium.org/297003/diff/1/6
File net/url_request/url_request_data_job.h (right):

http://codereview.chromium.org/297003/diff/1/6#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
nit: It's a new file, please put 2009 in the copyright, like this:

// Copyright (c) 2009 The Chromium Authors. All rights reserved.

http://codereview.chromium.org/297003/diff/1/6#newcode30
Line 30: void StartAsync();
nit: Method declaration should be before members declarations.

http://codereview.chromium.org/297003/diff/1/6#newcode31
Line 31: };
nit: Please use DISALLOW_COPY_AND_ASSIGN.

[darin (slow to review), 2009-10-19 16:05:25]

http://codereview.chromium.org/297003/diff/1/6
File net/url_request/url_request_data_job.h (right):

http://codereview.chromium.org/297003/diff/1/6#newcode13
Line 13: class URLRequestDataJob : public URLRequestJob {
please subclass URLRequestSimpleJob instead.  it will greatly simplify your
implementation here.

[darin (slow to review), 2009-10-19 16:09:08]

http://codereview.chromium.org/297003/diff/1/3
File net/base/net_util.cc (right):

http://codereview.chromium.org/297003/diff/1/3#newcode1043
Line 1043: return default_name.empty() ? L"download" : default_name;
I think you should just return default_name here even if it is empty.  I don't
think you want to invent strings like "download" at this layer.  Such strings
need to be localized, and this layer doesn't have a good way to do such things. 
The caller should have provided a localized default name.

[Roland, 2009-10-20 04:59:35]

Uploaded a new patch set that contains (almost all - see below) the corrections
and is now based off URLRequestSimpleJob.

On 2009/10/19 16:05:25, darin wrote:
> http://codereview.chromium.org/297003/diff/1/6#newcode13
> Line 13: class URLRequestDataJob : public URLRequestJob {
> please subclass URLRequestSimpleJob instead.  it will greatly simplify your
> implementation here.

Oh yes, it did! :)

> http://codereview.chromium.org/297003/diff/1/3
> File net/base/net_util.cc (right):
> 
> http://codereview.chromium.org/297003/diff/1/3#newcode1043
> Line 1043: return default_name.empty() ? L"download" : default_name;
> I think you should just return default_name here even if it is empty.  I don't
> think you want to invent strings like "download" at this layer.  Such strings
> need to be localized, and this layer doesn't have a good way to do such
things. 
> The caller should have provided a localized default name.

This is mainly to stay consistent with the fallback branch near the end of the
function, so I left this in in the new patch set. As an alternative to
"download" I also mulled using the scheme (i.e., "about" or "data") as final
fallback name.
Of course, if you think it'd still better, then I can change it to a simple
"return default_name;".

[darin (slow to review), 2009-10-20 05:36:41]

Looks good, but one minor issue:

http://codereview.chromium.org/297003/diff/7001/8002
File net/base/net_util.cc (right):

http://codereview.chromium.org/297003/diff/7001/8002#newcode1043
Line 1043: return default_name.empty() ? L"download" : default_name;
ok, i didn't realize this file already fell back to "download" in some cases :-(

please at least define a constant for "download" and use it both here and below.

i wonder if a bug should be filed about "download" being used in all locales.

[Roland, 2009-10-20 06:17:05]

Uploaded a new patch set with the constant.

Also filed bug http://code.google.com/p/chromium/issues/detail?id=25289 so that
the issue with the constant isn't forgotten.

[darin (slow to review), 2009-10-20 06:24:18]

http://codereview.chromium.org/297003/diff/10001/4003
File net/base/net_util.cc (right):

http://codereview.chromium.org/297003/diff/10001/4003#newcode1042
Line 1042: static const wchar_t* final_fallback_name = L"download";
please use google style for constants:  kFooBar

in this case, i wouldn't make it static since that doesn't add much (it actually
adds an initialization branch).  just do this:

  const wchar_t kFallbackName[] = L"download";

[Roland, 2009-10-20 06:53:56]

On 2009/10/20 06:24:18, darin wrote:
> http://codereview.chromium.org/297003/diff/10001/4003
> File net/base/net_util.cc (right):
> 
> http://codereview.chromium.org/297003/diff/10001/4003#newcode1042
> Line 1042: static const wchar_t* final_fallback_name = L"download";
> please use google style for constants:  kFooBar

Argh, of course! >_<; (I blame too much jumping between WebKit and Chromium, and
a lack of coffee!)

Also updated the comment to point at the new bug tracker issue.

[darin (slow to review), 2009-10-20 06:56:27]

LGTM

(nit: use http://crbug.com/ instead for brevity)

[Alpha Left Google, 2009-10-22 00:52:50]

Is this being landed? Or I can land it?

[Roland, 2009-10-22 04:20:41]

committed at http://code.google.com/p/chromium/source/detail?r=29759