Prepopulate the memoryCache with data:uri images.

Prepopulate the memoryCache with data:uri images.

This patch prepopulates the memoryCache for data:uri  images, fixing a
long-standing bug where data uri  subresources did not load for SVG images
(network loads are still disabled).

The two large changes are the following new methods:
1) resourceFromDataURIRequest has been added which returns a resource from a
data uri request. This function was placed on CachedResource because it  needs
to use the createResource helper function. I hope this function can be re-used
in a followup patch to reduce memory in data uri handling.

2) preCacheDataURIImage has been added which simply prefills the memory cache
with a data uri image.

----------------

Update after rollout: Several changes were needed to address test failures:

1) fast/backgrounds/solid-color-context-restore.html
This test was written 7 years ago and no longer tests what it should. The test
relied on a solid color data image being drawn before a selection rect changed
but the image was not actually drawing since data urls loaded and drew
asynchronously.
After this change, the bug in this test was exposed and the image would draw
after the selection changed. This patch removes this test entirely. The bug it
fixed only affected ImageMac which no longer exists. See wkbug.com/10158.

2) fast/forms/basic-buttons.html
This test relied on images not loading synchronously and the image size values
were checked in as 0. This test just needs a rebaseline to correctly set the
image sizes in the expectations.

3) http/tests/inspector/network/image-as-text-loading-data-url.html
This test was for pretty-printing very long data url errors. Because data image
loads no longer touch the network, the error message is no longer printed for
images. I have updated this test to use a long script data url instead of an
image. The test has been renamed "script-as-text-loading-data-url.html".

4) virtual/softwarecompositing/tiling/huge-layer-img.html
This test just needed a rebaseline.

5) I have added two tests to prove synchronicity:
data-uri-images-load-synchronously.html
data-uri-images-reload-synchronously.html
The first is a test for the specific change exposed by
fast/forms/basic-buttons.html: image sizes are now available synchronously. The
second verifies that we do not reload data uris but instead pull them from the
cache.

Lastly, this updated patch no longer checks for incorrect resource types coming
from the cache. This was redundant with a check in determineRevalidationPolicy.

BUG=224317
R=abarth@chromium.org

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

[abarth-chromium, 2013-06-05 23:42:51]

Looks great.  Some minor nits below.  japhet should also look at this CL.

https://codereview.chromium.org/16433002/diff/9002/Source/core/loader/cache/C...
File Source/core/loader/cache/CachedResourceLoader.cpp (right):

https://codereview.chromium.org/16433002/diff/9002/Source/core/loader/cache/C...
Source/core/loader/cache/CachedResourceLoader.cpp:105: KURL url = request.url();
this can be a const KURL& rather than a KURL

https://codereview.chromium.org/16433002/diff/9002/Source/core/loader/cache/C...
Source/core/loader/cache/CachedResourceLoader.cpp:111: if (!data ||
!data->size())
Checking the size here doesn't seem right.  It's better to no call appendData if
there isn't any data.

https://codereview.chromium.org/16433002/diff/9002/Source/core/loader/cache/C...
Source/core/loader/cache/CachedResourceLoader.cpp:182: KURL url =
request.resourceRequest().url();
const KURL&

https://codereview.chromium.org/16433002/diff/9002/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/16433002/diff/9002/public/platform/Platform.h...
public/platform/Platform.h:290: // Returns true if the data url was of a
supported mimetype and was parsed successfully.
"Returns true" <--- not accurate anymore

[Nate Chapin, 2013-06-05 23:50:54]

https://codereview.chromium.org/16433002/diff/9002/Source/core/loader/cache/C...
File Source/core/loader/cache/CachedResourceLoader.cpp (right):

https://codereview.chromium.org/16433002/diff/9002/Source/core/loader/cache/C...
Source/core/loader/cache/CachedResourceLoader.cpp:174:
preCacheDataURIImage(request);
Should this early return? Otherwise, won't we run the request normally in
addition?

[Nate Chapin, 2013-06-05 23:52:37]

