Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2443883003:
WebFonts intervention: understand if download time matches the decision (Closed)
|
Created:
4 years, 1 month ago by Takashi Toyoshima Modified:
4 years, 1 month ago CC:
chromium-reviews, blink-reviews-css, dglazkov+blink, apavlov+blink_chromium.org, darktears, blink-reviews, rwlbuis Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWebFonts intervention: understand if download time matches the decision
To understand download time correlation with intervention decision,
breakdown download time metrics per intervention decision.
We have download time metrics per data size, but this change adds only
10KB to 50KB range since it covers about 70% cases.
BUG=659371
Committed: https://crrev.com/3c3890ee4f2132a4a5e35ce9a2ceff73cac1b80e
Cr-Commit-Position: refs/heads/master@{#428072}
Patch Set 1 #Patch Set 2 : with metrics.xml #
Total comments: 2
Patch Set 3 : remove TODO #
Total comments: 1
Patch Set 4 : merge suffixes elements #
Messages
Total messages: 25 (14 generated)
Description was changed from ========== Not for submit now; WebFonts intervention: understand if download matches the decision To understand download time correlation with intervention decision, breakdown download time metrics per intervention decision. We have download time metrics per data size, but this change adds only 10KB to 50KB range since it covers about 70% cases. BUG= ========== to ========== Not for submit now; WebFonts intervention: understand if download matches the decision To understand download time correlation with intervention decision, breakdown download time metrics per intervention decision. We have download time metrics per data size, but this change adds only 10KB to 50KB range since it covers about 70% cases. BUG= ==========
toyoshim@chromium.org changed reviewers: + ksakamoto@chromium.org, tbansal@chromium.org
What do you think about this metrics? 70% of my comment comes from https://docs.google.com/document/d/1f2tcvW72A9haFkCNsVhKgi1iteNrCRHYA7Y_uIaut... tbansal: we chatted about adding breakdown loadtime per each InterventionResult case, but after looking actual code, probably this will work better than the original idea. do you have any concen about this? ktsakamot: what do you think about this change? also, I tried to remove isInterventionTriggerd argument and pass it in m_histograms' constructor. But I can not make it without using a triky code or making it as a pointer. If I can make it without any cost, I'd make it pass in the constructor.
lgtm
lgtm
Description was changed from ========== Not for submit now; WebFonts intervention: understand if download matches the decision To understand download time correlation with intervention decision, breakdown download time metrics per intervention decision. We have download time metrics per data size, but this change adds only 10KB to 50KB range since it covers about 70% cases. BUG= ========== to ========== WebFonts intervention: understand if download matches the decision To understand download time correlation with intervention decision, breakdown download time metrics per intervention decision. We have download time metrics per data size, but this change adds only 10KB to 50KB range since it covers about 70% cases. BUG= ==========
toyoshim@chromium.org changed reviewers: + isherman@chromium.org, kinuko@chromium.org
Description was changed from ========== WebFonts intervention: understand if download matches the decision To understand download time correlation with intervention decision, breakdown download time metrics per intervention decision. We have download time metrics per data size, but this change adds only 10KB to 50KB range since it covers about 70% cases. BUG= ========== to ========== WebFonts intervention: understand if download matches the decision To understand download time correlation with intervention decision, breakdown download time metrics per intervention decision. We have download time metrics per data size, but this change adds only 10KB to 50KB range since it covers about 70% cases. BUG=659371 ==========
Since you guys look fine with this change, I revised it to have a right fix for histograms.xml. Now it's ready for owners' review. isherman for metrics kinuko for blink Can you take a look?
lgtm https://codereview.chromium.org/2443883003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right): https://codereview.chromium.org/2443883003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:369: // TODO(toyoshim): edit histograms.xml. do we still need this TODO?
https://codereview.chromium.org/2443883003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right): https://codereview.chromium.org/2443883003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:369: // TODO(toyoshim): edit histograms.xml. oops, thank you for catching this.
https://codereview.chromium.org/2443883003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2443883003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:111385: +</histogram_suffixes> Could these two histogram_suffixes elements be combined into one, with multiple <suffix> elements listed?
On 2016/10/26 22:45:10, Ilya Sherman wrote: > https://codereview.chromium.org/2443883003/diff/40001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2443883003/diff/40001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:111385: +</histogram_suffixes> > Could these two histogram_suffixes elements be combined into one, with multiple > <suffix> elements listed? histograms.xml lgtm other than that
The CQ bit was checked by toyoshim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== WebFonts intervention: understand if download matches the decision To understand download time correlation with intervention decision, breakdown download time metrics per intervention decision. We have download time metrics per data size, but this change adds only 10KB to 50KB range since it covers about 70% cases. BUG=659371 ========== to ========== WebFonts intervention: understand if download time matches the decision To understand download time correlation with intervention decision, breakdown download time metrics per intervention decision. We have download time metrics per data size, but this change adds only 10KB to 50KB range since it covers about 70% cases. BUG=659371 ==========
The CQ bit was checked by toyoshim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ksakamoto@chromium.org, tbansal@chromium.org, kinuko@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2443883003/#ps60001 (title: "merge suffixes elements")
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: understand if download time matches the decision To understand download time correlation with intervention decision, breakdown download time metrics per intervention decision. We have download time metrics per data size, but this change adds only 10KB to 50KB range since it covers about 70% cases. BUG=659371 ========== to ========== WebFonts intervention: understand if download time matches the decision To understand download time correlation with intervention decision, breakdown download time metrics per intervention decision. We have download time metrics per data size, but this change adds only 10KB to 50KB range since it covers about 70% cases. BUG=659371 ==========
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: understand if download time matches the decision To understand download time correlation with intervention decision, breakdown download time metrics per intervention decision. We have download time metrics per data size, but this change adds only 10KB to 50KB range since it covers about 70% cases. BUG=659371 ========== to ========== WebFonts intervention: understand if download time matches the decision To understand download time correlation with intervention decision, breakdown download time metrics per intervention decision. We have download time metrics per data size, but this change adds only 10KB to 50KB range since it covers about 70% cases. BUG=659371 Committed: https://crrev.com/3c3890ee4f2132a4a5e35ce9a2ceff73cac1b80e Cr-Commit-Position: refs/heads/master@{#428072} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/3c3890ee4f2132a4a5e35ce9a2ceff73cac1b80e Cr-Commit-Position: refs/heads/master@{#428072} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
