Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2691093006:
Implement IPP Get-Jobs and Get-Printer-Attributes requests. (Closed)
DescriptionImplement IPP Get-Jobs and Get-Printer-Attributes requests.
CUPS provides cupsGetJobs2 but it doesn't provide the necessary fields
to report status accurately. Notably, it doesn't provide
job-impressions-completed or printer-state-reasons both of which are
necessary to differentiate errors.
BUG=684853
Review-Url: https://codereview.chromium.org/2691093006
Cr-Commit-Position: refs/heads/master@{#456225}
Committed: https://chromium.googlesource.com/chromium/src/+/a26877dfeb89300f2da958471d2ab9f72766cf38
Patch Set 1 #Patch Set 2 : revisions #Patch Set 3 : cleanup #Patch Set 4 : remove unused fields #Patch Set 5 : s/ipp_util/cups_jobs/ #Patch Set 6 : assign default values to id and state #
Total comments: 45
Patch Set 7 : address comments #Patch Set 8 : clean #Patch Set 9 : rebase #Patch Set 10 : rebase #
Total comments: 34
Patch Set 11 : finish comments from thestig #
Total comments: 2
Patch Set 12 : use a const array #
Messages
Total messages: 43 (24 generated)
skau@chromium.org changed reviewers: + justincarlson@chromium.org
Please take a look for style and clarity. I need a better name for ipp_util but I'm not sure what to call it.
On 2017/02/23 21:55:03, skau wrote: > Please take a look for style and clarity. I need a better name for ipp_util but > I'm not sure what to call it. Barring any better ideas, I renamed it to cups_jobs.
On 2017/02/23 21:55:03, skau wrote: > Please take a look for style and clarity. I need a better name for ipp_util but > I'm not sure what to call it. Barring any better ideas, I renamed it to cups_jobs.
Mostly looks fine, would appreciate a little more explanation in the code of what's going on in cups_jobs.cc though. https://codereview.chromium.org/2691093006/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc (right): https://codereview.chromium.org/2691093006/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:215: PurgeJobs(); Does this deserve a "Giving up" log message? https://codereview.chromium.org/2691093006/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:221: Nit: Remove blank line https://codereview.chromium.org/2691093006/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:252: ScheduleQuery(); The fact that the reschedule here doesn't take into account whatever the requested poll rate was (if the user requested a non-default poll rate) seems wrong to me. https://codereview.chromium.org/2691093006/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/printing/cups_print_job_manager_impl.h (right): https://codereview.chromium.org/2691093006/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.h:80: int retry_count = 0; Missing trailing underscore? https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... File printing/backend/cups_connection.cc (right): https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_connection.cc:24: const int kQueueLimit = 10; Probably a little more appropriate to use constexpr for these. https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_connection.cc:150: success = false; Should these failure cases also continue the loop? I can't quite tell your intent. https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... File printing/backend/cups_connection.h (right): https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_connection.h:26: QueueStatus(PrinterStatus status, std::vector<CupsJob> cups_jobs) I suspect the second argument here should be a const reference? https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_connection.h:52: // all the queries were successful. What can we expect about |jobs| if not all the queries were successful and this returns false? https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... File printing/backend/cups_jobs.cc (right): https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_jobs.cc:83: struct IppDeleter { I don't think you need to define a struct for this, you should be able to pass &ippDelete directly to the unique_ptr constructor, but if you like it better, sure. https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_jobs.cc:129: static const std::map<base::StringPiece, PR>* kLabelToReason = I'm pretty sure this doesn't need to be a pointer. https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_jobs.cc:169: PrinterStatus::PrinterReason::Reason ToReason(base::StringPiece reason) { Please add function comments for functions not declared in the header https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_jobs.cc:248: // forward decl Comment not useful, more useful would be why forward declaration is needed here. https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_jobs.cc:259: CupsJob* job = &(*jobs)[jobs->size() - 1]; &(*jobs)[jobs->size() - 1] is the same as &jobs->back() I think. https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_jobs.cc:288: return base::StringPrintf("ipp://localhost/printers/%s", id.c_str()); Doesn't this need escaping, depending on the id? Or do we have sufficient restrictions on the contents of id that we know it won't need escaping? https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_jobs.cc:321: for (const std::string& reason : reason_strings) { It's legit to do for (const std::string& reason : ParseCollection(attr, &reason_strings)) { } if you want, but if you think that's gross, fair enough. https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... File printing/backend/cups_jobs.h (right): https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_jobs.h:21: struct PRINTING_EXPORT CupsJob { Field comments please? At least for things that are non-obvious, like state_reasons? https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_jobs.h:104: // Attmepts to retrieve printer status using connection |http| for |printer_id|. Attempts https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_jobs.h:118: bool completed, Suggest renaming to "include_completed"
skau@chromium.org changed reviewers: + vitalybuka@chromium.org
vitalybuka@: Please review as OWNER of printing/*. justincarlon@: Thanks for the review. PTAL. https://codereview.chromium.org/2691093006/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc (right): https://codereview.chromium.org/2691093006/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:215: PurgeJobs(); On 2017/02/24 00:02:08, Carlson wrote: > Does this deserve a "Giving up" log message? Done. https://codereview.chromium.org/2691093006/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:221: On 2017/02/24 00:02:08, Carlson wrote: > Nit: Remove blank line Done. https://codereview.chromium.org/2691093006/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:252: ScheduleQuery(); On 2017/02/24 00:02:08, Carlson wrote: > The fact that the reschedule here doesn't take into account whatever the > requested poll rate was (if the user requested a non-default poll rate) seems > wrong to me. Hmm. The interval isn't intended to be configurable. The intention is that we poll every kPollRate unless nothing is polling (where we poll immediately) or we're encountering errors (where we back off). Do you find the code difficult to read or do you think that the behavior should be different? Or both? I've added a comment with my intention. https://codereview.chromium.org/2691093006/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/printing/cups_print_job_manager_impl.h (right): https://codereview.chromium.org/2691093006/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.h:80: int retry_count = 0; On 2017/02/24 00:02:08, Carlson wrote: > Missing trailing underscore? Done. https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... File printing/backend/cups_connection.cc (right): https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_connection.cc:24: const int kQueueLimit = 10; On 2017/02/24 00:02:08, Carlson wrote: > Probably a little more appropriate to use constexpr for these. Done. https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_connection.cc:150: success = false; On 2017/02/24 00:02:08, Carlson wrote: > Should these failure cases also continue the loop? I can't quite tell your > intent. Originally, the intent was to collect all data. However, I'm breaking out now to make the semantics clearer. https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... File printing/backend/cups_connection.h (right): https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_connection.h:26: QueueStatus(PrinterStatus status, std::vector<CupsJob> cups_jobs) On 2017/02/24 00:02:08, Carlson wrote: > I suspect the second argument here should be a const reference? I've removed the constructor and I'm populating the object in place instead. https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_connection.h:52: // all the queries were successful. On 2017/02/24 00:02:08, Carlson wrote: > What can we expect about |jobs| if not all the queries were successful and this > returns false? Done. https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... File printing/backend/cups_jobs.cc (right): https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_jobs.cc:83: struct IppDeleter { On 2017/02/24 00:02:08, Carlson wrote: > I don't think you need to define a struct for this, you should be able to pass > &ippDelete directly to the unique_ptr constructor, but if you like it better, > sure. I think the Deleter must be a type. How do you do that? https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_jobs.cc:129: static const std::map<base::StringPiece, PR>* kLabelToReason = On 2017/02/24 00:02:08, Carlson wrote: > I'm pretty sure this doesn't need to be a pointer. I think it needs to be a pointer to avoid being allocated at load. Or, I'm reading this wrong: http://en.cppreference.com/w/cpp/language/storage_duration#Storage_duration https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_jobs.cc:169: PrinterStatus::PrinterReason::Reason ToReason(base::StringPiece reason) { On 2017/02/24 00:02:09, Carlson wrote: > Please add function comments for functions not declared in the header Done. https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_jobs.cc:248: // forward decl On 2017/02/24 00:02:08, Carlson wrote: > Comment not useful, more useful would be why forward declaration is needed here. Done. https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_jobs.cc:259: CupsJob* job = &(*jobs)[jobs->size() - 1]; On 2017/02/24 00:02:09, Carlson wrote: > &(*jobs)[jobs->size() - 1] > > is the same as > > &jobs->back() > > I think. Done. In C++17, emplace_back will return a reference. :) Not that we'll be able to use that any time soon ... https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_jobs.cc:288: return base::StringPrintf("ipp://localhost/printers/%s", id.c_str()); On 2017/02/24 00:02:08, Carlson wrote: > Doesn't this need escaping, depending on the id? Or do we have sufficient > restrictions on the contents of id that we know it won't need escaping? I don't think so. CUPS rejects non-ascii characters and spaces in names so there won't be an id for which this won't resolve the printer correctly. It can, however, generate a funky url. Error checking should be handled higher up the stack. https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_jobs.cc:321: for (const std::string& reason : reason_strings) { On 2017/02/24 00:02:09, Carlson wrote: > It's legit to do > > for (const std::string& reason : ParseCollection(attr, &reason_strings)) { > } > if you want, but if you think that's gross, fair enough. I don't think so, ParseCollection returns void. Does C++11 guarantee that expression right of the : is only evaluated once? https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... File printing/backend/cups_jobs.h (right): https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_jobs.h:21: struct PRINTING_EXPORT CupsJob { On 2017/02/24 00:02:09, Carlson wrote: > Field comments please? At least for things that are non-obvious, like > state_reasons? Done. https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_jobs.h:104: // Attmepts to retrieve printer status using connection |http| for |printer_id|. On 2017/02/24 00:02:09, Carlson wrote: > Attempts Done. https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_jobs.h:118: bool completed, On 2017/02/24 00:02:09, Carlson wrote: > Suggest renaming to "include_completed" I've changed it to an enum. It's either completed jobs xor not-completed jobs. include_completed would imply that if it's true, you would get both.
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...
lgtm https://codereview.chromium.org/2691093006/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc (right): https://codereview.chromium.org/2691093006/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:252: ScheduleQuery(); On 2017/02/28 00:59:58, skau wrote: > On 2017/02/24 00:02:08, Carlson wrote: > > The fact that the reschedule here doesn't take into account whatever the > > requested poll rate was (if the user requested a non-default poll rate) seems > > wrong to me. > > Hmm. The interval isn't intended to be configurable. The intention is that we > poll every kPollRate unless nothing is polling (where we poll immediately) or > we're encountering errors (where we back off). Do you find the code difficult > to read or do you think that the behavior should be different? Or both? > > I've added a comment with my intention. The custom poll rate option is just for testing? If so, that seems fine. I don't have a more specific complaint than if the user requests a specific poll rate, having the reschedule happen at a *different* rate is surprising. https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... File printing/backend/cups_jobs.cc (right): https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_jobs.cc:83: struct IppDeleter { On 2017/02/28 00:59:59, skau wrote: > On 2017/02/24 00:02:08, Carlson wrote: > > I don't think you need to define a struct for this, you should be able to pass > > &ippDelete directly to the unique_ptr constructor, but if you like it better, > > sure. > > I think the Deleter must be a type. How do you do that? You can use a function pointer as the second type, e.g.: #include <memory> #include <stdio.h> struct Foo { Foo() { printf("Constructed\n"); } }; void deleteit(Foo* t) { printf("Baleeted\n"); } int main() { std::unique_ptr<Foo, void(*)(Foo*)> p(new Foo, &deleteit); return 0; } https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_jobs.cc:129: static const std::map<base::StringPiece, PR>* kLabelToReason = On 2017/02/28 00:59:59, skau wrote: > On 2017/02/24 00:02:08, Carlson wrote: > > I'm pretty sure this doesn't need to be a pointer. > > I think it needs to be a pointer to avoid being allocated at load. Or, I'm > reading this wrong: > http://en.cppreference.com/w/cpp/language/storage_duration#Storage_duration I think you have an incomplete reading here. The (static) *storage* for the object is, indeed allocated at load time (presumably in .bss? I dont' actually know that, but it's not important here), but the *initialization* for the object is deferred until first entry into the function. And is thread safe. This is the guarantee that makes Meyer's Singleton pattern work. See "Static Local Variables" here: http://en.cppreference.com/w/cpp/language/storage_duration https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_jobs.cc:288: return base::StringPrintf("ipp://localhost/printers/%s", id.c_str()); On 2017/02/28 00:59:59, skau wrote: > On 2017/02/24 00:02:08, Carlson wrote: > > Doesn't this need escaping, depending on the id? Or do we have sufficient > > restrictions on the contents of id that we know it won't need escaping? > > I don't think so. CUPS rejects non-ascii characters and spaces in names so there > won't be an id for which this won't resolve the printer correctly. It can, > however, generate a funky url. Error checking should be handled higher up the > stack. OK, but I already found a bug in CUPS' validation of that. :) https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_jobs.cc:321: for (const std::string& reason : reason_strings) { On 2017/02/28 00:59:59, skau wrote: > On 2017/02/24 00:02:09, Carlson wrote: > > It's legit to do > > > > for (const std::string& reason : ParseCollection(attr, &reason_strings)) { > > } > > if you want, but if you think that's gross, fair enough. > Oops, you're right, didn't look closely at the function. But yes to the latter part.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2691093006/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc (right): https://codereview.chromium.org/2691093006/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:252: ScheduleQuery(); On 2017/02/28 01:29:47, Carlson wrote: > On 2017/02/28 00:59:58, skau wrote: > > On 2017/02/24 00:02:08, Carlson wrote: > > > The fact that the reschedule here doesn't take into account whatever the > > > requested poll rate was (if the user requested a non-default poll rate) > seems > > > wrong to me. > > > > Hmm. The interval isn't intended to be configurable. The intention is that > we > > poll every kPollRate unless nothing is polling (where we poll immediately) or > > we're encountering errors (where we back off). Do you find the code difficult > > to read or do you think that the behavior should be different? Or both? > > > > I've added a comment with my intention. > > The custom poll rate option is just for testing? If so, that seems fine. > > I don't have a more specific complaint than if the user requests a specific poll > rate, having the reschedule happen at a *different* rate is surprising. Upon reading this more closely, I'm complaining about an issue that doesn't exist because I was thinking I was in a different function than I was. This is fine.
Thanks for the review. Comments have been addressed. https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... File printing/backend/cups_jobs.cc (right): https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_jobs.cc:83: struct IppDeleter { On 2017/02/28 01:29:47, Carlson wrote: > On 2017/02/28 00:59:59, skau wrote: > > On 2017/02/24 00:02:08, Carlson wrote: > > > I don't think you need to define a struct for this, you should be able to > pass > > > &ippDelete directly to the unique_ptr constructor, but if you like it > better, > > > sure. > > > > I think the Deleter must be a type. How do you do that? > > You can use a function pointer as the second type, e.g.: > > #include <memory> > > #include <stdio.h> > > struct Foo { > Foo() { printf("Constructed\n"); } > }; > > void deleteit(Foo* t) { > printf("Baleeted\n"); > } > > int main() { > std::unique_ptr<Foo, void(*)(Foo*)> p(new Foo, &deleteit); > return 0; > } > Right. Function pointers. This would be awful without auto. Or type aliasing. https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_jobs.cc:129: static const std::map<base::StringPiece, PR>* kLabelToReason = On 2017/02/28 01:29:47, Carlson wrote: > On 2017/02/28 00:59:59, skau wrote: > > On 2017/02/24 00:02:08, Carlson wrote: > > > I'm pretty sure this doesn't need to be a pointer. > > > > I think it needs to be a pointer to avoid being allocated at load. Or, I'm > > reading this wrong: > > http://en.cppreference.com/w/cpp/language/storage_duration#Storage_duration > > I think you have an incomplete reading here. The (static) *storage* for the > object is, indeed allocated at load time (presumably in .bss? I dont' actually > know that, but it's not important here), but the *initialization* for the object > is deferred until first entry into the function. And is thread safe. This is > the guarantee that makes Meyer's Singleton pattern work. > > See "Static Local Variables" here: > > http://en.cppreference.com/w/cpp/language/storage_duration > > Yep. I read the wrong heading. Also, I had to Google "Meyer's Singleton". https://codereview.chromium.org/2691093006/diff/100001/printing/backend/cups_... printing/backend/cups_jobs.cc:288: return base::StringPrintf("ipp://localhost/printers/%s", id.c_str()); On 2017/02/28 01:29:47, Carlson wrote: > On 2017/02/28 00:59:59, skau wrote: > > On 2017/02/24 00:02:08, Carlson wrote: > > > Doesn't this need escaping, depending on the id? Or do we have sufficient > > > restrictions on the contents of id that we know it won't need escaping? > > > > I don't think so. CUPS rejects non-ascii characters and spaces in names so > there > > won't be an id for which this won't resolve the printer correctly. It can, > > however, generate a funky url. Error checking should be handled higher up the > > stack. > > OK, but I already found a bug in CUPS' validation of that. :) Our ids will be valid at least since they're a SHA1s hex-encoded or a UUID.
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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
vitalybuka@: Can you take a look when I get a chance? It looks like thestig@ is back now so this'll be the last CL you have to review for me :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Implement IPP Get-Jobs and Get-Printer-Attributes requests. CUPS provides cupsGetJobs2 but it doesn't provide the necessary fields to report status accurately. Notably, it doesn't provide job-impressions-completed or printer-state-reasons both of which are necessary to differentiate errors. BUG=684853 ========== to ========== Implement IPP Get-Jobs and Get-Printer-Attributes requests. CUPS provides cupsGetJobs2 but it doesn't provide the necessary fields to report status accurately. Notably, it doesn't provide job-impressions-completed or printer-state-reasons both of which are necessary to differentiate errors. BUG=684853 ==========
skau@chromium.org changed reviewers: + thestig@chromium.org - vitalybuka@chromium.org
thestig@: Please review when you get a chance.
Didn't quite finish reading this CL. Just being nitty for the most part. https://codereview.chromium.org/2691093006/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc (right): https://codereview.chromium.org/2691093006/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:70: result.success = connection->GetJobs(printer_ids, &(result.queues)); nit: Do you need the extra parenthesis around result.queues? https://codereview.chromium.org/2691093006/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:176: // Query CUPS asynchronously. Post results back to UI thread. Can you move this to the header and consolidate it with the comment there? https://codereview.chromium.org/2691093006/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/printing/cups_print_job_manager_impl.h (right): https://codereview.chromium.org/2691093006/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.h:15: #include "base/synchronization/lock.h" nit: Not needed here? https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... File printing/backend/cups_connection.cc (right): https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... printing/backend/cups_connection.cc:24: constexpr int kQueueLimit = 10; Why not |kProcessingJobsLimit| and |kCompletedJobsLimit| for this constant and the one below? If you can briefly explain why you chose these values, that would be helpful to future readers. https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... printing/backend/cups_connection.cc:163: success = false; Drop the success variable and just return false? https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... File printing/backend/cups_jobs.cc (right): https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... printing/backend/cups_jobs.cc:18: using PR = PrinterStatus::PrinterReason::Reason; Maybe PReason? https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... File printing/backend/cups_jobs.h (right): https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... printing/backend/cups_jobs.h:12: #include <memory> Not needed? https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... printing/backend/cups_jobs.h:22: // Cooresponds to job-state from RFC2911. typo: Corresponds https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... printing/backend/cups_jobs.h:107: enum WhichJobs { Maybe JobCompletionState?
https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... File printing/backend/cups_jobs.cc (right): https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... printing/backend/cups_jobs.cc:86: std::unique_ptr<ipp_t, void (*)(ipp_t*)> WrapIpp(ipp_t* ipp) { ScopedIpp, like base::win::ScopedComPtr and friends. https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... printing/backend/cups_jobs.cc:97: converted_state = CupsJob::ABORTED; Just: return CupsJob::ABORTED; ? https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... printing/backend/cups_jobs.cc:172: if (entry == enum_map.end()) { return entry != enum_map.end() ? entry->second : PR::UNKNOWN_REASON; https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... printing/backend/cups_jobs.cc:278: void ParseFields(ipp_t* response, Do you think an iterative implementation might be easier to understand? https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... printing/backend/cups_jobs.cc:281: CupsJob* current_job, Isn't this always &jobs->back()? https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... printing/backend/cups_jobs.cc:356: std::vector<const char*> printer_attributes = { Can this be simpler? const char* const kPrinterAttributes[] = { ... };
https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... File printing/backend/cups_jobs.cc (right): https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... printing/backend/cups_jobs.cc:86: std::unique_ptr<ipp_t, void (*)(ipp_t*)> WrapIpp(ipp_t* ipp) { On 2017/03/08 02:02:16, Lei Zhang (super slow) wrote: > ScopedIpp, like base::win::ScopedComPtr and friends. Wait, I read this wrong. Ignore.
Addressed comments. PTAL. https://codereview.chromium.org/2691093006/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc (right): https://codereview.chromium.org/2691093006/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:70: result.success = connection->GetJobs(printer_ids, &(result.queues)); On 2017/03/07 23:31:29, Lei Zhang (super slow) wrote: > nit: Do you need the extra parenthesis around result.queues? Done. https://codereview.chromium.org/2691093006/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:176: // Query CUPS asynchronously. Post results back to UI thread. On 2017/03/07 23:31:29, Lei Zhang (super slow) wrote: > Can you move this to the header and consolidate it with the comment there? Done. https://codereview.chromium.org/2691093006/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/printing/cups_print_job_manager_impl.h (right): https://codereview.chromium.org/2691093006/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.h:15: #include "base/synchronization/lock.h" On 2017/03/07 23:31:29, Lei Zhang (super slow) wrote: > nit: Not needed here? Done. https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... File printing/backend/cups_connection.cc (right): https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... printing/backend/cups_connection.cc:24: constexpr int kQueueLimit = 10; On 2017/03/07 23:31:29, Lei Zhang (super slow) wrote: > Why not |kProcessingJobsLimit| and |kCompletedJobsLimit| for this constant and > the one below? If you can briefly explain why you chose these values, that would > be helpful to future readers. Done. https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... printing/backend/cups_connection.cc:163: success = false; On 2017/03/07 23:31:29, Lei Zhang (super slow) wrote: > Drop the success variable and just return false? Done. https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... File printing/backend/cups_jobs.cc (right): https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... printing/backend/cups_jobs.cc:18: using PR = PrinterStatus::PrinterReason::Reason; On 2017/03/07 23:31:29, Lei Zhang (super slow) wrote: > Maybe PReason? Done. https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... printing/backend/cups_jobs.cc:86: std::unique_ptr<ipp_t, void (*)(ipp_t*)> WrapIpp(ipp_t* ipp) { On 2017/03/08 02:21:10, Lei Zhang (super slow) wrote: > On 2017/03/08 02:02:16, Lei Zhang (super slow) wrote: > > ScopedIpp, like base::win::ScopedComPtr and friends. > > Wait, I read this wrong. Ignore. Acknowledged. https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... printing/backend/cups_jobs.cc:97: converted_state = CupsJob::ABORTED; On 2017/03/08 02:02:16, Lei Zhang (super slow) wrote: > Just: return CupsJob::ABORTED; ? Done. https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... printing/backend/cups_jobs.cc:172: if (entry == enum_map.end()) { On 2017/03/08 02:02:16, Lei Zhang (super slow) wrote: > return entry != enum_map.end() ? entry->second : PR::UNKNOWN_REASON; Done. https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... printing/backend/cups_jobs.cc:278: void ParseFields(ipp_t* response, On 2017/03/08 02:02:16, Lei Zhang (super slow) wrote: > Do you think an iterative implementation might be easier to understand? I find the recursive solution easy enough to read. However, I gave it some thought and managed an iterative version that's reasonably clear. https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... printing/backend/cups_jobs.cc:281: CupsJob* current_job, On 2017/03/08 02:02:16, Lei Zhang (super slow) wrote: > Isn't this always &jobs->back()? Acknowledged. https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... printing/backend/cups_jobs.cc:356: std::vector<const char*> printer_attributes = { On 2017/03/08 02:02:16, Lei Zhang (super slow) wrote: > Can this be simpler? > > const char* const kPrinterAttributes[] = { ... }; Done. Is there a way to have the compiler compute the size of the array? https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... File printing/backend/cups_jobs.h (right): https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... printing/backend/cups_jobs.h:12: #include <memory> On 2017/03/07 23:31:29, Lei Zhang (super slow) wrote: > Not needed? Done. https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... printing/backend/cups_jobs.h:22: // Cooresponds to job-state from RFC2911. On 2017/03/07 23:31:29, Lei Zhang (super slow) wrote: > typo: Corresponds Done. https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... printing/backend/cups_jobs.h:107: enum WhichJobs { On 2017/03/07 23:31:29, Lei Zhang (super slow) wrote: > Maybe JobCompletionState? Done.
lgtm https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... File printing/backend/cups_jobs.cc (right): https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... printing/backend/cups_jobs.cc:356: std::vector<const char*> printer_attributes = { On 2017/03/10 01:07:00, skau wrote: > On 2017/03/08 02:02:16, Lei Zhang (super slow) wrote: > > Can this be simpler? > > > > const char* const kPrinterAttributes[] = { ... }; > > Done. > > Is there a way to have the compiler compute the size of the array? Does arraysize(kPrinterAttributes) work? https://codereview.chromium.org/2691093006/diff/200001/printing/backend/cups_... File printing/backend/cups_jobs.cc (right): https://codereview.chromium.org/2691093006/diff/200001/printing/backend/cups_... printing/backend/cups_jobs.cc:88: const char* kPrinterAttributes[] = {kPrinterState, kPrinterStateReasons, const char* const - otherwise one can write kPrinterAttributes[0] = nullptr;
https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... File printing/backend/cups_jobs.cc (right): https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... printing/backend/cups_jobs.cc:356: std::vector<const char*> printer_attributes = { On 2017/03/10 01:52:52, Lei Zhang (super slow) wrote: > On 2017/03/10 01:07:00, skau wrote: > > On 2017/03/08 02:02:16, Lei Zhang (super slow) wrote: > > > Can this be simpler? > > > > > > const char* const kPrinterAttributes[] = { ... }; > > > > Done. > > > > Is there a way to have the compiler compute the size of the array? > > Does arraysize(kPrinterAttributes) work? const std::array<const char* const> is another option.
I've used a constexpr std::array. Upon a reading of https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... and http://en.cppreference.com/w/cpp/container/array I believe it to be conforming based on: "This container is an aggregate type with the same semantics as a struct holding a C-style array T[N] as its only non-static data member." and it holds raw pointers. https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... File printing/backend/cups_jobs.cc (right): https://codereview.chromium.org/2691093006/diff/180001/printing/backend/cups_... printing/backend/cups_jobs.cc:356: std::vector<const char*> printer_attributes = { On 2017/03/10 18:00:17, Carlson wrote: > On 2017/03/10 01:52:52, Lei Zhang (super slow) wrote: > > On 2017/03/10 01:07:00, skau wrote: > > > On 2017/03/08 02:02:16, Lei Zhang (super slow) wrote: > > > > Can this be simpler? > > > > > > > > const char* const kPrinterAttributes[] = { ... }; > > > > > > Done. > > > > > > Is there a way to have the compiler compute the size of the array? > > > > Does arraysize(kPrinterAttributes) work? > > const std::array<const char* const> is another option. Done. https://codereview.chromium.org/2691093006/diff/200001/printing/backend/cups_... File printing/backend/cups_jobs.cc (right): https://codereview.chromium.org/2691093006/diff/200001/printing/backend/cups_... printing/backend/cups_jobs.cc:88: const char* kPrinterAttributes[] = {kPrinterState, kPrinterStateReasons, On 2017/03/10 01:52:52, Lei Zhang (super slow) wrote: > const char* const - otherwise one can write kPrinterAttributes[0] = nullptr; Done.
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
The patchset sent to the CQ was uploaded after l-g-t-m from justincarlson@chromium.org, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2691093006/#ps220001 (title: "use a const array")
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": 220001, "attempt_start_ts": 1489190621225050,
"parent_rev": "dece487a37276f1043d8092acd01a761ec99c17e", "commit_rev":
"a26877dfeb89300f2da958471d2ab9f72766cf38"}
Message was sent while issue was closed.
Description was changed from ========== Implement IPP Get-Jobs and Get-Printer-Attributes requests. CUPS provides cupsGetJobs2 but it doesn't provide the necessary fields to report status accurately. Notably, it doesn't provide job-impressions-completed or printer-state-reasons both of which are necessary to differentiate errors. BUG=684853 ========== to ========== Implement IPP Get-Jobs and Get-Printer-Attributes requests. CUPS provides cupsGetJobs2 but it doesn't provide the necessary fields to report status accurately. Notably, it doesn't provide job-impressions-completed or printer-state-reasons both of which are necessary to differentiate errors. BUG=684853 Review-Url: https://codereview.chromium.org/2691093006 Cr-Commit-Position: refs/heads/master@{#456225} Committed: https://chromium.googlesource.com/chromium/src/+/a26877dfeb89300f2da958471d2a... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/a26877dfeb89300f2da958471d2a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
