|
|
DescriptionAdd initial PpdProvider implementation and associated PpdCache. Also add unit tests for both.
BUG=617254
Committed: https://crrev.com/c7d4aa6cfd11244553fb6308434587149a8518ba
Cr-Commit-Position: refs/heads/master@{#427353}
Patch Set 1 #Patch Set 2 : Remove extra includes #
Total comments: 16
Patch Set 3 : Address skau comments #Patch Set 4 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #Patch Set 5 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #
Total comments: 25
Patch Set 6 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #Patch Set 7 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #
Total comments: 21
Patch Set 8 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #
Total comments: 38
Patch Set 9 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #Patch Set 10 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #Patch Set 11 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #Patch Set 12 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #
Total comments: 61
Patch Set 13 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #Patch Set 14 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #Patch Set 15 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #Patch Set 16 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #
Total comments: 18
Patch Set 17 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #Patch Set 18 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #
Total comments: 26
Patch Set 19 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #
Total comments: 2
Patch Set 20 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #Patch Set 21 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #
Messages
Total messages: 63 (15 generated)
Description was changed from ========== Add PPDProvider barebones implementation and associated cache skeleton. Add Unit test framework for PPDProvider (unit test doesn't test much yet) BUG=617254 ========== to ========== Add PPDProvider barebones implementation and associated cache skeleton. Add Unit test framework for PPDProvider (unit test doesn't test much yet) Cache implementation is currently TBD, pending discussions around syncing. This is probably very wrong in many ways, since it's my first chromium patch, happy to take any suggestions including "do this in a completely different way". BUG=617254 ==========
justincarlson@chromium.org changed reviewers: + skau@chromium.org
Looks generally reasonable. We should chat about PPDFile. I made a comment in ppd_cache.h. I'm not sure if there's an official stance on whether base or std should be qualified. Unqualified seems more common in Chromium where qualified seems to be more common for Google. https://codereview.chromium.org/2343983004/diff/20001/chromeos/printing/ppd_c... File chromeos/printing/ppd_cache.cc (right): https://codereview.chromium.org/2343983004/diff/20001/chromeos/printing/ppd_c... chromeos/printing/ppd_cache.cc:43: std::unique_ptr<Printer::PPDFile> ret(new Printer::PPDFile); ret = base::MakeUnique<PPDFile>(). Raw use of new is frowned upon now. https://codereview.chromium.org/2343983004/diff/20001/chromeos/printing/ppd_c... chromeos/printing/ppd_cache.cc:61: // exist? We'll need to hook back into Chrome for the directory information since it's stored as a preference somewhere ... Due to dependency rules, we'll probably need to put an Options producer in chrome/browser/chromeos/printing. https://codereview.chromium.org/2343983004/diff/20001/chromeos/printing/ppd_c... File chromeos/printing/ppd_cache.h (right): https://codereview.chromium.org/2343983004/diff/20001/chromeos/printing/ppd_c... chromeos/printing/ppd_cache.h:34: virtual std::unique_ptr<Printer::PPDFile> Lookup( When specifying PPDFile, my thought was that it would be the input to PPDCache rather than a result. Then the PPDCache wouldn't need to store the mapping between (manufacturer, model) -> file since that relationship can be many -> one. https://codereview.chromium.org/2343983004/diff/20001/chromeos/printing/ppd_p... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2343983004/diff/20001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.cc:4: ppd_provider.h should be included first https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes https://codereview.chromium.org/2343983004/diff/20001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.cc:103: parsed_json->GetType() != Value::TYPE_DICTIONARY || // GetAsDictionary implies this check and will fail safely if the value isn't a dictionary. https://codereview.chromium.org/2343983004/diff/20001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.cc:171: // ret.api_key = ::google_apis::GetAPIKey(); It'll need to be passed in as it depends on the platform. The Chromium code base tries to avoid assuming that Chromium is the only app that uses this code. https://codereview.chromium.org/2343983004/diff/20001/chromeos/printing/ppd_p... File chromeos/printing/ppd_provider.h (right): https://codereview.chromium.org/2343983004/diff/20001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.h:34: // we found a ppd that we think will work, but wasn't an exact match? What would provide the "FALLBACK" signal? I think if we ask the server it'll give us a binary response even if the PPD doesn't explicitly support the given printer. https://codereview.chromium.org/2343983004/diff/20001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.h:43: typedef ::base::Callback<void(LookupResult, Printer::PPDFile)> LookupCallback; Chromium is moving away from typedef in favor of using aliases. i.e. using foo = bar;
Took a while to circle back to this, but I think this is in better shape now. PTAL? https://codereview.chromium.org/2343983004/diff/20001/chromeos/printing/ppd_c... File chromeos/printing/ppd_cache.cc (right): https://codereview.chromium.org/2343983004/diff/20001/chromeos/printing/ppd_c... chromeos/printing/ppd_cache.cc:43: std::unique_ptr<Printer::PPDFile> ret(new Printer::PPDFile); On 2016/09/17 00:59:59, skau wrote: > ret = base::MakeUnique<PPDFile>(). Raw use of new is frowned upon now. Done. https://codereview.chromium.org/2343983004/diff/20001/chromeos/printing/ppd_c... chromeos/printing/ppd_cache.cc:61: // exist? On 2016/09/17 00:59:59, skau wrote: > We'll need to hook back into Chrome for the directory information since it's > stored as a preference somewhere ... Due to dependency rules, we'll probably > need to put an Options producer in chrome/browser/chromeos/printing. This seems to fit nicely in what PathService provides, so I've provisionally added it there. https://codereview.chromium.org/2343983004/diff/20001/chromeos/printing/ppd_c... File chromeos/printing/ppd_cache.h (right): https://codereview.chromium.org/2343983004/diff/20001/chromeos/printing/ppd_c... chromeos/printing/ppd_cache.h:34: virtual std::unique_ptr<Printer::PPDFile> Lookup( On 2016/09/17 00:59:59, skau wrote: > When specifying PPDFile, my thought was that it would be the input to PPDCache > rather than a result. Then the PPDCache wouldn't need to store the mapping > between (manufacturer, model) -> file since that relationship can be many -> > one. Reworked this based on the discussions we had. https://codereview.chromium.org/2343983004/diff/20001/chromeos/printing/ppd_p... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2343983004/diff/20001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.cc:4: On 2016/09/17 00:59:59, skau wrote: > ppd_provider.h should be included first > https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes Done. https://codereview.chromium.org/2343983004/diff/20001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.cc:103: parsed_json->GetType() != Value::TYPE_DICTIONARY || // On 2016/09/17 00:59:59, skau wrote: > GetAsDictionary implies this check and will fail safely if the value isn't a > dictionary. Done. https://codereview.chromium.org/2343983004/diff/20001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.cc:171: // ret.api_key = ::google_apis::GetAPIKey(); On 2016/09/17 00:59:59, skau wrote: > It'll need to be passed in as it depends on the platform. The Chromium code > base tries to avoid assuming that Chromium is the only app that uses this code. Done. https://codereview.chromium.org/2343983004/diff/20001/chromeos/printing/ppd_p... File chromeos/printing/ppd_provider.h (right): https://codereview.chromium.org/2343983004/diff/20001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.h:34: // we found a ppd that we think will work, but wasn't an exact match? On 2016/09/17 00:59:59, skau wrote: > What would provide the "FALLBACK" signal? I think if we ask the server it'll > give us a binary response even if the PPD doesn't explicitly support the given > printer. I'm just speculating here that there may be some user-visible feedback we want to give at some point of the form "we don't *really* know what this printer is, but we think this driver will work." As for who tells us, presumably the quirks-server would for a server-provided ppd. If we use any of the mechanisms to generate ppds, there might be other sources, too. https://codereview.chromium.org/2343983004/diff/20001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.h:43: typedef ::base::Callback<void(LookupResult, Printer::PPDFile)> LookupCallback; On 2016/09/17 00:59:59, skau wrote: > Chromium is moving away from typedef in favor of using aliases. i.e. using foo = > bar; I keep forgetting you can do that, done.
Generally looks good. There's a few issues with convention and a couple things that are unclear. https://codereview.chromium.org/2343983004/diff/80001/chromeos/chromeos_paths.h File chromeos/chromeos_paths.h (right): https://codereview.chromium.org/2343983004/diff/80001/chromeos/chromeos_paths... chromeos/chromeos_paths.h:53: DIR_PRINTER_DRIVERS_CACHE, // Directory where ppds previously used It looks like this file is for chromeos system folders where we're dealing with user data. Take a look at CHROMEOS_WALLPAPER here: chrome/common/chrome_paths.h https://codereview.chromium.org/2343983004/diff/80001/chromeos/printing/ppd_c... File chromeos/printing/ppd_cache.cc (right): https://codereview.chromium.org/2343983004/diff/80001/chromeos/printing/ppd_c... chromeos/printing/ppd_cache.cc:29: const char* kLocalModel = "local"; const char[] for string constants https://codereview.chromium.org/2343983004/diff/80001/chromeos/printing/ppd_c... chromeos/printing/ppd_cache.cc:43: LOG(ERROR) << "Denying attempt to store auto-ppd under local namespace"; Denying attempt to retrieve local ppd from auto namespace? https://codereview.chromium.org/2343983004/diff/80001/chromeos/printing/ppd_c... chromeos/printing/ppd_cache.cc:46: if (!(KeyIsValid("Printer manufacturer", manufacturer) && Is "Printer manufacturer" a constant? https://codereview.chromium.org/2343983004/diff/80001/chromeos/printing/ppd_c... chromeos/printing/ppd_cache.cc:113: LOG(FATAL) << "Failed to cleanup partially-written file " LOG(FATAL) will crash Chrome and thus the entire UI. You'll want to still log an error and return. We generally only exit if it's a security issue or we risk user data corruption if we continue. https://codereview.chromium.org/2343983004/diff/80001/chromeos/printing/ppd_c... chromeos/printing/ppd_cache.cc:128: return cache_base_dir_.Append(base::StringPrintf("%zx", h)); Can we attach an appropriate file extension like .ppd.gz (if it's gzipped)? https://codereview.chromium.org/2343983004/diff/80001/chromeos/printing/ppd_c... chromeos/printing/ppd_cache.cc:133: bool KeyIsValid(const string& key_name, const string& key) const { Is there a way to clarify that this validation is independent of the key type? When I saw this, I interpreted the labels as a type of enumeration. https://codereview.chromium.org/2343983004/diff/80001/chromeos/printing/ppd_c... chromeos/printing/ppd_cache.cc:166: return ::base::MakeUnique<PPDCacheImpl>(PPDCache::Options()); nit: If we're always passing an Options struct, we can use a default argument in the signature. https://codereview.chromium.org/2343983004/diff/80001/chromeos/printing/ppd_c... File chromeos/printing/ppd_cache.h (right): https://codereview.chromium.org/2343983004/diff/80001/chromeos/printing/ppd_c... chromeos/printing/ppd_cache.h:18: // PPDCache is manages a cache of locally-stored ppd files. It stores typo (is) https://codereview.chromium.org/2343983004/diff/80001/chromeos/printing/ppd_c... chromeos/printing/ppd_cache.h:49: const std::string& key) const = 0; Would this be the appropriate place to document what we expect the key to be? Users of this function may not interact with Store* https://codereview.chromium.org/2343983004/diff/80001/chromeos/printing/ppd_p... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2343983004/diff/80001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.cc:109: auto tmp = cache_->FindLocal(printer_id); Can you write out this type? It's not clear that it's Optional<FilePath>. https://codereview.chromium.org/2343983004/diff/80001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.cc:136: if (parsed_json == nullptr || // are you forcing it to wrap with the comment //?
Reworked this given the PpdReference changes. Many of the comments are now obsolete, but I addressed the other ones. Also note that I'm punting a problem on how to get the PpdCache path into the objects by forcing it to be supplied at construction time. The naive approach to getting that connected to PathService's user-specific directories causes a dependency loop that I don't yet see a way of breaking. However, doing so should not change this work in any significant way. https://codereview.chromium.org/2343983004/diff/80001/chromeos/chromeos_paths.h File chromeos/chromeos_paths.h (right): https://codereview.chromium.org/2343983004/diff/80001/chromeos/chromeos_paths... chromeos/chromeos_paths.h:53: DIR_PRINTER_DRIVERS_CACHE, // Directory where ppds previously used On 2016/10/07 16:29:06, skau wrote: > It looks like this file is for chromeos system folders where we're dealing with > user data. Take a look at CHROMEOS_WALLPAPER here: chrome/common/chrome_paths.h Done. https://codereview.chromium.org/2343983004/diff/80001/chromeos/printing/ppd_c... File chromeos/printing/ppd_cache.cc (right): https://codereview.chromium.org/2343983004/diff/80001/chromeos/printing/ppd_c... chromeos/printing/ppd_cache.cc:29: const char* kLocalModel = "local"; On 2016/10/07 16:29:06, skau wrote: > const char[] for string constants OK, but any particular reason why? https://codereview.chromium.org/2343983004/diff/80001/chromeos/printing/ppd_c... chromeos/printing/ppd_cache.cc:43: LOG(ERROR) << "Denying attempt to store auto-ppd under local namespace"; On 2016/10/07 16:29:06, skau wrote: > Denying attempt to retrieve local ppd from auto namespace? Obsolete https://codereview.chromium.org/2343983004/diff/80001/chromeos/printing/ppd_c... chromeos/printing/ppd_cache.cc:46: if (!(KeyIsValid("Printer manufacturer", manufacturer) && On 2016/10/07 16:29:06, skau wrote: > Is "Printer manufacturer" a constant? Obsolete https://codereview.chromium.org/2343983004/diff/80001/chromeos/printing/ppd_c... chromeos/printing/ppd_cache.cc:113: LOG(FATAL) << "Failed to cleanup partially-written file " On 2016/10/07 16:29:06, skau wrote: > LOG(FATAL) will crash Chrome and thus the entire UI. You'll want to still log > an error and return. We generally only exit if it's a security issue or we risk > user data corruption if we continue. Done. https://codereview.chromium.org/2343983004/diff/80001/chromeos/printing/ppd_c... chromeos/printing/ppd_cache.cc:128: return cache_base_dir_.Append(base::StringPrintf("%zx", h)); On 2016/10/07 16:29:06, skau wrote: > Can we attach an appropriate file extension like .ppd.gz (if it's gzipped)? Done. https://codereview.chromium.org/2343983004/diff/80001/chromeos/printing/ppd_c... chromeos/printing/ppd_cache.cc:133: bool KeyIsValid(const string& key_name, const string& key) const { On 2016/10/07 16:29:06, skau wrote: > Is there a way to clarify that this validation is independent of the key type? > When I saw this, I interpreted the labels as a type of enumeration. Obsolete https://codereview.chromium.org/2343983004/diff/80001/chromeos/printing/ppd_c... chromeos/printing/ppd_cache.cc:166: return ::base::MakeUnique<PPDCacheImpl>(PPDCache::Options()); On 2016/10/07 16:29:06, skau wrote: > nit: If we're always passing an Options struct, we can use a default argument in > the signature. Done. https://codereview.chromium.org/2343983004/diff/80001/chromeos/printing/ppd_c... File chromeos/printing/ppd_cache.h (right): https://codereview.chromium.org/2343983004/diff/80001/chromeos/printing/ppd_c... chromeos/printing/ppd_cache.h:18: // PPDCache is manages a cache of locally-stored ppd files. It stores On 2016/10/07 16:29:06, skau wrote: > typo (is) Done. https://codereview.chromium.org/2343983004/diff/80001/chromeos/printing/ppd_c... chromeos/printing/ppd_cache.h:49: const std::string& key) const = 0; On 2016/10/07 16:29:06, skau wrote: > Would this be the appropriate place to document what we expect the key to be? > Users of this function may not interact with Store* Obsolete https://codereview.chromium.org/2343983004/diff/80001/chromeos/printing/ppd_p... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2343983004/diff/80001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.cc:109: auto tmp = cache_->FindLocal(printer_id); On 2016/10/07 16:29:06, skau wrote: > Can you write out this type? It's not clear that it's Optional<FilePath>. Done. https://codereview.chromium.org/2343983004/diff/80001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.cc:136: if (parsed_json == nullptr || // On 2016/10/07 16:29:06, skau wrote: > are you forcing it to wrap with the comment //? Yes. I think it's easier to read this way, and AFAIK that's still the best way to force a line break with automatic formatting.
Please make it clear that you should create multiple PpdResolvers if you want to run concurrent requests. https://codereview.chromium.org/2343983004/diff/80001/chromeos/printing/ppd_c... File chromeos/printing/ppd_cache.cc (right): https://codereview.chromium.org/2343983004/diff/80001/chromeos/printing/ppd_c... chromeos/printing/ppd_cache.cc:29: const char* kLocalModel = "local"; On 2016/10/14 19:28:56, Carlson wrote: > On 2016/10/07 16:29:06, skau wrote: > > const char[] for string constants > > OK, but any particular reason why? Consistency. It looks to be the common interpretation of the style guide. https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache.cc (right): https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:38: // Return a ascii-hex version of the data in data, interpreted as a list of data in |data| https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:102: // Try to clean up the file, as it may have partial contents. Note leading spaces https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:139: FilePath GetCachePathBase(const Printer::PpdReference& ref) const { If user_supplied_url and effective_manufacturer is present do we want this to hash to a different value than if user_supplied_url is provided? This seems like a bad place to enforce precedence. Would it be better to have different functions for the different types of data that we store? https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:155: string ascii_hash = Hexify(crypto::SHA256HashString(full_key)); Would HexEncode do what you want here? https://cs.chromium.org/chromium/src/base/strings/string_number_conversions.h... https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache.h (right): https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.h:20: // it operates like a persistent hash from PpdReference to files. If hash? do you mean mapping? https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.h:21: // you give the same PpdReference to Find that was previously passed typo? https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache_unittest.cc (right): https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/ppd_... chromeos/printing/ppd_cache_unittest.cc:67: // Check that path has a value, and the contents of the referenced fule s/fule/file https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/ppd_... chromeos/printing/ppd_cache_unittest.cc:69: void CheckFileContentsAre(Optional<FilePath> result, Are? Contents match? https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:76: PpdProvider::ResolveCallback cb) override { Given what we're doing with fetcher_ we need a guard here to reject concurrent calls. https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:117: bool CachePpd(const Printer::PpdReference& ppd_reference, I liked having separate Store functions as it was clear how the entry was keyed. However, if it's awkward to have 2 Store functions and only 1 Resolve, I can see the logic there. https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider.h (right): https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.h:26: // CUPS-PostScript Printer Description (PPD) files. It provides ppd's that a ppds. No apostrophe. https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider_unittest.cc (right): https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/ppd_... chromeos/printing/ppd_provider_unittest.cc:28: #if 0 ? https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/prin... File chromeos/printing/printer_configuration.h (right): https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/prin... chromeos/printing/printer_configuration.h:21: // want to update PpdResolver and PpdCache::GetCachePath. We should include the resolution hierarchy in this comment. https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/prin... File chromeos/printing/printer_translator_unittest.cc (right): https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/prin... chromeos/printing/printer_translator_unittest.cc:107: // FIXME add a test that we don't set fields that are empty. FIXME
Addressed comments. If we do think this can be called concurrently, I'm going to also make this thread-safe, will re-mail when that's done. https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:76: PpdProvider::ResolveCallback cb) override { On 2016/10/14 22:10:16, skau wrote: > Given what we're doing with fetcher_ we need a guard here to reject concurrent > calls. That's something I've been unclear on. There will presumably be one of these per user/profile/whatever. Can we actually have concurrent usage on that, or is the print settings effectively non-concurrent? I thought it was serialized. If it's not, then I can convert the class to handle concurrency, I just didn't think that was needed. https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:117: bool CachePpd(const Printer::PpdReference& ppd_reference, On 2016/10/14 22:10:16, skau wrote: > I liked having separate Store functions as it was clear how the entry was keyed. > However, if it's awkward to have 2 Store functions and only 1 Resolve, I can > see the logic there. The issue is *normally* storing is invisible to the user -- it's a cacheing detail that should be handled entirely by the resolver. The exception to this is users manually adding a ppd, which is why we have this hook at all. https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider.h (right): https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.h:26: // CUPS-PostScript Printer Description (PPD) files. It provides ppd's that a On 2016/10/14 22:10:17, skau wrote: > ppds. No apostrophe. Done. https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider_unittest.cc (right): https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/ppd_... chromeos/printing/ppd_provider_unittest.cc:28: #if 0 On 2016/10/14 22:10:17, skau wrote: > ? Umm..umm...PAY NO ATTENTION TO THE COMMENTED OUT TESTS THAT I FORGOT TO REENABLE. :) (fixed and reenabled the tests, which led to a couple other bugfixes...) https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/prin... File chromeos/printing/printer_configuration.h (right): https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/prin... chromeos/printing/printer_configuration.h:21: // want to update PpdResolver and PpdCache::GetCachePath. On 2016/10/14 22:10:17, skau wrote: > We should include the resolution hierarchy in this comment. Done. https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/prin... File chromeos/printing/printer_translator_unittest.cc (right): https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/prin... chromeos/printing/printer_translator_unittest.cc:107: // FIXME add a test that we don't set fields that are empty. On 2016/10/14 22:10:17, skau wrote: > FIXME apparently chromium presubmits don't pickup FIXME? FIXME was stale (test is already in)
PpdProvider doesn't need to be thread safe. I'd just like some warning of accidental usage. Two more comments that got lost somehow. https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2343983004/diff/120001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:76: PpdProvider::ResolveCallback cb) override { On 2016/10/14 23:05:56, Carlson wrote: > On 2016/10/14 22:10:16, skau wrote: > > Given what we're doing with fetcher_ we need a guard here to reject concurrent > > calls. > > That's something I've been unclear on. There will presumably be one of these > per user/profile/whatever. Can we actually have concurrent usage on that, or is > the print settings effectively non-concurrent? > > I thought it was serialized. If it's not, then I can convert the class to > handle concurrency, I just didn't think that was needed. Right now, the only use is serial. But if we ever do prefetching, somebody is going to forget that and get some weird bugs. Just add a DCHECK(!fetcher_) to go along with the comment in the header file. https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache.cc (right): https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:30: char HexifyNibble(int nibble) { Is this equivalent to HexEncode? https://cs.chromium.org/chromium/src/base/strings/string_number_conversions.h... https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:139: FilePath GetCachePathBase(const Printer::PpdReference& ref) const { Is it desirable that { "user_supplied_ppd_url": "foo" } does not hash to the same thing as { "user_supplied_url": "foo", "effective_manufacturer": "eee", "effective_model": "m" }?
https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache.cc (right): https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:30: char HexifyNibble(int nibble) { On 2016/10/14 23:51:49, skau wrote: > Is this equivalent to HexEncode? > https://cs.chromium.org/chromium/src/base/strings/string_number_conversions.h... I spent seriously like 15 minutes looking for that before rolling my own because I knew it had to exist, but somehow never found it. Thanks! https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:139: FilePath GetCachePathBase(const Printer::PpdReference& ref) const { On 2016/10/14 23:51:49, skau wrote: > Is it desirable that { "user_supplied_ppd_url": "foo" } does not hash to the > same thing as { "user_supplied_url": "foo", "effective_manufacturer": "eee", > "effective_model": "m" }? I *think* so, but could be convinced otherwise. My basic thought here is that *any* change in what we know about a printer should cause us to re-run the resolution logic, as we might get a different answer this time. In the case of the user_supplied field not changing but the other fields changing, we should just re-resolve to the same thing again, I think. The other alternative that I considered as doing a cascading hash, in which we just hash the highest priority filled in fields and then stop. It would have slightly nicer behaviour in the case you cited, but seems like it might be a little more error-prone in the long term; I like the simplicity of the idea that changing a field means you miss in the cache as an underlying principle. If you want me to change it that way, will do.
skau@chromium.org changed reviewers: + thestig@chromium.org
Adding thestig@ for committer's review https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache.cc (right): https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:139: FilePath GetCachePathBase(const Printer::PpdReference& ref) const { On 2016/10/17 16:43:20, Carlson wrote: > On 2016/10/14 23:51:49, skau wrote: > > Is it desirable that { "user_supplied_ppd_url": "foo" } does not hash to the > > same thing as { "user_supplied_url": "foo", "effective_manufacturer": "eee", > > "effective_model": "m" }? > > I *think* so, but could be convinced otherwise. My basic thought here is that > *any* change in what we know about a printer should cause us to re-run the > resolution logic, as we might get a different answer this time. In the case of > the user_supplied field not changing but the other fields changing, we should > just re-resolve to the same thing again, I think. > > The other alternative that I considered as doing a cascading hash, in which we > just hash the highest priority filled in fields and then stop. It would have > slightly nicer behaviour in the case you cited, but seems like it might be a > little more error-prone in the long term; I like the simplicity of the idea that > changing a field means you miss in the cache as an underlying principle. > > If you want me to change it that way, will do. Please change it. While we will need to make sure that fields are grouped properly for a cascading hash, it doesn't make sense to update a file that we already have. We should be explicit about fetching updates since we'd want to avoid breaking a working configuration.
I only glanced at the CL. https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache.cc (right): https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:163: }; Add DISALLOW_COPY_AND_ASSIGN(...); https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache.h (right): https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.h:19: // PpdCache manages a cache of locally-stored ppd files. At its core, PPD https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.h:21: // you give the same PpdReference to Find that was previously passed Find() https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.h:27: struct Options { Can you add this once you actually have options? https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.h:28: // Nothing here yet. We may want to add ttl-style options here eventually Some people do not like using "we" in comments because it is unclear who "we" refers to. e.g. Here it may refer to the developers, but the "we" on line 38" refers to the code. What may be better is: TODO(username): Consider adding options for ... Otherwise, Find() will miss in the cache... https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.h:43: virtual base::Optional<base::FilePath> Find( Do you need the Optional? Can an empty FilePath represent a failure instead? Ditto below. https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache_unittest.cc (right): https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_cache_unittest.cc:19: using ::base::FilePath; What's the motivation for these? It's not like writing out "base::" is really going to make the code that much longer. https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_cache_unittest.cc:138: } // anonymous namespace s/annoymous // (Run git cl lint) https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:188: "https://", // Unneeded comments? https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:232: return ::base::MakeUnique<PpdProviderImpl>(api_key, url_context_getter, Most code just write base:Foo. Is there another base namespace somewhere that may cause confusion? https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider.h (right): https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.h:78: // cb will be called on the Network thread with the result of the lookup. |cb| https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/prin... File chromeos/printing/printer_configuration.h (right): https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/prin... chromeos/printing/printer_configuration.h:18: // Information needed to find the PPD file for this printer. Why is this still here? Didn't these changes land in https://codereview.chromium.org/2408293002/ already? Please rebased.
https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache.cc (right): https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:139: FilePath GetCachePathBase(const Printer::PpdReference& ref) const { On 2016/10/17 17:16:26, skau wrote: > On 2016/10/17 16:43:20, Carlson wrote: > > On 2016/10/14 23:51:49, skau wrote: > > > Is it desirable that { "user_supplied_ppd_url": "foo" } does not hash to the > > > same thing as { "user_supplied_url": "foo", "effective_manufacturer": "eee", > > > "effective_model": "m" }? > > > > I *think* so, but could be convinced otherwise. My basic thought here is that > > *any* change in what we know about a printer should cause us to re-run the > > resolution logic, as we might get a different answer this time. In the case > of > > the user_supplied field not changing but the other fields changing, we should > > just re-resolve to the same thing again, I think. > > > > The other alternative that I considered as doing a cascading hash, in which we > > just hash the highest priority filled in fields and then stop. It would have > > slightly nicer behaviour in the case you cited, but seems like it might be a > > little more error-prone in the long term; I like the simplicity of the idea > that > > changing a field means you miss in the cache as an underlying principle. > > > > If you want me to change it that way, will do. > > Please change it. While we will need to make sure that fields are grouped > properly for a cascading hash, it doesn't make sense to update a file that we > already have. We should be explicit about fetching updates since we'd want to > avoid breaking a working configuration. Done. https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:163: }; On 2016/10/17 17:54:06, Lei Zhang wrote: > Add DISALLOW_COPY_AND_ASSIGN(...); Done. https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache.h (right): https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.h:19: // PpdCache manages a cache of locally-stored ppd files. At its core, On 2016/10/17 17:54:06, Lei Zhang wrote: > PPD Done. https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.h:21: // you give the same PpdReference to Find that was previously passed On 2016/10/17 17:54:06, Lei Zhang wrote: > Find() Done. https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.h:27: struct Options { On 2016/10/17 17:54:06, Lei Zhang wrote: > Can you add this once you actually have options? Done. https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.h:28: // Nothing here yet. We may want to add ttl-style options here eventually On 2016/10/17 17:54:06, Lei Zhang wrote: > Some people do not like using "we" in comments because it is unclear who "we" > refers to. e.g. Here it may refer to the developers, but the "we" on line 38" > refers to the code. > > What may be better is: > > TODO(username): Consider adding options for ... > > Otherwise, Find() will miss in the cache... Not sure if use of vague "some people" in comment about vague pronoun was meant to be ironic or not. :) Anyways, removed options. https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.h:43: virtual base::Optional<base::FilePath> Find( On 2016/10/17 17:54:07, Lei Zhang wrote: > Do you need the Optional? Can an empty FilePath represent a failure instead? > Ditto below. We could do that, but I'm generally against special-return-values-meaning-failure as a software engineering practice. My first choice here was StatusOr<FilePath>, but we don't seem to have that in this code base, alas. https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache_unittest.cc (right): https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_cache_unittest.cc:19: using ::base::FilePath; On 2016/10/17 17:54:07, Lei Zhang wrote: > What's the motivation for these? It's not like writing out "base::" is really > going to make the code that much longer. Subjective readability? https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_cache_unittest.cc:138: } // anonymous namespace On 2016/10/17 17:54:07, Lei Zhang wrote: > s/annoymous // > > (Run git cl lint) I did run git cl lint and it doesn't pick this up. Weird. Removed. https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:188: "https://", // On 2016/10/17 17:54:07, Lei Zhang wrote: > Unneeded comments? clang-format line breaks. https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:232: return ::base::MakeUnique<PpdProviderImpl>(api_key, url_context_getter, On 2016/10/17 17:54:07, Lei Zhang wrote: > Most code just write base:Foo. Is there another base namespace somewhere that > may cause confusion? Done. https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider.h (right): https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.h:78: // cb will be called on the Network thread with the result of the lookup. On 2016/10/17 17:54:07, Lei Zhang wrote: > |cb| Done. https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/prin... File chromeos/printing/printer_configuration.h (right): https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/prin... chromeos/printing/printer_configuration.h:18: // Information needed to find the PPD file for this printer. On 2016/10/17 17:54:07, Lei Zhang wrote: > Why is this still here? Didn't these changes land in > https://codereview.chromium.org/2408293002/ already? Please rebased. Done.
https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache.h (right): https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.h:43: virtual base::Optional<base::FilePath> Find( On 2016/10/18 19:05:01, Carlson wrote: > On 2016/10/17 17:54:07, Lei Zhang wrote: > > Do you need the Optional? Can an empty FilePath represent a failure instead? > > Ditto below. > > We could do that, but I'm generally against > special-return-values-meaning-failure as a software engineering practice. Where do you draw the line there? Does malloc() returning NULL count as special-return-values-meaning-failure? Another way to think about it is: when does it make sense for Find() to return an empty FilePath? Wouldn't you want to check for that as an error condition? We didn't get base::Optional until 5 months ago. Somehow we survived without it before. https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache_unittest.cc (right): https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_cache_unittest.cc:19: using ::base::FilePath; On 2016/10/18 19:05:01, Carlson wrote: > On 2016/10/17 17:54:07, Lei Zhang wrote: > > What's the motivation for these? It's not like writing out "base::" is really > > going to make the code that much longer. > > Subjective readability? In general, I would suggest writing code to fit in with the established coding style, which is what other developers are used to seeing. If you are in the UK, and you keep writing "check" when everyone else writes "cheque" ... If you run: git grep '^using std::string;'|wc -l, it will return ~250. Most of those are clustered in a couple directories, where the subteam has decided they are going to do it that way in their little corner. For perspective, base/ alone has ~800 .cc files and there's only a single "using std::string;" statement.
https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache.h (right): https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.h:43: virtual base::Optional<base::FilePath> Find( On 2016/10/18 19:37:00, Lei Zhang wrote: > On 2016/10/18 19:05:01, Carlson wrote: > > On 2016/10/17 17:54:07, Lei Zhang wrote: > > > Do you need the Optional? Can an empty FilePath represent a failure instead? > > > Ditto below. > > > > We could do that, but I'm generally against > > special-return-values-meaning-failure as a software engineering practice. > > Where do you draw the line there? Does malloc() returning NULL count as > special-return-values-meaning-failure? > > Another way to think about it is: when does it make sense for Find() to return > an empty FilePath? Wouldn't you want to check for that as an error condition? > > We didn't get base::Optional until 5 months ago. Somehow we survived without it > before. If you want to litigate Optional as a useful tool, this is probably not the right forum. Optional is a perfectly useful and accepted tool. The fact that it is new to the chromium code base is more or less irrelevant -- we also survived without unique_ptr for a long time, as it was only accepted less than a year ago, but that doesn't mean it's a bad idea. The question of relevance here is whether this API makes sense. The biggest thing Optional buys us here is making it harder to accidentally use an error condition return value. If we use the "empty-file-path-means-error" convention, a naive caller that blindly uses the returned FilePath might get weird results that may or may not be easily trackable back to failing to check the result of this call. It also makes it immediately obvious from the function signature that you may *not* get a valid FilePath back from this function, which is also valuable, otherwise you are relying on users noticing documentation to call the function in the right way. The only "cost" to using Optional here is that the caller has to say .value() to get the FilePath out, which seems pretty minimal to me. If you want to draw a line in the sand because it's not your preference, so be it, but frankly I think you've straying pretty far from "is this reasonable code" and well into "does this code precisely match how I would implement it myself". https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache_unittest.cc (right): https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_cache_unittest.cc:19: using ::base::FilePath; On 2016/10/18 19:37:00, Lei Zhang wrote: > On 2016/10/18 19:05:01, Carlson wrote: > > On 2016/10/17 17:54:07, Lei Zhang wrote: > > > What's the motivation for these? It's not like writing out "base::" is > really > > > going to make the code that much longer. > > > > Subjective readability? > > In general, I would suggest writing code to fit in with the established coding > style, which is what other developers are used to seeing. If you are in the UK, > and you keep writing "check" when everyone else writes "cheque" ... > > If you run: git grep '^using std::string;'|wc -l, it will return ~250. Most of > those are clustered in a couple directories, where the subteam has decided they > are going to do it that way in their little corner. For perspective, base/ alone > has ~800 .cc files and there's only a single "using std::string;" statement. See earlier comment about "acceptable" vs "how you would write it". I'm well within Style Guide here (at least, I believe I am; if you can point to something that says otherwise I'll certainly change this).
https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache.h (right): https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.h:43: virtual base::Optional<base::FilePath> Find( On 2016/10/18 20:06:28, Carlson wrote: > If you want to litigate Optional as a useful tool, this is probably not the > right forum. Optional is a perfectly useful and accepted tool. The fact that > it is new to the chromium code base is more or less irrelevant -- we also > survived without unique_ptr for a long time, as it was only accepted less than a > year ago, but that doesn't mean it's a bad idea. The question of relevance here > is whether this API makes sense. You wrote "happy to take any suggestions" so I'm giving you suggestions. You wrote "I'm generally against ..." so I'm trying to understand your broad generalizations. It's your code here. If you feel Optional is correct here, and the other folks who have to work with your code are ok with it, then great, have fun. If you want to use Optional everywhere, then maybe you will have more litigation with other reviewers in the future. https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md is relevant here, but my interpretation doesn't sway the argument in either direction. Also, we have had unique_ptr as scoped_ptr in Chromium for ~10 years. It's not new, whereas base::Optional is. > The biggest thing Optional buys us here is making it harder to accidentally use > an error condition return value. If we use the "empty-file-path-means-error" > convention, a naive caller that blindly uses the returned FilePath might get > weird results that may or may not be easily trackable back to failing to check > the result of this call. > > It also makes it immediately obvious from the function signature that you may > *not* get a valid FilePath back from this function, which is also valuable, > otherwise you are relying on users noticing documentation to call the function > in the right way. By convention, in many places, an empty FilePath means "failure", "false", or "the end." See base::FileEnumerator::Next() and drive::util::ExtractDrivePath() as a couple selected examples. Basically anything in Chromium that deals with base::FilePath tends to use an empty FilePath to indicate failure. Yes, I'm sure at some point someone forgot to check the return result and created a bug, but in general, we are not losing sleep over this. Yes, this may corrolate with the fact that we didn't have Optional for a long time. But then again, the various FilePath API authors also didn't go out of their way make their code return a FilePath pointer. > The only "cost" to using Optional here is that the caller has to say .value() to > get the FilePath out, which seems pretty minimal to me. Without Optional, there is one less state to be concerned about: non-empty FilePath -> probably a valid path empty FilePath -> failure but with Optional, now we have: nullopt_t -> failure non-empty FilePath -> probably a valid path empty FilePath -> also a failure??? The caller now has one more possibility to worry about. If you add a comment and say "returned FilePath will never be empty" then ok, but see my comment above about convention. > If you want to draw a line in the sand because it's not your preference, so be > it, but frankly I think you've straying pretty far from "is this reasonable > code" and well into "does this code precisely match how I would implement it > myself". I never said you can't do this. Once again, I'm just trying to understand your position and point out what's more commonly done. To go with my probably not perfect UK analogy, if everyone else drives on the left, go with the flow. https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache_unittest.cc (right): https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_cache_unittest.cc:19: using ::base::FilePath; On 2016/10/18 20:06:28, Carlson wrote: > On 2016/10/18 19:37:00, Lei Zhang wrote: > > On 2016/10/18 19:05:01, Carlson wrote: > > > On 2016/10/17 17:54:07, Lei Zhang wrote: > > > > What's the motivation for these? It's not like writing out "base::" is > > really > > > > going to make the code that much longer. > > > > > > Subjective readability? > > > > In general, I would suggest writing code to fit in with the established coding > > style, which is what other developers are used to seeing. If you are in the > UK, > > and you keep writing "check" when everyone else writes "cheque" ... > > > > If you run: git grep '^using std::string;'|wc -l, it will return ~250. Most of > > those are clustered in a couple directories, where the subteam has decided > they > > are going to do it that way in their little corner. For perspective, base/ > alone > > has ~800 .cc files and there's only a single "using std::string;" statement. > > See earlier comment about "acceptable" vs "how you would write it". I'm well > within Style Guide here (at least, I believe I am; if you can point to something > that says otherwise I'll certainly change this). What I'm saying is "this is how 99% of the code is written." There are many things the Style Guide does not cover explicitly, but if you read the beginning of it, you'll see the points I'm trying to convey: - Optimize for the reader, not the writer - Be consistent with existing code
https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache.h (right): https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.h:43: virtual base::Optional<base::FilePath> Find( On 2016/10/18 22:25:34, Lei Zhang wrote: > On 2016/10/18 20:06:28, Carlson wrote: > > If you want to litigate Optional as a useful tool, this is probably not the > > right forum. Optional is a perfectly useful and accepted tool. The fact that > > it is new to the chromium code base is more or less irrelevant -- we also > > survived without unique_ptr for a long time, as it was only accepted less than > a > > year ago, but that doesn't mean it's a bad idea. The question of relevance > here > > is whether this API makes sense. > > You wrote "happy to take any suggestions" so I'm giving you suggestions. You > wrote "I'm generally against ..." so I'm trying to understand your broad > generalizations. Social subtleties don't always come across well in code review, and it sounds like I misinterpreted the spirit of this discussion from your end. Apologies for that. I do appreciate and recognize that we both have the same goal, that is making APIs sane from the get-go. > > The biggest thing Optional buys us here is making it harder to accidentally > use > > an error condition return value. If we use the "empty-file-path-means-error" > > convention, a naive caller that blindly uses the returned FilePath might get > > weird results that may or may not be easily trackable back to failing to check > > the result of this call. > > > > It also makes it immediately obvious from the function signature that you may > > *not* get a valid FilePath back from this function, which is also valuable, > > otherwise you are relying on users noticing documentation to call the function > > in the right way. > > By convention, in many places, an empty FilePath means "failure", "false", or > "the end." See base::FileEnumerator::Next() and drive::util::ExtractDrivePath() > as a couple selected examples. > > Basically anything in Chromium that deals with base::FilePath tends to use an > empty FilePath to indicate failure. Yes, I'm sure at some point someone forgot > to check the return result and created a bug, but in general, we are not losing > sleep over this. Yes, this may corrolate with the fact that we didn't have > Optional for a long time. But then again, the various FilePath API authors also > didn't go out of their way make their code return a FilePath pointer. I think it's fair to summarize the issue you bring up here as one of judging whether the benefits of using the New Thing (Optional) outweigh the implicit cost of having an API that deviates from the common usage (empty-FilePath-as-error-value). My hope is that, as better structs for these things become available, we *will* see APIs move to more explicit, type-enforced error values, including FilePath libraries. As I mentioned earlier, I think the *best* answer here would be StatusOr, as it has precisely the desired semantics -- "Either a FilePath or an error will be returned and you're going to get a compile error if you don't check that error...", but Optional gets us most of the way there by demuxing the error value from the return type. (As an aside, I was, until relatively recently, pretty unconvinced that StatusOr and friends were good ideas; mostly because they do tend to be quite verbose, especially in headers. But having worked for a while with code that uses them extensively, I'm firmly convinced that the added compiler support for ensuring that callsites are correct is well worth the syntactic overhead). Anyways, to make a long story short (too late) I'm quite convinced that, although the case is not clear-cut, Optional here is sufficiently better than bare FilePath that it's worth breaking with wider codebase convention. > > > The only "cost" to using Optional here is that the caller has to say .value() > to > > get the FilePath out, which seems pretty minimal to me. > > Without Optional, there is one less state to be concerned about: > > non-empty FilePath -> probably a valid path > empty FilePath -> failure > > but with Optional, now we have: > > nullopt_t -> failure > non-empty FilePath -> probably a valid path > empty FilePath -> also a failure??? The latter case would definitely be considered a library bug. Updated comment to be more explicit about this. > > > If you want to draw a line in the sand because it's not your preference, so be > > it, but frankly I think you've straying pretty far from "is this reasonable > > code" and well into "does this code precisely match how I would implement it > > myself". > > I never said you can't do this. Once again, I'm just trying to understand your > position and point out what's more commonly done. To go with my probably not > perfect UK analogy, if everyone else drives on the left, go with the flow. Again, apologies for the overly aggressive reply. https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache_unittest.cc (right): https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_... chromeos/printing/ppd_cache_unittest.cc:19: using ::base::FilePath; On 2016/10/18 22:25:34, Lei Zhang wrote: > On 2016/10/18 20:06:28, Carlson wrote: > > On 2016/10/18 19:37:00, Lei Zhang wrote: > > > On 2016/10/18 19:05:01, Carlson wrote: > > > > On 2016/10/17 17:54:07, Lei Zhang wrote: > > > > > What's the motivation for these? It's not like writing out "base::" is > > > really > > > > > going to make the code that much longer. > > > > > > > > Subjective readability? > > > > > > In general, I would suggest writing code to fit in with the established > coding > > > style, which is what other developers are used to seeing. If you are in the > > UK, > > > and you keep writing "check" when everyone else writes "cheque" ... > > > > > > If you run: git grep '^using std::string;'|wc -l, it will return ~250. Most > of > > > those are clustered in a couple directories, where the subteam has decided > > they > > > are going to do it that way in their little corner. For perspective, base/ > > alone > > > has ~800 .cc files and there's only a single "using std::string;" statement. > > > > See earlier comment about "acceptable" vs "how you would write it". I'm well > > within Style Guide here (at least, I believe I am; if you can point to > something > > that says otherwise I'll certainly change this). > > What I'm saying is "this is how 99% of the code is written." There are many > things the Style Guide does not cover explicitly, but if you read the beginning > of it, you'll see the points I'm trying to convey: > > - Optimize for the reader, not the writer > - Be consistent with existing code Sometimes what I really want is a "Disagree, but did it anyways" quick response. :) (Done)
Description was changed from ========== Add PPDProvider barebones implementation and associated cache skeleton. Add Unit test framework for PPDProvider (unit test doesn't test much yet) Cache implementation is currently TBD, pending discussions around syncing. This is probably very wrong in many ways, since it's my first chromium patch, happy to take any suggestions including "do this in a completely different way". BUG=617254 ========== to ========== Add initial PpdProvider implementation and associated PpdCache. Also add unit tests for both. BUG=617254 ==========
https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache.cc (right): https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:9: #include "base/path_service.h" Not used? https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:12: #include "chromeos/printing/ppd_cache.h" This should go above everything else. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:13: #include "crypto/sha2.h" Not used? https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:18: using base::File; Can we be consistent with the changes to the unit test? BTW, if you ever have to work with "namespace data_reduction_proxy" then ya, you'd want to add "using data_reduction_proxy" or else fitting under 80 cols is going to be hard. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:66: contents_path = GetCachePathBase(reference).AddExtension(".ppd"); combine with previous line? https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:100: // there's also no real cost to being paranoid here, so we use SHA-256 as the Is this out of date or not implemented? https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:125: LOG(DFATAL) << "PpdCache hashing empty PpdReference, this probably means " NOTREACHED() https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:146: return ::base::MakeUnique<PpdCacheImpl>(cache_base_dir); More ::base https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache.h (right): https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.h:41: // Take the contents of a ppd file, store it to the cache, and return the more s/ppd/PPD/ https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache_unittest.cc (right): https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_cache_unittest.cc:138: EXPECT_EQ(cache->Store(ref, kTestPpdContents).value().Extension(), ".ppd"); Arguments should be flipped. See line 71. As is, gtest will print out funny messages on failure. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:4: #include "chromeos/printing/ppd_provider.h" blank line above. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:71: CHECK_GT(options_.max_ppd_contents_size_, static_cast<size_t>(0)); You may be able to just write "0U" and avoid the static_cast. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:77: if (fetcher_ != nullptr) { fop != nullptr -> foo bar == nullptr -> !bar https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:79: // Even though the caller has done something wrong, try to do something See the "CHECK(), DCHECK(), and NOTREACHED()" philosophy on https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md - maybe you don't want to bother handling this? https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:137: // Scope the allocated fetcher_ into this function so we clean it up when |fetcher_| https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:149: auto parsed_json = ::base::JSONReader::Read(contents); You can take advantage of base::DictionaryValue::From(). https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:151: if (parsed_json == nullptr || // I also have not seen anyone else do this. It's an ugly hack but I don't have any better suggestions right now, aside from base::DictionaryValue::From() will let you avoid this particular case. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:153: // Malformed response. TODO(justincarlson) - LOG something here? Or let the caller handle it since it needs to check for SERVER_ERROR anyway? BTW, "TODO(username): Comment" is the most common format. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:196: "https://", // Again, I don't struggle with clang-format, so I don't know what to do here. Maybe it would look better if you do base::StringPrintf()? https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:227: void ForwardingURLFetcherDelegate::OnURLFetchComplete( Since you implemented PpdProviderImpl methods inline, do that for this too? Especially since it's a one liner. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider_unittest.cc (right): https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider_unittest.cc:78: void CaptureResolveResult(CapturedResolveResult* capture, Can this be CaptureResolveResultCallback? It's a bit hard to differenciate from CapturedResolveResult. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider_unittest.cc:91: EXPECT_EQ(captured.result, PpdProvider::SUCCESS); More arguments to reverse. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider_unittest.cc:120: FilePath contents_path = temp_dir.GetPath().Append("response"); BTW, base::FilePath::Append(c_str) is fine here, but does not work cross platform. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider_unittest.cc:122: CHECK_GT(base::WriteFile(contents_path, kQuirksResponse, Maybe: int bytes_written = base::WriteFile(...); CHECK_GT(bytes_written, 0); Since it takes the same # of lines without the lonely 0. Or better yet, make sure this test wrote out strlen(kQuirksResponse).
Mostly resolved, but need more info on a couple of things (particularly ppd_provider.cc:79 and ppd_provider.cc:151) https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache.cc (right): https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:9: #include "base/path_service.h" On 2016/10/19 01:25:09, Lei Zhang wrote: > Not used? Thanks. Is there any iwyu tooling that's easy to invoke? I'm sure there are others that could be cleaned up. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:12: #include "chromeos/printing/ppd_cache.h" On 2016/10/19 01:25:09, Lei Zhang wrote: > This should go above everything else. Done. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:13: #include "crypto/sha2.h" On 2016/10/19 01:25:09, Lei Zhang wrote: > Not used? Oh boy, this was actually a serious error on my part -- I forgot to actually...hash, and was just hexencoding the to-be-hashed key instead of the hashed key. It'd be nice to have a unit test to catch something like this if I could think of a reasonable way to structure one. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:18: using base::File; On 2016/10/19 01:25:09, Lei Zhang wrote: > Can we be consistent with the changes to the unit test? > > BTW, if you ever have to work with "namespace data_reduction_proxy" then ya, > you'd want to add "using data_reduction_proxy" or else fitting under 80 cols is > going to be hard. As a compromise, I removed the ones that were seldom used below, but left the ones that are used many times. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:66: contents_path = GetCachePathBase(reference).AddExtension(".ppd"); On 2016/10/19 01:25:09, Lei Zhang wrote: > combine with previous line? Done. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:100: // there's also no real cost to being paranoid here, so we use SHA-256 as the On 2016/10/19 01:25:09, Lei Zhang wrote: > Is this out of date or not implemented? Noted above, forgot to implement. Thanks for noticing this. It's in now. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:125: LOG(DFATAL) << "PpdCache hashing empty PpdReference, this probably means " On 2016/10/19 01:25:09, Lei Zhang wrote: > NOTREACHED() Sure. Out of curiosity, what's the difference? Is it just that it tags the log with "NOTREACHED hit." (and maybe is a little better macro name for intent here?) https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:146: return ::base::MakeUnique<PpdCacheImpl>(cache_base_dir); On 2016/10/19 01:25:09, Lei Zhang wrote: > More ::base Done. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache.h (right): https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.h:41: // Take the contents of a ppd file, store it to the cache, and return the On 2016/10/19 01:25:10, Lei Zhang wrote: > more s/ppd/PPD/ Done. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache_unittest.cc (right): https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_cache_unittest.cc:138: EXPECT_EQ(cache->Store(ref, kTestPpdContents).value().Extension(), ".ppd"); On 2016/10/19 01:25:10, Lei Zhang wrote: > Arguments should be flipped. See line 71. As is, gtest will print out funny > messages on failure. Done. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:4: #include "chromeos/printing/ppd_provider.h" On 2016/10/19 01:25:10, Lei Zhang wrote: > blank line above. Done. Am I misrunning lint somehow? I would expect 'git cl lint' to turn up stuff like this. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:71: CHECK_GT(options_.max_ppd_contents_size_, static_cast<size_t>(0)); On 2016/10/19 01:25:10, Lei Zhang wrote: > You may be able to just write "0U" and avoid the static_cast. Done. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:77: if (fetcher_ != nullptr) { On 2016/10/19 01:25:10, Lei Zhang wrote: > fop != nullptr -> foo > bar == nullptr -> !bar Acknowledged. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:79: // Even though the caller has done something wrong, try to do something On 2016/10/19 01:25:10, Lei Zhang wrote: > See the "CHECK(), DCHECK(), and NOTREACHED()" philosophy on > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md - > maybe you don't want to bother handling this? Thanks for the link, it's good to know the consensus view here. Initially I didn't implement the fallback because I thought this was an appropriate place to fail fast and crash the browser if someone violates the threading model. skau@ thought it was not reasonable to crash in this circumstance (a view that I also think has significant merits -- you can certainly envision a release that has code that triggers this check only very infrequently, but frequently enough to be annoying for a substantial number of users) So whats your ultimate opinion about reasonableness-to-crash here? If you think it's not reasonable, I'll just LOG(ERROR) it, otherwise I'll remove the fallback and change it to LOG(FATAL). (semi-relatedly, is there a way in this code review tool to flag a specific comment response as needing further followup? I don't see any obvious way to mark comments as resolved or not) https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:137: // Scope the allocated fetcher_ into this function so we clean it up when On 2016/10/19 01:25:10, Lei Zhang wrote: > |fetcher_| Done. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:149: auto parsed_json = ::base::JSONReader::Read(contents); On 2016/10/19 01:25:10, Lei Zhang wrote: > You can take advantage of base::DictionaryValue::From(). Thanks for the pointer. Done, but I don't feel entirely ok about it; From() doesn't actually explicitly enumerate what "if it is a dictionary" means w.r.t. nullptr. The implementation does seems to do what we want, though. It just seems like borderline 'let's depend on the implementation details and not the stated contract'. This seems more likely to be a comment oversight than something that is likely to change though. (Famous last words...) https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:151: if (parsed_json == nullptr || // On 2016/10/19 01:25:10, Lei Zhang wrote: > I also have not seen anyone else do this. It's an ugly hack but I don't have any > better suggestions right now, aside from base::DictionaryValue::From() will let > you avoid this particular case. Sorry, not clear on what 'this' refers to here. Can you clarify? https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:153: // Malformed response. TODO(justincarlson) - LOG something here? On 2016/10/19 01:25:10, Lei Zhang wrote: > Or let the caller handle it since it needs to check for SERVER_ERROR anyway? > > BTW, "TODO(username): Comment" is the most common format. Good point, removed TODO https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:196: "https://", // On 2016/10/19 01:25:10, Lei Zhang wrote: > Again, I don't struggle with clang-format, so I don't know what to do here. > Maybe it would look better if you do base::StringPrintf()? (isn't clang-format what's underneath 'git cl format'? Maybe I have that wrong.) Converted to StringPrintf, though I think it's marginally less readable than vertically-aligned-interspersed. Admittedly, some of the readability is dependent on syntax highlighting though -- I think the vertical version is a lot easier to understand immediately when the literals are a different color from the std::string's. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:227: void ForwardingURLFetcherDelegate::OnURLFetchComplete( On 2016/10/19 01:25:10, Lei Zhang wrote: > Since you implemented PpdProviderImpl methods inline, do that for this too? > Especially since it's a one liner. I wish I could, but I can't because of recursive dependencies -- PpdProviderImpl contains a ForwardingURLFetcherDelegate, and this function invokes OnURLFetchComplete, so one of the functions needs to be out of line. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider_unittest.cc (right): https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider_unittest.cc:78: void CaptureResolveResult(CapturedResolveResult* capture, On 2016/10/19 01:25:10, Lei Zhang wrote: > Can this be CaptureResolveResultCallback? It's a bit hard to differenciate from > CapturedResolveResult. Done. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider_unittest.cc:91: EXPECT_EQ(captured.result, PpdProvider::SUCCESS); On 2016/10/19 01:25:10, Lei Zhang wrote: > More arguments to reverse. Done. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider_unittest.cc:120: FilePath contents_path = temp_dir.GetPath().Append("response"); On 2016/10/19 01:25:10, Lei Zhang wrote: > BTW, base::FilePath::Append(c_str) is fine here, but does not work cross > platform. Acknowledged. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider_unittest.cc:122: CHECK_GT(base::WriteFile(contents_path, kQuirksResponse, On 2016/10/19 01:25:10, Lei Zhang wrote: > Maybe: > > int bytes_written = base::WriteFile(...); > CHECK_GT(bytes_written, 0); > > Since it takes the same # of lines without the lonely 0. > > Or better yet, make sure this test wrote out strlen(kQuirksResponse). Done.
https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache.cc (right): https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:9: #include "base/path_service.h" On 2016/10/19 16:38:26, Carlson wrote: > On 2016/10/19 01:25:09, Lei Zhang wrote: > > Not used? > > Thanks. Is there any iwyu tooling that's easy to invoke? I'm sure there are > others that could be cleaned up. They exist but they don't work well for Chromium in general. IWYU tools tend to have trouble with platform-specific code / includes. Though this is ChromeOS only so maybe they'll work here? https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:125: LOG(DFATAL) << "PpdCache hashing empty PpdReference, this probably means " On 2016/10/19 16:38:26, Carlson wrote: > On 2016/10/19 01:25:09, Lei Zhang wrote: > > NOTREACHED() > > Sure. Out of curiosity, what's the difference? Is it just that it tags the log > with "NOTREACHED hit." (and maybe is a little better macro name for intent > here?) NOTREACHED() better describes the intent. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:4: #include "chromeos/printing/ppd_provider.h" On 2016/10/19 16:38:27, Carlson wrote: > On 2016/10/19 01:25:10, Lei Zhang wrote: > > blank line above. > > Done. Am I misrunning lint somehow? I would expect 'git cl lint' to turn up > stuff like this. Indeed, rather strange. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:79: // Even though the caller has done something wrong, try to do something On 2016/10/19 16:38:26, Carlson wrote: > On 2016/10/19 01:25:10, Lei Zhang wrote: > > See the "CHECK(), DCHECK(), and NOTREACHED()" philosophy on > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md > - > > maybe you don't want to bother handling this? > > Thanks for the link, it's good to know the consensus view here. > > Initially I didn't implement the fallback because I thought this was an > appropriate place to fail fast and crash the browser if someone violates the > threading model. skau@ thought it was not reasonable to crash in this > circumstance (a view that I also think has significant merits -- you can > certainly envision a release that has code that triggers this check only very > infrequently, but frequently enough to be annoying for a substantial number of > users) > > So whats your ultimate opinion about reasonableness-to-crash here? If you think > it's not reasonable, I'll just LOG(ERROR) it, otherwise I'll remove the fallback > and change it to LOG(FATAL). "A consequence of this is that you should not handle DCHECK() failures, even if failure would result in a crash." Just crash. If you reach this point because there is bad code, give up and crash. Users will be annoyed. They will file bugs. Or it will show up on the crash server and someone will take notice. Best case the bugs will get fixed before reaching stable channel. If you do NOTREACHED() and handle it, then in release builds, code execution just blows through the NOTREACHED(), and the caller may or may not notice the INTERNAL_ERROR. The bug may stay masked for a long time and developers won't even realize there is a problem. > (semi-relatedly, is there a way in this code review tool to flag a specific > comment response as needing further followup? I don't see any obvious way to > mark comments as resolved or not) Don't think so. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:149: auto parsed_json = ::base::JSONReader::Read(contents); On 2016/10/19 16:38:27, Carlson wrote: > On 2016/10/19 01:25:10, Lei Zhang wrote: > > You can take advantage of base::DictionaryValue::From(). > > Thanks for the pointer. > > Done, but I don't feel entirely ok about it; From() doesn't actually explicitly > enumerate what "if it is a dictionary" means w.r.t. nullptr. The implementation > does seems to do what we want, though. It just seems like borderline 'let's > depend on the implementation details and not the stated contract'. > > This seems more likely to be a comment oversight than something that is likely > to change though. (Famous last words...) Apparently I approved the CL that added that comment, so it makes sense to me. Nobody else has complainted about it AFAIK. I would like to hear why you are not ok with it and what can be done to improve it. Comments / (separate) patches welcome. Given: // Returns |value| if it is a dictionary, nullptr otherwise. static std::unique_ptr<DictionaryValue> From(std::unique_ptr<Value> value); Without peeking at the impl, my reading of the comment + signature is as follows: if (!value || !value->IsType(TYPE_DICTIONARY)) return std::unique_ptr<DictionaryValue>(); return std::unique_ptr<DictionaryValue>(value.release()); // Or std::move() or whatever to make this work. Is the comment unclear? Do you see other interpretations? https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:151: if (parsed_json == nullptr || // On 2016/10/19 16:38:27, Carlson wrote: > On 2016/10/19 01:25:10, Lei Zhang wrote: > > I also have not seen anyone else do this. It's an ugly hack but I don't have > any > > better suggestions right now, aside from base::DictionaryValue::From() will > let > > you avoid this particular case. > > Sorry, not clear on what 'this' refers to here. Can you clarify? Putting "//" at the end to avoid formatting. It, uh... sticks out.
I think I've addressed everything so far. If I missed something, apologies; it's a little hard to tell what's unresolved. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache.cc (right): https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:9: #include "base/path_service.h" On 2016/10/19 23:15:45, Lei Zhang wrote: > On 2016/10/19 16:38:26, Carlson wrote: > > On 2016/10/19 01:25:09, Lei Zhang wrote: > > > Not used? > > > > Thanks. Is there any iwyu tooling that's easy to invoke? I'm sure there are > > others that could be cleaned up. > > They exist but they don't work well for Chromium in general. IWYU tools tend to > have trouble with platform-specific code / includes. Though this is ChromeOS > only so maybe they'll work here? Acknowledged. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:125: LOG(DFATAL) << "PpdCache hashing empty PpdReference, this probably means " On 2016/10/19 23:15:45, Lei Zhang wrote: > On 2016/10/19 16:38:26, Carlson wrote: > > On 2016/10/19 01:25:09, Lei Zhang wrote: > > > NOTREACHED() > > > > Sure. Out of curiosity, what's the difference? Is it just that it tags the > log > > with "NOTREACHED hit." (and maybe is a little better macro name for intent > > here?) > > NOTREACHED() better describes the intent. Acknowledged. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:4: #include "chromeos/printing/ppd_provider.h" On 2016/10/19 23:15:46, Lei Zhang wrote: > On 2016/10/19 16:38:27, Carlson wrote: > > On 2016/10/19 01:25:10, Lei Zhang wrote: > > > blank line above. > > > > Done. Am I misrunning lint somehow? I would expect 'git cl lint' to turn up > > stuff like this. > > Indeed, rather strange. Acknowledged. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:79: // Even though the caller has done something wrong, try to do something On 2016/10/19 23:15:46, Lei Zhang wrote: > On 2016/10/19 16:38:26, Carlson wrote: > > On 2016/10/19 01:25:10, Lei Zhang wrote: > > > See the "CHECK(), DCHECK(), and NOTREACHED()" philosophy on > > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md > > - > > > maybe you don't want to bother handling this? > > > > Thanks for the link, it's good to know the consensus view here. > > > > Initially I didn't implement the fallback because I thought this was an > > appropriate place to fail fast and crash the browser if someone violates the > > threading model. skau@ thought it was not reasonable to crash in this > > circumstance (a view that I also think has significant merits -- you can > > certainly envision a release that has code that triggers this check only very > > infrequently, but frequently enough to be annoying for a substantial number of > > users) > > > > So whats your ultimate opinion about reasonableness-to-crash here? If you > think > > it's not reasonable, I'll just LOG(ERROR) it, otherwise I'll remove the > fallback > > and change it to LOG(FATAL). > > "A consequence of this is that you should not handle DCHECK() failures, even if > failure would result in a crash." > > Just crash. If you reach this point because there is bad code, give up and > crash. Users will be annoyed. They will file bugs. Or it will show up on the > crash server and someone will take notice. Best case the bugs will get fixed > before reaching stable channel. > > If you do NOTREACHED() and handle it, then in release builds, code execution > just blows through the NOTREACHED(), and the caller may or may not notice the > INTERNAL_ERROR. The bug may stay masked for a long time and developers won't > even realize there is a problem. > > > (semi-relatedly, is there a way in this code review tool to flag a specific > > comment response as needing further followup? I don't see any obvious way to > > mark comments as resolved or not) > > Don't think so. Done. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:149: auto parsed_json = ::base::JSONReader::Read(contents); On 2016/10/19 23:15:46, Lei Zhang wrote: > On 2016/10/19 16:38:27, Carlson wrote: > > On 2016/10/19 01:25:10, Lei Zhang wrote: > > > You can take advantage of base::DictionaryValue::From(). > > > > Thanks for the pointer. > > > > Done, but I don't feel entirely ok about it; From() doesn't actually > explicitly > > enumerate what "if it is a dictionary" means w.r.t. nullptr. The > implementation > > does seems to do what we want, though. It just seems like borderline 'let's > > depend on the implementation details and not the stated contract'. > > > > This seems more likely to be a comment oversight than something that is likely > > to change though. (Famous last words...) > > Apparently I approved the CL that added that comment, so it makes sense to me. > Nobody else has complainted about it AFAIK. I would like to hear why you are not > ok with it and what can be done to improve it. Comments / (separate) patches > welcome. > > Given: > > // Returns |value| if it is a dictionary, nullptr otherwise. > static std::unique_ptr<DictionaryValue> From(std::unique_ptr<Value> value); > > Without peeking at the impl, my reading of the comment + signature is as > follows: > > if (!value || !value->IsType(TYPE_DICTIONARY)) > return std::unique_ptr<DictionaryValue>(); > return std::unique_ptr<DictionaryValue>(value.release()); // Or std::move() or > whatever to make this work. > > Is the comment unclear? Do you see other interpretations? Ironically, I think this is a case of special-value-conventions-leading-to-unclear-semantics. :) It's not clear from the comment whether nullptr is in the set of "inputs that might reasonably be dictionaries" or the set of "garbage input" (e.g. something that might crash the function. In other words, would suggest that the comment should read something like "Returns |value| if it is non-null and a dictionary, nullptr otherwise". I'll probably send a patch for this when I get a minute. https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:151: if (parsed_json == nullptr || // On 2016/10/19 23:15:46, Lei Zhang wrote: > On 2016/10/19 16:38:27, Carlson wrote: > > On 2016/10/19 01:25:10, Lei Zhang wrote: > > > I also have not seen anyone else do this. It's an ugly hack but I don't have > > any > > > better suggestions right now, aside from base::DictionaryValue::From() will > > let > > > you avoid this particular case. > > > > Sorry, not clear on what 'this' refers to here. Can you clarify? > > Putting "//" at the end to avoid formatting. It, uh... sticks out. Acknowledged.
https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:149: auto parsed_json = ::base::JSONReader::Read(contents); On 2016/10/19 23:38:56, Carlson wrote: > Ironically, I think this is a case of > special-value-conventions-leading-to-unclear-semantics. :) It's not clear from > the comment whether nullptr is in the set of "inputs that might reasonably be > dictionaries" or the set of "garbage input" (e.g. something that might crash the > function. Given it was added to deal with all the places where people write "if (value && value->IsDict() then cast..." ya, it does the nullptr check. For fun, survey 10 Chromium developers and see if they think base::DictionaryValue::From(std::unique_ptr<base::Value>()) would return a st::unique_ptr<base::DictionaryValue>(), or crash? > In other words, would suggest that the comment should read something like > "Returns |value| if it is non-null and a dictionary, nullptr otherwise". I'll > probably send a patch for this when I get a minute. > Sure, can't hurt. Please note there's at least 1 other From() method in that file.
On 2016/10/19 23:51:55, Lei Zhang wrote: > https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... > File chromeos/printing/ppd_provider.cc (right): > > https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_... > chromeos/printing/ppd_provider.cc:149: auto parsed_json = > ::base::JSONReader::Read(contents); > On 2016/10/19 23:38:56, Carlson wrote: > > Ironically, I think this is a case of > > special-value-conventions-leading-to-unclear-semantics. :) It's not clear > from > > the comment whether nullptr is in the set of "inputs that might reasonably be > > dictionaries" or the set of "garbage input" (e.g. something that might crash > the > > function. > > Given it was added to deal with all the places where people write "if (value && > value->IsDict() then cast..." ya, it does the nullptr check. > > For fun, survey 10 Chromium developers and see if they think > base::DictionaryValue::From(std::unique_ptr<base::Value>()) would return a > st::unique_ptr<base::DictionaryValue>(), or crash? I would argue that the most relevant audience for API comments is actually developers *not* familiar with the codebase. So I put up a G+ poll, and am curious what will happen. Totally scientific, I'm sure. > > > In other words, would suggest that the comment should read something like > > "Returns |value| if it is non-null and a dictionary, nullptr otherwise". I'll > > probably send a patch for this when I get a minute. > > > > Sure, can't hurt. Please note there's at least 1 other From() method in that > file. OK, will do, but in the meantime, I can haz approval? Or was there something else outstanding that you want addressed?
On 2016/10/20 17:39:29, Carlson wrote: > I would argue that the most relevant audience for API comments is actually > developers *not* familiar with the codebase. So I put up a G+ poll, and am > curious what will happen. Totally scientific, I'm sure. Totally! :) > OK, will do, but in the meantime, I can haz approval? Or was there something > else outstanding that you want addressed? skau@ first? I haven't looked again, but probably all I would have are nitpicks.
I'll take a look. I was waiting for the style issues to be arbitrated. :) On Thu, Oct 20, 2016, 2:28 PM <thestig@chromium.org> wrote: > On 2016/10/20 17:39:29, Carlson wrote: > > I would argue that the most relevant audience for API comments is > actually > > developers *not* familiar with the codebase. So I put up a G+ poll, and > am > > curious what will happen. Totally scientific, I'm sure. > > Totally! :) > > > > OK, will do, but in the meantime, I can haz approval? Or was there > something > > else outstanding that you want addressed? > > skau@ first? I haven't looked again, but probably all I would have are > nitpicks. > > https://codereview.chromium.org/2343983004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/10/21 00:26:21, skau wrote: > I'll take a look. I was waiting for the style issues to be arbitrated. :) That's doing it in the wrong order. Work out the CL for correctness first.
Just once concern regarding the whether we should post the callback from the PpdProvider back to the original thread or if it's okay to run it on the network thread. And a few nits. https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache.cc (right): https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:70: // Try to clean up the file, as it may have partial contents. Note spaces before Try https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:120: << "means something is wrong"; nit: noting that something is wrong is implied by NOTREACHED. But it is funnier this way. https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache.h (right): https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.h:48: // and remain valid until the next Store* call. Store* https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache_unittest.cc (right): https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_... chromeos/printing/ppd_cache_unittest.cc:62: // Check that path has a value, and the contents of the referenced fule /fule/file/ https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_... chromeos/printing/ppd_cache_unittest.cc:63: // are contents. |expected_contents|? https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:94: url_context_getter_->GetNetworkTaskRunner()->PostTask( Are we trying to post the callback to the network thread rather than the calling thread? https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:122: options_.max_ppd_contents_size_)) { nit: {} is commonly excluded from single statements following an if in the Chromium codebase. It's just a style preference. https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:176: done_callback_.Run(PpdProvider::SUCCESS, ppd_file.value()); Can you double check that the callback can be invoked from the network thread?
Just once concern regarding the whether we should post the callback from the PpdProvider back to the original thread or if it's okay to run it on the network thread. And a few nits.
On 2016/10/21 00:37:51, Lei Zhang wrote: > On 2016/10/21 00:26:21, skau wrote: > > I'll take a look. I was waiting for the style issues to be arbitrated. :) > > That's doing it in the wrong order. Work out the CL for correctness first. What I meant was that I spot checked it earlier but was waiting for an issue to and I looped you in concurrently. I didn't want to jump in to lgtm during the discussion.
On 2016/10/21 00:37:51, Lei Zhang wrote: > On 2016/10/21 00:26:21, skau wrote: > > I'll take a look. I was waiting for the style issues to be arbitrated. :) > > That's doing it in the wrong order. Work out the CL for correctness first. What I meant was that I spot checked it earlier but was waiting for an issue to and I looped you in concurrently. I didn't want to jump in to lgtm during the discussion.
On 2016/10/21 00:37:51, Lei Zhang wrote: > On 2016/10/21 00:26:21, skau wrote: > > I'll take a look. I was waiting for the style issues to be arbitrated. :) > > That's doing it in the wrong order. Work out the CL for correctness first. What I meant was that I spot checked it earlier but was waiting for an issue to and I looped you in concurrently. I didn't want to jump in to lgtm during the discussion.
On 2016/10/21 00:37:51, Lei Zhang wrote: > On 2016/10/21 00:26:21, skau wrote: > > I'll take a look. I was waiting for the style issues to be arbitrated. :) > > That's doing it in the wrong order. Work out the CL for correctness first. What I meant was that I spot checked it earlier but was waiting for an issue to and I looped you in concurrently. I didn't want to jump in to lgtm during the discussion.
not lgtm
https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache.cc (right): https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:70: // Try to clean up the file, as it may have partial contents. Note On 2016/10/21 16:12:56, skau wrote: > spaces before Try Done. https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.cc:120: << "means something is wrong"; On 2016/10/21 16:12:56, skau wrote: > nit: noting that something is wrong is implied by NOTREACHED. But it is funnier > this way. Decreased comedy level. https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache.h (right): https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_... chromeos/printing/ppd_cache.h:48: // and remain valid until the next Store* call. On 2016/10/21 16:12:56, skau wrote: > Store* Done. https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache_unittest.cc (right): https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_... chromeos/printing/ppd_cache_unittest.cc:62: // Check that path has a value, and the contents of the referenced fule On 2016/10/21 16:12:56, skau wrote: > /fule/file/ Done. https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_... chromeos/printing/ppd_cache_unittest.cc:63: // are contents. On 2016/10/21 16:12:56, skau wrote: > |expected_contents|? Done. https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:94: url_context_getter_->GetNetworkTaskRunner()->PostTask( On 2016/10/21 16:12:57, skau wrote: > Are we trying to post the callback to the network thread rather than the calling > thread? I thought I had seen that URLFetcher always invokes things on the network thread, so I was trying to match that behavior. However, I just found https://cs.chromium.org/chromium/src/net/url_request/url_fetcher.h?rcl=0&l=67 that says otherwise, so converted to just do the simpler call-from-the-current-thread thing. There's a reasonable argument that we should still post this as a task to the current runner instead of running it directly, but I think it should be ok to just run directly. https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:122: options_.max_ppd_contents_size_)) { On 2016/10/21 16:12:57, skau wrote: > nit: {} is commonly excluded from single statements following an if in the > Chromium codebase. It's just a style preference. Having been bitten one too many times by bugs of the form "let's add a statement to this if oh crap it wasn't in the if because no braces", I'd rather leave this as is. https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:176: done_callback_.Run(PpdProvider::SUCCESS, ppd_file.value()); On 2016/10/21 16:12:56, skau wrote: > Can you double check that the callback can be invoked from the network thread? Don't think this is relevant now.
lgtm Thanks for putting up with all the comments. https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:94: url_context_getter_->GetNetworkTaskRunner()->PostTask( On 2016/10/24 17:32:33, Carlson wrote: > On 2016/10/21 16:12:57, skau wrote: > > Are we trying to post the callback to the network thread rather than the > calling > > thread? > > I thought I had seen that URLFetcher always invokes things on the network > thread, so I was trying to match that behavior. However, I just found > > https://cs.chromium.org/chromium/src/net/url_request/url_fetcher.h?rcl=0&l=67 > > that says otherwise, so converted to just do the simpler > call-from-the-current-thread thing. > > There's a reasonable argument that we should still post this as a task to the > current runner instead of running it directly, but I think it should be ok to > just run directly. Please continue to post the callback on the current thread. It's easier to program against the PpdProvider if callback always runs after the message loop completes. You can used SequencedTaskRunner::Get() to post a runnable on the current task runner.
https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:94: url_context_getter_->GetNetworkTaskRunner()->PostTask( On 2016/10/24 17:55:33, skau wrote: > On 2016/10/24 17:32:33, Carlson wrote: > > On 2016/10/21 16:12:57, skau wrote: > > > Are we trying to post the callback to the network thread rather than the > > calling > > > thread? > > > > I thought I had seen that URLFetcher always invokes things on the network > > thread, so I was trying to match that behavior. However, I just found > > > > https://cs.chromium.org/chromium/src/net/url_request/url_fetcher.h?rcl=0&l=67 > > > > that says otherwise, so converted to just do the simpler > > call-from-the-current-thread thing. > > > > There's a reasonable argument that we should still post this as a task to the > > current runner instead of running it directly, but I think it should be ok to > > just run directly. > Please continue to post the callback on the current thread. It's easier to > program against the PpdProvider if callback always runs after the message loop > completes. You can used SequencedTaskRunner::Get() to post a runnable on the > current task runner. My instincts definitely run the other way here. I think code that does something like * Resolve() * ...do other stuff needed to handle the callback correctly... * return to message loop is likely to be fragile in the future and so should be disallowed. But OK, done. (Note we discussed offline that this means we don't have thread affinity, only sequencing).
The CQ bit was checked by justincarlson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skau@chromium.org Link to the patchset: https://codereview.chromium.org/2343983004/#ps340001 (title: "Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th…")
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
On 2016/10/24 20:18:36, commit-bot: I haz the power wrote: > No L-G-T-M from a valid reviewer yet. > CQ run can only be started by full committers or once the patch has > received an L-G-T-M from a full committer. > Even if an L-G-T-M may have been provided, it was from a non-committer, > _not_ a full super star committer. > See http://www.chromium.org/getting-involved/become-a-committer > Note that this has nothing to do with OWNERS files. Oops, I still need approval from thestig@, I think.
https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache_unittest.cc (right): https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... chromeos/printing/ppd_cache_unittest.cc:48: PpdCacheTest() { CHECK(ppd_cache_temp_dir_.CreateUniqueTempDir()); } Many unit tests override SetUp() and do: ASSERT_TRUE(foo_dir_.CreateUniqueTempDir()); It may allow gtest to clean up slightly nicer than a CHECK() in the rare cases when CreateUniqueTempDir() fails? https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:26: using ::base::FilePath; BTW, I thought we agreed to omit the leading "::" https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:55: PpdProviderImpl* owner_; PpdProviderImpl* const owner_; https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:131: // Called on the network thread when the fetcher completes its fetch. Can base::SequenceChecker help make sure you are on the right sequence here? https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider.h (right): https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.h:62: // Maximum size of the contents of a PPD file. Trying to use a PPD file Presumably: ... of a PPD file in bytes. https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider_unittest.cc (right): https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... chromeos/printing/ppd_provider_unittest.cc:46: CHECK(ppd_cache_temp_dir_.CreateUniqueTempDir()); Also move to SetUp() and ASSERT_TRUE() instead? https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... chromeos/printing/ppd_provider_unittest.cc:49: // Create the ppd_provider_ to be used for this test. |ppd_provider_| https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... chromeos/printing/ppd_provider_unittest.cc:66: scoped_refptr<::net::URLRequestContextGetter> request_context_getter_; No leading "::". No other code refers to URLRequestContextGetter that way. https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... chromeos/printing/ppd_provider_unittest.cc:112: GURL expected_url(string("https://") + kTestQuirksServer // foo + bar + qux is preferred over foo +bar +qux https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... chromeos/printing/ppd_provider_unittest.cc:112: GURL expected_url(string("https://") + kTestQuirksServer // Using empty comments to force formatting still feels wrong. Imagine what the code base would look like if everyone did this to get their piece of code formatted just the way they like it. https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... chromeos/printing/ppd_provider_unittest.cc:124: CHECK_EQ(bytes_written, static_cast<int>(strlen(kQuirksResponse))); ASSERT_EQ?
https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... File chromeos/printing/ppd_cache_unittest.cc (right): https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... chromeos/printing/ppd_cache_unittest.cc:48: PpdCacheTest() { CHECK(ppd_cache_temp_dir_.CreateUniqueTempDir()); } On 2016/10/24 22:26:24, Lei Zhang wrote: > Many unit tests override SetUp() and do: > > ASSERT_TRUE(foo_dir_.CreateUniqueTempDir()); > > It may allow gtest to clean up slightly nicer than a CHECK() in the rare cases > when CreateUniqueTempDir() fails? Good point, done. https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:26: using ::base::FilePath; On 2016/10/24 22:26:24, Lei Zhang wrote: > BTW, I thought we agreed to omit the leading "::" Oh, I thought you just meant inline. Sure, done. https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:55: PpdProviderImpl* owner_; On 2016/10/24 22:26:24, Lei Zhang wrote: > PpdProviderImpl* const owner_; Done. https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:131: // Called on the network thread when the fetcher completes its fetch. On 2016/10/24 22:26:24, Lei Zhang wrote: > Can base::SequenceChecker help make sure you are on the right sequence here? Done. https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider.h (right): https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.h:62: // Maximum size of the contents of a PPD file. Trying to use a PPD file On 2016/10/24 22:26:24, Lei Zhang wrote: > Presumably: ... of a PPD file in bytes. Done. https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider_unittest.cc (right): https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... chromeos/printing/ppd_provider_unittest.cc:46: CHECK(ppd_cache_temp_dir_.CreateUniqueTempDir()); On 2016/10/24 22:26:24, Lei Zhang wrote: > Also move to SetUp() and ASSERT_TRUE() instead? Done. https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... chromeos/printing/ppd_provider_unittest.cc:49: // Create the ppd_provider_ to be used for this test. On 2016/10/24 22:26:24, Lei Zhang wrote: > |ppd_provider_| Comment was sort of pointless anyways, so removed. https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... chromeos/printing/ppd_provider_unittest.cc:66: scoped_refptr<::net::URLRequestContextGetter> request_context_getter_; On 2016/10/24 22:26:24, Lei Zhang wrote: > No leading "::". No other code refers to URLRequestContextGetter that way. Done, cleaned up a few others, too. https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... chromeos/printing/ppd_provider_unittest.cc:112: GURL expected_url(string("https://") + kTestQuirksServer // On 2016/10/24 22:26:24, Lei Zhang wrote: > foo + > bar + > qux > > is preferred over > > foo > +bar > +qux Done. https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... chromeos/printing/ppd_provider_unittest.cc:112: GURL expected_url(string("https://") + kTestQuirksServer // On 2016/10/24 22:26:24, Lei Zhang wrote: > Using empty comments to force formatting still feels wrong. Imagine what the > code base would look like if everyone did this to get their piece of code > formatted just the way they like it. Acknowledged. https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... chromeos/printing/ppd_provider_unittest.cc:124: CHECK_EQ(bytes_written, static_cast<int>(strlen(kQuirksResponse))); On 2016/10/24 22:26:24, Lei Zhang wrote: > ASSERT_EQ? Done.
https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider_unittest.cc (right): https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... chromeos/printing/ppd_provider_unittest.cc:112: GURL expected_url(string("https://") + kTestQuirksServer // On 2016/10/24 22:48:44, Carlson wrote: > On 2016/10/24 22:26:24, Lei Zhang wrote: > > Using empty comments to force formatting still feels wrong. Imagine what the > > code base would look like if everyone did this to get their piece of code > > formatted just the way they like it. > > Acknowledged. So does base::StringPrintf() help here like it did in ppd_provider.cc? If yes, great, let's go with that. If not, can you please remove the comments, and let clang-format do its thing? It's what all of chromeos/ does so it would be nice to be consistent. Once again, this is a creative use of comments, but nobody else does this. https://codereview.chromium.org/2343983004/diff/360001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2343983004/diff/360001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:209: // Check that Resolve and its callback are sequenced appropriately. Resolve()
https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider_unittest.cc (right): https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... chromeos/printing/ppd_provider_unittest.cc:112: GURL expected_url(string("https://") + kTestQuirksServer // On 2016/10/24 23:33:43, Lei Zhang wrote: > On 2016/10/24 22:48:44, Carlson wrote: > > On 2016/10/24 22:26:24, Lei Zhang wrote: > > > Using empty comments to force formatting still feels wrong. Imagine what the > > > code base would look like if everyone did this to get their piece of code > > > formatted just the way they like it. > > > > Acknowledged. > > So does base::StringPrintf() help here like it did in ppd_provider.cc? If yes, > great, let's go with that. If not, can you please remove the comments, and let > clang-format do its thing? It's what all of chromeos/ does so it would be nice > to be consistent. Once again, this is a creative use of comments, but nobody > else does this. I think base::StringPrintf is less readable here than vertical formatting. Pulled the lines back a bit to pair url prefixes with associated variable contents, which I actually think looks a little better. We seem to have fundamentally incompatible views about how "not commonly done" should inform the structure of new code. While I think it's a factor in deciding how to do things, I don't think it should totally override personal judgement on complicated expressions (unless, of course style guide says otherwise). https://codereview.chromium.org/2343983004/diff/360001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2343983004/diff/360001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:209: // Check that Resolve and its callback are sequenced appropriately. On 2016/10/24 23:33:43, Lei Zhang wrote: > Resolve() Done.
- I really thinks we have spent way too much time arguing over some not so important things. - Part of why clang-format exists is to stop the bike shedding. Let the computer do the formatting and stop worrying about it. - I would be happy to chat about this if you think actual talking will help us reach an agreement. Or if you'd like, we can go up a directory or two to chromeos/OWNERS or top level OWNERS and get their input. Whatever you'd like to help us break the impasse. https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider_unittest.cc (right): https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... chromeos/printing/ppd_provider_unittest.cc:112: GURL expected_url(string("https://") + kTestQuirksServer // On 2016/10/25 00:04:01, Carlson wrote: > I think base::StringPrintf is less readable here than vertical formatting. Ack. > Pulled the lines back a bit to pair url prefixes with associated variable > contents, which I actually think looks a little better. > > We seem to have fundamentally incompatible views about how "not commonly done" > should inform the structure of new code. While I think it's a factor in > deciding how to do things, I don't think it should totally override personal > judgement on complicated expressions (unless, of course style guide says > otherwise). The factors that I'm considering are: - You strongly prefer this style. (I'm not against listing things vertically) - Listing arguments vertically sometimes does make it more readable. (I've certainly done this in cases, and even had clang-format format it away) - In chromeos/ the top level OWNERS have stated they want the directory clang-formatted, as is the case in many, but not all Chromium code. - Even though this is a newly created file, it lives in chromeos/ and should stick with chromeos/ rules. It is part of a large project and at some point other people will have to read / modify this code. For their benefit, it would be good to "be consistent with existing code" and stick with the expectations set in the larger project. - I can't find another example in the code base where someone checked in code that does this to defeat clang-format. Of the above, consistency is the one that weighs most heavily to me, as seen at the parting words of the C++ style guide: "Use common sense and BE CONSISTENT. If you are editing code, take a few minutes to look at the code around you and determine its style. If they use spaces around their if clauses, you should, too. If their comments have little boxes of stars around them, make your comments have little boxes of stars around them too."
https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider_unittest.cc (right): https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_... chromeos/printing/ppd_provider_unittest.cc:112: GURL expected_url(string("https://") + kTestQuirksServer // On 2016/10/25 00:33:44, Lei Zhang wrote: > On 2016/10/25 00:04:01, Carlson wrote: > > I think base::StringPrintf is less readable here than vertical formatting. > > Ack. > > > Pulled the lines back a bit to pair url prefixes with associated variable > > contents, which I actually think looks a little better. > > > > We seem to have fundamentally incompatible views about how "not commonly done" > > should inform the structure of new code. While I think it's a factor in > > deciding how to do things, I don't think it should totally override personal > > judgement on complicated expressions (unless, of course style guide says > > otherwise). > > The factors that I'm considering are: > > - You strongly prefer this style. (I'm not against listing things vertically) > - Listing arguments vertically sometimes does make it more readable. (I've > certainly done this in cases, and even had clang-format format it away) > - In chromeos/ the top level OWNERS have stated they want the directory > clang-formatted, as is the case in many, but not all Chromium code. > - Even though this is a newly created file, it lives in chromeos/ and should > stick with chromeos/ rules. It is part of a large project and at some point > other people will have to read / modify this code. For their benefit, it would > be good to "be consistent with existing code" and stick with the expectations > set in the larger project. > - I can't find another example in the code base where someone checked in code > that does this to defeat clang-format. > > Of the above, consistency is the one that weighs most heavily to me, as seen at > the parting words of the C++ style guide: > > "Use common sense and BE CONSISTENT. > > If you are editing code, take a few minutes to look at the code around you and > determine its style. If they use spaces around their if clauses, you should, > too. If their comments have little boxes of stars around them, make your > comments have little boxes of stars around them too." Took discussion offline, but made change.
lgtm
The CQ bit was checked by thestig@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 justincarlson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skau@chromium.org Link to the patchset: https://codereview.chromium.org/2343983004/#ps400001 (title: "Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th…")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add initial PpdProvider implementation and associated PpdCache. Also add unit tests for both. BUG=617254 ========== to ========== Add initial PpdProvider implementation and associated PpdCache. Also add unit tests for both. BUG=617254 ==========
Message was sent while issue was closed.
Committed patchset #21 (id:400001)
Message was sent while issue was closed.
Description was changed from ========== Add initial PpdProvider implementation and associated PpdCache. Also add unit tests for both. BUG=617254 ========== to ========== Add initial PpdProvider implementation and associated PpdCache. Also add unit tests for both. BUG=617254 Committed: https://crrev.com/c7d4aa6cfd11244553fb6308434587149a8518ba Cr-Commit-Position: refs/heads/master@{#427353} ==========
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/c7d4aa6cfd11244553fb6308434587149a8518ba Cr-Commit-Position: refs/heads/master@{#427353} |