Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1731963002:
Adding filtering behavior for NTP suggestions. (Closed)
DescriptionAdding filtering behavior for NTP suggestions.
For child accounts, do not suggest popular sites. For Whitelist entry
point suggestions filter the web sites that were already suggested. For
most visited suggestions, filter suggestions that are blocked for the
child (for example if a child visited a web site that would appear on
most visited, but the parent manually blocks it).
BUG=586097
Committed: https://crrev.com/9514b30ab99713fff7436137f2d803e52d97f361
Cr-Commit-Position: refs/heads/master@{#378016}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 11
Patch Set 4 : Addressing comments #
Total comments: 8
Patch Set 5 : #Patch Set 6 : Merge #Patch Set 7 : Merge #Patch Set 8 : Merge with master #Patch Set 9 : Removing wrongly added file #Messages
Total messages: 17 (7 generated)
Description was changed from ========== Adding filtering behavior for NTP suggestions. For child accounts, do not suggest popular sites. For Whitelist entry point suggestions filter the web sites that were already suggested. For most visited suggestions, filter suggestions that are blocked for the child (for example if a child visited a web site that would appear on most visited, but the parent manually blocks it). BUG=586097 ========== to ========== Adding filtering behavior for NTP suggestions. For child accounts, do not suggest popular sites. For Whitelist entry point suggestions filter the web sites that were already suggested. For most visited suggestions, filter suggestions that are blocked for the child (for example if a child visited a web site that would appear on most visited, but the parent manually blocks it). BUG=586097 ==========
atanasova@chromium.org changed reviewers: + bauerb@chromium.org, treib@chromium.org
Description was changed from ========== Adding filtering behavior for NTP suggestions. For child accounts, do not suggest popular sites. For Whitelist entry point suggestions filter the web sites that were already suggested. For most visited suggestions, filter suggestions that are blocked for the child (for example if a child visited a web site that would appear on most visited, but the parent manually blocks it). BUG=586097 ========== to ========== Adding filtering behavior for NTP suggestions. For child accounts, do not suggest popular sites. For Whitelist entry point suggestions filter the web sites that were already suggested. For most visited suggestions, filter suggestions that are blocked for the child (for example if a child visited a web site that would appear on most visited, but the parent manually blocks it). BUG=586097 ==========
https://codereview.chromium.org/1731963002/diff/40001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1731963002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:481: ->GetURLFilterForUIThread(); Like my late comment on the other CL: Please only do this if the profile is supervised. https://codereview.chromium.org/1731963002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:492: SupervisedUserURLFilter::FilteringBehavior::ALLOW) This should probably be "== BLOCK". (WARN isn't actually used, but while it's still there, we should treat it correctly.) https://codereview.chromium.org/1731963002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:509: ->GetURLFilterForUIThread(); nit: Move this down to where it's actually needed. https://codereview.chromium.org/1731963002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:586: // For unicorn child accounts popular sites suggestions will not be added. What's this "unicorn" thing you speak of? ;) (Just "child accounts" please!) https://codereview.chromium.org/1731963002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:611: // Skip suggestions already present in personal. or whitelists
https://codereview.chromium.org/1731963002/diff/40001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1731963002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:481: ->GetURLFilterForUIThread(); On 2016/02/25 10:34:32, Marc Treib wrote: > Like my late comment on the other CL: Please only do this if the profile is > supervised. For non-child accounts the behavior will always be ALLOW, right? I though that adding an extra piece of logic during the comparison might seem more complicated (e.g. if we leave this null in case of non-child accounts, or if we want to even check whether we have a child during that check). Obviously this is not complicated it by much, but it seemed like an additional check that needs to be remembered in the future. https://codereview.chromium.org/1731963002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:492: SupervisedUserURLFilter::FilteringBehavior::ALLOW) On 2016/02/25 10:34:32, Marc Treib wrote: > This should probably be "== BLOCK". (WARN isn't actually used, but while it's > still there, we should treat it correctly.) Done. https://codereview.chromium.org/1731963002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:509: ->GetURLFilterForUIThread(); On 2016/02/25 10:34:32, Marc Treib wrote: > nit: Move this down to where it's actually needed. Done. https://codereview.chromium.org/1731963002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:586: // For unicorn child accounts popular sites suggestions will not be added. On 2016/02/25 10:34:32, Marc Treib wrote: > What's this "unicorn" thing you speak of? ;) (Just "child accounts" please!) Done. https://codereview.chromium.org/1731963002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:611: // Skip suggestions already present in personal. On 2016/02/25 10:34:32, Marc Treib wrote: > or whitelists Done.
https://codereview.chromium.org/1731963002/diff/60001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1731963002/diff/60001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:493: continue; I would add braces around this if the condition is long enough to break it into multiple lines. https://codereview.chromium.org/1731963002/diff/60001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:586: MostVisitedSites::SuggestionsVector popular_sites_suggestions; I think I'd move this back down and just return an empty SuggestionsVector for supervised users. https://codereview.chromium.org/1731963002/diff/60001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:588: if (profile_->IsChild()) { Key this off IsSupervised()? Most of the code here doesn't make assumptions about which type of supervised user we're dealing with.
https://codereview.chromium.org/1731963002/diff/40001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1731963002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:481: ->GetURLFilterForUIThread(); On 2016/02/25 11:09:10, atanasova wrote: > On 2016/02/25 10:34:32, Marc Treib wrote: > > Like my late comment on the other CL: Please only do this if the profile is > > supervised. > > For non-child accounts the behavior will always be ALLOW, right? > I though that adding an extra piece of logic during the comparison might seem > more complicated (e.g. if we leave this null in case of non-child accounts, or > if we want to even check whether we have a child during that check). Obviously > this is not complicated it by much, but it seemed like an additional check that > needs to be remembered in the future. Well, my policy has always been to not even get a SUService if the profile isn't supervised. But seems that Bernhard disagrees on that, so...whatever. :) https://codereview.chromium.org/1731963002/diff/60001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1731963002/diff/60001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:588: if (profile_->IsChild()) { On 2016/02/25 11:14:48, Bernhard Bauer wrote: > Key this off IsSupervised()? Most of the code here doesn't make assumptions > about which type of supervised user we're dealing with. Well, it's essentially a product decision. Do we want popular sites for non-child SUs? Also, nit: No braces required here :)
https://codereview.chromium.org/1731963002/diff/60001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1731963002/diff/60001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:493: continue; On 2016/02/25 11:14:48, Bernhard Bauer wrote: > I would add braces around this if the condition is long enough to break it into > multiple lines. Done here and the other places that have the comparison (they break the same way). https://codereview.chromium.org/1731963002/diff/60001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:586: MostVisitedSites::SuggestionsVector popular_sites_suggestions; On 2016/02/25 11:14:48, Bernhard Bauer wrote: > I think I'd move this back down and just return an empty SuggestionsVector for > supervised users. Done. https://codereview.chromium.org/1731963002/diff/60001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:588: if (profile_->IsChild()) { On 2016/02/25 11:14:48, Bernhard Bauer wrote: > Key this off IsSupervised()? Most of the code here doesn't make assumptions > about which type of supervised user we're dealing with. I will ping Patrick about this, I though the agreement was to disable popular sites for child accounts until we have child specific suggestions.
https://codereview.chromium.org/1731963002/diff/60001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1731963002/diff/60001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:588: if (profile_->IsChild()) { On 2016/02/25 11:31:32, atanasova wrote: > On 2016/02/25 11:14:48, Bernhard Bauer wrote: > > Key this off IsSupervised()? Most of the code here doesn't make assumptions > > about which type of supervised user we're dealing with. > > I will ping Patrick about this, I though the agreement was to disable popular > sites for child accounts until we have child specific suggestions. As I mentioned on the email thread, on Android they are equivalent right now, so this is just about being consistent with what other code does. But then again, I'm okay with leaving that discussion for later if you want to get this landed, so I guess LGTM.
The CQ bit was checked by atanasova@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/1731963002/#ps160001 (title: "Removing wrongly added file")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1731963002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1731963002/160001
Message was sent while issue was closed.
Description was changed from ========== Adding filtering behavior for NTP suggestions. For child accounts, do not suggest popular sites. For Whitelist entry point suggestions filter the web sites that were already suggested. For most visited suggestions, filter suggestions that are blocked for the child (for example if a child visited a web site that would appear on most visited, but the parent manually blocks it). BUG=586097 ========== to ========== Adding filtering behavior for NTP suggestions. For child accounts, do not suggest popular sites. For Whitelist entry point suggestions filter the web sites that were already suggested. For most visited suggestions, filter suggestions that are blocked for the child (for example if a child visited a web site that would appear on most visited, but the parent manually blocks it). BUG=586097 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Adding filtering behavior for NTP suggestions. For child accounts, do not suggest popular sites. For Whitelist entry point suggestions filter the web sites that were already suggested. For most visited suggestions, filter suggestions that are blocked for the child (for example if a child visited a web site that would appear on most visited, but the parent manually blocks it). BUG=586097 ========== to ========== Adding filtering behavior for NTP suggestions. For child accounts, do not suggest popular sites. For Whitelist entry point suggestions filter the web sites that were already suggested. For most visited suggestions, filter suggestions that are blocked for the child (for example if a child visited a web site that would appear on most visited, but the parent manually blocks it). BUG=586097 Committed: https://crrev.com/9514b30ab99713fff7436137f2d803e52d97f361 Cr-Commit-Position: refs/heads/master@{#378016} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/9514b30ab99713fff7436137f2d803e52d97f361 Cr-Commit-Position: refs/heads/master@{#378016} |
