CUPS printing: Do not add scanners to the printer list.

CUPS printing: Do not add scanners to the printer list.

BUG=91755
TEST=see bug

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95949

[Lei Zhang, 2011-08-08 19:38:38]

kmadhusu: code review
mark: is the Mac #define ok?

[kmadhusu, 2011-08-08 20:09:34]

LGTM (with a nit)

http://codereview.chromium.org/7569028/diff/2001/printing/backend/print_backe...
File printing/backend/print_backend_cups.cc (right):

http://codereview.chromium.org/7569028/diff/2001/printing/backend/print_backe...
printing/backend/print_backend_cups.cc:139: base::StringToInt(type_str, &type);
nit: Handle if (!base::StringToInt(...))

[Lei Zhang, 2011-08-08 20:27:25]

http://codereview.chromium.org/7569028/diff/2001/printing/backend/print_backe...
File printing/backend/print_backend_cups.cc (right):

http://codereview.chromium.org/7569028/diff/2001/printing/backend/print_backe...
printing/backend/print_backend_cups.cc:139: base::StringToInt(type_str, &type);
On 2011/08/08 20:09:35, kmadhusu wrote:
> nit: Handle if (!base::StringToInt(...))

Done.

[Mark Mentovai, 2011-08-08 21:02:07]

LG otherwise

http://codereview.chromium.org/7569028/diff/5001/printing/backend/print_backe...
File printing/backend/print_backend_cups.cc (right):

http://codereview.chromium.org/7569028/diff/5001/printing/backend/print_backe...
printing/backend/print_backend_cups.cc:25: // Mac still builds against the 10.5
SDK, which does not have this in cups.h.
Don’t do this. You need more #if:

#if defined(OS_MACOSX)
#include <AvailabilityMacros.h>
#endif

#if defined(OS_MACOSX)
#if MAC_OS_X_VERSION_MAX_ALLOWED <= MAC_OS_X_VERSION_10_5
const int CUPS_PRINTER_SCANNER = 0x2000000;  // Scanner-only device
#endif
#else

That says “if the SDK is 10.5 or less, declare this…”

I’ve used const int because the system header uses an enum. Since it’s a named
enum, cups_ptype_e, we can’t add a new value to it here. A const int is closer
in spirit to a member of an enum than a #define is.

“Inserting” values into enums in this fashion is dangerous in light of certain
compiler optimizations. Be careful out there!

Note that I’ve also copied the comment from the system header. I consider this
important.

[Lei Zhang, 2011-08-08 22:04:02]

http://codereview.chromium.org/7569028/diff/5001/printing/backend/print_backe...
File printing/backend/print_backend_cups.cc (right):

http://codereview.chromium.org/7569028/diff/5001/printing/backend/print_backe...
printing/backend/print_backend_cups.cc:25: // Mac still builds against the 10.5
SDK, which does not have this in cups.h.
Mark: Thanks, more #ifs added.

On 2011/08/08 21:02:07, Mark Mentovai wrote:
> Don’t do this. You need more #if:
> 
> #if defined(OS_MACOSX)
> #include <AvailabilityMacros.h>
> #endif
> 
> #if defined(OS_MACOSX)
> #if MAC_OS_X_VERSION_MAX_ALLOWED <= MAC_OS_X_VERSION_10_5
> const int CUPS_PRINTER_SCANNER = 0x2000000;  // Scanner-only device
> #endif
> #else
> 
> That says “if the SDK is 10.5 or less, declare this…”
> 
> I’ve used const int because the system header uses an enum. Since it’s a named
> enum, cups_ptype_e, we can’t add a new value to it here. A const int is closer
> in spirit to a member of an enum than a #define is.
> 
> “Inserting” values into enums in this fashion is dangerous in light of certain
> compiler optimizations. Be careful out there!
> 
> Note that I’ve also copied the comment from the system header. I consider this
> important.

[Mark Mentovai, 2011-08-08 22:11:05]

LGTM

http://codereview.chromium.org/7569028/diff/4002/printing/backend/print_backe...
File printing/backend/print_backend_cups.cc (right):

http://codereview.chromium.org/7569028/diff/4002/printing/backend/print_backe...
printing/backend/print_backend_cups.cc:11: #if defined(OS_MACOSX)
The chain that you’re using to pick up this definition is pretty weak. Add your
own #include for "build/build_config.h"?

[Lei Zhang, 2011-08-08 22:28:08]

http://codereview.chromium.org/7569028/diff/4002/printing/backend/print_backe...
File printing/backend/print_backend_cups.cc (right):

http://codereview.chromium.org/7569028/diff/4002/printing/backend/print_backe...
printing/backend/print_backend_cups.cc:11: #if defined(OS_MACOSX)
On 2011/08/08 22:11:05, Mark Mentovai wrote:
> The chain that you’re using to pick up this definition is pretty weak. Add
your
> own #include for "build/build_config.h"?

Done.

[Mark Mentovai, 2011-08-08 22:29:43]

LGTM

[commit-bot: I haz the power, 2011-08-09 05:41:28]

Change committed as 95949