Move SupervisedUserUrlFilter to components [chromium/src : main]
Anthi Orfanou (Gerrit)
Attention is currently required from: chrome-family...@google.com.
Anthi Orfanou would like chrome-family...@google.com to review this change.
Move SupervisedUserUrlFilter to components
This factoring is done to allow iOS support of parental controls.
The parts that need to be derived from content (which cannot be
moved to components) are moved into a separate method which is
passed as a callback to the url filter.
Bug: b/265758915
Change-Id: I07ac5e2865586e43792d7fb944f098abcc3a26fa
---
M chrome/BUILD.gn
M chrome/android/BUILD.gn
M chrome/browser/BUILD.gn
M chrome/browser/feedback/android/family_info_feedback_source_unittest.cc
M chrome/browser/ntp_tiles/chrome_most_visited_sites_factory.cc
M chrome/browser/renderer_context_menu/render_view_context_menu.cc
M chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
M chrome/browser/search/search.cc
M chrome/browser/search/search_unittest.cc
D chrome/browser/supervised_user/extensions_utils.h
M chrome/browser/supervised_user/parental_control_metrics_unittest.cc
R chrome/browser/supervised_user/supervised_user_browser_utils.cc
A chrome/browser/supervised_user/supervised_user_browser_utils.h
M chrome/browser/supervised_user/supervised_user_navigation_observer.cc
M chrome/browser/supervised_user/supervised_user_navigation_observer.h
M chrome/browser/supervised_user/supervised_user_navigation_throttle.cc
M chrome/browser/supervised_user/supervised_user_navigation_throttle.h
M chrome/browser/supervised_user/supervised_user_navigation_throttle_browsertest.cc
M chrome/browser/supervised_user/supervised_user_pref_store.cc
M chrome/browser/supervised_user/supervised_user_service.cc
M chrome/browser/supervised_user/supervised_user_service.h
M chrome/browser/supervised_user/supervised_user_service_factory.cc
M chrome/browser/supervised_user/supervised_user_url_filter_browsertest.cc
M chrome/browser/supervised_user/supervised_user_url_filter_extensions_unittest.cc
M chrome/browser/ui/webui/family_link_user_internals/family_link_user_internals_message_handler.cc
M chrome/browser/ui/webui/family_link_user_internals/family_link_user_internals_message_handler.h
M chrome/browser/ui/webui/history/browsing_history_handler.cc
M chrome/test/BUILD.gn
M components/supervised_user/DEPS
M components/supervised_user/core/browser/BUILD.gn
R components/supervised_user/core/browser/supervised_user_url_filter.cc
R components/supervised_user/core/browser/supervised_user_url_filter.h
R components/supervised_user/core/browser/supervised_user_url_filter_unittest.cc
M components/supervised_user/core/common/supervised_user_constants.cc
M components/supervised_user/core/common/supervised_user_constants.h
35 files changed, 236 insertions(+), 201 deletions(-)
To view, visit change 4270994. To unsubscribe, or for help writing mail filters, visit settings.
Anthi Orfanou (Gerrit)
Attention is currently required from: chrome-family...@google.com.
Anthi Orfanou (Gerrit)
Attention is currently required from: Nohemi Fernandez, chrome-family...@google.com.
Anthi Orfanou would like Nohemi Fernandez to review this change.
To view, visit change 4270994. To unsubscribe, or for help writing mail filters, visit settings.
Anthi Orfanou (Gerrit)
Attention is currently required from: Alex Ilin, Anthi Orfanou, Nohemi Fernandez, chrome-family...@google.com.
Anthi Orfanou would like Alex Ilin to review this change.
Anthi Orfanou (Gerrit)
Attention is currently required from: Alex Ilin, Anthi Orfanou, Nohemi Fernandez, chrome-family...@google.com.
Patch set 4:Commit-Queue +1
gwsq (Gerrit)
Attention is currently required from: Nohemi Fernandez, Tomek Jurkiewicz, chrome-family...@google.com.
gwsq would like Tomek Jurkiewicz to review this change authored by Anthi Orfanou.
To view, visit change 4270994. To unsubscribe, or for help writing mail filters, visit settings.
gwsq (Gerrit)
Attention is currently required from: Nohemi Fernandez, Tomek Jurkiewicz.
Anthi Orfanou has uploaded this change for review.
gwsq (Gerrit)
Attention is currently required from: Nohemi Fernandez, Tomek Jurkiewicz.
Reviewer source(s):
t...@google.com is from context(googleclient/chrome/chromium_gwsq/chrome/browser/supervised_user/config.gwsq)
Anthi Orfanou (Gerrit)
Attention is currently required from: Nohemi Fernandez, Tomek Jurkiewicz.
1 comment:
Patchset:
Do we also need to move the test https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/supervised_user/supervised_user_url_filter_browsertest.cc to components?
It has a lot of dependencies on chrome/browser, some of which we intend to move on next steps (e.g. supervised_user_service).
To view, visit change 4270994. To unsubscribe, or for help writing mail filters, visit settings.
Tomek Jurkiewicz (Gerrit)
Attention is currently required from: Anthi Orfanou, Nohemi Fernandez.
3 comments:
File chrome/browser/supervised_user/supervised_user_browser_utils.h:
This doesn't look like moved with `git mv`. The problem is that the history of the file won't be preserved.
File chrome/browser/supervised_user/supervised_user_url_filter_extensions_unittest.cc:
Patch Set #4, Line 31:
base::BindRepeating([]() { return std::string(); }));The parameter list of lambda literal is not required here (as the lambda is parameterless).
File components/supervised_user/core/browser/supervised_user_url_filter.h:
Patch Set #4, Line 122:
explicit SupervisedUserURLFilter(The explicit keyword was used here to prevent the creation of a converting constructor (1 parameter). Since you're adding a parameter, the explicit keyword is no longer needed here.
To view, visit change 4270994. To unsubscribe, or for help writing mail filters, visit settings.
Anthi Orfanou (Gerrit)
Attention is currently required from: Alex Ilin, Nohemi Fernandez, Tomek Jurkiewicz.
3 comments:
File chrome/browser/supervised_user/supervised_user_browser_utils.h:
This doesn't look like moved with `git mv`. […]
I tried to recover this file (uploaded a new patchest with the old file, see diff between patchsets 5 and 6) and then used git mv to rename it (patchset 7), but git still treats it as a file deletion...
The file is relatively new (only one more version). Do we necessarily need to preserve the history?
File chrome/browser/supervised_user/supervised_user_url_filter_extensions_unittest.cc:
Patch Set #4, Line 31:
base::BindRepeating([]() { return std::string(); }));The parameter list of lambda literal is not required here (as the lambda is parameterless).
Done
File components/supervised_user/core/browser/supervised_user_url_filter.h:
Patch Set #4, Line 122:
explicit SupervisedUserURLFilter(
The explicit keyword was used here to prevent the creation of a converting constructor (1 parameter) […]
Done
To view, visit change 4270994. To unsubscribe, or for help writing mail filters, visit settings.
Anthi Orfanou (Gerrit)
Attention is currently required from: Tomek Jurkiewicz.
Anthi Orfanou removed Nohemi Fernandez from this change.
35 files changed, 238 insertions(+), 202 deletions(-)
Anthi Orfanou (Gerrit)
Attention is currently required from: Tomek Jurkiewicz.
1 comment:
Patchset:
I am sorry, I accidentally uploaded a wrong patch, which was mean to go on a linked CL (not on this one, I accidentally did commit amend and messed up this). I have re-uploaded the right patch.
To view, visit change 4270994. To unsubscribe, or for help writing mail filters, visit settings.
Anthi Orfanou (Gerrit)
Attention is currently required from: Alex Ilin, Tomek Jurkiewicz.
Alex Ilin (Gerrit)
Attention is currently required from: Anthi Orfanou, Tomek Jurkiewicz.
6 comments:
File chrome/browser/supervised_user/supervised_user_browser_utils.cc:
std::string GetCountryCode() {
std::string country;
variations::VariationsService* variations_service =
g_browser_process->variations_service();
if (variations_service) {
country = variations_service->GetStoredPermanentCountry();
if (country.empty()) {
country = variations_service->GetLatestCountry();
}
}
return country;
}nit: Please move it below the `IsSupportedChromeExtensionURL()` so that the declaration order in the .h file matches the definition order in the .cc file.
I can no longer find this rule in the style guide but I think it's still useful for visual navigation.
File components/supervised_user/core/browser/supervised_user_url_filter.h:
Patch Set #12, Line 139:
ShouldSkipParentManualAllowlistFilteringDuplicating this function in three different files feels like a regression. Could you move this helper to some utility file instead?
Patch Set #12, Line 29:
supervised_user`SupervisedUserURLFilter` also should be put inside of the `supervised_user` namespace.
Patch Set #12, Line 38:
CountryCallbackalternative naming suggestion: `CountryGetter` by analogy with [1]
Patch Set #12, Line 38:
typedef base::RepeatingCallback<std::string()> CountryCallback;nit: `using CountryCallback = base::RepeatingCallback<std::string()>`
In new code, using is preferable to typedef, because it provides a more consistent syntax with the rest of C++ and works with templates.
File components/supervised_user/core/browser/supervised_user_url_filter.cc:
Patch Set #12, Line 98:
supervised_usernit: should be redundant after you move this class in the namespace.
To view, visit change 4270994. To unsubscribe, or for help writing mail filters, visit settings.
Nohemi Fernandez (Gerrit)
Attention is currently required from: Anthi Orfanou, Tomek Jurkiewicz.
1 comment:
File components/supervised_user/core/browser/supervised_user_url_filter.cc:
Patch Set #12, Line 155:
CountryCallback country_callbackI would prefer to limit the number of callbacks that we introduce in the constructor to functions that are not available across platforms (e.g. extensions is only relevant for Desktop / ChromeOS platforms).
Here you can pass in the `VariationsService` directly and use it to retrieve the country below. This will also allow iOS to follow the same pattern for retrieving a country.
To view, visit change 4270994. To unsubscribe, or for help writing mail filters, visit settings.
Tomek Jurkiewicz (Gerrit)
Attention is currently required from: Alex Ilin, Anthi Orfanou.
1 comment:
File chrome/browser/supervised_user/supervised_user_browser_utils.h:
I don't think Git/Gerrit understands that a file was moved with a `git mv` command. […]
Yes, something I didn't know when making the comment (sorry, I just inferred that from some other very popular vc system that we use at google :))
To view, visit change 4270994. To unsubscribe, or for help writing mail filters, visit settings.
Anthi Orfanou (Gerrit)
Attention is currently required from: Alex Ilin, Nohemi Fernandez, Tomek Jurkiewicz.
8 comments:
File chrome/browser/supervised_user/supervised_user_browser_utils.h:
I don't think Git/Gerrit understands that a file was moved with a `git mv` command. […]
Done
File chrome/browser/supervised_user/supervised_user_browser_utils.cc:
std::string GetCountryCode() {
std::string country;
variations::VariationsService* variations_service =
g_browser_process->variations_service();
if (variations_service) {
country = variations_service->GetStoredPermanentCountry();
if (country.empty()) {
country = variations_service->GetLatestCountry();
}
}
return country;
}
nit: Please move it below the `IsSupportedChromeExtensionURL()` so that the declaration order in the […]
The method was removed.
File components/supervised_user/core/browser/supervised_user_url_filter.h:
Patch Set #12, Line 139:
ShouldSkipParentManualAllowlistFiltering
Duplicating this function in three different files feels like a regression. […]
done
Patch Set #12, Line 29:
supervised_user`SupervisedUserURLFilter` also should be put inside of the `supervised_user` namespace.
Done
Patch Set #12, Line 38:
typedef base::RepeatingCallback<std::string()> CountryCallback;
nit: `using CountryCallback = base::RepeatingCallback<std::string()>` […]
I'll keep this in mind, this specific instance is removed.
Patch Set #12, Line 38:
CountryCallback
alternative naming suggestion: `CountryGetter` by analogy with [1] […]
Removed.
File components/supervised_user/core/browser/supervised_user_url_filter.cc:
Patch Set #12, Line 98:
supervised_user
nit: should be redundant after you move this class in the namespace.
Done
Patch Set #12, Line 155:
CountryCallback country_callback
I would prefer to limit the number of callbacks that we introduce in the constructor to functions th […]
Done. I had tried this before but some tests were failing during the teardown. I can reproduce this problem again (e.g. https://ci.chromium.org/ui/p/chromium/builders/try/linux-chromeos-rel/b8788360007220834049/test-results?q=ExactID%3Aninja%3A%2F%2Fchrome%2Ftest%3Abrowser_tests%2FSupervisedUserIframeFilterTest.IframesWithSameDomainAsMainFrameAllowed%2FLocalWebApprovalsEnabled.0+VHash%3A282db19e8ac0a6be&clean=&sortby=&groupby=).
Originally I though that maybe I shouldn't be getting an services references from global context, so I went for the callback solution where I didn't encounter this.
I will look into fixing the failing tests, but so far it's not clear to me where is the problem
To view, visit change 4270994. To unsubscribe, or for help writing mail filters, visit settings.
Nohemi Fernandez (Gerrit)
Attention is currently required from: Alex Ilin, Anthi Orfanou, Tomek Jurkiewicz.
Patch set 20:Code-Review +1
2 comments:
Patchset:
Overall LGTM for the structure, leaving it to Alex and Tomek for additional detailed review.
File components/supervised_user/core/browser/supervised_user_url_filter.cc:
Patch Set #20, Line 157:
variations::VariationsService* variations_serviceThis looks good if we want to have exactly the same behavior as previously. To go one step further in ensuring we don't pull in unnecessary dependencies, could you confirm whether it is possible for the country to change during the lifecycle of the service?
If the country does not change dynamically it may be enough to pass it as a parameter in the factory, otherwise keeping it as is works well.
To view, visit change 4270994. To unsubscribe, or for help writing mail filters, visit settings.
Anthi Orfanou (Gerrit)
Attention is currently required from: Alex Ilin, Nohemi Fernandez, Tomek Jurkiewicz.
1 comment:
File components/supervised_user/core/browser/supervised_user_url_filter.cc:
Patch Set #20, Line 157:
variations::VariationsService* variations_service
This looks good if we want to have exactly the same behavior as previously. […]
Hi Nohemi, we have discussed this topic in 3P chat support with you and Tomek (February 20), where you have identified one such potential change in the country (https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/supervised_user/supervised_user_url_filter.cc;l=495;drc=55b21ed4b6909e270cf1618c6b10c24f4ac8c50e) and we decided to keep the logic as it is for now.
Id your comment about confirming manually this case?
Perhaps I should leave a todo comment to confirm this later?
To view, visit change 4270994. To unsubscribe, or for help writing mail filters, visit settings.
Alex Ilin (Gerrit)
Attention is currently required from: Anthi Orfanou, Nohemi Fernandez, Tomek Jurkiewicz.
2 comments:
File components/supervised_user/core/browser/supervised_user_url_filter.h:
Patch Set #20, Line 30:
namespace supervised_user {No need to open the `supervised_user` namespace twice. You can move the `SupervisedUserDenylist` forward declaration below, before SupervisedUserURLFilter:
```
namespace supervised_user {class SupervisedUserDenylist;
// This class manages the filtering behavior for URLs, i.e. it tells callers
// if a URL should be allowed or blocked. It uses information ...
class SupervisedUserURLFilter {
// ...
};
}
```From C++ style guide:
Namespaces wrap the entire source file after includes, gflags definitions/declarations and forward declarations of classes from other namespaces.
https://google.github.io/styleguide/cppguide.html#Namespaces
File components/supervised_user/core/browser/supervised_user_url_filter.cc:
Patch Set #20, Line 153:
namespace supervised_user {Please avoid closing and re-opening the same namespace repeatedly. You can open the `supervised_user` namespace at the top of the file only once. `supervised_user` might have a nested anonymous namespace like this:
```
namespace supervised_user {namespace {
bool IsSameDomain(const GURL& url1, const GURL& url2) {
return net::registry_controlled_domains::SameDomainOrHost(
url1, url2, EXCLUDE_PRIVATE_REGISTRIES);
}bool SetFilteringBehaviorResult(
SupervisedUserURLFilter::FilteringBehavior behavior,
SupervisedUserURLFilter::FilteringBehavior* result) {
// ...
}// ...
constexpr char kBlockedSitesCountHistogramName[] =
"FamilyUser.ManagedSiteListCount.Blocked";} // namespace
SupervisedUserURLFilter::SupervisedUserURLFilter(
ValidateURLSupportCallback check_webstore_url_callback,
variations::VariationsService* variations_service) // ...
// ...
// move this method down below, close to other `SupervisedUserURLFilter` methods.
SupervisedUserURLFilter::FilteringBehavior
GetBehaviorFromSafeSearchClassification(
safe_search_api::Classification classification) {
// ...
}} // namespace supervised_user
```
To view, visit change 4270994. To unsubscribe, or for help writing mail filters, visit settings.
Anthi Orfanou (Gerrit)
Attention is currently required from: Alex Ilin, Nohemi Fernandez, Tomek Jurkiewicz.
Patch set 24:Commit-Queue +1
4 comments:
Patchset:
Do we also need to move the test https://source.chromium. […]
Discussed on the chat, it will be moved but it can be done later.
Patchset:
There is one functional change between last reviewed patchet (21) and the present to address the errors in the browser tests: https://ci.chromium.org/ui/p/chromium/builders/try/linux-chromeos-rel/b8788010048926328433/test-results?q=ExactID%3Aninja%3A%2F%2Fchrome%2Ftest%3Abrowser_tests%2FSupervisedUserIframeFilterTest.IframesWithSameDomainAsMainFrameAllowed%2FLocalWebApprovalsEnabled.0+VHash%3A282db19e8ac0a6be&clean=&sortby=&groupby=
Url Filter no longer holds directly the variation service to obtain a country code it needs, but now has a delegate interface to obtain this information. The interface is implemented in supervised user service factory and passed down to the url filter.
File components/supervised_user/core/browser/supervised_user_url_filter.h:
Patch Set #20, Line 30:
namespace supervised_user {
No need to open the `supervised_user` namespace twice. […]
Done
File components/supervised_user/core/browser/supervised_user_url_filter.cc:
Patch Set #20, Line 153:
namespace supervised_user {
Please avoid closing and re-opening the same namespace repeatedly. […]
Done
To view, visit change 4270994. To unsubscribe, or for help writing mail filters, visit settings.
Anthi Orfanou (Gerrit)
Attention is currently required from: Anthi Orfanou.
Anthi Orfanou uploaded patch set #26 to this change.
Move SupervisedUserUrlFilter to components
This factoring is done to allow iOS support of parental controls.
The parts that need to be derived from content (which cannot be
moved to components) are moved into a separate method which is
accessed through a new interface (delegate) on the url filter.
Bug: b/265758915
Change-Id: I07ac5e2865586e43792d7fb944f098abcc3a26fa
---
M chrome/BUILD.gn
M chrome/android/BUILD.gn
M chrome/browser/BUILD.gn
M chrome/browser/feedback/android/family_info_feedback_source.cc
M chrome/browser/feedback/android/family_info_feedback_source_unittest.cc
M chrome/browser/ntp_tiles/chrome_most_visited_sites_factory.cc
M chrome/browser/renderer_context_menu/render_view_context_menu.cc
M chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
M chrome/browser/search/search.cc
M chrome/browser/search/search_unittest.cc
D chrome/browser/supervised_user/extensions_utils.h
M chrome/browser/supervised_user/parental_control_metrics_unittest.cc
R chrome/browser/supervised_user/supervised_user_browser_utils.cc
A chrome/browser/supervised_user/supervised_user_browser_utils.h
M chrome/browser/supervised_user/supervised_user_navigation_observer.cc
M chrome/browser/supervised_user/supervised_user_navigation_observer.h
M chrome/browser/supervised_user/supervised_user_navigation_throttle.cc
M chrome/browser/supervised_user/supervised_user_navigation_throttle.h
M chrome/browser/supervised_user/supervised_user_navigation_throttle_browsertest.cc
M chrome/browser/supervised_user/supervised_user_pref_store.cc
M chrome/browser/supervised_user/supervised_user_service.cc
M chrome/browser/supervised_user/supervised_user_service.h
M chrome/browser/supervised_user/supervised_user_service_factory.cc
M chrome/browser/supervised_user/supervised_user_service_factory.h
M chrome/browser/supervised_user/supervised_user_service_unittest.cc
M chrome/browser/supervised_user/supervised_user_url_filter_browsertest.cc
M chrome/browser/supervised_user/supervised_user_url_filter_extensions_unittest.cc
M chrome/browser/ui/webui/family_link_user_internals/family_link_user_internals_message_handler.cc
M chrome/browser/ui/webui/family_link_user_internals/family_link_user_internals_message_handler.h
M chrome/browser/ui/webui/history/browsing_history_handler.cc
M chrome/test/BUILD.gn
M components/supervised_user/DEPS
M components/supervised_user/core/browser/BUILD.gn
R components/supervised_user/core/browser/supervised_user_url_filter.cc
R components/supervised_user/core/browser/supervised_user_url_filter.h
R components/supervised_user/core/browser/supervised_user_url_filter_unittest.cc
M components/supervised_user/core/common/supervised_user_constants.cc
M components/supervised_user/core/common/supervised_user_constants.h
38 files changed, 528 insertions(+), 392 deletions(-)
To view, visit change 4270994. To unsubscribe, or for help writing mail filters, visit settings.
Anthi Orfanou (Gerrit)
Attention is currently required from: Alex Ilin, Tomek Jurkiewicz.
Anthi Orfanou uploaded patch set #27 to this change.
Move SupervisedUserUrlFilter to components
This refactoring is done to allow iOS support of parental controls.
38 files changed, 513 insertions(+), 392 deletions(-)
To view, visit change 4270994. To unsubscribe, or for help writing mail filters, visit settings.
Alex Ilin (Gerrit)
Attention is currently required from: Anthi Orfanou, Tomek Jurkiewicz.
6 comments:
File chrome/browser/supervised_user/supervised_user_service.h:
Patch Set #27, Line 267:
service_delegatenit: I'd go for the `url_filter_delegate` name here to make it clearer that this is a delegate for the `url_filter`. Imagine that you have two classes that need a ServiceDelegate.
File chrome/browser/supervised_user/supervised_user_service_factory.h:
Patch Set #27, Line 44:
class FilterServiceDelegateImplsupervised_user_service_factory.h shouldn't expose this class.
You could either declare this class in the supervised_user_service_factory.cc or in a separate file .h file.
File chrome/browser/supervised_user/supervised_user_service_factory.cc:
Patch Set #27, Line 78:
std::string country;unused, please remove
Patch Set #27, Line 79:
GetCountryCodeDo you want to keep this as a standalone function? If not, you could just inline its code here.
File components/supervised_user/core/browser/supervised_user_url_filter.h:
Patch Set #27, Line 104:
ServiceDelegatenit: does this name imply `SupervisedUserServiceDelegate`? I'd probably just name it `Delegate` since this class has only one delegate in `SupervisedUserURLFilter`.
Another design possibility is to make a delegate class that will be shared between all `components/supervised_user` classes. `components/signin` has a `SigninClient` [1] class like this.
File components/supervised_user/core/browser/supervised_user_url_filter.cc:
Patch Set #27, Line 482:
get()no need to call `get()` here. std::unique_ptr<> overloads `operator->()` so you can simply write `service_delegate_->GetCountryCode();`
To view, visit change 4270994. To unsubscribe, or for help writing mail filters, visit settings.
Anthi Orfanou (Gerrit)
Attention is currently required from: Alex Ilin, Nohemi Fernandez, Tomek Jurkiewicz.
6 comments:
File chrome/browser/supervised_user/supervised_user_service.h:
Patch Set #27, Line 267:
service_delegate
nit: I'd go for the `url_filter_delegate` name here to make it clearer that this is a delegate for t […]
Done
File chrome/browser/supervised_user/supervised_user_service_factory.h:
Patch Set #27, Line 44:
class FilterServiceDelegateImpl
supervised_user_service_factory.h shouldn't expose this class. […]
Done
File chrome/browser/supervised_user/supervised_user_service_factory.cc:
Patch Set #27, Line 78:
std::string country;unused, please remove
Done
Patch Set #27, Line 79:
GetCountryCodeDo you want to keep this as a standalone function? If not, you could just inline its code here.
Done
File components/supervised_user/core/browser/supervised_user_url_filter.h:
Patch Set #27, Line 104:
ServiceDelegate
nit: does this name imply `SupervisedUserServiceDelegate`? I'd probably just name it `Delegate` sinc […]
No, the name does not relate to the supervised user service, I just names it like that to specify that it is an entry point to other services (e.g. variation service). But since there is no other delegate within the class, I renamed.
I think I will try to keep things simple for now, and introduces interfaces as needed within supervised_user as I expect they will have different lifetimes (delegates per request vs per service).
File components/supervised_user/core/browser/supervised_user_url_filter.cc:
Patch Set #27, Line 482:
get()
no need to call `get()` here. […]
Done
To view, visit change 4270994. To unsubscribe, or for help writing mail filters, visit settings.
Alex Ilin (Gerrit)
Attention is currently required from: Anthi Orfanou.
Patch set 29:Code-Review +1
3 comments:
Patchset:
LGTM % compile error
File components/supervised_user/core/browser/supervised_user_url_filter.h:
Patch Set #27, Line 104:
ServiceDelegate
No, the name does not relate to the supervised user service, I just names it like that to specify th […]
Sounds good to me.
File components/supervised_user/core/browser/supervised_user_url_filter_unittest.cc:
Patch Set #29, Line 77:
ServiceThis need needs to be updated.
To view, visit change 4270994. To unsubscribe, or for help writing mail filters, visit settings.
Anthi Orfanou (Gerrit)
Attention is currently required from: Alex Ilin.
Anthi Orfanou (Gerrit)
Attention is currently required from: Alex Ilin.
Patch set 30:Commit-Queue +2
Anthi Orfanou (Gerrit)
Attention is currently required from: Alex Ilin, Dominic Battré.
Anthi Orfanou would like Dominic Battré to review this change.
M chrome/browser/supervised_user/supervised_user_service_unittest.cc
M chrome/browser/supervised_user/supervised_user_url_filter_browsertest.cc
M chrome/browser/supervised_user/supervised_user_url_filter_extensions_unittest.cc
M chrome/browser/ui/webui/family_link_user_internals/family_link_user_internals_message_handler.cc
M chrome/browser/ui/webui/family_link_user_internals/family_link_user_internals_message_handler.h
M chrome/browser/ui/webui/history/browsing_history_handler.cc
M chrome/test/BUILD.gn
M components/supervised_user/DEPS
M components/supervised_user/core/browser/BUILD.gn
R components/supervised_user/core/browser/supervised_user_url_filter.cc
R components/supervised_user/core/browser/supervised_user_url_filter.h
R components/supervised_user/core/browser/supervised_user_url_filter_unittest.cc
M components/supervised_user/core/common/supervised_user_constants.cc
M components/supervised_user/core/common/supervised_user_constants.h
37 files changed, 509 insertions(+), 392 deletions(-)
Anthi Orfanou (Gerrit)
Attention is currently required from: Alex Ilin, Dominic Battré.
1 comment:
Patchset:
Adding bat...@chromium.org to review dependencies on components/url_matcher.
To view, visit change 4270994. To unsubscribe, or for help writing mail filters, visit settings.
Anthi Orfanou (Gerrit)
File components/supervised_user/core/browser/supervised_user_url_filter_unittest.cc:
Patch Set #29, Line 77:
ServiceThis need needs to be updated.
Done
To view, visit change 4270994. To unsubscribe, or for help writing mail filters, visit settings.
Dominic Battré (Gerrit)
Attention is currently required from: Anthi Orfanou.
Patch set 30:Code-Review +1
1 comment:
Patchset:
components/supervised_user/DEPS LGTM (did not check the rest)
To view, visit change 4270994. To unsubscribe, or for help writing mail filters, visit settings.
Anthi Orfanou (Gerrit)
Attention is currently required from: Anthi Orfanou.
Patch set 30:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
Move SupervisedUserUrlFilter to components
This refactoring is done to allow iOS support of parental controls.
The parts that need to be derived from content (which cannot be
moved to components) are moved into a separate method which is
accessed through a new interface (delegate) on the url filter.
Bug: b/265758915
Change-Id: I07ac5e2865586e43792d7fb944f098abcc3a26fa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4270994
Reviewed-by: Alex Ilin <alex...@chromium.org>
Reviewed-by: Dominic Battré <bat...@chromium.org>
Commit-Queue: Anthi Orfanou <ant...@google.com>
Cr-Commit-Position: refs/heads/main@{#1114453}
Nohemi Fernandez (Gerrit)
1 comment:
File components/supervised_user/core/browser/supervised_user_url_filter.cc:
Patch Set #20, Line 157:
variations::VariationsService* variations_serviceHi Nohemi, we have discussed this topic in 3P chat support with you and Tomek (February 20), where y […]
Closing as resolved to get country code inline when necessary.
To view, visit change 4270994. To unsubscribe, or for help writing mail filters, visit settings.