Create a new print backend for the updated CUPS APIs

Implement a new backend for modern CUPS implementations

As of CUPS 1.6, CUPS has moved away from PPD attributes to IPP attributes.  This new backend uses the IPP APIs and will be better suited to use with CUPS going forward.  Until we can bump our minumum CUPS version to 1.7, the only client will be Chrome OS where the version is 2.1.3.

BUG=607668
TEST=With --enable-native-cups enabled, print dialog will show available CUPS printers if any are configured.

Committed: https://crrev.com/b779ce61d5369a930af480c265c0c5b924888914
Cr-Commit-Position: refs/heads/master@{#407011}

[skau, 2016-07-01 03:10:20]

Description was changed from

==========
Create a new print backend for the updated CUPS APIs

CUPS has deprecated many of the APIs that we were using.  Write a new
backend for CUPS 1.6+ so that we can migrate to it as the minimum CUPS
library versions are uprev'd.

BUG=607668
==========

to

==========
Implement a new backend for modern CUPS implementations
    
    As of CUPS 1.6, CUPS has moved away from PPD attributes to IPP
    attributes.  This new backend uses the IPP APIs and will be better
    suited to use with CUPS going forward.  Until we can bump our minumum
    CUPS version to 1.7, the only client will be Chrome OS where the version
    is 2.1.3.
    
    BUG=607668
    
    TEST=With --enable-native-cups enabled, print dialog will show available
    CUPS printers if any are configured.
==========

[skau, 2016-07-01 03:10:59]

Description was changed from

==========
Implement a new backend for modern CUPS implementations
    
    As of CUPS 1.6, CUPS has moved away from PPD attributes to IPP
    attributes.  This new backend uses the IPP APIs and will be better
    suited to use with CUPS going forward.  Until we can bump our minumum
    CUPS version to 1.7, the only client will be Chrome OS where the version
    is 2.1.3.
    
    BUG=607668
    
    TEST=With --enable-native-cups enabled, print dialog will show available
    CUPS printers if any are configured.
==========

to

==========
Implement a new backend for modern CUPS implementations
    
As of CUPS 1.6, CUPS has moved away from PPD attributes to IPP attributes.  This
new backend uses the IPP APIs and will be better suited to use with CUPS going
forward.  Until we can bump our minumum CUPS version to 1.7, the only client
will be Chrome OS where the version is 2.1.3.
    
BUG=607668
    
TEST=With --enable-native-cups enabled, print dialog will show available CUPS
printers if any are configured.
==========

[skau, 2016-07-08 00:45:38]

skau@chromium.org changed reviewers:
+ briannorris@chromium.org, thestig@chromium.org

[skau, 2016-07-08 00:45:39]

This is the implementation of the new CUPS backend.  The CL is a bit large.  The
primary motivation for the change is that CUPS options values are now expected
to match what is returned from the IPP APIs.

The next CL will allow printing.  This CL only enables printer discovery.

[Lei Zhang, 2016-07-08 01:19:50]

I only took a quick glance. BTW, if you think the code is ready, you should run
tryjobs.

What about Mac? I think with OS X 10.8 as the minimum, CUPS should be at least
1.6 or maybe 1.7.

https://codereview.chromium.org/2105463002/diff/80001/printing/BUILD.gn
File printing/BUILD.gn (right):

https://codereview.chromium.org/2105463002/diff/80001/printing/BUILD.gn#newco...
printing/BUILD.gn:172: if (is_chromeos) {
How about GYP?

https://codereview.chromium.org/2105463002/diff/80001/printing/backend/cups_c...
File printing/backend/cups_connection.cc (right):

https://codereview.chromium.org/2105463002/diff/80001/printing/backend/cups_c...
printing/backend/cups_connection.cc:91: if (connection == NULL)
NULL -> nullptr
foo == NULL -> !foo

https://codereview.chromium.org/2105463002/diff/80001/printing/backend/cups_c...
File printing/backend/cups_connection.h (right):

https://codereview.chromium.org/2105463002/diff/80001/printing/backend/cups_c...
printing/backend/cups_connection.h:49: std::shared_ptr<http_t> cups_http_;
Currently banned: https://chromium-cpp.appspot.com/

https://codereview.chromium.org/2105463002/diff/80001/printing/backend/print_...
File printing/backend/print_backend_chromeos.cc (left):

https://codereview.chromium.org/2105463002/diff/80001/printing/backend/print_...
printing/backend/print_backend_chromeos.cc:96: return new
PrintBackendCUPS(print_server_url,
Do you want to exclude building PrintBackendCUPS on ChromeOS then?

[skau, 2016-07-08 21:24:09]

Thanks for the review.  I've addressed the comments.  I'm hoping that the design
is generally okay.  PTAL.

https://codereview.chromium.org/2105463002/diff/80001/printing/BUILD.gn
File printing/BUILD.gn (right):

https://codereview.chromium.org/2105463002/diff/80001/printing/BUILD.gn#newco...
printing/BUILD.gn:172: if (is_chromeos) {
On 2016/07/08 01:19:50, Lei Zhang wrote:
> How about GYP?

Sorry, I forgot.  On the bright side, it looks like GYP should go away by end of
summer.

https://codereview.chromium.org/2105463002/diff/80001/printing/backend/cups_c...
File printing/backend/cups_connection.cc (right):

https://codereview.chromium.org/2105463002/diff/80001/printing/backend/cups_c...
printing/backend/cups_connection.cc:91: if (connection == NULL)
On 2016/07/08 01:19:50, Lei Zhang wrote:
> NULL -> nullptr
> foo == NULL -> !foo

Thanks!

https://codereview.chromium.org/2105463002/diff/80001/printing/backend/cups_c...
File printing/backend/cups_connection.h (right):

https://codereview.chromium.org/2105463002/diff/80001/printing/backend/cups_c...
printing/backend/cups_connection.h:49: std::shared_ptr<http_t> cups_http_;
On 2016/07/08 01:19:50, Lei Zhang wrote:
> Currently banned: https://chromium-cpp.appspot.com/

I've switched to a WeakPtr implementation.  Lmk what you think.

https://codereview.chromium.org/2105463002/diff/80001/printing/backend/print_...
File printing/backend/print_backend_chromeos.cc (left):

https://codereview.chromium.org/2105463002/diff/80001/printing/backend/print_...
printing/backend/print_backend_chromeos.cc:96: return new
PrintBackendCUPS(print_server_url,
On 2016/07/08 01:19:50, Lei Zhang wrote:
> Do you want to exclude building PrintBackendCUPS on ChromeOS then?

I'm doing it with the BUILD file.  Should the file still be conditionally
compiled?

[Lei Zhang, 2016-07-13 00:34:27]

Getting around to taking a look.

https://codereview.chromium.org/2105463002/diff/120001/printing/BUILD.gn
File printing/BUILD.gn (right):

https://codereview.chromium.org/2105463002/diff/120001/printing/BUILD.gn#newc...
printing/BUILD.gn:174: "backend/cups_connection.cc",
Can we use this backend on Mac as well?

https://codereview.chromium.org/2105463002/diff/120001/printing/printing.gyp
File printing/printing.gyp (right):

https://codereview.chromium.org/2105463002/diff/120001/printing/printing.gyp#...
printing/printing.gyp:177: 'conditions': [
I think the redness means you have tabs, and not spaces.

https://codereview.chromium.org/2105463002/diff/120001/printing/printing.gyp#...
printing/printing.gyp:189: }],
GYP can do if-else:

['chromeos==1', {
  ...
}, {
  ... # else case
}],

https://codereview.chromium.org/2105463002/diff/120001/printing/printing.gyp#...
printing/printing.gyp:213: 'printed_document_linux.cc',
alphabetical order

[Lei Zhang, 2016-07-13 01:08:43]

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
File printing/backend/cups_connection.cc (right):

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_connection.cc:20: cupsFreeDests(1, dest);
Can |dests_| be: std::vector<std::unique_ptr<cups_dest_t*, DestinationDeleter>>
? Then you can just clear |dests_| and be done... and that happens automatically
in the dtor when the enumerator gets destroyed.

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_connection.cc:87: std::string host_string;
Only used inside the if block below - move it there.

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_connection.cc:101: httpConnect2(host, port, NULL,
AF_UNSPEC, cups_encryption_,
nullptr all around

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_connection.cc:107: impl_ = std::unique_ptr<Impl>(new
Impl(connection));
Use base::MakeUnique<Impl>(connection) ?

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
File printing/backend/cups_connection.h (right):

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_connection.h:30: explicit CupsConnection(const GURL&
print_server_url,
Not explicit

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_connection.h:38: // Returns the number of destinations and
populates |destinations|.
Comment doesn't match code.

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_connection.h:42: CupsPrinter* GetPrinter(const
std::string& printer_name);
Caller owns the returned pointer, right? Make it return an unique_ptr.

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_connection.h:56: class Impl {
Put this as the first thing in the private section, so all the member variables
are together.

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_connection.h:61: base::WeakPtr<http_t> GetHttp();
Can you explain the ownership model? Why do we need WeakPtrs?

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
File printing/backend/cups_ipp_util.h (right):

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_ipp_util.h:16: void ExtractColor(const CupsPrinter&
printer,
Why not just return the color, and let the caller populate |printer_info| ?

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
File printing/backend/cups_printer.cc (right):

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_printer.cc:57: return NULL;
More NULLs

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_printer.cc:68: if (attr == nullptr) {
More equality checks again nullptr.

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_printer.cc:167: if (!make_and_model) {
return make_and_model ? std::string(make_and_model) : std::string();

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
File printing/backend/cups_printer.h (right):

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_printer.h:26: class DestinationDeleter {
Could some of these be shared for use with print_backend_cups.cc? They seem
generally useful for any CUPS code that have to deal with these structures.

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_printer.h:41: explicit CupsPrinter(base::WeakPtr<http_t>
http,
Not explicit.

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/print...
File printing/backend/print_backend_chromeos.cc (right):

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/print...
printing/backend/print_backend_chromeos.cc:18: // Provides a stubbed out
PrintBackend implementation for use on ChromeOS.
Update comment.

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/print...
printing/backend/print_backend_chromeos.cc:97: CupsConnection
connection(print_server_url,
This should be an unique_ptr, and PrintBackendCupsIpp should store an
unique_ptr. https://google.github.io/styleguide/cppguide.html#Rvalue_references

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/print...
File printing/backend/print_backend_cups_ipp.cc (right):

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/print...
printing/backend/print_backend_cups_ipp.cc:51:
printer_list->push_back(std::move(basic_info));
I don't think the std::move() works here. You still end up calling the copy
constructor.

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/print...
printing/backend/print_backend_cups_ipp.cc:66: return "";
return std::string();

[Lei Zhang, 2016-07-13 01:32:04]

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
File printing/backend/cups_printer.h (right):

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_printer.h:92: bool StreamData(char* buffer, int len);
Can this take a const std::vector<char>&  - use STL containers as long as
possible.

[skau, 2016-07-14 20:43:06]

Thanks for the thorough review.  Comments have been addressed. PTAL.

Ping me if you want to talk about the WeakPtrs.

https://codereview.chromium.org/2105463002/diff/120001/printing/BUILD.gn
File printing/BUILD.gn (right):

https://codereview.chromium.org/2105463002/diff/120001/printing/BUILD.gn#newc...
printing/BUILD.gn:174: "backend/cups_connection.cc",
On 2016/07/13 00:34:27, Lei Zhang wrote:
> Can we use this backend on Mac as well?

It will be compatible.  I did a test on my Macbook and it worked.  However,
there's no IPP attribute to CDD translation right now so the old CUPS backend is
still needed for devices that can act as a cloud print connector.  Also, my plan
is to validate it on Chrome OS first before flipping Mac.  Sound good?

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
File printing/backend/cups_connection.cc (right):

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_connection.cc:20: cupsFreeDests(1, dest);
On 2016/07/13 01:08:42, Lei Zhang wrote:
> Can |dests_| be: std::vector<std::unique_ptr<cups_dest_t*,
DestinationDeleter>>
> ? Then you can just clear |dests_| and be done... and that happens
automatically
> in the dtor when the enumerator gets destroyed.

I didn't think of that.  Thanks!

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_connection.cc:87: std::string host_string;
On 2016/07/13 01:08:42, Lei Zhang wrote:
> Only used inside the if block below - move it there.

I needed the c_str buffer to stay in scope.  I switched it to a string instead
since that should be more efficient than copying it.

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_connection.cc:101: httpConnect2(host, port, NULL,
AF_UNSPEC, cups_encryption_,
On 2016/07/13 01:08:42, Lei Zhang wrote:
> nullptr all around

Done.

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_connection.cc:107: impl_ = std::unique_ptr<Impl>(new
Impl(connection));
On 2016/07/13 01:08:42, Lei Zhang wrote:
> Use base::MakeUnique<Impl>(connection) ?

Done.

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
File printing/backend/cups_connection.h (right):

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_connection.h:30: explicit CupsConnection(const GURL&
print_server_url,
On 2016/07/13 01:08:42, Lei Zhang wrote:
> Not explicit

Done.

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_connection.h:38: // Returns the number of destinations and
populates |destinations|.
On 2016/07/13 01:08:42, Lei Zhang wrote:
> Comment doesn't match code.

Done.

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_connection.h:42: CupsPrinter* GetPrinter(const
std::string& printer_name);
On 2016/07/13 01:08:42, Lei Zhang wrote:
> Caller owns the returned pointer, right? Make it return an unique_ptr.

Yes it does.

Done.

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_connection.h:56: class Impl {
On 2016/07/13 01:08:42, Lei Zhang wrote:
> Put this as the first thing in the private section, so all the member
variables
> are together.

Done.

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_connection.h:61: base::WeakPtr<http_t> GetHttp();
On 2016/07/13 01:08:42, Lei Zhang wrote:
> Can you explain the ownership model? Why do we need WeakPtrs?

CupsConnection owns the http_t so I can make sure that it gets closed.  However,
CupsPrinters don't have the same lifetime as the CupsConnection so I wanted to
make sure that a CupsPrinter object would fail gracefully in that situation.  I
would make it shared except that the style guide recommends to avoid shared refs
if possible.

Thoughts?

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
File printing/backend/cups_ipp_util.h (right):

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_ipp_util.h:16: void ExtractColor(const CupsPrinter&
printer,
On 2016/07/13 01:08:42, Lei Zhang wrote:
> Why not just return the color, and let the caller populate |printer_info| ?

Laziness.  I've fixed it so it's more useful.

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
File printing/backend/cups_printer.cc (right):

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_printer.cc:57: return NULL;
On 2016/07/13 01:08:42, Lei Zhang wrote:
> More NULLs

Done.

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_printer.cc:68: if (attr == nullptr) {
On 2016/07/13 01:08:42, Lei Zhang wrote:
> More equality checks again nullptr.

Done.

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_printer.cc:167: if (!make_and_model) {
On 2016/07/13 01:08:42, Lei Zhang wrote:
> return make_and_model ? std::string(make_and_model) : std::string();

Done.

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
File printing/backend/cups_printer.h (right):

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_printer.h:26: class DestinationDeleter {
On 2016/07/13 01:08:42, Lei Zhang wrote:
> Could some of these be shared for use with print_backend_cups.cc? They seem
> generally useful for any CUPS code that have to deal with these structures.

I've moved them to a separate file.  It's probably useful as we convert to smart
pointers.

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_printer.h:41: explicit CupsPrinter(base::WeakPtr<http_t>
http,
On 2016/07/13 01:08:42, Lei Zhang wrote:
> Not explicit.

Done.

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_printer.h:92: bool StreamData(char* buffer, int len);
On 2016/07/13 01:32:04, Lei Zhang wrote:
> Can this take a const std::vector<char>&  - use STL containers as long as
> possible.

Done.

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/print...
File printing/backend/print_backend_chromeos.cc (right):

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/print...
printing/backend/print_backend_chromeos.cc:18: // Provides a stubbed out
PrintBackend implementation for use on ChromeOS.
On 2016/07/13 01:08:43, Lei Zhang (Slow Thursday) wrote:
> Update comment.

Done.

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/print...
printing/backend/print_backend_chromeos.cc:97: CupsConnection
connection(print_server_url,
On 2016/07/13 01:08:43, Lei Zhang wrote:
> This should be an unique_ptr, and PrintBackendCupsIpp should store an
> unique_ptr.
https://google.github.io/styleguide/cppguide.html#Rvalue_references

Thanks.  I somehow missed that in the style guide.

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/print...
File printing/backend/print_backend_cups_ipp.cc (right):

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/print...
printing/backend/print_backend_cups_ipp.cc:51:
printer_list->push_back(std::move(basic_info));
On 2016/07/13 01:08:43, Lei Zhang wrote:
> I don't think the std::move() works here. You still end up calling the copy
> constructor.

Yes.  PrintBasicInfo doesn't have a move constructor so it ends up calling the
copy constructor anyway.

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/print...
printing/backend/print_backend_cups_ipp.cc:66: return "";
On 2016/07/13 01:08:43, Lei Zhang wrote:
> return std::string();

Done.

https://codereview.chromium.org/2105463002/diff/120001/printing/printing.gyp
File printing/printing.gyp (right):

https://codereview.chromium.org/2105463002/diff/120001/printing/printing.gyp#...
printing/printing.gyp:177: 'conditions': [
On 2016/07/13 00:34:27, Lei Zhang wrote:
> I think the redness means you have tabs, and not spaces.

Done.

https://codereview.chromium.org/2105463002/diff/120001/printing/printing.gyp#...
printing/printing.gyp:189: }],
On 2016/07/13 00:34:27, Lei Zhang wrote:
> GYP can do if-else:
> 
> ['chromeos==1', {
>   ...
> }, {
>   ... # else case
> }],

I didn't see that.  Thanks.

https://codereview.chromium.org/2105463002/diff/120001/printing/printing.gyp#...
printing/printing.gyp:213: 'printed_document_linux.cc',
On 2016/07/13 00:34:27, Lei Zhang wrote:
> alphabetical order

Done.

[skau, 2016-07-14 20:44:23]

The CQ bit was checked by skau@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-14 20:44:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-14 23:17:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-14 23:17:04]

Dry run: This issue passed the CQ dry run.

[skau, 2016-07-18 17:40:58]

Friendly ping. :)

[Lei Zhang, 2016-07-19 19:32:58]

A bit backlogged here. I'll finish reviewing in the afternoon.

https://codereview.chromium.org/2105463002/diff/120001/printing/BUILD.gn
File printing/BUILD.gn (right):

https://codereview.chromium.org/2105463002/diff/120001/printing/BUILD.gn#newc...
printing/BUILD.gn:174: "backend/cups_connection.cc",
On 2016/07/14 20:43:05, skau wrote:
> On 2016/07/13 00:34:27, Lei Zhang wrote:
> > Can we use this backend on Mac as well?
> 
> It will be compatible.  I did a test on my Macbook and it worked.  However,
> there's no IPP attribute to CDD translation right now so the old CUPS backend
is
> still needed for devices that can act as a cloud print connector.  Also, my
plan
> is to validate it on Chrome OS first before flipping Mac.  Sound good?

Sure. We don't have to do everything in one CL. Just wondering about the
feasibility and future work.

https://codereview.chromium.org/2105463002/diff/180001/printing/backend/cups_...
File printing/backend/cups_deleters.h (right):

https://codereview.chromium.org/2105463002/diff/180001/printing/backend/cups_...
printing/backend/cups_deleters.h:12: class HttpDeleter {
Can we make these structs and avoid all the default ctors? See
base/memory/free_deleter.h and base/files/scoped_file.h for examples of other
deleters/closers. Forgot to mention this earlier.

[Lei Zhang, 2016-07-19 22:26:39]

Just thinking about the object lifetimes now...

https://codereview.chromium.org/2105463002/diff/180001/printing/backend/cups_...
File printing/backend/cups_connection.cc (right):

https://codereview.chromium.org/2105463002/diff/180001/printing/backend/cups_...
printing/backend/cups_connection.cc:34: std::vector<std::unique_ptr<cups_dest_t,
printing::DestinationDeleter>>&
Getting a bit unwieldy to write. Might be good to add a using Foo =
std::vector<...> line and reuse Foo here and on line 40.

https://codereview.chromium.org/2105463002/diff/180001/printing/backend/cups_...
printing/backend/cups_connection.cc:129: dests.clear();  // CupsPrinter takes
ownership of all the cups_dest_t objects
Do you feel it would be clearer if we did:

auto dests = std::move(enumerator.get_dests());
std::vector<CupsPrinter> printers;
for (...) { ... }
// no need to clear here
return printers;

https://codereview.chromium.org/2105463002/diff/180001/printing/backend/cups_...
File printing/backend/cups_ipp_util.cc (right):

https://codereview.chromium.org/2105463002/diff/180001/printing/backend/cups_...
printing/backend/cups_ipp_util.cc:44: const std::map<const base::StringPiece,
ColorModel> kColorMap{
Can't do this. It adds a static initializer. Given the size of |kColorMap|, just
make this a C array and iterate through it. Remove the <map> include too.

const struct ColorList {
  const char* color;
  ColorModel model;
} kColorList[] = {
  { ... },
  ...
};

https://codereview.chromium.org/2105463002/diff/180001/printing/backend/cups_...
printing/backend/cups_ipp_util.cc:190: }  // anonymous namespace
no "anonymous "

https://codereview.chromium.org/2105463002/diff/180001/printing/backend/cups_...
printing/backend/cups_ipp_util.cc:198: const char* value = ippGetString(attr, 0,
nullptr);
Just combine with the next line and drop |value|. Ditto in a few places below.

https://codereview.chromium.org/2105463002/diff/180001/printing/backend/cups_...
printing/backend/cups_ipp_util.cc:233: PrinterSemanticCapsAndDefaults::Paper p =
ParsePaper(paper);
Also less wordy if it's just combined with the next line.

https://codereview.chromium.org/2105463002/diff/180001/printing/backend/cups_...
printing/backend/cups_ipp_util.cc:244: if (attr) {
If |attr| is nullptr, what happens? Does |lower_bound| and |upper_bound| get
initialized properly? In the previous patch set, this code just set
|copies_capable| to false directly if |attr| was nullptr.

https://codereview.chromium.org/2105463002/diff/180001/printing/backend/cups_...
printing/backend/cups_ipp_util.cc:255: auto iter = std::find(values.begin(),
values.end(), kCollated);
Just: return ContainsValue(values, kCollated);

Same for PrinterSupportsValue() above. ContainsValue is in base/stl_util.h.

[Lei Zhang, 2016-07-19 22:49:17]

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
File printing/backend/cups_connection.h (right):

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_connection.h:61: base::WeakPtr<http_t> GetHttp();
On 2016/07/14 20:43:05, skau wrote:
> On 2016/07/13 01:08:42, Lei Zhang wrote:
> > Can you explain the ownership model? Why do we need WeakPtrs?
> 
> CupsConnection owns the http_t so I can make sure that it gets closed. 
However,
> CupsPrinters don't have the same lifetime as the CupsConnection so I wanted to
> make sure that a CupsPrinter object would fail gracefully in that situation. 
I
> would make it shared except that the style guide recommends to avoid shared
refs
> if possible.

Can you show we where CupsPrinters can outlive CupsConnection? I'm not seeing
it.

> Thoughts?

Look for "counted" in
https://www.chromium.org/developers/smart-pointer-guidelines and
https://www.chromium.org/developers/coding-style/important-abstractions-and-d...
to see why std::shared_ptr, base::RefCounted, and friends are discouraged.

https://codereview.chromium.org/2105463002/diff/180001/printing/backend/cups_...
File printing/backend/cups_connection.h (right):

https://codereview.chromium.org/2105463002/diff/180001/printing/backend/cups_...
printing/backend/cups_connection.h:67: base::WeakPtr<http_t> cups_http_;
Since CupsConnection owns |impl_|, as written |CuspConnection::cups_http_| is
always valid during its lifetime because destruction happens in reverse order.
This gets destroyed first before |Impl::cups_http_|. Also, nobody ever calls
impl_.reset(), and all the std::move(impl_) calls have corresponding
std::move(cups_http_) calls at the same time.

https://codereview.chromium.org/2105463002/diff/200001/printing/backend/cups_...
File printing/backend/cups_connection.cc (right):

https://codereview.chromium.org/2105463002/diff/200001/printing/backend/cups_...
printing/backend/cups_connection.cc:16: static const int kTimeoutMs = 3000;
Noo static inside anonymous naemspaces

https://codereview.chromium.org/2105463002/diff/200001/printing/backend/cups_...
printing/backend/cups_connection.cc:20: static int cups_callback(void*
user_data, unsigned flags, cups_dest_t* dest) {
Can this be a private method?

https://codereview.chromium.org/2105463002/diff/200001/printing/backend/cups_...
printing/backend/cups_connection.cc:32: // Returns the collected destinations. 
Remove desired destinations from the
I think the Remove... comment can be inferred from the container of
std::unique_ptr behavior.

https://codereview.chromium.org/2105463002/diff/200001/printing/backend/cups_...
printing/backend/cups_connection.cc:34: std::vector<std::unique_ptr<cups_dest_t,
printing::DestinationDeleter>>&
If you extend the printing namespace to color the anonymous namespace, you can
avoid using printing:: and this might fix on 1 line.

https://codereview.chromium.org/2105463002/diff/200001/printing/backend/cups_...
printing/backend/cups_connection.cc:42: };
DISALLOW_COPY_AND_ASSIGN() is always good to have.

[skau, 2016-07-20 23:56:26]

The CQ bit was checked by skau@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 23:57:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skau, 2016-07-20 23:58:23]

Comments addressed.  Added some tests and documented the lifetime requirements
for CupsPrinter.

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
File printing/backend/cups_connection.h (right):

https://codereview.chromium.org/2105463002/diff/120001/printing/backend/cups_...
printing/backend/cups_connection.h:61: base::WeakPtr<http_t> GetHttp();
On 2016/07/19 22:49:17, Lei Zhang wrote:
> On 2016/07/14 20:43:05, skau wrote:
> > On 2016/07/13 01:08:42, Lei Zhang wrote:
> > > Can you explain the ownership model? Why do we need WeakPtrs?
> > 
> > CupsConnection owns the http_t so I can make sure that it gets closed. 
> However,
> > CupsPrinters don't have the same lifetime as the CupsConnection so I wanted
to
> > make sure that a CupsPrinter object would fail gracefully in that situation.

> I
> > would make it shared except that the style guide recommends to avoid shared
> refs
> > if possible.
> 
> Can you show we where CupsPrinters can outlive CupsConnection? I'm not seeing
> it.
> 
> > Thoughts?
> 
> Look for "counted" in
> https://www.chromium.org/developers/smart-pointer-guidelines and
>
https://www.chromium.org/developers/coding-style/important-abstractions-and-d...
> to see why std::shared_ptr, base::RefCounted, and friends are discouraged.

Per offline discussion, building printers with raw pointers and documented the
requirement.

https://codereview.chromium.org/2105463002/diff/180001/printing/backend/cups_...
File printing/backend/cups_connection.cc (right):

https://codereview.chromium.org/2105463002/diff/180001/printing/backend/cups_...
printing/backend/cups_connection.cc:34: std::vector<std::unique_ptr<cups_dest_t,
printing::DestinationDeleter>>&
On 2016/07/19 22:26:39, Lei Zhang wrote:
> Getting a bit unwieldy to write. Might be good to add a using Foo =
> std::vector<...> line and reuse Foo here and on line 40.

Moving the class into the printing namespace worked out.

https://codereview.chromium.org/2105463002/diff/180001/printing/backend/cups_...
printing/backend/cups_connection.cc:129: dests.clear();  // CupsPrinter takes
ownership of all the cups_dest_t objects
On 2016/07/19 22:26:39, Lei Zhang wrote:
> Do you feel it would be clearer if we did:
> 
> auto dests = std::move(enumerator.get_dests());
> std::vector<CupsPrinter> printers;
> for (...) { ... }
> // no need to clear here
> return printers;

That seems reasonable.  I'll defer to you as to if it's clearer.  The affect of
the move isn't immediately clear to me.  It's because get_dests() returns a 
reference correct?

https://codereview.chromium.org/2105463002/diff/180001/printing/backend/cups_...
File printing/backend/cups_deleters.h (right):

https://codereview.chromium.org/2105463002/diff/180001/printing/backend/cups_...
printing/backend/cups_deleters.h:12: class HttpDeleter {
On 2016/07/19 19:32:58, Lei Zhang wrote:
> Can we make these structs and avoid all the default ctors? See
> base/memory/free_deleter.h and base/files/scoped_file.h for examples of other
> deleters/closers. Forgot to mention this earlier.

Done.

https://codereview.chromium.org/2105463002/diff/180001/printing/backend/cups_...
File printing/backend/cups_ipp_util.cc (right):

https://codereview.chromium.org/2105463002/diff/180001/printing/backend/cups_...
printing/backend/cups_ipp_util.cc:44: const std::map<const base::StringPiece,
ColorModel> kColorMap{
On 2016/07/19 22:26:39, Lei Zhang wrote:
> Can't do this. It adds a static initializer. Given the size of |kColorMap|,
just
> make this a C array and iterate through it. Remove the <map> include too.
> 
> const struct ColorList {
>   const char* color;
>   ColorModel model;
> } kColorList[] = {
>   { ... },
>   ...
> };

Thanks.  That works well.

https://codereview.chromium.org/2105463002/diff/180001/printing/backend/cups_...
printing/backend/cups_ipp_util.cc:190: }  // anonymous namespace
On 2016/07/19 22:26:39, Lei Zhang wrote:
> no "anonymous "

Done.

https://codereview.chromium.org/2105463002/diff/180001/printing/backend/cups_...
printing/backend/cups_ipp_util.cc:198: const char* value = ippGetString(attr, 0,
nullptr);
On 2016/07/19 22:26:39, Lei Zhang wrote:
> Just combine with the next line and drop |value|. Ditto in a few places below.

Done.

https://codereview.chromium.org/2105463002/diff/180001/printing/backend/cups_...
printing/backend/cups_ipp_util.cc:233: PrinterSemanticCapsAndDefaults::Paper p =
ParsePaper(paper);
On 2016/07/19 22:26:39, Lei Zhang wrote:
> Also less wordy if it's just combined with the next line.

Done.

https://codereview.chromium.org/2105463002/diff/180001/printing/backend/cups_...
printing/backend/cups_ipp_util.cc:244: if (attr) {
On 2016/07/19 22:26:39, Lei Zhang wrote:
> If |attr| is nullptr, what happens? Does |lower_bound| and |upper_bound| get
> initialized properly? In the previous patch set, this code just set
> |copies_capable| to false directly if |attr| was nullptr.

Whoops.  I'll write some unit tests.

https://codereview.chromium.org/2105463002/diff/180001/printing/backend/cups_...
printing/backend/cups_ipp_util.cc:255: auto iter = std::find(values.begin(),
values.end(), kCollated);
On 2016/07/19 22:26:39, Lei Zhang wrote:
> Just: return ContainsValue(values, kCollated);
> 
> Same for PrinterSupportsValue() above. ContainsValue is in base/stl_util.h.

Thanks.  I didn't know we had that.

https://codereview.chromium.org/2105463002/diff/200001/printing/backend/cups_...
File printing/backend/cups_connection.cc (right):

https://codereview.chromium.org/2105463002/diff/200001/printing/backend/cups_...
printing/backend/cups_connection.cc:16: static const int kTimeoutMs = 3000;
On 2016/07/19 22:49:17, Lei Zhang wrote:
> Noo static inside anonymous naemspaces

Done.

https://codereview.chromium.org/2105463002/diff/200001/printing/backend/cups_...
printing/backend/cups_connection.cc:20: static int cups_callback(void*
user_data, unsigned flags, cups_dest_t* dest) {
On 2016/07/19 22:49:17, Lei Zhang wrote:
> Can this be a private method?

Unfortunately not.  I need a function pointer matching cups_..._cp_t* which
can't be constructed from a member function.  But I definitely tried!  Long
story short, the class becomes part of the type of the function.

https://codereview.chromium.org/2105463002/diff/200001/printing/backend/cups_...
printing/backend/cups_connection.cc:32: // Returns the collected destinations. 
Remove desired destinations from the
On 2016/07/19 22:49:17, Lei Zhang wrote:
> I think the Remove... comment can be inferred from the container of
> std::unique_ptr behavior.

Done.

https://codereview.chromium.org/2105463002/diff/200001/printing/backend/cups_...
printing/backend/cups_connection.cc:34: std::vector<std::unique_ptr<cups_dest_t,
printing::DestinationDeleter>>&
On 2016/07/19 22:49:17, Lei Zhang wrote:
> If you extend the printing namespace to color the anonymous namespace, you can
> avoid using printing:: and this might fix on 1 line.

Done.

https://codereview.chromium.org/2105463002/diff/200001/printing/backend/cups_...
printing/backend/cups_connection.cc:42: };
On 2016/07/19 22:49:17, Lei Zhang wrote:
> DISALLOW_COPY_AND_ASSIGN() is always good to have.

Done.

[Lei Zhang, 2016-07-21 00:46:20]

I think it's gonna be mostly nits from here onwards.

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
File printing/backend/cups_connection.cc (right):

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
printing/backend/cups_connection.cc:80: http_t* connection =
Drop |connection| and just do: cups_http_.reset(...), then return !!cups_http_;

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
File printing/backend/cups_ipp_util.cc (right):

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
printing/backend/cups_ipp_util.cc:55: for (const ColorMap color : kColorList) {
const ColorMap&

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
printing/backend/cups_ipp_util.cc:249: // Retrieves the supported number of
copies from |printer| and writing the
Comments to describe a function go where it is declared, in this case, in the
header.

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
File printing/backend/cups_ipp_util_unittest.cc (right):

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
printing/backend/cups_ipp_util_unittest.cc:33:
strings.emplace_back(ippGetString(attr, i, NULL));
nullptr, more below. :-P

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
printing/backend/cups_ipp_util_unittest.cc:73: delete printer_;
unique_ptr, reset() here.

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
printing/backend/cups_ipp_util_unittest.cc:99:
printing::PrinterSemanticCapsAndDefaults caps;
Put the entire thing in namespace printing and save yourself some typing.

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
printing/backend/cups_ipp_util_unittest.cc:100: 
too

many

blank spaces.

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
printing/backend/cups_ipp_util_unittest.cc:175: EXPECT_EQ(paper.display_name,
"iso a4");
EXPECT_EQ(expected_value, actual_value);

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
File printing/backend/cups_printer.cc (right):

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
printing/backend/cups_printer.cc:173: cups_dinfo_t* info =
cupsCopyDestInfo(cups_http_, destination_.get());
Same pattern, dest_info_.reset(cupsFoo()); return !!dest_info_;

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
printing/backend/cups_printer.cc:215: DCHECK(cups_http_);
Given cups_http_ can be "http_t* const", if you want this DCHECK, then do it in
the ctor.

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
printing/backend/cups_printer.cc:216: DCHECK(destination_);
BTW, we use |destination_| and |dest_info_| all over. Why do we only DCHECK
here?

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
File printing/backend/cups_printer.h (right):

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
printing/backend/cups_printer.h:57: // This object now owns |dest| and |info|.
No need to explain std::unique_ptr semantics.

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
printing/backend/cups_printer.h:69: ipp_attribute_t* GetSupportedOptionValues(
Bunch up all the overrides:

// CupsOptionProvider:
void Foo() override;
void Bar() override;
...
void Last() override;

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
printing/backend/cups_printer.h:85: const std::string GetName() const;
If you are returning a copy of the string, there's no need to force const-ness.
Caller should be free to do whatever they want with their copy.

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
printing/backend/cups_printer.h:115: http_t* cups_http_;
This can be: http_t* const cups_http_; since the pointer itself never changes.

[commit-bot: I haz the power, 2016-07-21 00:55:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-21 00:55:14]

Dry run: This issue passed the CQ dry run.

[skau, 2016-07-21 20:07:34]

Comments addressed.  PTAL.

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
File printing/backend/cups_connection.cc (right):

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
printing/backend/cups_connection.cc:80: http_t* connection =
On 2016/07/21 00:46:19, Lei Zhang wrote:
> Drop |connection| and just do: cups_http_.reset(...), then return
!!cups_http_;

Done.

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
File printing/backend/cups_ipp_util.cc (right):

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
printing/backend/cups_ipp_util.cc:55: for (const ColorMap color : kColorList) {
On 2016/07/21 00:46:19, Lei Zhang wrote:
> const ColorMap&

Done.

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
printing/backend/cups_ipp_util.cc:249: // Retrieves the supported number of
copies from |printer| and writing the
On 2016/07/21 00:46:19, Lei Zhang wrote:
> Comments to describe a function go where it is declared, in this case, in the
> header.

Done.

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
File printing/backend/cups_ipp_util_unittest.cc (right):

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
printing/backend/cups_ipp_util_unittest.cc:33:
strings.emplace_back(ippGetString(attr, i, NULL));
On 2016/07/21 00:46:19, Lei Zhang wrote:
> nullptr, more below. :-P

I'll break this habit soon!

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
printing/backend/cups_ipp_util_unittest.cc:73: delete printer_;
On 2016/07/21 00:46:19, Lei Zhang wrote:
> unique_ptr, reset() here.

Done.

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
printing/backend/cups_ipp_util_unittest.cc:99:
printing::PrinterSemanticCapsAndDefaults caps;
On 2016/07/21 00:46:19, Lei Zhang wrote:
> Put the entire thing in namespace printing and save yourself some typing.

Done.

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
printing/backend/cups_ipp_util_unittest.cc:100: 
On 2016/07/21 00:46:19, Lei Zhang wrote:
> too
> 
> many
> 
> blank spaces.

lol

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
printing/backend/cups_ipp_util_unittest.cc:175: EXPECT_EQ(paper.display_name,
"iso a4");
On 2016/07/21 00:46:20, Lei Zhang wrote:
> EXPECT_EQ(expected_value, actual_value);

Done.

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
File printing/backend/cups_printer.cc (right):

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
printing/backend/cups_printer.cc:173: cups_dinfo_t* info =
cupsCopyDestInfo(cups_http_, destination_.get());
On 2016/07/21 00:46:20, Lei Zhang wrote:
> Same pattern, dest_info_.reset(cupsFoo()); return !!dest_info_;

Done.

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
printing/backend/cups_printer.cc:215: DCHECK(cups_http_);
On 2016/07/21 00:46:20, Lei Zhang wrote:
> Given cups_http_ can be "http_t* const", if you want this DCHECK, then do it
in
> the ctor.

Done.

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
printing/backend/cups_printer.cc:216: DCHECK(destination_);
On 2016/07/21 00:46:20, Lei Zhang wrote:
> BTW, we use |destination_| and |dest_info_| all over. Why do we only DCHECK
> here?

The code was moved from where there were different guarantees.  It's fixed now.

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
File printing/backend/cups_printer.h (right):

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
printing/backend/cups_printer.h:57: // This object now owns |dest| and |info|.
On 2016/07/21 00:46:20, Lei Zhang wrote:
> No need to explain std::unique_ptr semantics.

Done.

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
printing/backend/cups_printer.h:69: ipp_attribute_t* GetSupportedOptionValues(
On 2016/07/21 00:46:20, Lei Zhang wrote:
> Bunch up all the overrides:
> 
> // CupsOptionProvider:
> void Foo() override;
> void Bar() override;
> ...
> void Last() override;

Done.

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
printing/backend/cups_printer.h:85: const std::string GetName() const;
On 2016/07/21 00:46:20, Lei Zhang wrote:
> If you are returning a copy of the string, there's no need to force
const-ness.
> Caller should be free to do whatever they want with their copy.

Done.

https://codereview.chromium.org/2105463002/diff/260001/printing/backend/cups_...
printing/backend/cups_printer.h:115: http_t* cups_http_;
On 2016/07/21 00:46:20, Lei Zhang wrote:
> This can be: http_t* const cups_http_; since the pointer itself never changes.

Done.

[Lei Zhang, 2016-07-21 20:31:31]

lgtm, thanks for addressing all the nitty gritty bits.

https://codereview.chromium.org/2105463002/diff/280001/printing/backend/cups_...
File printing/backend/cups_ipp_util.h (right):

https://codereview.chromium.org/2105463002/diff/280001/printing/backend/cups_...
printing/backend/cups_ipp_util.h:31: // extremeties of the range into
|lower_bound| and |upper_bound|  Values are set
s/  /. /

https://codereview.chromium.org/2105463002/diff/280001/printing/backend/cups_...
File printing/backend/cups_printer.h (right):

https://codereview.chromium.org/2105463002/diff/280001/printing/backend/cups_...
printing/backend/cups_printer.h:59: // lazily initialized when needed.
Combine with previous line. If you are using "git cl format" to automagically
fix formatting, be aware that it won't fix this because it doesn't want to
accidentally ruin anyone's ASCII art.

[skau, 2016-07-21 23:33:10]

Thank you very much.  All done.

https://codereview.chromium.org/2105463002/diff/280001/printing/backend/cups_...
File printing/backend/cups_ipp_util.h (right):

https://codereview.chromium.org/2105463002/diff/280001/printing/backend/cups_...
printing/backend/cups_ipp_util.h:31: // extremeties of the range into
|lower_bound| and |upper_bound|  Values are set
On 2016/07/21 20:31:30, Lei Zhang wrote:
> s/  /. /

Done.

https://codereview.chromium.org/2105463002/diff/280001/printing/backend/cups_...
File printing/backend/cups_printer.h (right):

https://codereview.chromium.org/2105463002/diff/280001/printing/backend/cups_...
printing/backend/cups_printer.h:59: // lazily initialized when needed.
On 2016/07/21 20:31:30, Lei Zhang wrote:
> Combine with previous line. If you are using "git cl format" to automagically
> fix formatting, be aware that it won't fix this because it doesn't want to
> accidentally ruin anyone's ASCII art.

I have been using git cl format.  Thanks.

[skau, 2016-07-21 23:33:21]

The CQ bit was checked by skau@chromium.org

[skau, 2016-07-21 23:33:22]

The CQ bit was unchecked by skau@chromium.org

[skau, 2016-07-21 23:34:24]

The CQ bit was checked by skau@chromium.org

[skau, 2016-07-21 23:34:24]

The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2105463002/#ps300001
(title: "Fix typos.")

[commit-bot: I haz the power, 2016-07-21 23:35:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 03:16:57]

Description was changed from

==========
Implement a new backend for modern CUPS implementations
    
As of CUPS 1.6, CUPS has moved away from PPD attributes to IPP attributes.  This
new backend uses the IPP APIs and will be better suited to use with CUPS going
forward.  Until we can bump our minumum CUPS version to 1.7, the only client
will be Chrome OS where the version is 2.1.3.
    
BUG=607668
    
TEST=With --enable-native-cups enabled, print dialog will show available CUPS
printers if any are configured.
==========

to

==========
Implement a new backend for modern CUPS implementations
    
As of CUPS 1.6, CUPS has moved away from PPD attributes to IPP attributes.  This
new backend uses the IPP APIs and will be better suited to use with CUPS going
forward.  Until we can bump our minumum CUPS version to 1.7, the only client
will be Chrome OS where the version is 2.1.3.
    
BUG=607668
    
TEST=With --enable-native-cups enabled, print dialog will show available CUPS
printers if any are configured.
==========

[commit-bot: I haz the power, 2016-07-22 03:17:04]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-07-22 03:19:50]

Description was changed from

==========
Implement a new backend for modern CUPS implementations
    
As of CUPS 1.6, CUPS has moved away from PPD attributes to IPP attributes.  This
new backend uses the IPP APIs and will be better suited to use with CUPS going
forward.  Until we can bump our minumum CUPS version to 1.7, the only client
will be Chrome OS where the version is 2.1.3.
    
BUG=607668
    
TEST=With --enable-native-cups enabled, print dialog will show available CUPS
printers if any are configured.
==========

to

==========
Implement a new backend for modern CUPS implementations

As of CUPS 1.6, CUPS has moved away from PPD attributes to IPP attributes.  This
new backend uses the IPP APIs and will be better suited to use with CUPS going
forward.  Until we can bump our minumum CUPS version to 1.7, the only client
will be Chrome OS where the version is 2.1.3.

BUG=607668

TEST=With --enable-native-cups enabled, print dialog will show available CUPS
printers if any are configured.

Committed: https://crrev.com/b779ce61d5369a930af480c265c0c5b924888914
Cr-Commit-Position: refs/heads/master@{#407011}
==========

[commit-bot: I haz the power, 2016-07-22 03:19:51]

Patchset 16 (id:??) landed as
https://crrev.com/b779ce61d5369a930af480c265c0c5b924888914
Cr-Commit-Position: refs/heads/master@{#407011}