Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2233103002:
Move full hash caching logic to v4_get_hash_protocol_manager (Closed)
|
Created:
4 years, 4 months ago by vakh (use Gerrit instead) Modified:
4 years, 3 months ago CC:
chromium-reviews, noé, woz, francois_mozilla.com Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove the logic to cache the full hash responses from the server into v4_get_hash_protocol_manager.
Design doc: https://goto.google.com/design-doc-v4-full-hash-manager
Overall, this CL does the following:
1. Moves caching to V4GetHashProtocolManager.
2. Refactors V4GetHashProtocolManager to use the data structures used in rest of
the V4 code.
3. Re-defines how cache is implemented.
TODOs:
1. Done: Merge the response from the server and the response from the cache.
2. Figure out what UMA metrics to add (I have removed some that are no longer
valid after the refactor).
BUG=616647, 543161
Committed: https://crrev.com/1d2b239717bd198bd42f11e468e1eaca6b94cdc1
Cr-Commit-Position: refs/heads/master@{#418303}
Patch Set 1 #
Total comments: 7
Patch Set 2 : hash_map<HashPrefix, hash_map<FullHash, FullHashExtraInfo>> #Patch Set 3 : prefix->{pttl, [full_hash, store, metadata, pttl]} #Patch Set 4 : fetch and sync and rebase #Patch Set 5 : Implement merging cache results and results from the server #Patch Set 6 : Remove unused code and tests #Patch Set 7 : kcarattini: Remove RecordV4GetHashCheckResult function since it is not being used anymore #Patch Set 8 : unique_ptr for V4GetHasProtocolManager. Fix the lone failing unit test. #
Total comments: 2
Patch Set 9 : Add a comment. Remove an unused method and an unused typedef. #Patch Set 10 : Move another test from database_manager to get_hash_manager #
Total comments: 8
Patch Set 11 : Remove more tests from db_manager. Simplify another test. #Patch Set 12 : Remove more tests from db_manager. Simplify another test. #
Total comments: 71
Patch Set 13 : Remove TestV4GetHashProtocolManager from db_mgr_unittest. Add another unittest. #Patch Set 14 : Incoorporate nparker@'s feedback #Patch Set 15 : Bring back the histogram to check if there were any hits in the response from the server #
Total comments: 23
Patch Set 16 : shess@ feedback. Remove a comment about UMA. #Patch Set 17 : shess@ feedback - part 2. #Messages
Total messages: 129 (82 generated)
Description was changed from ========== Some tests passing. Some commented BUG=616647 ========== to ========== Move the logic to cache the full hash responses from the server into v4_get_hash_protocol_manager. BUG=616647 ==========
nparker@chromium.org changed reviewers: + nparker@chromium.org
I'll let kcarattini review the caching logic parts. https://codereview.chromium.org/2233103002/diff/1/components/safe_browsing_db... File components/safe_browsing_db/database_manager.cc (right): https://codereview.chromium.org/2233103002/diff/1/components/safe_browsing_db... components/safe_browsing_db/database_manager.cc:133: v4_get_hash_protocol_manager_->GetFullHashCachedResults(SB_THREAT_TYPE_API_ABUSE, How about moving all of this code into v4_get_hash_protocol_manager? That is, so the caller doesn't even know there is a cache. Ideally it'd be just an optimization, and shouldn't affect the PM API. https://codereview.chromium.org/2233103002/diff/1/components/safe_browsing_db... File components/safe_browsing_db/database_manager_unittest.cc (right): https://codereview.chromium.org/2233103002/diff/1/components/safe_browsing_db... components/safe_browsing_db/database_manager_unittest.cc:261: /* What's this for? This should be moved or removed. https://codereview.chromium.org/2233103002/diff/1/components/safe_browsing_db... File components/safe_browsing_db/v4_get_hash_protocol_manager.cc (right): https://codereview.chromium.org/2233103002/diff/1/components/safe_browsing_db... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:392: const FullHashCallback& callback = it->second.second; nit: I might vote for a simple struct rather than a pair, since second.second is pretty opaque. https://codereview.chromium.org/2233103002/diff/1/components/safe_browsing_db... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:424: v4_full_hash_cache_[SB_THREAT_TYPE_API_ABUSE][prefix] = The threat type shouldn't be hardcoded here... it probably should be pulled from the request. https://codereview.chromium.org/2233103002/diff/1/components/safe_browsing_db... File components/safe_browsing_db/v4_get_hash_protocol_manager.h (right): https://codereview.chromium.org/2233103002/diff/1/components/safe_browsing_db... components/safe_browsing_db/v4_get_hash_protocol_manager.h:156: ThreatTypeToResultsMap* v4_full_hash_cache(){ Does this accessor add value? You could just make the member protected. https://codereview.chromium.org/2233103002/diff/1/components/safe_browsing_db... components/safe_browsing_db/v4_get_hash_protocol_manager.h:162: typedef base::hash_map<const net::URLFetcher*, std::pair<std::vector<SBPrefix>, FullHashCallback>> PendingHashRequests; >80 char line. https://codereview.chromium.org/2233103002/diff/1/components/safe_browsing_db... components/safe_browsing_db/v4_get_hash_protocol_manager.h:199: // A cache of V4 full hash results for api checks. Is this just for API requests?
vakh@chromium.org changed reviewers: - nparker@chromium.org
Sorry, this CL wasn't meant to be reviewed. I worked on this but ended up spending too much time on trying to move the tests also and in the end decided to let kcarattini@ get to it. This CL was meant to be a possible starting point by her.
Ah ok. I tend to review anything with my name attached as reviewer. On Mon, Aug 15, 2016 at 5:52 PM <vakh@chromium.org> wrote: > Sorry, this CL wasn't meant to be reviewed. I worked on this but ended up > spending too much time on trying to move the tests also and in the end > decided > to let kcarattini@ get to it. > This CL was meant to be a possible starting point by her. > > https://codereview.chromium.org/2233103002/ > -- 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.
Product code working :) -- Tests not so much :(
Added comments. Ordered functions alphabetically.
Got more tests running.
I'm bringing this CL back from the dead. This is a major refactor that: 1. Moves caching to V4GetHashProtocolManager. 2. Refactors V4GetHashProtocolManager to use the data structures used in rest of the V4 code. 3. Re-defines how caching is implemented. TODOs: 0. Design doc. Coming soon(ish). 1. Merge the response from the server and the response from the cache. 2. Figure out what UMA metrics to add (I have removed some that are no longer valid after the refactor).
vakh@chromium.org changed reviewers: + kcarattini@chromium.org, nparker@chromium.org
Description was changed from ========== Move the logic to cache the full hash responses from the server into v4_get_hash_protocol_manager. BUG=616647 ========== to ========== Move the logic to cache the full hash responses from the server into v4_get_hash_protocol_manager. BUG=616647, 543161 ==========
Description was changed from ========== Move the logic to cache the full hash responses from the server into v4_get_hash_protocol_manager. BUG=616647, 543161 ========== to ========== Move the logic to cache the full hash responses from the server into v4_get_hash_protocol_manager. Overall, this CL does the following: 1. Moves caching to V4GetHashProtocolManager. 2. Refactors V4GetHashProtocolManager to use the data structures used in rest of the V4 code. 3. Re-defines how cache is implemented. TODOs: 0. Design doc. Coming soon(ish). 1. Merge the response from the server and the response from the cache. 2. Figure out what UMA metrics to add (I have removed some that are no longer valid after the refactor). BUG=616647, 543161 ==========
hash_map<HashPrefix, hash_map<FullHash, FullHashExtraInfo>>
Per discussion offline: Hold off on reviewing this till the doc and code are adjusted.
prefix->{pttl, [full_hash, store, metadata, pttl]}
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
fetch and sync and rebase
Implement merging cache results and results from the server
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Minor BUILD.gn fixes
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...
Description was changed from ========== Move the logic to cache the full hash responses from the server into v4_get_hash_protocol_manager. Overall, this CL does the following: 1. Moves caching to V4GetHashProtocolManager. 2. Refactors V4GetHashProtocolManager to use the data structures used in rest of the V4 code. 3. Re-defines how cache is implemented. TODOs: 0. Design doc. Coming soon(ish). 1. Merge the response from the server and the response from the cache. 2. Figure out what UMA metrics to add (I have removed some that are no longer valid after the refactor). BUG=616647, 543161 ========== to ========== Move the logic to cache the full hash responses from the server into v4_get_hash_protocol_manager. Design doc: https://goto.google.com/design-doc-v4-full-hash-manager Overall, this CL does the following: 1. Moves caching to V4GetHashProtocolManager. 2. Refactors V4GetHashProtocolManager to use the data structures used in rest of the V4 code. 3. Re-defines how cache is implemented. TODOs: 1. Merge the response from the server and the response from the cache. 2. Figure out what UMA metrics to add (I have removed some that are no longer valid after the refactor). BUG=616647, 543161 ==========
Description was changed from ========== Move the logic to cache the full hash responses from the server into v4_get_hash_protocol_manager. Design doc: https://goto.google.com/design-doc-v4-full-hash-manager Overall, this CL does the following: 1. Moves caching to V4GetHashProtocolManager. 2. Refactors V4GetHashProtocolManager to use the data structures used in rest of the V4 code. 3. Re-defines how cache is implemented. TODOs: 1. Merge the response from the server and the response from the cache. 2. Figure out what UMA metrics to add (I have removed some that are no longer valid after the refactor). BUG=616647, 543161 ========== to ========== Move the logic to cache the full hash responses from the server into v4_get_hash_protocol_manager. Design doc: https://goto.google.com/design-doc-v4-full-hash-manager Overall, this CL does the following: 1. Moves caching to V4GetHashProtocolManager. 2. Refactors V4GetHashProtocolManager to use the data structures used in rest of the V4 code. 3. Re-defines how cache is implemented. TODOs: 1. Done: Merge the response from the server and the response from the cache. 2. Figure out what UMA metrics to add (I have removed some that are no longer valid after the refactor). BUG=616647, 543161 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Remove unused code and tests
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...
Minor BUILD.gn fixes
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Minor BUILD.gn fixes
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...
Patchset #11 (id:200001) has been deleted
Patchset #9 (id:160001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #2 (id:40001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
kcarattini: Remove RecordV4GetHashCheckResult function since it is not being used anymore
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: + shess@chromium.org
I've re-written most of the code from patches 1 and 2 so please review with a fresh mind. Please find a link to the design doc in the description section of the CL. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
unique_ptr for V4GetHasProtocolManager. Fix the lone failing unit test.
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: This issue passed the CQ dry run.
Add a comment. Remove an unused method and an unused typedef.
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: This issue passed the CQ dry run.
Move another test from database_manager to get_hash_manager
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: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Remove more tests from db_manager. Simplify another test.
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...
Remove more tests from db_manager. Simplify another test.
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: This issue passed the CQ dry run.
Remove TestV4GetHashProtocolManager from db_mgr_unittest. Add another unittest.
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...
Lots of code! Overall LGTM, with lots of mostly-little nits. My comments came in over a number of patches, so they might refer to old code. https://codereview.chromium.org/2233103002/diff/260001/components/safe_browsi... File components/safe_browsing_db/BUILD.gn (right): https://codereview.chromium.org/2233103002/diff/260001/components/safe_browsi... components/safe_browsing_db/BUILD.gn:333: # "database_manager_unittest.cc", is this staying? https://codereview.chromium.org/2233103002/diff/300001/components/safe_browsi... File components/safe_browsing_db/database_manager.cc (right): https://codereview.chromium.org/2233103002/diff/300001/components/safe_browsi... components/safe_browsing_db/database_manager.cc:28: base::hash_set<UpdateListIdentifier> stores_to_look({GetChromeUrlApiId()}); Maybe add a TODO to pull in the list of all stores. Once you do that, it'll start exercising more of the caching logic (multiple stores fetched, only one is actively used). https://codereview.chromium.org/2233103002/diff/300001/components/safe_browsi... File components/safe_browsing_db/database_manager.h (right): https://codereview.chromium.org/2233103002/diff/300001/components/safe_browsi... components/safe_browsing_db/database_manager.h:185: SafeBrowsingDatabaseManager::Client* client() const { return client_; } nit: can remove SafeBrowsingDatabaseManager:: https://codereview.chromium.org/2233103002/diff/300001/components/safe_browsi... components/safe_browsing_db/database_manager.h:188: void set_start_time(base::TimeTicks start) {start_time_ = start;} nit: I think this should be camel case: SetStartTime(..) https://codereview.chromium.org/2233103002/diff/300001/components/safe_browsi... components/safe_browsing_db/database_manager.h:231: // Called on the IO thread wheh the SafeBrowsingProtocolManager has received nit: s/wheh/when (existing typo) https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... File components/safe_browsing_db/v4_database.cc (right): https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_database.cc:175: DCHECK(matched_store_and_hash_prefixes); nit: You don't need the DCHECK if you're accessing the pointer right away, since it'll crash immediately anyway. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_database.cc:183: matched_store_and_hash_prefixes->push_back( nit: Does this work: ->emplace_back(identifier, hash_prefix) That's simpler and may avoid a temporary. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... File components/safe_browsing_db/v4_get_hash_protocol_manager.cc (right): https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:112: return base::WrapUnique(new V4GetHashProtocolManager( The new recommended way, from ptr_util.h: // MakeUnique<T>(args) should be preferred over WrapUnique(new T(args)): bare // calls to `new` should be treated with scrutiny. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:208: } nit: Could just DCHECK(!stores_to_request.empty()) up top, with the same effect. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:243: // (i.e. treat the page as safe). nit: "i.e. just use the cache and potentially treat the page as safe" https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:286: full_hash_to_store_and_hash_prefixes[full_hash].push_back( .emplace_back(GetChromeUrlApiId(), prefix), if that works. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:302: DCHECK(prefixes_to_request); nit: can skip checking the ptr if you're going to call a method on it right away. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:338: for (const auto& it : full_hash_to_store_and_hash_prefixes) { I'm assuming this code is not modified, ya? If that's not true, I'll look at it in detail. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:523: if (match.has_threat_entry_metadata()) { Can you reduce the nested if's by negating them and retuning immediately if there's an unexpected value? https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:604: // If negative_cache_expire is null, don't cache the results it's not clear nit: s/it's/since it's/ https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:612: full_hash_cache_[prefix].full_hash_infos.clear(); This looks up prefix multiple times. You could use find() once to get an iterator to modify the contents. If it doesn't exist, you'd need to insert an empty entry first. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:635: full_hash_to_store_and_hash_prefixes.at(fhi.full_hash)) { rather than looking up full_hash again, save the iterator returned by .find() above https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:690: const FullHashCallback& callback = fhci->callback; just call fhci->callback.Run(..) https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... File components/safe_browsing_db/v4_get_hash_protocol_manager.h (right): https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.h:62: explicit FullHashInfo(const FullHash& full_hash, nit: I don't think you need explicit if the ctor has > 1 arg (since it couldn't accidentally cast from 3 args) https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.h:105: explicit FullHashCallbackInfo( nit: don't need explicit https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.h:111: FullHashCallback callback); nit: Pass callbacks by const-reference (they do ref-counting internally, but you can avoid that w/ const-ref). https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.h:122: // The fetcher that will return the response from the server. Maybe comment on why it's here (so you can delete it after?) https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.h:171: // synchronously. Add a comment on what "apis" is and why it's different here (explaining to yourself 12 months from now...) https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.h:248: // permission api metadata for full hashes in those |full_hash_infos| that nit: capitalize API (And TTL for that matter) when they are separate words https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.h:250: void OnFullHashForApi(ThreatMetadataForApiCallback api_callback, pass callbacks by const ref. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.h:259: bool ParseHashResponse(const std::string& data, nit: How about response_data instead of data? https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.h:263: // Parses the store specific |metadata| information from the |match|. nit: describe what it returns https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... File components/safe_browsing_db/v4_get_hash_protocol_manager_unittest.cc (right): https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager_unittest.cc:170: FullHashToStoreAndHashPrefixesMap full_hash_to_store_and_hash_prefixes; nit: This is a long type name and a long var name. Could you make the var name shorter? It gets hard to read. Here and below https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager_unittest.cc:172: StoreAndHashPrefix(GetUrlSocEngId(), HashPrefix("AHashPrefix"))); Is it ok to have the prefix not match the full hash that in the map key? https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager_unittest.cc:384: { I'm glad you scoped these separately, and I think you could go a step further and make them separate tests since they aren't dependent on each other. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager_unittest.cc:537: TEST_F(V4GetHashProtocolManagerTest, GetCachedResults) { An overall comment: I think it would be less fragile to use methods on *pm to store things in the cache rather than modifying its internal state. That would also verify that when things get cached, their TTL/etc are written correctly. I'm torn on if this is important enough to rewrite these tests -- I'll let you decide. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager_unittest.cc:539: FullHash full_hash("example.com/"); nit: A url-as-hash is sortof confusing https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager_unittest.cc:558: CachedHashPrefixInfo* entry = &(*cache)[prefix]; Do all these cases need to be in one test? Could you split them into separate tests, all calling a common setup function? It's pretty hard to tell how they interact, and what the state of the cache is at each step (since it's the cumulative affect of all preceding lines). A possible middleground: Clear out *entry at the start of each sub-test. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager_unittest.cc:609: FullHash full_hash_1("example.com/test"); nit: url-as-hash is confusing https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... File components/safe_browsing_db/v4_local_database_manager.cc (right): https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_local_database_manager.cc:157: full_hash_to_store_and_hash_prefixes[full_hash] = nit: use .insert(..) so it doesn't create a temporary first and then overwrite it. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... File components/safe_browsing_db/v4_local_database_manager_unittest.cc (right): https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_local_database_manager_unittest.cc:117: store_and_hash_prefixes.push_back( nit: emplace_back(..,..) here and below https://codereview.chromium.org/2233103002/diff/340001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2233103002/diff/340001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:49627: -<histogram name="SafeBrowsing.V4GetHashCheckResult" I think we should be able to use this -- it's useful to know how often the response has something that matches out full-hashes.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Incoorporate nparker@'s feedback
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Thanks for the quick review. I've incorporated all feedback, except the histogram change. PTAL. https://codereview.chromium.org/2233103002/diff/260001/components/safe_browsi... File components/safe_browsing_db/BUILD.gn (right): https://codereview.chromium.org/2233103002/diff/260001/components/safe_browsi... components/safe_browsing_db/BUILD.gn:333: # "database_manager_unittest.cc", On 2016/09/09 21:26:20, Nathan Parker wrote: > is this staying? Fixed in a later patch. https://codereview.chromium.org/2233103002/diff/300001/components/safe_browsi... File components/safe_browsing_db/database_manager.cc (right): https://codereview.chromium.org/2233103002/diff/300001/components/safe_browsi... components/safe_browsing_db/database_manager.cc:28: base::hash_set<UpdateListIdentifier> stores_to_look({GetChromeUrlApiId()}); On 2016/09/09 21:26:20, Nathan Parker wrote: > Maybe add a TODO to pull in the list of all stores. Once you do that, it'll > start exercising more of the caching logic (multiple stores fetched, only one is > actively used). This database manager only cares about this store. The V4LDBM is the one that will care about all other stores. https://codereview.chromium.org/2233103002/diff/300001/components/safe_browsi... File components/safe_browsing_db/database_manager.h (right): https://codereview.chromium.org/2233103002/diff/300001/components/safe_browsi... components/safe_browsing_db/database_manager.h:185: SafeBrowsingDatabaseManager::Client* client() const { return client_; } On 2016/09/09 21:26:20, Nathan Parker wrote: > nit: can remove SafeBrowsingDatabaseManager:: Done. https://codereview.chromium.org/2233103002/diff/300001/components/safe_browsi... components/safe_browsing_db/database_manager.h:188: void set_start_time(base::TimeTicks start) {start_time_ = start;} On 2016/09/09 21:26:20, Nathan Parker wrote: > nit: I think this should be camel case: SetStartTime(..) "cheap" functions may use lower case with underscores. Source: https://google.github.io/styleguide/cppguide.html#Function_Names https://codereview.chromium.org/2233103002/diff/300001/components/safe_browsi... components/safe_browsing_db/database_manager.h:231: // Called on the IO thread wheh the SafeBrowsingProtocolManager has received On 2016/09/09 21:26:20, Nathan Parker wrote: > nit: s/wheh/when (existing typo) Done. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... File components/safe_browsing_db/v4_database.cc (right): https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_database.cc:175: DCHECK(matched_store_and_hash_prefixes); On 2016/09/09 21:26:20, Nathan Parker wrote: > nit: You don't need the DCHECK if you're accessing the pointer right away, since > it'll crash immediately anyway. Done. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_database.cc:183: matched_store_and_hash_prefixes->push_back( On 2016/09/09 21:26:20, Nathan Parker wrote: > nit: Does this work: > > ->emplace_back(identifier, hash_prefix) > > That's simpler and may avoid a temporary. Done. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... File components/safe_browsing_db/v4_get_hash_protocol_manager.cc (right): https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:112: return base::WrapUnique(new V4GetHashProtocolManager( On 2016/09/09 21:26:21, Nathan Parker wrote: > The new recommended way, from ptr_util.h: > > // MakeUnique<T>(args) should be preferred over WrapUnique(new T(args)): bare > // calls to `new` should be treated with scrutiny. I used that at first, but it requires the constructor to be visible to non-friends (i.e. public), which it is not. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:208: } On 2016/09/09 21:26:21, Nathan Parker wrote: > nit: Could just DCHECK(!stores_to_request.empty()) up top, with the same effect. Done. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:243: // (i.e. treat the page as safe). On 2016/09/09 21:26:21, Nathan Parker wrote: > nit: "i.e. just use the cache and potentially treat the page as safe" Done. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:286: full_hash_to_store_and_hash_prefixes[full_hash].push_back( On 2016/09/09 21:26:21, Nathan Parker wrote: > .emplace_back(GetChromeUrlApiId(), prefix), if that works. > Done. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:302: DCHECK(prefixes_to_request); On 2016/09/09 21:26:21, Nathan Parker wrote: > nit: can skip checking the ptr if you're going to call a method on it right > away. Done. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:338: for (const auto& it : full_hash_to_store_and_hash_prefixes) { On 2016/09/09 21:26:21, Nathan Parker wrote: > I'm assuming this code is not modified, ya? If that's not true, I'll look at it > in detail. Most of it has been re-written. Please do take a look. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:523: if (match.has_threat_entry_metadata()) { On 2016/09/09 21:26:20, Nathan Parker wrote: > Can you reduce the nested if's by negating them and retuning immediately if > there's an unexpected value? Done. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:604: // If negative_cache_expire is null, don't cache the results it's not clear On 2016/09/09 21:26:21, Nathan Parker wrote: > nit: s/it's/since it's/ Done. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:612: full_hash_cache_[prefix].full_hash_infos.clear(); On 2016/09/09 21:26:21, Nathan Parker wrote: > This looks up prefix multiple times. You could use find() once to get an > iterator to modify the contents. If it doesn't exist, you'd need to insert an > empty entry first. Changed to get a reference to it first (or add it if not already present) and then use that reference. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:635: full_hash_to_store_and_hash_prefixes.at(fhi.full_hash)) { On 2016/09/09 21:26:21, Nathan Parker wrote: > rather than looking up full_hash again, save the iterator returned by .find() > above Done. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:690: const FullHashCallback& callback = fhci->callback; On 2016/09/09 21:26:21, Nathan Parker wrote: > just call fhci->callback.Run(..) Done. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... File components/safe_browsing_db/v4_get_hash_protocol_manager.h (right): https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.h:62: explicit FullHashInfo(const FullHash& full_hash, On 2016/09/09 21:26:21, Nathan Parker wrote: > nit: I don't think you need explicit if the ctor has > 1 arg (since it couldn't > accidentally cast from 3 args) Good catch. Also not for copy constructors so removed that also. https://google.github.io/styleguide/cppguide.html#Implicit_Conversions https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.h:105: explicit FullHashCallbackInfo( On 2016/09/09 21:26:21, Nathan Parker wrote: > nit: don't need explicit Done. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.h:111: FullHashCallback callback); On 2016/09/09 21:26:21, Nathan Parker wrote: > nit: Pass callbacks by const-reference (they do ref-counting internally, but you > can avoid that w/ const-ref). Done. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.h:122: // The fetcher that will return the response from the server. On 2016/09/09 21:26:21, Nathan Parker wrote: > Maybe comment on why it's here (so you can delete it after?) Done. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.h:171: // synchronously. On 2016/09/09 21:26:21, Nathan Parker wrote: > Add a comment on what "apis" is and why it's different here (explaining to > yourself 12 months from now...) Done. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.h:248: // permission api metadata for full hashes in those |full_hash_infos| that On 2016/09/09 21:26:21, Nathan Parker wrote: > nit: capitalize API (And TTL for that matter) when they are separate words Done. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.h:250: void OnFullHashForApi(ThreatMetadataForApiCallback api_callback, On 2016/09/09 21:26:21, Nathan Parker wrote: > pass callbacks by const ref. Done. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.h:259: bool ParseHashResponse(const std::string& data, On 2016/09/09 21:26:21, Nathan Parker wrote: > nit: How about response_data instead of data? Done. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.h:263: // Parses the store specific |metadata| information from the |match|. On 2016/09/09 21:26:21, Nathan Parker wrote: > nit: describe what it returns Done. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... File components/safe_browsing_db/v4_get_hash_protocol_manager_unittest.cc (right): https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager_unittest.cc:170: FullHashToStoreAndHashPrefixesMap full_hash_to_store_and_hash_prefixes; On 2016/09/09 21:26:22, Nathan Parker wrote: > nit: This is a long type name and a long var name. Could you make the var name > shorter? It gets hard to read. Here and below If you don't mind it too much, I actually prefer the type names to be a bit wordy if they represent something that's otherwise difficult to convey using a shorter name. Changed the var name. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager_unittest.cc:172: StoreAndHashPrefix(GetUrlSocEngId(), HashPrefix("AHashPrefix"))); On 2016/09/09 21:26:22, Nathan Parker wrote: > Is it ok to have the prefix not match the full hash that in the map key? In general not. But in this case it is irrelevant because the response from the server is that of error. However, in the spirit of keeping it consistent, I've changed it. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager_unittest.cc:384: { On 2016/09/09 21:26:22, Nathan Parker wrote: > I'm glad you scoped these separately, and I think you could go a step further > and make them separate tests since they aren't dependent on each other. I'm usually in favor of keeping tests small and focused. In this case, since they are doing a related thing, it might be easier for a future developer to get the big picture of what they are testing. I think there's some value in keeping these together. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager_unittest.cc:539: FullHash full_hash("example.com/"); On 2016/09/09 21:26:22, Nathan Parker wrote: > nit: A url-as-hash is sortof confusing Done. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager_unittest.cc:558: CachedHashPrefixInfo* entry = &(*cache)[prefix]; On 2016/09/09 21:26:21, Nathan Parker wrote: > Do all these cases need to be in one test? Could you split them into separate > tests, all calling a common setup function? It's pretty hard to tell how they > interact, and what the state of the cache is at each step (since it's the > cumulative affect of all preceding lines). > > A possible middleground: Clear out *entry at the start of each sub-test. Scoped each of the cases, and clear the cache before each one. These tests are doing related stuff so it is good to be able to keep the close to each other to spot the differences easily. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager_unittest.cc:609: FullHash full_hash_1("example.com/test"); On 2016/09/09 21:26:22, Nathan Parker wrote: > nit: url-as-hash is confusing Done. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... File components/safe_browsing_db/v4_local_database_manager_unittest.cc (right): https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_local_database_manager_unittest.cc:117: store_and_hash_prefixes.push_back( On 2016/09/09 21:26:22, Nathan Parker wrote: > nit: emplace_back(..,..) > here and below Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #7 (id:220001) has been deleted
+francois@mozilla.com Scott, Kendra: Could you also please take a look? Thanks.
https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... File components/safe_browsing_db/v4_get_hash_protocol_manager.cc (right): https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:338: for (const auto& it : full_hash_to_store_and_hash_prefixes) { On 2016/09/09 23:25:17, vakh wrote: > On 2016/09/09 21:26:21, Nathan Parker wrote: > > I'm assuming this code is not modified, ya? If that's not true, I'll look at > it > > in detail. > > Most of it has been re-written. Please do take a look. Seems ok to me. I see you aren't removing anything, but that should be ok if you plan to periodically clean it up. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:344: const auto& prefix_entry = full_hash_cache_.find(prefix); I think this shouldn't be a reference, since find doesn't return a reference. You might end up with a reference to a temporary value, and I don't know what the scope of that is.
https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... File components/safe_browsing_db/v4_get_hash_protocol_manager.cc (right): https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:338: for (const auto& it : full_hash_to_store_and_hash_prefixes) { On 2016/09/10 00:26:25, Nathan Parker wrote: > On 2016/09/09 23:25:17, vakh wrote: > > On 2016/09/09 21:26:21, Nathan Parker wrote: > > > I'm assuming this code is not modified, ya? If that's not true, I'll look > at > > it > > > in detail. > > > > Most of it has been re-written. Please do take a look. > > Seems ok to me. I see you aren't removing anything, but that should be ok if > you plan to periodically clean it up. Acknowledged. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:344: const auto& prefix_entry = full_hash_cache_.find(prefix); On 2016/09/10 00:26:25, Nathan Parker wrote: > I think this shouldn't be a reference, since find doesn't return a reference. > You might end up with a reference to a temporary value, and I don't know what > the scope of that is. It's a common pattern: https://cs.chromium.org/search/?q=const%5C+auto.*find&sq=package:chromium&typ...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Bring back the histogram to check if there were any hits in the response from the server
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: + holte@chromium.org
+holte@ for histograms.xml
https://codereview.chromium.org/2233103002/diff/340001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2233103002/diff/340001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:49627: -<histogram name="SafeBrowsing.V4GetHashCheckResult" On 2016/09/09 21:26:22, Nathan Parker wrote: > I think we should be able to use this -- it's useful to know how often the > response has something that matches out full-hashes. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... File components/safe_browsing_db/v4_get_hash_protocol_manager.cc (right): https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:344: const auto& prefix_entry = full_hash_cache_.find(prefix); On 2016/09/10 00:29:29, vakh wrote: > On 2016/09/10 00:26:25, Nathan Parker wrote: > > I think this shouldn't be a reference, since find doesn't return a reference. > > You might end up with a reference to a temporary value, and I don't know what > > the scope of that is. > > It's a common pattern: > https://cs.chromium.org/search/?q=const%5C+auto.*find&sq=package:chromium&typ... But this is a more common pattern: https://cs.chromium.org/search/?q=auto%5C+.%2B%3D%5C+.%2Bfind&sq=package:chro... Some discussion here implies they are approximately equivalent: http://stackoverflow.com/questions/4684646/why-assign-a-return-value-to-a-ref... I won't nit pick.
lgtm. Your call on the minor nits. I'm feeling like this is getting far enough beyond me that I'm not terribly concerned I'll have the opportunity to work on this code anyhow :-). https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... File components/safe_browsing_db/v4_get_hash_protocol_manager.cc (right): https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:344: const auto& prefix_entry = full_hash_cache_.find(prefix); On 2016/09/12 17:02:07, Nathan Parker wrote: > On 2016/09/10 00:29:29, vakh wrote: > > On 2016/09/10 00:26:25, Nathan Parker wrote: > > > I think this shouldn't be a reference, since find doesn't return a > reference. > > > You might end up with a reference to a temporary value, and I don't know > what > > > the scope of that is. > > > > It's a common pattern: > > > https://cs.chromium.org/search/?q=const%5C+auto.*find&sq=package:chromium&typ... > > But this is a more common pattern: > https://cs.chromium.org/search/?q=auto%5C+.%2B%3D%5C+.%2Bfind&sq=package:chro... > > Some discussion here implies they are approximately equivalent: > http://stackoverflow.com/questions/4684646/why-assign-a-return-value-to-a-ref... > > I won't nit pick. In general, the temporary lives for the scope of the statement, but this is special-cased to extend the scoped to match that of the reference. Offer only good for const refs. On the flip side, move-constructing an iterator is likely to be efficient :-). https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... File components/safe_browsing_db/database_manager_unittest.cc (right): https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... components/safe_browsing_db/database_manager_unittest.cc:80: &full_hash); It's a specific string. I wonder if it would be more self-documenting to just say: // This will match "https://www.example.com/more". full_hash = crypto::SHA256HashString("example.com/more"); https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... File components/safe_browsing_db/v4_get_hash_protocol_manager.cc (right): https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:168: prefixes_requested(prefixes_requested) {} AFAICT from the calling code, all of these could be passed in with std::move(), though that would probably be annoying, so I'd only bother if they were likely to be expensive. The one parameter where it might be worthwhile at some time would be callback, which I believe is implemented as a ref-counting wrapper around internal info, which implies that there's locking going on in there. https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:524: positive_ttl = clock_->Now(); Is this meant to be a placeholder which will not match? In that case you should subtract TimeDelta(1s), just to make sure that it can never be flaky. https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:537: ThreatMetadata* metadata) { Probably the various string constants in here should be interned into a kGlobalOfSomeSortToPreventTypos. https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:635: full_hash_info.full_hash, prefix)) { Is this iterating the vector of prefixes, then iterating the vector of full hashes looking for matches? If so, that's probably not great if you can get more than a handful of items in there. Though I guess parallel sorted iteration would be complicated. Sigh. [This does have me wondering if the abstractions should maintain a 32-bit quick-prefix in the main object to efficiently short-circuit many checks.] https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:650: bool matched_full_hash = full_hash_to_store_and_hash_prefixes.end() != it; I dunno, feels strained versus just inlining the !=. https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... File components/safe_browsing_db/v4_get_hash_protocol_manager.h (right): https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.h:57: base::Time positive_ttl; I'm slightly unnerved by this use of "ttl" with an absolute time, as I'm more used to "ttl" being used with a time delta. I think I see what you mean, I'm just nervous about whether this will someday lead to mistakes. https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.h:98: // were no matches, and that the resource is safe. Is this going to acquire additional parameters? Or did it lose parameters during development?
shess@ feedback. Remove a comment about UMA.
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...
Thanks for the review, shess@ holte@ -- could you please review the histograms? Thanks. https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... File components/safe_browsing_db/v4_get_hash_protocol_manager.cc (right): https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:344: const auto& prefix_entry = full_hash_cache_.find(prefix); On 2016/09/12 23:31:34, Scott Hess wrote: > On 2016/09/12 17:02:07, Nathan Parker wrote: > > On 2016/09/10 00:29:29, vakh wrote: > > > On 2016/09/10 00:26:25, Nathan Parker wrote: > > > > I think this shouldn't be a reference, since find doesn't return a > > reference. > > > > You might end up with a reference to a temporary value, and I don't know > > what > > > > the scope of that is. > > > > > > It's a common pattern: > > > > > > https://cs.chromium.org/search/?q=const%5C+auto.*find&sq=package:chromium&typ... > > > > But this is a more common pattern: > > > https://cs.chromium.org/search/?q=auto%5C+.%2B%3D%5C+.%2Bfind&sq=package:chro... > > > > Some discussion here implies they are approximately equivalent: > > > http://stackoverflow.com/questions/4684646/why-assign-a-return-value-to-a-ref... > > > > I won't nit pick. > > In general, the temporary lives for the scope of the statement, but this is > special-cased to extend the scoped to match that of the reference. Offer only > good for const refs. > > On the flip side, move-constructing an iterator is likely to be efficient :-). Done. Changed to "auto it = " https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... File components/safe_browsing_db/database_manager_unittest.cc (right): https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... components/safe_browsing_db/database_manager_unittest.cc:80: &full_hash); On 2016/09/12 23:31:34, Scott Hess wrote: > It's a specific string. I wonder if it would be more self-documenting to just > say: > // This will match "https://www.example.com/more". > full_hash = crypto::SHA256HashString("example.com/more"); Good idea. Done. https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... File components/safe_browsing_db/v4_get_hash_protocol_manager.cc (right): https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:168: prefixes_requested(prefixes_requested) {} On 2016/09/12 23:31:35, Scott Hess wrote: > AFAICT from the calling code, all of these could be passed in with std::move(), > though that would probably be annoying, so I'd only bother if they were likely > to be expensive. The one parameter where it might be worthwhile at some time > would be callback, which I believe is implemented as a ref-counting wrapper > around internal info, which implies that there's locking going on in there. Acknowledged. Work for when we put in perf markers. https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:524: positive_ttl = clock_->Now(); On 2016/09/12 23:31:35, Scott Hess wrote: > Is this meant to be a placeholder which will not match? In that case you should > subtract TimeDelta(1s), just to make sure that it can never be flaky. Good idea. Done. https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:537: ThreatMetadata* metadata) { On 2016/09/12 23:31:34, Scott Hess wrote: > Probably the various string constants in here should be interned into a > kGlobalOfSomeSortToPreventTypos. Done. https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:635: full_hash_info.full_hash, prefix)) { On 2016/09/12 23:31:35, Scott Hess wrote: > Is this iterating the vector of prefixes, then iterating the vector of full > hashes looking for matches? If so, that's probably not great if you can get > more than a handful of items in there. Though I guess parallel sorted iteration > would be complicated. Sigh. > > [This does have me wondering if the abstractions should maintain a 32-bit > quick-prefix in the main object to efficiently short-circuit many checks.] The hash prefixes can be >32 bits now so that won't always work. https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:650: bool matched_full_hash = full_hash_to_store_and_hash_prefixes.end() != it; On 2016/09/12 23:31:35, Scott Hess wrote: > I dunno, feels strained versus just inlining the !=. Done. https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... File components/safe_browsing_db/v4_get_hash_protocol_manager.h (right): https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.h:57: base::Time positive_ttl; On 2016/09/12 23:31:35, Scott Hess wrote: > I'm slightly unnerved by this use of "ttl" with an absolute time, as I'm more > used to "ttl" being used with a time delta. I think I see what you mean, I'm > just nervous about whether this will someday lead to mistakes. I understand your concern but I feel like there are two reasons why it's not very likely to happen: 1. Anyone changing this code in any meaningful way would/should know what this "ttl" stands for. I'd consider that a minimum requirement. 2. This stored as base::Time so assigning or comparing against TimeDelta would lead to compilation failure. That being said, I'm totally willing to change it to something else that conveys the meaning just as (or more) clearly. If you have a term in mind, please let me know. https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.h:98: // were no matches, and that the resource is safe. On 2016/09/12 23:31:35, Scott Hess wrote: > Is this going to acquire additional parameters? Or did it lose parameters > during development? Are you asking this because of "Parameters:"? I thought that was the convention but I did not bother to look up what happens in case of single arguments. I just looked it up and "Parameter:" occurs only 6 times in the entire codebase so I'd consider that uncommon. (No parameters expected to be added in the near future.)
still lgtm... https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... File components/safe_browsing_db/v4_get_hash_protocol_manager.cc (right): https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:344: const auto& prefix_entry = full_hash_cache_.find(prefix); On 2016/09/13 00:12:06, vakh (Varun Khaneja) wrote: > On 2016/09/12 23:31:34, Scott Hess wrote: > > On 2016/09/12 17:02:07, Nathan Parker wrote: > > > On 2016/09/10 00:29:29, vakh wrote: > > > > On 2016/09/10 00:26:25, Nathan Parker wrote: > > > > > I think this shouldn't be a reference, since find doesn't return a > > > reference. > > > > > You might end up with a reference to a temporary value, and I don't know > > > what > > > > > the scope of that is. > > > > > > > > It's a common pattern: > > > > > > > > > > https://cs.chromium.org/search/?q=const%5C+auto.*find&sq=package:chromium&typ... > > > > > > But this is a more common pattern: > > > > > > https://cs.chromium.org/search/?q=auto%5C+.%2B%3D%5C+.%2Bfind&sq=package:chro... > > > > > > Some discussion here implies they are approximately equivalent: > > > > > > http://stackoverflow.com/questions/4684646/why-assign-a-return-value-to-a-ref... > > > > > > I won't nit pick. > > > > In general, the temporary lives for the scope of the statement, but this is > > special-cased to extend the scoped to match that of the reference. Offer only > > good for const refs. > > > > On the flip side, move-constructing an iterator is likely to be efficient :-). > > Done. Changed to "auto it = " Compiler is standing to the side saying "I'm going to emit the same code either way." https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... File components/safe_browsing_db/v4_get_hash_protocol_manager.cc (right): https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:635: full_hash_info.full_hash, prefix)) { On 2016/09/13 00:12:07, vakh (Varun Khaneja) wrote: > On 2016/09/12 23:31:35, Scott Hess wrote: > > Is this iterating the vector of prefixes, then iterating the vector of full > > hashes looking for matches? If so, that's probably not great if you can get > > more than a handful of items in there. Though I guess parallel sorted > iteration > > would be complicated. Sigh. > > > > [This does have me wondering if the abstractions should maintain a 32-bit > > quick-prefix in the main object to efficiently short-circuit many checks.] > The hash prefixes can be >32 bits now so that won't always work. Mostly what I mean is that right now much of this is looking at a lot of indirections to do anything. For instance, to compare hash prefixes you have to pull up length info from the string object then string data from another heap allocation, _then_ you start comparing. If there were a 32-bit prefix at the top level, you could just compare those directly which would potentially short-circuit most other comparisons. Basically I'm nervous that there's going to be a lot of performance drag in the STL stacking going on, and it's probably going to be somewhat challenging to isolate. https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... File components/safe_browsing_db/v4_get_hash_protocol_manager.h (right): https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.h:57: base::Time positive_ttl; On 2016/09/13 00:12:07, vakh (Varun Khaneja) wrote: > On 2016/09/12 23:31:35, Scott Hess wrote: > > I'm slightly unnerved by this use of "ttl" with an absolute time, as I'm more > > used to "ttl" being used with a time delta. I think I see what you mean, I'm > > just nervous about whether this will someday lead to mistakes. > > I understand your concern but I feel like there are two reasons why it's not > very likely to happen: > 1. Anyone changing this code in any meaningful way would/should know what this > "ttl" stands for. I'd consider that a minimum requirement. > 2. This stored as base::Time so assigning or comparing against TimeDelta would > lead to compilation failure. > > That being said, I'm totally willing to change it to something else that conveys > the meaning just as (or more) clearly. If you have a term in mind, please let me > know. Sometimes the code uses "expire" for this concept, though "expiry" would be the better sense. I guess maybe if nobody else has remarked on it it's not an issue. https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.h:98: // were no matches, and that the resource is safe. On 2016/09/13 00:12:07, vakh (Varun Khaneja) wrote: > On 2016/09/12 23:31:35, Scott Hess wrote: > > Is this going to acquire additional parameters? Or did it lose parameters > > during development? > > Are you asking this because of "Parameters:"? > I thought that was the convention but I did not bother to look up what happens > in case of single arguments. I just looked it up and "Parameter:" occurs only 6 > times in the entire codebase so I'd consider that uncommon. > (No parameters expected to be added in the near future.) Mostly because the comment is setup like "Here's the list of things that will happen", and then there's only a single item. It might flow better (and be easier to rewrap in Emacs) if it just reads like a paragraph without structure. If the concern is having the anonymous parameter be confusing, you can always name it. void(const std::vec<FHI>& results) should parse fine. But I don't think it's necessary if there's just a single parameter, "the parameter" is sufficient to describe it.
shess@ feedback - part 2.
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/2233103002/diff/340001/components/safe_browsi... File components/safe_browsing_db/v4_get_hash_protocol_manager.cc (right): https://codereview.chromium.org/2233103002/diff/340001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:344: const auto& prefix_entry = full_hash_cache_.find(prefix); On 2016/09/13 00:31:34, Scott Hess wrote: > On 2016/09/13 00:12:06, vakh (Varun Khaneja) wrote: > > On 2016/09/12 23:31:34, Scott Hess wrote: > > > On 2016/09/12 17:02:07, Nathan Parker wrote: > > > > On 2016/09/10 00:29:29, vakh wrote: > > > > > On 2016/09/10 00:26:25, Nathan Parker wrote: > > > > > > I think this shouldn't be a reference, since find doesn't return a > > > > reference. > > > > > > You might end up with a reference to a temporary value, and I don't > know > > > > what > > > > > > the scope of that is. > > > > > > > > > > It's a common pattern: > > > > > > > > > > > > > > > https://cs.chromium.org/search/?q=const%5C+auto.*find&sq=package:chromium&typ... > > > > > > > > But this is a more common pattern: > > > > > > > > > > https://cs.chromium.org/search/?q=auto%5C+.%2B%3D%5C+.%2Bfind&sq=package:chro... > > > > > > > > Some discussion here implies they are approximately equivalent: > > > > > > > > > > http://stackoverflow.com/questions/4684646/why-assign-a-return-value-to-a-ref... > > > > > > > > I won't nit pick. > > > > > > In general, the temporary lives for the scope of the statement, but this is > > > special-cased to extend the scoped to match that of the reference. Offer > only > > > good for const refs. > > > > > > On the flip side, move-constructing an iterator is likely to be efficient > :-). > > > > Done. Changed to "auto it = " > > Compiler is standing to the side saying "I'm going to emit the same code either > way." Acknowledged. :) https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... File components/safe_browsing_db/v4_get_hash_protocol_manager.cc (right): https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.cc:635: full_hash_info.full_hash, prefix)) { On 2016/09/13 00:31:34, Scott Hess wrote: > On 2016/09/13 00:12:07, vakh (Varun Khaneja) wrote: > > On 2016/09/12 23:31:35, Scott Hess wrote: > > > Is this iterating the vector of prefixes, then iterating the vector of full > > > hashes looking for matches? If so, that's probably not great if you can get > > > more than a handful of items in there. Though I guess parallel sorted > > iteration > > > would be complicated. Sigh. > > > > > > [This does have me wondering if the abstractions should maintain a 32-bit > > > quick-prefix in the main object to efficiently short-circuit many checks.] > > The hash prefixes can be >32 bits now so that won't always work. > > Mostly what I mean is that right now much of this is looking at a lot of > indirections to do anything. For instance, to compare hash prefixes you have to > pull up length info from the string object then string data from another heap > allocation, _then_ you start comparing. If there were a 32-bit prefix at the > top level, you could just compare those directly which would potentially > short-circuit most other comparisons. > > Basically I'm nervous that there's going to be a lot of performance drag in the > STL stacking going on, and it's probably going to be somewhat challenging to > isolate. Yes, I share that concern. But, as you said, this will be useful only if there are more than a handful of hash prefixes, which we don't expect to be the case. I would prefer to add a UMA counter to see how many hash prefixes do we send requests for anyway before trying to optimize it. I'll add that UMA counter, along with a bunch of other metrics that I need to add to track performance. https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... File components/safe_browsing_db/v4_get_hash_protocol_manager.h (right): https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.h:57: base::Time positive_ttl; On 2016/09/13 00:31:34, Scott Hess wrote: > On 2016/09/13 00:12:07, vakh (Varun Khaneja) wrote: > > On 2016/09/12 23:31:35, Scott Hess wrote: > > > I'm slightly unnerved by this use of "ttl" with an absolute time, as I'm > more > > > used to "ttl" being used with a time delta. I think I see what you mean, > I'm > > > just nervous about whether this will someday lead to mistakes. > > > > I understand your concern but I feel like there are two reasons why it's not > > very likely to happen: > > 1. Anyone changing this code in any meaningful way would/should know what this > > "ttl" stands for. I'd consider that a minimum requirement. > > 2. This stored as base::Time so assigning or comparing against TimeDelta would > > lead to compilation failure. > > > > That being said, I'm totally willing to change it to something else that > conveys > > the meaning just as (or more) clearly. If you have a term in mind, please let > me > > know. > > Sometimes the code uses "expire" for this concept, though "expiry" would be the > better sense. I guess maybe if nobody else has remarked on it it's not an > issue. Done. https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.h:98: // were no matches, and that the resource is safe. On 2016/09/13 00:31:34, Scott Hess wrote: > On 2016/09/13 00:12:07, vakh (Varun Khaneja) wrote: > > On 2016/09/12 23:31:35, Scott Hess wrote: > > > Is this going to acquire additional parameters? Or did it lose parameters > > > during development? > > > > Are you asking this because of "Parameters:"? > > I thought that was the convention but I did not bother to look up what happens > > in case of single arguments. I just looked it up and "Parameter:" occurs only > 6 > > times in the entire codebase so I'd consider that uncommon. > > (No parameters expected to be added in the near future.) > > Mostly because the comment is setup like "Here's the list of things that will > happen", and then there's only a single item. It might flow better (and be > easier to rewrap in Emacs) if it just reads like a paragraph without structure. > > If the concern is having the anonymous parameter be confusing, you can always > name it. void(const std::vec<FHI>& results) should parse fine. But I don't > think it's necessary if there's just a single parameter, "the parameter" is > sufficient to describe it. Done. https://codereview.chromium.org/2233103002/diff/400001/components/safe_browsi... components/safe_browsing_db/v4_get_hash_protocol_manager.h:98: // were no matches, and that the resource is safe. On 2016/09/13 00:31:34, Scott Hess wrote: > On 2016/09/13 00:12:07, vakh (Varun Khaneja) wrote: > > On 2016/09/12 23:31:35, Scott Hess wrote: > > > Is this going to acquire additional parameters? Or did it lose parameters > > > during development? > > > > Are you asking this because of "Parameters:"? > > I thought that was the convention but I did not bother to look up what happens > > in case of single arguments. I just looked it up and "Parameter:" occurs only > 6 > > times in the entire codebase so I'd consider that uncommon. > > (No parameters expected to be added in the near future.) > > Mostly because the comment is setup like "Here's the list of things that will > happen", and then there's only a single item. It might flow better (and be > easier to rewrap in Emacs) if it just reads like a paragraph without structure. > > If the concern is having the anonymous parameter be confusing, you can always > name it. void(const std::vec<FHI>& results) should parse fine. But I don't > think it's necessary if there's just a single parameter, "the parameter" is > sufficient to describe it. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
histograms lgtm
The CQ bit was checked by vakh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org, shess@chromium.org Link to the patchset: https://codereview.chromium.org/2233103002/#ps440001 (title: "shess@ feedback - part 2.")
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.
Description was changed from ========== Move the logic to cache the full hash responses from the server into v4_get_hash_protocol_manager. Design doc: https://goto.google.com/design-doc-v4-full-hash-manager Overall, this CL does the following: 1. Moves caching to V4GetHashProtocolManager. 2. Refactors V4GetHashProtocolManager to use the data structures used in rest of the V4 code. 3. Re-defines how cache is implemented. TODOs: 1. Done: Merge the response from the server and the response from the cache. 2. Figure out what UMA metrics to add (I have removed some that are no longer valid after the refactor). BUG=616647, 543161 ========== to ========== Move the logic to cache the full hash responses from the server into v4_get_hash_protocol_manager. Design doc: https://goto.google.com/design-doc-v4-full-hash-manager Overall, this CL does the following: 1. Moves caching to V4GetHashProtocolManager. 2. Refactors V4GetHashProtocolManager to use the data structures used in rest of the V4 code. 3. Re-defines how cache is implemented. TODOs: 1. Done: Merge the response from the server and the response from the cache. 2. Figure out what UMA metrics to add (I have removed some that are no longer valid after the refactor). BUG=616647, 543161 ==========
Message was sent while issue was closed.
Committed patchset #17 (id:440001)
Message was sent while issue was closed.
Description was changed from ========== Move the logic to cache the full hash responses from the server into v4_get_hash_protocol_manager. Design doc: https://goto.google.com/design-doc-v4-full-hash-manager Overall, this CL does the following: 1. Moves caching to V4GetHashProtocolManager. 2. Refactors V4GetHashProtocolManager to use the data structures used in rest of the V4 code. 3. Re-defines how cache is implemented. TODOs: 1. Done: Merge the response from the server and the response from the cache. 2. Figure out what UMA metrics to add (I have removed some that are no longer valid after the refactor). BUG=616647, 543161 ========== to ========== Move the logic to cache the full hash responses from the server into v4_get_hash_protocol_manager. Design doc: https://goto.google.com/design-doc-v4-full-hash-manager Overall, this CL does the following: 1. Moves caching to V4GetHashProtocolManager. 2. Refactors V4GetHashProtocolManager to use the data structures used in rest of the V4 code. 3. Re-defines how cache is implemented. TODOs: 1. Done: Merge the response from the server and the response from the cache. 2. Figure out what UMA metrics to add (I have removed some that are no longer valid after the refactor). BUG=616647, 543161 Committed: https://crrev.com/1d2b239717bd198bd42f11e468e1eaca6b94cdc1 Cr-Commit-Position: refs/heads/master@{#418303} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/1d2b239717bd198bd42f11e468e1eaca6b94cdc1 Cr-Commit-Position: refs/heads/master@{#418303}
Message was sent while issue was closed.
lgtm Sorry for the delay. Thanks for doing this! Kendra |
