Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2474943002:
doc.write intervention warning message changed to replace cross-origin with cross site (etdl+1) (Closed)
|
Created:
4 years, 1 month ago by shivanisha Modified:
3 years, 10 months ago CC:
blink-reviews, chromium-reviews, gavinp+loader_chromium.org, loading-reviews_chromium.org, tyoshino+watch_chromium.org, Yoav Weiss Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptiondoc.write intervention warning message changed to replace cross-origin with cross
site (etld+1)
BUG=658983
Review-Url: https://codereview.chromium.org/2474943002
Cr-Commit-Position: refs/heads/master@{#446993}
Committed: https://chromium.googlesource.com/chromium/src/+/52c2ea5fbafcf7efc85e8f8fc4865073f3b6aaf1
Patch Set 1 #Patch Set 2 : Added the histogram for same site with different scheme. #
Total comments: 11
Patch Set 3 : Feedback incorporated. #Patch Set 4 : Rebased till refs/heads/master@{#444422} #Patch Set 5 : Rebased till refs/heads/master@{#444737} #Patch Set 6 : Rebased till refs/heads/master@{#445735} #Patch Set 7 : Rebased till refs/heads/master@{#446099} #
Total comments: 3
Patch Set 8 : Converted to UMA_HISTOGRAM_ENUMERATION #
Total comments: 2
Patch Set 9 : Bot failures fixed. #
Total comments: 1
Patch Set 10 : Added a helper function #Messages
Total messages: 100 (68 generated)
The CQ bit was checked by shivanisha@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 ========== doc.write intervention warning message changed to replace cross-origin with cross site (etdl+1) BUG=658983 ========== to ========== japhet@, PTAL, thanks! doc.write intervention warning message changed to replace cross-origin with cross site (etdl+1) BUG=658983 ==========
shivanisha@chromium.org changed reviewers: + japhet@chromium.org
Description was changed from ========== japhet@, PTAL, thanks! doc.write intervention warning message changed to replace cross-origin with cross site (etdl+1) BUG=658983 ========== to ========== doc.write intervention warning message changed to replace cross-origin with cross site (etdl+1) BUG=658983 ==========
Description was changed from ========== doc.write intervention warning message changed to replace cross-origin with cross site (etdl+1) BUG=658983 ========== to ========== doc.write intervention warning message changed to replace cross-origin with cross site (etdl+1) BUG=658983 ==========
On 2016/11/03 17:03:33, shivanisha wrote: > Description was changed from > > ========== > doc.write intervention warning message changed to replace cross-origin with > cross site (etdl+1) > BUG=658983 > ========== > > to > > ========== > doc.write intervention warning message changed to replace cross-origin with > cross > site (etdl+1) > BUG=658983 > ========== As I commented on the intent to implement, can we make the use of the term "cross-site" mean the same thing across Chromium? Currently, the intervention only takes into account etld + 1, which does not match the term "cross site" as used elsewhere in Chromium (which is etld + 1 *and* scheme).
The CQ bit was checked by shivanisha@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 checked by shivanisha@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
shivanisha@chromium.org changed reviewers: + jkarlin@chromium.org
Added a histogram to help decide whether we should also check same scheme while deciding not to block the script as is done in other cases of "same site" usage. On the other hand we do not want to block the scripts very aggressively causing page breaks. Thus, will see the counts before deciding. jkarlin@, PTAL, Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2474943002/diff/40001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.h (right): https://codereview.chromium.org/2474943002/diff/40001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.h:66: bool doc_write_block_same_site_diff_scheme_; Prefer to set these two bools to false here instead of the constructor. https://codereview.chromium.org/2474943002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/2474943002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:86: "A Parser-blocking, cross site (i.e. different eTLD+1) script, " + url + s/cross site/cross-site/ https://codereview.chromium.org/2474943002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:160: // same scheme while deciding not to block the script as is done in other s/deciding not to block/deciding whether or not to block/ https://codereview.chromium.org/2474943002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:162: // scripts very aggressively causing page breaks. Thus, will see the counts s/the scripts very aggressively causing page breaks/more scripts than necessary/ https://codereview.chromium.org/2474943002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:162: // scripts very aggressively causing page breaks. Thus, will see the counts Final sentence can go, that's made clear from the first sentence. https://codereview.chromium.org/2474943002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2474943002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41423: + This metric measures the number of pages that have same-site but different Add commas or parens around "but different scheme"
The CQ bit was checked by shivanisha@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...
https://codereview.chromium.org/2474943002/diff/40001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.h (right): https://codereview.chromium.org/2474943002/diff/40001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.h:66: bool doc_write_block_same_site_diff_scheme_; On 2016/11/29 at 16:19:12, jkarlin wrote: > Prefer to set these two bools to false here instead of the constructor. Done. https://codereview.chromium.org/2474943002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/2474943002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:86: "A Parser-blocking, cross site (i.e. different eTLD+1) script, " + url + On 2016/11/29 at 16:19:12, jkarlin wrote: > s/cross site/cross-site/ done https://codereview.chromium.org/2474943002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:160: // same scheme while deciding not to block the script as is done in other On 2016/11/29 at 16:19:12, jkarlin wrote: > s/deciding not to block/deciding whether or not to block/ done. https://codereview.chromium.org/2474943002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:162: // scripts very aggressively causing page breaks. Thus, will see the counts On 2016/11/29 at 16:19:12, jkarlin wrote: > Final sentence can go, that's made clear from the first sentence. Done https://codereview.chromium.org/2474943002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2474943002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41423: + This metric measures the number of pages that have same-site but different On 2016/11/29 at 16:19:12, jkarlin wrote: > Add commas or parens around "but different scheme" Done
On 2016/12/06 18:18:08, shivanisha wrote: > https://codereview.chromium.org/2474943002/diff/40001/chrome/browser/page_loa... > File > chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.h > (right): > > https://codereview.chromium.org/2474943002/diff/40001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.h:66: > bool doc_write_block_same_site_diff_scheme_; > On 2016/11/29 at 16:19:12, jkarlin wrote: > > Prefer to set these two bools to false here instead of the constructor. > > Done. > > https://codereview.chromium.org/2474943002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): > > https://codereview.chromium.org/2474943002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:86: "A > Parser-blocking, cross site (i.e. different eTLD+1) script, " + url + > On 2016/11/29 at 16:19:12, jkarlin wrote: > > s/cross site/cross-site/ > > done > > https://codereview.chromium.org/2474943002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:160: // same scheme > while deciding not to block the script as is done in other > On 2016/11/29 at 16:19:12, jkarlin wrote: > > s/deciding not to block/deciding whether or not to block/ > > done. > > https://codereview.chromium.org/2474943002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:162: // scripts very > aggressively causing page breaks. Thus, will see the counts > On 2016/11/29 at 16:19:12, jkarlin wrote: > > Final sentence can go, that's made clear from the first sentence. > > Done > > https://codereview.chromium.org/2474943002/diff/40001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2474943002/diff/40001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:41423: + This metric measures the > number of pages that have same-site but different > On 2016/11/29 at 16:19:12, jkarlin wrote: > > Add commas or parens around "but different scheme" > > Done If jkarlin's happy, I'm happy.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
So sorry for being slow! lgtm.
On 2017/01/09 at 15:29:50, jkarlin wrote: > So sorry for being slow! lgtm. japhet@, jkarlin has lgtm'ed. PTAL, Thanks!
lgtm
Description was changed from ========== doc.write intervention warning message changed to replace cross-origin with cross site (etdl+1) BUG=658983 ========== to ========== doc.write intervention warning message changed to replace cross-origin with cross site (etld+1) BUG=658983 ==========
bmcquade@chromium.org changed reviewers: + bmcquade@chromium.org
Glad to see this change! Are we ok to land it? Would be great to have this before M57 branch point.
bmcquade@chromium.org changed reviewers: - bmcquade@chromium.org
The CQ bit was checked by shivanisha@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...
On 2017/01/18 at 18:12:03, bmcquade wrote: > Glad to see this change! Are we ok to land it? Would be great to have this before M57 branch point. Yes, I am working on landing this. The commit bots are running now.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by shivanisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org, jkarlin@chromium.org Link to the patchset: https://codereview.chromium.org/2474943002/#ps80001 (title: "Rebased till refs/heads/master@{#444422}")
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
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
On 2017/01/18 at 22:34:27, commit-bot wrote: > Try jobs failed on following builders: > win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) Retrying the failed bots. The failures do not look related to this change. If not successful, will try for a merge after branch cut.
The CQ bit was checked by shivanisha@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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by shivanisha@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: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by shivanisha@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...
Some of the bot failures are being tracked via https://bugs.chromium.org/p/chromium/issues/detail?id=684573 Once they are fixed, hopefully commit will proceed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by shivanisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org, jkarlin@chromium.org Link to the patchset: https://codereview.chromium.org/2474943002/#ps120001 (title: "Rebased till refs/heads/master@{#445735}")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
shivanisha@chromium.org changed reviewers: + holte@chromium.org
On 2017/01/25 at 20:39:17, commit-bot wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) holte@, PTAL for histograms.xml, Thanks!
The CQ bit was checked by shivanisha@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...
https://codereview.chromium.org/2474943002/diff/140001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2474943002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:144: UMA_HISTOGRAM_COUNTS( If you are only ever recording a sample value of 1 to this histogram, it would be better to use a UMA_HISTOGRAM_ENUMERATION with only 1 bucket. This is creating a 50 bucket histogram, and only recording to one value. Even better would be to consolidate it with the other single bucket histograms above into a single enumerated histogram.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2474943002/diff/140001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2474943002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:144: UMA_HISTOGRAM_COUNTS( Ah, I see, UMA_HISTOGRAM_COUNTS creates a 50 bucket histogram! Would merging the above histogram PageLoad.Clients.DocWrite.Block.ReloadCount and the new one in an enumeration affect the ongoing Finch experiment for which PageLoad.Clients.DocWrite.Block.ReloadCount is an important metric. Does an enumeration enable to see each bucket's values at all major percentiles? If it affects the ongoing finch trial in any way then I will probably just make the new histogram an enum. WDYT?
On 2017/01/26 at 21:32:22, shivanisha wrote: > https://codereview.chromium.org/2474943002/diff/140001/chrome/browser/page_lo... > File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc (right): > > https://codereview.chromium.org/2474943002/diff/140001/chrome/browser/page_lo... > chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:144: UMA_HISTOGRAM_COUNTS( > Ah, I see, UMA_HISTOGRAM_COUNTS creates a 50 bucket histogram! > Would merging the above histogram PageLoad.Clients.DocWrite.Block.ReloadCount and the new one in an enumeration affect the ongoing Finch experiment for which PageLoad.Clients.DocWrite.Block.ReloadCount is an important metric. Does an enumeration enable to see each bucket's values at all major percentiles? If it affects the ongoing finch trial in any way then I will probably just make the new histogram an enum. WDYT? Bryan, As per holte@'s feedback, after some clarification, I might be converting PageLoad.Clients.DocWrite.Block.ReloadCount and PageLoad.Clients.DocWrite.Block.SameSiteDiffSchemeCount to an enumerated histogram. Do you see any concerns in changing the PageLoad.Clients.DocWrite.Block.ReloadCount histogram to an enum because of the impact it might have on the ongoing finch experiment? Also, was there a specific reason to use UMA_HISTOGRAM_BOOLEAN for PageLoad.Clients.DocWrite.Block.Count in https://codereview.chromium.org/2644543002 ? Can we use UMA_HISTOGRAM_ENUMERATION for this as well in the same enum as the above two histograms (if it does not impact the existing finch experiment)? - Shivani
https://codereview.chromium.org/2474943002/diff/140001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2474943002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:144: UMA_HISTOGRAM_COUNTS( On 2017/01/26 21:32:22, shivanisha wrote: > Ah, I see, UMA_HISTOGRAM_COUNTS creates a 50 bucket histogram! > Would merging the above histogram PageLoad.Clients.DocWrite.Block.ReloadCount > and the new one in an enumeration affect the ongoing Finch experiment for which > PageLoad.Clients.DocWrite.Block.ReloadCount is an important metric. Does an > enumeration enable to see each bucket's values at all major percentiles? If it > affects the ongoing finch trial in any way then I will probably just make the > new histogram an enum. WDYT? If you want to preserve the existing metrics as they are collected now that sounds reasonable to me. You could add a new metric and plan to deprecate the old metric once you've switched any ongoing analysis to the new metric. Neither the current method or recording as an enum will give you meaningful percentiles as we usually produce them. The current versions won't, since the percentiles on the value of the samples recorded, which for your metric is always 1. (We don't present them for enum histograms, since that's not normally meaningful.) You are probably thinking of percentiles of counts of these over some kind of scope, e.g per user per day, per session or something else, but you would need to do some custom analysis for that, or do that aggregation on the client and record the counts as the sample value.
On 2017/01/26 at 21:50:05, shivanisha wrote: > On 2017/01/26 at 21:32:22, shivanisha wrote: > > https://codereview.chromium.org/2474943002/diff/140001/chrome/browser/page_lo... > > File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc (right): > > > > https://codereview.chromium.org/2474943002/diff/140001/chrome/browser/page_lo... > > chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:144: UMA_HISTOGRAM_COUNTS( > > Ah, I see, UMA_HISTOGRAM_COUNTS creates a 50 bucket histogram! > > Would merging the above histogram PageLoad.Clients.DocWrite.Block.ReloadCount and the new one in an enumeration affect the ongoing Finch experiment for which PageLoad.Clients.DocWrite.Block.ReloadCount is an important metric. Does an enumeration enable to see each bucket's values at all major percentiles? If it affects the ongoing finch trial in any way then I will probably just make the new histogram an enum. WDYT? > > Bryan, > As per holte@'s feedback, after some clarification, I might be converting PageLoad.Clients.DocWrite.Block.ReloadCount and PageLoad.Clients.DocWrite.Block.SameSiteDiffSchemeCount to an enumerated histogram. Do you see any concerns in changing the PageLoad.Clients.DocWrite.Block.ReloadCount histogram to an enum because of the impact it might have on the ongoing finch experiment? > > Also, was there a specific reason to use UMA_HISTOGRAM_BOOLEAN for PageLoad.Clients.DocWrite.Block.Count in https://codereview.chromium.org/2644543002 ? Can we use UMA_HISTOGRAM_ENUMERATION for this as well in the same enum as the above two histograms (if it does not impact the existing finch experiment)? > > - Shivani At this point, I have a slight preference for keeping them as is to simplify analysis, but if there's a need to factor them into a common histogram, I'm on board with that. I do agree that in hindsight it would have been better to use an enum - we can definiely keep that in mind for similar cases in the future.
On 2017/01/26 at 23:50:22, bmcquade wrote: > On 2017/01/26 at 21:50:05, shivanisha wrote: > > On 2017/01/26 at 21:32:22, shivanisha wrote: > > > https://codereview.chromium.org/2474943002/diff/140001/chrome/browser/page_lo... > > > File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc (right): > > > > > > https://codereview.chromium.org/2474943002/diff/140001/chrome/browser/page_lo... > > > chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:144: UMA_HISTOGRAM_COUNTS( > > > Ah, I see, UMA_HISTOGRAM_COUNTS creates a 50 bucket histogram! > > > Would merging the above histogram PageLoad.Clients.DocWrite.Block.ReloadCount and the new one in an enumeration affect the ongoing Finch experiment for which PageLoad.Clients.DocWrite.Block.ReloadCount is an important metric. Does an enumeration enable to see each bucket's values at all major percentiles? If it affects the ongoing finch trial in any way then I will probably just make the new histogram an enum. WDYT? > > > > Bryan, > > As per holte@'s feedback, after some clarification, I might be converting PageLoad.Clients.DocWrite.Block.ReloadCount and PageLoad.Clients.DocWrite.Block.SameSiteDiffSchemeCount to an enumerated histogram. Do you see any concerns in changing the PageLoad.Clients.DocWrite.Block.ReloadCount histogram to an enum because of the impact it might have on the ongoing finch experiment? > > > > Also, was there a specific reason to use UMA_HISTOGRAM_BOOLEAN for PageLoad.Clients.DocWrite.Block.Count in https://codereview.chromium.org/2644543002 ? Can we use UMA_HISTOGRAM_ENUMERATION for this as well in the same enum as the above two histograms (if it does not impact the existing finch experiment)? > > > > - Shivani > > At this point, I have a slight preference for keeping them as is to simplify analysis, but if there's a need to factor them into a common histogram, I'm on board with that. I do agree that in hindsight it would have been better to use an enum - we can definiely keep that in mind for similar cases in the future. I could open a separate bug for making all 3 of them part of an enum and create a separate CL for that. In this CL, I could switch the new histogram to use UMA_HISTOGRAM_BOOLEAN since it's only a true or a false for each page load. It would then not have the overhead of COUNT and by itself wouldn't be a natural fit for ENUM since its just one type in the enum. All I need in terms of metrics is total count of such instances. Does that sound good?
On 2017/01/27 02:21:06, shivanisha wrote: > On 2017/01/26 at 23:50:22, bmcquade wrote: > > On 2017/01/26 at 21:50:05, shivanisha wrote: > > > On 2017/01/26 at 21:32:22, shivanisha wrote: > > > > > https://codereview.chromium.org/2474943002/diff/140001/chrome/browser/page_lo... > > > > File > chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc > (right): > > > > > > > > > https://codereview.chromium.org/2474943002/diff/140001/chrome/browser/page_lo... > > > > > chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:144: > UMA_HISTOGRAM_COUNTS( > > > > Ah, I see, UMA_HISTOGRAM_COUNTS creates a 50 bucket histogram! > > > > Would merging the above histogram > PageLoad.Clients.DocWrite.Block.ReloadCount and the new one in an enumeration > affect the ongoing Finch experiment for which > PageLoad.Clients.DocWrite.Block.ReloadCount is an important metric. Does an > enumeration enable to see each bucket's values at all major percentiles? If it > affects the ongoing finch trial in any way then I will probably just make the > new histogram an enum. WDYT? > > > > > > Bryan, > > > As per holte@'s feedback, after some clarification, I might be converting > PageLoad.Clients.DocWrite.Block.ReloadCount and > PageLoad.Clients.DocWrite.Block.SameSiteDiffSchemeCount to an enumerated > histogram. Do you see any concerns in changing the > PageLoad.Clients.DocWrite.Block.ReloadCount histogram to an enum because of the > impact it might have on the ongoing finch experiment? > > > > > > Also, was there a specific reason to use UMA_HISTOGRAM_BOOLEAN for > PageLoad.Clients.DocWrite.Block.Count in > https://codereview.chromium.org/2644543002 ? Can we use > UMA_HISTOGRAM_ENUMERATION for this as well in the same enum as the above two > histograms (if it does not impact the existing finch experiment)? > > > > > > - Shivani > > > > At this point, I have a slight preference for keeping them as is to simplify > analysis, but if there's a need to factor them into a common histogram, I'm on > board with that. I do agree that in hindsight it would have been better to use > an enum - we can definiely keep that in mind for similar cases in the future. > > I could open a separate bug for making all 3 of them part of an enum and create > a separate CL for that. In this CL, I could switch the new histogram to use > UMA_HISTOGRAM_BOOLEAN since it's only a true or a false for each page load. It > would then not have the overhead of COUNT and by itself wouldn't be a natural > fit for ENUM since its just one type in the enum. All I need in terms of metrics > is total count of such instances. > Does that sound good? I'd say just create an ENUM histogram for the new one, even if you are only recording the one bucket now. That way you can expand it later easily.
The CQ bit was checked by shivanisha@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...
Patchset #8 (id:160001) has been deleted
The CQ bit was checked by shivanisha@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...
On 2017/01/27 at 03:11:13, holte wrote: > On 2017/01/27 02:21:06, shivanisha wrote: > > On 2017/01/26 at 23:50:22, bmcquade wrote: > > > On 2017/01/26 at 21:50:05, shivanisha wrote: > > > > On 2017/01/26 at 21:32:22, shivanisha wrote: > > > > > > > https://codereview.chromium.org/2474943002/diff/140001/chrome/browser/page_lo... > > > > > File > > chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc > > (right): > > > > > > > > > > > > https://codereview.chromium.org/2474943002/diff/140001/chrome/browser/page_lo... > > > > > > > chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:144: > > UMA_HISTOGRAM_COUNTS( > > > > > Ah, I see, UMA_HISTOGRAM_COUNTS creates a 50 bucket histogram! > > > > > Would merging the above histogram > > PageLoad.Clients.DocWrite.Block.ReloadCount and the new one in an enumeration > > affect the ongoing Finch experiment for which > > PageLoad.Clients.DocWrite.Block.ReloadCount is an important metric. Does an > > enumeration enable to see each bucket's values at all major percentiles? If it > > affects the ongoing finch trial in any way then I will probably just make the > > new histogram an enum. WDYT? > > > > > > > > Bryan, > > > > As per holte@'s feedback, after some clarification, I might be converting > > PageLoad.Clients.DocWrite.Block.ReloadCount and > > PageLoad.Clients.DocWrite.Block.SameSiteDiffSchemeCount to an enumerated > > histogram. Do you see any concerns in changing the > > PageLoad.Clients.DocWrite.Block.ReloadCount histogram to an enum because of the > > impact it might have on the ongoing finch experiment? > > > > > > > > Also, was there a specific reason to use UMA_HISTOGRAM_BOOLEAN for > > PageLoad.Clients.DocWrite.Block.Count in > > https://codereview.chromium.org/2644543002 ? Can we use > > UMA_HISTOGRAM_ENUMERATION for this as well in the same enum as the above two > > histograms (if it does not impact the existing finch experiment)? > > > > > > > > - Shivani > > > > > > At this point, I have a slight preference for keeping them as is to simplify > > analysis, but if there's a need to factor them into a common histogram, I'm on > > board with that. I do agree that in hindsight it would have been better to use > > an enum - we can definiely keep that in mind for similar cases in the future. > > > > I could open a separate bug for making all 3 of them part of an enum and create > > a separate CL for that. In this CL, I could switch the new histogram to use > > UMA_HISTOGRAM_BOOLEAN since it's only a true or a false for each page load. It > > would then not have the overhead of COUNT and by itself wouldn't be a natural > > fit for ENUM since its just one type in the enum. All I need in terms of metrics > > is total count of such instances. > > Does that sound good? > > I'd say just create an ENUM histogram for the new one, even if you are only recording the one bucket now. > That way you can expand it later easily. Sounds good. Uploaded new patch with the change.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
bmcquade@chromium.org changed reviewers: + bmcquade@chromium.org
lgtm, thank you!
On 2017/01/27 at 18:38:01, bmcquade wrote: > lgtm, thank you! Also, created https://bugs.chromium.org/p/chromium/issues/detail?id=686093 for tracking the change to existing metrics.
The CQ bit was checked by shivanisha@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...
https://codereview.chromium.org/2474943002/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2474943002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:142: blink::WebLoadingBehaviorFlag::WebLoadingBehaviorDocumentWriteBlock)); This dcheck is incorrect since its possible 1 script to be "same site and different scheme" but another script to be blocked and thus for a page load both can happen together. Removing this dcheck. Thanks to the perf telemetry unit test working on real world pages like bbc! https://codereview.chromium.org/2474943002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:146: LOADING_BEHAVIOR_MAX); The same perf telemetry unit test is also failing because it seems 1 enum value is not allowed by histograms.cc. The test is crashing in DCHECK(valid_arguments) in https://cs.chromium.org/chromium/src/base/metrics/histogram.cc?q=histogram.cc... So I am adding the other 2 cases and 2 new histograms. Will deprecate the existing histograms after some time as part of https://bugs.chromium.org/p/chromium/issues/detail?id=686093
lgtm % comment https://codereview.chromium.org/2474943002/diff/200001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2474943002/diff/200001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:128: UMA_HISTOGRAM_ENUMERATION(internal::kHistogramDocWriteBlockLoadingBehavior, Putting this into a helper function in anonymous namespace rather than invoking the macro 3 times.
The CQ bit was checked by shivanisha@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...
Added a helper function as per feedback.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by shivanisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org, jkarlin@chromium.org, bmcquade@chromium.org, holte@chromium.org Link to the patchset: https://codereview.chromium.org/2474943002/#ps220001 (title: "Added a helper function")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1485785604058850,
"parent_rev": "716b1b327524cda0d7eda4a44f367828e9d5183c", "commit_rev":
"52c2ea5fbafcf7efc85e8f8fc4865073f3b6aaf1"}
Message was sent while issue was closed.
Description was changed from ========== doc.write intervention warning message changed to replace cross-origin with cross site (etld+1) BUG=658983 ========== to ========== doc.write intervention warning message changed to replace cross-origin with cross site (etld+1) BUG=658983 Review-Url: https://codereview.chromium.org/2474943002 Cr-Commit-Position: refs/heads/master@{#446993} Committed: https://chromium.googlesource.com/chromium/src/+/52c2ea5fbafcf7efc85e8f8fc486... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:220001) as https://chromium.googlesource.com/chromium/src/+/52c2ea5fbafcf7efc85e8f8fc486... |
