Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1954973004:
Remove MostVisitedSites => SupervisedUserService dep. (Closed)
|
Created:
4 years, 7 months ago by sfiera Modified:
4 years, 7 months ago Reviewers:
Marc Treib CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove MostVisitedSites => SupervisedUserService dep.
Replace SupervisedUserService in MostVisitedSites with an interface.
Implement it in most_visited_sites_bridge.cc on top of
SupervisedUserService.
BUG=603026
Committed: https://crrev.com/79e643bb8dec304b745f91e83a112d19c2a62eb7
Cr-Commit-Position: refs/heads/master@{#393553}
Patch Set 1 #
Total comments: 4
Patch Set 2 : #Patch Set 3 : #
Total comments: 30
Patch Set 4 : #
Total comments: 3
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #
Messages
Total messages: 24 (9 generated)
sfiera@chromium.org changed reviewers: + treib@chromium.org
This is a nasty first cut and I wanted to show it to you before I spent much time making it presentable. The obvious alternative would be to get //c/b/supervised_user/... into its own component, but I assume we want to leave that yak unshaven. If I do polish this, I think I'll put an accessor for is_child_profile in the interface as well, since it ties into the same theme.
Yup, componentizing c/b/supervised_user/ is not something I'd like to start... Generally, I think this is the right approach, though IMO it can be simplified quite a bit. Some first comments below. https://codereview.chromium.org/1954973004/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/1954973004/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.h:61: virtual void RemoveObserver(Observer* observer) = 0; There'll only ever be one observer, namely the MostVisitedSites instance, right? Then let's make that explicit. https://codereview.chromium.org/1954973004/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.h:62: virtual SupervisedUserURLFilter* GetURLFilterForUIThread() = 0; Like this, you still have a dependency on SupervisedUserURLFilter. Fortunately, it's only used to check if a given URL is blocked. So, make this bool IsURLBlocked(const GURL&) instead?
Now ready for a proper review. https://codereview.chromium.org/1954973004/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/1954973004/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.h:61: virtual void RemoveObserver(Observer* observer) = 0; On 2016/05/06 13:24:11, Marc Treib wrote: > There'll only ever be one observer, namely the MostVisitedSites instance, right? > Then let's make that explicit. Done. https://codereview.chromium.org/1954973004/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.h:62: virtual SupervisedUserURLFilter* GetURLFilterForUIThread() = 0; On 2016/05/06 13:24:11, Marc Treib wrote: > Like this, you still have a dependency on SupervisedUserURLFilter. Fortunately, > it's only used to check if a given URL is blocked. So, make this > bool IsURLBlocked(const GURL&) > instead? Done.
I think the CL description could be a bit more descriptive ;) https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:392: if (supervisor_->IsBlocked(visited.url)) { nit: no braces required https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:425: if (supervisor_->IsBlocked(GURL(suggestion.url()))) { Also here https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:471: if (supervisor_->IsBlocked(whitelist.entry_point)) { Aaand again :) https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.h:53: virtual void OnBlockedSitesChanged() {} nit: pure virtual? https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.h:148: // SupervisedUserServiceObserver implementation. MostVisitedSitesSupervisor::Observer https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites_bridge.cc (right): https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites_bridge.cc:63: MostVisitedSitesBridge::SupervisorBridge::SetObserver(Observer* new_observer) { Can this method move into MostVisitedSitesSupervisor? IMO it fits better there (even if it won't be a pure interface anymore). WDYT? https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites_bridge.cc:74: SupervisedUserServiceFactory::GetForProfile(profile_); wrong indent https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites_bridge.cc:77: SupervisedUserURLFilter::FilteringBehavior::BLOCK; here too https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites_bridge.cc:84: SupervisedUserServiceFactory::GetForProfile(profile_); and here https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites_bridge.cc:86: results.emplace_back(Whitelist{ I think when called like this, emplace_back will do the exact same thing as push_back. If you actually want in-place construction, you need to leave out the "Whitelist", just pass the args directly to emplace_back. (You'll probably have to define an explicit ctor for that to work.) https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites_bridge.cc:100: if (observer_) { nit: no braces https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites_bridge.h (right): https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites_bridge.h:63: std::unique_ptr<Observer> observer_; Hm, can we rename this to something more explicit, or at least add a comment? When first reading this, I got confused about all the observers around here.
Description was changed from ========== Remove MostVisitedSites => SupervisedUserService dep. BUG=603026 ========== to ========== Remove MostVisitedSites => SupervisedUserService dep. Replace SupervisedUserService in MostVisitedSites with an interface. Implement it in most_visited_sites_bridge.cc on top of SupervisedUserService. BUG=603026 ==========
Description was changed from ========== Remove MostVisitedSites => SupervisedUserService dep. Replace SupervisedUserService in MostVisitedSites with an interface. Implement it in most_visited_sites_bridge.cc on top of SupervisedUserService. BUG=603026 ========== to ========== Remove MostVisitedSites => SupervisedUserService dep. Replace SupervisedUserService in MostVisitedSites with an interface. Implement it in most_visited_sites_bridge.cc on top of SupervisedUserService. BUG=603026 ==========
https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:392: if (supervisor_->IsBlocked(visited.url)) { On 2016/05/09 08:22:48, Marc Treib wrote: > nit: no braces required Done, but the braces weren't mine. https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:425: if (supervisor_->IsBlocked(GURL(suggestion.url()))) { On 2016/05/09 08:22:48, Marc Treib wrote: > Also here Also here. https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:471: if (supervisor_->IsBlocked(whitelist.entry_point)) { On 2016/05/09 08:22:48, Marc Treib wrote: > Aaand again :) Not mine either. https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.h:53: virtual void OnBlockedSitesChanged() {} On 2016/05/09 08:22:48, Marc Treib wrote: > nit: pure virtual? Done. https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.h:148: // SupervisedUserServiceObserver implementation. On 2016/05/09 08:22:48, Marc Treib wrote: > MostVisitedSitesSupervisor::Observer Done. https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites_bridge.cc (right): https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites_bridge.cc:63: MostVisitedSitesBridge::SupervisorBridge::SetObserver(Observer* new_observer) { On 2016/05/09 08:22:48, Marc Treib wrote: > Can this method move into MostVisitedSitesSupervisor? IMO it fits better there > (even if it won't be a pure interface anymore). WDYT? A rough check of Chromium code suggests that of AddObserver() methods, there are: 101 pure virtual https://cs.corp.google.com/search/?q=package:clankium+void%5C+AddObserver.*%3...; 193 override https://cs.corp.google.com/search/?q=package:clankium+void%5C+AddObserver.*ov...; 302 neither https://cs.corp.google.com/search/?q=package:clankium+void%5C+AddObserver.*%5C); Spot-checking a couple of samples from category 3 suggests that most are concrete classes, no pure virtuals, so I think that what you suggest is generally not The Chromium Way. https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites_bridge.cc:74: SupervisedUserServiceFactory::GetForProfile(profile_); On 2016/05/09 08:22:48, Marc Treib wrote: > wrong indent Done. https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites_bridge.cc:77: SupervisedUserURLFilter::FilteringBehavior::BLOCK; On 2016/05/09 08:22:48, Marc Treib wrote: > here too Done. https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites_bridge.cc:84: SupervisedUserServiceFactory::GetForProfile(profile_); On 2016/05/09 08:22:48, Marc Treib wrote: > and here Done. https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites_bridge.cc:86: results.emplace_back(Whitelist{ On 2016/05/09 08:22:48, Marc Treib wrote: > I think when called like this, emplace_back will do the exact same thing as > push_back. If you actually want in-place construction, you need to leave out the > "Whitelist", just pass the args directly to emplace_back. (You'll probably have > to define an explicit ctor for that to work.) I tried without the explicit ctor, and you're right, it won't work without one. I do think that the compiler is allowed to elide the copies given the way I've written it, and probably does. Do you want me to change this, or do more investigation? My inclination was to write something clear that the compiler can probably optimize, and not worry about it too much :) https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites_bridge.cc:100: if (observer_) { On 2016/05/09 08:22:48, Marc Treib wrote: > nit: no braces (OK, these were mine) https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites_bridge.h (right): https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites_bridge.h:63: std::unique_ptr<Observer> observer_; On 2016/05/09 08:22:48, Marc Treib wrote: > Hm, can we rename this to something more explicit, or at least add a comment? > When first reading this, I got confused about all the observers around here. Done.
LGTM, thanks! https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:392: if (supervisor_->IsBlocked(visited.url)) { On 2016/05/09 09:33:35, sfiera wrote: > On 2016/05/09 08:22:48, Marc Treib wrote: > > nit: no braces required > > Done, but the braces weren't mine. Your change made the condition fit on one line, so made the braces unnecessary :) (The rule is roughly: No braces if both the condition and the body fit on one line each. To be fair, it's not followed everywhere in Chromium, and the more general rule is "do what the code around you does".) https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites_bridge.cc (right): https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites_bridge.cc:63: MostVisitedSitesBridge::SupervisorBridge::SetObserver(Observer* new_observer) { On 2016/05/09 09:33:35, sfiera wrote: > On 2016/05/09 08:22:48, Marc Treib wrote: > > Can this method move into MostVisitedSitesSupervisor? IMO it fits better there > > (even if it won't be a pure interface anymore). WDYT? > > A rough check of Chromium code suggests that of AddObserver() methods, there > are: > > 101 pure virtual > https://cs.corp.google.com/search/?q=package:clankium+void%5C+AddObserver.*%3...; > 193 override > https://cs.corp.google.com/search/?q=package:clankium+void%5C+AddObserver.*ov...; > 302 neither > https://cs.corp.google.com/search/?q=package:clankium+void%5C+AddObserver.*%5C); > > Spot-checking a couple of samples from category 3 suggests that most are > concrete classes, no pure virtuals, so I think that what you suggest is > generally not The Chromium Way. Okay, fair enough. My rationale was that the method has quite specific semantics, so every implementation would basically need to duplicate this exact implementation. But since there'll probably only ever be this one implementation, I guess that doesn't matter. https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites_bridge.cc:86: results.emplace_back(Whitelist{ On 2016/05/09 09:33:35, sfiera wrote: > On 2016/05/09 08:22:48, Marc Treib wrote: > > I think when called like this, emplace_back will do the exact same thing as > > push_back. If you actually want in-place construction, you need to leave out > the > > "Whitelist", just pass the args directly to emplace_back. (You'll probably > have > > to define an explicit ctor for that to work.) > > I tried without the explicit ctor, and you're right, it won't work without one. > I do think that the compiler is allowed to elide the copies given the way I've > written it, and probably does. > > Do you want me to change this, or do more investigation? My inclination was to > write something clear that the compiler can probably optimize, and not worry > about it too much :) Eh, it's fine. I guess I'd slightly prefer push_back since AFAIK it'll do the exact same thing, and this is sort of an abuse of emplace_back IMO. But it doesn't really matter. https://codereview.chromium.org/1954973004/diff/60001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/1954973004/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.h:63: virtual void SetObserver(Observer* new_observer) = 0; nit: I'd call this just "observer" - any reason for the "new_" prefix? https://codereview.chromium.org/1954973004/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.h:149: void OnBlockedSitesChanged() override; Not your doing, but this can be private.
https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:392: if (supervisor_->IsBlocked(visited.url)) { On 2016/05/09 09:55:12, Marc Treib wrote: > On 2016/05/09 09:33:35, sfiera wrote: > > On 2016/05/09 08:22:48, Marc Treib wrote: > > > nit: no braces required > > > > Done, but the braces weren't mine. > > Your change made the condition fit on one line, so made the braces unnecessary > :) > (The rule is roughly: No braces if both the condition and the body fit on one > line each. To be fair, it's not followed everywhere in Chromium, and the more > general rule is "do what the code around you does".) I guess I should have been running git cl format anyway, but I've done so now and will try to do so in the future because I don't want to remember rules like that :) https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites_bridge.cc (right): https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites_bridge.cc:63: MostVisitedSitesBridge::SupervisorBridge::SetObserver(Observer* new_observer) { On 2016/05/09 09:55:12, Marc Treib wrote: > On 2016/05/09 09:33:35, sfiera wrote: > > On 2016/05/09 08:22:48, Marc Treib wrote: > > > Can this method move into MostVisitedSitesSupervisor? IMO it fits better > there > > > (even if it won't be a pure interface anymore). WDYT? > > > > A rough check of Chromium code suggests that of AddObserver() methods, there > > are: > > > > 101 pure virtual > > > https://cs.corp.google.com/search/?q=package:clankium+void%5C+AddObserver.*%3...; > > 193 override > > > https://cs.corp.google.com/search/?q=package:clankium+void%5C+AddObserver.*ov...; > > 302 neither > > > https://cs.corp.google.com/search/?q=package:clankium+void%5C+AddObserver.*%5C); > > > > Spot-checking a couple of samples from category 3 suggests that most are > > concrete classes, no pure virtuals, so I think that what you suggest is > > generally not The Chromium Way. > > Okay, fair enough. > My rationale was that the method has quite specific semantics, so every > implementation would basically need to duplicate this exact implementation. But > since there'll probably only ever be this one implementation, I guess that > doesn't matter. If I were to go further, I think that really The Chromium Way would be to have {Add,Remove}Observer() and delegate the implementation to an ObserverList, right? We can head that way if the interface needs to be used anywhere else. https://codereview.chromium.org/1954973004/diff/60001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.h (right): https://codereview.chromium.org/1954973004/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.h:63: virtual void SetObserver(Observer* new_observer) = 0; On 2016/05/09 09:55:13, Marc Treib wrote: > nit: I'd call this just "observer" - any reason for the "new_" prefix? I found the naming to help with reading the implementation of this method.
https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/1954973004/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:392: if (supervisor_->IsBlocked(visited.url)) { On 2016/05/09 10:26:17, sfiera wrote: > On 2016/05/09 09:55:12, Marc Treib wrote: > > On 2016/05/09 09:33:35, sfiera wrote: > > > On 2016/05/09 08:22:48, Marc Treib wrote: > > > > nit: no braces required > > > > > > Done, but the braces weren't mine. > > > > Your change made the condition fit on one line, so made the braces unnecessary > > :) > > (The rule is roughly: No braces if both the condition and the body fit on one > > line each. To be fair, it's not followed everywhere in Chromium, and the more > > general rule is "do what the code around you does".) > > I guess I should have been running git cl format anyway, but I've done so now > and will try to do so in the future because I don't want to remember rules like > that :) That's generally a good idea. It won't add or remove braces though.
So, running git cl format is not enough to avoid style nits? :/
Nope, not this particular style nit :( Most other things should be covered though. On Mon, May 9, 2016 at 12:40 PM <sfiera@chromium.org> wrote: > So, running git cl format is not enough to avoid style nits? :/ > > https://codereview.chromium.org/1954973004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by sfiera@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/1954973004/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1954973004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1954973004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by sfiera@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/1954973004/#ps140001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1954973004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1954973004/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Remove MostVisitedSites => SupervisedUserService dep. Replace SupervisedUserService in MostVisitedSites with an interface. Implement it in most_visited_sites_bridge.cc on top of SupervisedUserService. BUG=603026 ========== to ========== Remove MostVisitedSites => SupervisedUserService dep. Replace SupervisedUserService in MostVisitedSites with an interface. Implement it in most_visited_sites_bridge.cc on top of SupervisedUserService. BUG=603026 Committed: https://crrev.com/79e643bb8dec304b745f91e83a112d19c2a62eb7 Cr-Commit-Position: refs/heads/master@{#393553} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/79e643bb8dec304b745f91e83a112d19c2a62eb7 Cr-Commit-Position: refs/heads/master@{#393553} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
