Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1707013002:
[WIP][SVG 3/4] Allow invalid data: URLs to be served synchronously by Resource::load()
|
Created:
4 years, 10 months ago by hiroshige Modified:
3 years, 8 months ago CC:
chromium-reviews, tyoshino+watch_chromium.org, Yoav Weiss, gavinp+loader_chromium.org, blink-reviews, loading-reviews+fetch_chromium.org, Nate Chapin Base URL:
https://chromium.googlesource.com/chromium/src.git@Loader_SVGImage_Fix2 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow invalid data: URLs to be served synchronously by Resource::load()
This CL allows Resoure::load() to complete loading of invalid data: URLs
synchronously.
This is to ensure all data: font URLs to be served synchronously by
Resource::load() for Issue 382170, including invalid data: URLs.
This CL is [3/4] to load FontResource of data: URLs synchronously so that
all subresource loading of SVG image completes before loading of the SVG file.
BUG=382170
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : Fix unit test errors. #Patch Set 5 : #
Total comments: 2
Patch Set 6 : Rebase + reflect comments. #
Total comments: 1
Patch Set 7 : Rebase. #Patch Set 8 : auto-Rebase #Patch Set 9 : auto-Rebase #Messages
Total messages: 36 (19 generated)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1707013002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1707013002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== [WIP] precache data URLs in case of error BUG=382170 ========== to ========== [WIP] precache data URLs in case of error BUG=382170 ==========
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1707013002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1707013002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
The CQ bit was unchecked by hiroshige@chromium.org
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1707013002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1707013002/60001
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1707013002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1707013002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from
==========
[WIP] precache data URLs in case of error
BUG=382170
==========
to
==========
Allow invalid data: URLs to be served synchronously by ResourceFetcher
This CL allows ResourceFetcher::resourceForStaticData() to create and return
Resource (with error) for invalid data: URLs and thus serve it synchronously.
This is to ensure all data: font URLs to be served synchronously by
ResourceFetcher for Issue 382170, including invalid data: URLs.
This CL is [3/5] to load FontResource of data: URLs synchronously so that
all subresource loading of SVG image completes before loading of the SVG file:
[1/5] Make fonts with data: URLs to be served by resourceForStaticData().
https://codereview.chromium.org/1700233003/
[2/5] Do not reload FontResource created by resourceForStaticData().
https://codereview.chromium.org/1704693002/
[3/5] Make invalid data: URLs to be served by resourceForStaticData().
https://codereview.chromium.org/1707013002/ (This CL)
[4/5] Add layout tests.
https://codereview.chromium.org/1699323002/
[5/5] Check SVG's subresource load completion in ImageLoader::notifyFinished().
https://codereview.chromium.org/1515963005/
BUG=382170
==========
hiroshige@chromium.org changed reviewers: + japhet@chromium.org, yhirano@chromium.org
PTAL.
https://codereview.chromium.org/1707013002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/1707013002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:346: resource->setOptions(request.options()); Can we do the this and the changes above it earlier in the function, so that we don't have to duplicate it?
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1707013002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1707013002/100001
https://codereview.chromium.org/1707013002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/1707013002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:346: resource->setOptions(request.options()); On 2016/03/01 19:05:06, Nate Chapin wrote: > Can we do the this and the changes above it earlier in the function, so that we > don't have to duplicate it? Done.
LGTM https://codereview.chromium.org/1707013002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/1707013002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:352: resource->error(Resource::LoadError); ASSERT(url.protocolIsData()) here, just to be clear that SubstituteData loads shouldn't error like this?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1707013002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1707013002/120001
Description was changed from
==========
Allow invalid data: URLs to be served synchronously by ResourceFetcher
This CL allows ResourceFetcher::resourceForStaticData() to create and return
Resource (with error) for invalid data: URLs and thus serve it synchronously.
This is to ensure all data: font URLs to be served synchronously by
ResourceFetcher for Issue 382170, including invalid data: URLs.
This CL is [3/5] to load FontResource of data: URLs synchronously so that
all subresource loading of SVG image completes before loading of the SVG file:
[1/5] Make fonts with data: URLs to be served by resourceForStaticData().
https://codereview.chromium.org/1700233003/
[2/5] Do not reload FontResource created by resourceForStaticData().
https://codereview.chromium.org/1704693002/
[3/5] Make invalid data: URLs to be served by resourceForStaticData().
https://codereview.chromium.org/1707013002/ (This CL)
[4/5] Add layout tests.
https://codereview.chromium.org/1699323002/
[5/5] Check SVG's subresource load completion in ImageLoader::notifyFinished().
https://codereview.chromium.org/1515963005/
BUG=382170
==========
to
==========
Allow invalid data: URLs to be served synchronously by ResourceFetcher
This CL allows ResourceFetcher::resourceForStaticData() to create and return
Resource (with error) for invalid data: URLs and thus serve it synchronously.
This is to ensure all data: font URLs to be served synchronously by
ResourceFetcher for Issue 382170, including invalid data: URLs.
This CL is [3/4] to load FontResource of data: URLs synchronously so that
all subresource loading of SVG image completes before loading of the SVG file.
BUG=382170
==========
Description was changed from ========== Allow invalid data: URLs to be served synchronously by ResourceFetcher This CL allows ResourceFetcher::resourceForStaticData() to create and return Resource (with error) for invalid data: URLs and thus serve it synchronously. This is to ensure all data: font URLs to be served synchronously by ResourceFetcher for Issue 382170, including invalid data: URLs. This CL is [3/4] to load FontResource of data: URLs synchronously so that all subresource loading of SVG image completes before loading of the SVG file. BUG=382170 ========== to ========== Allow invalid data: URLs to be served synchronously by Resource::load() This CL allows Resoure::load() to complete loading of invalid data: URLs synchronously. This is to ensure all data: font URLs to be served synchronously by Resource::load() for Issue 382170, including invalid data: URLs. This CL is [3/4] to load FontResource of data: URLs synchronously so that all subresource loading of SVG image completes before loading of the SVG file. BUG=382170 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
yhirano@chromium.org changed reviewers: - yhirano@chromium.org
(Removing myself from the reviewers list.) |
