Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14697022:
Cache for mDNS records (Closed)
|
Created:
7 years, 7 months ago by Noam Samuel Modified:
7 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, mmenke Base URL:
https://chromium.googlesource.com/chromium/src.git@record_parsed_klassbit Visibility:
Public. |
DescriptionCache for mDNS records
A cache for storing mDNS records. Supports scheduling expirations for records,
though the actual expiration mechanism may need to be iterated upon.
BUG=233821
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201656
Patch Set 1 #
Total comments: 26
Patch Set 2 : #Patch Set 3 : #
Total comments: 36
Patch Set 4 : #
Total comments: 16
Patch Set 5 : #Patch Set 6 : Removed copy constructor from RecordParsed #
Total comments: 23
Patch Set 7 : #
Messages
Total messages: 25 (0 generated)
Can this use an ExpiringCache?
On 2013/05/14 11:42:32, cbentzel wrote: > Can this use an ExpiringCache? My one concern is that ExpiringCache doesn't seem to have an explicit way to expire records in a timely manner (so that expired records can be, for example, removed from a user-facing list around when their expiration time arrives). This can be mitigated by keeping track of potential record expirations and Get()ing them as they (may) expire, though at that point we'd be managing the details of expiration closely enough that the abstraction provided by ExpiringCache would not be that valuable. Another option would to add those features to ExpiringCache and then use it.
I'm going to have to take a closer look at how this cache will be used before I can review this. (In particular the use of linked_ptr.) https://codereview.chromium.org/14697022/diff/1/net/dns/dns_protocol.h File net/dns/dns_protocol.h (right): https://codereview.chromium.org/14697022/diff/1/net/dns/dns_protocol.h#newcode93 net/dns/dns_protocol.h:93: // The top bit of the MDns class is the cache flush bit. I'm confused by this comment. What is MDns class? What is the cache flush bit? https://codereview.chromium.org/14697022/diff/1/net/dns/mdns_cache.h File net/dns/mdns_cache.h (right): https://codereview.chromium.org/14697022/diff/1/net/dns/mdns_cache.h#newcode23 net/dns/mdns_cache.h:23: class NET_EXPORT_PRIVATE MDnsCache { Needs class-level or file-level comment. https://codereview.chromium.org/14697022/diff/1/net/dns/mdns_cache.h#newcode32 net/dns/mdns_cache.h:32: class DnsRecordCacheKey { Needs comment here or at the file-level. https://codereview.chromium.org/14697022/diff/1/net/dns/mdns_cache.h#newcode47 net/dns/mdns_cache.h:47: uint16 type_; Does it really need to be uint16? Why not unsigned? https://codereview.chromium.org/14697022/diff/1/net/dns/mdns_cache.h#newcode52 net/dns/mdns_cache.h:52: class Query { Comment? It seems to me that this implements the STL predicate design pattern. I think it'd make sense to just use it directly with std::find_if. https://codereview.chromium.org/14697022/diff/1/net/dns/mdns_cache.h#newcode87 net/dns/mdns_cache.h:87: UpdateType UpdateDnsRecord(const RecordParsed* record); Use scoped_ptr<RecordParsed> instead of the comment about ownership. https://codereview.chromium.org/14697022/diff/1/net/dns/mdns_cache.h#newcode92 net/dns/mdns_cache.h:92: std::list<const RecordParsed*>* results, If you always return |results| why do you need to return bool? Is results->empty() not enough? Do you definitely need a linked list? Why not a vector? If you used find_if, you could return an iterator, but I think a vector would work fine. https://codereview.chromium.org/14697022/diff/1/net/dns/mdns_cache.h#newcode99 net/dns/mdns_cache.h:99: std::list<linked_ptr<const RecordParsed> >* records_removed, Ditto. https://codereview.chromium.org/14697022/diff/1/net/dns/mdns_cache.h#newcode104 net/dns/mdns_cache.h:104: base::Time GetNextExpiration() const { return next_expiration_; } If it's a simple accessor, code style says it should be "next_expiration()". https://codereview.chromium.org/14697022/diff/1/net/dns/mdns_cache.h#newcode109 net/dns/mdns_cache.h:109: base::Time GetEffectiveExpiration(const RecordParsed* entry); Needs comment. https://codereview.chromium.org/14697022/diff/1/net/dns/mdns_cache.h#newcode110 net/dns/mdns_cache.h:110: std::string GetOptionalFieldForRecord(const RecordParsed* record); Ditto. https://codereview.chromium.org/14697022/diff/1/net/dns/record_rdata.cc File net/dns/record_rdata.cc (right): https://codereview.chromium.org/14697022/diff/1/net/dns/record_rdata.cc#newco... net/dns/record_rdata.cc:68: return scoped_ptr<const RecordRdata>(return_value.Pass()); I think you can use |return_value.PassAs<const RecordRdata>()|.
https://codereview.chromium.org/14697022/diff/1/net/dns/dns_protocol.h File net/dns/dns_protocol.h (right): https://codereview.chromium.org/14697022/diff/1/net/dns/dns_protocol.h#newcode93 net/dns/dns_protocol.h:93: // The top bit of the MDns class is the cache flush bit. On 2013/05/14 18:14:27, szym wrote: > I'm confused by this comment. What is MDns class? What is the cache flush bit? Sorry about that, the comment is rather vague. I will change it to explain that it is the top bit of the rrclass and added a reference to http://www.rfc-editor.org/rfc/rfc6762.txt section 10.2. https://codereview.chromium.org/14697022/diff/1/net/dns/mdns_cache.h File net/dns/mdns_cache.h (right): https://codereview.chromium.org/14697022/diff/1/net/dns/mdns_cache.h#newcode52 net/dns/mdns_cache.h:52: class Query { It's slightly different because it assumes records are sorted and that all records matching a certain query are contiguous. In practice, the uses of this are fairly narrow. This is basically just a wrapper for either type/name query or type-only query. In hindsight, I think it might be better to do away with this class and just write a function to query the cache for records of a certain type and optionally name (with an empty name implying all records of a certain type). This class structure is far too general-looking, when in reality it has very limited use cases. On 2013/05/14 18:14:27, szym wrote: > Comment? > > It seems to me that this implements the STL predicate design pattern. I think > it'd make sense to just use it directly with std::find_if. https://codereview.chromium.org/14697022/diff/1/net/dns/mdns_cache.h#newcode92 net/dns/mdns_cache.h:92: std::list<const RecordParsed*>* results, Hm. I think you're right that ideally an iterator would be better. As mentioned above, find_if doesn't really make sense in this context. However, it might be beneficial to just write an iterator. On 2013/05/14 18:14:27, szym wrote: > If you always return |results| why do you need to return bool? Is > results->empty() not enough? > > Do you definitely need a linked list? Why not a vector? > > If you used find_if, you could return an iterator, but I think a vector would > work fine.
As for the use of linked_ptr, it is used so that I can store the pointers in an STL container. Each pointer is actually owned by the cache, but since a map that uses scoped_ptr as its value type will not compile (at least, not under clang), I opted to use a linked ptr instead. I do agree that using a list of linked ptrs in the interface was a mistake, and have refactored the cache to use a callback to notify its owner of record deletion instead. https://codereview.chromium.org/14697022/diff/1/net/dns/mdns_cache.h File net/dns/mdns_cache.h (right): https://codereview.chromium.org/14697022/diff/1/net/dns/mdns_cache.h#newcode23 net/dns/mdns_cache.h:23: class NET_EXPORT_PRIVATE MDnsCache { On 2013/05/14 18:14:27, szym wrote: > Needs class-level or file-level comment. Done. https://codereview.chromium.org/14697022/diff/1/net/dns/mdns_cache.h#newcode32 net/dns/mdns_cache.h:32: class DnsRecordCacheKey { On 2013/05/14 18:14:27, szym wrote: > Needs comment here or at the file-level. Done. https://codereview.chromium.org/14697022/diff/1/net/dns/mdns_cache.h#newcode47 net/dns/mdns_cache.h:47: uint16 type_; On 2013/05/14 18:14:27, szym wrote: > Does it really need to be uint16? Why not unsigned? Done. https://codereview.chromium.org/14697022/diff/1/net/dns/mdns_cache.h#newcode52 net/dns/mdns_cache.h:52: class Query { On 2013/05/14 19:28:01, Noam Samuel (chromium) wrote: > It's slightly different because it assumes records are sorted and that all > records matching a certain query are contiguous. In practice, the uses of this > are fairly narrow. This is basically just a wrapper for either type/name query > or type-only query. > > In hindsight, I think it might be better to do away with this class and just > write a function to query the cache for records of a certain type and optionally > name (with an empty name implying all records of a certain type). This class > structure is far too general-looking, when in reality it has very limited use > cases. > > On 2013/05/14 18:14:27, szym wrote: > > Comment? > > > > It seems to me that this implements the STL predicate design pattern. I think > > it'd make sense to just use it directly with std::find_if. > Changed querying to remove this class. https://codereview.chromium.org/14697022/diff/1/net/dns/mdns_cache.h#newcode87 net/dns/mdns_cache.h:87: UpdateType UpdateDnsRecord(const RecordParsed* record); On 2013/05/14 18:14:27, szym wrote: > Use scoped_ptr<RecordParsed> instead of the comment about ownership. Done. https://codereview.chromium.org/14697022/diff/1/net/dns/mdns_cache.h#newcode92 net/dns/mdns_cache.h:92: std::list<const RecordParsed*>* results, On 2013/05/14 19:28:01, Noam Samuel (chromium) wrote: > Hm. I think you're right that ideally an iterator would be better. As mentioned > above, find_if doesn't really make sense in this context. However, it might be > beneficial to just write an iterator. > > On 2013/05/14 18:14:27, szym wrote: > > If you always return |results| why do you need to return bool? Is > > results->empty() not enough? > > > > Do you definitely need a linked list? Why not a vector? > > > > If you used find_if, you could return an iterator, but I think a vector would > > work fine. > On second thought, writing an iterator will probably be overkill for this. If you think it will be beneficial I can write one, but my intuition from starting on it is that it will introduce more complexity than it's worth. https://codereview.chromium.org/14697022/diff/1/net/dns/mdns_cache.h#newcode99 net/dns/mdns_cache.h:99: std::list<linked_ptr<const RecordParsed> >* records_removed, On 2013/05/14 18:14:27, szym wrote: > Ditto. Made irrelevant. https://codereview.chromium.org/14697022/diff/1/net/dns/mdns_cache.h#newcode104 net/dns/mdns_cache.h:104: base::Time GetNextExpiration() const { return next_expiration_; } On 2013/05/14 18:14:27, szym wrote: > If it's a simple accessor, code style says it should be "next_expiration()". Done. https://codereview.chromium.org/14697022/diff/1/net/dns/mdns_cache.h#newcode109 net/dns/mdns_cache.h:109: base::Time GetEffectiveExpiration(const RecordParsed* entry); On 2013/05/14 18:14:27, szym wrote: > Needs comment. Done. https://codereview.chromium.org/14697022/diff/1/net/dns/mdns_cache.h#newcode110 net/dns/mdns_cache.h:110: std::string GetOptionalFieldForRecord(const RecordParsed* record); On 2013/05/14 18:14:27, szym wrote: > Ditto. Done.
https://codereview.chromium.org/14697022/diff/1/net/dns/record_rdata.cc File net/dns/record_rdata.cc (right): https://codereview.chromium.org/14697022/diff/1/net/dns/record_rdata.cc#newco... net/dns/record_rdata.cc:68: return scoped_ptr<const RecordRdata>(return_value.Pass()); On 2013/05/14 18:14:27, szym wrote: > I think you can use |return_value.PassAs<const RecordRdata>()|. Done.
If you only need linked_ptr to be able to store it in a std::map, then you should use simple pointers. On clear (and in destructor) call STLDeleteValues(map).
SGTM, done. On 2013/05/15 20:45:16, szym wrote: > If you only need linked_ptr to be able to store it in a std::map, then you > should use simple pointers. On clear (and in destructor) call > STLDeleteValues(map).
https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.cc File net/dns/mdns_cache.cc (right): https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.cc#new... net/dns/mdns_cache.cc:18: // Allows time for hosts to send updated records. I suggest referring to RFC 6762 Section 10.1 here. https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.cc#new... net/dns/mdns_cache.cc:76: MDnsCacheMap::iterator i = mdns_cache_.find(cache_key); I'd suggest: const RecordParsed*& found = mdns_cache_[cache_key]; if (found) { if (...) type = RecordChanged; delete found; } else { type = RecordAdded; } found = record.release(); This way you are doing at most one lookup. Alternatively, if the reference is confusing, you can use iterators: std::pair<MDnsCacheMap::iterator, bool> found = mdns_cache_.insert(make_pair(cache_key, record.get())); if (found.second) { type = RecordAdded; ... } else { ... delete found.first->second; found.first->second = record.release(); } https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.cc#new... net/dns/mdns_cache.cc:79: type = RecordChanged; IMPORTANT: There is no RecordParsed::IsEqual. https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.cc#new... net/dns/mdns_cache.cc:91: void MDnsCache::CleanupRecords( nit: fits on a single line. https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.cc#new... net/dns/mdns_cache.cc:95: // We are guaranteed that next_expiration_ will be at or before the next nit: || around variable names. https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.cc#new... net/dns/mdns_cache.cc:100: std::vector<MDnsCacheMap::iterator> to_delete; Instead of keeping a handle on the iterators, remove them from the map as you go through it the first time. for (map::iterator i = cache_.begin(); i != ..end(); ) { if (now >= expiration) { // TODO: take a hold of i->second or delete it. cache_.erase(i++); } else { ++i; } } If you don't want to run record_removed_callback_ until they are all removed from the cache, you could keep a std::vector<const RecordParsed*> until later. But if that does not matter, just run it now and delete the record. https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.cc#new... net/dns/mdns_cache.cc:127: results->clear(); DCHECK(results) https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.h File net/dns/mdns_cache.h (right): https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.h#newc... net/dns/mdns_cache.h:17: /** Chromium C++ style says use // comments only. I'd also suggest moving this to just above the class. https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.h#newc... net/dns/mdns_cache.h:28: nit: remove blank line https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.h#newc... net/dns/mdns_cache.h:38: RecordRemoved, RecordRemoved is never used. Do you need it? https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.h#newc... net/dns/mdns_cache.h:43: // The calls are done synchronously. |record_removed_callback| is called in CleanupRecords only, so I'd pass it to CleanupRecords rather than making it a permanent property of the cache. If you keep it as is, explain here that the callback is called in CleanupRecords only (as opposed to Clear or destructor). https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.h#newc... net/dns/mdns_cache.h:59: // Empty the cache, call |record_removed_callback_| for every removed record. Do you really mean "empty the cache", or is it "remove expired records"? https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.h#newc... net/dns/mdns_cache.h:63: // Is updated when CleapRecords or UpdateDnsRecord are called. Fix typo. Should explain returns base::Time() when empty. https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.h#newc... net/dns/mdns_cache.h:78: nit: remove blank https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.h#newc... net/dns/mdns_cache.h:82: I believe this needs explicit copy constructor and operator=, but I'd settle for a comment that the implicit ones are used on purpose. https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.h#newc... net/dns/mdns_cache.h:98: // nonzero ttl, as explained in RFC 6762 Section 10.1. nit: Use "TTL" consistently (not "ttl"). https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.h#newc... net/dns/mdns_cache.h:108: // next_expiration_ is always guaranteed to be less than or equal to the next nit: Wrap references to variables in ||. To avoid redundant comments, comment either at the accessor or here. https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.h#newc... net/dns/mdns_cache.h:113: RecordRemovedCallback record_removed_callback_; If you keep it and don't intend to change it, make const.
https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.h File net/dns/mdns_cache.h (right): https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.h#newc... net/dns/mdns_cache.h:38: RecordRemoved, On 2013/05/17 05:04:34, szym wrote: > RecordRemoved is never used. Do you need it? Hm. I added it here so I could use it to signify update types in the MDns layer, but considering that its use is overloaded, perhaps it is better to pull it out and put it somewhere else, such as a common header. I'm just not sure where to put it short of making an "mdns_common" header.
Hey, uploaded a new patch. This time I got the diff-base right, so all the files should be present (sorry about that). https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.cc File net/dns/mdns_cache.cc (right): https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.cc#new... net/dns/mdns_cache.cc:18: // Allows time for hosts to send updated records. On 2013/05/17 05:04:34, szym wrote: > I suggest referring to RFC 6762 Section 10.1 here. Done. https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.cc#new... net/dns/mdns_cache.cc:76: MDnsCacheMap::iterator i = mdns_cache_.find(cache_key); On 2013/05/17 05:04:34, szym wrote: > I'd suggest: > > const RecordParsed*& found = mdns_cache_[cache_key]; > if (found) { > if (...) > type = RecordChanged; > delete found; > } else { > type = RecordAdded; > } > found = record.release(); > > This way you are doing at most one lookup. > > Alternatively, if the reference is confusing, you can use iterators: > > std::pair<MDnsCacheMap::iterator, bool> found = > mdns_cache_.insert(make_pair(cache_key, record.get())); > if (found.second) { > type = RecordAdded; > ... > } else { > ... > delete found.first->second; > found.first->second = record.release(); > } Done. https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.cc#new... net/dns/mdns_cache.cc:79: type = RecordChanged; Oh whoops. Uploaded the patch wrong. This upload is diffed against master and should contain all files. On 2013/05/17 05:04:34, szym wrote: > IMPORTANT: There is no RecordParsed::IsEqual. https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.cc#new... net/dns/mdns_cache.cc:91: void MDnsCache::CleanupRecords( On 2013/05/17 05:04:34, szym wrote: > nit: fits on a single line. Done. https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.cc#new... net/dns/mdns_cache.cc:95: // We are guaranteed that next_expiration_ will be at or before the next On 2013/05/17 05:04:34, szym wrote: > nit: || around variable names. Done. https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.cc#new... net/dns/mdns_cache.cc:100: std::vector<MDnsCacheMap::iterator> to_delete; On 2013/05/17 05:04:34, szym wrote: > Instead of keeping a handle on the iterators, remove them from the map as you go > through it the first time. > > for (map::iterator i = cache_.begin(); i != ..end(); ) { > if (now >= expiration) { > // TODO: take a hold of i->second or delete it. > cache_.erase(i++); > } else { > ++i; > } > } > > If you don't want to run record_removed_callback_ until they are all removed > from the cache, you could keep a std::vector<const RecordParsed*> until later. > But if that does not matter, just run it now and delete the record. Done. https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.cc#new... net/dns/mdns_cache.cc:127: results->clear(); On 2013/05/17 05:04:34, szym wrote: > DCHECK(results) Done. https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.h File net/dns/mdns_cache.h (right): https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.h#newc... net/dns/mdns_cache.h:17: /** On 2013/05/17 05:04:34, szym wrote: > Chromium C++ style says use // comments only. > > I'd also suggest moving this to just above the class. Done. https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.h#newc... net/dns/mdns_cache.h:28: On 2013/05/17 05:04:34, szym wrote: > nit: remove blank line Done. https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.h#newc... net/dns/mdns_cache.h:43: // The calls are done synchronously. |record_removed_callback| now passed to CleanupRecords. On 2013/05/17 05:04:34, szym wrote: > |record_removed_callback| is called in CleanupRecords only, so I'd pass it to > CleanupRecords rather than making it a permanent property of the cache. If you > keep it as is, explain here that the callback is called in CleanupRecords only > (as opposed to Clear or destructor). > https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.h#newc... net/dns/mdns_cache.h:59: // Empty the cache, call |record_removed_callback_| for every removed record. On 2013/05/17 05:04:34, szym wrote: > Do you really mean "empty the cache", or is it "remove expired records"? Done. https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.h#newc... net/dns/mdns_cache.h:63: // Is updated when CleapRecords or UpdateDnsRecord are called. On 2013/05/17 05:04:34, szym wrote: > Fix typo. Should explain returns base::Time() when empty. Done. https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.h#newc... net/dns/mdns_cache.h:78: On 2013/05/17 05:04:34, szym wrote: > nit: remove blank Done. https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.h#newc... net/dns/mdns_cache.h:82: On 2013/05/17 05:04:34, szym wrote: > I believe this needs explicit copy constructor and operator=, but I'd settle for > a comment that the implicit ones are used on purpose. Done. https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.h#newc... net/dns/mdns_cache.h:98: // nonzero ttl, as explained in RFC 6762 Section 10.1. On 2013/05/17 05:04:34, szym wrote: > nit: Use "TTL" consistently (not "ttl"). Done. https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.h#newc... net/dns/mdns_cache.h:108: // next_expiration_ is always guaranteed to be less than or equal to the next Removed comment here. Better to have comment in publicly-facing accessor. On 2013/05/17 05:04:34, szym wrote: > nit: Wrap references to variables in ||. > > To avoid redundant comments, comment either at the accessor or here. https://codereview.chromium.org/14697022/diff/32001/net/dns/mdns_cache.h#newc... net/dns/mdns_cache.h:113: RecordRemovedCallback record_removed_callback_; Removed. On 2013/05/17 05:04:34, szym wrote: > If you keep it and don't intend to change it, make const.
https://codereview.chromium.org/14697022/diff/49001/net/dns/mdns_cache.cc File net/dns/mdns_cache.cc (right): https://codereview.chromium.org/14697022/diff/49001/net/dns/mdns_cache.cc#new... net/dns/mdns_cache.cc:155: // Records are deleted only upon request to make this op idempotent The op would still be idempotent (same result) if you did delete expired results, but I think it's reasonable to only remove them in CleanupRecords. Anyway, please mark FindDnsRecords const. https://codereview.chromium.org/14697022/diff/49001/net/dns/mdns_cache.cc#new... net/dns/mdns_cache.cc:162: std::string MDnsCache::GetOptionalFieldForRecord(const RecordParsed* record) { Mark const. https://codereview.chromium.org/14697022/diff/49001/net/dns/mdns_cache.cc#new... net/dns/mdns_cache.cc:173: base::Time MDnsCache::GetEffectiveExpiration(const RecordParsed* record) { Mark const. https://codereview.chromium.org/14697022/diff/49001/net/dns/mdns_cache.h File net/dns/mdns_cache.h (right): https://codereview.chromium.org/14697022/diff/49001/net/dns/mdns_cache.h#newc... net/dns/mdns_cache.h:69: class DnsRecordCacheKey { I'd just call this |Key|. https://codereview.chromium.org/14697022/diff/49001/net/dns/mdns_cache.h#newc... net/dns/mdns_cache.h:91: MDnsCacheMap; I'd just call this |RecordMap|. https://codereview.chromium.org/14697022/diff/49001/net/dns/record_parsed.h File net/dns/record_parsed.h (right): https://codereview.chromium.org/14697022/diff/49001/net/dns/record_parsed.h#n... net/dns/record_parsed.h:10: #include "base/time.h" header order https://codereview.chromium.org/14697022/diff/49001/net/dns/record_parsed.h#n... net/dns/record_parsed.h:23: RecordParsed(const RecordParsed&); Do you need this?
https://codereview.chromium.org/14697022/diff/49001/net/dns/mdns_cache.cc File net/dns/mdns_cache.cc (right): https://codereview.chromium.org/14697022/diff/49001/net/dns/mdns_cache.cc#new... net/dns/mdns_cache.cc:155: // Records are deleted only upon request to make this op idempotent Fixed comment. On 2013/05/20 19:35:15, szym wrote: > The op would still be idempotent (same result) if you did delete expired > results, but I think it's reasonable to only remove them in CleanupRecords. > > Anyway, please mark FindDnsRecords const. https://codereview.chromium.org/14697022/diff/49001/net/dns/mdns_cache.cc#new... net/dns/mdns_cache.cc:162: std::string MDnsCache::GetOptionalFieldForRecord(const RecordParsed* record) { On 2013/05/20 19:35:15, szym wrote: > Mark const. Done. https://codereview.chromium.org/14697022/diff/49001/net/dns/mdns_cache.cc#new... net/dns/mdns_cache.cc:173: base::Time MDnsCache::GetEffectiveExpiration(const RecordParsed* record) { On 2013/05/20 19:35:15, szym wrote: > Mark const. Done. https://codereview.chromium.org/14697022/diff/49001/net/dns/mdns_cache.h File net/dns/mdns_cache.h (right): https://codereview.chromium.org/14697022/diff/49001/net/dns/mdns_cache.h#newc... net/dns/mdns_cache.h:69: class DnsRecordCacheKey { On 2013/05/20 19:35:15, szym wrote: > I'd just call this |Key|. Done. https://codereview.chromium.org/14697022/diff/49001/net/dns/mdns_cache.h#newc... net/dns/mdns_cache.h:91: MDnsCacheMap; On 2013/05/20 19:35:15, szym wrote: > I'd just call this |RecordMap|. Done. https://codereview.chromium.org/14697022/diff/49001/net/dns/record_parsed.h File net/dns/record_parsed.h (right): https://codereview.chromium.org/14697022/diff/49001/net/dns/record_parsed.h#n... net/dns/record_parsed.h:10: #include "base/time.h" Fixed. On 2013/05/20 19:35:15, szym wrote: > header order https://codereview.chromium.org/14697022/diff/49001/net/dns/record_parsed.h#n... net/dns/record_parsed.h:23: RecordParsed(const RecordParsed&); On 2013/05/20 19:35:15, szym wrote: > Do you need this? It's used to pass RecordParsed objects asynchronously through the message loop.
https://codereview.chromium.org/14697022/diff/49001/net/dns/record_parsed.h File net/dns/record_parsed.h (right): https://codereview.chromium.org/14697022/diff/49001/net/dns/record_parsed.h#n... net/dns/record_parsed.h:23: RecordParsed(const RecordParsed&); On 2013/05/20 21:07:02, Noam Samuel (chromium) wrote: > On 2013/05/20 19:35:15, szym wrote: > > Do you need this? > > It's used to pass RecordParsed objects asynchronously through the message loop. It's not used in this CL. I know it's a hassle, but could you move it (and the tests) to the CL that will need it?
https://codereview.chromium.org/14697022/diff/49001/net/dns/record_parsed.h File net/dns/record_parsed.h (right): https://codereview.chromium.org/14697022/diff/49001/net/dns/record_parsed.h#n... net/dns/record_parsed.h:23: RecordParsed(const RecordParsed&); On 2013/05/20 21:13:03, szym wrote: > On 2013/05/20 21:07:02, Noam Samuel (chromium) wrote: > > On 2013/05/20 19:35:15, szym wrote: > > > Do you need this? > > > > It's used to pass RecordParsed objects asynchronously through the message > loop. > > It's not used in this CL. I know it's a hassle, but could you move it (and the > tests) to the CL that will need it? Also, Google style suggests using just CopyFrom() or Clone() rather than the copy constructor, unless some STL-dependent code needs a copy constructor.
OK, removed copy constructor and related code from this CL. On 2013/05/20 21:20:50, szym wrote: > https://codereview.chromium.org/14697022/diff/49001/net/dns/record_parsed.h > File net/dns/record_parsed.h (right): > > https://codereview.chromium.org/14697022/diff/49001/net/dns/record_parsed.h#n... > net/dns/record_parsed.h:23: RecordParsed(const RecordParsed&); > On 2013/05/20 21:13:03, szym wrote: > > On 2013/05/20 21:07:02, Noam Samuel (chromium) wrote: > > > On 2013/05/20 19:35:15, szym wrote: > > > > Do you need this? > > > > > > It's used to pass RecordParsed objects asynchronously through the message > > loop. > > > > It's not used in this CL. I know it's a hassle, but could you move it (and the > > tests) to the CL that will need it? > > Also, Google style suggests using just CopyFrom() or Clone() rather than the > copy constructor, unless some STL-dependent code needs a copy constructor.
lgtm https://codereview.chromium.org/14697022/diff/60001/net/dns/mdns_cache.cc File net/dns/mdns_cache.cc (right): https://codereview.chromium.org/14697022/diff/60001/net/dns/mdns_cache.cc#new... net/dns/mdns_cache.cc:47: const MDnsCache::Key& key) const { one line https://codereview.chromium.org/14697022/diff/60001/net/dns/mdns_cache.cc#new... net/dns/mdns_cache.cc:60: const MDnsCache::Key& key) const { one line https://codereview.chromium.org/14697022/diff/60001/net/dns/mdns_cache.cc#new... net/dns/mdns_cache.cc:60: const MDnsCache::Key& key) const { one line https://codereview.chromium.org/14697022/diff/60001/net/dns/mdns_cache.cc#new... net/dns/mdns_cache.cc:145: mdns_cache_.lower_bound(Key(type, name, "")); one line https://codereview.chromium.org/14697022/diff/60001/net/dns/mdns_cache.h File net/dns/mdns_cache.h (right): https://codereview.chromium.org/14697022/diff/60001/net/dns/mdns_cache.h#newc... net/dns/mdns_cache.h:22: // MDNS CACHE nit: unnecessary caps. https://codereview.chromium.org/14697022/diff/60001/net/dns/mdns_cache.h#newc... net/dns/mdns_cache.h:26: nit: unnecessary blank line. https://codereview.chromium.org/14697022/diff/60001/net/dns/mdns_cache_unitte... File net/dns/mdns_cache_unittest.cc (right): https://codereview.chromium.org/14697022/diff/60001/net/dns/mdns_cache_unitte... net/dns/mdns_cache_unittest.cc:233: } please add // namespace net and a blank line above https://codereview.chromium.org/14697022/diff/60001/net/dns/record_parsed_uni... File net/dns/record_parsed_unittest.cc (right): https://codereview.chromium.org/14697022/diff/60001/net/dns/record_parsed_uni... net/dns/record_parsed_unittest.cc:74: } please add // namespace net and a blank line above https://codereview.chromium.org/14697022/diff/60001/net/dns/record_rdata.cc File net/dns/record_rdata.cc (right): https://codereview.chromium.org/14697022/diff/60001/net/dns/record_rdata.cc#n... net/dns/record_rdata.cc:215: nit: remove blank https://codereview.chromium.org/14697022/diff/60001/net/dns/record_rdata.h File net/dns/record_rdata.h (right): https://codereview.chromium.org/14697022/diff/60001/net/dns/record_rdata.h#ne... net/dns/record_rdata.h:183: } please add // namespace net and a blank line above https://codereview.chromium.org/14697022/diff/60001/net/dns/record_rdata_unit... File net/dns/record_rdata_unittest.cc (right): https://codereview.chromium.org/14697022/diff/60001/net/dns/record_rdata_unit... net/dns/record_rdata_unittest.cc:91: 0x00, 0x00, 0x00, 0x01 // ::1 I'd suggest something slightly more complex than that. https://codereview.chromium.org/14697022/diff/60001/net/dns/record_rdata_unit... net/dns/record_rdata_unittest.cc:101: IPAddressToString(record_obj->address())); You need to include net_util.h for that.
https://codereview.chromium.org/14697022/diff/60001/net/dns/mdns_cache.cc File net/dns/mdns_cache.cc (right): https://codereview.chromium.org/14697022/diff/60001/net/dns/mdns_cache.cc#new... net/dns/mdns_cache.cc:47: const MDnsCache::Key& key) const { On 2013/05/21 22:09:05, szym wrote: > one line Done. https://codereview.chromium.org/14697022/diff/60001/net/dns/mdns_cache.cc#new... net/dns/mdns_cache.cc:60: const MDnsCache::Key& key) const { On 2013/05/21 22:09:05, szym wrote: > one line Done. https://codereview.chromium.org/14697022/diff/60001/net/dns/mdns_cache.cc#new... net/dns/mdns_cache.cc:145: mdns_cache_.lower_bound(Key(type, name, "")); On 2013/05/21 22:09:05, szym wrote: > one line Done. https://codereview.chromium.org/14697022/diff/60001/net/dns/mdns_cache.h File net/dns/mdns_cache.h (right): https://codereview.chromium.org/14697022/diff/60001/net/dns/mdns_cache.h#newc... net/dns/mdns_cache.h:22: // MDNS CACHE On 2013/05/21 22:09:05, szym wrote: > nit: unnecessary caps. Done. https://codereview.chromium.org/14697022/diff/60001/net/dns/mdns_cache.h#newc... net/dns/mdns_cache.h:26: On 2013/05/21 22:09:05, szym wrote: > nit: unnecessary blank line. Done. https://codereview.chromium.org/14697022/diff/60001/net/dns/mdns_cache_unitte... File net/dns/mdns_cache_unittest.cc (right): https://codereview.chromium.org/14697022/diff/60001/net/dns/mdns_cache_unitte... net/dns/mdns_cache_unittest.cc:233: } On 2013/05/21 22:09:05, szym wrote: > please add // namespace net > and a blank line above Done. https://codereview.chromium.org/14697022/diff/60001/net/dns/record_parsed_uni... File net/dns/record_parsed_unittest.cc (right): https://codereview.chromium.org/14697022/diff/60001/net/dns/record_parsed_uni... net/dns/record_parsed_unittest.cc:74: } On 2013/05/21 22:09:05, szym wrote: > please add // namespace net > and a blank line above Done. https://codereview.chromium.org/14697022/diff/60001/net/dns/record_rdata.cc File net/dns/record_rdata.cc (right): https://codereview.chromium.org/14697022/diff/60001/net/dns/record_rdata.cc#n... net/dns/record_rdata.cc:215: On 2013/05/21 22:09:05, szym wrote: > nit: remove blank Done. https://codereview.chromium.org/14697022/diff/60001/net/dns/record_rdata.h File net/dns/record_rdata.h (right): https://codereview.chromium.org/14697022/diff/60001/net/dns/record_rdata.h#ne... net/dns/record_rdata.h:183: } On 2013/05/21 22:09:05, szym wrote: > please add // namespace net > and a blank line above Done. https://codereview.chromium.org/14697022/diff/60001/net/dns/record_rdata_unit... File net/dns/record_rdata_unittest.cc (right): https://codereview.chromium.org/14697022/diff/60001/net/dns/record_rdata_unit... net/dns/record_rdata_unittest.cc:91: 0x00, 0x00, 0x00, 0x01 // ::1 On 2013/05/21 22:09:05, szym wrote: > I'd suggest something slightly more complex than that. Done. https://codereview.chromium.org/14697022/diff/60001/net/dns/record_rdata_unit... net/dns/record_rdata_unittest.cc:101: IPAddressToString(record_obj->address())); On 2013/05/21 22:09:05, szym wrote: > You need to include net_util.h for that. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noamsml@chromium.org/14697022/84001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
Looks like all issues are issues with HEAD. Retrying now that the tree is open. On 2013/05/21 23:40:40, I haz the power (commit-bot) wrote: > Sorry for I got bad news for ya. > Compile failed with a clobber build on linux_chromeos_clang. > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... > Your code is likely broken or HEAD is junk. Please ensure your > code is not broken then alert the build sheriffs. > Look at the try server FAQ for more details.
Looks like all issues are issues with HEAD. Retrying now that the tree is open. On 2013/05/21 23:40:40, I haz the power (commit-bot) wrote: > Sorry for I got bad news for ya. > Compile failed with a clobber build on linux_chromeos_clang. > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... > Your code is likely broken or HEAD is junk. Please ensure your > code is not broken then alert the build sheriffs. > Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noamsml@chromium.org/14697022/84001
Message was sent while issue was closed.
Change committed as 201656 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