On 2013/06/05 23:50:54, Nate Chapin wrote:
>
https://codereview.chromium.org/16433002/diff/9002/Source/core/loader/cache/C...
> File Source/core/loader/cache/CachedResourceLoader.cpp (right):
> 
>
https://codereview.chromium.org/16433002/diff/9002/Source/core/loader/cache/C...
> Source/core/loader/cache/CachedResourceLoader.cpp:174:
> preCacheDataURIImage(request);
> Should this early return? Otherwise, won't we run the request normally in
> addition?

Wow, I totally flaked with that question.

More correctly: Is there any case in which we won't use the cached version when
requestResource() finds it?

[pdr., 2013-06-05 23:53:25]

https://codereview.chromium.org/16433002/diff/9002/Source/core/loader/cache/C...
File Source/core/loader/cache/CachedResourceLoader.cpp (right):

https://codereview.chromium.org/16433002/diff/9002/Source/core/loader/cache/C...
Source/core/loader/cache/CachedResourceLoader.cpp:105: KURL url = request.url();
On 2013/06/05 23:42:51, abarth wrote:
> this can be a const KURL& rather than a KURL

Done.

https://codereview.chromium.org/16433002/diff/9002/Source/core/loader/cache/C...
Source/core/loader/cache/CachedResourceLoader.cpp:111: if (!data ||
!data->size())
On 2013/06/05 23:42:51, abarth wrote:
> Checking the size here doesn't seem right.  It's better to no call appendData
if
> there isn't any data.

Done.

https://codereview.chromium.org/16433002/diff/9002/Source/core/loader/cache/C...
Source/core/loader/cache/CachedResourceLoader.cpp:174:
preCacheDataURIImage(request);
On 2013/06/05 23:50:54, Nate Chapin wrote:
> Should this early return? Otherwise, won't we run the request normally in
> addition?

That's the idea: we pre-populate the memory cache and then run the request
normally. This prevents us from having to duplicate some of the loader logic.

https://codereview.chromium.org/16433002/diff/9002/Source/core/loader/cache/C...
Source/core/loader/cache/CachedResourceLoader.cpp:182: KURL url =
request.resourceRequest().url();
On 2013/06/05 23:42:51, abarth wrote:
> const KURL&

Done.

https://codereview.chromium.org/16433002/diff/9002/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/16433002/diff/9002/public/platform/Platform.h...
public/platform/Platform.h:290: // Returns true if the data url was of a
supported mimetype and was parsed successfully.
On 2013/06/05 23:42:51, abarth wrote:
> "Returns true" <--- not accurate anymore

Done.

[pdr., 2013-06-05 23:56:22]

On 2013/06/05 23:52:37, Nate Chapin wrote:
> On 2013/06/05 23:50:54, Nate Chapin wrote:
> >
>
https://codereview.chromium.org/16433002/diff/9002/Source/core/loader/cache/C...
> > File Source/core/loader/cache/CachedResourceLoader.cpp (right):
> > 
> >
>
https://codereview.chromium.org/16433002/diff/9002/Source/core/loader/cache/C...
> > Source/core/loader/cache/CachedResourceLoader.cpp:174:
> > preCacheDataURIImage(request);
> > Should this early return? Otherwise, won't we run the request normally in
> > addition?
> 
> Wow, I totally flaked with that question.
> 
> More correctly: Is there any case in which we won't use the cached version
when
> requestResource() finds it?

That's a good question. A quick scan says we will not, but I think we should
still fall through the CachedResourceLoader::requestResource logic as there are
logging calls.

[pdr., 2013-06-06 18:54:27]

On 2013/06/05 23:56:22, pdr wrote:
> On 2013/06/05 23:52:37, Nate Chapin wrote:
> > On 2013/06/05 23:50:54, Nate Chapin wrote:
> > >
> >
>
https://codereview.chromium.org/16433002/diff/9002/Source/core/loader/cache/C...
> > > File Source/core/loader/cache/CachedResourceLoader.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/16433002/diff/9002/Source/core/loader/cache/C...
> > > Source/core/loader/cache/CachedResourceLoader.cpp:174:
> > > preCacheDataURIImage(request);
> > > Should this early return? Otherwise, won't we run the request normally in
> > > addition?
> > 
> > Wow, I totally flaked with that question.
> > 
> > More correctly: Is there any case in which we won't use the cached version
> when
> > requestResource() finds it?
> 
> That's a good question. A quick scan says we will not, but I think we should
> still fall through the CachedResourceLoader::requestResource logic as there
are
> logging calls.

