Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2891643002:
Add a method to query IPP printers for attributes. (Closed)
DescriptionAdd a method to query IPP printers for attributes.
We determine if a printer can be configured without a PPD by checking
the supported IPP version and some other attributes. Additionally, we
fetch the printer-make-and-model that can be used to attempt to lookup a
PPD.
BUG=685676
Review-Url: https://codereview.chromium.org/2891643002
Cr-Commit-Position: refs/heads/master@{#478065}
Committed: https://chromium.googlesource.com/chromium/src/+/bafbc3fe297223ffbef775053a1407910ce730e6
Patch Set 1 #
Total comments: 23
Patch Set 2 : fix compilation #
Total comments: 4
Patch Set 3 : add thread assertions #
Total comments: 16
Patch Set 4 : address comments #
Total comments: 6
Patch Set 5 : comments addressed #
Total comments: 5
Patch Set 6 : more robust parsing #Patch Set 7 : remove statics #
Total comments: 10
Patch Set 8 : comments addressed #Patch Set 9 : bug fixes and review comments #Patch Set 10 : minor revision #
Total comments: 2
Patch Set 11 : check empty #
Dependent Patchsets: Messages
Total messages: 35 (11 generated)
skau@chromium.org changed reviewers: + justincarlson@chromium.org
Can you take a first look?
https://codereview.chromium.org/2891643002/diff/1/chrome/browser/chromeos/pri... File chrome/browser/chromeos/printing/printer_info.h (right): https://codereview.chromium.org/2891643002/diff/1/chrome/browser/chromeos/pri... chrome/browser/chromeos/printing/printer_info.h:25: void QueryPrinter(const std::string& host, Function name should probably reflect the fact that this is IPP. https://codereview.chromium.org/2891643002/diff/1/chrome/browser/chromeos/pri... File chrome/browser/chromeos/printing/printer_info_cups.cc (right): https://codereview.chromium.org/2891643002/diff/1/chrome/browser/chromeos/pri... chrome/browser/chromeos/printing/printer_info_cups.cc:20: // Returns true if any of the |ipp_versions| are greater than or equal to 2.0. what's the pair? <major, minor> ? https://codereview.chromium.org/2891643002/diff/1/chrome/browser/chromeos/pri... chrome/browser/chromeos/printing/printer_info_cups.cc:35: std::unique_ptr<::printing::PrinterInfo> QueryPrinterImpl( Function comment? https://codereview.chromium.org/2891643002/diff/1/chrome/browser/chromeos/pri... chrome/browser/chromeos/printing/printer_info_cups.cc:63: (first_space + 1) < make_and_model.length()) { I don't think the second half of this conditional is needed? If we found a space, then first_space will be between [0, length-1]. If first_space is length-1, then first_space+1 is length, which would cause make_and_model.substr(first_space + 1) to be an empty string(piece), which is what you want. Am I missing something? https://codereview.chromium.org/2891643002/diff/1/chrome/browser/chromeos/pri... chrome/browser/chromeos/printing/printer_info_cups.cc:83: A comment on why you want these specific traits would be useful. https://codereview.chromium.org/2891643002/diff/1/printing/backend/cups_jobs.cc File printing/backend/cups_jobs.cc (right): https://codereview.chromium.org/2891643002/diff/1/printing/backend/cups_jobs.... printing/backend/cups_jobs.cc:355: std::pair<int, int> ToVersionNumber(const std::string& version, bool* success) { Function comment? (Also, shouldn't these free functions be in the anonymous namespace?) https://codereview.chromium.org/2891643002/diff/1/printing/backend/cups_jobs.... printing/backend/cups_jobs.cc:355: std::pair<int, int> ToVersionNumber(const std::string& version, bool* success) { Not sure this is a formal thing, but it seems like the more common idiom for a function like this is to return the bool and pass the std::pair<int, int> as a pointer, not the reverse. I don't have data to back that up, though. https://codereview.chromium.org/2891643002/diff/1/printing/backend/cups_jobs.... printing/backend/cups_jobs.cc:358: DCHECK_EQ(2U, pieces.size()); This is input validation, and so can't be a DCHECK, I think. If a printer gives you a stupid string back, it shouldn't crash the browser. https://codereview.chromium.org/2891643002/diff/1/printing/backend/cups_jobs.... printing/backend/cups_jobs.cc:364: return {major, minor}; Structure this so that we return a known bad major,minor tuple in the case of lack of success? The caller shouldn't use the values in that case, but returning uninitialized memory gives me heeby-jeebies. (This sort of function seems like a good use case for StatusOr, if we supported that in the main browser code. :P) https://codereview.chromium.org/2891643002/diff/1/printing/backend/cups_jobs.... printing/backend/cups_jobs.cc:374: if (name.empty()) { This case is not explicitly needed, but if you like it, cool. https://codereview.chromium.org/2891643002/diff/1/printing/backend/cups_jobs.... printing/backend/cups_jobs.cc:378: if (name == kPrinterMakeAndModel) { Paranoid favor, can you make the StringPiece conversion explicit when you're using '=='? i.e. if (name == StringPiece(kPrinterMakeAndModel)) Otherwise, if someone changes the type of 'name' to const char * (or auto), things will still compile but break in surprising ways. Not that that's ever happened to me. Nope. Never. And it wasn't a real PITA to debug, either. https://codereview.chromium.org/2891643002/diff/1/printing/backend/cups_jobs.h File printing/backend/cups_jobs.h (right): https://codereview.chromium.org/2891643002/diff/1/printing/backend/cups_jobs.... printing/backend/cups_jobs.h:147: // request to populate |printer_info|. Returns false if the request failed. This is a long latency function with no callback, that should be noted in the comment.
Description was changed from ========== Add a method to query IPP printers for attributes. We determine if a printer can be configured without a PPD by checking the supported IPP version and some other attributes. This information is also useful for validating that a printer is reachable. BUG=685676 ========== to ========== Add a method to query IPP printers for attributes. We determine if a printer can be configured without a PPD by checking the supported IPP version and some other attributes. Additionally, we fetch the printer-make-and-model that can be used to attempt to lookup a PPD. BUG=685676 ==========
Description was changed from ========== Add a method to query IPP printers for attributes. We determine if a printer can be configured without a PPD by checking the supported IPP version and some other attributes. Additionally, we fetch the printer-make-and-model that can be used to attempt to lookup a PPD. BUG=685676 ========== to ========== Add a method to query IPP printers for attributes. We determine if a printer can be configured without a PPD by checking the supported IPP version and some other attributes. Additionally, we fetch the printer-make-and-model that can be used to attempt to lookup a PPD. BUG=685676 ==========
Thanks for the review. I've had a chance to pick this up again. PTAL. https://codereview.chromium.org/2891643002/diff/1/chrome/browser/chromeos/pri... File chrome/browser/chromeos/printing/printer_info.h (right): https://codereview.chromium.org/2891643002/diff/1/chrome/browser/chromeos/pri... chrome/browser/chromeos/printing/printer_info.h:25: void QueryPrinter(const std::string& host, On 2017/05/25 19:04:35, Carlson wrote: > Function name should probably reflect the fact that this is IPP. Done. https://codereview.chromium.org/2891643002/diff/1/chrome/browser/chromeos/pri... File chrome/browser/chromeos/printing/printer_info_cups.cc (right): https://codereview.chromium.org/2891643002/diff/1/chrome/browser/chromeos/pri... chrome/browser/chromeos/printing/printer_info_cups.cc:20: // Returns true if any of the |ipp_versions| are greater than or equal to 2.0. On 2017/05/25 19:04:35, Carlson wrote: > what's the pair? <major, minor> ? Done. https://codereview.chromium.org/2891643002/diff/1/chrome/browser/chromeos/pri... chrome/browser/chromeos/printing/printer_info_cups.cc:35: std::unique_ptr<::printing::PrinterInfo> QueryPrinterImpl( On 2017/05/25 19:04:35, Carlson wrote: > Function comment? Done. https://codereview.chromium.org/2891643002/diff/1/chrome/browser/chromeos/pri... chrome/browser/chromeos/printing/printer_info_cups.cc:63: (first_space + 1) < make_and_model.length()) { On 2017/05/25 19:04:36, Carlson wrote: > I don't think the second half of this conditional is needed? > > If we found a space, then first_space will be between [0, length-1]. > > If first_space is length-1, then first_space+1 is length, which would cause > make_and_model.substr(first_space + 1) to be an empty string(piece), which is > what you want. > > Am I missing something? Yes. You are correct. The primary concern is a trailing space which would result in an empty string. https://codereview.chromium.org/2891643002/diff/1/chrome/browser/chromeos/pri... chrome/browser/chromeos/printing/printer_info_cups.cc:83: On 2017/05/25 19:04:35, Carlson wrote: > A comment on why you want these specific traits would be useful. Done. https://codereview.chromium.org/2891643002/diff/1/printing/backend/cups_jobs.cc File printing/backend/cups_jobs.cc (right): https://codereview.chromium.org/2891643002/diff/1/printing/backend/cups_jobs.... printing/backend/cups_jobs.cc:355: std::pair<int, int> ToVersionNumber(const std::string& version, bool* success) { On 2017/05/25 19:04:36, Carlson wrote: > Not sure this is a formal thing, but it seems like the more common idiom for a > function like this is to return the bool and pass the std::pair<int, int> as a > pointer, not the reverse. > > I don't have data to back that up, though. I've seen both but returning a bool has been more common for us. https://codereview.chromium.org/2891643002/diff/1/printing/backend/cups_jobs.... printing/backend/cups_jobs.cc:358: DCHECK_EQ(2U, pieces.size()); On 2017/05/25 19:04:36, Carlson wrote: > This is input validation, and so can't be a DCHECK, I think. If a printer gives > you a stupid string back, it shouldn't crash the browser. Done. https://codereview.chromium.org/2891643002/diff/1/printing/backend/cups_jobs.... printing/backend/cups_jobs.cc:364: return {major, minor}; On 2017/05/25 19:04:36, Carlson wrote: > Structure this so that we return a known bad major,minor tuple in the case of > lack of success? The caller shouldn't use the values in that case, but > returning uninitialized memory gives me heeby-jeebies. > > (This sort of function seems like a good use case for StatusOr, if we supported > that in the main browser code. :P) Done. https://codereview.chromium.org/2891643002/diff/1/printing/backend/cups_jobs.... printing/backend/cups_jobs.cc:374: if (name.empty()) { On 2017/05/25 19:04:36, Carlson wrote: > This case is not explicitly needed, but if you like it, cool. Done. https://codereview.chromium.org/2891643002/diff/1/printing/backend/cups_jobs.... printing/backend/cups_jobs.cc:378: if (name == kPrinterMakeAndModel) { On 2017/05/25 19:04:36, Carlson wrote: > Paranoid favor, can you make the StringPiece conversion explicit when you're > using '=='? i.e. if (name == StringPiece(kPrinterMakeAndModel)) > > Otherwise, if someone changes the type of 'name' to const char * (or auto), > things will still compile but break in surprising ways. > > Not that that's ever happened to me. Nope. Never. And it wasn't a real PITA > to debug, either. Sounds like a traumatic experience. Let's avoid that. https://codereview.chromium.org/2891643002/diff/1/printing/backend/cups_jobs.h File printing/backend/cups_jobs.h (right): https://codereview.chromium.org/2891643002/diff/1/printing/backend/cups_jobs.... printing/backend/cups_jobs.h:147: // request to populate |printer_info|. Returns false if the request failed. On 2017/05/25 19:04:36, Carlson wrote: > This is a long latency function with no callback, that should be noted in the > comment. I've added a file comment since it applies to all the functions.
https://codereview.chromium.org/2891643002/diff/20001/printing/backend/cups_j... File printing/backend/cups_jobs.cc (right): https://codereview.chromium.org/2891643002/diff/20001/printing/backend/cups_j... printing/backend/cups_jobs.cc:331: DCHECK_EQ(IPP_TAG_TEXT, ippGetValueTag(attr)); Is DCHECK the right thing here? If the condition fails and we're not in debug mode, what could happen? https://codereview.chromium.org/2891643002/diff/20001/printing/backend/cups_j... File printing/backend/cups_jobs.h (right): https://codereview.chromium.org/2891643002/diff/20001/printing/backend/cups_j... printing/backend/cups_jobs.h:20: // operations block on the network request and cannot be run on the UI thread. Can you add assertions to the relevant functions about not running on the UI thread? I'm sure someone's going to do it anyways by accident. :)
skau@chromium.org changed reviewers: + thestig@chromium.org
Added thread assertions. thestig@: PTAL printing/backend/* https://codereview.chromium.org/2891643002/diff/20001/printing/backend/cups_j... File printing/backend/cups_jobs.cc (right): https://codereview.chromium.org/2891643002/diff/20001/printing/backend/cups_j... printing/backend/cups_jobs.cc:331: DCHECK_EQ(IPP_TAG_TEXT, ippGetValueTag(attr)); On 2017/05/30 17:36:41, Carlson wrote: > Is DCHECK the right thing here? If the condition fails and we're not in debug > mode, what could happen? I believe that DCHECK is correct. This condition is not expected to happen unless the library changed and will crash on ippGetString. https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md https://codereview.chromium.org/2891643002/diff/20001/printing/backend/cups_j... File printing/backend/cups_jobs.h (right): https://codereview.chromium.org/2891643002/diff/20001/printing/backend/cups_j... printing/backend/cups_jobs.h:20: // operations block on the network request and cannot be run on the UI thread. On 2017/05/30 17:36:42, Carlson wrote: > Can you add assertions to the relevant functions about not > running on the UI thread? I'm sure someone's going to do it anyways by > accident. :) I'm looking into it. There's a migration away from thread assertions in favor of using the TaskScheduler.
https://codereview.chromium.org/2891643002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_info_cups.cc (right): https://codereview.chromium.org/2891643002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_info_cups.cc:77: // TODO(skau): Handle manufacturers with two word names. What happens if a user hits this case? https://codereview.chromium.org/2891643002/diff/40001/printing/backend/cups_j... File printing/backend/cups_jobs.cc (right): https://codereview.chromium.org/2891643002/diff/40001/printing/backend/cups_j... printing/backend/cups_jobs.cc:108: using IppPtr = std::unique_ptr<ipp_t, void (*)(ipp_t*)>; How about ScopedIppPtr here and ScopedHttpPtr below, to make it clearer what type of smart pointers they are? https://codereview.chromium.org/2891643002/diff/40001/printing/backend/cups_j... printing/backend/cups_jobs.cc:307: bool ToVersionNumber(const std::string& version, Do you want to use base::Version and not bother doing your own parsing? https://codereview.chromium.org/2891643002/diff/40001/printing/backend/cups_j... printing/backend/cups_jobs.cc:395: base::ThreadRestrictions::AssertIOAllowed(); Check this first. Ditto on line 480. If you look at base::ThreadRestrictions::AssertIOAllowed() usage elsewhere, we usually check what thread we are on first, before doing any other checks. Same for DCHECK_CURRENTLY_ON() in chrome/ as another set of examples. https://codereview.chromium.org/2891643002/diff/40001/printing/backend/cups_j... printing/backend/cups_jobs.cc:462: auto response = Can we put the actual type here, since it's not immediately clear what GetPrinterAttributes() returns? Ditto above on line 448. https://codereview.chromium.org/2891643002/diff/40001/printing/backend/cups_j... File printing/backend/cups_jobs.h (right): https://codereview.chromium.org/2891643002/diff/40001/printing/backend/cups_j... printing/backend/cups_jobs.h:154: // Queries the printer at |address| on |port| with a Get-Printer-Attributes Curious why this take [address, port] rather than a http_t* like the other functions.
Thanks for the reviews. Comments addressed. PTAL. https://codereview.chromium.org/2891643002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_info_cups.cc (right): https://codereview.chromium.org/2891643002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_info_cups.cc:77: // TODO(skau): Handle manufacturers with two word names. On 2017/05/31 00:48:03, Lei Zhang wrote: > What happens if a user hits this case? The manufacturer name gets spliced and added to the model name. https://codereview.chromium.org/2891643002/diff/40001/printing/backend/cups_j... File printing/backend/cups_jobs.cc (right): https://codereview.chromium.org/2891643002/diff/40001/printing/backend/cups_j... printing/backend/cups_jobs.cc:108: using IppPtr = std::unique_ptr<ipp_t, void (*)(ipp_t*)>; On 2017/05/31 00:48:03, Lei Zhang wrote: > How about ScopedIppPtr here and ScopedHttpPtr below, to make it clearer what > type of smart pointers they are? Done. https://codereview.chromium.org/2891643002/diff/40001/printing/backend/cups_j... printing/backend/cups_jobs.cc:307: bool ToVersionNumber(const std::string& version, On 2017/05/31 00:48:03, Lei Zhang wrote: > Do you want to use base::Version and not bother doing your own parsing? I didn't see that. Thanks! https://codereview.chromium.org/2891643002/diff/40001/printing/backend/cups_j... printing/backend/cups_jobs.cc:395: base::ThreadRestrictions::AssertIOAllowed(); On 2017/05/31 00:48:03, Lei Zhang wrote: > Check this first. Ditto on line 480. > > If you look at base::ThreadRestrictions::AssertIOAllowed() usage elsewhere, we > usually check what thread we are on first, before doing any other checks. Same > for DCHECK_CURRENTLY_ON() in chrome/ as another set of examples. Done. https://codereview.chromium.org/2891643002/diff/40001/printing/backend/cups_j... printing/backend/cups_jobs.cc:462: auto response = On 2017/05/31 00:48:03, Lei Zhang wrote: > Can we put the actual type here, since it's not immediately clear what > GetPrinterAttributes() returns? Ditto above on line 448. Done. https://codereview.chromium.org/2891643002/diff/40001/printing/backend/cups_j... File printing/backend/cups_jobs.h (right): https://codereview.chromium.org/2891643002/diff/40001/printing/backend/cups_j... printing/backend/cups_jobs.h:154: // Queries the printer at |address| on |port| with a Get-Printer-Attributes On 2017/05/31 00:48:03, Lei Zhang wrote: > Curious why this take [address, port] rather than a http_t* like the other > functions. The other functions can reuse http_t since they're querying the same IPP server repeatedly. This function builds its own http_t because the connection isn't reused right now.
https://codereview.chromium.org/2891643002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_info_cups.cc (right): https://codereview.chromium.org/2891643002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_info_cups.cc:77: // TODO(skau): Handle manufacturers with two word names. On 2017/05/31 18:37:51, skau wrote: > On 2017/05/31 00:48:03, Lei Zhang wrote: > > What happens if a user hits this case? > > The manufacturer name gets spliced and added to the model name. What I mean is, if a user has a "Hewlett Packard 5000" printer, and they hit this code while registering their printer, do they end up stuck with a "Hewlett" / "Packard 5000" printer that they can't change the name of? https://codereview.chromium.org/2891643002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_info_cups.cc (right): https://codereview.chromium.org/2891643002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_info_cups.cc:26: const std::vector<uint32_t> components = {2, 0}; Make these 2 consts static. See the storage duration section of http://en.cppreference.com/w/cpp/language/storage_duration https://codereview.chromium.org/2891643002/diff/60001/printing/backend/cups_j... File printing/backend/cups_jobs.cc (right): https://codereview.chromium.org/2891643002/diff/60001/printing/backend/cups_j... printing/backend/cups_jobs.cc:15: #include "base/strings/string_number_conversions.h" Probably don't need this and string_split.h anymore. https://codereview.chromium.org/2891643002/diff/60001/printing/backend/cups_j... File printing/backend/cups_jobs.h (right): https://codereview.chromium.org/2891643002/diff/60001/printing/backend/cups_j... printing/backend/cups_jobs.h:133: // A collection of supported IPP protcol versions. typo: protocol
PTAL https://codereview.chromium.org/2891643002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_info_cups.cc (right): https://codereview.chromium.org/2891643002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_info_cups.cc:77: // TODO(skau): Handle manufacturers with two word names. On 2017/05/31 23:00:03, Lei Zhang wrote: > On 2017/05/31 18:37:51, skau wrote: > > On 2017/05/31 00:48:03, Lei Zhang wrote: > > > What happens if a user hits this case? > > > > The manufacturer name gets spliced and added to the model name. > > What I mean is, if a user has a "Hewlett Packard 5000" printer, and they hit > this code while registering their printer, do they end up stuck with a "Hewlett" > / "Packard 5000" printer that they can't change the name of? That is correct. Alternatively, I could save the whole string in model until we can parse the string against a known dictionary. However, even Hewlett Packard uses HP for most of their printers. https://codereview.chromium.org/2891643002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_info_cups.cc (right): https://codereview.chromium.org/2891643002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_info_cups.cc:26: const std::vector<uint32_t> components = {2, 0}; On 2017/05/31 23:00:03, Lei Zhang wrote: > Make these 2 consts static. See the storage duration section of > http://en.cppreference.com/w/cpp/language/storage_duration Done. https://codereview.chromium.org/2891643002/diff/60001/printing/backend/cups_j... File printing/backend/cups_jobs.cc (right): https://codereview.chromium.org/2891643002/diff/60001/printing/backend/cups_j... printing/backend/cups_jobs.cc:15: #include "base/strings/string_number_conversions.h" On 2017/05/31 23:00:03, Lei Zhang wrote: > Probably don't need this and string_split.h anymore. Done. https://codereview.chromium.org/2891643002/diff/60001/printing/backend/cups_j... File printing/backend/cups_jobs.h (right): https://codereview.chromium.org/2891643002/diff/60001/printing/backend/cups_j... printing/backend/cups_jobs.h:133: // A collection of supported IPP protcol versions. On 2017/05/31 23:00:03, Lei Zhang wrote: > typo: protocol Done.
https://codereview.chromium.org/2891643002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_info_cups.cc (right): https://codereview.chromium.org/2891643002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_info_cups.cc:77: // TODO(skau): Handle manufacturers with two word names. On 2017/06/01 22:50:45, skau wrote: > On 2017/05/31 23:00:03, Lei Zhang wrote: > > On 2017/05/31 18:37:51, skau wrote: > > > On 2017/05/31 00:48:03, Lei Zhang wrote: > > > > What happens if a user hits this case? > > > > > > The manufacturer name gets spliced and added to the model name. > > > > What I mean is, if a user has a "Hewlett Packard 5000" printer, and they hit > > this code while registering their printer, do they end up stuck with a > "Hewlett" > > / "Packard 5000" printer that they can't change the name of? > > That is correct. Alternatively, I could save the whole string in model until we > can parse the string against a known dictionary. However, even Hewlett Packard > uses HP for most of their printers. Have you discussed this issue with others to see if this is ok for now or not? I know HP uses HP for their model names. I was just using it as an example. I know for a fact that on Windows there exists manufacturer names with a spaces in them. I haven't looked through CUPS drivers to see how it is there. https://codereview.chromium.org/2891643002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_info_cups.cc (right): https://codereview.chromium.org/2891643002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_info_cups.cc:26: static const std::vector<uint32_t>* kComponents = Was it not possible to make these non-pointers?
PTAL. Unfortunately, we lack the data to solve the parsing issue completely right now. However, I've added handling for multi-word names that I'm aware of using our PPD corpus. We're working on adding some metrics collection for printer make and model strings. https://codereview.chromium.org/2891643002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_info_cups.cc (right): https://codereview.chromium.org/2891643002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_info_cups.cc:77: // TODO(skau): Handle manufacturers with two word names. On 2017/06/01 23:25:11, Lei Zhang wrote: > On 2017/06/01 22:50:45, skau wrote: > > On 2017/05/31 23:00:03, Lei Zhang wrote: > > > On 2017/05/31 18:37:51, skau wrote: > > > > On 2017/05/31 00:48:03, Lei Zhang wrote: > > > > > What happens if a user hits this case? > > > > > > > > The manufacturer name gets spliced and added to the model name. > > > > > > What I mean is, if a user has a "Hewlett Packard 5000" printer, and they hit > > > this code while registering their printer, do they end up stuck with a > > "Hewlett" > > > / "Packard 5000" printer that they can't change the name of? > > > > That is correct. Alternatively, I could save the whole string in model until > we > > can parse the string against a known dictionary. However, even Hewlett > Packard > > uses HP for most of their printers. > > Have you discussed this issue with others to see if this is ok for now or not? > > I know HP uses HP for their model names. I was just using it as an example. I > know for a fact that on Windows there exists manufacturer names with a spaces in > them. I haven't looked through CUPS drivers to see how it is there. We lack the data to solve this properly at this time. I talked this over with adlr@ and I'm handling the multi-word names that we know of. We'll return to this with a proper solution after we've compiled a list of manufacturer names. https://codereview.chromium.org/2891643002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_info_cups.cc (right): https://codereview.chromium.org/2891643002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_info_cups.cc:26: static const std::vector<uint32_t>* kComponents = On 2017/06/01 23:25:11, Lei Zhang wrote: > Was it not possible to make these non-pointers? No, it's non POD so there's a clang warning that an exit-time destructor is required.
https://codereview.chromium.org/2891643002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_info_cups.cc (right): https://codereview.chromium.org/2891643002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_info_cups.cc:26: static const std::vector<uint32_t>* kComponents = On 2017/06/02 23:58:36, skau wrote: > On 2017/06/01 23:25:11, Lei Zhang wrote: > > Was it not possible to make these non-pointers? > > No, it's non POD so there's a clang warning that an exit-time destructor is > required. Can't you just do return version.components()[0] >= 2 ? (Do we need to be paranoid that the versions in ipp_versions are valid?)
https://codereview.chromium.org/2891643002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_info_cups.cc (right): https://codereview.chromium.org/2891643002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_info_cups.cc:26: static const std::vector<uint32_t>* kComponents = On 2017/06/02 23:58:36, skau wrote: > On 2017/06/01 23:25:11, Lei Zhang wrote: > > Was it not possible to make these non-pointers? > > No, it's non POD so there's a clang warning that an exit-time destructor is > required. I wanted to suggest CR_DEFINE_STATIC_LOCAL, but now I'm slightly worried about thread safety. (Either way) Since I don't know for sure how QueryIppPrinter() will be called. Though I'm guessing it's on the UI thread, so this will be parsing the results on the UI thread only and there's no thread safety issue.
I took justin's suggestion and removed the statics. https://codereview.chromium.org/2891643002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_info_cups.cc (right): https://codereview.chromium.org/2891643002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_info_cups.cc:26: static const std::vector<uint32_t>* kComponents = On 2017/06/03 00:07:25, Carlson wrote: > On 2017/06/02 23:58:36, skau wrote: > > On 2017/06/01 23:25:11, Lei Zhang wrote: > > > Was it not possible to make these non-pointers? > > > > No, it's non POD so there's a clang warning that an exit-time destructor is > > required. > > Can't you just do > > return version.components()[0] >= 2 > > ? > > (Do we need to be paranoid that the versions in ipp_versions are valid?) The current parsing code checks validity before adding it to the vector. There's a DCHECK in the code for the operator verifying validity. Invalid versions would indicate a coding error.
The simpler check works for me. https://codereview.chromium.org/2891643002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/printing/printer_info_cups.cc (right): https://codereview.chromium.org/2891643002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/printing/printer_info_cups.cc:55: DCHECK(version.IsValid()); How about making this a real check? return version.IsValid() && version.components()[0] >= 2; Because if the DCHECK fails, you are going to crash. https://codereview.chromium.org/2891643002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/printing/printer_info_cups.cc:106: // TODO(skau): Handle manufacturers with two word names. Mostly obsolete now? https://codereview.chromium.org/2891643002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/printing/printer_info_cups.cc:116: // If there's only one word or an empty string, use it. Do you have a collection of PPDs to test with to see if this ever happens? I was also thinking what if the make is missing, but the model is multi-word? Since this is data coming from CUPS, and OS X uses CUPS - how does Apple deal with this?
https://codereview.chromium.org/2891643002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/printing/printer_info_cups.cc (right): https://codereview.chromium.org/2891643002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/printing/printer_info_cups.cc:28: const std::array<base::StringPiece, 4> kMultiWordManufacturers{ Oh, you can't use StringPiece here. That creates static initializers. Stick with const char*.
Comments addressed. PTAL. https://codereview.chromium.org/2891643002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/printing/printer_info_cups.cc (right): https://codereview.chromium.org/2891643002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/printing/printer_info_cups.cc:28: const std::array<base::StringPiece, 4> kMultiWordManufacturers{ On 2017/06/03 01:17:14, Lei Zhang wrote: > Oh, you can't use StringPiece here. That creates static initializers. Stick with > const char*. Done. https://codereview.chromium.org/2891643002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/printing/printer_info_cups.cc:55: DCHECK(version.IsValid()); On 2017/06/03 00:53:14, Lei Zhang wrote: > How about making this a real check? > > return version.IsValid() && version.components()[0] >= 2; > > Because if the DCHECK fails, you are going to crash. Done. https://codereview.chromium.org/2891643002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/printing/printer_info_cups.cc:106: // TODO(skau): Handle manufacturers with two word names. On 2017/06/03 00:53:14, Lei Zhang wrote: > Mostly obsolete now? Done. https://codereview.chromium.org/2891643002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/printing/printer_info_cups.cc:116: // If there's only one word or an empty string, use it. On 2017/06/03 00:53:14, Lei Zhang wrote: > Do you have a collection of PPDs to test with to see if this ever happens? I was > also thinking what if the make is missing, but the model is multi-word? > > Since this is data coming from CUPS, and OS X uses CUPS - how does Apple deal > with this? This isn't an expected condition. Per the IPP spec, the string we're querying should be composed of at least two tokens. However, it's possible we're talking to a device that implements the spec incorrectly and this is the safest way for us to handle that.
https://codereview.chromium.org/2891643002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/printing/printer_info_cups.cc (right): https://codereview.chromium.org/2891643002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/printing/printer_info_cups.cc:116: // If there's only one word or an empty string, use it. On 2017/06/05 19:51:03, skau wrote: > On 2017/06/03 00:53:14, Lei Zhang wrote: > > Do you have a collection of PPDs to test with to see if this ever happens? I > was > > also thinking what if the make is missing, but the model is multi-word? > > > > Since this is data coming from CUPS, and OS X uses CUPS - how does Apple deal > > with this? > > This isn't an expected condition. Per the IPP spec, the string we're querying > should be composed of at least two tokens. However, it's possible we're talking > to a device that implements the spec incorrectly and this is the safest way for > us to handle that. If it's against the spec, would it be reasonable to just fail?
Thanks for the reviews. PTAL. Sorry for the delay. https://codereview.chromium.org/2891643002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/printing/printer_info_cups.cc (right): https://codereview.chromium.org/2891643002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/printing/printer_info_cups.cc:116: // If there's only one word or an empty string, use it. On 2017/06/06 22:02:37, Lei Zhang wrote: > On 2017/06/05 19:51:03, skau wrote: > > On 2017/06/03 00:53:14, Lei Zhang wrote: > > > Do you have a collection of PPDs to test with to see if this ever happens? I > > was > > > also thinking what if the make is missing, but the model is multi-word? > > > > > > Since this is data coming from CUPS, and OS X uses CUPS - how does Apple > deal > > > with this? > > > > This isn't an expected condition. Per the IPP spec, the string we're querying > > should be composed of at least two tokens. However, it's possible we're > talking > > to a device that implements the spec incorrectly and this is the safest way > for > > us to handle that. > > If it's against the spec, would it be reasonable to just fail? I'd rather not rely on the printer implementing the IPP spec correctly. And technically, there's nothing that specifies that a space should appear in this field. 4.4.8 printer-make-and-model (text(127)) This Printer attribute identifies the make and model of the device. The device manufacturer may initially populate this attribute. At any rate, we've decided that we're going to eliminate the 'make' field since this parsing is causing a lot of trouble for limited benefit. This is the bug I've filed crbug.com/730242. However, I'm going to keep this in this CL because the data in user prefs will need to be migrated before we can eliminate it.
https://codereview.chromium.org/2891643002/diff/180001/printing/backend/cups_... File printing/backend/cups_jobs.cc (right): https://codereview.chromium.org/2891643002/diff/180001/printing/backend/cups_... printing/backend/cups_jobs.cc:387: resource_path.front() == '/' ? resource_path : "/" + resource_path; Does front() work if |resource_path| is an empty string?
https://codereview.chromium.org/2891643002/diff/180001/printing/backend/cups_... File printing/backend/cups_jobs.cc (right): https://codereview.chromium.org/2891643002/diff/180001/printing/backend/cups_... printing/backend/cups_jobs.cc:387: resource_path.front() == '/' ? resource_path : "/" + resource_path; On 2017/06/07 22:29:28, Lei Zhang wrote: > Does front() work if |resource_path| is an empty string? No. front() is undefined for an empty string. :(
lgtm
The CQ bit was checked by skau@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 skau@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1496952028611280,
"parent_rev": "05c03751b8a506eac6bb553dbe9d1d5cbf406aa3", "commit_rev":
"bafbc3fe297223ffbef775053a1407910ce730e6"}
Message was sent while issue was closed.
Description was changed from ========== Add a method to query IPP printers for attributes. We determine if a printer can be configured without a PPD by checking the supported IPP version and some other attributes. Additionally, we fetch the printer-make-and-model that can be used to attempt to lookup a PPD. BUG=685676 ========== to ========== Add a method to query IPP printers for attributes. We determine if a printer can be configured without a PPD by checking the supported IPP version and some other attributes. Additionally, we fetch the printer-make-and-model that can be used to attempt to lookup a PPD. BUG=685676 Review-Url: https://codereview.chromium.org/2891643002 Cr-Commit-Position: refs/heads/master@{#478065} Committed: https://chromium.googlesource.com/chromium/src/+/bafbc3fe297223ffbef775053a14... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/bafbc3fe297223ffbef775053a14... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
