Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2229983002:
Send the list of used and unused resources for precache (Closed)
DescriptionSend the list of used and unused resources for precache
When precache manifest is fetched, the list of used and unused resources
corresponding to the top host are sent as request parameters in the
manifest URL. The resource URLs are hashed individually and sent as
base64 encoded string.
BUG=626463
Committed: https://crrev.com/4284452ea2f25f75aa1cb6e2386850fedd81ffe8
Cr-Commit-Position: refs/heads/master@{#414721}
Patch Set 1 #
Total comments: 84
Patch Set 2 : added-test #
Total comments: 23
Patch Set 3 : Added more tests #
Total comments: 18
Patch Set 4 : Addressed comments, removed timestamp from proto #
Total comments: 6
Patch Set 5 : Addressed comments #
Total comments: 1
Patch Set 6 : Addressed nits #Messages
Total messages: 48 (35 generated)
rajendrant@chromium.org changed reviewers: + bengr@chromium.org
Adding unittests. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... File components/precache/core/precache_fetcher.cc (left): https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:346: // TODO(bengr): Consider accessing these directly from the proto. Since the proto datastructure does not support removing elements from the front in O(1), it makes sense to copy to member variables. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... File components/precache/core/proto/unfinished_work.proto (right): https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/proto/unfinished_work.proto:27: // repeated PrecacheManifestURL manifest = 3; Deprecating this field. Since pending manifests can be saved in top_host itself.
https://codereview.chromium.org/2229983002/diff/1/components/precache/content... File components/precache/content/precache_manager_unittest.cc (right): https://codereview.chromium.org/2229983002/diff/1/components/precache/content... components/precache/content/precache_manager_unittest.cc:411: 1))); // Pair("Precache.Latency.Prefetch", 1), Please remove the dead code. Why are these no longer needed? https://codereview.chromium.org/2229983002/diff/1/components/precache/content... components/precache/content/precache_manager_unittest.cc:491: // expected_histogram_count_map["Precache.Latency.Prefetch"] += 3; Please remove the dead code. Why are these no longer needed? https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... File components/precache/core/precache_fetcher.cc (right): https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:163: base::SHA1HashBytes( test that this works on an empty string. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... File components/precache/core/precache_fetcher.h (right): https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.h:187: // Retrieves the manifest info in DB thread. Manifest info for each of the in -> on the https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.h:193: // Callback invoked when manifest info for all the top hosts is retrieved. when -> when the https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.h:208: // PrecacheDatabase should be accessed in the DB thread. in -> on https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... File components/precache/core/precache_referrer_host_table.cc (right): https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_referrer_host_table.cc:27: PrecacheReferrerHostEntry PrecacheReferrerHostTable::GetReferrerHost( Can this method be const? https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... File components/precache/core/precache_referrer_host_table.h (right): https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_referrer_host_table.h:32: const int64_t id; #include <stdint.h> https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... File components/precache/core/precache_url_table.h (right): https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_url_table.h:56: void GetURLListForReferrerHost(int64_t referrer_host_id, #include <stdint.h> https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... File components/precache/core/proto/precache.proto (right): https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/proto/precache.proto:37: optional PrecacheManifestId id = 3; Add a comment. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... File components/precache/core/proto/unfinished_work.proto (right): https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/proto/unfinished_work.proto:27: // repeated PrecacheManifestURL manifest = 3; On 2016/08/09 22:56:48, Raj wrote: > Deprecating this field. Since pending manifests can be saved in top_host itself. Typically, deprecation looks like this: // DEPRECATED: Manifest URLs remaining to be fetched. repeated PrecacheManifestURL deprecated_manifest = 3 [deprecated=true]
rajendrant@chromium.org changed reviewers: + sclittle@chromium.org
ptal. Maybe we could split into multiple CLs, for easy merge to M53. Reduced functionality of just sending url hash back and not covering all cases (pause/resume, etc) can be merged quickly. Subsequent non-M53 CLs can fix/restructure the code better.
Did a first pass, I'll review again after these comments are resolved. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... File components/precache/core/precache_database.h (right): https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_database.h:99: std::deque<GURL>& used_urls, Don't pass stuff in Chromium by non-const reference, it's against the style guide. Just pass a pointer if you need to. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_database.h:184: base::Time last_precache_timestamp_; nit: Could you add #include "base/time/time.h", since it's needed here? You can also remove the forward declaration of base::Time at the top of this file too. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... File components/precache/core/precache_fetcher.cc (left): https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:7: #include <algorithm> You still need <algorithm> for std::min. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... File components/precache/core/precache_fetcher.cc (right): https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:159: std::string hashes; nit: to avoid a bunch of reallocations, you could add: hashes.reserve(urls.size() * base::kSHA1Length); after this line so that it won't have to reallocate as you add to the string in the for loop. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:161: std::string url_spec = url.spec(); nit: change to const std::string& to avoid string copy https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:167: hashes.append(reinterpret_cast<const char*>(sha1_hash), 8); Replace "8" with "arraysize(sha1_hash)". https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:331: base::WeakPtr<PrecacheDatabase> precache_database, nit: pass by const ref to avoid extra Add/Remove ref operations. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:351: // Copy resources to member variable as a convenience. You're already changing the member variables so much, so why not just remove most of them altogether? Could you just keep track of the current resource index then, and increment that as you go along? That would make all these copies and member variables unnecessary. You could also just call unfinished_work_->add_resource() or whatever as new resources need to be fetched. You might also greatly simplify CancelPrecaching, since you could just call stuff like e.g. unfinished_work_->resource().erase(unfinished_work_->resource().begin(), unfinished_work_->resource().begin() + resource_index_); instead of copying everything back to the unfinished_work_ from the member variables. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:517: std::set<std::string> seen_top_hosts; You should move this into the below if-statement since it's only used within there. Also, I think you might be able to just do this with an std::set<base::StringPiece> here, and avoid a bunch of string copies, since none of the top hosts are being removed and you're just referencing them directly from the unfinished_work_ anyways. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:518: std::unique_ptr<std::deque<std::string>> top_hosts_to_fetch( You can remove the std::unique_ptr around this if you just use std::move when passing it to the callback below, and change all the methods that currently take ownership of the std::unique_ptr to take in a "std::deque<...>" (note how there's no "const &") and use std::move to take ownership of them in turn. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:528: if (top_hosts_to_fetch->empty() && resources_to_fetch_.empty()) { |top_hosts_to_fetch| will always be empty at this point, won't it? https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:535: top_hosts_to_fetch->emplace_back(host.hostname()); nit: I think you can just use push_back here, it'll have the same effect, plus it's simpler. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:541: top_hosts_to_fetch->emplace_back(host); nit: I think you can replace emplace_back with push_back here. Same with everywhere else you use emplace_back. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:561: std::deque<GURL> used_urls, unused_urls; nit: Can this just be an std::vector? Typically an std::vector is best unless there's a good reason since it's simpler. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:586: net::EscapeQueryParamValue(manifest.hostname, false), false)); Can't you just add all the query params when you create the manifest URL here? Repeatedly copying the GURL is kinda expensive. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... File components/precache/core/precache_fetcher.h (right): https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.h:10: #include <algorithm> Remove this include from the header file and put it in the .cc file. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.h:50: ManifestHostInfo(const ManifestHostInfo& other); Is it possible to make this struct movable as well? Doesn't have to be too complicated - just also add: ManifestHostInfo(ManifestHostInfo&&) = default; ManifestHostInfo& operator=(ManifestHostInfo&&) = default; (You'll probably want to just have the declarations here in the .h and the =default definitions in the .cc like you do here for the copy constructor.) That'll greatly reduce the number of string copies performed when dealing with these objects, since it'll just be copying the |manifest_url| then and moving everything else. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.h:54: const int64_t manifest_id; Can you change these to not be const? You can just pass around a "const ManifestHostInfo&" if you want them not to be modified. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.h:216: std::list<std::pair<GURL, std::string>> resources_to_fetch_; If you do chose to keep this member variable instead of just using the |unfinished_work_| with a |resource_to_fetch_index_|, then please change this to also be an std::deque instead of an std::list. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... File components/precache/core/precache_fetcher_unittest.cc (right): https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher_unittest.cc:96: std::multiset<GURL>& requested_urls() { return requested_urls_; } You shouldn't return a non-const reference here. Use a setter instead if you really need to change modify the value. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher_unittest.cc:121: class MockURLFetcherFactory : public net::URLFetcherFactory { FYI, for future CLs: Wow, we should really not need to use gmock for these tests. A lot of this test file baffles me - perhaps we should do a big cleanup of the precache component and all the tests at some point, just to make it easier to understand and develop faster. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher_unittest.cc:659: // unfinished_work->add_top_host()->set_hostname("bad-manifest.com"); remove dead code, here and elsewhere https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... File components/precache/core/precache_referrer_host_table.cc (right): https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_referrer_host_table.cc:73: db_->GetCachedStatement(SQL_FROM_HERE, "DELETE FROM precache_urls")); Update this to say "precache_referrer_hosts". Could you please add tests for this file? https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... File components/precache/core/precache_referrer_host_table.h (right): https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_referrer_host_table.h:21: static const int64_t INVALID_ID; nit: Name this like a constant, e.g. kInvalidId https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_referrer_host_table.h:33: const GURL referrer_host; Change all these to be non-const. Just pass around a "const PrecacheReferrerHostEntry&" if you want them to not be modified. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... File components/precache/core/precache_url_table.h (right): https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_url_table.h:57: std::deque<GURL>& used_urls, Don't pass by non-const ref, just pass a pointer instead. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_url_table.h:58: std::deque<GURL>& unused_urls); Can these just be std::vector instead of std::deque? https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... File components/precache/core/proto/precache.proto (right): https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/proto/precache.proto:20: optional string tophostname = 2; nit: maybe change to top_host_name? https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/proto/precache.proto:24: // Time at the creation of this manifest. What determines this time? The server? If so, then timestamps aren't guaranteed to be unique IDs, and this could cause weird bugs to happen. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/proto/precache.proto:25: optional Timestamp timestamp = 1; Why is this specifically a timestamp? Could this just be some opaque ID set by the server?
Addressed comments. Added more unittests. https://codereview.chromium.org/2229983002/diff/1/components/precache/content... File components/precache/content/precache_manager_unittest.cc (right): https://codereview.chromium.org/2229983002/diff/1/components/precache/content... components/precache/content/precache_manager_unittest.cc:411: 1))); // Pair("Precache.Latency.Prefetch", 1), On 2016/08/11 18:49:14, bengr wrote: > Please remove the dead code. > > Why are these no longer needed? Fixed test. https://codereview.chromium.org/2229983002/diff/1/components/precache/content... components/precache/content/precache_manager_unittest.cc:491: // expected_histogram_count_map["Precache.Latency.Prefetch"] += 3; On 2016/08/11 18:49:14, bengr wrote: > Please remove the dead code. > > Why are these no longer needed? Fixed test. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... File components/precache/core/precache_database.h (right): https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_database.h:99: std::deque<GURL>& used_urls, On 2016/08/11 22:52:35, sclittle wrote: > Don't pass stuff in Chromium by non-const reference, it's against the style > guide. Just pass a pointer if you need to. Done. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_database.h:184: base::Time last_precache_timestamp_; On 2016/08/11 22:52:35, sclittle wrote: > nit: Could you add #include "base/time/time.h", since it's needed here? You can > also remove the forward declaration of base::Time at the top of this file too. Done. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... File components/precache/core/precache_fetcher.cc (left): https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:7: #include <algorithm> On 2016/08/11 22:52:35, sclittle wrote: > You still need <algorithm> for std::min. Done. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... File components/precache/core/precache_fetcher.cc (right): https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:159: std::string hashes; On 2016/08/11 22:52:35, sclittle wrote: > nit: to avoid a bunch of reallocations, you could add: > > hashes.reserve(urls.size() * base::kSHA1Length); > > after this line so that it won't have to reallocate as you add to the string in > the for loop. Done. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:161: std::string url_spec = url.spec(); On 2016/08/11 22:52:35, sclittle wrote: > nit: change to const std::string& to avoid string copy Done. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:167: hashes.append(reinterpret_cast<const char*>(sha1_hash), 8); On 2016/08/11 22:52:35, sclittle wrote: > Replace "8" with "arraysize(sha1_hash)". hmm. sha1 is actually 20 bytes. But we are using only 8 bytes of it, as a tradeoff between sending more bytes vs to reduce hash collision. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:331: base::WeakPtr<PrecacheDatabase> precache_database, On 2016/08/11 22:52:35, sclittle wrote: > nit: pass by const ref to avoid extra Add/Remove ref operations. Done. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:351: // Copy resources to member variable as a convenience. On 2016/08/11 22:52:35, sclittle wrote: > You're already changing the member variables so much, so why not just remove > most of them altogether? > > Could you just keep track of the current resource index then, and increment that > as you go along? That would make all these copies and member variables > unnecessary. You could also just call unfinished_work_->add_resource() or > whatever as new resources need to be fetched. > > You might also greatly simplify CancelPrecaching, since you could just call > stuff like e.g. > > unfinished_work_->resource().erase(unfinished_work_->resource().begin(), > unfinished_work_->resource().begin() + resource_index_); > > instead of copying everything back to the unfinished_work_ from the member > variables. hmm. That is a good suggestion. Added a TODO for now, to do in a separate CL. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:517: std::set<std::string> seen_top_hosts; On 2016/08/11 22:52:35, sclittle wrote: > You should move this into the below if-statement since it's only used within > there. > > Also, I think you might be able to just do this with an > std::set<base::StringPiece> here, and avoid a bunch of string copies, since none > of the top hosts are being removed and you're just referencing them directly > from the unfinished_work_ anyways. Done. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:518: std::unique_ptr<std::deque<std::string>> top_hosts_to_fetch( On 2016/08/11 22:52:35, sclittle wrote: > You can remove the std::unique_ptr around this if you just use std::move when > passing it to the callback below, and change all the methods that currently take > ownership of the std::unique_ptr to take in a "std::deque<...>" (note how > there's no "const &") and use std::move to take ownership of them in turn. Nice. It works. But top_hosts_to_fetch is a local variable in stack now, and I do not know how std::move could move it without copy. At least a small std::deque structure would get copied ? https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:528: if (top_hosts_to_fetch->empty() && resources_to_fetch_.empty()) { On 2016/08/11 22:52:35, sclittle wrote: > |top_hosts_to_fetch| will always be empty at this point, won't it? Yep. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:535: top_hosts_to_fetch->emplace_back(host.hostname()); On 2016/08/11 22:52:35, sclittle wrote: > nit: I think you can just use push_back here, it'll have the same effect, plus > it's simpler. Done. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:541: top_hosts_to_fetch->emplace_back(host); On 2016/08/11 22:52:35, sclittle wrote: > nit: I think you can replace emplace_back with push_back here. Same with > everywhere else you use emplace_back. Done. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:561: std::deque<GURL> used_urls, unused_urls; On 2016/08/11 22:52:35, sclittle wrote: > nit: Can this just be an std::vector? Typically an std::vector is best unless > there's a good reason since it's simpler. Done. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:586: net::EscapeQueryParamValue(manifest.hostname, false), false)); On 2016/08/11 22:52:35, sclittle wrote: > Can't you just add all the query params when you create the manifest URL here? > Repeatedly copying the GURL is kinda expensive. I do not see any GURL constructor that takes query parameters. Do you know of an api. Or do you suggest to directly form the querystring and call ReplaceComponents(). https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... File components/precache/core/precache_fetcher.h (right): https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.h:10: #include <algorithm> On 2016/08/11 22:52:36, sclittle wrote: > Remove this include from the header file and put it in the .cc file. Done. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.h:50: ManifestHostInfo(const ManifestHostInfo& other); On 2016/08/11 22:52:35, sclittle wrote: > Is it possible to make this struct movable as well? Doesn't have to be too > complicated - just also add: > > ManifestHostInfo(ManifestHostInfo&&) = default; > ManifestHostInfo& operator=(ManifestHostInfo&&) = default; > > (You'll probably want to just have the declarations here in the .h and the > =default definitions in the .cc like you do here for the copy constructor.) > > That'll greatly reduce the number of string copies performed when dealing with > these objects, since it'll just be copying the |manifest_url| then and moving > everything else. Nice. Done. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.h:54: const int64_t manifest_id; On 2016/08/11 22:52:36, sclittle wrote: > Can you change these to not be const? You can just pass around a "const > ManifestHostInfo&" if you want them not to be modified. Done. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.h:187: // Retrieves the manifest info in DB thread. Manifest info for each of the On 2016/08/11 18:49:15, bengr wrote: > in -> on the Done. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.h:193: // Callback invoked when manifest info for all the top hosts is retrieved. On 2016/08/11 18:49:14, bengr wrote: > when -> when the Done. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.h:208: // PrecacheDatabase should be accessed in the DB thread. On 2016/08/11 18:49:14, bengr wrote: > in -> on Done. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.h:216: std::list<std::pair<GURL, std::string>> resources_to_fetch_; On 2016/08/11 22:52:36, sclittle wrote: > If you do chose to keep this member variable instead of just using the > |unfinished_work_| with a |resource_to_fetch_index_|, then please change this to > also be an std::deque instead of an std::list. Done. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... File components/precache/core/precache_fetcher_unittest.cc (right): https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher_unittest.cc:96: std::multiset<GURL>& requested_urls() { return requested_urls_; } On 2016/08/11 22:52:36, sclittle wrote: > You shouldn't return a non-const reference here. Use a setter instead if you > really need to change modify the value. Done. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher_unittest.cc:121: class MockURLFetcherFactory : public net::URLFetcherFactory { On 2016/08/11 22:52:36, sclittle wrote: > FYI, for future CLs: Wow, we should really not need to use gmock for these > tests. A lot of this test file baffles me - perhaps we should do a big cleanup > of the precache component and all the tests at some point, just to make it > easier to understand and develop faster. Acknowledged. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... File components/precache/core/precache_referrer_host_table.cc (right): https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_referrer_host_table.cc:27: PrecacheReferrerHostEntry PrecacheReferrerHostTable::GetReferrerHost( On 2016/08/11 18:49:15, bengr wrote: > Can this method be const? Actually const. But cannot be const. :) db_ is being accessed. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_referrer_host_table.cc:73: db_->GetCachedStatement(SQL_FROM_HERE, "DELETE FROM precache_urls")); On 2016/08/11 22:52:36, sclittle wrote: > Update this to say "precache_referrer_hosts". > > Could you please add tests for this file? Done. Adding tests. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... File components/precache/core/precache_referrer_host_table.h (right): https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_referrer_host_table.h:21: static const int64_t INVALID_ID; On 2016/08/11 22:52:36, sclittle wrote: > nit: Name this like a constant, e.g. kInvalidId Done. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_referrer_host_table.h:32: const int64_t id; On 2016/08/11 18:49:15, bengr wrote: > #include <stdint.h> Done. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_referrer_host_table.h:33: const GURL referrer_host; On 2016/08/11 22:52:36, sclittle wrote: > Change all these to be non-const. Just pass around a "const > PrecacheReferrerHostEntry&" if you want them to not be modified. Done. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... File components/precache/core/precache_url_table.h (right): https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_url_table.h:56: void GetURLListForReferrerHost(int64_t referrer_host_id, On 2016/08/11 18:49:15, bengr wrote: > #include <stdint.h> Done. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_url_table.h:57: std::deque<GURL>& used_urls, On 2016/08/11 22:52:36, sclittle wrote: > Don't pass by non-const ref, just pass a pointer instead. Done. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_url_table.h:58: std::deque<GURL>& unused_urls); On 2016/08/11 22:52:36, sclittle wrote: > Can these just be std::vector instead of std::deque? Done. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... File components/precache/core/proto/precache.proto (right): https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/proto/precache.proto:20: optional string tophostname = 2; On 2016/08/11 22:52:36, sclittle wrote: > nit: maybe change to top_host_name? Done. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/proto/precache.proto:24: // Time at the creation of this manifest. On 2016/08/11 22:52:36, sclittle wrote: > What determines this time? The server? If so, then timestamps aren't guaranteed > to be unique IDs, and this could cause weird bugs to happen. The manifest_id(timestamp) is just an opaque data for Chrome. It need not be unique compared to different manifests. We do not enforce uniqueness in the DB as well. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/proto/precache.proto:25: optional Timestamp timestamp = 1; On 2016/08/11 22:52:36, sclittle wrote: > Why is this specifically a timestamp? Could this just be some opaque ID set by > the server? I will try to change this manifest_id to a simple string or a number, so that timestamp confusion is avoided. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/proto/precache.proto:37: optional PrecacheManifestId id = 3; On 2016/08/11 18:49:15, bengr wrote: > Add a comment. Done. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... File components/precache/core/proto/unfinished_work.proto (right): https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/proto/unfinished_work.proto:27: // repeated PrecacheManifestURL manifest = 3; On 2016/08/11 18:49:15, bengr wrote: > On 2016/08/09 22:56:48, Raj wrote: > > Deprecating this field. Since pending manifests can be saved in top_host > itself. > > Typically, deprecation looks like this: > > // DEPRECATED: Manifest URLs remaining to be fetched. > repeated PrecacheManifestURL deprecated_manifest = 3 [deprecated=true] Done.
It's looking better, here's some more comments. https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... File components/precache/core/precache_fetcher.cc (right): https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:167: hashes.append(reinterpret_cast<const char*>(sha1_hash), 8); On 2016/08/12 19:04:21, Raj wrote: > On 2016/08/11 22:52:35, sclittle wrote: > > Replace "8" with "arraysize(sha1_hash)". > > hmm. sha1 is actually 20 bytes. But we are using only 8 bytes of it, as a > tradeoff between sending more bytes vs to reduce hash collision. Oh, OK, that makes sense. Could you explain that here in a comment then? https://codereview.chromium.org/2229983002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:586: net::EscapeQueryParamValue(manifest.hostname, false), false)); On 2016/08/12 19:04:20, Raj wrote: > On 2016/08/11 22:52:35, sclittle wrote: > > Can't you just add all the query params when you create the manifest URL here? > > Repeatedly copying the GURL is kinda expensive. > > I do not see any GURL constructor that takes query parameters. Do you know of an > api. > Or do you suggest to directly form the querystring and call ReplaceComponents(). Ok, this is probably fine then. I don't see a nicer way to do it than this, although this is kinda expensive. https://codereview.chromium.org/2229983002/diff/20001/components/precache/cor... File components/precache/core/precache_fetcher.cc (right): https://codereview.chromium.org/2229983002/diff/20001/components/precache/cor... components/precache/core/precache_fetcher.cc:163: const int hash_bytes_size = 8; nit: Change this to be a size_t instead of an int, since it represents some number of bytes in memory, and name this like a constant, e.g. kHashBytesSize. https://codereview.chromium.org/2229983002/diff/20001/components/precache/cor... components/precache/core/precache_fetcher.cc:524: std::unique_ptr<std::deque<ManifestHostInfo>> top_hosts_info( nit: I think you can still remove this std::unique_ptr and not need this variable at all if you use PostTaskAndReplyWithResult: https://cs.chromium.org/chromium/src/base/task_runner_util.h?sq=package:chromium If you change RetrieveManifestInfo to return the deque instead of setting it to a pointer, and you change OnManifestInfoRetrieved to take in just a "std::deque<ManifestHostInfo> manifests_info" instead of a unique_ptr, then you could do it like this: base::PostTaskAndReplyWithResult(db_task_runner_.get(), FROM_HERE, base::Bind(&PrecacheFetcher::RetrieveManifestInfo, AsWeakPtr(), std::move(top_hosts_to_fetch)), base::Bind(&PrecacheFetcher::OnManifestInfoRetrieved, AsWeakPtr()); https://codereview.chromium.org/2229983002/diff/20001/components/precache/cor... components/precache/core/precache_fetcher.cc:571: hosts_info->emplace_back(referrer_host_info.id, host, nit: use push_back instead of emplace_back, since that'll use the move constructor anyways, here and elsewhere. https://codereview.chromium.org/2229983002/diff/40001/components/precache/cor... File components/precache/core/precache_fetcher.cc (right): https://codereview.chromium.org/2229983002/diff/40001/components/precache/cor... components/precache/core/precache_fetcher.cc:163: const int hash_bytes_size = 8; nit: make hash_bytes_size a size_t. https://codereview.chromium.org/2229983002/diff/40001/components/precache/cor... components/precache/core/precache_fetcher.cc:493: int pending_top_hosts = nit: Change this to be a size_t. https://codereview.chromium.org/2229983002/diff/40001/components/precache/cor... File components/precache/core/precache_fetcher.h (right): https://codereview.chromium.org/2229983002/diff/40001/components/precache/cor... components/precache/core/precache_fetcher.h:130: // |precache_database| should be accessed on;y in |db_task_runner|. nit: fix comment, it says "on;y" https://codereview.chromium.org/2229983002/diff/40001/components/precache/cor... File components/precache/core/precache_fetcher_unittest.cc (right): https://codereview.chromium.org/2229983002/diff/40001/components/precache/cor... components/precache/core/precache_fetcher_unittest.cc:654: // initial_work->add_resource()->set_url(kGoodResourceURL); nit: remove dead code https://codereview.chromium.org/2229983002/diff/40001/components/precache/cor... components/precache/core/precache_fetcher_unittest.cc:663: // base::RunLoop().RunUntilIdle(); nit: remove dead code, here and elsewhere https://codereview.chromium.org/2229983002/diff/40001/components/precache/cor... components/precache/core/precache_fetcher_unittest.cc:1214: TEST_F(PrecacheFetcherTest, GetResourceURLBase64Hash) { nit: this should just be a TEST, not a TEST_F if it doesn't need to use any stuff from the PrecacheFetcherTest class. You could change it to TEST(PrecacheFetcherStandaloneTest, GetResourceURLBase64Hash) https://codereview.chromium.org/2229983002/diff/40001/components/precache/cor... File components/precache/core/precache_referrer_host_table.h (right): https://codereview.chromium.org/2229983002/diff/40001/components/precache/cor... components/precache/core/precache_referrer_host_table.h:35: bool operator==(const PrecacheReferrerHostEntry entry) const { Make this take in a const PrecacheReferrerHostEntry&, not just a const PrecacheReferrerHostEntry https://codereview.chromium.org/2229983002/diff/40001/components/precache/cor... components/precache/core/precache_referrer_host_table.h:36: return id == entry.id && referrer_host == entry.referrer_host && nit: Move this definition to the .cc file. https://codereview.chromium.org/2229983002/diff/40001/components/precache/cor... components/precache/core/precache_referrer_host_table.h:38: ; nit: remove this line
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/2229983002/diff/20001/components/precache/cor... File components/precache/core/precache_fetcher.cc (right): https://codereview.chromium.org/2229983002/diff/20001/components/precache/cor... components/precache/core/precache_fetcher.cc:163: const int hash_bytes_size = 8; On 2016/08/15 20:13:09, sclittle wrote: > nit: Change this to be a size_t instead of an int, since it represents some > number of bytes in memory, and name this like a constant, e.g. kHashBytesSize. Done. https://codereview.chromium.org/2229983002/diff/20001/components/precache/cor... components/precache/core/precache_fetcher.cc:524: std::unique_ptr<std::deque<ManifestHostInfo>> top_hosts_info( On 2016/08/15 20:13:09, sclittle wrote: > nit: I think you can still remove this std::unique_ptr and not need this > variable at all if you use PostTaskAndReplyWithResult: > https://cs.chromium.org/chromium/src/base/task_runner_util.h?sq=package:chromium > > If you change RetrieveManifestInfo to return the deque instead of setting it to > a pointer, and you change OnManifestInfoRetrieved to take in just a > "std::deque<ManifestHostInfo> manifests_info" instead of a unique_ptr, then you > could do it like this: > > base::PostTaskAndReplyWithResult(db_task_runner_.get(), FROM_HERE, > base::Bind(&PrecacheFetcher::RetrieveManifestInfo, AsWeakPtr(), > std::move(top_hosts_to_fetch)), > base::Bind(&PrecacheFetcher::OnManifestInfoRetrieved, AsWeakPtr()); Tried this earlier. PostTaskAndReplyWithResult() does not take weak_ptr, and fails with compile-time assert. ../../base/bind_internal.h:293:3: error: static assertion failed: weak_ptrs can only bind to methods without return values https://cs.chromium.org/chromium/src/base/bind_internal.h?rcl=0&l=293 Also, note my current code has base::Unretained(this) for the reply callback. base::Bind(&PrecacheFetcher::OnManifestInfoRetrieved, base::Unretained(this), ...) Using AsWeakPtr() did not work, since the weakptr will be checked in DB thread (while the weakptr was created in UI thread). Following is the runtime DCHECK hit. 08-16 10:30:22.624 29153-29153 A: Abort message: '[FATAL:weak_ptr.cc(26)] Check failed: sequence_checker_.CalledOnValidSequence(). WeakPtrs must be checked on the same sequenced thread. https://codereview.chromium.org/2229983002/diff/20001/components/precache/cor... components/precache/core/precache_fetcher.cc:571: hosts_info->emplace_back(referrer_host_info.id, host, On 2016/08/15 20:13:09, sclittle wrote: > nit: use push_back instead of emplace_back, since that'll use the move > constructor anyways, here and elsewhere. I have changed it. But emplace_back seems to be better though. It explicitly indicates there is less copy (even if move is not supported). It is less code. hosts_info->emplace_back(...) hosts_info->push_back(ManifestHostInfo(...)) https://codereview.chromium.org/2229983002/diff/40001/components/precache/cor... File components/precache/core/precache_fetcher.cc (right): https://codereview.chromium.org/2229983002/diff/40001/components/precache/cor... components/precache/core/precache_fetcher.cc:163: const int hash_bytes_size = 8; On 2016/08/15 20:13:10, sclittle wrote: > nit: make hash_bytes_size a size_t. Done. https://codereview.chromium.org/2229983002/diff/40001/components/precache/cor... components/precache/core/precache_fetcher.cc:493: int pending_top_hosts = On 2016/08/15 20:13:10, sclittle wrote: > nit: Change this to be a size_t. Done. https://codereview.chromium.org/2229983002/diff/40001/components/precache/cor... File components/precache/core/precache_fetcher.h (right): https://codereview.chromium.org/2229983002/diff/40001/components/precache/cor... components/precache/core/precache_fetcher.h:130: // |precache_database| should be accessed on;y in |db_task_runner|. On 2016/08/15 20:13:10, sclittle wrote: > nit: fix comment, it says "on;y" Done. https://codereview.chromium.org/2229983002/diff/40001/components/precache/cor... File components/precache/core/precache_fetcher_unittest.cc (right): https://codereview.chromium.org/2229983002/diff/40001/components/precache/cor... components/precache/core/precache_fetcher_unittest.cc:654: // initial_work->add_resource()->set_url(kGoodResourceURL); On 2016/08/15 20:13:10, sclittle wrote: > nit: remove dead code Done. https://codereview.chromium.org/2229983002/diff/40001/components/precache/cor... components/precache/core/precache_fetcher_unittest.cc:663: // base::RunLoop().RunUntilIdle(); On 2016/08/15 20:13:10, sclittle wrote: > nit: remove dead code, here and elsewhere Done. https://codereview.chromium.org/2229983002/diff/40001/components/precache/cor... components/precache/core/precache_fetcher_unittest.cc:1214: TEST_F(PrecacheFetcherTest, GetResourceURLBase64Hash) { On 2016/08/15 20:13:10, sclittle wrote: > nit: this should just be a TEST, not a TEST_F if it doesn't need to use any > stuff from the PrecacheFetcherTest class. > > You could change it to TEST(PrecacheFetcherStandaloneTest, > GetResourceURLBase64Hash) Done. https://codereview.chromium.org/2229983002/diff/40001/components/precache/cor... File components/precache/core/precache_referrer_host_table.h (right): https://codereview.chromium.org/2229983002/diff/40001/components/precache/cor... components/precache/core/precache_referrer_host_table.h:35: bool operator==(const PrecacheReferrerHostEntry entry) const { On 2016/08/15 20:13:10, sclittle wrote: > Make this take in a const PrecacheReferrerHostEntry&, not just a const > PrecacheReferrerHostEntry Done. https://codereview.chromium.org/2229983002/diff/40001/components/precache/cor... components/precache/core/precache_referrer_host_table.h:36: return id == entry.id && referrer_host == entry.referrer_host && On 2016/08/15 20:13:10, sclittle wrote: > nit: Move this definition to the .cc file. Done. https://codereview.chromium.org/2229983002/diff/40001/components/precache/cor... components/precache/core/precache_referrer_host_table.h:38: ; On 2016/08/15 20:13:10, sclittle wrote: > nit: remove this line Done.
https://codereview.chromium.org/2229983002/diff/20001/components/precache/cor... File components/precache/core/precache_fetcher.cc (right): https://codereview.chromium.org/2229983002/diff/20001/components/precache/cor... components/precache/core/precache_fetcher.cc:523: std::deque<std::string> top_hosts_to_fetch; nit: Why is this a std::deque? Could this just be an std::vector? https://codereview.chromium.org/2229983002/diff/20001/components/precache/cor... components/precache/core/precache_fetcher.cc:524: std::unique_ptr<std::deque<ManifestHostInfo>> top_hosts_info( On 2016/08/16 17:54:37, Raj wrote: > On 2016/08/15 20:13:09, sclittle wrote: > > nit: I think you can still remove this std::unique_ptr and not need this > > variable at all if you use PostTaskAndReplyWithResult: > > > https://cs.chromium.org/chromium/src/base/task_runner_util.h?sq=package:chromium > > > > If you change RetrieveManifestInfo to return the deque instead of setting it > to > > a pointer, and you change OnManifestInfoRetrieved to take in just a > > "std::deque<ManifestHostInfo> manifests_info" instead of a unique_ptr, then > you > > could do it like this: > > > > base::PostTaskAndReplyWithResult(db_task_runner_.get(), FROM_HERE, > > base::Bind(&PrecacheFetcher::RetrieveManifestInfo, AsWeakPtr(), > > std::move(top_hosts_to_fetch)), > > base::Bind(&PrecacheFetcher::OnManifestInfoRetrieved, AsWeakPtr()); > > Tried this earlier. PostTaskAndReplyWithResult() does not take weak_ptr, and > fails with compile-time assert. > ../../base/bind_internal.h:293:3: error: static assertion failed: weak_ptrs can > only bind to methods without return values > > https://cs.chromium.org/chromium/src/base/bind_internal.h?rcl=0&l=293 > > > Also, note my current code has base::Unretained(this) for the reply callback. > base::Bind(&PrecacheFetcher::OnManifestInfoRetrieved, base::Unretained(this), > ...) > Using AsWeakPtr() did not work, since the weakptr will be checked in DB thread > (while the weakptr was created in UI thread). > Following is the runtime DCHECK hit. > 08-16 10:30:22.624 29153-29153 A: Abort message: '[FATAL:weak_ptr.cc(26)] Check > failed: sequence_checker_.CalledOnValidSequence(). WeakPtrs must be checked on > the same sequenced thread. Darn, good catch though. https://codereview.chromium.org/2229983002/diff/20001/components/precache/cor... components/precache/core/precache_fetcher.cc:559: base::Unretained(this), base::Passed(&top_hosts_info))); using base::Unretained(this) here doesn't seem safe, could you make this use AsWeakPtr instead? https://codereview.chromium.org/2229983002/diff/20001/components/precache/cor... components/precache/core/precache_fetcher.cc:562: void PrecacheFetcher::RetrieveManifestInfo( Maybe move this method to the PrecacheDatabase instead of the PrecacheFetcher? The PrecacheDatabase lives on the DB thread, so the lifetimes should work out OK. https://codereview.chromium.org/2229983002/diff/20001/components/precache/cor... components/precache/core/precache_fetcher.cc:571: hosts_info->emplace_back(referrer_host_info.id, host, On 2016/08/16 17:54:37, Raj wrote: > On 2016/08/15 20:13:09, sclittle wrote: > > nit: use push_back instead of emplace_back, since that'll use the move > > constructor anyways, here and elsewhere. > > I have changed it. > But emplace_back seems to be better though. It explicitly indicates there is > less copy (even if move is not supported). > > It is less code. > hosts_info->emplace_back(...) > hosts_info->push_back(ManifestHostInfo(...)) Yep, there's slightly less copy, but the extra copy in this case is just a few pointers since it's done as a move operation (and the GURL in the struct is empty and thus is cheap to copy), so it's not really a big deal. Whichever one you prefer more is fine, but I'd lean toward using push_back unless there's a good reason to use emplace_back. https://codereview.chromium.org/2229983002/diff/20001/components/precache/cor... components/precache/core/precache_fetcher.cc:587: top_hosts_to_fetch_.reset(new std::deque<ManifestHostInfo>()); Could you just move the existing deque instead of constructing a whole new one here? e.g. top_hosts_to_fetch_ = std::move(manifests_info); Then, you could use std::remove_if to remove the invalid IDs from the deque in O(n) time without needing to copy the whole deque, and before doing any work to construct the manifest_urls, e.g.: top_hosts_to_fetch_->erase( std::remove_if(top_hosts_to_fetch_->begin(), top_hosts_to_fetch_->end(), [](const ManifestHostInfo& manifest) { return manifest.manifest_id == PrecacheReferrerHostEntry::kInvalidId; }), top_hosts_to_fetch_->end()); https://codereview.chromium.org/2229983002/diff/20001/components/precache/cor... components/precache/core/precache_fetcher.cc:593: if (manifest_url.is_valid() && It seems like the manifest_url could only be invalid if the |prefix| is an invalid URL. You should still make sure that the |prefix| is a valid URL, since it could be set by Finch and that shouldn't cause Chrome to crash/perform weirdly if it's set wrong accidentally. You could remove the is_valid() condition here and just have an escape hatch above before the for loop, e.g.: if (!GURL(prefix).is_valid()) { // Don't attempt to fetch any manifests if the manifest URL prefix // is invalid. top_hosts_to_fetch_.clear(); return; } https://codereview.chromium.org/2229983002/diff/20001/components/precache/cor... File components/precache/core/precache_fetcher.h (right): https://codereview.chromium.org/2229983002/diff/20001/components/precache/cor... components/precache/core/precache_fetcher.h:47: const std::string& used_url_hash, nit: since these hash strings can get pretty long, you could change all these strings in the constructor to take in just "std::string"s instead of "const std::string&"s then move them into the members. https://codereview.chromium.org/2229983002/diff/20001/components/precache/cor... components/precache/core/precache_fetcher.h:214: std::unique_ptr<std::deque<ManifestHostInfo>> top_hosts_to_fetch_; nit: could you remove the std::unique_ptr here and just std::move the deque into this? https://codereview.chromium.org/2229983002/diff/80001/components/precache/con... File components/precache/content/precache_manager_unittest.cc (right): https://codereview.chromium.org/2229983002/diff/80001/components/precache/con... components/precache/content/precache_manager_unittest.cc:206: precache_manager_->precache_database_->RecordURLPrefetch( You don't need to access the PrecacheDatabase this way - you could just use the existing PrecacheDatabase* precache_database_ member of this test file. You could also remove this test from being a friend of PrecacheManager that way. https://codereview.chromium.org/2229983002/diff/80001/components/precache/cor... File components/precache/core/proto/precache.proto (right): https://codereview.chromium.org/2229983002/diff/80001/components/precache/cor... components/precache/core/proto/precache.proto:22: // Time at the creation of this manifest. nit: Remove this comment - ideally, this ID should be completely opaque, and it shouldn't matter whether or not it's the time at the creation of this manifest. https://codereview.chromium.org/2229983002/diff/80001/components/precache/cor... File components/precache/core/proto/unfinished_work.proto (right): https://codereview.chromium.org/2229983002/diff/80001/components/precache/cor... components/precache/core/proto/unfinished_work.proto:31: // DEPRECATED: Manifest URLs remaining to be fetched. nit: While this is the normal way to deprecate things in a proto, I'd recommend removing it completely, or commenting out these lines entirely. That way, the compiler won't generate getters and setters for it, leading to a smaller binary size. Same thing with the DeprecatedPrecacheManifestURL message above.
ptal https://codereview.chromium.org/2229983002/diff/20001/components/precache/cor... File components/precache/core/precache_fetcher.cc (right): https://codereview.chromium.org/2229983002/diff/20001/components/precache/cor... components/precache/core/precache_fetcher.cc:523: std::deque<std::string> top_hosts_to_fetch; On 2016/08/16 21:05:05, sclittle wrote: > nit: Why is this a std::deque? Could this just be an std::vector? Done. https://codereview.chromium.org/2229983002/diff/20001/components/precache/cor... components/precache/core/precache_fetcher.cc:559: base::Unretained(this), base::Passed(&top_hosts_info))); On 2016/08/16 21:05:05, sclittle wrote: > using base::Unretained(this) here doesn't seem safe, could you make this use > AsWeakPtr instead? Done. https://codereview.chromium.org/2229983002/diff/20001/components/precache/cor... components/precache/core/precache_fetcher.cc:562: void PrecacheFetcher::RetrieveManifestInfo( On 2016/08/16 21:05:05, sclittle wrote: > Maybe move this method to the PrecacheDatabase instead of the PrecacheFetcher? > The PrecacheDatabase lives on the DB thread, so the lifetimes should work out > OK. Moved it to an anonymous function. Moving to PrecacheDatabase means, this function will return vector<PrecacheReferrerHostInfo> where each PrecacheReferrerHostInfo contains two vectors used_urls, unused_urls. Another option is to return used_url_hash, unused_url_hash, which means moving the hash computation logic to databse, which is not appropriate. https://codereview.chromium.org/2229983002/diff/20001/components/precache/cor... components/precache/core/precache_fetcher.cc:587: top_hosts_to_fetch_.reset(new std::deque<ManifestHostInfo>()); On 2016/08/16 21:05:05, sclittle wrote: > Could you just move the existing deque instead of constructing a whole new one > here? e.g. top_hosts_to_fetch_ = std::move(manifests_info); > > Then, you could use std::remove_if to remove the invalid IDs from the deque in > O(n) time without needing to copy the whole deque, and before doing any work to > construct the manifest_urls, e.g.: > > top_hosts_to_fetch_->erase( > std::remove_if(top_hosts_to_fetch_->begin(), > top_hosts_to_fetch_->end(), > [](const ManifestHostInfo& manifest) { > return manifest.manifest_id == > PrecacheReferrerHostEntry::kInvalidId; > }), > top_hosts_to_fetch_->end()); Done. https://codereview.chromium.org/2229983002/diff/20001/components/precache/cor... components/precache/core/precache_fetcher.cc:593: if (manifest_url.is_valid() && On 2016/08/16 21:05:05, sclittle wrote: > It seems like the manifest_url could only be invalid if the |prefix| is an > invalid URL. > > You should still make sure that the |prefix| is a valid URL, since it could be > set by Finch and that shouldn't cause Chrome to crash/perform weirdly if it's > set wrong accidentally. You could remove the is_valid() condition here and just > have an escape hatch above before the for loop, e.g.: > > if (!GURL(prefix).is_valid()) { > // Don't attempt to fetch any manifests if the manifest URL prefix > // is invalid. > top_hosts_to_fetch_.clear(); > return; > } Done. https://codereview.chromium.org/2229983002/diff/20001/components/precache/cor... File components/precache/core/precache_fetcher.h (right): https://codereview.chromium.org/2229983002/diff/20001/components/precache/cor... components/precache/core/precache_fetcher.h:47: const std::string& used_url_hash, On 2016/08/16 21:05:05, sclittle wrote: > nit: since these hash strings can get pretty long, you could change all these > strings in the constructor to take in just "std::string"s instead of "const > std::string&"s then move them into the members. hmm. I don't see any benefit. The strings will get copied to the constructor arguments due to "std::string". Without any improvement, using move() seems unnecessary. https://codereview.chromium.org/2229983002/diff/20001/components/precache/cor... components/precache/core/precache_fetcher.h:214: std::unique_ptr<std::deque<ManifestHostInfo>> top_hosts_to_fetch_; On 2016/08/16 21:05:05, sclittle wrote: > nit: could you remove the std::unique_ptr here and just std::move the deque into > this? Done. https://codereview.chromium.org/2229983002/diff/80001/components/precache/con... File components/precache/content/precache_manager_unittest.cc (right): https://codereview.chromium.org/2229983002/diff/80001/components/precache/con... components/precache/content/precache_manager_unittest.cc:206: precache_manager_->precache_database_->RecordURLPrefetch( On 2016/08/16 21:05:05, sclittle wrote: > You don't need to access the PrecacheDatabase this way - you could just use the > existing PrecacheDatabase* precache_database_ member of this test file. > > You could also remove this test from being a friend of PrecacheManager that way. Done. Still cannot remove this test from friend of PrecacheManager. https://codereview.chromium.org/2229983002/diff/80001/components/precache/cor... File components/precache/core/proto/precache.proto (right): https://codereview.chromium.org/2229983002/diff/80001/components/precache/cor... components/precache/core/proto/precache.proto:22: // Time at the creation of this manifest. On 2016/08/16 21:05:06, sclittle wrote: > nit: Remove this comment - ideally, this ID should be completely opaque, and it > shouldn't matter whether or not it's the time at the creation of this manifest. Yes. I missed removing it. https://codereview.chromium.org/2229983002/diff/80001/components/precache/cor... File components/precache/core/proto/unfinished_work.proto (right): https://codereview.chromium.org/2229983002/diff/80001/components/precache/cor... components/precache/core/proto/unfinished_work.proto:31: // DEPRECATED: Manifest URLs remaining to be fetched. On 2016/08/16 21:05:06, sclittle wrote: > nit: While this is the normal way to deprecate things in a proto, I'd recommend > removing it completely, or commenting out these lines entirely. That way, the > compiler won't generate getters and setters for it, leading to a smaller binary > size. > > Same thing with the DeprecatedPrecacheManifestURL message above. Yes. Makes sense.
lgtm % nit. https://codereview.chromium.org/2229983002/diff/20001/components/precache/cor... File components/precache/core/precache_fetcher.cc (right): https://codereview.chromium.org/2229983002/diff/20001/components/precache/cor... components/precache/core/precache_fetcher.cc:562: void PrecacheFetcher::RetrieveManifestInfo( On 2016/08/18 00:23:23, Raj wrote: > On 2016/08/16 21:05:05, sclittle wrote: > > Maybe move this method to the PrecacheDatabase instead of the PrecacheFetcher? > > The PrecacheDatabase lives on the DB thread, so the lifetimes should work out > > OK. > > Moved it to an anonymous function. > > Moving to PrecacheDatabase means, this function will return > vector<PrecacheReferrerHostInfo> where each PrecacheReferrerHostInfo contains > two vectors used_urls, unused_urls. > > Another option is to return used_url_hash, unused_url_hash, which means moving > the hash computation logic to databse, which is not appropriate. Ok, sounds good. Thanks for investigating this. https://codereview.chromium.org/2229983002/diff/100001/components/precache/co... File components/precache/core/precache_fetcher.cc (right): https://codereview.chromium.org/2229983002/diff/100001/components/precache/co... components/precache/core/precache_fetcher.cc:204: return std::move(hosts_info); nit: remove the std::move here. You don't have to call std::move when returning a local variable from a function - the compiler will do that automatically.
The CQ bit was checked by rajendrant@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-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #6 (id:120001) has been deleted
The CQ bit was checked by rajendrant@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #6 (id:140001) has been deleted
The CQ bit was checked by rajendrant@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_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #6 (id:160001) has been deleted
The CQ bit was checked by rajendrant@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #6 (id:180001) has been deleted
The CQ bit was checked by rajendrant@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Patchset #6 (id:200001) has been deleted
The CQ bit was checked by rajendrant@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.
The CQ bit was checked by rajendrant@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sclittle@chromium.org Link to the patchset: https://codereview.chromium.org/2229983002/#ps220001 (title: "Addressed nits")
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:220001)
Message was sent while issue was closed.
Description was changed from ========== Send the list of used and unused resources for precache When precache manifest is fetched, the list of used and unused resources corresponding to the top host are sent as request parameters in the manifest URL. The resource URLs are hashed individually and sent as base64 encoded string. BUG=626463 ========== to ========== Send the list of used and unused resources for precache When precache manifest is fetched, the list of used and unused resources corresponding to the top host are sent as request parameters in the manifest URL. The resource URLs are hashed individually and sent as base64 encoded string. BUG=626463 Committed: https://crrev.com/4284452ea2f25f75aa1cb6e2386850fedd81ffe8 Cr-Commit-Position: refs/heads/master@{#414721} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/4284452ea2f25f75aa1cb6e2386850fedd81ffe8 Cr-Commit-Position: refs/heads/master@{#414721} |