Expanding on this a bit: MemoryCache::removeFragmentIdentifierIfNeeded doesn't
affect data uris, and the isValid check is redundant with trying to parse the
content.

[abarth-chromium, 2013-06-07 20:36:11]

lgtm

https://codereview.chromium.org/16433002/diff/14001/Source/core/loader/cache/...
File Source/core/loader/cache/CachedResourceLoader.cpp (right):

https://codereview.chromium.org/16433002/diff/14001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.cpp:115: CachedResource* resource
= createResource(CachedResource::ImageResource, request, charset);
We'll eventually want to expand this beyond just images, but this is a good
start.

https://codereview.chromium.org/16433002/diff/14001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.cpp:117: // FIXME: AppendData
caises an unnecessary memcpy.
caises -> causes

https://codereview.chromium.org/16433002/diff/14001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.cpp:189:
memoryCache()->remove(existing); // Cache entry is incorrect.
Is there a test for this case?

https://codereview.chromium.org/16433002/diff/14001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.cpp:192: if (CachedResource*
resource = resourceFromDataURIRequest(request.mutableResourceRequest()))
Why does this need to be mutable?  It doesn't seem like you're going to alter
the resource request.

[pdr., 2013-06-07 22:00:23]

PTAL

https://codereview.chromium.org/16433002/diff/14001/Source/core/loader/cache/...
File Source/core/loader/cache/CachedResourceLoader.cpp (right):

https://codereview.chromium.org/16433002/diff/14001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.cpp:117: // FIXME: AppendData
caises an unnecessary memcpy.
On 2013/06/07 20:36:11, abarth wrote:
> caises -> causes

Done.

https://codereview.chromium.org/16433002/diff/14001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.cpp:189:
memoryCache()->remove(existing); // Cache entry is incorrect.
On 2013/06/07 20:36:11, abarth wrote:
> Is there a test for this case?

I don't think a test can hit this codepath. I tried a few ways such as loading
an SVG image as a font or in an iframe and we end up not caching it, or properly
caching it.

I've replaced the "memoryCache()->remove(existing);" line with an ASSERT.

https://codereview.chromium.org/16433002/diff/14001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.cpp:192: if (CachedResource*
resource = resourceFromDataURIRequest(request.mutableResourceRequest()))
On 2013/06/07 20:36:11, abarth wrote:
> Why does this need to be mutable?  It doesn't seem like you're going to alter
> the resource request.

Nope! Required some minor fixups to make this true though (CachedRawResource,
etc.)

[abarth-chromium, 2013-06-07 22:16:07]

https://codereview.chromium.org/16433002/diff/24001/Source/core/loader/cache/...
File Source/core/loader/cache/CachedResourceLoader.cpp (right):

https://codereview.chromium.org/16433002/diff/24001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.cpp:214: void
CachedResourceLoader::preCacheDataURIImage(CachedResourceRequest& request)
While we're in the "const" business, we might want to add one to "const
CachedResourceRequest&"  :)

https://codereview.chromium.org/16433002/diff/24001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.cpp:222: return;
Wait, why don't you need to remove the existing resource?

There's no reason why the existing resource needs to be of type
CachedResource::ImageResource.  He code you had before was correct.

As a more general note, we try to avoid ASSERTing a condition and then handling
the negation.  Either the condition can fail, in which case the ASSERT is wrong,
or the condition cannot fail, in which case there's no point in handling the
negation.

[pdr., 2013-06-07 22:27:24]

PTOML :)

On 2013/06/07 22:16:07, abarth wrote:
>
https://codereview.chromium.org/16433002/diff/24001/Source/core/loader/cache/...
> File Source/core/loader/cache/CachedResourceLoader.cpp (right):
> 
>
https://codereview.chromium.org/16433002/diff/24001/Source/core/loader/cache/...
> Source/core/loader/cache/CachedResourceLoader.cpp:214: void
> CachedResourceLoader::preCacheDataURIImage(CachedResourceRequest& request)
> While we're in the "const" business, we might want to add one to "const
> CachedResourceRequest&"  :)

