Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2151953003:
PVer4: Drop hash prefixes based on the removals field in the response (Closed)
|
Created:
4 years, 5 months ago by vakh (use Gerrit instead) Modified:
4 years, 5 months ago CC:
chromium-reviews, Scott Hess - ex-Googler, woz Base URL:
https://chromium.googlesource.com/chromium/src.git@02_iterators Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPVer4: Drop hash prefixes based on the removals field in the response
received from the PVer4 service.
BUG=543161
Committed: https://crrev.com/fe520540d21f308e949d986f74aee5e96d06c296
Cr-Commit-Position: refs/heads/master@{#406114}
Patch Set 1 #Patch Set 2 : Use const& #
Total comments: 10
Patch Set 3 : git pull #Patch Set 4 : nparker@ CR feedback #
Total comments: 6
Patch Set 5 : nparker@ CR feedback #
Total comments: 4
Patch Set 6 : Don't iterate over removals. Update removals comment. #
Messages
Total messages: 37 (20 generated)
The CQ bit was checked by vakh@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...
vakh@chromium.org changed reviewers: + noelutz@chromium.org, nparker@chromium.org
nparker, noelutz: Please review the changes in *
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Use const&
The CQ bit was checked by vakh@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_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2151953003/diff/20001/components/safe_browsin... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2151953003/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:59: using ::google::protobuf::int32; seems like this might conflict with an existing int32... not sure though. https://codereview.chromium.org/2151953003/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:117: int removals_size = response->removals_size(); no need for the temp removals_size if you're not reusing it. https://codereview.chromium.org/2151953003/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:120: for (const auto& removal : response->removals()) { You could simplify this since you expect 0 or 1 removals (i.e. you don't need a loop). DCHECK_LE(response->removals_size(), 1); const RepeatedField<int32> empty_removals; // this could be static, up top. const RepeatedField<int32>* raw_removals = &empty_removals; if (!response->removals_empty()) { // (dcheck that it's raw) raw_removals = &removals[0].raw_indices().raw_removals() } https://codereview.chromium.org/2151953003/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:125: raw_removals = raw_indices.indices(); This does a copy https://codereview.chromium.org/2151953003/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:255: const int* removals_iter = raw_removals.begin(); Put this down by total_picked_from_old, since you don't need it before then.
git pull
The CQ bit was checked by vakh@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...
nparker@ CR feedback
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Thanks for the feedback. PTAL. https://codereview.chromium.org/2151953003/diff/20001/components/safe_browsin... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2151953003/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:59: using ::google::protobuf::int32; On 2016/07/15 16:49:38, Nathan Parker wrote: > seems like this might conflict with an existing int32... not sure though. Can't find any definitions: https://cs.chromium.org/search/?q=%5Cbint32%5Cb+f:cc$+case:yes&sq=package:chr... https://codereview.chromium.org/2151953003/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:117: int removals_size = response->removals_size(); On 2016/07/15 16:49:38, Nathan Parker wrote: > no need for the temp removals_size if you're not reusing it. Done. https://codereview.chromium.org/2151953003/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:120: for (const auto& removal : response->removals()) { On 2016/07/15 16:49:38, Nathan Parker wrote: > You could simplify this since you expect 0 or 1 removals (i.e. you don't need a > loop). > > DCHECK_LE(response->removals_size(), 1); > const RepeatedField<int32> empty_removals; // this could be static, up top. > const RepeatedField<int32>* raw_removals = &empty_removals; > if (!response->removals_empty()) { > // (dcheck that it's raw) > raw_removals = &removals[0].raw_indices().raw_removals() > } Done. https://codereview.chromium.org/2151953003/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:125: raw_removals = raw_indices.indices(); On 2016/07/15 16:49:38, Nathan Parker wrote: > This does a copy Done. https://codereview.chromium.org/2151953003/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:255: const int* removals_iter = raw_removals.begin(); On 2016/07/15 16:49:38, Nathan Parker wrote: > Put this down by total_picked_from_old, since you don't need it before then. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2151953003/diff/60001/components/safe_browsin... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2151953003/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:118: const RepeatedField<int32>* raw_removals = &empty_removals_; Does MergeUpdate() need this ptr beyond the lifetime of the function call? If not, then you could just declare empty_removeals on the stack in this function. https://codereview.chromium.org/2151953003/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:279: const int* removals_iter = raw_removals ? raw_removals->begin() : nullptr; If you're going to check raw_removals for null, then you could call MergeUpdate() with a nullptr rather than empty_removals. https://codereview.chromium.org/2151953003/diff/60001/components/safe_browsin... File components/safe_browsing_db/v4_store.h (right): https://codereview.chromium.org/2151953003/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store.h:269: const ::google::protobuf::RepeatedField<::google::protobuf::int32> Could this be declared in an empty namespace in the .cc, so it doesn't clutter the .h file?
nparker@ CR feedback
The CQ bit was checked by vakh@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...
https://codereview.chromium.org/2151953003/diff/60001/components/safe_browsin... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2151953003/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:118: const RepeatedField<int32>* raw_removals = &empty_removals_; On 2016/07/18 19:54:35, Nathan Parker wrote: > Does MergeUpdate() need this ptr beyond the lifetime of the function call? If > not, then you could just declare empty_removeals on the stack in this function. Done. https://codereview.chromium.org/2151953003/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:279: const int* removals_iter = raw_removals ? raw_removals->begin() : nullptr; On 2016/07/18 19:54:35, Nathan Parker wrote: > If you're going to check raw_removals for null, then you could call > MergeUpdate() with a nullptr rather than empty_removals. Done. https://codereview.chromium.org/2151953003/diff/60001/components/safe_browsin... File components/safe_browsing_db/v4_store.h (right): https://codereview.chromium.org/2151953003/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store.h:269: const ::google::protobuf::RepeatedField<::google::protobuf::int32> On 2016/07/18 19:54:35, Nathan Parker wrote: > Could this be declared in an empty namespace in the .cc, so it doesn't clutter > the .h file? Done.
https://codereview.chromium.org/2151953003/diff/80001/components/safe_browsin... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2151953003/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:119: for (const auto& removal : response->removals()) { This still iterates over the loop, though you expect at most one. You can just take the first one then. https://codereview.chromium.org/2151953003/diff/80001/components/safe_browsin... File components/safe_browsing_db/v4_store.h (right): https://codereview.chromium.org/2151953003/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_store.h:255: // TODO(vakh): Process removals also. remove todo, and mention that raw_removals can be null.
Don't iterate over removals. Update removals comment.
The CQ bit was checked by vakh@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...
https://codereview.chromium.org/2151953003/diff/80001/components/safe_browsin... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2151953003/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:119: for (const auto& removal : response->removals()) { On 2016/07/18 20:42:39, Nathan Parker wrote: > This still iterates over the loop, though you expect at most one. You can just > take the first one then. Done. https://codereview.chromium.org/2151953003/diff/80001/components/safe_browsin... File components/safe_browsing_db/v4_store.h (right): https://codereview.chromium.org/2151953003/diff/80001/components/safe_browsin... components/safe_browsing_db/v4_store.h:255: // TODO(vakh): Process removals also. On 2016/07/18 20:42:39, Nathan Parker wrote: > remove todo, and mention that raw_removals can be null. Done.
lgtm
The CQ bit was unchecked by vakh@chromium.org
The CQ bit was checked by vakh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== PVer4: Drop hash prefixes based on the removals field in the response received from the PVer4 service. BUG=543161 ========== to ========== PVer4: Drop hash prefixes based on the removals field in the response received from the PVer4 service. BUG=543161 Committed: https://crrev.com/fe520540d21f308e949d986f74aee5e96d06c296 Cr-Commit-Position: refs/heads/master@{#406114} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/fe520540d21f308e949d986f74aee5e96d06c296 Cr-Commit-Position: refs/heads/master@{#406114} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
