Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7980036:
CUPS printing: Define CUPS_PRINTER_SCANNER for Linux if CUPS is < 1.4 (Closed)
|
Created:
9 years, 3 months ago by chrelad Modified:
9 years, 3 months ago CC:
chromium-reviews Visibility:
Public. |
DescriptionCUPS printing: Define CUPS_PRINTER_SCANNER for Linux if CUPS is < 1.4
BUG=97409
TEST=Try compiling on Linux with CUPS < 1.4
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102248
Patch Set 1 #
Total comments: 6
Patch Set 2 : Combining definitions of CUPS_PRINTER_SCANNER #
Total comments: 2
Patch Set 3 : Indenting conditionals, uncommenting #endif #Patch Set 4 : Broke the OS_LINUX conditions onto the last line #
Total comments: 1
Patch Set 5 : Rebasing AUTHORS which was behind trunk #Patch Set 6 : Removing entry after attempt to reconsile merge conflict #Patch Set 7 : Trying again to resolve merge conflicts with ToT #Patch Set 8 : Interactive rebase to git the AUTHORS file straightened out #Messages
Total messages: 19 (0 generated)
CUPS printing: Define CUPS_PRINTER_SCANNER for Linux if CUPS is < 1.4 BUG=97409 TEST=Try compiling on Linux with CUPS < 1.4
+thestig@ (since he wrote the mac specific code) Thanks. http://codereview.chromium.org/7980036/diff/1/printing/backend/print_backend_... File printing/backend/print_backend_cups.cc (right): http://codereview.chromium.org/7980036/diff/1/printing/backend/print_backend_... printing/backend/print_backend_cups.cc:88: const int CUPS_PRINTER_SCANNER = 0x2000000; // Scanner-only device lines 29-32 declares this variables for OS_MAC. You may want to add this linux specific code there. #if defined (USE_CUPS) #if (defined(OS_LINUX) && CUPS_VERSION_MAJOR == 1 && CUPS_VERSION_MINOR < 4) || (defined(OS_MACOSX).....) const int CUPS_PRINTER_SCANNER = ... #endif #endif
Hmmm, not working well for me due to the 80 character limit. Due to the long conditional, my wrapping is causing errors like: printing/backend/print_backend_cups.cc:29:27: error: operator '&&' has no right operand Any thoughts on how to get the entire conditional one one line or avoid the compiler errors? http://codereview.chromium.org/7980036/diff/1/printing/backend/print_backend_... File printing/backend/print_backend_cups.cc (right): http://codereview.chromium.org/7980036/diff/1/printing/backend/print_backend_... printing/backend/print_backend_cups.cc:88: const int CUPS_PRINTER_SCANNER = 0x2000000; // Scanner-only device Hmmm, not working well for me due to the 80 character limit. Due to the long conditional, my wrapping is causing errors like: printing/backend/print_backend_cups.cc:29:27: error: operator '&&' has no right operand Any thoughts on how to get the entire conditional one one line or avoid the compiler errors? On 2011/09/21 17:15:30, kmadhusu wrote: > lines 29-32 declares this variables for OS_MAC. You may want to add this linux > specific code there. > > #if defined (USE_CUPS) > #if (defined(OS_LINUX) && CUPS_VERSION_MAJOR == 1 && CUPS_VERSION_MINOR < 4) || > (defined(OS_MACOSX).....) > const int CUPS_PRINTER_SCANNER = ... > #endif > #endif
http://codereview.chromium.org/7980036/diff/1/printing/backend/print_backend_... File printing/backend/print_backend_cups.cc (right): http://codereview.chromium.org/7980036/diff/1/printing/backend/print_backend_... printing/backend/print_backend_cups.cc:88: const int CUPS_PRINTER_SCANNER = 0x2000000; // Scanner-only device On 2011/09/21 17:53:10, chrelad wrote: > Hmmm, not working well for me due to the 80 character limit. Due to the long > conditional, my wrapping is causing errors like: > > printing/backend/print_backend_cups.cc:29:27: error: operator '&&' has no right > operand > > Any thoughts on how to get the entire conditional one one line or avoid the > compiler errors? > On 2011/09/21 17:15:30, kmadhusu wrote: > > lines 29-32 declares this variables for OS_MAC. You may want to add this linux > > specific code there. > > > > #if defined (USE_CUPS) > > #if (defined(OS_LINUX) && CUPS_VERSION_MAJOR == 1 && CUPS_VERSION_MINOR < 4) > || > > (defined(OS_MACOSX).....) > > const int CUPS_PRINTER_SCANNER = ... > > #endif > > #endif > Try using line continuation '\' character before the wrap. Eg: #if (defined(OS_POSIX) && \ defined(ABC) && _POSIX_MONOTONIC_CLOCK >= 0) || \ defined(OS_FREEBSD) || defined(OS_OPENBSD) || defined(OS_ANDROID)
http://codereview.chromium.org/7980036/diff/1/printing/backend/print_backend_... File printing/backend/print_backend_cups.cc (right): http://codereview.chromium.org/7980036/diff/1/printing/backend/print_backend_... printing/backend/print_backend_cups.cc:88: const int CUPS_PRINTER_SCANNER = 0x2000000; // Scanner-only device On 2011/09/21 18:01:50, kmadhusu wrote: > On 2011/09/21 17:53:10, chrelad wrote: > > Hmmm, not working well for me due to the 80 character limit. Due to the long > > conditional, my wrapping is causing errors like: > > > > printing/backend/print_backend_cups.cc:29:27: error: operator '&&' has no > right > > operand > > > > Any thoughts on how to get the entire conditional one one line or avoid the > > compiler errors? > > On 2011/09/21 17:15:30, kmadhusu wrote: > > > lines 29-32 declares this variables for OS_MAC. You may want to add this > linux > > > specific code there. > > > > > > #if defined (USE_CUPS) > > > #if (defined(OS_LINUX) && CUPS_VERSION_MAJOR == 1 && CUPS_VERSION_MINOR < 4) > > || > > > (defined(OS_MACOSX).....) > > > const int CUPS_PRINTER_SCANNER = ... > > > #endif > > > #endif > > > > Try using line continuation '\' character before the wrap. > > Eg: > #if (defined(OS_POSIX) && \ > defined(ABC) && _POSIX_MONOTONIC_CLOCK >= 0) || \ > defined(OS_FREEBSD) || defined(OS_OPENBSD) || defined(OS_ANDROID) This is print_backend_cups.cc, you don't need to check USE_CUPS in here. I'd just put it after line 33. The #else block is effectively POSIX minus MAC.
Moved the conditional definition of CUPS_PRINTER_SCANNER up into the Mac definition. http://codereview.chromium.org/7980036/diff/1/printing/backend/print_backend_... File printing/backend/print_backend_cups.cc (right): http://codereview.chromium.org/7980036/diff/1/printing/backend/print_backend_... printing/backend/print_backend_cups.cc:88: const int CUPS_PRINTER_SCANNER = 0x2000000; // Scanner-only device Done
Huh, okay; well, could you look at the latest version and see if that would work? It's a bit convoluted, but reduces redundancy. Not sure which is better to have, probably redundancy since it's pre-processed out anyway? http://codereview.chromium.org/7980036/diff/1/printing/backend/print_backend_... File printing/backend/print_backend_cups.cc (right): http://codereview.chromium.org/7980036/diff/1/printing/backend/print_backend_... printing/backend/print_backend_cups.cc:88: const int CUPS_PRINTER_SCANNER = 0x2000000; // Scanner-only device Huh, okay; well, could you look at the latest version and see if that would work? On 2011/09/21 18:22:34, Lei Zhang wrote: > On 2011/09/21 18:01:50, kmadhusu wrote: > > On 2011/09/21 17:53:10, chrelad wrote: > > > Hmmm, not working well for me due to the 80 character limit. Due to the long > > > conditional, my wrapping is causing errors like: > > > > > > printing/backend/print_backend_cups.cc:29:27: error: operator '&&' has no > > right > > > operand > > > > > > Any thoughts on how to get the entire conditional one one line or avoid the > > > compiler errors? > > > On 2011/09/21 17:15:30, kmadhusu wrote: > > > > lines 29-32 declares this variables for OS_MAC. You may want to add this > > linux > > > > specific code there. > > > > > > > > #if defined (USE_CUPS) > > > > #if (defined(OS_LINUX) && CUPS_VERSION_MAJOR == 1 && CUPS_VERSION_MINOR < > 4) > > > || > > > > (defined(OS_MACOSX).....) > > > > const int CUPS_PRINTER_SCANNER = ... > > > > #endif > > > > #endif > > > > > > > Try using line continuation '\' character before the wrap. > > > > Eg: > > #if (defined(OS_POSIX) && \ > > defined(ABC) && _POSIX_MONOTONIC_CLOCK >= 0) || \ > > defined(OS_FREEBSD) || defined(OS_OPENBSD) || defined(OS_ANDROID) > > This is print_backend_cups.cc, you don't need to check USE_CUPS in here. I'd > just put it after line 33. The #else block is effectively POSIX minus MAC.
http://codereview.chromium.org/7980036/diff/4001/printing/backend/print_backe... File printing/backend/print_backend_cups.cc (right): http://codereview.chromium.org/7980036/diff/4001/printing/backend/print_backe... printing/backend/print_backend_cups.cc:32: CUPS_VERSION_MINOR < 4) How about doing a little formatting to line up the conditions so it's more readable? You don't need the comment on the #endif. #if (defined(OS_MACOSX) && \ MAC_OS_X_VERSION_MAX_ALLOWED <= MAC_OS_X_VERSION_10_5) \ (defined(OS_LINUX) && \ CUPS_VERSION_MAJOR == 1 && CUPS_VERSION_MINOR < 4)
Indenting the conditionals and removing the comment on the #endif. http://codereview.chromium.org/7980036/diff/4001/printing/backend/print_backe... File printing/backend/print_backend_cups.cc (right): http://codereview.chromium.org/7980036/diff/4001/printing/backend/print_backe... printing/backend/print_backend_cups.cc:32: CUPS_VERSION_MINOR < 4) Done
Broke the OS_LINUX conditions onto last line per Lei Zhang's comment; missed that in the second patch. http://codereview.chromium.org/7980036/diff/11/printing/backend/print_backend... File printing/backend/print_backend_cups.cc (right): http://codereview.chromium.org/7980036/diff/11/printing/backend/print_backend... printing/backend/print_backend_cups.cc:31: (defined(OS_LINUX) && \ Broke the OS_LINUX conditions onto last line per Lei Zhang's comment; missed that in the second patch.
lgtm
Can't apply patch for file AUTHORS. While running patch -p1 --forward --force; patching file AUTHORS Hunk #1 FAILED at 123. 1 out of 1 hunk FAILED -- saving rejects to file AUTHORS.rej
Hmmm, does that mean I'm behind master in my AUTHORS file?
Yes.
Okay, I'll fix that probably tonight; sorry, big meeting :(
On 2011/09/21 21:25:35, chrelad wrote: > Okay, I'll fix that probably tonight; sorry, big meeting :( Thx. BTW, have you signed the CLA? (see the Legal section in http://www.chromium.org/developers/contributing-code)
Yes, I did that first and asked the devs on IRC if that was good enough and they gave the all clear; I think I'm legal now :)
Okay, I merged the trunk version of AUTHORS with my branches; so the merge should work now... Hopefully :)
Change committed as 102248 |
