Do not preload data URIs.

Do not preload data URIs.

Data URI preload creates an extra fetch request which is even shown in DevTools,
but there is no need to preload since data is already there.

BUG=482779
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197922

[dgozman, 2015-06-24 16:43:02]

dgozman@chromium.org changed reviewers:
+ yoav@yoav.ws

[dgozman, 2015-06-24 16:43:02]

Yoav, could you please take a look?
Do we have unit test exercising this code?

Thanks,
Dmitry

[Yoav Weiss, 2015-06-25 10:21:30]

It may be easier to unit test that if it was implemented earlier, as part of
HTMLPreloadScanner::shouldPreload (and you could also claim that it would be
cleaner to do so).

The problem there is that you only have the URL as a string, not KURL.

You could change PreloadRequest's resourceURL to be a KURL, and work off that.

https://codereview.chromium.org/1203143002/diff/1/Source/core/html/parser/HTM...
File Source/core/html/parser/HTMLResourcePreloader.cpp (right):

https://codereview.chromium.org/1203143002/diff/1/Source/core/html/parser/HTM...
Source/core/html/parser/HTMLResourcePreloader.cpp:80: if
(preload->resourceType() == Resource::Image && request.url().protocolIsData())
I wouldn't block that on images only. It makes no sense to do that for any kind
of resource

[dgozman, 2015-06-25 10:35:51]

On 2015/06/25 10:21:30, Yoav Weiss wrote:
> It may be easier to unit test that if it was implemented earlier, as part of
> HTMLPreloadScanner::shouldPreload (and you could also claim that it would be
> cleaner to do so).
> 
> The problem there is that you only have the URL as a string, not KURL.
> 
> You could change PreloadRequest's resourceURL to be a KURL, and work off that.
That was my first idea. However, PreloadRequest uses
Document::completeURLWithOverride to compute the actual resource url, and I am
not sure about bringing Document to HTMLPreloadScanner. WDYT?


> 
>
https://codereview.chromium.org/1203143002/diff/1/Source/core/html/parser/HTM...
> File Source/core/html/parser/HTMLResourcePreloader.cpp (right):
> 
>
https://codereview.chromium.org/1203143002/diff/1/Source/core/html/parser/HTM...
> Source/core/html/parser/HTMLResourcePreloader.cpp:80: if
> (preload->resourceType() == Resource::Image && request.url().protocolIsData())
> I wouldn't block that on images only. It makes no sense to do that for any
kind
> of resource
Okay. I'm just being very cautious since I have not enough knowledge in the
area.

[Yoav Weiss, 2015-06-25 10:59:04]

On 2015/06/25 10:35:51, dgozman wrote:
> On 2015/06/25 10:21:30, Yoav Weiss wrote:
> > It may be easier to unit test that if it was implemented earlier, as part of
> > HTMLPreloadScanner::shouldPreload (and you could also claim that it would be
> > cleaner to do so).
> > 
> > The problem there is that you only have the URL as a string, not KURL.
> > 
> > You could change PreloadRequest's resourceURL to be a KURL, and work off
that.
> That was my first idea. However, PreloadRequest uses
> Document::completeURLWithOverride to compute the actual resource url, and I am
> not sure about bringing Document to HTMLPreloadScanner. WDYT?

Oh noes, we can't do that. HTMLPreloadScanner runs in the parser thread.
I'll look into better options.

Otherwise, we can always add unit tests for HTMLResourcePreloader.

> 
> 
> > 
> >
>
https://codereview.chromium.org/1203143002/diff/1/Source/core/html/parser/HTM...
> > File Source/core/html/parser/HTMLResourcePreloader.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1203143002/diff/1/Source/core/html/parser/HTM...
> > Source/core/html/parser/HTMLResourcePreloader.cpp:80: if
> > (preload->resourceType() == Resource::Image &&
request.url().protocolIsData())
> > I wouldn't block that on images only. It makes no sense to do that for any
> kind
> > of resource
> Okay. I'm just being very cautious since I have not enough knowledge in the
> area.

