WIP: Cache-aware FontResource loading (issue 2390583002 by shaochuan@chromium.org)

[shao...@chromium.org]

Reviewers: kouhei, yhirano, hiroshige, Kunihiko Sakamoto, toyoshim
CL: https://codereview.chromium.org/2390583002/

Message:
This is the tentative implementation based on the design notes
(https://docs.google.com/a/chromium.org/document/d/1vaGzhJyfR6ozSiMfphdOrwrca51MNVRT6K2r5M1xtlE/edit?usp=sharing)
and previous offline discussions, PTAL.
Changes to RemoteFontFaceSource are not yet included.

Description:
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.

Affected files (+90, -0 lines):
M third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp
M third_party/WebKit/Source/core/fetch/Resource.h
M third_party/WebKit/Source/core/fetch/Resource.cpp
M third_party/WebKit/Source/core/fetch/ResourceClient.h
M third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
M third_party/WebKit/Source/platform/network/ResourceRequest.h
M third_party/WebKit/Source/platform/network/ResourceRequest.cpp


Index: third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp
diff --git a/third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp b/third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp
index 6a88cbe223d3a28d1510ec6cb5bf1bc2700cdbaf..fb8e7b5e567bebac0b87ab76c76c47b0d48aa73c 100644
--- a/third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp
+++ b/third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp
@@ -88,6 +88,7 @@ FontResource* CSSFontFaceSrcValue::fetch(Document* document) const {
if (!m_fetched) {
FetchRequest request(ResourceRequest(m_absoluteResource),
FetchInitiatorTypeNames::css);
+ request.mutableResourceRequest().setIsCacheAwareLoadingEnabled(true);
request.setContentSecurityCheck(m_shouldCheckContentSecurityPolicy);
SecurityOrigin* securityOrigin = document->getSecurityOrigin();
setCrossOriginAccessControl(request, securityOrigin);
Index: third_party/WebKit/Source/core/fetch/Resource.cpp
diff --git a/third_party/WebKit/Source/core/fetch/Resource.cpp b/third_party/WebKit/Source/core/fetch/Resource.cpp
index 9d88d0191a75c2bba0f1e63d567acc4874e14f93..b31394ae2d01f01a881bf94082a09743a43f30d2 100644
--- a/third_party/WebKit/Source/core/fetch/Resource.cpp
+++ b/third_party/WebKit/Source/core/fetch/Resource.cpp
@@ -312,6 +312,7 @@ Resource::Resource(const ResourceRequest& request,
m_linkPreload(false),
m_isRevalidating(false),
m_isAlive(false),
+ m_isAddClientProhibited(false),
m_options(options),
m_responseTimestamp(currentTime()),
m_cancelTimer(this, &Resource::cancelTimerFired),
@@ -686,6 +687,8 @@ void Resource::willAddClientOrObserver(PreloadReferencePolicy policy) {

void Resource::addClient(ResourceClient* client,
PreloadReferencePolicy policy) {
+ DCHECK(!m_isAddClientProhibited);
+
willAddClientOrObserver(policy);

if (m_isRevalidating) {
@@ -1074,4 +1077,15 @@ bool Resource::isLoadEventBlockingResourceType() const {
return false;
}

+void Resource::willReloadAfterDiskCacheMiss() {
+ m_resourceRequest.deactivateCacheAwareLoading();
+
+ m_isAddClientProhibited = true;
+ ResourceClientWalker<ResourceClient> w(m_clients);
+ while (ResourceClient* c = w.next()) {
+ c->willReloadAfterDiskCacheMiss(this);
+ }
+ m_isAddClientProhibited = false;
+}
+
} // namespace blink
Index: third_party/WebKit/Source/core/fetch/Resource.h
diff --git a/third_party/WebKit/Source/core/fetch/Resource.h b/third_party/WebKit/Source/core/fetch/Resource.h
index 08f24c01b62c5fa797af8745a5788313364e4668..5c97dfa3d5bfa1b696708935e7241077e0e7360a 100644
--- a/third_party/WebKit/Source/core/fetch/Resource.h
+++ b/third_party/WebKit/Source/core/fetch/Resource.h
@@ -35,6 +35,7 @@
#include "platform/network/ResourceResponse.h"
#include "platform/scheduler/CancellableTaskFactory.h"
#include "platform/web_process_memory_dump.h"
+#include "public/platform/WebCachePolicy.h"
#include "public/platform/WebDataConsumerHandle.h"
#include "wtf/Allocator.h"
#include "wtf/HashCountedSet.h"
@@ -286,6 +287,13 @@ class CORE_EXPORT Resource : public GarbageCollectedFinalized<Resource>,

virtual bool canReuse(const ResourceRequest&) const { return true; }

+ virtual void willReloadAfterDiskCacheMiss();
+
+ // TODO(632580): Workaround to persist cache-aware state, remove after fixed.
+ void setResourceRequest(const ResourceRequest& resourceRequest) {
+ m_resourceRequest = resourceRequest;
+ }
+
// Used by the MemoryCache to reduce the memory consumption of the entry.
void prune();

@@ -409,6 +417,8 @@ class CORE_EXPORT Resource : public GarbageCollectedFinalized<Resource>,
bool m_isRevalidating : 1;
bool m_isAlive : 1;

+ bool m_isAddClientProhibited : 1;
+
// Ordered list of all redirects followed while fetching this resource.
Vector<RedirectPair> m_redirectChain;

Index: third_party/WebKit/Source/core/fetch/ResourceClient.h
diff --git a/third_party/WebKit/Source/core/fetch/ResourceClient.h b/third_party/WebKit/Source/core/fetch/ResourceClient.h
index f75dd04d45ee58eb8ae5429cff552226b124d8c8..76cc1cceb1df0a577377f4ac6d71707c910b3650 100644
--- a/third_party/WebKit/Source/core/fetch/ResourceClient.h
+++ b/third_party/WebKit/Source/core/fetch/ResourceClient.h
@@ -52,6 +52,8 @@ class CORE_EXPORT ResourceClient : public GarbageCollectedMixin {
return BaseResourceType;
}

+ virtual void willReloadAfterDiskCacheMiss(Resource*) {}
+
// Name for debugging, e.g. shown in memory-infra.
virtual String debugName() const = 0;

Index: third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
diff --git a/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp b/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
index aab6e32f909db869ae8dbc90db1b6bae95c83f27..ac0881ea45584084e232cd4c99ed10c8374b9e5a 100644
--- a/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
+++ b/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
@@ -1108,6 +1108,16 @@ void ResourceFetcher::didFinishLoading(Resource* resource,

void ResourceFetcher::didFailLoading(Resource* resource,
const ResourceError& error) {
+ if (resource->resourceRequest().isCacheAwareLoadingActivated()) {
+ // Assume error.errorCode() == net::ERR_CACHE_MISS, resend request with
+ // existing ResourceLoader.
+ resource->willReloadAfterDiskCacheMiss();
+ resource->loader()->start(resource->resourceRequest(),
+ context().loadingTaskRunner(),
+ context().defersLoading());
+ return;
+ }
+
TRACE_EVENT_ASYNC_END0("blink.net", "Resource", resource->identifier());
removeResourceLoader(resource->loader());
m_resourceTimingInfoMap.take(const_cast<Resource*>(resource));
@@ -1220,6 +1230,9 @@ bool ResourceFetcher::startLoad(Resource* resource) {
if (sourceOrigin && sourceOrigin->hasSuborigin())
request.setSkipServiceWorker(WebURLRequest::SkipServiceWorker::All);

+ // TODO(632580): Workaround to persist cache-aware state, remove after fixed.
+ resource->setResourceRequest(request);
+
ResourceLoader* loader = ResourceLoader::create(this, resource);
if (resource->shouldBlockLoadEvent())
m_loaders.add(loader);
@@ -1332,6 +1345,7 @@ void ResourceFetcher::willSendRequest(unsigned long identifier,
const ResourceLoaderOptions& options) {
context().dispatchWillSendRequest(identifier, newRequest, redirectResponse,
options.initiatorInfo);
+ newRequest.mayActivateCacheAwareLoading();
}

void ResourceFetcher::updateAllImageResourcePriorities() {
Index: third_party/WebKit/Source/platform/network/ResourceRequest.cpp
diff --git a/third_party/WebKit/Source/platform/network/ResourceRequest.cpp b/third_party/WebKit/Source/platform/network/ResourceRequest.cpp
index dd9afd70463b65ca5578de5a29332df80b5968fd..4c6596d666b7ccee5ee559a0d36f0c5ebc2708d9 100644
--- a/third_party/WebKit/Source/platform/network/ResourceRequest.cpp
+++ b/third_party/WebKit/Source/platform/network/ResourceRequest.cpp
@@ -88,6 +88,9 @@ ResourceRequest::ResourceRequest(CrossThreadResourceRequestData* data)
m_uiStartTime = data->m_uiStartTime;
m_isExternalRequest = data->m_isExternalRequest;
m_inputPerfMetricReportPolicy = data->m_inputPerfMetricReportPolicy;
+ m_isCacheAwareLoadingEnabled = data->m_isCacheAwareLoadingEnabled;
+ m_isCacheAwareLoadingActivated = data->m_isCacheAwareLoadingActivated;
+ m_savedCachePolicy = data->m_savedCachePolicy;
m_redirectStatus = data->m_redirectStatus;
}

@@ -136,6 +139,9 @@ std::unique_ptr<CrossThreadResourceRequestData> ResourceRequest::copyData()
data->m_uiStartTime = m_uiStartTime;
data->m_isExternalRequest = m_isExternalRequest;
data->m_inputPerfMetricReportPolicy = m_inputPerfMetricReportPolicy;
+ data->m_isCacheAwareLoadingEnabled = m_isCacheAwareLoadingEnabled;
+ data->m_isCacheAwareLoadingActivated = m_isCacheAwareLoadingActivated;
+ data->m_savedCachePolicy = m_savedCachePolicy;
data->m_redirectStatus = m_redirectStatus;
return data;
}
@@ -387,6 +393,25 @@ bool ResourceRequest::hasCacheValidatorFields() const {
!m_httpHeaderFields.get(HTTPNames::ETag).isEmpty();
}

+void ResourceRequest::mayActivateCacheAwareLoading() {
+ if (!m_isCacheAwareLoadingEnabled)
+ return;
+
+ if (m_cachePolicy == WebCachePolicy::BypassingCache ||
+ m_cachePolicy == WebCachePolicy::ReturnCacheDataDontLoad)
+ return;
+
+ m_savedCachePolicy = m_cachePolicy;
+ m_cachePolicy = WebCachePolicy::ReturnCacheDataDontLoad;
+ m_isCacheAwareLoadingActivated = true;
+}
+
+void ResourceRequest::deactivateCacheAwareLoading() {
+ DCHECK(m_isCacheAwareLoadingActivated);
+ m_cachePolicy = m_savedCachePolicy;
+ m_isCacheAwareLoadingActivated = false;
+}
+
void ResourceRequest::initialize(const KURL& url) {
m_url = url;
m_cachePolicy = WebCachePolicy::UseProtocolCachePolicy;
@@ -419,6 +444,9 @@ void ResourceRequest::initialize(const KURL& url) {
m_inputPerfMetricReportPolicy = InputToLoadPerfMetricReportPolicy::NoReport;
m_redirectStatus = RedirectStatus::NoRedirect;
m_requestorOrigin = SecurityOrigin::createUnique();
+ m_isCacheAwareLoadingEnabled = false;
+ m_isCacheAwareLoadingActivated = false;
+ m_savedCachePolicy = WebCachePolicy::UseProtocolCachePolicy;
}

} // namespace blink
Index: third_party/WebKit/Source/platform/network/ResourceRequest.h
diff --git a/third_party/WebKit/Source/platform/network/ResourceRequest.h b/third_party/WebKit/Source/platform/network/ResourceRequest.h
index 8682a497ea5430af68372f09f248e4434df41c3a..c3efd40fcbb5b9a05a40b7c515e7c4f7e1ad6f1f 100644
--- a/third_party/WebKit/Source/platform/network/ResourceRequest.h
+++ b/third_party/WebKit/Source/platform/network/ResourceRequest.h
@@ -302,6 +302,21 @@ class PLATFORM_EXPORT ResourceRequest final {
void setRedirectStatus(RedirectStatus status) { m_redirectStatus = status; }
RedirectStatus redirectStatus() const { return m_redirectStatus; }

+ bool isCacheAwareLoadingEnabled() const {
+ return m_isCacheAwareLoadingEnabled;
+ }
+
+ void setIsCacheAwareLoadingEnabled(bool isCacheAwareLoadingEnabled) {
+ m_isCacheAwareLoadingEnabled = isCacheAwareLoadingEnabled;
+ }
+
+ bool isCacheAwareLoadingActivated() const {
+ return m_isCacheAwareLoadingActivated;
+ }
+
+ void mayActivateCacheAwareLoading();
+ void deactivateCacheAwareLoading();
+
private:
void initialize(const KURL&);

@@ -343,6 +358,9 @@ class PLATFORM_EXPORT ResourceRequest final {
double m_uiStartTime;
bool m_isExternalRequest;
InputToLoadPerfMetricReportPolicy m_inputPerfMetricReportPolicy;
+ bool m_isCacheAwareLoadingEnabled;
+ bool m_isCacheAwareLoadingActivated;
+ WebCachePolicy m_savedCachePolicy;

mutable CacheControlHeader m_cacheControlHeaderCache;

@@ -392,6 +410,9 @@ struct CrossThreadResourceRequestData {
double m_uiStartTime;
bool m_isExternalRequest;
InputToLoadPerfMetricReportPolicy m_inputPerfMetricReportPolicy;
+ bool m_isCacheAwareLoadingEnabled;
+ bool m_isCacheAwareLoadingActivated;
+ WebCachePolicy m_savedCachePolicy;
ResourceRequest::RedirectStatus m_redirectStatus;
};

[hiro...@chromium.org]

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/core/fetch/ResourceFetcher.cpp
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp#newcode1112
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/

[kou...@chromium.org]

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/core/fetch/Resource.cpp
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/core/fetch/Resource.cpp#newcode1083
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.h?type=cs&sq=package:chromium&rcl=1475464636&l=36

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/core/fetch/Resource.h
File third_party/WebKit/Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/core/fetch/Resource.h#newcode292
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/

[hiro...@chromium.org]

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/core/fetch/Resource.h
File third_party/WebKit/Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/core/fetch/Resource.h#newcode292
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/

[ksak...@chromium.org]

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp
File third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp (right):

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp#newcode91
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_xLhS4/edit#heading=h.syp8l2efqxu1

https://codereview.chromium.org/2390583002/

[shao...@chromium.org]

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/core/fetch/ResourceFetcher.cpp
> File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):
>
>
https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp#newcode1112
> 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.

https://codereview.chromium.org/2390583002/

[shao...@chromium.org]

OK, modifying FontFace::initCSSFontFace() to consider font-display option.

https://codereview.chromium.org/2390583002/

[toyo...@chromium.org]

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp
File third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp (right):

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp#newcode91
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/

[toyo...@chromium.org]

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/platform/network/ResourceRequest.cpp
File third_party/WebKit/Source/platform/network/ResourceRequest.cpp
(right):

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/platform/network/ResourceRequest.cpp#newcode400
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/platform/network/ResourceRequest.cpp#newcode405
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.

https://codereview.chromium.org/2390583002/

[shao...@chromium.org]

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/core/fetch/Resource.cpp
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/core/fetch/Resource.cpp#newcode1083
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.h?type=cs&sq=package:chromium&rcl=1475464636&l=36

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/platform/network/ResourceRequest.cpp
File third_party/WebKit/Source/platform/network/ResourceRequest.cpp
(right):

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/platform/network/ResourceRequest.cpp#newcode405
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/

[toyo...@chromium.org]

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/platform/network/ResourceRequest.cpp
File third_party/WebKit/Source/platform/network/ResourceRequest.cpp
(right):

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/platform/network/ResourceRequest.cpp#newcode405
third_party/WebKit/Source/platform/network/ResourceRequest.cpp:405:
m_cachePolicy = WebCachePolicy::ReturnCacheDataDontLoad;

How about using crbug.com/570205 for this TODO.

https://codereview.chromium.org/2390583002/

[yhi...@chromium.org]

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/core/fetch/Resource.cpp
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/core/fetch/Resource.cpp#newcode1081
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/core/fetch/Resource.h
File third_party/WebKit/Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/core/fetch/Resource.h#newcode290
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/core/fetch/Resource.h#newcode420
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/core/fetch/ResourceFetcher.cpp
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp#newcode1112
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/

[shao...@chromium.org]

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp
File third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp (right):

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp#newcode91
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/core/fetch/Resource.cpp
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/core/fetch/Resource.cpp#newcode1081
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/core/fetch/Resource.h
File third_party/WebKit/Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/core/fetch/Resource.h#newcode290
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/core/fetch/Resource.h#newcode420
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/core/fetch/ResourceFetcher.cpp
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp#newcode1112
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/platform/network/ResourceRequest.cpp
File third_party/WebKit/Source/platform/network/ResourceRequest.cpp
(right):

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/platform/network/ResourceRequest.cpp#newcode400
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/platform/network/ResourceRequest.cpp#newcode405
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/

[shao...@chromium.org]

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp#newcode1112
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.

https://codereview.chromium.org/2390583002/

[toyo...@chromium.org]

lg with nits, then defer to fetch owners.


https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp
File third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp (right):

https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp#newcode37
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/Source/core/fetch/Resource.cpp
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Source/core/fetch/Resource.cpp#newcode1092
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/Source/core/fetch/ResourceClient.h
File third_party/WebKit/Source/core/fetch/ResourceClient.h (right):

https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Source/core/fetch/ResourceClient.h#newcode55
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/Source/core/fetch/ResourceFetcher.cpp
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp#newcode1114
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.

https://codereview.chromium.org/2390583002/

[hiro...@chromium.org]

> 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/core/fetch/Resource.cpp
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/core/fetch/Resource.cpp#newcode1083
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.h?type=cs&sq=package:chromium&rcl=1475464636&l=36
>
> 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/Source/core/css/CSSFontFaceSrcValue.cpp
File third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp (right):

https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp#newcode92
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/Source/core/fetch/Resource.cpp
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Source/core/fetch/Resource.cpp#newcode420
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/Source/core/fetch/ResourceClient.h
File third_party/WebKit/Source/core/fetch/ResourceClient.h (right):

https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Source/core/fetch/ResourceClient.h#newcode55
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)

https://codereview.chromium.org/2390583002/

[shao...@chromium.org]

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/platform/network/ResourceRequest.cpp
File third_party/WebKit/Source/platform/network/ResourceRequest.cpp
(right):

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/platform/network/ResourceRequest.cpp#newcode405
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/Source/core/css/CSSFontFaceSrcValue.cpp
File third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp (right):

https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp#newcode37
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/Source/core/css/CSSFontFaceSrcValue.cpp#newcode92
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/Source/core/fetch/Resource.cpp
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Source/core/fetch/Resource.cpp#newcode1092
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/Source/core/fetch/ResourceClient.h
File third_party/WebKit/Source/core/fetch/ResourceClient.h (right):

https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Source/core/fetch/ResourceClient.h#newcode55
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/Source/core/fetch/ResourceFetcher.cpp
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp#newcode1114
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/

[yhi...@chromium.org]

https://codereview.chromium.org/2390583002/diff/60001/third_party/WebKit/Source/platform/network/ResourceRequest.cpp
File third_party/WebKit/Source/platform/network/ResourceRequest.cpp
(right):

https://codereview.chromium.org/2390583002/diff/60001/third_party/WebKit/Source/platform/network/ResourceRequest.cpp#newcode403
third_party/WebKit/Source/platform/network/ResourceRequest.cpp:403:
return;
DCHECK(!m_isCacheAwareLoadingActivated)?

https://codereview.chromium.org/2390583002/diff/60001/third_party/WebKit/Source/platform/network/ResourceRequest.cpp#newcode413
third_party/WebKit/Source/platform/network/ResourceRequest.cpp:413:
default:
This section is not needed.

https://codereview.chromium.org/2390583002/

[shao...@chromium.org]

- 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/core/fetch/Resource.cpp
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/core/fetch/Resource.cpp#newcode1083
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.h?type=cs&sq=package:chromium&rcl=1475464636&l=36
> >
> > 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/core/fetch/ResourceFetcher.cpp
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp#newcode1112
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:

Now dropping flags in didFinishLoading().

https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Source/core/fetch/Resource.cpp
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Source/core/fetch/Resource.cpp#newcode420
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/Source/core/fetch/ResourceFetcher.cpp
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2390583002/diff/40001/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp#newcode1114
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/Source/platform/network/ResourceRequest.cpp
File third_party/WebKit/Source/platform/network/ResourceRequest.cpp
(right):

https://codereview.chromium.org/2390583002/diff/60001/third_party/WebKit/Source/platform/network/ResourceRequest.cpp#newcode403
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/Source/platform/network/ResourceRequest.cpp#newcode413
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/

[yhi...@chromium.org]

https://codereview.chromium.org/2390583002/diff/60001/third_party/WebKit/Source/platform/network/ResourceRequest.cpp
File third_party/WebKit/Source/platform/network/ResourceRequest.cpp
(right):

https://codereview.chromium.org/2390583002/diff/60001/third_party/WebKit/Source/platform/network/ResourceRequest.cpp#newcode413
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.

https://codereview.chromium.org/2390583002/

[shao...@chromium.org]

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/Source/platform/network/ResourceRequest.cpp
File third_party/WebKit/Source/platform/network/ResourceRequest.cpp
(right):

https://codereview.chromium.org/2390583002/diff/60001/third_party/WebKit/Source/platform/network/ResourceRequest.cpp#newcode413
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/WebKit/Source/core/fetch/ResourceFetcher.cpp#oldcode113
.

https://codereview.chromium.org/2390583002/

[ksak...@chromium.org]

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/Source/core/css/RemoteFontFaceSource.cpp
File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp
(right):

https://codereview.chromium.org/2390583002/diff/140001/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp#newcode159
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.

https://codereview.chromium.org/2390583002/

[shao...@chromium.org]

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()?

[shao...@chromium.org]

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/Source/core/css/RemoteFontFaceSource.cpp
File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp
(right):

https://codereview.chromium.org/2390583002/diff/140001/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp#newcode159
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.

https://codereview.chromium.org/2390583002/

[ksak...@chromium.org]

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/Source/core/css/RemoteFontFaceSource.cpp
File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp
(right):

https://codereview.chromium.org/2390583002/diff/160001/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp#newcode168
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?

https://codereview.chromium.org/2390583002/

[shao...@chromium.org]

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.

[ksak...@chromium.org]

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.

https://codereview.chromium.org/2390583002/

[shao...@chromium.org]

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.

https://codereview.chromium.org/2390583002/

[shao...@chromium.org]

font-display: swap handling was missing in PS8. We still need an additional
callback in RemoteFontFaceSource to switchToSwapPeriod() in this case.

https://codereview.chromium.org/2390583002/

[ksak...@chromium.org]

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/Source/core/css/RemoteFontFaceSource.cpp
File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp
(right):

https://codereview.chromium.org/2390583002/diff/200001/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp#newcode59
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/

[yhi...@chromium.org]

https://codereview.chromium.org/2390583002/diff/200001/third_party/WebKit/Source/core/fetch/FontResource.cpp
File third_party/WebKit/Source/core/fetch/FontResource.cpp (right):

https://codereview.chromium.org/2390583002/diff/200001/third_party/WebKit/Source/core/fetch/FontResource.cpp#newcode156
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.

https://codereview.chromium.org/2390583002/

[shao...@chromium.org]

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/Source/core/css/RemoteFontFaceSource.cpp
File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp
(right):

https://codereview.chromium.org/2390583002/diff/200001/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp#newcode59
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/

[ksak...@chromium.org]

https://codereview.chromium.org/2390583002/diff/200001/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp
File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp
(right):

https://codereview.chromium.org/2390583002/diff/200001/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp#newcode59
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/Source/core/fetch/FontResource.cpp
File third_party/WebKit/Source/core/fetch/FontResource.cpp (right):

https://codereview.chromium.org/2390583002/diff/200001/third_party/WebKit/Source/core/fetch/FontResource.cpp#newcode42
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/

[shao...@chromium.org]

https://codereview.chromium.org/2390583002/diff/200001/third_party/WebKit/Source/core/fetch/FontResource.cpp
File third_party/WebKit/Source/core/fetch/FontResource.cpp (right):

https://codereview.chromium.org/2390583002/diff/200001/third_party/WebKit/Source/core/fetch/FontResource.cpp#newcode156
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.

https://codereview.chromium.org/2390583002/

[yhi...@chromium.org]

Can you add comments as I asked at [1] and [2]?

1:
https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/core/fetch/Resource.h#newcode290
2:
https://codereview.chromium.org/2390583002/diff/200001/third_party/WebKit/Source/core/fetch/FontResource.cpp#newcode156

You added comments for [1] but it looks removed after that...


https://codereview.chromium.org/2390583002/

[shao...@chromium.org]

Oops, [1] was removed during refactoring. Re-added comments.

https://codereview.chromium.org/2390583002/diff/200001/third_party/WebKit/Source/core/fetch/FontResource.cpp
File third_party/WebKit/Source/core/fetch/FontResource.cpp (right):

https://codereview.chromium.org/2390583002/diff/200001/third_party/WebKit/Source/core/fetch/FontResource.cpp#newcode42
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/Source/core/fetch/FontResource.cpp#newcode156
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/Source/platform/RuntimeEnabledFeatures.in
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
(right):

https://codereview.chromium.org/2390583002/diff/200001/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in#newcode255
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

https://codereview.chromium.org/2390583002/

[shao...@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.

https://codereview.chromium.org/2390583002/

[shao...@chromium.org]

@tkent: @haraken is on a business trip, could you take a look at changes under
platform/? Thanks.

https://codereview.chromium.org/2390583002/

[tk...@chromium.org]

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/

[Shao-Chuan Lee]

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.

[shao...@chromium.org]

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/

[kou...@chromium.org]

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/core/fetch/Resource.cpp
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2390583002/diff/1/third_party/WebKit/Source/core/fetch/Resource.cpp#newcode1083
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.h?type=cs&sq=package:chromium&rcl=1475464636&l=36
>
> 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=ForbiddenScope+file:webkit&sq=package:chromium&dr=CSs&l=992

https://codereview.chromium.org/2390583002/diff/320001/content/child/web_url_request_util.cc
File content/child/web_url_request_util.cc (right):

https://codereview.chromium.org/2390583002/diff/320001/content/child/web_url_request_util.cc#newcode490
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/Source/core/fetch/FontResource.cpp
File third_party/WebKit/Source/core/fetch/FontResource.cpp (right):

https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Source/core/fetch/FontResource.cpp#newcode42
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/Source/core/fetch/Resource.h
File third_party/WebKit/Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Source/core/fetch/Resource.h#newcode306
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/Source/core/fetch/ResourceLoader.cpp
File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Source/core/fetch/ResourceLoader.cpp#newcode143
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/Source/platform/network/ResourceRequest.cpp
File third_party/WebKit/Source/platform/network/ResourceRequest.cpp
(right):

https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Source/platform/network/ResourceRequest.cpp#newcode416
third_party/WebKit/Source/platform/network/ResourceRequest.cpp:416:
break;
default:
ASSERT_NOT_REACHED()?

https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Source/platform/network/ResourceRequest.cpp#newcode427
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/

[yhi...@chromium.org]

https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Source/platform/network/ResourceRequest.cpp
File third_party/WebKit/Source/platform/network/ResourceRequest.cpp
(right):

https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Source/platform/network/ResourceRequest.cpp#newcode416
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/Source/platform/network/ResourceRequest.cpp#newcode413

https://codereview.chromium.org/2390583002/

[kou...@chromium.org]

https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Source/platform/network/ResourceRequest.cpp
File third_party/WebKit/Source/platform/network/ResourceRequest.cpp
(right):

https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Source/platform/network/ResourceRequest.cpp#newcode416
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/Source/platform/network/ResourceRequest.cpp#newcode413

Got it. Please follow yhirano-san.

https://codereview.chromium.org/2390583002/

[yhi...@chromium.org]

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/

[shao...@chromium.org]

https://codereview.chromium.org/2390583002/diff/320001/content/child/web_url_request_util.cc
File content/child/web_url_request_util.cc (right):

https://codereview.chromium.org/2390583002/diff/320001/content/child/web_url_request_util.cc#newcode490
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/Source/core/fetch/FontResource.cpp
File third_party/WebKit/Source/core/fetch/FontResource.cpp (right):

https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Source/core/fetch/FontResource.cpp#newcode42
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/Source/core/fetch/Resource.h
File third_party/WebKit/Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Source/core/fetch/Resource.h#newcode306
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/Source/core/fetch/ResourceLoader.cpp
File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Source/core/fetch/ResourceLoader.cpp#newcode143
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/exported/WebURLError.cpp?rcl=0&l=58
Blink does not interpret the meaning of reason, as suggested in
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebURLError.h?rcl=0&l=48

https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Source/platform/network/ResourceRequest.cpp
File third_party/WebKit/Source/platform/network/ResourceRequest.cpp
(right):

https://codereview.chromium.org/2390583002/diff/320001/third_party/WebKit/Source/platform/network/ResourceRequest.cpp#newcode427
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.

https://codereview.chromium.org/2390583002/

[shao...@chromium.org]

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/

[kou...@chromium.org]

https://codereview.chromium.org/2390583002/diff/320001/content/child/web_url_request_util.cc
File content/child/web_url_request_util.cc (right):

https://codereview.chromium.org/2390583002/diff/320001/content/child/web_url_request_util.cc#newcode490
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/

[shao...@chromium.org]

https://codereview.chromium.org/2390583002/diff/320001/content/child/web_url_request_util.cc
File content/child/web_url_request_util.cc (right):

https://codereview.chromium.org/2390583002/diff/320001/content/child/web_url_request_util.cc#newcode490
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?

https://codereview.chromium.org/2390583002/

[kou...@chromium.org]

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/network/DEPS?sq=package:chromium&dr

Let me create a CL

https://codereview.chromium.org/2390583002/

[ksak...@chromium.org]

https://codereview.chromium.org/2390583002/diff/340001/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp
File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp
(right):

https://codereview.chromium.org/2390583002/diff/340001/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp#newcode253
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.

https://codereview.chromium.org/2390583002/

[yhi...@chromium.org]

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.

https://codereview.chromium.org/2390583002/

[shao...@chromium.org]

Having a method in blink::NetworkUtils should be nice?

https://codereview.chromium.org/2390583002/

[kou...@chromium.org]

sgtm

https://codereview.chromium.org/2390583002/

[shao...@chromium.org]

Now having utility methods under blink::NetworkUtils to workaround this.
PTAL

https://codereview.chromium.org/2390583002/

[kou...@chromium.org]

lgtm from myside

https://codereview.chromium.org/2390583002/

[shao...@chromium.org]

https://codereview.chromium.org/2390583002/diff/340001/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp
File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp
(right):

https://codereview.chromium.org/2390583002/diff/340001/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp#newcode253
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/

[kin...@chromium.org]

https://codereview.chromium.org/2390583002/diff/400001/third_party/WebKit/Source/platform/exported/WebURLError.cpp
File third_party/WebKit/Source/platform/exported/WebURLError.cpp
(right):

https://codereview.chromium.org/2390583002/diff/400001/third_party/WebKit/Source/platform/exported/WebURLError.cpp#newcode71
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/

[kin...@chromium.org]

https://codereview.chromium.org/2390583002/diff/400001/third_party/WebKit/Source/core/fetch/ResourceLoader.cpp
File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/2390583002/diff/400001/third_party/WebKit/Source/core/fetch/ResourceLoader.cpp#newcode142
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/Source/platform/exported/WebURLError.cpp
File third_party/WebKit/Source/platform/exported/WebURLError.cpp
(right):

https://codereview.chromium.org/2390583002/diff/400001/third_party/WebKit/Source/platform/exported/WebURLError.cpp#newcode71
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/

[shao...@chromium.org]

https://codereview.chromium.org/2390583002/diff/400001/third_party/WebKit/Source/core/fetch/ResourceLoader.cpp
File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/2390583002/diff/400001/third_party/WebKit/Source/core/fetch/ResourceLoader.cpp#newcode142
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/Source/platform/exported/WebURLError.cpp
File third_party/WebKit/Source/platform/exported/WebURLError.cpp
(right):

https://codereview.chromium.org/2390583002/diff/400001/third_party/WebKit/Source/platform/exported/WebURLError.cpp#newcode71
third_party/WebKit/Source/platform/exported/WebURLError.cpp:71:
error.reason = NetworkUtils::cacheMissErrorCode();

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?

https://codereview.chromium.org/2390583002/

[shao...@chromium.org]

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/

[yhi...@chromium.org]

https://codereview.chromium.org/2390583002/diff/440001/third_party/WebKit/Source/platform/network/ResourceError.cpp
File third_party/WebKit/Source/platform/network/ResourceError.cpp
(right):

https://codereview.chromium.org/2390583002/diff/440001/third_party/WebKit/Source/platform/network/ResourceError.cpp#newcode51
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/Source/platform/network/ResourceError.cpp#newcode73
third_party/WebKit/Source/platform/network/ResourceError.cpp:73: return
m_errorCode == net::ERR_CACHE_MISS;
We need to check m_domain as well.

https://codereview.chromium.org/2390583002/

[shao...@chromium.org]

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.

https://codereview.chromium.org/2390583002/