Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1890753002:
SafeBrowsing: Track and cancel API checks. (Closed)
|
Created:
4 years, 8 months ago by kcarattini Modified:
4 years, 8 months ago Reviewers:
Nathan Parker CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, vakh (use Gerrit instead) Base URL:
https://chromium.googlesource.com/chromium/src.git@osb-impl-2 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSafeBrowsing: Track and cancel API checks.
BUG=561867
Committed: https://crrev.com/784ead663a31e2652f5b60e90eebe79d103a2b88
Cr-Commit-Position: refs/heads/master@{#388432}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Review Comments #Patch Set 3 : Use raw pointer in set #
Total comments: 10
Patch Set 4 : Review COmments #Patch Set 5 : Check callback #
Depends on Patchset: Messages
Total messages: 20 (6 generated)
kcarattini@chromium.org changed reviewers: + nparker@chromium.org
This follows similar logic to the current LocalDatabaseManager. I'm happy to discuss alternatives, though, because at the moment this requires only one in-progress check per client per url.
The CQ bit was checked by kcarattini@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1890753002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1890753002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/1890753002/diff/1/components/safe_browsing_db... File components/safe_browsing_db/database_manager.cc (right): https://codereview.chromium.org/1890753002/diff/1/components/safe_browsing_db... components/safe_browsing_db/database_manager.cc:45: for (CurrentApiChecks::iterator it = api_checks_.begin(); for (auto itr : api_checks_) ? https://codereview.chromium.org/1890753002/diff/1/components/safe_browsing_db... components/safe_browsing_db/database_manager.cc:65: return false; Add NOTREACHED() https://codereview.chromium.org/1890753002/diff/1/components/safe_browsing_db... components/safe_browsing_db/database_manager.cc:98: api_checks_.insert(check); I _think_ this could be a scoped_ptr if you just assume the api_checks_ set owns it. I think that's how the local and remote db managers do it. https://codereview.chromium.org/1890753002/diff/1/components/safe_browsing_db... File components/safe_browsing_db/database_manager.h (right): https://codereview.chromium.org/1890753002/diff/1/components/safe_browsing_db... components/safe_browsing_db/database_manager.h:184: // should wait for results before calling this method a second time. We probably want a dcheck to enforce that.
https://codereview.chromium.org/1890753002/diff/1/components/safe_browsing_db... File components/safe_browsing_db/database_manager.cc (right): https://codereview.chromium.org/1890753002/diff/1/components/safe_browsing_db... components/safe_browsing_db/database_manager.cc:45: for (CurrentApiChecks::iterator it = api_checks_.begin(); On 2016/04/15 23:27:35, Nathan Parker wrote: > for (auto itr : api_checks_) ? Done. https://codereview.chromium.org/1890753002/diff/1/components/safe_browsing_db... components/safe_browsing_db/database_manager.cc:65: return false; On 2016/04/15 23:27:35, Nathan Parker wrote: > Add NOTREACHED() Done. https://codereview.chromium.org/1890753002/diff/1/components/safe_browsing_db... components/safe_browsing_db/database_manager.cc:98: api_checks_.insert(check); On 2016/04/15 23:27:35, Nathan Parker wrote: > I _think_ this could be a scoped_ptr if you just assume the api_checks_ set owns > it. I think that's how the local and remote db managers do it. The local and remote managers both store the raw pointer in the set/vector and manage the memory manually. I think unique_ptr would work in the set, but then I'd have to pass around the raw pointer to the non-owners. Which of those do you prefer? https://codereview.chromium.org/1890753002/diff/1/components/safe_browsing_db... File components/safe_browsing_db/database_manager.h (right): https://codereview.chromium.org/1890753002/diff/1/components/safe_browsing_db... components/safe_browsing_db/database_manager.h:184: // should wait for results before calling this method a second time. On 2016/04/15 23:27:35, Nathan Parker wrote: > We probably want a dcheck to enforce that. Done.
https://codereview.chromium.org/1890753002/diff/1/components/safe_browsing_db... File components/safe_browsing_db/database_manager.cc (right): https://codereview.chromium.org/1890753002/diff/1/components/safe_browsing_db... components/safe_browsing_db/database_manager.cc:98: api_checks_.insert(check); On 2016/04/18 03:00:09, kcarattini wrote: > On 2016/04/15 23:27:35, Nathan Parker wrote: > > I _think_ this could be a scoped_ptr if you just assume the api_checks_ set > owns > > it. I think that's how the local and remote db managers do it. > > The local and remote managers both store the raw pointer in the set/vector and > manage the memory manually. I think unique_ptr would work in the set, but then > I'd have to pass around the raw pointer to the non-owners. Which of those do you > prefer? The raw ptr is probably fine since that continues an existing pattern. Just document that they're owned there.
On 2016/04/18 17:29:58, Nathan Parker wrote: > https://codereview.chromium.org/1890753002/diff/1/components/safe_browsing_db... > File components/safe_browsing_db/database_manager.cc (right): > > https://codereview.chromium.org/1890753002/diff/1/components/safe_browsing_db... > components/safe_browsing_db/database_manager.cc:98: api_checks_.insert(check); > On 2016/04/18 03:00:09, kcarattini wrote: > > On 2016/04/15 23:27:35, Nathan Parker wrote: > > > I _think_ this could be a scoped_ptr if you just assume the api_checks_ set > > owns > > > it. I think that's how the local and remote db managers do it. > > > > The local and remote managers both store the raw pointer in the set/vector and > > manage the memory manually. I think unique_ptr would work in the set, but then > > I'd have to pass around the raw pointer to the non-owners. Which of those do > you > > prefer? > > The raw ptr is probably fine since that continues an existing pattern. Just > document that they're owned there. Ok, switched back to using raw pointer. PTAL.
lgtm with some comments https://codereview.chromium.org/1890753002/diff/40001/components/safe_browsin... File components/safe_browsing_db/database_manager.cc (right): https://codereview.chromium.org/1890753002/diff/40001/components/safe_browsin... components/safe_browsing_db/database_manager.cc:57: for (CurrentApiChecks::iterator it = api_checks_.begin(); for (auto it : api_checks).. And if you want to get fancy, this whole function could be replaced with a call to std::find_if(..) and a lambda. I'm generally not a fancy guy though. https://codereview.chromium.org/1890753002/diff/40001/components/safe_browsin... File components/safe_browsing_db/database_manager.h (right): https://codereview.chromium.org/1890753002/diff/40001/components/safe_browsin... components/safe_browsing_db/database_manager.h:240: CurrentApiChecks::iterator FindClientCheck(Client* client); This should be FindApiClientCheck, ya? https://codereview.chromium.org/1890753002/diff/40001/components/safe_browsin... File components/safe_browsing_db/database_manager_unittest.cc (right): https://codereview.chromium.org/1890753002/diff/40001/components/safe_browsin... components/safe_browsing_db/database_manager_unittest.cc:52: base::TimeDelta::FromSeconds(delay_seconds_)); Does this use real time, or just ensure the ordering of tasks (real time delays in test are generally unreliable.) https://codereview.chromium.org/1890753002/diff/40001/components/safe_browsin... components/safe_browsing_db/database_manager_unittest.cc:235: EXPECT_EQ(0ul, permissions.size()); Does this verify that the callback was invoked?
https://codereview.chromium.org/1890753002/diff/40001/components/safe_browsin... File components/safe_browsing_db/database_manager.cc (right): https://codereview.chromium.org/1890753002/diff/40001/components/safe_browsin... components/safe_browsing_db/database_manager.cc:57: for (CurrentApiChecks::iterator it = api_checks_.begin(); On 2016/04/19 18:05:19, Nathan Parker wrote: > for (auto it : api_checks).. > > And if you want to get fancy, this whole function could be replaced with a call > to std::find_if(..) and a lambda. I'm generally not a fancy guy though. I'm ok with leaving as-is. https://codereview.chromium.org/1890753002/diff/40001/components/safe_browsin... File components/safe_browsing_db/database_manager.h (right): https://codereview.chromium.org/1890753002/diff/40001/components/safe_browsin... components/safe_browsing_db/database_manager.h:240: CurrentApiChecks::iterator FindClientCheck(Client* client); On 2016/04/19 18:05:19, Nathan Parker wrote: > This should be FindApiClientCheck, ya? Yeah. Changed to FindClientApiCheck. https://codereview.chromium.org/1890753002/diff/40001/components/safe_browsin... File components/safe_browsing_db/database_manager_unittest.cc (right): https://codereview.chromium.org/1890753002/diff/40001/components/safe_browsin... components/safe_browsing_db/database_manager_unittest.cc:52: base::TimeDelta::FromSeconds(delay_seconds_)); On 2016/04/19 18:05:19, Nathan Parker wrote: > Does this use real time, or just ensure the ordering of tasks (real time delays > in test are generally unreliable.) I'm not sure. It doesn't appear to use real time because I set a rather long delay and the test does not take that long to run. Would you suggest a different way of doing this? https://codereview.chromium.org/1890753002/diff/40001/components/safe_browsin... components/safe_browsing_db/database_manager_unittest.cc:235: EXPECT_EQ(0ul, permissions.size()); On 2016/04/19 18:05:19, Nathan Parker wrote: > Does this verify that the callback was invoked? No. How would I do that?
https://codereview.chromium.org/1890753002/diff/40001/components/safe_browsin... File components/safe_browsing_db/database_manager_unittest.cc (right): https://codereview.chromium.org/1890753002/diff/40001/components/safe_browsin... components/safe_browsing_db/database_manager_unittest.cc:235: EXPECT_EQ(0ul, permissions.size()); On 2016/04/20 01:55:53, kcarattini wrote: > On 2016/04/19 18:05:19, Nathan Parker wrote: > > Does this verify that the callback was invoked? > > No. How would I do that? You could do something like set a "was_callback_invoked_" bool in OnCheckApiBlacklistUrlResult() of the client.
https://codereview.chromium.org/1890753002/diff/40001/components/safe_browsin... File components/safe_browsing_db/database_manager_unittest.cc (right): https://codereview.chromium.org/1890753002/diff/40001/components/safe_browsin... components/safe_browsing_db/database_manager_unittest.cc:235: EXPECT_EQ(0ul, permissions.size()); On 2016/04/20 03:03:35, Nathan Parker wrote: > On 2016/04/20 01:55:53, kcarattini wrote: > > On 2016/04/19 18:05:19, Nathan Parker wrote: > > > Does this verify that the callback was invoked? > > > > No. How would I do that? > > You could do something like set a "was_callback_invoked_" bool in > OnCheckApiBlacklistUrlResult() of the client. Oh, that callback. Done.
The CQ bit was checked by kcarattini@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org Link to the patchset: https://codereview.chromium.org/1890753002/#ps80001 (title: "Check callback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1890753002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1890753002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== SafeBrowsing: Track and cancel API checks. BUG=561867 ========== to ========== SafeBrowsing: Track and cancel API checks. BUG=561867 Committed: https://crrev.com/784ead663a31e2652f5b60e90eebe79d103a2b88 Cr-Commit-Position: refs/heads/master@{#388432} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/784ead663a31e2652f5b60e90eebe79d103a2b88 Cr-Commit-Position: refs/heads/master@{#388432} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
