Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1710923003:
Implements cachetool to manipulate simple typed cache. (Closed)
DescriptionImplements cachetool to manipulate simple typed cache.
BUG=582080
Committed: https://crrev.com/ebc4faedfca14bab8f932d03e1332b1b72e8430e
Cr-Commit-Position: refs/heads/master@{#377007}
Patch Set 1 #
Total comments: 42
Patch Set 2 : Addresses Gavin's comment #
Total comments: 7
Patch Set 3 : Changes PrintHelp's return type to void #
Total comments: 10
Patch Set 4 : Addresses nits #
Depends on Patchset: Messages
Total messages: 18 (6 generated)
gabadie@chromium.org changed reviewers: + gavinp@chromium.org
gavinp@chromium.org: Thanks for the help in make this tool. Here I have all the features I need (for now). The blockfile cache may come after if needed. PTAL. =)
Description was changed from ========== Implements cachetool to manipulate simple tpyed cache. BUG=582080 ========== to ========== Implements cachetool to manipulate simple typed cache. BUG=582080 ==========
Looks pretty good (despite the large number of nitpicky comments). I think the description could be improved. How about: "Introducing cachetool: A cache printing and enumeration tool." ? https://codereview.chromium.org/1710923003/diff/1/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/1710923003/diff/1/net/net.gyp#newcode808 net/net.gyp:808: 'target_name': 'cachetool', I believe this goes above "dump_cache" because we sort these sections lexicographically by name. Although clearly there's no existing sort, so I guess you can ignore this. https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... File net/tools/cachetool/cachetool.cc (right): https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:21: using namespace disk_cache; See https://google.github.io/styleguide/cppguide.html#Namespaces , which says, in part: "You may not use a using-directive to make all names from a namespace available." Enumerate the names you'd like to use, or qualify each use. Sorry. https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:27: // Create a simple typed cache back-end. grammar: "Create a SimpleCache backend." But at that point, it recapitulates the function name, so might as well delete the comment. https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:28: Backend* CreateSimpleBackend(const base::FilePath& cache_path) { This should return scoped_ptr<Backend> or scoped_ptr<SimpleBackendImpl>. I suggest the former. Bonus points: figure out if the code Egor wrote in net/disk_cache/cache_creator.cc is worth using? I think not, Egor probably would agree. If he disagrees, he's probably the one who's right. https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:34: DCHECK_EQ(net::OK, cb.GetResult(rv)); This somewhat defeats the purpose of your later check on the return value of this method. Wouldn't: if (cb.GetResult(rv) != net::OK) return nullptr; be more in keeping with the apparent goal of returning with a useful error message when a non-simple-cache directory is given? https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:39: // Prints all cache's keys to the stdout. "Print call of a cache's keys to stdout." Explanation: "cache" requires an indefinite article here in English. As well, stdout does not require, and is incorrect if you use a definite article here. Compare "France is a European country" and "La France est un État de l'Europe", which contains two good examples of definite articles disappearing in translation to English. You may be happy to learn that I am constantly failing to add articles when I attempt to write or speak la française. https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:40: int ListKeys(Backend* cache_backend) { I think "bool" is a better return code for ListKeys and the other printers; we aren't particularly propogating interesting information about our error codes (this function has NO POSSIBLE ERRORS, the other printer only ever returns 0 or 1), and so I don't see a justification for inverting our sense of right and wrong. https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:56: // Prints a key's stream to the stdout. "Print a key's stream to stdout." https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:57: int GetKeyStream(Backend* cache_backend, const std::string& key, int index) { I'm not a huge fan of this API; the magic index values of 0 and 1 are bad. Could we make |index| an enum class argument perhaps? Frustrating that we'll be recapitulating the constant from the HttpCache, but I think it improves readability. https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:58: if (index < 0 || index > 2) { "2" is really problematic here. Why is that the maximum? An enumeration helps here again. https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:67: std::cerr << "Couldn't find key." << std::endl; Isn't it the entry that we couldn't find? https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:103: fwrite(buffer->StartOfBuffer(), 1, buffer->offset(), stdout); Intermixing fwrite and std::cout is dangerous. I believe this use is safe in the code paths I've looked at, but why not just use std::cout.write(buffer->StartOfBuffer(), buffer->offset()); and then it's far easier to trust it's working? https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:119: // Prints the command line's help. "Print the command line help." https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:120: int PrintHelp() { This shouldn't return a value. https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:121: std::cout << "cachetool <cache_path> <cache_backend_type> <subcommand> ..." Document available cache backend types. https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:125: std::cout << " validate: Open the cache and return." << std::endl; "Open a cache and return, confirming the cache exists and is of the right type" (might even be useful to clarify that this in no way validates the structure of the entire cache, just that it can be opened). https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:126: std::cout << " list_keys: List all cache's keys." << std::endl; "List all keys in a cache." https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:127: std::cout << " get_stream <key> <index>: Get key's stream." << std::endl; "Print a particular stream for a given key" Also, can we use <index> symbolically here, and then document those values, i.e. "headers" and "body" ? https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:165: if (subcommand == "validate") { Why don't we check that there's only exactly three arguments for validate and list_keys? https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:174: int index = std::stoi(args[4]); Earlier I mentioned that I'd like these to be symbolic. But also, shouldn't we consider base::StringToInt if we are going to be parsing strings into integers? At the very least, we should fail on errors.
New revision uploaded. PTAL =) https://codereview.chromium.org/1710923003/diff/1/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/1710923003/diff/1/net/net.gyp#newcode808 net/net.gyp:808: 'target_name': 'cachetool', On 2016/02/19 16:21:11, gavinp wrote: > I believe this goes above "dump_cache" because we sort these sections > lexicographically by name. Although clearly there's no existing sort, so I guess > you can ignore this. Done. https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... File net/tools/cachetool/cachetool.cc (right): https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:21: using namespace disk_cache; On 2016/02/19 16:21:11, gavinp wrote: > See https://google.github.io/styleguide/cppguide.html#Namespaces , which says, > in part: > > "You may not use a using-directive to make all names from a namespace > available." > > Enumerate the names you'd like to use, or qualify each use. Sorry. Done. https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:27: // Create a simple typed cache back-end. On 2016/02/19 16:21:11, gavinp wrote: > grammar: "Create a SimpleCache backend." > > But at that point, it recapitulates the function name, so might as well delete > the comment. Done. https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:28: Backend* CreateSimpleBackend(const base::FilePath& cache_path) { On 2016/02/19 16:21:11, gavinp wrote: > This should return scoped_ptr<Backend> or scoped_ptr<SimpleBackendImpl>. I > suggest the former. > > Bonus points: figure out if the code Egor wrote in > net/disk_cache/cache_creator.cc is worth using? I think not, Egor probably would > agree. If he disagrees, he's probably the one who's right. Egor not here yet. We can universalize this tool latter on. https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:34: DCHECK_EQ(net::OK, cb.GetResult(rv)); On 2016/02/19 16:21:11, gavinp wrote: > This somewhat defeats the purpose of your later check on the return value of > this method. Wouldn't: > > if (cb.GetResult(rv) != net::OK) > return nullptr; > > be more in keeping with the apparent goal of returning with a useful error > message when a non-simple-cache directory is given? Oups... Done. https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:39: // Prints all cache's keys to the stdout. On 2016/02/19 16:21:12, gavinp wrote: > "Print call of a cache's keys to stdout." > > Explanation: "cache" requires an indefinite article here in English. > > As well, stdout does not require, and is incorrect if you use a definite article > here. Compare "France is a European country" and "La France est un État de > l'Europe", which contains two good examples of definite articles disappearing in > translation to English. You may be happy to learn that I am constantly failing > to add articles when I attempt to write or speak la française. Ahah ^^ Thanks for that outstanding example! =) Done. https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:40: int ListKeys(Backend* cache_backend) { On 2016/02/19 16:21:11, gavinp wrote: > I think "bool" is a better return code for ListKeys and the other printers; we > aren't particularly propogating interesting information about our error codes > (this function has NO POSSIBLE ERRORS, the other printer only ever returns 0 or > 1), and so I don't see a justification for inverting our sense of right and > wrong. Done. https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:56: // Prints a key's stream to the stdout. On 2016/02/19 16:21:11, gavinp wrote: > "Print a key's stream to stdout." Done. https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:57: int GetKeyStream(Backend* cache_backend, const std::string& key, int index) { On 2016/02/19 16:21:11, gavinp wrote: > I'm not a huge fan of this API; the magic index values of 0 and 1 are bad. Could > we make |index| an enum class argument perhaps? Frustrating that we'll be > recapitulating the constant from the HttpCache, but I think it improves > readability. I totally agree this issue makes the code not pretty. But to me, adding enum class only in this non production code peace would not be the proper thing to do. The correct fix would be a tiny refactor to change the API to use the enum class instead. Sadly, this is not the point of this change. This command line tool is more a offline plumbing work, than maintained code for user experience. For more context, this tool is meant to be used for other plumbing work (hack?) to make the NoState-Prefetch experimental benchmark working. Therefore I would prefer to make this tool as thin as possible: not adding any candy abstractions in mapping enums <-> index and doing other stuff such as passing string to the command line instead of an integer. https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:58: if (index < 0 || index > 2) { On 2016/02/19 16:21:11, gavinp wrote: > "2" is really problematic here. Why is that the maximum? An enumeration helps > here again. Stuffing with comment. https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:67: std::cerr << "Couldn't find key." << std::endl; On 2016/02/19 16:21:12, gavinp wrote: > Isn't it the entry that we couldn't find? Done. https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:103: fwrite(buffer->StartOfBuffer(), 1, buffer->offset(), stdout); On 2016/02/19 16:21:11, gavinp wrote: > Intermixing fwrite and std::cout is dangerous. I believe this use is safe in the > code paths I've looked at, but why not just use > > std::cout.write(buffer->StartOfBuffer(), buffer->offset()); > > and then it's far easier to trust it's working? My bad, have always been used to use C entry point when dealing with binary blobs. Fixed. https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:119: // Prints the command line's help. On 2016/02/19 16:21:11, gavinp wrote: > "Print the command line help." Done. https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:120: int PrintHelp() { On 2016/02/19 16:21:11, gavinp wrote: > This shouldn't return a value. It returns 1 to have handy oneliners return PrintHelp() in main() https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:121: std::cout << "cachetool <cache_path> <cache_backend_type> <subcommand> ..." On 2016/02/19 16:21:11, gavinp wrote: > Document available cache backend types. Done. https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:125: std::cout << " validate: Open the cache and return." << std::endl; On 2016/02/19 16:21:11, gavinp wrote: > "Open a cache and return, confirming the cache exists and is of the right type" > > (might even be useful to clarify that this in no way validates the structure of > the entire cache, just that it can be opened). Done. https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:126: std::cout << " list_keys: List all cache's keys." << std::endl; On 2016/02/19 16:21:11, gavinp wrote: > "List all keys in a cache." Done. https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:127: std::cout << " get_stream <key> <index>: Get key's stream." << std::endl; On 2016/02/19 16:21:11, gavinp wrote: > "Print a particular stream for a given key" > > Also, can we use <index> symbolically here, and then document those values, i.e. > "headers" and "body" ? Done. https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:165: if (subcommand == "validate") { On 2016/02/19 16:21:11, gavinp wrote: > Why don't we check that there's only exactly three arguments for validate and > list_keys? Oups... Done. https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:174: int index = std::stoi(args[4]); On 2016/02/19 16:21:11, gavinp wrote: > Earlier I mentioned that I'd like these to be symbolic. But also, shouldn't we > consider base::StringToInt if we are going to be parsing strings into integers? > At the very least, we should fail on errors. Ah sweet, I was looking into a better function in the std for detectings errors. Thanks!
looking quite good. looking forward to chatting about this later today. https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... File net/tools/cachetool/cachetool.cc (right): https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:57: int GetKeyStream(Backend* cache_backend, const std::string& key, int index) { On 2016/02/22 14:35:45, gabadie wrote: > On 2016/02/19 16:21:11, gavinp wrote: > > I'm not a huge fan of this API; the magic index values of 0 and 1 are bad. > Could > > we make |index| an enum class argument perhaps? Frustrating that we'll be > > recapitulating the constant from the HttpCache, but I think it improves > > readability. > > I totally agree this issue makes the code not pretty. > > But to me, adding enum class only in this non production code peace would not be > the proper thing to do. The correct fix would be a tiny refactor to change the > API to use the enum class instead. Sadly, this is not the point of this change. > > This command line tool is more a offline plumbing work, than maintained code for > user experience. For more context, this tool is meant to be used for other > plumbing work (hack?) to make the NoState-Prefetch experimental benchmark > working. Therefore I would prefer to make this tool as thin as possible: not > adding any candy abstractions in mapping enums <-> index and doing other stuff > such as passing string to the command line instead of an integer. Fair point. And you're placing the blame where it belongs. https://codereview.chromium.org/1710923003/diff/1/net/tools/cachetool/cacheto... net/tools/cachetool/cachetool.cc:120: int PrintHelp() { On 2016/02/22 14:35:46, gabadie wrote: > On 2016/02/19 16:21:11, gavinp wrote: > > This shouldn't return a value. > > It returns 1 to have handy oneliners return PrintHelp() in main() That's too tricky, too ugly. I wouldn't approve "return PrintHelp(), 1;" in a review, and I definitely won't approve having PrintHelp unconditionally return a constant in order to make code lower down be cute. Sorry. https://codereview.chromium.org/1710923003/diff/20001/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/1710923003/diff/20001/net/net.gyp#newcode533 net/net.gyp:533: 'cert/mock_client_cert_verifier.h', Not really a problem here, but it can be a good idea to upload rebase/merges separately, so reviewers don't confuse outside updates with your code. https://codereview.chromium.org/1710923003/diff/20001/net/tools/cachetool/cac... File net/tools/cachetool/cachetool.cc (right): https://codereview.chromium.org/1710923003/diff/20001/net/tools/cachetool/cac... net/tools/cachetool/cachetool.cc:42: // Print call of a cache's keys to stdout. Typo: call --> all https://codereview.chromium.org/1710923003/diff/20001/net/tools/cachetool/cac... net/tools/cachetool/cachetool.cc:137: std::cout << " get_stream <key> <index>: Print a particular stream for a" How about we document the various crazy values of index here, and remove the earlier comment? It's a single point of documentation, and it's more useful for being in the help output. https://codereview.chromium.org/1710923003/diff/20001/net/tools/cachetool/cac... net/tools/cachetool/cachetool.cc:177: if (args.size() != 3) { Don't need braces on a single line if. There's a bunch of these in here. The formal rule is at https://google.github.io/styleguide/cppguide.html#Conditionals , and it says "In general, curly braces are not required for single-line statements, but they are allowed if you like them; conditional or loop statements with complex conditions or statements may be more readable with curly braces." However, practice in Chrome has been to not have them. You're welcome to them if you're unduly attached to them though.
New revision uploaded. PTAL =) https://codereview.chromium.org/1710923003/diff/20001/net/tools/cachetool/cac... File net/tools/cachetool/cachetool.cc (right): https://codereview.chromium.org/1710923003/diff/20001/net/tools/cachetool/cac... net/tools/cachetool/cachetool.cc:42: // Print call of a cache's keys to stdout. On 2016/02/22 15:44:57, gavinp wrote: > Typo: call --> all Done. https://codereview.chromium.org/1710923003/diff/20001/net/tools/cachetool/cac... net/tools/cachetool/cachetool.cc:137: std::cout << " get_stream <key> <index>: Print a particular stream for a" On 2016/02/22 15:44:57, gavinp wrote: > How about we document the various crazy values of index here, and remove the > earlier comment? It's a single point of documentation, and it's more useful for > being in the help output. Done. https://codereview.chromium.org/1710923003/diff/20001/net/tools/cachetool/cac... net/tools/cachetool/cachetool.cc:177: if (args.size() != 3) { On 2016/02/22 15:44:57, gavinp wrote: > Don't need braces on a single line if. There's a bunch of these in here. The > formal rule is at https://google.github.io/styleguide/cppguide.html#Conditionals > , and it says > > "In general, curly braces are not required for single-line statements, but > they are allowed if you like them; conditional or loop statements with complex > conditions or statements may be more readable with curly braces." > > However, practice in Chrome has been to not have them. You're welcome to them if > you're unduly attached to them though. Done.
lgtm. You can do what you want with my nits. https://codereview.chromium.org/1710923003/diff/40001/net/tools/cachetool/cac... File net/tools/cachetool/cachetool.cc (right): https://codereview.chromium.org/1710923003/diff/40001/net/tools/cachetool/cac... net/tools/cachetool/cachetool.cc:77: for (;;) { Nit: I very slightly prefer "while (true)" here, but I can't find justification in our coding style to actually make more than a nit here. https://codereview.chromium.org/1710923003/diff/40001/net/tools/cachetool/cac... net/tools/cachetool/cachetool.cc:88: if (rv == 0) { Nit: you could lose these braces. https://codereview.chromium.org/1710923003/diff/40001/net/tools/cachetool/cac... net/tools/cachetool/cachetool.cc:132: std::cout << " list_keys: List all keys in cache." << std::endl; Nit: "a cache" or "the cache" (yes, I'm telling you to add an article) https://codereview.chromium.org/1710923003/diff/40001/net/tools/cachetool/cac... net/tools/cachetool/cachetool.cc:137: std::cout << " 0 (HTTP response header)" << std::endl; nit: headers https://codereview.chromium.org/1710923003/diff/40001/net/tools/cachetool/cac... net/tools/cachetool/cachetool.cc:214: return successful_command ? 0 : 1; Nit: you could return !successful_command here.
Thanks Gavin! Commiting. https://codereview.chromium.org/1710923003/diff/40001/net/tools/cachetool/cac... File net/tools/cachetool/cachetool.cc (right): https://codereview.chromium.org/1710923003/diff/40001/net/tools/cachetool/cac... net/tools/cachetool/cachetool.cc:77: for (;;) { On 2016/02/23 15:38:15, gavinp wrote: > Nit: I very slightly prefer "while (true)" here, but I can't find justification > in our coding style to actually make more than a nit here. Done. https://codereview.chromium.org/1710923003/diff/40001/net/tools/cachetool/cac... net/tools/cachetool/cachetool.cc:88: if (rv == 0) { On 2016/02/23 15:38:15, gavinp wrote: > Nit: you could lose these braces. Done. https://codereview.chromium.org/1710923003/diff/40001/net/tools/cachetool/cac... net/tools/cachetool/cachetool.cc:132: std::cout << " list_keys: List all keys in cache." << std::endl; On 2016/02/23 15:38:15, gavinp wrote: > Nit: "a cache" or "the cache" > > (yes, I'm telling you to add an article) Done. https://codereview.chromium.org/1710923003/diff/40001/net/tools/cachetool/cac... net/tools/cachetool/cachetool.cc:137: std::cout << " 0 (HTTP response header)" << std::endl; On 2016/02/23 15:38:15, gavinp wrote: > nit: headers Done. https://codereview.chromium.org/1710923003/diff/40001/net/tools/cachetool/cac... net/tools/cachetool/cachetool.cc:214: return successful_command ? 0 : 1; On 2016/02/23 15:38:14, gavinp wrote: > Nit: you could return !successful_command here. Done.
The CQ bit was checked by gabadie@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gavinp@chromium.org Link to the patchset: https://codereview.chromium.org/1710923003/#ps60001 (title: "Addresses nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1710923003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1710923003/60001
Message was sent while issue was closed.
Description was changed from ========== Implements cachetool to manipulate simple typed cache. BUG=582080 ========== to ========== Implements cachetool to manipulate simple typed cache. BUG=582080 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Implements cachetool to manipulate simple typed cache. BUG=582080 ========== to ========== Implements cachetool to manipulate simple typed cache. BUG=582080 Committed: https://crrev.com/ebc4faedfca14bab8f932d03e1332b1b72e8430e Cr-Commit-Position: refs/heads/master@{#377007} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ebc4faedfca14bab8f932d03e1332b1b72e8430e Cr-Commit-Position: refs/heads/master@{#377007}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1728653002/ by engedy@chromium.org. The reason for reverting is: Broke Win Build, see: https://build.chromium.org/p/chromium/builders/Win/builds/40605.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
