Set the same headers for data URLs in WebURLLoaderImpl and ResourceFetcher

Set the same headers for data URLs in WebURLLoaderImpl and ResourceFetcher

Response headers are appended by net::URLRequestDataJob::BuildResponse()
when WebURLLoaderImpl returns data URL response in a renderer process,
but not when ResourceFetcher returns data URL response synchronously by
ResourceFetcher::resourceForStaticData().

This CL makes the response headers to be added in both cases, by making
NetworkUtils::parseDataURL() (that is used from resourceForStaticData())
to call net::URLRequestDataJob::BuildResponse() and set response headers.

BUG=704032
Review-Url: https://codereview.chromium.org/2766583002
Cr-Commit-Position: refs/heads/master@{#459026}
Committed: https://chromium.googlesource.com/chromium/src/+/5b0d721fcc5ac7f22db722c7514a52acf8b661ea

[hiroshige, 2017-03-21 05:33:03]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 05:33:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-03-21 05:37:14]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 05:37:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-03-21 05:42:40]

Description was changed from

==========
Set the same headers for data URLs in WebURLLoaderImpl and ResourceFetcher

BUG=
==========

to

==========
Set the same headers for data URLs in WebURLLoaderImpl and ResourceFetcher

Response headers are appended by net::URLRequestDataJob::BuildResponse()
when WebURLLoaderImpl returns data URL response in a renderer process,
but not when ResourceFetcher returns data URL response synchronously by
ResourceFetcher::resourceForStaticData().

This CL makes the response headers to be added in both cases, by making
NetworkUtils::parseDataURL() (that is used from resourceForStaticData())
to call net::URLRequestDataJob::BuildResponse() and set response headers.

BUG=
==========

[hiroshige, 2017-03-21 05:42:41]

hiroshige@chromium.org changed reviewers:
+ kinuko@chromium.org, yhirano@chromium.org

[commit-bot: I haz the power, 2017-03-21 07:16:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 07:16:09]

Dry run: This issue passed the CQ dry run.

[kouhei (in TOK), 2017-03-21 07:29:19]

kouhei@chromium.org changed reviewers:
+ kouhei@chromium.org

[kouhei (in TOK), 2017-03-21 07:29:20]

Thanks for working on this!

https://codereview.chromium.org/2766583002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
(right):

https://codereview.chromium.org/2766583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:375: data =
PassRefPtr<SharedBuffer>(NetworkUtils::parseDataURL(url, response));
can we note that response is modified by parseDataURL?
(I thought response was used here uninit)

[hiroshige, 2017-03-21 08:00:13]

PTAL.

https://codereview.chromium.org/2766583002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp (left):

https://codereview.chromium.org/2766583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:391:
response.setHTTPStatusCode(200);
This block is moved to parseDataURL().

[hiroshige, 2017-03-21 08:04:47]

https://codereview.chromium.org/2766583002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
(right):

https://codereview.chromium.org/2766583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:375: data =
PassRefPtr<SharedBuffer>(NetworkUtils::parseDataURL(url, response));
On 2017/03/21 07:29:20, kouhei wrote:
> can we note that response is modified by parseDataURL?
> (I thought response was used here uninit)

Done.

[hiroshige, 2017-03-21 08:04:54]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 08:05:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 09:39:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 09:39:35]

Dry run: This issue passed the CQ dry run.

[kouhei (in TOK), 2017-03-21 11:07:49]

kouhei@chromium.org changed reviewers:
+ toyoshim@chromium.org

[kouhei (in TOK), 2017-03-21 11:07:49]

https://codereview.chromium.org/2766583002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/DEPS (right):

https://codereview.chromium.org/2766583002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/DEPS:10:
"+net/url_request/url_request_data_job.h",
Should we move NetworkUtils or parseDataURL to platform/loader?

[kinuko, 2017-03-21 12:42:47]

Could we file a bug for this one (or use an existing bug)?

Should this code maybe live in its own file rather than in NetworkUtils (which
contains more lower-layer stuff)?

[Takashi Toyoshima, 2017-03-22 06:26:21]

Discussed offline. We would move platform/network to platform/loader/base.
Probably it's fine to use crbug.com/699369 to leave file moving works as a TODO?

[hiroshige, 2017-03-22 08:05:15]

Description was changed from

==========
Set the same headers for data URLs in WebURLLoaderImpl and ResourceFetcher

Response headers are appended by net::URLRequestDataJob::BuildResponse()
when WebURLLoaderImpl returns data URL response in a renderer process,
but not when ResourceFetcher returns data URL response synchronously by
ResourceFetcher::resourceForStaticData().

This CL makes the response headers to be added in both cases, by making
NetworkUtils::parseDataURL() (that is used from resourceForStaticData())
to call net::URLRequestDataJob::BuildResponse() and set response headers.

BUG=
==========

to

==========
Set the same headers for data URLs in WebURLLoaderImpl and ResourceFetcher

