Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2367683002:
Add metrics for paths in file scheme exceptions in content settings. (Closed)
DescriptionAdd metrics for paths in file scheme exceptions in content settings.
These will tell how often file scheme exceptions have a non-empty
path vs. a wildcard path, and which content settings types use them.
BUG=628759, 621724
Committed: https://crrev.com/37df8891dff743a4a693575058ce22ca22adbe2a
Cr-Commit-Position: refs/heads/master@{#421248}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address review comments #
Total comments: 8
Patch Set 3 : Address isherman@'s comments #Patch Set 4 : Fix UMA name #
Total comments: 4
Patch Set 5 : Address msramek@'s comments #
Messages
Total messages: 27 (11 generated)
The CQ bit was checked by alexmos@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.
alexmos@chromium.org changed reviewers: + lshang@chromium.org, msramek@chromium.org
msramek@ and lshang@: please take a look. This adds a few more metrics for content settings exceptions for the file scheme, following up on lshang@'s https://codereview.chromium.org/2226643002/. The goal is to see how often the path is specified vs. a scheme-wide exception (i.e., file:///foo/bar vs. file:///*), and which content settings types use it. Supporting file path exceptions is really difficult with out-of-process iframes, since the top frame's URL isn't available when the top frame is in another process (but its origin is) - see issue 628759, where this comes up for images/scripts/autoplay. This will help inform any decisions regarding path support.
Also could you add a test in content_settings_pattern_unittest.cc to test the HasPath() method? https://codereview.chromium.org/2367683002/diff/1/components/content_settings... File components/content_settings/core/common/content_settings_pattern.cc (right): https://codereview.chromium.org/2367683002/diff/1/components/content_settings... components/content_settings/core/common/content_settings_pattern.cc:619: return false; How about using DCHECK here? Since HasPath() is only valid for file scheme, other schemes calling this method should be invalid, instead of returning false, I think.
LGTM when you address Liu's comments.
I think this can be done without adding a new method:
file_path_wildcard = ContentSettingsPattern::FromString("file:///*");
switch (pattern.Compare(file_wildcard)) {
case IDENTITY: // File scheme, no path.
case PREDECESSOR: // File scheme, has a path.
default: // Not a file scheme pattern.
}
But I'm okay with adding the method, as it's going to be a little bit faster :)
https://codereview.chromium.org/2367683002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):
https://codereview.chromium.org/2367683002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:6815: + Records how often a content
settings exception for the file: scheme has a
You should probably mention when is the histogram recorded.
The CQ bit was checked by alexmos@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...
Thanks for reviewing! Comments addressed, and a small unit test added. I'll wait until tomorrow before landing in case you want to take another look. https://codereview.chromium.org/2367683002/diff/1/components/content_settings... File components/content_settings/core/common/content_settings_pattern.cc (right): https://codereview.chromium.org/2367683002/diff/1/components/content_settings... components/content_settings/core/common/content_settings_pattern.cc:619: return false; On 2016/09/26 01:57:13, lshang wrote: > How about using DCHECK here? Since HasPath() is only valid for file scheme, > other schemes calling this method should be invalid, instead of returning false, > I think. Done. https://codereview.chromium.org/2367683002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2367683002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:6815: + Records how often a content settings exception for the file: scheme has a On 2016/09/26 13:19:31, msramek wrote: > You should probably mention when is the histogram recorded. Done.
alexmos@chromium.org changed reviewers: + isherman@chromium.org
+isherman for histograms.xml
LGTM!(●'◡'●)
Metrics LGTM % nits: https://codereview.chromium.org/2367683002/diff/20001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2367683002/diff/20001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:639: "ContentSettings.ExceptionSchemeFile.TypeWithPath", Optional nit: I'd suggest more dots in the name -- perhaps "ContentSettings.ExceptionScheme.File.Type.WithPath". https://codereview.chromium.org/2367683002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2367683002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6812: +<histogram name="ContentSettings.ExceptionSchemeFile.HasPath" enum="Boolean"> nit: Please define a custom enum, BooleanHasPath, with more semantically contentful labels. https://codereview.chromium.org/2367683002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6816: + non-empty path. Recorded at browser start. nit: Probably worth mentioning explicitly that this is recorded *once per exception* at browser start. Ditto for the other metrics.
Thanks! https://codereview.chromium.org/2367683002/diff/20001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2367683002/diff/20001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:639: "ContentSettings.ExceptionSchemeFile.TypeWithPath", On 2016/09/27 00:54:33, Ilya Sherman wrote: > Optional nit: I'd suggest more dots in the name -- perhaps > "ContentSettings.ExceptionScheme.File.Type.WithPath". Done. https://codereview.chromium.org/2367683002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2367683002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6812: +<histogram name="ContentSettings.ExceptionSchemeFile.HasPath" enum="Boolean"> On 2016/09/27 00:54:34, Ilya Sherman wrote: > nit: Please define a custom enum, BooleanHasPath, with more semantically > contentful labels. Done. https://codereview.chromium.org/2367683002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6816: + non-empty path. Recorded at browser start. On 2016/09/27 00:54:33, Ilya Sherman wrote: > nit: Probably worth mentioning explicitly that this is recorded *once per > exception* at browser start. Ditto for the other metrics. Done.
https://codereview.chromium.org/2367683002/diff/20001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2367683002/diff/20001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:639: "ContentSettings.ExceptionSchemeFile.TypeWithPath", On 2016/09/27 01:11:22, alexmos wrote: > On 2016/09/27 00:54:33, Ilya Sherman wrote: > > Optional nit: I'd suggest more dots in the name -- perhaps > > "ContentSettings.ExceptionScheme.File.Type.WithPath". > > Done. Hmm, it looks like you updated the name in histograms.xml, but not in this file. Please double-check to make sure they're consistent =)
https://codereview.chromium.org/2367683002/diff/20001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2367683002/diff/20001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:639: "ContentSettings.ExceptionSchemeFile.TypeWithPath", On 2016/09/27 01:15:25, Ilya Sherman wrote: > On 2016/09/27 01:11:22, alexmos wrote: > > On 2016/09/27 00:54:33, Ilya Sherman wrote: > > > Optional nit: I'd suggest more dots in the name -- perhaps > > > "ContentSettings.ExceptionScheme.File.Type.WithPath". > > > > Done. > > Hmm, it looks like you updated the name in histograms.xml, but not in this file. > Please double-check to make sure they're consistent =) My bad - somehow the file ended up not being saved. Fixed now, thanks for noticing!
LGTM, thanks.
Still LGTM % comments. https://codereview.chromium.org/2367683002/diff/60001/components/content_sett... File components/content_settings/core/common/content_settings_pattern.cc (right): https://codereview.chromium.org/2367683002/diff/60001/components/content_sett... components/content_settings/core/common/content_settings_pattern.cc:618: DCHECK(GetScheme() == SCHEME_FILE); nit: DCHECK_EQ https://codereview.chromium.org/2367683002/diff/60001/components/content_sett... File components/content_settings/core/common/content_settings_pattern.h (right): https://codereview.chromium.org/2367683002/diff/60001/components/content_sett... components/content_settings/core/common/content_settings_pattern.h:197: // True if this pattern has a non-empty path. Can only be true for patterns With the DCHECK, this comment should say "can only be used" instead of "can only be true".
Thanks! https://codereview.chromium.org/2367683002/diff/60001/components/content_sett... File components/content_settings/core/common/content_settings_pattern.cc (right): https://codereview.chromium.org/2367683002/diff/60001/components/content_sett... components/content_settings/core/common/content_settings_pattern.cc:618: DCHECK(GetScheme() == SCHEME_FILE); On 2016/09/27 11:11:38, msramek wrote: > nit: DCHECK_EQ Done. https://codereview.chromium.org/2367683002/diff/60001/components/content_sett... File components/content_settings/core/common/content_settings_pattern.h (right): https://codereview.chromium.org/2367683002/diff/60001/components/content_sett... components/content_settings/core/common/content_settings_pattern.h:197: // True if this pattern has a non-empty path. Can only be true for patterns On 2016/09/27 11:11:38, msramek wrote: > With the DCHECK, this comment should say "can only be used" instead of "can only > be true". Done.
The CQ bit was checked by alexmos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lshang@chromium.org, msramek@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2367683002/#ps80001 (title: "Address msramek@'s comments")
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.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add metrics for paths in file scheme exceptions in content settings. These will tell how often file scheme exceptions have a non-empty path vs. a wildcard path, and which content settings types use them. BUG=628759, 621724 ========== to ========== Add metrics for paths in file scheme exceptions in content settings. These will tell how often file scheme exceptions have a non-empty path vs. a wildcard path, and which content settings types use them. BUG=628759, 621724 Committed: https://crrev.com/37df8891dff743a4a693575058ce22ca22adbe2a Cr-Commit-Position: refs/heads/master@{#421248} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/37df8891dff743a4a693575058ce22ca22adbe2a Cr-Commit-Position: refs/heads/master@{#421248} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
