Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2743923002:
Network traffic annotation added to safe_search_url_checker. (Closed)
|
Created:
3 years, 9 months ago by Ramin Halavati Modified:
3 years, 9 months ago CC:
chromium-reviews, battre, msramek Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to safe_search_url_checker.
Network traffic annotation is added to network request of
chrome/browser/safe_search_api/safe_search_url_checker.cc.
BUG=656607
Review-Url: https://codereview.chromium.org/2743923002
Cr-Commit-Position: refs/heads/master@{#457436}
Committed: https://chromium.googlesource.com/chromium/src/+/14a2141814db4d7f906a3bf7902ecb2a99a82f08
Patch Set 1 #
Total comments: 18
Patch Set 2 : Annotation updated. #
Total comments: 7
Patch Set 3 : Comments addressed. #Patch Set 4 : nits #
Total comments: 2
Patch Set 5 : Annotation updated and moved. #Patch Set 6 : nits #Patch Set 7 : Annotation updated #
Total comments: 2
Patch Set 8 : Annotation updated. #
Messages
Total messages: 19 (6 generated)
rhalavati@chromium.org changed reviewers: + treib@chromium.org
Mark, I've added annotation template to safe_search_url_checker, please suggest content for it. I will repeat the references here: Please take a look at the protobuf scheme in: https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... for the definition of the annotations. You can find a sample annotation in: https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s... Entriprise policy options are here: https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/ch... And more description on enterprise policy settings is here: http://dev.chromium.org/administrators/policy-list-3 Please tell me if you need any clarifications from my side. If you want to learn more, see: https://docs.google.com/document/d/1FWEFTHzdqIA1wHUo_DuSpJjppW22otCMPCOA5NumhJU.
treib@chromium.org changed reviewers: + msramek@chromium.org
https://codereview.chromium.org/2743923002/diff/1/chrome/browser/safe_search_... File chrome/browser/safe_search_api/safe_search_url_checker.cc (right): https://codereview.chromium.org/2743923002/diff/1/chrome/browser/safe_search_... chrome/browser/safe_search_api/safe_search_url_checker.cc:54: net::DefineNetworkTrafficAnnotation("...", R"( safe_search_api https://codereview.chromium.org/2743923002/diff/1/chrome/browser/safe_search_... chrome/browser/safe_search_api/safe_search_url_checker.cc:56: sender: "..." "SafeSearch API"? (That's really the target rather than the sender, but I don't know what else to put here.) https://codereview.chromium.org/2743923002/diff/1/chrome/browser/safe_search_... chrome/browser/safe_search_api/safe_search_url_checker.cc:57: description: "..." Checks whether a given URL (or set of URLs) is considered safe by Google SafeSearch. https://codereview.chromium.org/2743923002/diff/1/chrome/browser/safe_search_... chrome/browser/safe_search_api/safe_search_url_checker.cc:59: data: "..." URL(s) to be checked. https://codereview.chromium.org/2743923002/diff/1/chrome/browser/safe_search_... chrome/browser/safe_search_api/safe_search_url_checker.cc:60: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER GOOGLE_OWNED_SERVICE https://codereview.chromium.org/2743923002/diff/1/chrome/browser/safe_search_... chrome/browser/safe_search_api/safe_search_url_checker.cc:63: cookies_allowed: false/true false https://codereview.chromium.org/2743923002/diff/1/chrome/browser/safe_search_... chrome/browser/safe_search_api/safe_search_url_checker.cc:64: cookies_store: "..." n/a https://codereview.chromium.org/2743923002/diff/1/chrome/browser/safe_search_... chrome/browser/safe_search_api/safe_search_url_checker.cc:65: setting: "..." Right now, this functionality is only used for child accounts (which aren't publicly launched yet), and of course the child can't turn this off by themselves. The child's parents will have access to a setting that enables or disabled this feature. +msramek was working on some other feature that would use the SafeSearch API, but I don't know what came of that.
Comments addressed, please review. https://codereview.chromium.org/2743923002/diff/1/chrome/browser/safe_search_... File chrome/browser/safe_search_api/safe_search_url_checker.cc (right): https://codereview.chromium.org/2743923002/diff/1/chrome/browser/safe_search_... chrome/browser/safe_search_api/safe_search_url_checker.cc:54: net::DefineNetworkTrafficAnnotation("...", R"( On 2017/03/10 10:47:56, Marc Treib wrote: > safe_search_api Done. https://codereview.chromium.org/2743923002/diff/1/chrome/browser/safe_search_... chrome/browser/safe_search_api/safe_search_url_checker.cc:56: sender: "..." On 2017/03/10 10:47:56, Marc Treib wrote: > "SafeSearch API"? (That's really the target rather than the sender, but I don't > know what else to put here.) What about 'Safe Search URL Checker'? https://codereview.chromium.org/2743923002/diff/1/chrome/browser/safe_search_... chrome/browser/safe_search_api/safe_search_url_checker.cc:57: description: "..." On 2017/03/10 10:47:56, Marc Treib wrote: > Checks whether a given URL (or set of URLs) is considered safe by Google > SafeSearch. Done. https://codereview.chromium.org/2743923002/diff/1/chrome/browser/safe_search_... chrome/browser/safe_search_api/safe_search_url_checker.cc:60: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/03/10 10:47:56, Marc Treib wrote: > GOOGLE_OWNED_SERVICE Done. https://codereview.chromium.org/2743923002/diff/1/chrome/browser/safe_search_... chrome/browser/safe_search_api/safe_search_url_checker.cc:63: cookies_allowed: false/true On 2017/03/10 10:47:56, Marc Treib wrote: > false Done. https://codereview.chromium.org/2743923002/diff/1/chrome/browser/safe_search_... chrome/browser/safe_search_api/safe_search_url_checker.cc:64: cookies_store: "..." On 2017/03/10 10:47:56, Marc Treib wrote: > n/a Done. https://codereview.chromium.org/2743923002/diff/1/chrome/browser/safe_search_... chrome/browser/safe_search_api/safe_search_url_checker.cc:65: setting: "..." On 2017/03/10 10:47:56, Marc Treib wrote: > Right now, this functionality is only used for child accounts (which aren't > publicly launched yet), and of course the child can't turn this off by > themselves. The child's parents will have access to a setting that enables or > disabled this feature. > > +msramek was working on some other feature that would use the SafeSearch API, > but I don't know what came of that. Acknowledged. https://codereview.chromium.org/2743923002/diff/1/chrome/browser/safe_search_... chrome/browser/safe_search_api/safe_search_url_checker.cc:71: } Do these two policies have any effect on this feature: http://dev.chromium.org/administrators/policy-list-3#ForceSafeSearch http://dev.chromium.org/administrators/policy-list-3#ForceGoogleSafeSearch
https://codereview.chromium.org/2743923002/diff/1/chrome/browser/safe_search_... File chrome/browser/safe_search_api/safe_search_url_checker.cc (right): https://codereview.chromium.org/2743923002/diff/1/chrome/browser/safe_search_... chrome/browser/safe_search_api/safe_search_url_checker.cc:56: sender: "..." On 2017/03/10 11:23:34, Ramin Halavati wrote: > On 2017/03/10 10:47:56, Marc Treib wrote: > > "SafeSearch API"? (That's really the target rather than the sender, but I > don't > > know what else to put here.) > > What about 'Safe Search URL Checker'? Yup, that sounds good! https://codereview.chromium.org/2743923002/diff/1/chrome/browser/safe_search_... chrome/browser/safe_search_api/safe_search_url_checker.cc:71: } On 2017/03/10 11:23:34, Ramin Halavati wrote: > Do these two policies have any effect on this feature: > > http://dev.chromium.org/administrators/policy-list-3#ForceSafeSearch > http://dev.chromium.org/administrators/policy-list-3#ForceGoogleSafeSearch No, they're unrelated. Those (force-)enable SafeSearch on Google searches. https://codereview.chromium.org/2743923002/diff/20001/chrome/browser/safe_sea... File chrome/browser/safe_search_api/safe_search_url_checker.cc (right): https://codereview.chromium.org/2743923002/diff/20001/chrome/browser/safe_sea... chrome/browser/safe_search_api/safe_search_url_checker.cc:60: trigger: "..." Oh, this one is still missing. For child accounts, it's triggered on every navigation (if the parent has enabled the feature). But this class just implements talking to the API, so it's really independent. Maybe msramek can comment on other (planned) uses. Anyway, I'd be concerned about putting too much detail here, as that could easily get outdated.
https://codereview.chromium.org/2743923002/diff/20001/chrome/browser/safe_sea... File chrome/browser/safe_search_api/safe_search_url_checker.cc (right): https://codereview.chromium.org/2743923002/diff/20001/chrome/browser/safe_sea... chrome/browser/safe_search_api/safe_search_url_checker.cc:60: trigger: "..." On 2017/03/10 11:39:20, Marc Treib wrote: > Oh, this one is still missing. > > For child accounts, it's triggered on every navigation (if the parent has > enabled the feature). But this class just implements talking to the API, so it's > really independent. Maybe msramek can comment on other (planned) uses. > > Anyway, I'd be concerned about putting too much detail here, as that could > easily get outdated. The other usecase, for which I extracted this code out, is on hold. We can therefore use Marc's description. "If the parent enabled this feature for the child account, this is sent for every navigation." However, I agree that this could get outdated, and therefore I suggest moving the annotation a layer above (to the supervised_users/ code). https://codereview.chromium.org/2743923002/diff/20001/chrome/browser/safe_sea... chrome/browser/safe_search_api/safe_search_url_checker.cc:67: "This feature is only used in child accounts and cannot be " Shouldn't we mention the SU dashboard then? https://codereview.chromium.org/2743923002/diff/20001/chrome/browser/safe_sea... chrome/browser/safe_search_api/safe_search_url_checker.cc:70: [POLICY_NAME] { Ramin, how do we want to handle these cases? The relation of parent<->child is very similar to that of admin<->corp user. The parent can manage certain settings the same way as admin does. So while there's technically no enterprise policy, there is something very similar.
Annotation updated, please review. https://codereview.chromium.org/2743923002/diff/20001/chrome/browser/safe_sea... File chrome/browser/safe_search_api/safe_search_url_checker.cc (right): https://codereview.chromium.org/2743923002/diff/20001/chrome/browser/safe_sea... chrome/browser/safe_search_api/safe_search_url_checker.cc:60: trigger: "..." On 2017/03/14 16:13:02, msramek wrote: > On 2017/03/10 11:39:20, Marc Treib wrote: > > Oh, this one is still missing. > > > > For child accounts, it's triggered on every navigation (if the parent has > > enabled the feature). But this class just implements talking to the API, so > it's > > really independent. Maybe msramek can comment on other (planned) uses. > > > > Anyway, I'd be concerned about putting too much detail here, as that could > > easily get outdated. > > The other usecase, for which I extracted this code out, is on hold. We can > therefore use Marc's description. > > "If the parent enabled this feature for the child account, this is sent for > every navigation." > > However, I agree that this could get outdated, and therefore I suggest moving > the annotation a layer above (to the supervised_users/ code). Done. https://codereview.chromium.org/2743923002/diff/20001/chrome/browser/safe_sea... chrome/browser/safe_search_api/safe_search_url_checker.cc:67: "This feature is only used in child accounts and cannot be " On 2017/03/14 16:13:02, msramek wrote: > Shouldn't we mention the SU dashboard then? Done. https://codereview.chromium.org/2743923002/diff/20001/chrome/browser/safe_sea... chrome/browser/safe_search_api/safe_search_url_checker.cc:70: [POLICY_NAME] { On 2017/03/14 16:13:02, msramek wrote: > Ramin, how do we want to handle these cases? > > The relation of parent<->child is very similar to that of admin<->corp user. The > parent can manage certain settings the same way as admin does. So while there's > technically no enterprise policy, there is something very similar. As discussed, we will keep it free-form now and if the number of cases like this decrease, we will add new fields for it.
lgtm with one more nit https://codereview.chromium.org/2743923002/diff/60001/chrome/browser/safe_sea... File chrome/browser/safe_search_api/safe_search_url_checker.cc (right): https://codereview.chromium.org/2743923002/diff/60001/chrome/browser/safe_sea... chrome/browser/safe_search_api/safe_search_url_checker.cc:71: "Supervised Users dashboard." s/Supervised Users/the family/ ("Supervised Users" are the Chrome-specific thing; these don't have access to the SafeSearch API. Only child accounts do, hence "family dashboard".)
Annotation updated and moved to supervised_user_url_filter.cc. Please review. https://codereview.chromium.org/2743923002/diff/60001/chrome/browser/safe_sea... File chrome/browser/safe_search_api/safe_search_url_checker.cc (right): https://codereview.chromium.org/2743923002/diff/60001/chrome/browser/safe_sea... chrome/browser/safe_search_api/safe_search_url_checker.cc:71: "Supervised Users dashboard." On 2017/03/16 09:05:38, Marc Treib wrote: > s/Supervised Users/the family/ > > ("Supervised Users" are the Chrome-specific thing; these don't have access to > the SafeSearch API. Only child accounts do, hence "family dashboard".) Done.
LGTM, thanks! https://codereview.chromium.org/2743923002/diff/120001/chrome/browser/supervi... File chrome/browser/supervised_user/supervised_user_url_filter.cc (right): https://codereview.chromium.org/2743923002/diff/120001/chrome/browser/supervi... chrome/browser/supervised_user/supervised_user_url_filter.cc:535: net::DefineNetworkTrafficAnnotation("supervised_users_url_filter", R"( nit: supervised_user_url_filter (remove one "s")
Thank you, comment addressed. https://codereview.chromium.org/2743923002/diff/120001/chrome/browser/supervi... File chrome/browser/supervised_user/supervised_user_url_filter.cc (right): https://codereview.chromium.org/2743923002/diff/120001/chrome/browser/supervi... chrome/browser/supervised_user/supervised_user_url_filter.cc:535: net::DefineNetworkTrafficAnnotation("supervised_users_url_filter", R"( On 2017/03/16 13:35:59, Marc Treib wrote: > nit: supervised_user_url_filter (remove one "s") Done.
LGTM
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the patchset: https://codereview.chromium.org/2743923002/#ps140001 (title: "Annotation updated.")
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": 140001, "attempt_start_ts": 1489673470961540,
"parent_rev": "2b9c05fb215e52b9b360eb11118e3ee5c3b657e6", "commit_rev":
"14a2141814db4d7f906a3bf7902ecb2a99a82f08"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to safe_search_url_checker. Network traffic annotation is added to network request of chrome/browser/safe_search_api/safe_search_url_checker.cc. BUG=656607 ========== to ========== Network traffic annotation added to safe_search_url_checker. Network traffic annotation is added to network request of chrome/browser/safe_search_api/safe_search_url_checker.cc. BUG=656607 Review-Url: https://codereview.chromium.org/2743923002 Cr-Commit-Position: refs/heads/master@{#457436} Committed: https://chromium.googlesource.com/chromium/src/+/14a2141814db4d7f906a3bf7902e... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/14a2141814db4d7f906a3bf7902e... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