Done. I tried to go even further but requestImage does need to modify the
request.

> 
>
https://codereview.chromium.org/16433002/diff/24001/Source/core/loader/cache/...
> Source/core/loader/cache/CachedResourceLoader.cpp:222: return;
> Wait, why don't you need to remove the existing resource?
> 
> There's no reason why the existing resource needs to be of type
> CachedResource::ImageResource.  He code you had before was correct.
> 
> As a more general note, we try to avoid ASSERTing a condition and then
handling
> the negation.  Either the condition can fail, in which case the ASSERT is
wrong,
> or the condition cannot fail, in which case there's no point in handling the
> negation.

I've returned the code to how it was before. I don't think we can write a
testcase that hits that codepath though.

[abarth-chromium, 2013-06-07 22:40:02]

Ok.  LGTM

I still think it's possible we can hit that line of code, but we can talk more
about it on Monday.

[commit-bot: I haz the power, 2013-06-07 22:41:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pdr@chromium.org/16433002/26001

[commit-bot: I haz the power, 2013-06-07 22:41:12]

Can't process patch for file
LayoutTests/svg/as-image/resources/image-with-svg-data-uri.svg.
Binary file is empty. Maybe the file wasn't uploaded in the first place?

[pdr., 2013-06-07 23:03:25]

Committed patchset #7 manually as r152058 (presubmit successful).

[pdr., 2013-06-07 23:05:00]

On 2013/06/07 23:03:25, pdr wrote:
> Committed patchset #7 manually as r152058 (presubmit successful).

CQ does not like binary files.
I ran the tests locally and dcommitted.

If you're thinking of rolling this patch out, stop thinking and roll it out :)

[pdr., 2013-06-09 05:23:07]

PTAL

[abarth-chromium, 2013-06-09 05:57:38]

Ok, LGTM.  There's some risk that this will break sites, but those sites are
already flaky depending on whether the data URL happens to be cached already.

https://codereview.chromium.org/16433002/diff/36001/Source/core/loader/cache/...
File Source/core/loader/cache/CachedResourceLoader.cpp (right):

https://codereview.chromium.org/16433002/diff/36001/Source/core/loader/cache/...
Source/core/loader/cache/CachedResourceLoader.cpp:220: return;
Ok, this variation makes sense too.

[abarth-chromium, 2013-06-09 05:58:14]

https://codereview.chromium.org/16433002/diff/36001/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/16433002/diff/36001/LayoutTests/TestExpectati...
LayoutTests/TestExpectations:853: # webkit.org/b/48454 [ Win Debug ]
virtual/softwarecompositing/tiling/huge-layer-img.html [ Slow ]
Why comment out this line?  Maybe just delete it?

[pdr., 2013-06-09 22:13:12]

On 2013/06/09 05:58:14, abarth wrote:
>
https://codereview.chromium.org/16433002/diff/36001/LayoutTests/TestExpectations
> File LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/16433002/diff/36001/LayoutTests/TestExpectati...
> LayoutTests/TestExpectations:853: # webkit.org/b/48454 [ Win Debug ]
> virtual/softwarecompositing/tiling/huge-layer-img.html [ Slow ]
> Why comment out this line?  Maybe just delete it?

Thank you for the quick review Adam!

I think the test is likely to be genuinely slow since both Skia and Blink have
lots of debug checks in their drawing routines. Once I rebase, I'll uncomment
it.

I'm going to go ahead and dcommit since it's the weekend and, hopefully, few
people will be affected if it has to be rolled out again.

[pdr., 2013-06-09 22:16:28]

Committed patchset #9 manually as r152093 (presubmit successful).

[tyoshino (SeeGerritForStatus), 2013-06-10 11:37:26]

http://build.chromium.org/p/chromium.webkit/builders/Android%20Tests%20%28dbg...

Android Tests bot is failing constantly since this CL. Please take a look at the
log.