Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2777093007:
[subresource_filter] Add metrics for UI / related things (Closed)
|
Created:
3 years, 8 months ago by Charlie Harrison Modified:
3 years, 8 months ago CC:
chromium-reviews, subresource-filter-reviews_chromium.org, asvitkine+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[subresource_filter] Add metrics for UI / related things
This patch adds a new profile keyed service for keeping profile-scoped
state for the subresource filter.
For now, this is used to implement a content_service::Observer to be
notified of any ContentSettings changes for that particular profile.
BUG=705260
Review-Url: https://codereview.chromium.org/2777093007
Cr-Commit-Position: refs/heads/master@{#460792}
Committed: https://chromium.googlesource.com/chromium/src/+/6b2ff2dfdf2c6edfe16f8fe1765b9637d250cf3b
Patch Set 1 #
Total comments: 16
Patch Set 2 : msramek review + unit tests #
Total comments: 1
Patch Set 3 : msramek review #
Total comments: 2
Patch Set 4 : rkaplow review #Messages
Total messages: 39 (28 generated)
The CQ bit was checked by csharrison@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...
Description was changed from ========== [subresource_filter] Add metrics for UI / related things BUG= ========== to ========== [subresource_filter] Add metrics for UI / related things This patch adds a new profile keyed service for keeping profile-scoped state for the subresource filter. For now, this is used to implement a content_service::Observer to be notified of any ContentSettings changes for that particular profile. BUG=705260 ==========
csharrison@chromium.org changed reviewers: + msramek@chromium.org
msramek: Would you mind taking a first look at the usage of content_settings::Observer? In particular, I am wondering if there's an easier way to do this than creating a Profile keyed service to effectively get a 1:1 mapping with the content setting map. Browser tests incoming once I know this method is ok.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: CQ has no permission to schedule in bucket master.tryserver.chromium.linux
The CQ bit was checked by csharrison@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: CQ has no permission to schedule in bucket master.tryserver.chromium.linux
Generally looks good, but I left a bunch of comments :) https://codereview.chromium.org/2777093007/diff/1/chrome/browser/subresource_... File chrome/browser/subresource_filter/subresource_filter_content_settings_observer_factory.cc (right): https://codereview.chromium.org/2777093007/diff/1/chrome/browser/subresource_... chrome/browser/subresource_filter/subresource_filter_content_settings_observer_factory.cc:25: void SubresourceFilterContentSettingsObserver::OnContentSettingChanged( This method probably deserves a unittest. You mention a browsertest in your comment, but that's probably not needed; it seems to me that you just need a TestingProfile and the UI thread here. https://codereview.chromium.org/2777093007/diff/1/chrome/browser/subresource_... chrome/browser/subresource_filter/subresource_filter_content_settings_observer_factory.cc:27: const ContentSettingsPattern& secondary_pattern, Do we care about the secondary pattern? If a user sets the content setting through a permission prompt on "google.com", they'll get ["google.com", "*"]. The settings UI only allows specifying primary patterns, so again, they can only add ["google.com", "*"]. That's fine. Below you call GetContentSetting(url, url, ...) = GetContentSettingForUrl("google.com", "google.com", ...), which will return the value for ["google.com", "*"], because "*" matches "google.com". However, an extension or administrator could theoretically add an exception for ["google.com", "example.org"]. Then below, you call GetContentSetting(url, url, ...) = GetContentSettingForUrl("google.com", "google.com") which would not match ["google.com", "example.org"] and would return something broader, e.g. the default setting. As far as I see, we don't have subresource filter in the extension API or admin policy, so I just propose: DCHECK(secondary_pattern == ContentSettingsPattern::Wildcard()); https://codereview.chromium.org/2777093007/diff/1/chrome/browser/subresource_... chrome/browser/subresource_filter/subresource_filter_content_settings_observer_factory.cc:40: GetContentSettingForUrl(GURL("https://example.com")); I'm so going to mess with your metrics by flipping the setting on example.com :) You can use HostContentSettingsMap::GetDefaultContentSetting(). This returns ContentSetting, which is safe, because SUBRESOURCE_FILTER indeed is a content setting (and not just a website setting). https://codereview.chromium.org/2777093007/diff/1/chrome/browser/subresource_... chrome/browser/subresource_filter/subresource_filter_content_settings_observer_factory.cc:41: if (global_setting == CONTENT_SETTING_BLOCK) { style nit: Use a switch instead of NOTREACHED() to ensure that the enumeration is exhaustive at compile-time https://codereview.chromium.org/2777093007/diff/1/chrome/browser/subresource_... chrome/browser/subresource_filter/subresource_filter_content_settings_observer_factory.cc:61: if (setting == CONTENT_SETTING_BLOCK) { Ditto. https://codereview.chromium.org/2777093007/diff/1/chrome/browser/subresource_... File chrome/browser/subresource_filter/subresource_filter_content_settings_observer_factory.h (right): https://codereview.chromium.org/2777093007/diff/1/chrome/browser/subresource_... chrome/browser/subresource_filter/subresource_filter_content_settings_observer_factory.h:20: class SubresourceFilterContentSettingsObserver optional nit: Putting the KeyedService to the same file as its factory seems like a strange pattern. I understand that we never need to directly access it, so I'm just wondering if we need to encapsulate them together better. Or maybe put the declaration into an anonymous namespace in the .cc file. Note that the factory declaration doesn't need to see the observer's declaration. https://codereview.chromium.org/2777093007/diff/1/chrome/browser/subresource_... chrome/browser/subresource_filter/subresource_filter_content_settings_observer_factory.h:44: class SubresourceFilterContentSettingsObserverFactory I share your feeling that a KeyedService seems like an overkill, but I don't have a better idea. HCSM is scoped to a Profile, you need the observer to be scoped to a Profile as well... Nothing else in this directory already has this property, so this is probably the correct way to do it.
The CQ bit was checked by csharrison@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 the review. I've added a unit test suite. One question. I noticed that - Setting a default setting results in an invalid primary pattern for the observer - Trying to SetContentSettingCustomScope with an invalid pattern fails elsewhere So, as long as I check first that the we aren't setting a default setting, should the primary pattern always be valid? I've DCHECK'd this in the code but I'm not confident it is correct. https://codereview.chromium.org/2777093007/diff/1/chrome/browser/subresource_... File chrome/browser/subresource_filter/subresource_filter_content_settings_observer_factory.cc (right): https://codereview.chromium.org/2777093007/diff/1/chrome/browser/subresource_... chrome/browser/subresource_filter/subresource_filter_content_settings_observer_factory.cc:25: void SubresourceFilterContentSettingsObserver::OnContentSettingChanged( On 2017/03/29 09:41:25, msramek wrote: > This method probably deserves a unittest. You mention a browsertest in your > comment, but that's probably not needed; it seems to me that you just need a > TestingProfile and the UI thread here. Done. https://codereview.chromium.org/2777093007/diff/1/chrome/browser/subresource_... chrome/browser/subresource_filter/subresource_filter_content_settings_observer_factory.cc:27: const ContentSettingsPattern& secondary_pattern, On 2017/03/29 09:41:25, msramek wrote: > Do we care about the secondary pattern? > > If a user sets the content setting through a permission prompt on "google.com", > they'll get ["google.com", "*"]. > > The settings UI only allows specifying primary patterns, so again, they can only > add ["google.com", "*"]. > > That's fine. Below you call GetContentSetting(url, url, ...) = > GetContentSettingForUrl("google.com", "google.com", ...), which will return the > value for ["google.com", "*"], because "*" matches "google.com". > > However, an extension or administrator could theoretically add an exception for > ["google.com", "example.org"]. > > Then below, you call GetContentSetting(url, url, ...) = > GetContentSettingForUrl("google.com", "google.com") which would not match > ["google.com", "example.org"] and would return something broader, e.g. the > default setting. > > As far as I see, we don't have subresource filter in the extension API or admin > policy, so I just propose: > > DCHECK(secondary_pattern == ContentSettingsPattern::Wildcard()); Done. We may add this functionality in the future but for now I've just made a note to remove this if we do. https://codereview.chromium.org/2777093007/diff/1/chrome/browser/subresource_... chrome/browser/subresource_filter/subresource_filter_content_settings_observer_factory.cc:40: GetContentSettingForUrl(GURL("https://example.com")); On 2017/03/29 09:41:25, msramek wrote: > I'm so going to mess with your metrics by flipping the setting on http://example.com :) > > You can use HostContentSettingsMap::GetDefaultContentSetting(). This returns > ContentSetting, which is safe, because SUBRESOURCE_FILTER indeed is a content > setting (and not just a website setting). Ha! Done. I knew there was a better way than this. https://codereview.chromium.org/2777093007/diff/1/chrome/browser/subresource_... chrome/browser/subresource_filter/subresource_filter_content_settings_observer_factory.cc:41: if (global_setting == CONTENT_SETTING_BLOCK) { On 2017/03/29 09:41:25, msramek wrote: > style nit: Use a switch instead of NOTREACHED() to ensure that the enumeration > is exhaustive at compile-time There are 5 content settings that I'm not handling here. Do you propose I add them all in the switch with a NOTREACHED() if they are encountered? This is what I've done (+ hoisted it to a helper function since we do it twice). https://codereview.chromium.org/2777093007/diff/1/chrome/browser/subresource_... chrome/browser/subresource_filter/subresource_filter_content_settings_observer_factory.cc:61: if (setting == CONTENT_SETTING_BLOCK) { On 2017/03/29 09:41:25, msramek wrote: > Ditto. Done. https://codereview.chromium.org/2777093007/diff/1/chrome/browser/subresource_... File chrome/browser/subresource_filter/subresource_filter_content_settings_observer_factory.h (right): https://codereview.chromium.org/2777093007/diff/1/chrome/browser/subresource_... chrome/browser/subresource_filter/subresource_filter_content_settings_observer_factory.h:20: class SubresourceFilterContentSettingsObserver On 2017/03/29 09:41:25, msramek wrote: > optional nit: Putting the KeyedService to the same file as its factory seems > like a strange pattern. > > I understand that we never need to directly access it, so I'm just wondering if > we need to encapsulate them together better. Or maybe put the declaration into > an anonymous namespace in the .cc file. Note that the factory declaration > doesn't need to see the observer's declaration. Putting this anonymous in the cc file sounds good to me. I was originally concerned about the GetForProfile method in the factory, but that could easily be renamed to "EnsureForProfile" or something so it doesn't need to actually return anything. https://codereview.chromium.org/2777093007/diff/1/chrome/browser/subresource_... chrome/browser/subresource_filter/subresource_filter_content_settings_observer_factory.h:44: class SubresourceFilterContentSettingsObserverFactory On 2017/03/29 09:41:25, msramek wrote: > I share your feeling that a KeyedService seems like an overkill, but I don't > have a better idea. HCSM is scoped to a Profile, you need the observer to be > scoped to a Profile as well... Nothing else in this directory already has this > property, so this is probably the correct way to do it. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Nice! LGTM. https://codereview.chromium.org/2777093007/diff/1/chrome/browser/subresource_... File chrome/browser/subresource_filter/subresource_filter_content_settings_observer_factory.cc (right): https://codereview.chromium.org/2777093007/diff/1/chrome/browser/subresource_... chrome/browser/subresource_filter/subresource_filter_content_settings_observer_factory.cc:41: if (global_setting == CONTENT_SETTING_BLOCK) { On 2017/03/29 15:23:32, Charlie Harrison wrote: > On 2017/03/29 09:41:25, msramek wrote: > > style nit: Use a switch instead of NOTREACHED() to ensure that the enumeration > > is exhaustive at compile-time > > There are 5 content settings that I'm not handling here. Do you propose I add > them all in the switch with a NOTREACHED() if they are encountered? This is what > I've done (+ hoisted it to a helper function since we do it twice). Sigh... For a moment, I completely forgot that we're not working with just a 2-member enum. This is actually worse than what you had, because it forces everyone who adds a new value for their setting to also update your code. So, we *could* have a 2-set enum: enum SETTING { ALLOW = CONTENT_SETTING_ALLOW, BLOCK = CONTENT_SETTING_BLOCK }; But I'd probably just revert this part to the original. Sorry! https://codereview.chromium.org/2777093007/diff/20001/chrome/browser/subresou... File chrome/browser/subresource_filter/subresource_filter_content_settings_observer_factory.cc (right): https://codereview.chromium.org/2777093007/diff/20001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_content_settings_observer_factory.cc:83: if (details.update_all()) { To your question, yes, DefaultProvider apparently sends empty (and thus invalid) patterns instead of Wildcards()s when a default setting is updated. That seems like a remnant of some very old design choice... But your solution should be correct.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
csharrison@chromium.org changed reviewers: + melandory@chromium.org
Many thanks for the review. Tanja: would you take a look at this patch? If you are overloaded please just look at the high-level and I can ask Pavel to review in more detail (I've been asking a lot of you lately). https://codereview.chromium.org/2777093007/diff/1/chrome/browser/subresource_... File chrome/browser/subresource_filter/subresource_filter_content_settings_observer_factory.cc (right): https://codereview.chromium.org/2777093007/diff/1/chrome/browser/subresource_... chrome/browser/subresource_filter/subresource_filter_content_settings_observer_factory.cc:41: if (global_setting == CONTENT_SETTING_BLOCK) { On 2017/03/29 17:37:30, msramek wrote: > On 2017/03/29 15:23:32, Charlie Harrison wrote: > > On 2017/03/29 09:41:25, msramek wrote: > > > style nit: Use a switch instead of NOTREACHED() to ensure that the > enumeration > > > is exhaustive at compile-time > > > > There are 5 content settings that I'm not handling here. Do you propose I add > > them all in the switch with a NOTREACHED() if they are encountered? This is > what > > I've done (+ hoisted it to a helper function since we do it twice). > > Sigh... For a moment, I completely forgot that we're not working with just a > 2-member enum. This is actually worse than what you had, because it forces > everyone who adds a new value for their setting to also update your code. > > So, we *could* have a 2-set enum: > > enum SETTING { ALLOW = CONTENT_SETTING_ALLOW, BLOCK = CONTENT_SETTING_BLOCK }; > > But I'd probably just revert this part to the original. Sorry! No worries! Reverted back to the if chain.
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 csharrison@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm
csharrison@chromium.org changed reviewers: + rkaplow@chromium.org
rkaplow, would you PTAL at histograms.xml?
lgtm https://codereview.chromium.org/2777093007/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2777093007/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:69257: + SubresourceFilter. can you mention exactly when this will get triggered
https://codereview.chromium.org/2777093007/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2777093007/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:69257: + SubresourceFilter. On 2017/03/30 15:04:08, rkaplow (slow) wrote: > can you mention exactly when this will get triggered Done, to the best of my ability.
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msramek@chromium.org, melandory@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2777093007/#ps50001 (title: "rkaplow review")
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": 50001, "attempt_start_ts": 1490887160952310,
"parent_rev": "6a9f7d4d9ff9f203411ff11c650ad8b134f43855", "commit_rev":
"6b2ff2dfdf2c6edfe16f8fe1765b9637d250cf3b"}
Message was sent while issue was closed.
Description was changed from ========== [subresource_filter] Add metrics for UI / related things This patch adds a new profile keyed service for keeping profile-scoped state for the subresource filter. For now, this is used to implement a content_service::Observer to be notified of any ContentSettings changes for that particular profile. BUG=705260 ========== to ========== [subresource_filter] Add metrics for UI / related things This patch adds a new profile keyed service for keeping profile-scoped state for the subresource filter. For now, this is used to implement a content_service::Observer to be notified of any ContentSettings changes for that particular profile. BUG=705260 Review-Url: https://codereview.chromium.org/2777093007 Cr-Commit-Position: refs/heads/master@{#460792} Committed: https://chromium.googlesource.com/chromium/src/+/6b2ff2dfdf2c6edfe16f8fe1765b... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:50001) as https://chromium.googlesource.com/chromium/src/+/6b2ff2dfdf2c6edfe16f8fe1765b... |