Response headers are appended by net::URLRequestDataJob::BuildResponse()
when WebURLLoaderImpl returns data URL response in a renderer process,
but not when ResourceFetcher returns data URL response synchronously by
ResourceFetcher::resourceForStaticData().

This CL makes the response headers to be added in both cases, by making
NetworkUtils::parseDataURL() (that is used from resourceForStaticData())
to call net::URLRequestDataJob::BuildResponse() and set response headers.

BUG=704032
==========

[hiroshige, 2017-03-22 08:09:15]

On 2017/03/22 06:26:21, Takashi Toyoshima wrote:
> Could we file a bug for this one (or use an existing bug)?
Created Issue 704032.

> Discussed offline. We would move platform/network to platform/loader/base.
> Probably it's fine to use crbug.com/699369 to leave file moving works as a
TODO?
Do we need TODO inside this CL?

[hiroshige, 2017-03-22 08:09:33]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 08:09:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 09:37:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 09:37:48]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-03-22 14:05:38]

lgtm, given that we're going to restructure the file/directory soon

https://codereview.chromium.org/2766583002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/NetworkUtils.h (right):

https://codereview.chromium.org/2766583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/NetworkUtils.h:33: PLATFORM_EXPORT
PassRefPtr<SharedBuffer> parseDataURL(const KURL&,
nit: can we rename this to sth like parseDataURLAndPopulateResponse() ?

[kouhei (in TOK), 2017-03-23 00:37:15]

lgtm

https://codereview.chromium.org/2766583002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/NetworkUtils.cpp (right):

https://codereview.chromium.org/2766583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/NetworkUtils.cpp:104: return
std::move(data);
I don't think std::move is needed here, as we can assume NRVO

[hiroshige, 2017-03-23 03:21:41]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 03:22:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-03-23 03:22:23]

https://codereview.chromium.org/2766583002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/NetworkUtils.cpp (right):

https://codereview.chromium.org/2766583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/NetworkUtils.cpp:104: return
std::move(data);
On 2017/03/23 00:37:15, kouhei wrote:
> I don't think std::move is needed here, as we can assume NRVO

Done.

https://codereview.chromium.org/2766583002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/NetworkUtils.h (right):

https://codereview.chromium.org/2766583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/NetworkUtils.h:33: PLATFORM_EXPORT
PassRefPtr<SharedBuffer> parseDataURL(const KURL&,
On 2017/03/22 14:05:38, kinuko wrote:
> nit: can we rename this to sth like parseDataURLAndPopulateResponse() ?

Done.

[commit-bot: I haz the power, 2017-03-23 03:38:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 03:38:42]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[hiroshige, 2017-03-23 04:18:33]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2017-03-23 04:18:33]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
kouhei@chromium.org
Link to the patchset: https://codereview.chromium.org/2766583002/#ps80001
(title: "reflect comments")

[commit-bot: I haz the power, 2017-03-23 04:18:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 04:36:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 04:36:18]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[yhirano, 2017-03-23 04:53:42]

lgtm

[hiroshige, 2017-03-23 05:45:26]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2017-03-23 05:45:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 05:59:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 05:59:54]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[hiroshige, 2017-03-23 06:47:24]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2017-03-23 06:47:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 07:03:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 07:03:25]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[kouhei (in TOK), 2017-03-23 08:18:54]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2017-03-23 08:19:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 08:33:08]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490257134549090,
"parent_rev": "33a5703a620ec246ee08214e6c880068b6e9d687", "commit_rev":
"5b0d721fcc5ac7f22db722c7514a52acf8b661ea"}

[commit-bot: I haz the power, 2017-03-23 08:33:47]

Description was changed from

==========
Set the same headers for data URLs in WebURLLoaderImpl and ResourceFetcher

Response headers are appended by net::URLRequestDataJob::BuildResponse()
when WebURLLoaderImpl returns data URL response in a renderer process,
but not when ResourceFetcher returns data URL response synchronously by
ResourceFetcher::resourceForStaticData().

This CL makes the response headers to be added in both cases, by making
NetworkUtils::parseDataURL() (that is used from resourceForStaticData())
to call net::URLRequestDataJob::BuildResponse() and set response headers.

BUG=704032
==========

to

==========
Set the same headers for data URLs in WebURLLoaderImpl and ResourceFetcher

Response headers are appended by net::URLRequestDataJob::BuildResponse()
when WebURLLoaderImpl returns data URL response in a renderer process,
but not when ResourceFetcher returns data URL response synchronously by
ResourceFetcher::resourceForStaticData().

This CL makes the response headers to be added in both cases, by making
NetworkUtils::parseDataURL() (that is used from resourceForStaticData())
to call net::URLRequestDataJob::BuildResponse() and set response headers.

BUG=704032

Review-Url: https://codereview.chromium.org/2766583002
Cr-Commit-Position: refs/heads/master@{#459026}
Committed:
https://chromium.googlesource.com/chromium/src/+/5b0d721fcc5ac7f22db722c7514a...
==========

[commit-bot: I haz the power, 2017-03-23 08:33:50]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/5b0d721fcc5ac7f22db722c7514a...