Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(53)

Issue 16433002: Prepopulate the memoryCache with data:uri images. (Closed)

Created:
6 years, 2 months ago by pdr.
Modified:
6 years, 2 months ago
CC:
blink-reviews, eae+blinkwatch, gavinp+loader_chromium.org
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Update for review #

Patch Set 3 : Fix LayoutTest crash due to empty data uri #

Total comments: 10

Patch Set 4 : Address reviewer comments #

Total comments: 7

Patch Set 5 : Update per reviewer comments #

Patch Set 6 : Assert for wrong cache type #

Total comments: 2

Patch Set 7 : Constification #

Patch Set 8 : Update after rollout due to several test failures #

Patch Set 9 : Update TestExpectations #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+180 lines, -90 lines) Patch
M LayoutTests/TestExpectations View 1 2 3 4 5 6 7 8 3 chunks +7 lines, -2 lines 1 comment Download
D LayoutTests/fast/backgrounds/solid-color-context-restore.html View 1 2 3 4 5 6 7 1 chunk +0 lines, -23 lines 0 comments Download
D LayoutTests/fast/backgrounds/solid-color-context-restore-expected.txt View 1 2 3 4 5 6 7 1 chunk +0 lines, -15 lines 0 comments Download
D LayoutTests/http/tests/inspector/network/image-as-text-loading-data-url.html View 1 2 3 4 5 6 7 1 chunk +0 lines, -30 lines 0 comments Download
A LayoutTests/http/tests/inspector/network/script-as-text-loading-data-url.html View 1 2 3 4 5 6 7 1 chunk +30 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/inspector/network/script-as-text-loading-data-url-expected.txt View 1 2 3 4 5 6 7 1 chunk +5 lines, -0 lines 0 comments Download
A LayoutTests/loader/data-uri-images-load-synchronously.html View 1 2 3 4 5 6 7 1 chunk +18 lines, -0 lines 0 comments Download
A LayoutTests/loader/data-uri-images-load-synchronously-expected.txt View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
A LayoutTests/loader/data-uri-images-reload-synchronously.html View 1 2 3 4 5 6 7 1 chunk +28 lines, -0 lines 0 comments Download
A LayoutTests/loader/data-uri-images-reload-synchronously-expected.txt View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
D LayoutTests/platform/chromium-android/fast/backgrounds/solid-color-context-restore-expected.png View 1 2 3 4 5 6 7 Binary file 0 comments Download
D LayoutTests/platform/chromium-linux/fast/backgrounds/solid-color-context-restore-expected.png View 1 2 3 4 5 6 7 Binary file 0 comments Download
D LayoutTests/platform/chromium-mac-lion/fast/backgrounds/solid-color-context-restore-expected.png View 1 2 3 4 5 6 7 Binary file 0 comments Download
D LayoutTests/platform/chromium-mac-snowleopard/fast/backgrounds/solid-color-context-restore-expected.png View 1 2 3 4 5 6 7 Binary file 0 comments Download
D LayoutTests/platform/chromium-mac/fast/backgrounds/solid-color-context-restore-expected.png View 1 2 3 4 5 6 7 Binary file 0 comments Download
D LayoutTests/platform/chromium-win/fast/backgrounds/solid-color-context-restore-expected.png View 1 2 3 4 5 6 7 Binary file 0 comments Download
D LayoutTests/platform/chromium-win/fast/backgrounds/solid-color-context-restore-expected.txt View 1 2 3 4 5 6 7 1 chunk +0 lines, -15 lines 0 comments Download
A LayoutTests/svg/as-image/resources/image-with-data-uri.svg View 1 Binary file 0 comments Download
A LayoutTests/svg/as-image/resources/image-with-svg-data-uri.svg View 1 Binary file 0 comments Download
A LayoutTests/svg/as-image/svg-image-with-data-uri.html View 1 chunk +8 lines, -0 lines 0 comments Download
A LayoutTests/svg/as-image/svg-image-with-data-uri-expected.html View 1 chunk +10 lines, -0 lines 0 comments Download
A LayoutTests/svg/as-image/svg-image-with-nested-data-uri.html View 1 chunk +8 lines, -0 lines 0 comments Download
A LayoutTests/svg/as-image/svg-image-with-nested-data-uri-expected.html View 1 chunk +10 lines, -0 lines 0 comments Download
M Source/core/loader/cache/CachedRawResource.h View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M Source/core/loader/cache/CachedRawResource.cpp View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M Source/core/loader/cache/CachedResourceLoader.h View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/loader/cache/CachedResourceLoader.cpp View 1 2 3 4 5 6 7 5 chunks +46 lines, -3 lines 1 comment Download
M public/platform/Platform.h View 1 2 3 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 22 (0 generated)
pdr.
6 years, 2 months ago (2013-06-05 23:32:11 UTC) #1
abarth-chromium
Looks great. Some minor nits below. japhet should also look at this CL. https://codereview.chromium.org/16433002/diff/9002/Source/core/loader/cache/CachedResourceLoader.cpp File ...
6 years, 2 months ago (2013-06-05 23:42:51 UTC) #2
Nate Chapin
https://codereview.chromium.org/16433002/diff/9002/Source/core/loader/cache/CachedResourceLoader.cpp File Source/core/loader/cache/CachedResourceLoader.cpp (right): https://codereview.chromium.org/16433002/diff/9002/Source/core/loader/cache/CachedResourceLoader.cpp#newcode174 Source/core/loader/cache/CachedResourceLoader.cpp:174: preCacheDataURIImage(request); Should this early return? Otherwise, won't we run ...
6 years, 2 months ago (2013-06-05 23:50:54 UTC) #3
Nate Chapin
On 2013/06/05 23:50:54, Nate Chapin wrote: > https://codereview.chromium.org/16433002/diff/9002/Source/core/loader/cache/CachedResourceLoader.cpp > File Source/core/loader/cache/CachedResourceLoader.cpp (right): > > https://codereview.chromium.org/16433002/diff/9002/Source/core/loader/cache/CachedResourceLoader.cpp#newcode174 ...
6 years, 2 months ago (2013-06-05 23:52:37 UTC) #4
pdr.
https://codereview.chromium.org/16433002/diff/9002/Source/core/loader/cache/CachedResourceLoader.cpp File Source/core/loader/cache/CachedResourceLoader.cpp (right): https://codereview.chromium.org/16433002/diff/9002/Source/core/loader/cache/CachedResourceLoader.cpp#newcode105 Source/core/loader/cache/CachedResourceLoader.cpp:105: KURL url = request.url(); On 2013/06/05 23:42:51, abarth wrote: ...
6 years, 2 months ago (2013-06-05 23:53:25 UTC) #5
pdr.
On 2013/06/05 23:52:37, Nate Chapin wrote: > On 2013/06/05 23:50:54, Nate Chapin wrote: > > ...
6 years, 2 months ago (2013-06-05 23:56:22 UTC) #6
pdr.
On 2013/06/05 23:56:22, pdr wrote: > On 2013/06/05 23:52:37, Nate Chapin wrote: > > On ...
6 years, 2 months ago (2013-06-06 18:54:27 UTC) #7
abarth-chromium
lgtm https://codereview.chromium.org/16433002/diff/14001/Source/core/loader/cache/CachedResourceLoader.cpp File Source/core/loader/cache/CachedResourceLoader.cpp (right): https://codereview.chromium.org/16433002/diff/14001/Source/core/loader/cache/CachedResourceLoader.cpp#newcode115 Source/core/loader/cache/CachedResourceLoader.cpp:115: CachedResource* resource = createResource(CachedResource::ImageResource, request, charset); We'll eventually ...
6 years, 2 months ago (2013-06-07 20:36:11 UTC) #8
pdr.
PTAL https://codereview.chromium.org/16433002/diff/14001/Source/core/loader/cache/CachedResourceLoader.cpp File Source/core/loader/cache/CachedResourceLoader.cpp (right): https://codereview.chromium.org/16433002/diff/14001/Source/core/loader/cache/CachedResourceLoader.cpp#newcode117 Source/core/loader/cache/CachedResourceLoader.cpp:117: // FIXME: AppendData caises an unnecessary memcpy. On ...
6 years, 2 months ago (2013-06-07 22:00:23 UTC) #9
abarth-chromium
https://codereview.chromium.org/16433002/diff/24001/Source/core/loader/cache/CachedResourceLoader.cpp File Source/core/loader/cache/CachedResourceLoader.cpp (right): https://codereview.chromium.org/16433002/diff/24001/Source/core/loader/cache/CachedResourceLoader.cpp#newcode214 Source/core/loader/cache/CachedResourceLoader.cpp:214: void CachedResourceLoader::preCacheDataURIImage(CachedResourceRequest& request) While we're in the "const" business, ...
6 years, 2 months ago (2013-06-07 22:16:07 UTC) #10
pdr.
PTOML :) On 2013/06/07 22:16:07, abarth wrote: > https://codereview.chromium.org/16433002/diff/24001/Source/core/loader/cache/CachedResourceLoader.cpp > File Source/core/loader/cache/CachedResourceLoader.cpp (right): > > ...
6 years, 2 months ago (2013-06-07 22:27:24 UTC) #11
abarth-chromium
Ok. LGTM I still think it's possible we can hit that line of code, but ...
6 years, 2 months ago (2013-06-07 22:40:02 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pdr@chromium.org/16433002/26001
6 years, 2 months ago (2013-06-07 22:41:11 UTC) #13
commit-bot: I haz the power
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 ...
6 years, 2 months ago (2013-06-07 22:41:12 UTC) #14
pdr.
Committed patchset #7 manually as r152058 (presubmit successful).
6 years, 2 months ago (2013-06-07 23:03:25 UTC) #15
pdr.
On 2013/06/07 23:03:25, pdr wrote: > Committed patchset #7 manually as r152058 (presubmit successful). CQ ...
6 years, 2 months ago (2013-06-07 23:05:00 UTC) #16
pdr.
PTAL
6 years, 2 months ago (2013-06-09 05:23:07 UTC) #17
abarth-chromium
Ok, LGTM. There's some risk that this will break sites, but those sites are already ...
6 years, 2 months ago (2013-06-09 05:57:38 UTC) #18
abarth-chromium
https://codereview.chromium.org/16433002/diff/36001/LayoutTests/TestExpectations File LayoutTests/TestExpectations (right): https://codereview.chromium.org/16433002/diff/36001/LayoutTests/TestExpectations#newcode853 LayoutTests/TestExpectations:853: # webkit.org/b/48454 [ Win Debug ] virtual/softwarecompositing/tiling/huge-layer-img.html [ Slow ...
6 years, 2 months ago (2013-06-09 05:58:14 UTC) #19
pdr.
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/TestExpectations#newcode853 > ...
6 years, 2 months ago (2013-06-09 22:13:12 UTC) #20
pdr.
Committed patchset #9 manually as r152093 (presubmit successful).
6 years, 2 months ago (2013-06-09 22:16:28 UTC) #21
tyoshino (SeeGerritForStatus)
6 years, 2 months ago (2013-06-10 11:37:26 UTC) #22
Message was sent while issue was closed.
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.

Powered by Google App Engine
This is Rietveld 408576698