[WebURLLoaderImpl] Don't respond to data URL request if downloadToFile is set

[WebURLLoaderImpl] Don't respond to data URL request if downloadToFile is set

BUG=405841
Committed: https://crrev.com/30b387429c8f5c7485b82d12bb70cc6181231020
Cr-Commit-Position: refs/heads/master@{#293905}

[eseidel, 2014-08-21 15:18:31]

Do we have a test to cover this if we break this again? :)

[tyoshino (SeeGerritForStatus), 2014-08-21 15:47:54]

On 2014/08/21 15:18:31, eseidel wrote:
> Do we have a test to cover this if we break this again? :)

Sorry. I forgot to enable and recheck the data URL layout tests
https://codereview.chromium.org/492183002/. The tests cover this change.

[tyoshino (SeeGerritForStatus), 2014-08-28 14:54:48]

ping as 1 w passed.

The layout test (https://codereview.chromium.org/492183002/) will be checked in
later.

[eseidel, 2014-08-28 15:14:53]

I would lgtm if I could, but I don't have the power.  I would try adding more
content/ reviewers.  I've been told they're a dying breed.

[tyoshino (SeeGerritForStatus), 2014-09-02 04:40:27]

On 2014/08/28 15:14:53, eseidel wrote:
> I would lgtm if I could, but I don't have the power.  I would try adding more
> content/ reviewers.  I've been told they're a dying breed.

+jam

This is required to reland https://codereview.chromium.org/294193002.

[tyoshino (SeeGerritForStatus), 2014-09-02 04:40:36]

tyoshino@chromium.org changed reviewers:
+ jam@chromium.org

[tyoshino (SeeGerritForStatus), 2014-09-02 04:40:48]

+jam

This is required to reland https://codereview.chromium.org/294193002.

[jam, 2014-09-03 17:34:07]

lgtm

[tyoshino (SeeGerritForStatus), 2014-09-04 07:03:41]

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

https://codereview.chromium.org/480413007/diff/20001/content/child/web_url_lo...
content/child/web_url_loader_impl.cc:662: return false;
Hmm, it didn't compile. I don't remember if I forgot to compile.

We need to create a GURL instance to call GURL::SchemeIs()

Fixed

[tyoshino (SeeGerritForStatus), 2014-09-04 08:57:40]

Sorry, Eric, John.

This CL didn't include the main change but just refactoring of the
CanHandleDataURL() method...

I added it in the patch set 3. PTAL.

[eseidel, 2014-09-04 16:31:25]

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

https://codereview.chromium.org/480413007/diff/40001/content/child/web_url_lo...
content/child/web_url_loader_impl.cc:315: if (IsHandlableDataURLRequest()) {
I would have used the active voice "CanHandleDataURLRequest()" instead of
passive "is handleable".

I might also have written a unittest for your new function.

But otherwise lgtm.

[tyoshino (SeeGerritForStatus), 2014-09-05 14:05:09]

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

https://codereview.chromium.org/480413007/diff/40001/content/child/web_url_lo...
content/child/web_url_loader_impl.cc:315: if (IsHandlableDataURLRequest()) {
On 2014/09/04 16:31:25, eseidel wrote:
> I would have used the active voice "CanHandleDataURLRequest()" instead of
> passive "is handleable".

In the name, I also wanted to mean that we're checking whether request_ is data
URL request or not. I.e. IsDataURLRequest && CanHandleByFastPath ->
IsHandlableDataURLRequest.

How about CanHandleRequestWithDataURLParser?

> 
> I might also have written a unittest for your new function.

The algorithm added is very simple. As darin suggested that I don't export and
test GetInfoFromDataURL() in the last CL, I'd also like to skip adding tests for
this.

> 
> But otherwise lgtm.

[eseidel, 2014-09-05 14:25:34]

sgtm.  lgtm

On Fri, Sep 5, 2014 at 7:05 AM,  <tyoshino@chromium.org> wrote:
>
>
https://codereview.chromium.org/480413007/diff/40001/content/child/web_url_lo...
> File content/child/web_url_loader_impl.cc (right):
>
>
https://codereview.chromium.org/480413007/diff/40001/content/child/web_url_lo...
> content/child/web_url_loader_impl.cc:315: if
> (IsHandlableDataURLRequest()) {
> On 2014/09/04 16:31:25, eseidel wrote:
>>
>> I would have used the active voice "CanHandleDataURLRequest()" instead
>
> of
>>
>> passive "is handleable".
>
>
> In the name, I also wanted to mean that we're checking whether request_
> is data URL request or not. I.e. IsDataURLRequest && CanHandleByFastPath
> -> IsHandlableDataURLRequest.
>
> How about CanHandleRequestWithDataURLParser?
>
>
>> I might also have written a unittest for your new function.
>
>
> The algorithm added is very simple. As darin suggested that I don't
> export and test GetInfoFromDataURL() in the last CL, I'd also like to
> skip adding tests for this.
>
>
>
>> But otherwise lgtm.
>
>
> https://codereview.chromium.org/480413007/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jam, 2014-09-09 00:33:15]

lgtm

[tyoshino (SeeGerritForStatus), 2014-09-09 02:32:48]

The CQ bit was checked by tyoshino@chromium.org

[commit-bot: I haz the power, 2014-09-09 02:35:13]

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

[darin (slow to review), 2014-09-09 05:59:11]

https://codereview.chromium.org/480413007/diff/1/content/child/web_url_loader...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/480413007/diff/1/content/child/web_url_loader...
content/child/web_url_loader_impl.cc:318: if (IsHandlableDataURLRequest()) {
Hmm, "Handleable" is probably a better spelling than "Handlable", but it still
reads fairly awkwardly. How about ShouldHandleDataURLRequest?

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

https://codereview.chromium.org/480413007/diff/60001/content/child/web_url_lo...
content/child/web_url_loader_impl.cc:659: bool
WebURLLoaderImpl::Context::CanHandleRequestWithDataURLParser() const {
I don't mean to hold up this CL or anything, so forgive the nit-pick. This name
suggests that elsewhere the DataURL is handled using something other than the
DataURL parser. But, in fact it is still handled using the same DataURL parser
in the browser process. Both here and there use the same parser. The difference
is that we can sometimes use the parser locally without delegating to the
ResourceLoaderBridge.

Hence, something like CanHandleDataURLRequestLocally might be a more accurate
name.

Anyways, sorry to nit pick on names! But, there... I think that's a better name
:)

[tyoshino (SeeGerritForStatus), 2014-09-09 07:09:47]

The CQ bit was unchecked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2014-09-09 07:11:59]

https://codereview.chromium.org/480413007/diff/1/content/child/web_url_loader...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/480413007/diff/1/content/child/web_url_loader...
content/child/web_url_loader_impl.cc:318: if (IsHandlableDataURLRequest()) {
On 2014/09/09 05:59:11, darin wrote:
> Hmm, "Handleable" is probably a better spelling than "Handlable", but it still
> reads fairly awkwardly. How about ShouldHandleDataURLRequest?

Thanks for suggestion.

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

https://codereview.chromium.org/480413007/diff/60001/content/child/web_url_lo...
content/child/web_url_loader_impl.cc:659: bool
WebURLLoaderImpl::Context::CanHandleRequestWithDataURLParser() const {
On 2014/09/09 05:59:11, darin wrote:
> I don't mean to hold up this CL or anything, so forgive the nit-pick. This
name
> suggests that elsewhere the DataURL is handled using something other than the
> DataURL parser. But, in fact it is still handled using the same DataURL parser
> in the browser process. Both here and there use the same parser. The
difference
> is that we can sometimes use the parser locally without delegating to the
> ResourceLoaderBridge.
> 
> Hence, something like CanHandleDataURLRequestLocally might be a more accurate
> name.
> 
> Anyways, sorry to nit pick on names! But, there... I think that's a better
name
> :)

OK! Fixed

[tyoshino (SeeGerritForStatus), 2014-09-09 07:12:04]

The CQ bit was checked by tyoshino@chromium.org

[commit-bot: I haz the power, 2014-09-09 07:14:30]

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

[commit-bot: I haz the power, 2014-09-09 09:26:03]

Committed patchset #5 (id:80001) as 91101840657c4367c196e4bf6730263fe86a2c12

[commit-bot: I haz the power, 2014-09-10 03:52:18]

Patchset 5 (id:??) landed as
https://crrev.com/30b387429c8f5c7485b82d12bb70cc6181231020
Cr-Commit-Position: refs/heads/master@{#293905}