[dgozman, 2015-06-26 10:11:54]

> > That was my first idea. However, PreloadRequest uses
> > Document::completeURLWithOverride to compute the actual resource url, and I
am
> > not sure about bringing Document to HTMLPreloadScanner. WDYT?
> 
> Oh noes, we can't do that. HTMLPreloadScanner runs in the parser thread.
> I'll look into better options.

Any ideas?

[Yoav Weiss, 2015-06-26 11:55:54]

On 2015/06/26 10:11:54, dgozman wrote:
> > > That was my first idea. However, PreloadRequest uses
> > > Document::completeURLWithOverride to compute the actual resource url, and
I
> am
> > > not sure about bringing Document to HTMLPreloadScanner. WDYT?
> > 
> > Oh noes, we can't do that. HTMLPreloadScanner runs in the parser thread.
> > I'll look into better options.
> 
> Any ideas?

Sorry for being slow to respond. On the roads...

Eventually, we probably want to make the logic behind
Document::completeURLWithOverride static and be able to access that with the
document's base URL and discovered <base> URLs from the HTMLPreloadScanner.
That's a fairly large rewrite though, so unless you're super eager to take on
that, I'd be fine with keeping the data URI avoidance where you have put it, and
add a TODO that we need to fix that up and move that logic into the preload
scanner. I'd hate to block a bug fix on refactoring :)

So, coming back to the question of testing, you can start an
HTMLResourcePreloader unit test file.
You probably want to create a DummyPageHolder and use its document (and a mock
loader) in order to keep track of when startPreload is sent out.
Then feed preload() with a PreloadRequest that contains a data URI and see that
nothing happens.

Does that make sense?

[Yoav Weiss, 2015-06-26 12:04:28]

On 2015/06/26 11:55:54, Yoav Weiss wrote:
> On 2015/06/26 10:11:54, dgozman wrote:
> > > > That was my first idea. However, PreloadRequest uses
> > > > Document::completeURLWithOverride to compute the actual resource url,
and
> I
> > am
> > > > not sure about bringing Document to HTMLPreloadScanner. WDYT?
> > > 
> > > Oh noes, we can't do that. HTMLPreloadScanner runs in the parser thread.
> > > I'll look into better options.
> > 
> > Any ideas?
> 
> Sorry for being slow to respond. On the roads...
> 
> Eventually, we probably want to make the logic behind
> Document::completeURLWithOverride static and be able to access that with the
> document's base URL and discovered <base> URLs from the HTMLPreloadScanner.
> That's a fairly large rewrite though, so unless you're super eager to take on
> that, I'd be fine with keeping the data URI avoidance where you have put it,
and
> add a TODO that we need to fix that up and move that logic into the preload
> scanner. I'd hate to block a bug fix on refactoring :)
> 
> So, coming back to the question of testing, you can start an
> HTMLResourcePreloader unit test file.
> You probably want to create a DummyPageHolder and use its document (and a mock
> loader) in order to keep track of when startPreload is sent out.
> Then feed preload() with a PreloadRequest that contains a data URI and see
that
> nothing happens.
> 
> Does that make sense?

Alternatively, we could get by with layout tests for now.
For an example on layout tests around the preloader, see
http/tests/loading/preload-image-src.html

[dgozman, 2015-06-26 12:51:01]

Please take another look. Now with layout test case (so easy to add, I like it)
and TODO.

[Yoav Weiss, 2015-06-26 13:56:46]

On 2015/06/26 12:51:01, dgozman wrote:
> Please take another look. Now with layout test case (so easy to add, I like
it)
> and TODO.

LGTM :)

[dgozman, 2015-06-26 14:01:05]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2015-06-26 14:01:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1203143002/20001

[commit-bot: I haz the power, 2015-06-26 15:06:44]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197922