Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2605213002:
componentize SafeBrowsingService (Closed)
|
Created:
3 years, 11 months ago by Nate Fischer Modified:
3 years, 11 months ago CC:
blundell+watchlist_chromium.org, chromium-reviews, droger+watchlist_chromium.org, grt+watch_chromium.org, sdefresne+watchlist_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncomponentize SafeBrowsingService
Create BaseSafeBrowsingService in the components layer and derive
SafeBrowsingService from that.
BUG=488675
Committed: https://crrev.com/e5f4a58871b512092e37080597014271bbb8fbc6
Cr-Commit-Position: refs/heads/master@{#441305}
Patch Set 1 #Patch Set 2 : Fix compile error #Patch Set 3 : Marking method as overriding #
Total comments: 13
Patch Set 4 : Removing some methods from base class and addressing comments #
Total comments: 9
Patch Set 5 : Change date to 2017, add explanatory comment about Local manager #Patch Set 6 : Remove explicit return statements #
Messages
Total messages: 36 (23 generated)
The CQ bit was checked by ntfschr@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...
ntfschr@chromium.org changed reviewers: + jialiul@chromium.org
Jialiu, Here's an initial version of the SafeBrowsingService refactor. Most of the methods are no-ops, since a lot of the methods you marked depend on ServicesDelegate, which is in the chrome layer. The UIManager stuff might be easier to do in another CL so that I can change BaseSafeBrowsingUIManager to reference BaseSafeBrowsingService and vice-versa. Or I can try to work that into this CL, let me know. I'll finish up the work when I get back on Tuesday.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ntfschr@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: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ntfschr@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: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Looking good. See my initial comments. After all the try bots are happy, I'll take another look. https://codereview.chromium.org/2605213002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_service.h (left): https://codereview.chromium.org/2605213002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_service.h:282: bool enabled_v4_only_; It probably makes more sense to keep enabled_v4_only_ in the subclass. Since WebView doesn't need to know whether it is V3 or V4. WDYT? https://codereview.chromium.org/2605213002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_service.h (right): https://codereview.chromium.org/2605213002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_service.h:183: net::URLRequestContextGetter* url_request_context_getter) override; Any particular reason for making StartOnIOThread(...), StopOnIOThread(...) Start(), Stop(), Observe(...), AddPrefService(), RemovePrefService, and RefreshState() public? https://codereview.chromium.org/2605213002/diff/40001/components/safe_browsin... File components/safe_browsing/base_safe_browsing_service.cc (right): https://codereview.chromium.org/2605213002/diff/40001/components/safe_browsin... components/safe_browsing/base_safe_browsing_service.cc:43: // TODO(ntfschr): initialize ui_manager_ once CreateUIManager is componentized CreateUIManager can simply return a BaseUIManager for now. https://codereview.chromium.org/2605213002/diff/40001/components/safe_browsin... components/safe_browsing/base_safe_browsing_service.cc:78: std::string BaseSafeBrowsingService::GetProtocolConfigClientName() const { Since this function is not called in the base class, let's move it to the subclass. It will also help reduce some dependency. https://codereview.chromium.org/2605213002/diff/40001/components/safe_browsin... File components/safe_browsing/base_safe_browsing_service.h (right): https://codereview.chromium.org/2605213002/diff/40001/components/safe_browsin... components/safe_browsing/base_safe_browsing_service.h:31: // The SafeBrowsingService owns both the UI and Database managers which do Please update this comment. https://codereview.chromium.org/2605213002/diff/40001/components/safe_browsin... components/safe_browsing/base_safe_browsing_service.h:94: bool enabled_; nit: member functions should be in front of member variables.
https://codereview.chromium.org/2605213002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_service.h (left): https://codereview.chromium.org/2605213002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_service.h:282: bool enabled_v4_only_; On 2016/12/29 at 17:41:37, Jialiu Lin wrote: > It probably makes more sense to keep enabled_v4_only_ in the subclass. Since WebView doesn't need to know whether it is V3 or V4. WDYT? Sounds good to me. I also moved enabled_by_prefs_, since it was also only used in the subclass. Let me know if I should move it back to components layer. https://codereview.chromium.org/2605213002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_service.h (right): https://codereview.chromium.org/2605213002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_service.h:183: net::URLRequestContextGetter* url_request_context_getter) override; On 2016/12/29 at 17:41:37, Jialiu Lin wrote: > Any particular reason for making StartOnIOThread(...), StopOnIOThread(...) Start(), Stop(), Observe(...), AddPrefService(), RemovePrefService, and RefreshState() public? I originally moved them to protected so that the subclass can inherit them. Since the base class has only dummy implementations, I just moved them back to where they were before. If the base class really does need these methods, I'll add them back to the base as either private or protected. https://codereview.chromium.org/2605213002/diff/40001/components/safe_browsin... File components/safe_browsing/base_safe_browsing_service.cc (right): https://codereview.chromium.org/2605213002/diff/40001/components/safe_browsin... components/safe_browsing/base_safe_browsing_service.cc:43: // TODO(ntfschr): initialize ui_manager_ once CreateUIManager is componentized On 2016/12/29 at 17:41:37, Jialiu Lin wrote: > CreateUIManager can simply return a BaseUIManager for now. This is tricky, since other files (e.g. permission_uma_util.cc) depend on SafeBrowsingService to have a UIManager, not the base class, as a member. Should I wrap all calls to ui_manager() in dynamic casts, or should I add the necessary methods (e.g. ReportPermissionAction()) to BaseUIManager? https://codereview.chromium.org/2605213002/diff/40001/components/safe_browsin... components/safe_browsing/base_safe_browsing_service.cc:78: std::string BaseSafeBrowsingService::GetProtocolConfigClientName() const { On 2016/12/29 at 17:41:37, Jialiu Lin wrote: > Since this function is not called in the base class, let's move it to the subclass. It will also help reduce some dependency. Done https://codereview.chromium.org/2605213002/diff/40001/components/safe_browsin... File components/safe_browsing/base_safe_browsing_service.h (right): https://codereview.chromium.org/2605213002/diff/40001/components/safe_browsin... components/safe_browsing/base_safe_browsing_service.h:31: // The SafeBrowsingService owns both the UI and Database managers which do On 2016/12/29 at 17:41:37, Jialiu Lin wrote: > Please update this comment. Done https://codereview.chromium.org/2605213002/diff/40001/components/safe_browsin... components/safe_browsing/base_safe_browsing_service.h:94: bool enabled_; On 2016/12/29 at 17:41:37, Jialiu Lin wrote: > nit: member functions should be in front of member variables. Done
The CQ bit was checked by ntfschr@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.
ntfschr@chromium.org changed reviewers: + mmenke@chromium.org
+mmenke@ for adding +net/url_request to DEPS file
On 2017/01/04 01:28:49, Nate Fischer wrote: > +mmenke@ for adding +net/url_request to DEPS file DEPS LGTM.
some nits. https://codereview.chromium.org/2605213002/diff/40001/components/safe_browsin... File components/safe_browsing/base_safe_browsing_service.cc (right): https://codereview.chromium.org/2605213002/diff/40001/components/safe_browsin... components/safe_browsing/base_safe_browsing_service.cc:43: // TODO(ntfschr): initialize ui_manager_ once CreateUIManager is componentized On 2017/01/03 22:39:53, Nate Fischer wrote: > On 2016/12/29 at 17:41:37, Jialiu Lin wrote: > > CreateUIManager can simply return a BaseUIManager for now. > > This is tricky, since other files (e.g. permission_uma_util.cc) depend on > SafeBrowsingService to have a UIManager, not the base class, as a member. Should > I wrap all calls to ui_manager() in dynamic casts, or should I add the necessary > methods (e.g. ReportPermissionAction()) to BaseUIManager? Ah, I see. Thanks for your explanation. https://codereview.chromium.org/2605213002/diff/60001/components/safe_browsin... File components/safe_browsing/base_safe_browsing_service.cc (right): https://codereview.chromium.org/2605213002/diff/60001/components/safe_browsin... components/safe_browsing/base_safe_browsing_service.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2605213002/diff/60001/components/safe_browsin... components/safe_browsing/base_safe_browsing_service.cc:62: #if defined(SAFE_BROWSING_DB_REMOTE) Maybe add a comment here to tell others if they need LocalSafeBrowsingDatabaseManager, they should use c/b/SafeBrowsingService instead. https://codereview.chromium.org/2605213002/diff/60001/components/safe_browsin... components/safe_browsing/base_safe_browsing_service.cc:79: return; don't need this return. https://codereview.chromium.org/2605213002/diff/60001/components/safe_browsin... File components/safe_browsing/base_safe_browsing_service.h (right): https://codereview.chromium.org/2605213002/diff/60001/components/safe_browsin... components/safe_browsing/base_safe_browsing_service.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. It probably should say "2017" here. Happy new year!
https://codereview.chromium.org/2605213002/diff/60001/components/safe_browsin... File components/safe_browsing/base_safe_browsing_service.cc (right): https://codereview.chromium.org/2605213002/diff/60001/components/safe_browsin... components/safe_browsing/base_safe_browsing_service.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2017/01/04 at 01:36:34, Jialiu Lin wrote: > 2017 done https://codereview.chromium.org/2605213002/diff/60001/components/safe_browsin... components/safe_browsing/base_safe_browsing_service.cc:62: #if defined(SAFE_BROWSING_DB_REMOTE) On 2017/01/04 at 01:36:34, Jialiu Lin wrote: > Maybe add a comment here to tell others if they need LocalSafeBrowsingDatabaseManager, they should use c/b/SafeBrowsingService instead. Done https://codereview.chromium.org/2605213002/diff/60001/components/safe_browsin... components/safe_browsing/base_safe_browsing_service.cc:79: return; On 2017/01/04 at 01:36:34, Jialiu Lin wrote: > don't need this return. Done. Should I also remove the explicit returns in Initialize() & Shutdown()? https://codereview.chromium.org/2605213002/diff/60001/components/safe_browsin... File components/safe_browsing/base_safe_browsing_service.h (right): https://codereview.chromium.org/2605213002/diff/60001/components/safe_browsin... components/safe_browsing/base_safe_browsing_service.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2017/01/04 at 01:36:34, Jialiu Lin wrote: > It probably should say "2017" here. > Happy new year! Done. Happy new year!
https://codereview.chromium.org/2605213002/diff/60001/components/safe_browsin... File components/safe_browsing/base_safe_browsing_service.cc (right): https://codereview.chromium.org/2605213002/diff/60001/components/safe_browsin... components/safe_browsing/base_safe_browsing_service.cc:79: return; On 2017/01/04 01:45:59, Nate Fischer wrote: > On 2017/01/04 at 01:36:34, Jialiu Lin wrote: > > don't need this return. > > Done. Should I also remove the explicit returns in Initialize() & Shutdown()? Yes, sounds good.
lgtm
The CQ bit was checked by ntfschr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org, jialiul@chromium.org Link to the patchset: https://codereview.chromium.org/2605213002/#ps100001 (title: "Remove explicit return statements")
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": 100001, "attempt_start_ts": 1483495054614330,
"parent_rev": "7c6897f7d90084df31431811bee55eeb0070ef4f", "commit_rev":
"5cbc45f3e8104fa277222d03c5531e3e78cfbe2e"}
Message was sent while issue was closed.
Description was changed from ========== componentize SafeBrowsingService Create BaseSafeBrowsingService in the components layer and derive SafeBrowsingService from that. BUG=488675 ========== to ========== componentize SafeBrowsingService Create BaseSafeBrowsingService in the components layer and derive SafeBrowsingService from that. BUG=488675 Review-Url: https://codereview.chromium.org/2605213002 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== componentize SafeBrowsingService Create BaseSafeBrowsingService in the components layer and derive SafeBrowsingService from that. BUG=488675 Review-Url: https://codereview.chromium.org/2605213002 ========== to ========== componentize SafeBrowsingService Create BaseSafeBrowsingService in the components layer and derive SafeBrowsingService from that. BUG=488675 Committed: https://crrev.com/e5f4a58871b512092e37080597014271bbb8fbc6 Cr-Commit-Position: refs/heads/master@{#441305} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/e5f4a58871b512092e37080597014271bbb8fbc6 Cr-Commit-Position: refs/heads/master@{#441305}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/2615713008/ by ntfschr@chromium.org. The reason for reverting is: We've decided to not componentize SafeBrowsingService and instead manually create the DataBaseManager and UIManager. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
