Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2095293002:
WebFonts intervention: Add metrics only for miss-cached loading (Closed)
|
Created:
4 years, 5 months ago by Takashi Toyoshima Modified:
4 years, 5 months ago CC:
chromium-reviews, tyoshino+watch_chromium.org, blink-reviews-css, dglazkov+blink, apavlov+blink_chromium.org, gavinp+loader_chromium.org, darktears, blink-reviews, loading-reviews+fetch_chromium.org, Nate Chapin, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWebFonts intervention: Add metrics only for miss-cached loading
Now the intervention is triggered on slow networks, but it also
triggered even if data come from the http (disk) or memory caches.
Ideally, we should not trigger the intervention in such cases,
but at this point, we take metrics that are counted only when
data come from network so that we can evaluate the NQE's signal accuracy.
Also this patch stop triggering the intervention when data come
from memory cache because this check is trivial unlike disk cache.
BUG=578029
Committed: https://crrev.com/d1023bafc3daf0d9ed5f47f5f781ea0888f57bf7
Cr-Commit-Position: refs/heads/master@{#402416}
Patch Set 1 #Patch Set 2 : v2 #Patch Set 3 : check memory cache too #
Total comments: 8
Patch Set 4 : kinuko review #
Messages
Total messages: 23 (10 generated)
Description was changed from ========== WebFonts intervention: Add metrics only for miss-cached loading Now the intervention is triggered on slow networks, but it also triggered even if the data is cached in the http (disk) cache. Ideally, we should not trigger the intervention in such cases, but at this point, we take metrics that are counted only when the data comes from network so that we can evaludate the NQE's signal accuracy. BUG=578029 ========== to ========== WebFonts intervention: Add metrics only for miss-cached loading Now the intervention is triggered on slow networks, but it also triggered even if the data is cached in the http (disk) cache. Ideally, we should not trigger the intervention in such cases, but at this point, we take metrics that are counted only when the data comes from network so that we can evaludate the NQE's signal accuracy. BUG=578029 ==========
toyoshim@chromium.org changed reviewers: + ksakamoto@chromium.org
ptal
https://codereview.chromium.org/2095293002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right): https://codereview.chromium.org/2095293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:120: m_histograms.fontLoaded(m_isInterventionTriggered, m_isLoadedFromMemoryCache || m_font->response().wasCached()); wasCached() returns disk cache result. So, if the first fetch comes from network, m_isLoadedFromMemoryCache and wasCached() are false. Then the next fetch will be m_isLoadedFromMemoryCache is true and wasCached() was false because the same response() is cached in the memory cache. If the first fetch comes from disk cache, wasCached() is true for both first and the second fetches.
https://codereview.chromium.org/2095293002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right): https://codereview.chromium.org/2095293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:46: return false; So, we no longer trigger the intervention for memory-cached fonts -- could you update the CL description for this behavior change?
Description was changed from ========== WebFonts intervention: Add metrics only for miss-cached loading Now the intervention is triggered on slow networks, but it also triggered even if the data is cached in the http (disk) cache. Ideally, we should not trigger the intervention in such cases, but at this point, we take metrics that are counted only when the data comes from network so that we can evaludate the NQE's signal accuracy. BUG=578029 ========== to ========== WebFonts intervention: Add metrics only for miss-cached loading Now the intervention is triggered on slow networks, but it also triggered even if data come from the http (disk) or memory caches. Ideally, we should not trigger the intervention in such cases, but at this point, we take metrics that are counted only when data come from network so that we can evaluate the NQE's signal accuracy. Also this patch stop triggering the intervention when data come from memory cache. BUG=578029 ==========
Description was changed from ========== WebFonts intervention: Add metrics only for miss-cached loading Now the intervention is triggered on slow networks, but it also triggered even if data come from the http (disk) or memory caches. Ideally, we should not trigger the intervention in such cases, but at this point, we take metrics that are counted only when data come from network so that we can evaluate the NQE's signal accuracy. Also this patch stop triggering the intervention when data come from memory cache. BUG=578029 ========== to ========== WebFonts intervention: Add metrics only for miss-cached loading Now the intervention is triggered on slow networks, but it also triggered even if data come from the http (disk) or memory caches. Ideally, we should not trigger the intervention in such cases, but at this point, we take metrics that are counted only when data come from network so that we can evaluate the NQE's signal accuracy. Also this patch stop triggering the intervention when data come from memory cache because this check is trivial unlike disk cache. BUG=578029 ==========
https://codereview.chromium.org/2095293002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right): https://codereview.chromium.org/2095293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:46: return false; On 2016/06/27 09:07:45, Kunihiko Sakamoto wrote: > So, we no longer trigger the intervention for memory-cached fonts -- could you > update the CL description for this behavior change? Done.
lgtm
toyoshim@chromium.org changed reviewers: + kinuko@chromium.org
This change was reviewed by ksakamoto. Kinuko, could you take a owner review?
lgtm % nits https://codereview.chromium.org/2095293002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/RemoteFontFaceSource.h (right): https://codereview.chromium.org/2095293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/RemoteFontFaceSource.h:67: void fontLoaded(bool isInterventionTriggered, bool isLoadedFromCache); usually prefer enum over bool... but ok this is internal histogram class https://codereview.chromium.org/2095293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/RemoteFontFaceSource.h:74: void recordInterventionResult(bool triggered, bool loadedFromCache); loadedFromCache -> isLoadedFromCache for consistency?
tbansal@chromium.org changed reviewers: + tbansal@chromium.org
toyoshim, thanks for doing this. One question below. https://codereview.chromium.org/2095293002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right): https://codereview.chromium.org/2095293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:330: if (!loadedFromCache) Will the accuracy histogram be logged if the font was base-64 encoded within the file (like a data URL)?
https://codereview.chromium.org/2095293002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/RemoteFontFaceSource.h (right): https://codereview.chromium.org/2095293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/RemoteFontFaceSource.h:74: void recordInterventionResult(bool triggered, bool loadedFromCache); On 2016/06/27 12:06:24, kinuko wrote: > loadedFromCache -> isLoadedFromCache for consistency? Done with triggered -> isTriggered.
https://codereview.chromium.org/2095293002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right): https://codereview.chromium.org/2095293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:330: if (!loadedFromCache) That's good point. Let me check if we do not trigger the intervention for data URL after submitting this CL. I expect font->isLoaded() returns true at the constructor. Once I check the behavior, I will report the result at the crbug.
The CQ bit was checked by toyoshim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, ksakamoto@chromium.org Link to the patchset: https://codereview.chromium.org/2095293002/#ps60001 (title: "kinuko review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== WebFonts intervention: Add metrics only for miss-cached loading Now the intervention is triggered on slow networks, but it also triggered even if data come from the http (disk) or memory caches. Ideally, we should not trigger the intervention in such cases, but at this point, we take metrics that are counted only when data come from network so that we can evaluate the NQE's signal accuracy. Also this patch stop triggering the intervention when data come from memory cache because this check is trivial unlike disk cache. BUG=578029 ========== to ========== WebFonts intervention: Add metrics only for miss-cached loading Now the intervention is triggered on slow networks, but it also triggered even if data come from the http (disk) or memory caches. Ideally, we should not trigger the intervention in such cases, but at this point, we take metrics that are counted only when data come from network so that we can evaluate the NQE's signal accuracy. Also this patch stop triggering the intervention when data come from memory cache because this check is trivial unlike disk cache. BUG=578029 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== WebFonts intervention: Add metrics only for miss-cached loading Now the intervention is triggered on slow networks, but it also triggered even if data come from the http (disk) or memory caches. Ideally, we should not trigger the intervention in such cases, but at this point, we take metrics that are counted only when data come from network so that we can evaluate the NQE's signal accuracy. Also this patch stop triggering the intervention when data come from memory cache because this check is trivial unlike disk cache. BUG=578029 ========== to ========== WebFonts intervention: Add metrics only for miss-cached loading Now the intervention is triggered on slow networks, but it also triggered even if data come from the http (disk) or memory caches. Ideally, we should not trigger the intervention in such cases, but at this point, we take metrics that are counted only when data come from network so that we can evaluate the NQE's signal accuracy. Also this patch stop triggering the intervention when data come from memory cache because this check is trivial unlike disk cache. BUG=578029 Committed: https://crrev.com/d1023bafc3daf0d9ed5f47f5f781ea0888f57bf7 Cr-Commit-Position: refs/heads/master@{#402416} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/d1023bafc3daf0d9ed5f47f5f781ea0888f57bf7 Cr-Commit-Position: refs/heads/master@{#402416} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
