Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2390583002:
[WIP] WebFonts cache-aware timeout adaption (Closed)
|
Created:
4 years, 2 months ago by Shao-Chuan Lee Modified:
4 years, 1 month ago Reviewers:
hiroshige, Takashi Toyoshima, Kunihiko Sakamoto, tkent, yhirano, haraken, kouhei (in TOK) CC:
chromium-reviews, tyoshino+watch_chromium.org, blink-reviews-css, Yoav Weiss, dglazkov+blink, apavlov+blink_chromium.org, gavinp+loader_chromium.org, darktears, blink-reviews, loading-reviews+fetch_chromium.org, Nate Chapin, rwlbuis, kinuko Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[WIP] WebFonts cache-aware timeout adaption
This CL introduces cache-aware Resource loading and adaptive webfont display
behavior, reducing unnecessary Flash of Unstyled Text (FOUT) if webfont is
already available in disk cache.
In cache-aware loading, the Resource is notified of disk cache miss from network
stack during loading. This is implemented by sending a request to load only from
disk cache, and retry with original request if the first one fails.
Resource::willReloadAfterDiskCacheMiss() is called before the retry. If the
cached response is a redirect, we fail the request as cache miss.
In webfont display, fallback font will be used if webfont loading time exceeds
certain timeout values in slow network. However it's observed that disk cache
RTT may also hit the timeout regardless of network speed. In such cases we would
like to block display until actual network request is being sent.
Now FetchRequest provides an option to enable cache-aware loading for
Resource::fetch(). If supplied cache policy doesn't conflict, cache-aware
loading will be activated for new Resource instances (revalidation is not
supported).
Note: This CL depends on https://crrev.com/2398613002, we are now using
ReturnCacheDataDontLoad which may return stale data from disk cache.
BUG=570205
Patch Set 1 #
Total comments: 28
Patch Set 2 : fix comments, add unit tests #Patch Set 3 : remove flag in finish() instead of responseReceived() #
Total comments: 14
Patch Set 4 : move callback impl to FontResource, remove unittests for now, fix #
Total comments: 6
Patch Set 5 : see comment #23 #Patch Set 6 : RemoteFontFaceSource cache-aware behavior, fix #
Total comments: 2
Patch Set 7 : revise RemoteFontFaceSource cache-aware logic, fix #
Total comments: 1
Patch Set 8 : move logic to FontResource #Patch Set 9 : handle font-display: swap #
Total comments: 9
Patch Set 10 : rebase, fix #Patch Set 11 : comments #Patch Set 12 : fix error generation in ResourceLoader::willFollowRedirect, init new field in WebURLError ctor, Res… #Patch Set 13 : move mayActivateCacheAwareLoading() back into ResourceRequest to reuse checks, comments #Patch Set 14 : handle case in RemoteFontFaceSource if cache-aware deactivated in startLoad(), rebase #
Total comments: 16
Patch Set 15 : fix #52 #
Total comments: 2
Patch Set 16 : remove isCacheMiss field in WebURLError/ResourceError #Patch Set 17 : switchToSwapPeriod(), Inspector disable cache check workaround #
Total comments: 5
Patch Set 18 : rebase #Patch Set 19 : check ERR_CACHE_MISS directly in ResourceError #
Total comments: 2
Messages
Total messages: 75 (11 generated)
This is the tentative implementation based on the design notes (https://docs.google.com/a/chromium.org/document/d/1vaGzhJyfR6ozSiMfphdOrwrca5...) and previous offline discussions, PTAL. Changes to RemoteFontFaceSource are not yet included.
The direction basically looks good, but there might be corner cases to consider. What (should) occur if: 1. Cache-only request to URL A 2. -> Disk cache returns redirect to URL B 3. -> URL B is not on disk cache 4. didFailLoading() is called ? We might have to distinguish such cases (where we can resend the request to network) from: 1. Cache-only request to URL A 2. -> Disk cache returns redirect to URL B 3. -> the redirect is rejected by e.g. CSP 4. didFailLoading() is called Also, exposing the willReloadAfterDiskCacheMiss() looks dangerous (especially for the current RawResources), so making it a FontResourceClient member might be better. https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1112: // Assume error.errorCode() == net::ERR_CACHE_MISS, resend request with - We have to drop the isCacheAwareLoadingActivated() flag once the first (cache-only) request is succeeded (i.e. received response or redirect) or failed (i.e. here). - We have to check whether this failure is really due to disk cache miss, because ResourceFetcher::didFailLoading() can be called by e.g. ResourceLoader::cancel(). Restarting request for non-disk-cache-miss failures increases unintended network requests.
https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.cpp:1083: m_isAddClientProhibited = true; AutoReset https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/AutoReset.... https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/Resource.h (right): https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.h:292: // TODO(632580): Workaround to persist cache-aware state, remove after fixed. hiroshige-san: Would you take a look?
https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/Resource.h (right): https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.h:292: // TODO(632580): Workaround to persist cache-aware state, remove after fixed. On 2016/10/03 08:11:47, kouhei wrote: > hiroshige-san: Would you take a look? Nate is working on the issue: https://codereview.chromium.org/2203613003/
https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp (right): https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp:91: request.mutableResourceRequest().setIsCacheAwareLoadingEnabled(true); I think we should do cache-aware loading only for @font-faces with font-display: (swap | fallback | optional). See the table in Toyoshima-san's doc: https://docs.google.com/document/d/1SKRVFza_USrdVZgJmPnKWmqbzj-kVoxMB2ECH_xLh...
On 2016/10/03 08:08:02, hiroshige wrote: > The direction basically looks good, but there might be corner cases to consider. > > What (should) occur if: > 1. Cache-only request to URL A > 2. -> Disk cache returns redirect to URL B > 3. -> URL B is not on disk cache > 4. didFailLoading() is called > ? > > We might have to distinguish such cases (where we can resend the request to > network) from: > 1. Cache-only request to URL A > 2. -> Disk cache returns redirect to URL B > 3. -> the redirect is rejected by e.g. CSP > 4. didFailLoading() is called I guess checking error.isAccessCheck() here should suffice? I've seen usages in ImageLoader. > > Also, exposing the willReloadAfterDiskCacheMiss() looks dangerous (especially > for the current RawResources), so making it a FontResourceClient member might be > better. Do you mean implementing only in FontResource/Client and check if Resource is FontResource at runtime? > > https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): > > https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1112: // Assume > error.errorCode() == net::ERR_CACHE_MISS, resend request with > - We have to drop the isCacheAwareLoadingActivated() flag once the first > (cache-only) request is succeeded (i.e. received response or redirect) or failed > (i.e. here). The flag is currently dropped in Resource::willReloadAfterDiskCacheMiss(), is it better to move it here? > - We have to check whether this failure is really due to disk cache miss, > because ResourceFetcher::didFailLoading() can be called by e.g. > ResourceLoader::cancel(). Restarting request for non-disk-cache-miss failures > increases unintended network requests. Adding check on error.isCancellation() for this.
On 2016/10/03 08:31:38, Kunihiko Sakamoto wrote: > https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp (right): > > https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp:91: > request.mutableResourceRequest().setIsCacheAwareLoadingEnabled(true); > I think we should do cache-aware loading only for @font-faces with font-display: > (swap | fallback | optional). > See the table in Toyoshima-san's doc: > https://docs.google.com/document/d/1SKRVFza_USrdVZgJmPnKWmqbzj-kVoxMB2ECH_xLh... OK, modifying FontFace::initCSSFontFace() to consider font-display option.
https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp (right): https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp:91: request.mutableResourceRequest().setIsCacheAwareLoadingEnabled(true); Since it will introduce additional complexity, we would use cache-aware loading always for WebFonts. This decision is based on data that requests rarely miss the cache and disk check is reasonable fast compared to network access in the case of cache miss. But, probably we should make this feature behind a flag. shao, can you add a new Runtime flag in Source/platform/RuntimeEnabledFeatures.in, and set it only when the runtime flag is set? Probably, it should contain status=experimental so that it is enabled in tests or users who enable chrome://flags/#enable-experimental-web-platform-features
https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/network/ResourceRequest.cpp (right): https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/network/ResourceRequest.cpp:400: if (m_cachePolicy == WebCachePolicy::BypassingCache || Using switch is better to catch new cache policy being added? We would handle the new policy in a right way when added. https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/network/ResourceRequest.cpp:405: m_cachePolicy = WebCachePolicy::ReturnCacheDataDontLoad; Should we have a TODO to use the right flag when //net is ready? Also it worth noting that this flag may return stale data until then.
Description was changed from ========== WIP: Cache-aware FontResource loading In cache-aware loading, we attempt to load the resource from disk cache by setting WebCachePolicy to ReturnCacheDataDontLoad. If the load failed, ResourceClients are notified by callback, and the request is resent with original cache policy. Here we enable cache-aware loading for FontResources by default. Note: ReturnCacheDataDontLoad maps to net::LOAD_ONLY_FROM_CACHE which may return stale data, changes in net are required to support loading from disk cache with validation. ========== to ========== WIP: WebFonts cache-aware timeout adaption In cache-aware loading, we attempt to load the resource from disk cache by setting WebCachePolicy to ReturnCacheDataDontLoad. If the load failed, ResourceClients are notified by callback, and the request is resent with original cache policy. Here we enable cache-aware loading for FontResources by default. Note: ReturnCacheDataDontLoad maps to net::LOAD_ONLY_FROM_CACHE which may return stale data, changes in net are required to support loading from disk cache with validation. BUG=570205 ==========
Description was changed from ========== WIP: WebFonts cache-aware timeout adaption In cache-aware loading, we attempt to load the resource from disk cache by setting WebCachePolicy to ReturnCacheDataDontLoad. If the load failed, ResourceClients are notified by callback, and the request is resent with original cache policy. Here we enable cache-aware loading for FontResources by default. Note: ReturnCacheDataDontLoad maps to net::LOAD_ONLY_FROM_CACHE which may return stale data, changes in net are required to support loading from disk cache with validation. BUG=570205 ========== to ========== WIP: Cache-aware FontResource loading In cache-aware loading, we attempt to load the resource from disk cache by setting WebCachePolicy to ReturnCacheDataDontLoad. If the load failed, ResourceClients are notified by callback, and the request is resent with original cache policy. Here we enable cache-aware loading for FontResources by default. Note: ReturnCacheDataDontLoad maps to net::LOAD_ONLY_FROM_CACHE which may return stale data, changes in net are required to support loading from disk cache with validation. BUG=570205 ==========
https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.cpp:1083: m_isAddClientProhibited = true; On 2016/10/03 08:11:47, kouhei wrote: > AutoReset > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/AutoReset.... I made this a bit-field like other fields in Resource, and the address cannot be taken for AutoReset. Maybe make this a debug-only field if there are memory usage concerns? https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/network/ResourceRequest.cpp (right): https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/network/ResourceRequest.cpp:405: m_cachePolicy = WebCachePolicy::ReturnCacheDataDontLoad; On 2016/10/03 09:06:35, toyoshim wrote: > Should we have a TODO to use the right flag when //net is ready? > Also it worth noting that this flag may return stale data until then. Do we already have a bug for this? Or we should file one for tracking progress.
https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/network/ResourceRequest.cpp (right): https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/network/ResourceRequest.cpp:405: m_cachePolicy = WebCachePolicy::ReturnCacheDataDontLoad; How about using crbug.com/570205 for this TODO.
Shouldn't we dispatch willReloadAfterDiskCacheMiss when - A resusable Resource X is found, - X is loading, and - cache-aware-loading is disabled or deactivated on X ? https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.cpp:1081: m_resourceRequest.deactivateCacheAwareLoading(); DCHECK(!m_isAddClientProhibited); https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/Resource.h (right): https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.h:290: virtual void willReloadAfterDiskCacheMiss(); Please add a comment that add (/ remove?)-ing clients is forbidden in this function. https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.h:420: bool m_isAddClientProhibited : 1; Is it a good idea to prohibit removeClient as well? https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1112: // Assume error.errorCode() == net::ERR_CACHE_MISS, resend request with On 2016/10/03 08:08:02, hiroshige wrote: > - We have to drop the isCacheAwareLoadingActivated() flag once the first > (cache-only) request is succeeded (i.e. received response or redirect) or failed > (i.e. here). > - We have to check whether this failure is really due to disk cache miss, > because ResourceFetcher::didFailLoading() can be called by e.g. > ResourceLoader::cancel(). Restarting request for non-disk-cache-miss failures > increases unintended network requests. But we don't need the former if we have the latter, right?
https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp (right): https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp:91: request.mutableResourceRequest().setIsCacheAwareLoadingEnabled(true); On 2016/10/03 09:01:04, toyoshim wrote: > Since it will introduce additional complexity, we would use cache-aware loading > always for WebFonts. This decision is based on data that requests rarely miss > the cache and disk check is reasonable fast compared to network access in the > case of cache miss. > > But, probably we should make this feature behind a flag. > > shao, can you add a new Runtime flag in > Source/platform/RuntimeEnabledFeatures.in, and set it only when the runtime flag > is set? > Probably, it should contain status=experimental so that it is enabled in tests > or users who enable chrome://flags/#enable-experimental-web-platform-features Done. https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.cpp:1081: m_resourceRequest.deactivateCacheAwareLoading(); On 2016/10/04 05:27:10, yhirano wrote: > DCHECK(!m_isAddClientProhibited); Done. https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/Resource.h (right): https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.h:290: virtual void willReloadAfterDiskCacheMiss(); On 2016/10/04 05:27:10, yhirano wrote: > Please add a comment that add (/ remove?)-ing clients is forbidden in this > function. Done. https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.h:420: bool m_isAddClientProhibited : 1; On 2016/10/04 05:27:10, yhirano wrote: > Is it a good idea to prohibit removeClient as well? Done. https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1112: // Assume error.errorCode() == net::ERR_CACHE_MISS, resend request with On 2016/10/04 05:27:10, yhirano wrote: > On 2016/10/03 08:08:02, hiroshige wrote: > > - We have to drop the isCacheAwareLoadingActivated() flag once the first > > (cache-only) request is succeeded (i.e. received response or redirect) or > failed > > (i.e. here). > > - We have to check whether this failure is really due to disk cache miss, > > because ResourceFetcher::didFailLoading() can be called by e.g. > > ResourceLoader::cancel(). Restarting request for non-disk-cache-miss failures > > increases unintended network requests. > > But we don't need the former if we have the latter, right? Now we check isCancellation() and isAccessCheck() here. Added checks to drop flags in Resource::willFollowRedirect() and Resource::responseReceived(). I think it's still required to drop the flag since we always assume the error is ERR_CACHE_MISS here. https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/network/ResourceRequest.cpp (right): https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/network/ResourceRequest.cpp:400: if (m_cachePolicy == WebCachePolicy::BypassingCache || On 2016/10/03 09:06:35, toyoshim wrote: > Using switch is better to catch new cache policy being added? > We would handle the new policy in a right way when added. Done. https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/network/ResourceRequest.cpp:405: m_cachePolicy = WebCachePolicy::ReturnCacheDataDontLoad; On 2016/10/04 04:12:02, toyoshim wrote: > How about using crbug.com/570205 for this TODO. Done.
https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1112: // Assume error.errorCode() == net::ERR_CACHE_MISS, resend request with On 2016/10/04 09:08:04, Shao-Chuan Lee wrote: > On 2016/10/04 05:27:10, yhirano wrote: > > On 2016/10/03 08:08:02, hiroshige wrote: > > > - We have to drop the isCacheAwareLoadingActivated() flag once the first > > > (cache-only) request is succeeded (i.e. received response or redirect) or > > failed > > > (i.e. here). > > > - We have to check whether this failure is really due to disk cache miss, > > > because ResourceFetcher::didFailLoading() can be called by e.g. > > > ResourceLoader::cancel(). Restarting request for non-disk-cache-miss > failures > > > increases unintended network requests. > > > > But we don't need the former if we have the latter, right? > > Now we check isCancellation() and isAccessCheck() here. > Added checks to drop flags in Resource::willFollowRedirect() and > Resource::responseReceived(). > > I think it's still required to drop the flag since we always assume the error is > ERR_CACHE_MISS here. Now dropping flags in Resource::finish() instead of responseReceived(), which is also called on failed requests.
lg with nits, then defer to fetch owners. https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp (right): https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp:37: #include "platform/CrossOriginAttributeValue.h" #include "platform/RuntimeEnabledFeatures.h" The file is autogened at <build-dir>/gen/blink/... https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/Resource.cpp:1092: while (ResourceClient* c = w.next()) { nit: we do not use {} for one-liner. https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceClient.h (right): https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceClient.h:55: virtual void willReloadAfterDiskCacheMiss(Resource*) {} not sure since this CL does not contain actual FontResourceClient implementation, but may be able to make it "const Resource*", and it could help to restrict accesses to Resource in the callback? https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1114: // existing ResourceLoader. Is it difficult to have an explicit method to know the missed cache case, like adding error.isMissedCache() or something? Having this sort of code based on an assumption that just rely on current implementation sounds not safe to me in terms of future-changes-proof.
> I guess checking error.isAccessCheck() here should suffice? Probably this is not sufficient, because there can be many code paths that leads load failure other than disk cache miss. I think we have to directly check whether we have disk cache miss and therefore we should re-initiate request to network, rather than checking non-disk-cache-miss cases. > Also, exposing the willReloadAfterDiskCacheMiss() looks dangerous (especially > for the current RawResources), so making it a FontResourceClient member might be > better. > Do you mean implementing only in FontResource/Client Yes, by overriding Resource::willReloadAfterDiskCacheMiss() by FontResource::willReloadAfterDiskCacheMiss() etc. https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.cpp:1083: m_isAddClientProhibited = true; On 2016/10/04 01:39:46, Shao-Chuan Lee wrote: > On 2016/10/03 08:11:47, kouhei wrote: > > AutoReset > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/AutoReset.... > > I made this a bit-field like other fields in Resource, and the address cannot be > taken for AutoReset. Maybe make this a debug-only field if there are memory > usage concerns? Probably we can make a normal bool and use AutoReset, because I think we don't have to save ~1 byte per Resource by sacrificing readability. https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp (right): https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp:92: RuntimeEnabledFeatures::webFontsCacheAwareTimeoutAdaptionEnabled()); Could you write like this? ResourceRequest resourceRequest(m_absoluteResource); resourceRequest.setIsCacheAwareLoadingEnabled(...); FetchRequest request(resourceRequest, ...); This is to reduce unnecessary mutableResourceRequest() calls. https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/Resource.cpp:420: if (m_resourceRequest.isCacheAwareLoadingActivated()) I'd like to prevent Resource from participating state control for cache-aware loading. The cache-aware loading should be managed only by ResourceFetcher and ResourceLoader, and Resource and ResourceClient should just receive notifications. By doing so, cache-aware loading doesn't complicate Resource state management. isCacheAwareLoadingActivated() looks like a state of current ongoing resource loading, not of Resource. Probably we can move this flag to ResourceLoader. https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceClient.h (right): https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceClient.h:55: virtual void willReloadAfterDiskCacheMiss(Resource*) {} On 2016/10/05 07:06:18, toyoshim wrote: > not sure since this CL does not contain actual FontResourceClient > implementation, but may be able to make it "const Resource*", and it could help > to restrict accesses to Resource in the callback? I also would like to restrict how ResourceClients accesses the Resource, but anyway ResourceClients have (mutable) references to Resource (e.g. as Member<Resource>), so I'm not sure making this pointer const helps much. (No reason to not doing so though)
Moved callback implementation to FontResource. Removed unittests for now, will add them back after design is settled. Next steps: - Move ResourceRequest manipulation to ResourceLoader/ResourceFetcher - Move cache-aware loading state to ResourceLoader - Propagate cache miss info into ResourceError from net/content https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/network/ResourceRequest.cpp (right): https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/network/ResourceRequest.cpp:405: m_cachePolicy = WebCachePolicy::ReturnCacheDataDontLoad; On 2016/10/04 09:08:04, Shao-Chuan Lee wrote: > On 2016/10/04 04:12:02, toyoshim wrote: > > How about using crbug.com/570205 for this TODO. > > Done. Now crbug.com/652649 is available for tracking. https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp (right): https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp:37: #include "platform/CrossOriginAttributeValue.h" On 2016/10/05 07:06:18, toyoshim wrote: > #include "platform/RuntimeEnabledFeatures.h" > > The file is autogened at <build-dir>/gen/blink/... Done. https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp:92: RuntimeEnabledFeatures::webFontsCacheAwareTimeoutAdaptionEnabled()); On 2016/10/05 08:27:10, hiroshige wrote: > Could you write like this? > > ResourceRequest resourceRequest(m_absoluteResource); > resourceRequest.setIsCacheAwareLoadingEnabled(...); > FetchRequest request(resourceRequest, ...); > > This is to reduce unnecessary mutableResourceRequest() calls. Done. https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/Resource.cpp:1092: while (ResourceClient* c = w.next()) { On 2016/10/05 07:06:18, toyoshim wrote: > nit: we do not use {} for one-liner. Done. https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceClient.h (right): https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceClient.h:55: virtual void willReloadAfterDiskCacheMiss(Resource*) {} On 2016/10/05 08:27:10, hiroshige wrote: > On 2016/10/05 07:06:18, toyoshim wrote: > > not sure since this CL does not contain actual FontResourceClient > > implementation, but may be able to make it "const Resource*", and it could > help > > to restrict accesses to Resource in the callback? > > I also would like to restrict how ResourceClients accesses the Resource, but > anyway ResourceClients have (mutable) references to Resource (e.g. as > Member<Resource>), so I'm not sure making this pointer const helps much. > (No reason to not doing so though) Now using const Resource* here. https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1114: // existing ResourceLoader. On 2016/10/05 07:06:18, toyoshim wrote: > Is it difficult to have an explicit method to know the missed cache case, like > adding error.isMissedCache() or something? Having this sort of code based on an > assumption that just rely on current implementation sounds not safe to me in > terms of future-changes-proof. Should we add a field for this in WebURLError and set it on content side?
https://codereview.chromium.org/2390583002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/network/ResourceRequest.cpp (right): https://codereview.chromium.org/2390583002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/ResourceRequest.cpp:403: return; DCHECK(!m_isCacheAwareLoadingActivated)? https://codereview.chromium.org/2390583002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/ResourceRequest.cpp:413: default: This section is not needed.
Patchset #5 (id:80001) has been deleted
- Moved isCacheAwareLoadingEnabled flag to FetchRequest - Moved activation logic and ResourceRequest manipulation to ResourceFetcher - Added isCacheMiss field to WebURLError/ResourceError to check net::ERR_CACHE_MISS in content - Don't support cached 3XX responses, fail as cache miss in willFollowRedirect() https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.cpp:1083: m_isAddClientProhibited = true; On 2016/10/05 08:27:10, hiroshige wrote: > On 2016/10/04 01:39:46, Shao-Chuan Lee wrote: > > On 2016/10/03 08:11:47, kouhei wrote: > > > AutoReset > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/AutoReset.... > > > > I made this a bit-field like other fields in Resource, and the address cannot > be > > taken for AutoReset. Maybe make this a debug-only field if there are memory > > usage concerns? > > Probably we can make a normal bool and use AutoReset, because I think we don't > have to save ~1 byte per Resource by sacrificing readability. Now we have ProhibitAddRemoveClientInScope. https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1112: // Assume error.errorCode() == net::ERR_CACHE_MISS, resend request with On 2016/10/04 09:46:25, Shao-Chuan Lee wrote: > On 2016/10/04 09:08:04, Shao-Chuan Lee wrote: > > On 2016/10/04 05:27:10, yhirano wrote: > > > On 2016/10/03 08:08:02, hiroshige wrote: > > > > - We have to drop the isCacheAwareLoadingActivated() flag once the first > > > > (cache-only) request is succeeded (i.e. received response or redirect) or > > > failed > > > > (i.e. here). > > > > - We have to check whether this failure is really due to disk cache miss, > > > > because ResourceFetcher::didFailLoading() can be called by e.g. > > > > ResourceLoader::cancel(). Restarting request for non-disk-cache-miss > > failures > > > > increases unintended network requests. > > > > > > But we don't need the former if we have the latter, right? > > > > Now we check isCancellation() and isAccessCheck() here. > > Added checks to drop flags in Resource::willFollowRedirect() and > > Resource::responseReceived(). > > > > I think it's still required to drop the flag since we always assume the error > is > > ERR_CACHE_MISS here. > > Now dropping flags in Resource::finish() instead of responseReceived(), which is > also called on failed requests. Now dropping flags in didFinishLoading(). https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/Resource.cpp:420: if (m_resourceRequest.isCacheAwareLoadingActivated()) On 2016/10/05 08:27:10, hiroshige wrote: > I'd like to prevent Resource from participating state control for cache-aware > loading. > The cache-aware loading should be managed only by ResourceFetcher and > ResourceLoader, and Resource and ResourceClient should just receive > notifications. By doing so, cache-aware loading doesn't complicate Resource > state management. > > isCacheAwareLoadingActivated() looks like a state of current ongoing resource > loading, not of Resource. > Probably we can move this flag to ResourceLoader. Moved manipulation logic to ResourceFetcher. Still keeping the Activated flag in ResourceRequest since it cannot (shouldn't) be modified in ResourceLoader methods. https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1114: // existing ResourceLoader. On 2016/10/05 10:16:10, Shao-Chuan Lee wrote: > On 2016/10/05 07:06:18, toyoshim wrote: > > Is it difficult to have an explicit method to know the missed cache case, like > > adding error.isMissedCache() or something? Having this sort of code based on > an > > assumption that just rely on current implementation sounds not safe to me in > > terms of future-changes-proof. > > Should we add a field for this in WebURLError and set it on content side? Done. https://codereview.chromium.org/2390583002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/network/ResourceRequest.cpp (right): https://codereview.chromium.org/2390583002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/ResourceRequest.cpp:403: return; On 2016/10/06 11:57:06, yhirano wrote: > DCHECK(!m_isCacheAwareLoadingActivated)? Done (now in activateCacheAwareLoading()). https://codereview.chromium.org/2390583002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/ResourceRequest.cpp:413: default: On 2016/10/06 11:57:06, yhirano wrote: > This section is not needed. I'm using this to ensure new flags are handled in the future, as suggested in #10.
https://codereview.chromium.org/2390583002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/network/ResourceRequest.cpp (right): https://codereview.chromium.org/2390583002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/ResourceRequest.cpp:413: default: On 2016/10/07 08:10:07, Shao-Chuan Lee wrote: > On 2016/10/06 11:57:06, yhirano wrote: > > This section is not needed. > > I'm using this to ensure new flags are handled in the future, as suggested in > #10. The compiler checks if each switch statement includes all cases, if there is no "default" section. The compiler check is better than NOTREACHED.
Patchset #6 (id:120001) has been deleted
ksakamoto@, toyoshim@: PTAL at changes to RemoteFontFaceSource. I guess doing the same as switchToSwapPeriod() does is enough for triggering a text re-render after disk cache miss? https://codereview.chromium.org/2390583002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/network/ResourceRequest.cpp (right): https://codereview.chromium.org/2390583002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/ResourceRequest.cpp:413: default: On 2016/10/11 04:17:07, yhirano wrote: > On 2016/10/07 08:10:07, Shao-Chuan Lee wrote: > > On 2016/10/06 11:57:06, yhirano wrote: > > > This section is not needed. > > > > I'm using this to ensure new flags are handled in the future, as suggested in > > #10. > > The compiler checks if each switch statement includes all cases, if there is no > "default" section. The compiler check is better than NOTREACHED. I see. Didn't notice we have -Wswitch enabled, thanks. Fixed in https://codereview.chromium.org/2390583002/diff2/100001:140001/third_party/We... .
I think cache-aware font loading should replace current logic around
"ShortLimitExceeded".
How about this:
void RemoteFontFaceSource::willReloadAfterDiskCacheMiss() {
fontDidNotLoadPromptly();
}
void RemoteFontFaceSource::fontLoadShortLimitExceeded(FontResource*) {
if (!isCacheAwareLoadingActivated())
fontDidNotLoadPromptly();
}
void RemoteFontFaceSource::fontDidNotLoadPromptly() {
// current code in fontLoadShortLimitExceeded()
...
}
https://codereview.chromium.org/2390583002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right):
https://codereview.chromium.org/2390583002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:159:
didBecomeVisibleFallback();
We should move to next period here, instead of keeping it and complicating the
condition for visibility of fallback font.
On 2016/10/11 08:07:38, Kunihiko Sakamoto wrote:
> I think cache-aware font loading should replace current logic around
> "ShortLimitExceeded".
>
> How about this:
>
> void RemoteFontFaceSource::willReloadAfterDiskCacheMiss() {
> fontDidNotLoadPromptly();
> }
This should be called only if short limit has exceeded,
isCacheAwareLoadingActivated() will become false after disk cache miss and this
may be called twice.
We should also do things in fontLoadLongLimitExceeded() if disk cache response
is that slow.
Maybe it's better to save current state and call
fontLoad{Short,Long}LimitExceeded() from willReloadAfterDiskCacheMiss()?
>
> void RemoteFontFaceSource::fontLoadShortLimitExceeded(FontResource*) {
> if (!isCacheAwareLoadingActivated())
> fontDidNotLoadPromptly();
> }
>
> void RemoteFontFaceSource::fontDidNotLoadPromptly() {
> // current code in fontLoadShortLimitExceeded()
> ...
> }
ksakamoto@: This should be more consistent by changing periods rather than overriding current period. PTAL Note that now cache-aware behavior is applied to all font-display types except when intervention is triggered. For 'auto', it's probably OK since intervention still works. For 'block', display will block until disk cache returns even if long limit has exceeded, is this acceptable? https://codereview.chromium.org/2390583002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right): https://codereview.chromium.org/2390583002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:159: didBecomeVisibleFallback(); On 2016/10/11 08:07:38, Kunihiko Sakamoto wrote: > We should move to next period here, instead of keeping it and complicating the > condition for visibility of fallback font. Done.
It looks better, but I still think we should not use short limit in cache-aware loading. On 2016/10/11 08:26:53, Shao-Chuan Lee wrote: > isCacheAwareLoadingActivated() will become false after disk cache miss and this > may be called twice. Ah then we should check RuntimeEnabledFeatures::webFontsCacheAwareTimeoutAdaptionEnabled() instead? On 2016/10/11 10:58:48, Shao-Chuan Lee wrote: > For 'block', display will block until disk cache returns even if long limit has > exceeded, is this acceptable? That's acceptable. Don't worry too much about such exceptional case. https://codereview.chromium.org/2390583002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right): https://codereview.chromium.org/2390583002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:168: if (m_font->loadLimitState() == FontResource::ShortLimitExceeded || Now we know that the font have to be fetched from network. What's the point of waiting until the short limit?
On 2016/10/12 01:38:46, Kunihiko Sakamoto wrote: > It looks better, but I still think we should not use short limit in cache-aware > loading. Do you mean we can just do the same as fontLoadShortLimitExceeded() upon disk cache miss if cache-aware is enabled? I was trying to respect original timing behavior if disk cache returns early, i.e. within 100ms. > > > On 2016/10/11 08:26:53, Shao-Chuan Lee wrote: > > isCacheAwareLoadingActivated() will become false after disk cache miss and > this > > may be called twice. > > Ah then we should check > RuntimeEnabledFeatures::webFontsCacheAwareTimeoutAdaptionEnabled() instead? > > > On 2016/10/11 10:58:48, Shao-Chuan Lee wrote: > > For 'block', display will block until disk cache returns even if long limit > has > > exceeded, is this acceptable? > > That's acceptable. Don't worry too much about such exceptional case. > > https://codereview.chromium.org/2390583002/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right): > > https://codereview.chromium.org/2390583002/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:168: if > (m_font->loadLimitState() == FontResource::ShortLimitExceeded || > Now we know that the font have to be fetched from network. What's the point of > waiting until the short limit?
On 2016/10/12 02:21:41, Shao-Chuan Lee wrote: > On 2016/10/12 01:38:46, Kunihiko Sakamoto wrote: > > It looks better, but I still think we should not use short limit in > cache-aware > > loading. > > Do you mean we can just do the same as fontLoadShortLimitExceeded() upon disk > cache miss if cache-aware is enabled? > I was trying to respect original timing behavior if disk cache returns early, > i.e. within 100ms. > Yes. That 100ms was just a rough heuristic to detect cache misses. Now we can know if the font is cached or not, so we don't need this anymore.
Moving cache-aware logic to FontResource, using short/long limit exceeded callbacks to manipulate font display, additional callback to RemoteFontFaceSource is not needed now. As discussed offline with ksakamoto@, we should still respect the 100ms short timeout in case network is fast enough, especially in font-display: optional.
font-display: swap handling was missing in PS8. We still need an additional callback in RemoteFontFaceSource to switchToSwapPeriod() in this case.
Let me check if my understanding is correct... For font-display:fallback or optional, this CL makes difference only when willReloadAfterDiskCacheMiss is not called within the short limit (100ms from start), correct? I'm wondering how often that happens. https://codereview.chromium.org/2390583002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right): https://codereview.chromium.org/2390583002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:59: m_period(BlockPeriod), Would this change current behavior when WebFontsCacheAwareTimeoutAdaption flag is off?
https://codereview.chromium.org/2390583002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/FontResource.cpp (right): https://codereview.chromium.org/2390583002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/FontResource.cpp:156: fontLoadShortLimitCallback(nullptr); This code assumes that startLoadLimitTimersIfNeeded has been called. Is the assumption always hold? It's a public method and we cannot assume when / if it's called in general. Even if it holds I want some documentation.
On 2016/10/12 06:10:15, Kunihiko Sakamoto wrote: > Let me check if my understanding is correct... > > For font-display:fallback or optional, this CL makes difference only when > willReloadAfterDiskCacheMiss is not called within the short limit (100ms from > start), correct? > I'm wondering how often that happens. Yes, now font display blocks for max(100ms, disk cache RTT) in both cases. Numbers from UMA (WebFont.DownloadTime.* - WebFont.MissedCache.DownloadTime.*) suggest that disk cache RTT often exceeds 100ms. https://codereview.chromium.org/2390583002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right): https://codereview.chromium.org/2390583002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:59: m_period(BlockPeriod), On 2016/10/12 06:10:14, Kunihiko Sakamoto wrote: > Would this change current behavior when WebFontsCacheAwareTimeoutAdaption flag > is off? If the flag is off then isCacheAwareLoadingActivated() will be false, and |m_period| will be correctly set below.
https://codereview.chromium.org/2390583002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right): https://codereview.chromium.org/2390583002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:59: m_period(BlockPeriod), On 2016/10/12 06:50:58, Shao-Chuan Lee wrote: > On 2016/10/12 06:10:14, Kunihiko Sakamoto wrote: > > Would this change current behavior when WebFontsCacheAwareTimeoutAdaption flag > > is off? > > If the flag is off then isCacheAwareLoadingActivated() will be false, and > |m_period| will be correctly set below. Ah OK, I missed that part. https://codereview.chromium.org/2390583002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/FontResource.cpp (right): https://codereview.chromium.org/2390583002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/FontResource.cpp:42: static const double fontLoadWaitShortLimitSec = 0.1; With cache-aware loading, disk cache RTT longer than this threshold won't result in FOUT anymore. Can you add a TODO comment saying that we should revisit this once cache-aware loading launched?
https://codereview.chromium.org/2390583002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/FontResource.cpp (right): https://codereview.chromium.org/2390583002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/FontResource.cpp:156: fontLoadShortLimitCallback(nullptr); On 2016/10/12 06:30:38, yhirano wrote: > This code assumes that startLoadLimitTimersIfNeeded has been called. Is the > assumption always hold? It's a public method and we cannot assume when / if it's > called in general. Even if it holds I want some documentation. Load limit timers are always started after FontResource loading is triggered in RemoteFontFaceSource::beginLoadIfNeeded(). However load limit timers may be restarted during loading, this will be fixed in https://crrev.com/2419753002. Thanks for noting this.
Can you add comments as I asked at [1] and [2]? 1: https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... 2: https://codereview.chromium.org/2390583002/diff/200001/third_party/WebKit/Sou... You added comments for [1] but it looks removed after that...
Oops, [1] was removed during refactoring. Re-added comments. https://codereview.chromium.org/2390583002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/FontResource.cpp (right): https://codereview.chromium.org/2390583002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/FontResource.cpp:42: static const double fontLoadWaitShortLimitSec = 0.1; On 2016/10/12 08:56:30, Kunihiko Sakamoto wrote: > With cache-aware loading, disk cache RTT longer than this threshold won't result > in FOUT anymore. > Can you add a TODO comment saying that we should revisit this once cache-aware > loading launched? Done. https://codereview.chromium.org/2390583002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/FontResource.cpp:156: fontLoadShortLimitCallback(nullptr); On 2016/10/13 05:20:45, Shao-Chuan Lee wrote: > On 2016/10/12 06:30:38, yhirano wrote: > > This code assumes that startLoadLimitTimersIfNeeded has been called. Is the > > assumption always hold? It's a public method and we cannot assume when / if > it's > > called in general. Even if it holds I want some documentation. > > Load limit timers are always started after FontResource loading is triggered in > RemoteFontFaceSource::beginLoadIfNeeded(). > However load limit timers may be restarted during loading, this will be fixed in > https://crrev.com/2419753002. Thanks for noting this. Added comments. https://codereview.chromium.org/2390583002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2390583002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:255: WebFontsCacheAwareTimeoutAdaption status=experimental Although "adaption" is also used, "adaptation" is much more common. https://goo.gl/nyvfjf
shaochuan@chromium.org changed reviewers: + haraken@chromium.org
haraken@: PTAL at changes under platform/. I wonder if it's OK to have fields for cache-aware state in ResourceRequest. If this is not preferable I have alternatives to move it somewhere else.
Patchset #12 (id:260001) has been deleted
shaochuan@chromium.org changed reviewers: + tkent@chromium.org
@tkent: @haraken is on a business trip, could you take a look at changes under platform/? Thanks.
On 2016/10/19 at 05:17:35, shaochuan wrote: > @tkent: @haraken is on a business trip, could you take a look at changes under platform/? Thanks. I'll approve after a loading team member says l-g-t-m for the whole change.
Thanks. This CL is still blocked by some issues: 1. This depends on https://crrev.com/2398613002/ and modifications to follow it is required. 2. Have layout tests available to verify behavior, although I'd prefer having the tests in a separate CL since it's not so trivial. On Wed, Oct 19, 2016 at 2:32 PM <tkent@chromium.org> wrote: > On 2016/10/19 at 05:17:35, shaochuan wrote: > > @tkent: @haraken is on a business trip, could you take a look at changes > under > platform/? Thanks. > > I'll approve after a loading team member says l-g-t-m for the whole change. > > > https://codereview.chromium.org/2390583002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Thanks. This CL is still blocked by some issues: 1. This depends on https://crrev.com/2398613002/ and modifications to follow it is required. 2. Have layout tests available to verify behavior, although I'd prefer having the tests in a separate CL since it's not so trivial. On Wed, Oct 19, 2016 at 2:32 PM <tkent@chromium.org> wrote: > On 2016/10/19 at 05:17:35, shaochuan wrote: > > @tkent: @haraken is on a business trip, could you take a look at changes > under > platform/? Thanks. > > I'll approve after a loading team member says l-g-t-m for the whole change. > > > https://codereview.chromium.org/2390583002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== WIP: Cache-aware FontResource loading In cache-aware loading, we attempt to load the resource from disk cache by setting WebCachePolicy to ReturnCacheDataDontLoad. If the load failed, ResourceClients are notified by callback, and the request is resent with original cache policy. Here we enable cache-aware loading for FontResources by default. Note: ReturnCacheDataDontLoad maps to net::LOAD_ONLY_FROM_CACHE which may return stale data, changes in net are required to support loading from disk cache with validation. BUG=570205 ========== to ========== WIP: WebFonts cache-aware timeout adaption This CL introduces cache-aware Resource loading and adaptive webfont display behavior, reducing unnecessary Flash of Unstyled Text (FOUT) if webfont is already available in disk cache. In cache-aware loading, the Resource is notified of disk cache miss from network stack during loading. This is implemented by sending a request to load only from disk cache, and retry with original request if the first one fails. Resource::willReloadAfterDiskCacheMiss() is called before the retry. If the cached response is a redirect, we fail the request as cache miss. In webfont display, fallback font will be used if webfont loading time exceeds certain timeout values in slow network. However it's observed that disk cache RTT may also hit the timeout regardless of network speed. In such cases we would like to block display until actual network request is being sent. Now FetchRequest provides an option to enable cache-aware loading for Resource::fetch(). If supplied cache policy doesn't conflict, cache-aware loading will be activated for new Resource instances (revalidation is not supported). Note: This CL depends on https://crrev.com/2398613002, we are now using ReturnCacheDataDontLoad which may return stale data from disk cache. BUG=570205 ==========
hiroshige@, yhirano@, kouhei@: Do you have further comments on overall loading design? If current design is OK, I'd like to break down this CL into separate CLs for review: - Add field in WebURLError/ResourceError to propagate cache miss info from content - Add new WebCachePolicy to load from disk cache with validation (depends on https://crrev.com/2398613002) - Cache-aware Resource loading - Cache-aware webfont display behavior
https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.cpp:1083: m_isAddClientProhibited = true; On 2016/10/04 01:39:46, Shao-Chuan Lee wrote: > On 2016/10/03 08:11:47, kouhei wrote: > > AutoReset > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/AutoReset.... > > I made this a bit-field like other fields in Resource, and the address cannot be > taken for AutoReset. Maybe make this a debug-only field if there are memory > usage concerns? You are right. You may want to introduce your own ForbiddenScope here as in: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/Vector.h?q... https://codereview.chromium.org/2390583002/diff/320001/content/child/web_url_... File content/child/web_url_request_util.cc (right): https://codereview.chromium.org/2390583002/diff/320001/content/child/web_url_... content/child/web_url_request_util.cc:490: error.isCacheMiss = reason == net::ERR_CACHE_MISS; Should this be moved to if branches below? Or would it be possible to add WebURLError::isCacheMiss() query method instead of keeping the bool? https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/FontResource.cpp (right): https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/FontResource.cpp:42: // BUG(570205): Revisit short limit value once cache-aware font display is TODO(shaochuan) Revisit ... crbug.com/570205. https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/Resource.h (right): https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/Resource.h:306: // TODO(632580): Workaround to persist cache-aware state, remove after fixed. TODO(username) https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right): https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:143: error.reason = 1; // non-zero Would it be possible to do "error.setIsCacheMiss()"? https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/network/ResourceRequest.cpp (right): https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/ResourceRequest.cpp:416: break; default: ASSERT_NOT_REACHED()? https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/ResourceRequest.cpp:427: // TODO(652649): Stale data may be retrieved with this flag. Switch to new TODO(shaochuan): ... crbug.com/652649
https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/network/ResourceRequest.cpp (right): https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/ResourceRequest.cpp:416: break; On 2016/10/20 10:43:07, kouhei wrote: > default: > ASSERT_NOT_REACHED()? This CL once had the statements but I asked to remove them. https://codereview.chromium.org/2390583002/diff/60001/third_party/WebKit/Sour...
https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/network/ResourceRequest.cpp (right): https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/ResourceRequest.cpp:416: break; On 2016/10/20 11:07:35, yhirano wrote: > On 2016/10/20 10:43:07, kouhei wrote: > > default: > > ASSERT_NOT_REACHED()? > > This CL once had the statements but I asked to remove them. > https://codereview.chromium.org/2390583002/diff/60001/third_party/WebKit/Sour... Got it. Please follow yhirano-san.
The design looks good to me, but I would like to see tests before landing this CL (or divided ones).
https://codereview.chromium.org/2390583002/diff/320001/content/child/web_url_... File content/child/web_url_request_util.cc (right): https://codereview.chromium.org/2390583002/diff/320001/content/child/web_url_... content/child/web_url_request_util.cc:490: error.isCacheMiss = reason == net::ERR_CACHE_MISS; On 2016/10/20 10:43:07, kouhei wrote: > Should this be moved to if branches below? > Or would it be possible to add WebURLError::isCacheMiss() query method instead > of keeping the bool? It's required to set the field here since WebURLError is in Blink and should not know about net::ERR_CACHE_MISS. Moved into if statement. https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/FontResource.cpp (right): https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/FontResource.cpp:42: // BUG(570205): Revisit short limit value once cache-aware font display is On 2016/10/20 10:43:07, kouhei wrote: > TODO(shaochuan) Revisit ... crbug.com/570205. Done. https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/Resource.h (right): https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/Resource.h:306: // TODO(632580): Workaround to persist cache-aware state, remove after fixed. On 2016/10/20 10:43:07, kouhei wrote: > TODO(username) Done. https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right): https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:143: error.reason = 1; // non-zero On 2016/10/20 10:43:07, kouhei wrote: > Would it be possible to do "error.setIsCacheMiss()"? This WebURLError object will be converted into ResourceError in |m_fetcher->didFailLoading()| in didFail(). Here we set reason to non-zero to bypass https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/expor... Blink does not interpret the meaning of reason, as suggested in https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebUR... https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/network/ResourceRequest.cpp (right): https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/ResourceRequest.cpp:427: // TODO(652649): Stale data may be retrieved with this flag. Switch to new On 2016/10/20 10:43:07, kouhei wrote: > TODO(shaochuan): ... crbug.com/652649 Done.
On 2016/10/21 02:26:47, yhirano wrote: > The design looks good to me, but I would like to see tests before landing this > CL (or divided ones). Layout test is available here: https://codereview.chromium.org/2438033003/ It's still hacky but works as a reference to verify display behavior. I'll have unit tests ready once design is fixed.
https://codereview.chromium.org/2390583002/diff/320001/content/child/web_url_... File content/child/web_url_request_util.cc (right): https://codereview.chromium.org/2390583002/diff/320001/content/child/web_url_... content/child/web_url_request_util.cc:490: error.isCacheMiss = reason == net::ERR_CACHE_MISS; On 2016/10/21 04:35:02, Shao-Chuan Lee wrote: > On 2016/10/20 10:43:07, kouhei wrote: > > Should this be moved to if branches below? > > Or would it be possible to add WebURLError::isCacheMiss() query method instead > > of keeping the bool? > > It's required to set the field here since WebURLError is in Blink and should not > know about net::ERR_CACHE_MISS. > Moved into if statement. Blink should not know about net::ERR_CACHE_MISS, but can we implement a predicate method bool WebURLError::isCacheMiss() const { return reason == net::ERR_CACHE_MISS; }
https://codereview.chromium.org/2390583002/diff/320001/content/child/web_url_... File content/child/web_url_request_util.cc (right): https://codereview.chromium.org/2390583002/diff/320001/content/child/web_url_... content/child/web_url_request_util.cc:490: error.isCacheMiss = reason == net::ERR_CACHE_MISS; On 2016/10/21 05:08:42, kouhei wrote: > On 2016/10/21 04:35:02, Shao-Chuan Lee wrote: > > On 2016/10/20 10:43:07, kouhei wrote: > > > Should this be moved to if branches below? > > > Or would it be possible to add WebURLError::isCacheMiss() query method > instead > > > of keeping the bool? > > > > It's required to set the field here since WebURLError is in Blink and should > not > > know about net::ERR_CACHE_MISS. > > Moved into if statement. > > Blink should not know about net::ERR_CACHE_MISS, but can we implement a > predicate method > bool WebURLError::isCacheMiss() const { return reason == net::ERR_CACHE_MISS; } This method should be in third_party/WebKit/public/platform/WebURLError.h or third_party/WebKit/Source/platform/exported/WebURLError.cpp, I think including net/base/net_errors.h from these places would be a dependency violation?
On 2016/10/21 05:22:17, Shao-Chuan Lee wrote: > https://codereview.chromium.org/2390583002/diff/320001/content/child/web_url_... > File content/child/web_url_request_util.cc (right): > > https://codereview.chromium.org/2390583002/diff/320001/content/child/web_url_... > content/child/web_url_request_util.cc:490: error.isCacheMiss = reason == > net::ERR_CACHE_MISS; > On 2016/10/21 05:08:42, kouhei wrote: > > On 2016/10/21 04:35:02, Shao-Chuan Lee wrote: > > > On 2016/10/20 10:43:07, kouhei wrote: > > > > Should this be moved to if branches below? > > > > Or would it be possible to add WebURLError::isCacheMiss() query method > > instead > > > > of keeping the bool? > > > > > > It's required to set the field here since WebURLError is in Blink and should > > not > > > know about net::ERR_CACHE_MISS. > > > Moved into if statement. > > > > Blink should not know about net::ERR_CACHE_MISS, but can we implement a > > predicate method > > bool WebURLError::isCacheMiss() const { return reason == net::ERR_CACHE_MISS; > } > > This method should be in third_party/WebKit/public/platform/WebURLError.h or > third_party/WebKit/Source/platform/exported/WebURLError.cpp, > I think including net/base/net_errors.h from these places would be a dependency > violation? I think we can move WebURLError to public/platform/network so it can depend on net/base https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/netwo... Let me create a CL
https://codereview.chromium.org/2390583002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right): https://codereview.chromium.org/2390583002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:253: m_period = SwapPeriod; You should call switchToSwapPeriod() instead, so that blank text is replaced with visible fallback font.
On 2016/10/21 05:26:50, kouhei wrote: > On 2016/10/21 05:22:17, Shao-Chuan Lee wrote: > > > https://codereview.chromium.org/2390583002/diff/320001/content/child/web_url_... > > File content/child/web_url_request_util.cc (right): > > > > > https://codereview.chromium.org/2390583002/diff/320001/content/child/web_url_... > > content/child/web_url_request_util.cc:490: error.isCacheMiss = reason == > > net::ERR_CACHE_MISS; > > On 2016/10/21 05:08:42, kouhei wrote: > > > On 2016/10/21 04:35:02, Shao-Chuan Lee wrote: > > > > On 2016/10/20 10:43:07, kouhei wrote: > > > > > Should this be moved to if branches below? > > > > > Or would it be possible to add WebURLError::isCacheMiss() query method > > > instead > > > > > of keeping the bool? > > > > > > > > It's required to set the field here since WebURLError is in Blink and > should > > > not > > > > know about net::ERR_CACHE_MISS. > > > > Moved into if statement. > > > > > > Blink should not know about net::ERR_CACHE_MISS, but can we implement a > > > predicate method > > > bool WebURLError::isCacheMiss() const { return reason == > net::ERR_CACHE_MISS; > > } > > > > This method should be in third_party/WebKit/public/platform/WebURLError.h or > > third_party/WebKit/Source/platform/exported/WebURLError.cpp, > > I think including net/base/net_errors.h from these places would be a > dependency > > violation? > > I think we can move WebURLError to public/platform/network so it can depend on > net/base > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/netwo... > > Let me create a CL WebURLError is "exported" so I think platform/exported is the right place for the class though my understanding of the public/ directory placement is not perfect. Instead we can have utility methods in platform/network and call it from platform/exported.
On 2016/10/21 05:47:25, yhirano wrote: > On 2016/10/21 05:26:50, kouhei wrote: > > On 2016/10/21 05:22:17, Shao-Chuan Lee wrote: > > > > > > https://codereview.chromium.org/2390583002/diff/320001/content/child/web_url_... > > > File content/child/web_url_request_util.cc (right): > > > > > > > > > https://codereview.chromium.org/2390583002/diff/320001/content/child/web_url_... > > > content/child/web_url_request_util.cc:490: error.isCacheMiss = reason == > > > net::ERR_CACHE_MISS; > > > On 2016/10/21 05:08:42, kouhei wrote: > > > > On 2016/10/21 04:35:02, Shao-Chuan Lee wrote: > > > > > On 2016/10/20 10:43:07, kouhei wrote: > > > > > > Should this be moved to if branches below? > > > > > > Or would it be possible to add WebURLError::isCacheMiss() query method > > > > instead > > > > > > of keeping the bool? > > > > > > > > > > It's required to set the field here since WebURLError is in Blink and > > should > > > > not > > > > > know about net::ERR_CACHE_MISS. > > > > > Moved into if statement. > > > > > > > > Blink should not know about net::ERR_CACHE_MISS, but can we implement a > > > > predicate method > > > > bool WebURLError::isCacheMiss() const { return reason == > > net::ERR_CACHE_MISS; > > > } > > > > > > This method should be in third_party/WebKit/public/platform/WebURLError.h or > > > third_party/WebKit/Source/platform/exported/WebURLError.cpp, > > > I think including net/base/net_errors.h from these places would be a > > dependency > > > violation? > > > > I think we can move WebURLError to public/platform/network so it can depend on > > net/base > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/netwo... > > > > Let me create a CL > > WebURLError is "exported" so I think platform/exported is the right place for > the class though my understanding of the public/ directory placement is not > perfect. Instead we can have utility methods in platform/network and call it > from platform/exported. Having a method in blink::NetworkUtils should be nice?
On 2016/10/21 05:52:47, Shao-Chuan Lee wrote: > On 2016/10/21 05:47:25, yhirano wrote: > > On 2016/10/21 05:26:50, kouhei wrote: > > > On 2016/10/21 05:22:17, Shao-Chuan Lee wrote: > > > > > > > > > > https://codereview.chromium.org/2390583002/diff/320001/content/child/web_url_... > > > > File content/child/web_url_request_util.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2390583002/diff/320001/content/child/web_url_... > > > > content/child/web_url_request_util.cc:490: error.isCacheMiss = reason == > > > > net::ERR_CACHE_MISS; > > > > On 2016/10/21 05:08:42, kouhei wrote: > > > > > On 2016/10/21 04:35:02, Shao-Chuan Lee wrote: > > > > > > On 2016/10/20 10:43:07, kouhei wrote: > > > > > > > Should this be moved to if branches below? > > > > > > > Or would it be possible to add WebURLError::isCacheMiss() query > method > > > > > instead > > > > > > > of keeping the bool? > > > > > > > > > > > > It's required to set the field here since WebURLError is in Blink and > > > should > > > > > not > > > > > > know about net::ERR_CACHE_MISS. > > > > > > Moved into if statement. > > > > > > > > > > Blink should not know about net::ERR_CACHE_MISS, but can we implement a > > > > > predicate method > > > > > bool WebURLError::isCacheMiss() const { return reason == > > > net::ERR_CACHE_MISS; > > > > } > > > > > > > > This method should be in third_party/WebKit/public/platform/WebURLError.h > or > > > > third_party/WebKit/Source/platform/exported/WebURLError.cpp, > > > > I think including net/base/net_errors.h from these places would be a > > > dependency > > > > violation? > > > > > > I think we can move WebURLError to public/platform/network so it can depend > on > > > net/base > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/netwo... > > > > > > Let me create a CL > > > > WebURLError is "exported" so I think platform/exported is the right place for > > the class though my understanding of the public/ directory placement is not > > perfect. Instead we can have utility methods in platform/network and call it > > from platform/exported. > > Having a method in blink::NetworkUtils should be nice? sgtm
Patchset #16 (id:360001) has been deleted
https://codereview.chromium.org/2390583002/diff/320001/content/child/web_url_... File content/child/web_url_request_util.cc (right): https://codereview.chromium.org/2390583002/diff/320001/content/child/web_url_... content/child/web_url_request_util.cc:490: error.isCacheMiss = reason == net::ERR_CACHE_MISS; On 2016/10/21 05:22:17, Shao-Chuan Lee wrote: > On 2016/10/21 05:08:42, kouhei wrote: > > On 2016/10/21 04:35:02, Shao-Chuan Lee wrote: > > > On 2016/10/20 10:43:07, kouhei wrote: > > > > Should this be moved to if branches below? > > > > Or would it be possible to add WebURLError::isCacheMiss() query method > > instead > > > > of keeping the bool? > > > > > > It's required to set the field here since WebURLError is in Blink and should > > not > > > know about net::ERR_CACHE_MISS. > > > Moved into if statement. > > > > Blink should not know about net::ERR_CACHE_MISS, but can we implement a > > predicate method > > bool WebURLError::isCacheMiss() const { return reason == net::ERR_CACHE_MISS; > } > > This method should be in third_party/WebKit/public/platform/WebURLError.h or > third_party/WebKit/Source/platform/exported/WebURLError.cpp, > I think including net/base/net_errors.h from these places would be a dependency > violation? Now having utility methods under blink::NetworkUtils to workaround this. PTAL
lgtm from myside
https://codereview.chromium.org/2390583002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right): https://codereview.chromium.org/2390583002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:253: m_period = SwapPeriod; On 2016/10/21 05:41:07, Kunihiko Sakamoto wrote: > You should call switchToSwapPeriod() instead, so that blank text is replaced > with visible fallback font. Done.
https://codereview.chromium.org/2390583002/diff/400001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/exported/WebURLError.cpp (right): https://codereview.chromium.org/2390583002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/exported/WebURLError.cpp:71: error.reason = NetworkUtils::cacheMissErrorCode(); This rather looks a little awkward layering workaround to me, I think we can try adding deps to net/base/ (or just net/base/net_errors.h to be more specific) from public/platform or Source/platform and ask Elliott about it. We wouldn't want to have uncontrolled dependency but net/base dependency from platform feels reasonable to me.
https://codereview.chromium.org/2390583002/diff/400001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right): https://codereview.chromium.org/2390583002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:142: didFail(nullptr, WebURLError::cacheMissError()); nit: I think we'd better create ResourceError here and pass it to didFail? (See how other code in this class is calling didFail). WebURLError is an interface for exposing ResourceError created inside blink to embedders outside blink, and we're inside blink. https://codereview.chromium.org/2390583002/diff/400001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/exported/WebURLError.cpp (right): https://codereview.chromium.org/2390583002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/exported/WebURLError.cpp:71: error.reason = NetworkUtils::cacheMissErrorCode(); On 2016/10/21 10:35:13, kinuko wrote: > This rather looks a little awkward layering workaround to me, I think we can try > adding deps to net/base/ (or just net/base/net_errors.h to be more specific) > from public/platform or Source/platform and ask Elliott about it. We wouldn't > want to have uncontrolled dependency but net/base dependency from platform feels > reasonable to me. Hmm... looking into this a little further, actually I feel adding one more boolean here was better aligned at least in the current code structure. As you mentioned somewhere WebURLError's reason code is not meant to be interpreted by blink and this change is violating it. I agree that current structure for WebURLError, ResourceError and net error is more convoluted than necessary and we should probably fix it, but I'd prefer doing that in a separate, dedicated one-off change.
https://codereview.chromium.org/2390583002/diff/400001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right): https://codereview.chromium.org/2390583002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:142: didFail(nullptr, WebURLError::cacheMissError()); On 2016/10/22 04:43:01, kinuko wrote: > nit: I think we'd better create ResourceError here and pass it to didFail? (See > how other code in this class is calling didFail). WebURLError is an interface > for exposing ResourceError created inside blink to embedders outside blink, and > we're inside blink. I've noticed that other places pass ResourceError to didFail(), but it's converted to WebURLError and back to ResourceError immediately when passing to ResourceFetcher, I wonder if I should follow existing code. https://codereview.chromium.org/2390583002/diff/400001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/exported/WebURLError.cpp (right): https://codereview.chromium.org/2390583002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/exported/WebURLError.cpp:71: error.reason = NetworkUtils::cacheMissErrorCode(); On 2016/10/22 04:43:01, kinuko wrote: > On 2016/10/21 10:35:13, kinuko wrote: > > This rather looks a little awkward layering workaround to me, I think we can > try > > adding deps to net/base/ (or just net/base/net_errors.h to be more specific) > > from public/platform or Source/platform and ask Elliott about it. We wouldn't > > want to have uncontrolled dependency but net/base dependency from platform > feels > > reasonable to me. > > Hmm... looking into this a little further, actually I feel adding one more > boolean here was better aligned at least in the current code structure. As you > mentioned somewhere WebURLError's reason code is not meant to be interpreted by > blink and this change is violating it. > > I agree that current structure for WebURLError, ResourceError and net error is > more convoluted than necessary and we should probably fix it, but I'd prefer > doing that in a separate, dedicated one-off change. I feel like using an enum for mapping from net:: errors we care about should be better, the existing isCancellation field can also be replaced. ResourceError will also have to be modified to be consistent. Adding a new field is a rather simple solution though. kouhei@ WDYT?
On 2016/10/25 04:18:20, Shao-Chuan Lee wrote: > https://codereview.chromium.org/2390583002/diff/400001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right): > > https://codereview.chromium.org/2390583002/diff/400001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:142: didFail(nullptr, > WebURLError::cacheMissError()); > On 2016/10/22 04:43:01, kinuko wrote: > > nit: I think we'd better create ResourceError here and pass it to didFail? > (See > > how other code in this class is calling didFail). WebURLError is an interface > > for exposing ResourceError created inside blink to embedders outside blink, > and > > we're inside blink. > > I've noticed that other places pass ResourceError to didFail(), but it's > converted to WebURLError and back to ResourceError immediately when passing to > ResourceFetcher, I wonder if I should follow existing code. > > https://codereview.chromium.org/2390583002/diff/400001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/exported/WebURLError.cpp (right): > > https://codereview.chromium.org/2390583002/diff/400001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/exported/WebURLError.cpp:71: error.reason = > NetworkUtils::cacheMissErrorCode(); > On 2016/10/22 04:43:01, kinuko wrote: > > On 2016/10/21 10:35:13, kinuko wrote: > > > This rather looks a little awkward layering workaround to me, I think we can > > try > > > adding deps to net/base/ (or just net/base/net_errors.h to be more specific) > > > from public/platform or Source/platform and ask Elliott about it. We > wouldn't > > > want to have uncontrolled dependency but net/base dependency from platform > > feels > > > reasonable to me. > > > > Hmm... looking into this a little further, actually I feel adding one more > > boolean here was better aligned at least in the current code structure. As you > > mentioned somewhere WebURLError's reason code is not meant to be interpreted > by > > blink and this change is violating it. > > > > I agree that current structure for WebURLError, ResourceError and net error is > > more convoluted than necessary and we should probably fix it, but I'd prefer > > doing that in a separate, dedicated one-off change. > > I feel like using an enum for mapping from net:: errors we care about should be > better, the existing isCancellation field can also be replaced. ResourceError > will also have to be modified to be consistent. > > Adding a new field is a rather simple solution though. kouhei@ WDYT? yhirano@ suggests using only ResourceError here. We can check the error code directly since ResourceError is also under platform/network, and the additional field in both WebURLError/ResourceError is not required. This sounds like a better approach.
https://codereview.chromium.org/2390583002/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/network/ResourceError.cpp (right): https://codereview.chromium.org/2390583002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/ResourceError.cpp:51: ResourceError error; error.m_domain = String(net::kErrorDomain); https://codereview.chromium.org/2390583002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/ResourceError.cpp:73: return m_errorCode == net::ERR_CACHE_MISS; We need to check m_domain as well.
Upcoming refactoring, as discussed offline with hiroshige@ and tyoshino@: (using CAL = cache-aware loading) 1. Move isCALEnabled flag to ResourceLoaderOptions 2. Move isCALActivated to ResourceLoader, remove savedCachePolicy It's better to have the flag as state during CAL in ResourceLoader, we'll simply override the cache policy in ResourceRequest in start(). However for Resources whose loading is deferred (e.g. FontResource) the ResourceClients may not determine whether CAL will be activated if added before startLoad(). A workaround is to call the cache miss callback immediately if CAL is enabled but eventually not activated, which should be OK in font-display case. Another issue is to revise determineRevalidationPolicy to cover edge cases. I guess this CL is getting too big, I'll continue this part in another CL.
Description was changed from ========== WIP: WebFonts cache-aware timeout adaption This CL introduces cache-aware Resource loading and adaptive webfont display behavior, reducing unnecessary Flash of Unstyled Text (FOUT) if webfont is already available in disk cache. In cache-aware loading, the Resource is notified of disk cache miss from network stack during loading. This is implemented by sending a request to load only from disk cache, and retry with original request if the first one fails. Resource::willReloadAfterDiskCacheMiss() is called before the retry. If the cached response is a redirect, we fail the request as cache miss. In webfont display, fallback font will be used if webfont loading time exceeds certain timeout values in slow network. However it's observed that disk cache RTT may also hit the timeout regardless of network speed. In such cases we would like to block display until actual network request is being sent. Now FetchRequest provides an option to enable cache-aware loading for Resource::fetch(). If supplied cache policy doesn't conflict, cache-aware loading will be activated for new Resource instances (revalidation is not supported). Note: This CL depends on https://crrev.com/2398613002, we are now using ReturnCacheDataDontLoad which may return stale data from disk cache. BUG=570205 ========== to ========== [WIP] WebFonts cache-aware timeout adaption This CL introduces cache-aware Resource loading and adaptive webfont display behavior, reducing unnecessary Flash of Unstyled Text (FOUT) if webfont is already available in disk cache. In cache-aware loading, the Resource is notified of disk cache miss from network stack during loading. This is implemented by sending a request to load only from disk cache, and retry with original request if the first one fails. Resource::willReloadAfterDiskCacheMiss() is called before the retry. If the cached response is a redirect, we fail the request as cache miss. In webfont display, fallback font will be used if webfont loading time exceeds certain timeout values in slow network. However it's observed that disk cache RTT may also hit the timeout regardless of network speed. In such cases we would like to block display until actual network request is being sent. Now FetchRequest provides an option to enable cache-aware loading for Resource::fetch(). If supplied cache policy doesn't conflict, cache-aware loading will be activated for new Resource instances (revalidation is not supported). Note: This CL depends on https://crrev.com/2398613002, we are now using ReturnCacheDataDontLoad which may return stale data from disk cache. BUG=570205 ========== |
