Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2795993002:
SupervisedUserNavigationThrottle: create only for supervised profiles (Closed)
|
Created:
3 years, 8 months ago by Marc Treib Modified:
3 years, 8 months ago Reviewers:
Bernhard Bauer CC:
chromium-reviews, pam+watch_chromium.org, skanuj+watch_chromium.org, melevin+watch_chromium.org, donnd+watch_chromium.org, jfweitz+watch_chromium.org, David Black, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, Jered, aberent Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupervisedUserNavigationThrottle: create only for supervised profiles
Before this CL, we were creating a SupervisedUserResourceThrottle for
each top-level navigation. If the current user isn't supervised, this
is pointless. One side-effect was that we were recording
ManagerUsers.Filter* histograms for all navigations, including for
non-supervised users.
With this CL, we create the throttle only for supervised users.
This is a spiritual re-land of https://crrev.com/2564043002/, but in the
meantime, much has changed (the ResourceThrottle has become a
NavigationThrottle).
BUG=671472
Review-Url: https://codereview.chromium.org/2795993002
Cr-Commit-Position: refs/heads/master@{#462045}
Committed: https://chromium.googlesource.com/chromium/src/+/bd6c71810f7c5f812e07e8c5f343a024d061cf0e
Patch Set 1 #
Total comments: 3
Patch Set 2 : simplify #
Total comments: 3
Patch Set 3 : review #
Messages
Total messages: 30 (22 generated)
The CQ bit was checked by treib@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.
treib@chromium.org changed reviewers: + bauerb@chromium.org
bauerb: Please review. Low-prio, feel free to postpone until convenient. aberent: FYI, since you own the test that was made flaky by the previous incarnation of this CL, https://crrev.com/2564043002/
https://codereview.chromium.org/2795993002/diff/1/chrome/browser/supervised_u... File chrome/browser/supervised_user/supervised_user_url_filter.h (right): https://codereview.chromium.org/2795993002/diff/1/chrome/browser/supervised_u... chrome/browser/supervised_user/supervised_user_url_filter.h:60: // Called whenever the filter changes. That's not correct, at least on trunk: This method is right now only called when the _whitelists_ change, not when e.g. user-defined exceptions change. Clients can observe the SupervisedUserService to get notified about any changes to URL filtering. As such, the additional call to OnSiteListUpdated() isn't actually necessary I think, because the SupervisedUserService will notify when it's turned on or off. I mean, we could of course change all of that and route all the notifications through this class, but then we should do it properly and do it for _all_ of them.
https://codereview.chromium.org/2795993002/diff/1/chrome/browser/supervised_u... File chrome/browser/supervised_user/supervised_user_url_filter.h (right): https://codereview.chromium.org/2795993002/diff/1/chrome/browser/supervised_u... chrome/browser/supervised_user/supervised_user_url_filter.h:60: // Called whenever the filter changes. On 2017/04/04 12:27:59, Bernhard (slow until Apr 6) wrote: > That's not correct, at least on trunk: This method is right now only called when > the _whitelists_ change, not when e.g. user-defined exceptions change. Clients > can observe the SupervisedUserService to get notified about any changes to URL > filtering. As such, the additional call to OnSiteListUpdated() isn't actually > necessary I think, because the SupervisedUserService will notify when it's > turned on or off. > > I mean, we could of course change all of that and route all the notifications > through this class, but then we should do it properly and do it for _all_ of > them. Ah, good catch! However, I noticed that we now have a SupervisedUserGoogleAuthNavigationThrottle, which checks the Profile's IsChild status. We could probably do the same here: Only create the throttle if profile->IsSupervised. That would allow us to get rid of all the changes to the URLFilter. WDYT?
The CQ bit was checked by treib@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 ========== Supervised Users: don't throttle navigations if filtering is disabled Before this CL, the SupervisedUserNavigationThrottle would check the SupervisedUserURLFilter for each top-level navigation. If the current user isn't supervised, this is pointless. One side-effect was that we were recording ManagerUsers.Filter* histograms for all navigations, including for non-supervised users. This CL adds an "enabled" field on SupervisedUserURLFilter, which is set to true iff the user is supervised. This allows us to early-out in the navigation throttle for regular users. This is a spiritual re-land of https://crrev.com/2564043002/, but in the meantime, much has changed (the ResourceThrottle has become a NavigationThrottle). BUG=671472 ========== to ========== SupervisedUserNavigationThrottle: create only for supervised profiles Before this CL, we were creating a SupervisedUserResourceThrottle for each top-level navigation. If the current user isn't supervised, this is pointless. One side-effect was that we were recording ManagerUsers.Filter* histograms for all navigations, including for non-supervised users. With this CL, we create the throttle only for supervised users. This is a spiritual re-land of https://crrev.com/2564043002/, but in the meantime, much has changed (the ResourceThrottle has become a NavigationThrottle). BUG=671472 ==========
PTAL again! (when you have time - not urgent at all) https://codereview.chromium.org/2795993002/diff/1/chrome/browser/supervised_u... File chrome/browser/supervised_user/supervised_user_url_filter.h (right): https://codereview.chromium.org/2795993002/diff/1/chrome/browser/supervised_u... chrome/browser/supervised_user/supervised_user_url_filter.h:60: // Called whenever the filter changes. On 2017/04/04 12:34:52, Marc Treib wrote: > On 2017/04/04 12:27:59, Bernhard (slow until Apr 6) wrote: > > That's not correct, at least on trunk: This method is right now only called > when > > the _whitelists_ change, not when e.g. user-defined exceptions change. Clients > > can observe the SupervisedUserService to get notified about any changes to URL > > filtering. As such, the additional call to OnSiteListUpdated() isn't actually > > necessary I think, because the SupervisedUserService will notify when it's > > turned on or off. > > > > I mean, we could of course change all of that and route all the notifications > > through this class, but then we should do it properly and do it for _all_ of > > them. > > Ah, good catch! > However, I noticed that we now have a > SupervisedUserGoogleAuthNavigationThrottle, which checks the Profile's IsChild > status. We could probably do the same here: Only create the throttle if > profile->IsSupervised. That would allow us to get rid of all the changes to the > URLFilter. WDYT? Patch set 2 does this, which massively simplifies the CL.
Nice, LGTM! https://codereview.chromium.org/2795993002/diff/20001/chrome/browser/supervis... File chrome/browser/supervised_user/supervised_user_navigation_throttle.cc (right): https://codereview.chromium.org/2795993002/diff/20001/chrome/browser/supervis... chrome/browser/supervised_user/supervised_user_navigation_throttle.cc:125: if (!profile->IsSupervised() || !navigation_handle->IsInMainFrame()) I would split these up into two separate checks. https://codereview.chromium.org/2795993002/diff/20001/chrome/browser/supervis... File chrome/browser/supervised_user/supervised_user_url_filter.h (right): https://codereview.chromium.org/2795993002/diff/20001/chrome/browser/supervis... chrome/browser/supervised_user/supervised_user_url_filter.h:61: // SetManualHosts/SetManualURLs. Thanks for adding the comment!
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 treib@chromium.org to run a CQ dry run
https://codereview.chromium.org/2795993002/diff/20001/chrome/browser/supervis... File chrome/browser/supervised_user/supervised_user_navigation_throttle.cc (right): https://codereview.chromium.org/2795993002/diff/20001/chrome/browser/supervis... chrome/browser/supervised_user/supervised_user_navigation_throttle.cc:125: if (!profile->IsSupervised() || !navigation_handle->IsInMainFrame()) On 2017/04/05 09:31:12, Bernhard (slow until Apr 6) wrote: > I would split these up into two separate checks. Done.
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by treib@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.
The CQ bit was checked by treib@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2795993002/#ps40001 (title: "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": 40001, "attempt_start_ts": 1491396696292650,
"parent_rev": "e0828110c362763556738df32990abbc38370ebc", "commit_rev":
"bd6c71810f7c5f812e07e8c5f343a024d061cf0e"}
Message was sent while issue was closed.
Description was changed from ========== SupervisedUserNavigationThrottle: create only for supervised profiles Before this CL, we were creating a SupervisedUserResourceThrottle for each top-level navigation. If the current user isn't supervised, this is pointless. One side-effect was that we were recording ManagerUsers.Filter* histograms for all navigations, including for non-supervised users. With this CL, we create the throttle only for supervised users. This is a spiritual re-land of https://crrev.com/2564043002/, but in the meantime, much has changed (the ResourceThrottle has become a NavigationThrottle). BUG=671472 ========== to ========== SupervisedUserNavigationThrottle: create only for supervised profiles Before this CL, we were creating a SupervisedUserResourceThrottle for each top-level navigation. If the current user isn't supervised, this is pointless. One side-effect was that we were recording ManagerUsers.Filter* histograms for all navigations, including for non-supervised users. With this CL, we create the throttle only for supervised users. This is a spiritual re-land of https://crrev.com/2564043002/, but in the meantime, much has changed (the ResourceThrottle has become a NavigationThrottle). BUG=671472 Review-Url: https://codereview.chromium.org/2795993002 Cr-Commit-Position: refs/heads/master@{#462045} Committed: https://chromium.googlesource.com/chromium/src/+/bd6c71810f7c5f812e07e8c5f343... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/bd6c71810f7c5f812e07e8c5f343... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
