Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2226643002:
Add metrics for schemes of content setting exceptions (Closed)
|
Created:
4 years, 4 months ago by lshang Modified:
4 years, 4 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org, markusheintz_, msramek+watch_chromium.org, raymes+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd metrics for schemes of content setting exceptions
Add a uma metric to collect data for schemes (http/https/file/chrome-extension) of
content setting exceptions.
BUG=621724
Committed: https://crrev.com/d06a12d4cf219e1c310cdc736736f35d6168de49
Cr-Commit-Position: refs/heads/master@{#411245}
Patch Set 1 #Patch Set 2 : collect data in one run #
Total comments: 12
Patch Set 3 : address review comments #
Total comments: 6
Patch Set 4 : address review comments #
Total comments: 6
Patch Set 5 : address review comments #
Messages
Total messages: 36 (26 generated)
Description was changed from ========== Add metrics for schemes of content setting exceptions format add metric BUG= ========== to ========== Add metrics for schemes of content setting exceptions Add a uma metric to collect data for schemes (http/https/file/chrome-extension) of content setting exceptions. BUG=621724 ==========
lshang@chromium.org changed reviewers: + msramek@chromium.org
PTAL thanks!
The CQ bit was checked by lshang@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.
Left a few comments :) Also, what is the motivation? https://codereview.chromium.org/2226643002/diff/20001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2226643002/diff/20001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:614: if (setting_entry.primary_pattern != ContentSettingsPattern::Wildcard()) { nit: Should we also check the secondary pattern for a wildcard? Not sure if that can actually happen but it at least makes the intention clearer. https://codereview.chromium.org/2226643002/diff/20001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:615: UMA_HISTOGRAM_ENUMERATION("ContentSettings.ExceptionScheme", We're recording exceptions from all sources - including policy, extensions etc. Is that intentional? https://codereview.chromium.org/2226643002/diff/20001/components/content_sett... File components/content_settings/core/common/content_settings_pattern.cc (right): https://codereview.chromium.org/2226643002/diff/20001/components/content_sett... components/content_settings/core/common/content_settings_pattern.cc:25: "unknown", I would add kFileSystemScheme as well. It seems to be a mess to handle it, and it's deprecated anyway as far as I know, so I wonder what the usage is. https://codereview.chromium.org/2226643002/diff/20001/components/content_sett... components/content_settings/core/common/content_settings_pattern.cc:603: return SCHEME_UNKNOWN; I would leave UNKNOWN for something that is really UNKNOWN (line 604) and be explicit here (line 603) that it's a WILDCARD. https://codereview.chromium.org/2226643002/diff/20001/components/content_sett... components/content_settings/core/common/content_settings_pattern.cc:611: return scheme; Optional style suggestion: Consider returning the found value directly from the for-loop (line 607), and hardcoding UNKNOWN here (line 611).
The CQ bit was checked by lshang@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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
Sorry for the confusion. I should add more background details. I'm recently looking into changing content settings to use url::Origins instead of GURLs (https://bugs.chromium.org/p/chromium/issues/detail?id=621724). The main problem now is that url::Origins don't have good support for file:// URLs (it doesn't retain path information, and every file:// URL is treated as unique to anyone else including itself), but we use file:// URLs for some content settings e.g. plugins. There are some discussions going on in the issue and email thread about whether we should add support for file:// URLs in url::Origins (that might break other things), or remove support for file URLs in content settings. Decisions haven't been made. But ahead of all these, I think gathering some stats about usage of file:// URLs in content settings would be a good reference. And since we're collecting file scheme data, why not collect other schemes by the way? That is where this CL come from:-) https://codereview.chromium.org/2226643002/diff/20001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2226643002/diff/20001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:614: if (setting_entry.primary_pattern != ContentSettingsPattern::Wildcard()) { On 2016/08/08 17:33:12, msramek wrote: > nit: Should we also check the secondary pattern for a wildcard? Not sure if that > can actually happen but it at least makes the intention clearer. Done. https://codereview.chromium.org/2226643002/diff/20001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:615: UMA_HISTOGRAM_ENUMERATION("ContentSettings.ExceptionScheme", On 2016/08/08 17:33:12, msramek wrote: > We're recording exceptions from all sources - including policy, extensions etc. > Is that intentional? I think we would want to collect scheme data from all sources including policy and extensions, if we want to see the effect of removing support for file:// schemes. Please see background details above. https://codereview.chromium.org/2226643002/diff/20001/components/content_sett... File components/content_settings/core/common/content_settings_pattern.cc (right): https://codereview.chromium.org/2226643002/diff/20001/components/content_sett... components/content_settings/core/common/content_settings_pattern.cc:25: "unknown", On 2016/08/08 17:33:12, msramek wrote: > I would add kFileSystemScheme as well. It seems to be a mess to handle it, and > it's deprecated anyway as far as I know, so I wonder what the usage is. I considered this scheme, but as you can see in the test, ContentSettingsPattern seems don't store filesystem scheme(it's scheme is ""), so I regard it as unknown before. https://codereview.chromium.org/2226643002/diff/20001/components/content_sett... components/content_settings/core/common/content_settings_pattern.cc:603: return SCHEME_UNKNOWN; On 2016/08/08 17:33:12, msramek wrote: > I would leave UNKNOWN for something that is really UNKNOWN (line 604) and be > explicit here (line 603) that it's a WILDCARD. Done. Good point! https://codereview.chromium.org/2226643002/diff/20001/components/content_sett... components/content_settings/core/common/content_settings_pattern.cc:611: return scheme; On 2016/08/08 17:33:12, msramek wrote: > Optional style suggestion: Consider returning the found value directly from the > for-loop (line 607), and hardcoding UNKNOWN here (line 611). Done. And I also changed UNKNOWN to OTHER, which I think is more clear when we add WILDCARD as well.
LGTM with a few more comments. Thanks for explaining the background. I have cc'd myself to the bug, as I want to be in the loop on this :) I think formalizing extensions and file paths as their own security origins is the way to go. https://codereview.chromium.org/2226643002/diff/20001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2226643002/diff/20001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:615: UMA_HISTOGRAM_ENUMERATION("ContentSettings.ExceptionScheme", On 2016/08/09 01:56:40, Liu Shang wrote: > On 2016/08/08 17:33:12, msramek wrote: > > We're recording exceptions from all sources - including policy, extensions > etc. > > Is that intentional? > > I think we would want to collect scheme data from all sources including policy > and extensions, if we want to see the effect of removing support for file:// > schemes. Please see background details above. Acknowledged. Yes, that makes sense now that I read about the background. https://codereview.chromium.org/2226643002/diff/20001/components/content_sett... File components/content_settings/core/common/content_settings_pattern.cc (right): https://codereview.chromium.org/2226643002/diff/20001/components/content_sett... components/content_settings/core/common/content_settings_pattern.cc:25: "unknown", On 2016/08/09 01:56:40, Liu Shang wrote: > On 2016/08/08 17:33:12, msramek wrote: > > I would add kFileSystemScheme as well. It seems to be a mess to handle it, and > > it's deprecated anyway as far as I know, so I wonder what the usage is. > > I considered this scheme, but as you can see in the test, ContentSettingsPattern > seems don't store filesystem scheme(it's scheme is ""), so I regard it as > unknown before. Acknowledged. https://codereview.chromium.org/2226643002/diff/40001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2226643002/diff/40001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:616: continue; nit: use braces {} if the if() takes more than one line https://codereview.chromium.org/2226643002/diff/40001/components/content_sett... File components/content_settings/core/common/content_settings_pattern.cc (right): https://codereview.chromium.org/2226643002/diff/40001/components/content_sett... components/content_settings/core/common/content_settings_pattern.cc:606: for (int i = 1; i < SCHEME_MAX - 1; ++i) { We're iterating over kSchemeNames, so I would specify arraysize(kSchemeNames) as the top bound. There currently isn't any coupling between kSchemeNames and SCHEME_MAX, i.e. it can't be easily verified that this loop won't go out of bounds. Please also introduce such a coupling by adding static_assert(arraysize(kSchemeNames) == SCHEME_MAX - 1) https://codereview.chromium.org/2226643002/diff/40001/components/content_sett... components/content_settings/core/common/content_settings_pattern.cc:607: if (parts_.scheme == kSchemeNames[i]) { nit: No braces needed for a one-line for() :)
The CQ bit was checked by lshang@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 lshang@chromium.org to run a CQ dry run
Patchset #4 (id:60001) 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lshang@chromium.org changed reviewers: + isherman@chromium.org
+isherman for tools/metrics/histograms/histograms.xml Thanks Martin! https://codereview.chromium.org/2226643002/diff/40001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2226643002/diff/40001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:616: continue; On 2016/08/09 10:47:43, msramek wrote: > nit: use braces {} if the if() takes more than one line Done. https://codereview.chromium.org/2226643002/diff/40001/components/content_sett... File components/content_settings/core/common/content_settings_pattern.cc (right): https://codereview.chromium.org/2226643002/diff/40001/components/content_sett... components/content_settings/core/common/content_settings_pattern.cc:606: for (int i = 1; i < SCHEME_MAX - 1; ++i) { On 2016/08/09 10:47:43, msramek wrote: > We're iterating over kSchemeNames, so I would specify arraysize(kSchemeNames) as > the top bound. There currently isn't any coupling between kSchemeNames and > SCHEME_MAX, i.e. it can't be easily verified that this loop won't go out of > bounds. > > Please also introduce such a coupling by adding > static_assert(arraysize(kSchemeNames) == SCHEME_MAX - 1) Done. Good advice! https://codereview.chromium.org/2226643002/diff/40001/components/content_sett... components/content_settings/core/common/content_settings_pattern.cc:607: if (parts_.scheme == kSchemeNames[i]) { On 2016/08/09 10:47:43, msramek wrote: > nit: No braces needed for a one-line for() :) Done.
metrics lgtm % comments: https://codereview.chromium.org/2226643002/diff/80001/components/content_sett... File components/content_settings/core/common/content_settings_pattern.h (right): https://codereview.chromium.org/2226643002/diff/80001/components/content_sett... components/content_settings/core/common/content_settings_pattern.h:56: enum SchemeType { Please document that this enum is used to back an UMA histogram, and should therefore be treated as append-only. https://codereview.chromium.org/2226643002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2226643002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6561: + The number of different content setting exception schemes at browser start. If I understand this description correctly, then I think this means something like "Records the content setting exception schemes present at browser start. If multiple exceptions with a single scheme exist, the corresponding bucket will be incremented multiple times." If I'm understanding correctly, please use that description, or a modified/even-clearer version of it. If I'm misunderstanding, please correct the description as needed. https://codereview.chromium.org/2226643002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:70679: + <int value="5" label="(other)"/> I'd recommend listing unknown/other as the first bucket. That way, if you ever add any additional known schemes, the "other" bucket won't end up somewhere in the middle of the list.
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
https://codereview.chromium.org/2226643002/diff/80001/components/content_sett... File components/content_settings/core/common/content_settings_pattern.h (right): https://codereview.chromium.org/2226643002/diff/80001/components/content_sett... components/content_settings/core/common/content_settings_pattern.h:56: enum SchemeType { On 2016/08/10 05:34:26, Ilya Sherman wrote: > Please document that this enum is used to back an UMA histogram, and should > therefore be treated as append-only. Done. https://codereview.chromium.org/2226643002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2226643002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6561: + The number of different content setting exception schemes at browser start. On 2016/08/10 05:34:26, Ilya Sherman wrote: > If I understand this description correctly, then I think this means something > like "Records the content setting exception schemes present at browser start. > If multiple exceptions with a single scheme exist, the corresponding bucket will > be incremented multiple times." > > If I'm understanding correctly, please use that description, or a > modified/even-clearer version of it. If I'm misunderstanding, please correct > the description as needed. Done. Yeah you understand correctly, I've changed the summary to be more clear. https://codereview.chromium.org/2226643002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:70679: + <int value="5" label="(other)"/> On 2016/08/10 05:34:26, Ilya Sherman wrote: > I'd recommend listing unknown/other as the first bucket. That way, if you ever > add any additional known schemes, the "other" bucket won't end up somewhere in > the middle of the list. Done. Moved all known schmes down to the list.
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.
The CQ bit was checked by lshang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msramek@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2226643002/#ps100001 (title: "address review 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.
Description was changed from ========== Add metrics for schemes of content setting exceptions Add a uma metric to collect data for schemes (http/https/file/chrome-extension) of content setting exceptions. BUG=621724 ========== to ========== Add metrics for schemes of content setting exceptions Add a uma metric to collect data for schemes (http/https/file/chrome-extension) of content setting exceptions. BUG=621724 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add metrics for schemes of content setting exceptions Add a uma metric to collect data for schemes (http/https/file/chrome-extension) of content setting exceptions. BUG=621724 ========== to ========== Add metrics for schemes of content setting exceptions Add a uma metric to collect data for schemes (http/https/file/chrome-extension) of content setting exceptions. BUG=621724 Committed: https://crrev.com/d06a12d4cf219e1c310cdc736736f35d6168de49 Cr-Commit-Position: refs/heads/master@{#411245} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d06a12d4cf219e1c310cdc736736f35d6168de49 Cr-Commit-Position: refs/heads/master@{#411245} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
