GeezerVison

[email protected] changed reviewers:
+ [email protected], [email protected]

The last CL to implement printing to CUPS on Chrome OS.

Please review at your earliest convenience.

Thanks!

https://codereview.chromium.org/2117713002/diff/40001/printing/metafile.h
File printing/metafile.h (right):

https://codereview.chromium.org/2117713002/diff/40001/printing/metafile.h#new...
printing/metafile.h:95: virtual bool GetDataAsVector(std::vector<char>* buffer)
const = 0;
Put this above SaveTo() since the impl below is already in that order?

https://codereview.chromium.org/2117713002/diff/40001/printing/printed_docume...
File printing/printed_document.cc (right):

https://codereview.chromium.org/2117713002/diff/40001/printing/printed_docume...
printing/printed_document.cc:267: // This function is not used on aura
linux/chromeos or android.
Comment is out of date.

https://codereview.chromium.org/2117713002/diff/40001/printing/printing_conte...
File printing/printing_context_chromeos.cc (right):

https://codereview.chromium.org/2117713002/diff/40001/printing/printing_conte...
printing/printing_context_chromeos.cc:46: bool use_native_cups_ = false;
g_use_native_cups ... but do you really need this?

https://codereview.chromium.org/2117713002/diff/40001/printing/printing_conte...
printing/printing_context_chromeos.cc:245: // HACK We should get this from the
device driver
What's going on here?

I've responded to the comments and fixed most of them.  I've left a comment
regarding the issue with the DPI.  I'll look into what might be the root cause
since I have a suspicion that dpi should not match printer resolution in some
circumstances.

https://codereview.chromium.org/2117713002/diff/40001/printing/metafile.h
File printing/metafile.h (right):

https://codereview.chromium.org/2117713002/diff/40001/printing/metafile.h#new...
printing/metafile.h:95: virtual bool GetDataAsVector(std::vector<char>* buffer)
const = 0;
On 2016/07/08 01:35:26, Lei Zhang wrote:
> Put this above SaveTo() since the impl below is already in that order?

Done.

https://codereview.chromium.org/2117713002/diff/40001/printing/printed_docume...
File printing/printed_document.cc (right):

https://codereview.chromium.org/2117713002/diff/40001/printing/printed_docume...
printing/printed_document.cc:267: // This function is not used on aura
linux/chromeos or android.
On 2016/07/08 01:35:26, Lei Zhang wrote:
> Comment is out of date.

Are there aura tests to verify that I haven't broken something?

https://codereview.chromium.org/2117713002/diff/40001/printing/printing_conte...
File printing/printing_context_chromeos.cc (right):

https://codereview.chromium.org/2117713002/diff/40001/printing/printing_conte...
printing/printing_context_chromeos.cc:46: bool use_native_cups_ = false;
On 2016/07/08 01:35:26, Lei Zhang wrote:
> g_use_native_cups ... but do you really need this?

Nope. I had it in for debugging.

https://codereview.chromium.org/2117713002/diff/40001/printing/printing_conte...
printing/printing_context_chromeos.cc:245: // HACK We should get this from the
device driver
On 2016/07/08 01:35:26, Lei Zhang wrote:
> What's going on here?

Setting the dpi using printer the reported printer resolution has undesirable
results so I've hardcoded it to 300dpi for the time being.  It seems like the
rendering resolution (which this is used for) should sometimes be different from
the requested printer resolution.

I've updated the comment to reference the bug where I'm working on the quality
issue and added more rationale.

Still got red trybots.

https://codereview.chromium.org/2117713002/diff/40001/printing/printed_docume...
File printing/printed_document.cc (right):

https://codereview.chromium.org/2117713002/diff/40001/printing/printed_docume...
printing/printed_document.cc:267: // This function is not used on aura
linux/chromeos or android.
On 2016/07/09 00:28:33, skau wrote:
> On 2016/07/08 01:35:26, Lei Zhang wrote:
> > Comment is out of date.
> 
> Are there aura tests to verify that I haven't broken something?

If you put a NOTREACHED() in the body, we can see if anyone actually reaches it.

https://codereview.chromium.org/2117713002/diff/80001/printing/printing_conte...
File printing/printing_context_chromeos.cc (right):

https://codereview.chromium.org/2117713002/diff/80001/printing/printing_conte...
printing/printing_context_chromeos.cc:148: return base::WrapUnique(new
PrintingContextChromeos(delegate));
base::MakeUnique

https://codereview.chromium.org/2117713002/diff/80001/printing/printing_conte...
printing/printing_context_chromeos.cc:178: LOG(WARNING) << "No printer
selected";
BTW, consider only logging the serious stuff.
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

https://codereview.chromium.org/2117713002/diff/80001/printing/printing_conte...
printing/printing_context_chromeos.cc:241: // TODO(skau): Convert to DCHECK when
https://crbug.com/613779 is resolved
Should this also mention line 182 above?

https://codereview.chromium.org/2117713002/diff/80001/printing/printing_conte...
printing/printing_context_chromeos.cc:271: if (printer == nullptr) {
More nullptr comparisons

https://codereview.chromium.org/2117713002/diff/80001/printing/printing_conte...
printing/printing_context_chromeos.cc:330: return CANCEL;
Consider putting a new line after, just like line 239 or line 346.

https://codereview.chromium.org/2117713002/diff/80001/printing/printing_conte...
printing/printing_context_chromeos.cc:389: printer_.reset(nullptr);
Just reset()

https://codereview.chromium.org/2117713002/diff/80001/printing/printing_conte...
File printing/printing_context_chromeos.h (right):

https://codereview.chromium.org/2117713002/diff/80001/printing/printing_conte...
printing/printing_context_chromeos.h:8: #include <cups/cups.h>
not needed here?

https://codereview.chromium.org/2117713002/diff/80001/printing/printing_conte...
printing/printing_context_chromeos.h:43: Result StreamData(char* buffer, int
len);
Can this take a vector?

The CQ bit was checked by [email protected] 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 [email protected]

The CQ bit was checked by [email protected] 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 [email protected]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_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 [email protected] 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...

Thanks for the review.  I'm pretty sure I fixed all the trybots, I'll keep an
eye on it.

https://codereview.chromium.org/2117713002/diff/40001/printing/printed_docume...
File printing/printed_document.cc (right):

https://codereview.chromium.org/2117713002/diff/40001/printing/printed_docume...
printing/printed_document.cc:267: // This function is not used on aura
linux/chromeos or android.
On 2016/07/13 01:43:05, Lei Zhang (Slow Thursday) wrote:
> On 2016/07/09 00:28:33, skau wrote:
> > On 2016/07/08 01:35:26, Lei Zhang wrote:
> > > Comment is out of date.
> > 
> > Are there aura tests to verify that I haven't broken something?
> 
> If you put a NOTREACHED() in the body, we can see if anyone actually reaches
it.

Done.

https://codereview.chromium.org/2117713002/diff/80001/printing/printing_conte...
File printing/printing_context_chromeos.cc (right):

https://codereview.chromium.org/2117713002/diff/80001/printing/printing_conte...
printing/printing_context_chromeos.cc:148: return base::WrapUnique(new
PrintingContextChromeos(delegate));
On 2016/07/13 01:43:05, Lei Zhang (Slow Thursday) wrote:
> base::MakeUnique

Done.

https://codereview.chromium.org/2117713002/diff/80001/printing/printing_conte...
printing/printing_context_chromeos.cc:178: LOG(WARNING) << "No printer
selected";
On 2016/07/13 01:43:05, Lei Zhang (Slow Thursday) wrote:
> BTW, consider only logging the serious stuff.
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

I've switched some to debug logging and removed this specific statement.  I left
a few though as they indicate an unexpected condition but don't warrant a crash.

https://codereview.chromium.org/2117713002/diff/80001/printing/printing_conte...
printing/printing_context_chromeos.cc:241: // TODO(skau): Convert to DCHECK when
https://crbug.com/613779 is resolved
On 2016/07/13 01:43:05, Lei Zhang (Slow Thursday) wrote:
> Should this also mention line 182 above?

They're linked now.

https://codereview.chromium.org/2117713002/diff/80001/printing/printing_conte...
printing/printing_context_chromeos.cc:271: if (printer == nullptr) {
On 2016/07/13 01:43:05, Lei Zhang (Slow Thursday) wrote:
> More nullptr comparisons

Done.

https://codereview.chromium.org/2117713002/diff/80001/printing/printing_conte...
printing/printing_context_chromeos.cc:271: if (printer == nullptr) {
On 2016/07/13 01:43:05, Lei Zhang (Slow Thursday) wrote:
> More nullptr comparisons

Done.

https://codereview.chromium.org/2117713002/diff/80001/printing/printing_conte...
printing/printing_context_chromeos.cc:330: return CANCEL;
On 2016/07/13 01:43:05, Lei Zhang (Slow Thursday) wrote:
> Consider putting a new line after, just like line 239 or line 346.

Done.

https://codereview.chromium.org/2117713002/diff/80001/printing/printing_conte...
printing/printing_context_chromeos.cc:389: printer_.reset(nullptr);
On 2016/07/13 01:43:05, Lei Zhang (Slow Thursday) wrote:
> Just reset()

Done.

https://codereview.chromium.org/2117713002/diff/80001/printing/printing_conte...
File printing/printing_context_chromeos.h (right):

https://codereview.chromium.org/2117713002/diff/80001/printing/printing_conte...
printing/printing_context_chromeos.h:8: #include <cups/cups.h>
On 2016/07/13 01:43:05, Lei Zhang (Slow Thursday) wrote:
> not needed here?

Done.

https://codereview.chromium.org/2117713002/diff/80001/printing/printing_conte...
printing/printing_context_chromeos.h:43: Result StreamData(char* buffer, int
len);
On 2016/07/13 01:43:05, Lei Zhang (Slow Thursday) wrote:
> Can this take a vector?

Yes

The CQ bit was unchecked by [email protected]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_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 [email protected] 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 checked by [email protected] 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 [email protected]

Dry run: This issue passed the CQ dry run.

https://codereview.chromium.org/2117713002/diff/200001/printing/metafile.h
File printing/metafile.h (right):

https://codereview.chromium.org/2117713002/diff/200001/printing/metafile.h#ne...
printing/metafile.h:167: bool GetDataAsVector(std::vector<char>* buffer) const;
This feels a bit weird. Why don't we just remove the OS_CHROME ifdefs? I think
that's cleaner, even it it adds an extra method that does not get used on other
platforms.

https://codereview.chromium.org/2117713002/diff/200001/printing/printing.gyp
File printing/printing.gyp (right):

https://codereview.chromium.org/2117713002/diff/200001/printing/printing.gyp#...
printing/printing.gyp:208: 'printed_document_chromeos.cc',
Should the changes here match printing/BUILD.gn? If this CL drags on a bit
longer, all the .gyp files might get deleted. :)

https://codereview.chromium.org/2117713002/diff/200001/printing/printing_cont...
File printing/printing_context_chromeos.cc (right):

https://codereview.chromium.org/2117713002/diff/200001/printing/printing_cont...
printing/printing_context_chromeos.cc:78: char* CopyStringToChar(const
base::StringPiece value, char** dst) {
Do you need |dst| here?

https://codereview.chromium.org/2117713002/diff/200001/printing/printing_cont...
printing/printing_context_chromeos.cc:79: int value_len = value.size() + 1;
Type should be size_t, or just drop this variable altogether and use
value.size() or value.size() + 1 below as needed. Instead of having value_len -
1.

https://codereview.chromium.org/2117713002/diff/200001/printing/printing_cont...
printing/printing_context_chromeos.cc:80: *dst = new char[value_len];
Who deletes these strings?

https://codereview.chromium.org/2117713002/diff/200001/printing/printing_cont...
printing/printing_context_chromeos.cc:81: value.copy(*dst, value_len);
Is this copying 1 extra byte than necessary?

https://codereview.chromium.org/2117713002/diff/200001/printing/printing_cont...
printing/printing_context_chromeos.cc:142: if
(PrintBackend::GetNativeCupsEnabled()) {
No need for braces -> compact down to 3 lines.

https://codereview.chromium.org/2117713002/diff/200001/printing/printing_cont...
File printing/printing_context_chromeos.h (right):

https://codereview.chromium.org/2117713002/diff/200001/printing/printing_cont...
printing/printing_context_chromeos.h:15: #include
"printing/backend/print_backend.h"
is this needed here?

https://codereview.chromium.org/2117713002/diff/200001/printing/printing_cont...
printing/printing_context_chromeos.h:41: Result StreamData(const
std::vector<char>& buffer);
Don't squeeze this into the middle of a list of overrides.

I addressed the comments and unified the constants.  PTAL.

https://codereview.chromium.org/2117713002/diff/200001/printing/metafile.h
File printing/metafile.h (right):

https://codereview.chromium.org/2117713002/diff/200001/printing/metafile.h#ne...
printing/metafile.h:167: bool GetDataAsVector(std::vector<char>* buffer) const;
On 2016/07/20 01:26:56, Lei Zhang wrote:
> This feels a bit weird. Why don't we just remove the OS_CHROME ifdefs? I think
> that's cleaner, even it it adds an extra method that does not get used on
other
> platforms.

It happens that there's a Windows implementation of MetafilePlayer.  But I'll go
implement it there.

https://codereview.chromium.org/2117713002/diff/200001/printing/printing.gyp
File printing/printing.gyp (right):

https://codereview.chromium.org/2117713002/diff/200001/printing/printing.gyp#...
printing/printing.gyp:208: 'printed_document_chromeos.cc',
On 2016/07/20 01:26:56, Lei Zhang wrote:
> Should the changes here match printing/BUILD.gn? If this CL drags on a bit
> longer, all the .gyp files might get deleted. :)

I'm counting the days...

https://codereview.chromium.org/2117713002/diff/200001/printing/printing_cont...
File printing/printing_context_chromeos.cc (right):

https://codereview.chromium.org/2117713002/diff/200001/printing/printing_cont...
printing/printing_context_chromeos.cc:78: char* CopyStringToChar(const
base::StringPiece value, char** dst) {
On 2016/07/20 01:26:56, Lei Zhang wrote:
> Do you need |dst| here?

Not really.  Removed.

https://codereview.chromium.org/2117713002/diff/200001/printing/printing_cont...
printing/printing_context_chromeos.cc:79: int value_len = value.size() + 1;
On 2016/07/20 01:26:56, Lei Zhang wrote:
> Type should be size_t, or just drop this variable altogether and use
> value.size() or value.size() + 1 below as needed. Instead of having value_len
-
> 1.

Done.

https://codereview.chromium.org/2117713002/diff/200001/printing/printing_cont...
printing/printing_context_chromeos.cc:80: *dst = new char[value_len];
On 2016/07/20 01:26:57, Lei Zhang wrote:
> Who deletes these strings?

It is currently leaking memory.  But I'll fix it so cups_option_t objects have
deleters.

https://codereview.chromium.org/2117713002/diff/200001/printing/printing_cont...
printing/printing_context_chromeos.cc:81: value.copy(*dst, value_len);
On 2016/07/20 01:26:56, Lei Zhang wrote:
> Is this copying 1 extra byte than necessary?

It is.  I think strlcpy is cleaner so I'm changing it.  Is there a standard way
to do this?  It seems like there should be...

https://codereview.chromium.org/2117713002/diff/200001/printing/printing_cont...
printing/printing_context_chromeos.cc:142: if
(PrintBackend::GetNativeCupsEnabled()) {
On 2016/07/20 01:26:56, Lei Zhang wrote:
> No need for braces -> compact down to 3 lines.

Done.

https://codereview.chromium.org/2117713002/diff/200001/printing/printing_cont...
File printing/printing_context_chromeos.h (right):

https://codereview.chromium.org/2117713002/diff/200001/printing/printing_cont...
printing/printing_context_chromeos.h:15: #include
"printing/backend/print_backend.h"
On 2016/07/20 01:26:57, Lei Zhang wrote:
> is this needed here?

Done.

https://codereview.chromium.org/2117713002/diff/200001/printing/printing_cont...
printing/printing_context_chromeos.h:41: Result StreamData(const
std::vector<char>& buffer);
On 2016/07/20 01:26:57, Lei Zhang wrote:
> Don't squeeze this into the middle of a list of overrides.

Done.

https://codereview.chromium.org/2117713002/diff/200001/printing/printing_cont...
File printing/printing_context_chromeos.cc (right):

https://codereview.chromium.org/2117713002/diff/200001/printing/printing_cont...
printing/printing_context_chromeos.cc:81: value.copy(*dst, value_len);
On 2016/07/22 23:36:14, skau wrote:
> On 2016/07/20 01:26:56, Lei Zhang wrote:
> > Is this copying 1 extra byte than necessary?
> 
> It is.  I think strlcpy is cleaner so I'm changing it.  Is there a standard
way
> to do this?  It seems like there should be...

How about using base::StringPiece::copy() ?

char* dst = new char[value.size() + 1];
value.copy(dst, value.size());
dst[value.size()] = '\0';
return dst;

https://codereview.chromium.org/2117713002/diff/220001/chrome/browser/printin...
File chrome/browser/printing/pdf_to_emf_converter.cc (right):

https://codereview.chromium.org/2117713002/diff/220001/chrome/browser/printin...
chrome/browser/printing/pdf_to_emf_converter.cc:263: bool
LazyEmf::GetDataAsVector(std::vector<char>* buffer) const {
Thanks for implemeting this... but will this ever get called? i.e. Should this
just be NOTREACHED(); return false;?

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

https://codereview.chromium.org/2117713002/diff/220001/printing/backend/cups_...
printing/backend/cups_ipp_util.h:17: extern const char kIppCollate[];  // RFC
3381
No need for comments here.

https://codereview.chromium.org/2117713002/diff/220001/printing/printed_docum...
File printing/printed_document_chromeos.cc (right):

https://codereview.chromium.org/2117713002/diff/220001/printing/printed_docum...
printing/printed_document_chromeos.cc:13: #endif  // USE_CUPS
So some of the printing code does not set the best example. This should be //
defined(USE_CUPS). i.e. #if $FOO should be mirrored by // $FOO.

Though in this particular case, I'd say don't even bother because:
1) there's no nesting
2) the #if is very close, so it's already obvious what this #endif pairs with.

Line 26 below should have the comment because nesting.
Line 44 below should have the comment because it's a bit far away from the #if.

https://codereview.chromium.org/2117713002/diff/220001/printing/printing_cont...
File printing/printing_context_chromeos.cc (right):

https://codereview.chromium.org/2117713002/diff/220001/printing/printing_cont...
printing/printing_context_chromeos.cc:34: using CupsOption =
std::unique_ptr<cups_option_t, OptionDeleter>;
Can you name this ScopedCupsOption, so it's more obvious that this is a smart
pointer?

https://codereview.chromium.org/2117713002/diff/220001/printing/printing_cont...
printing/printing_context_chromeos.cc:74: // is null terminated and will need to
be deleted.
The more common phrase here would be "the caller owns the returned string."

https://codereview.chromium.org/2117713002/diff/220001/printing/printing_cont...
printing/printing_context_chromeos.cc:75: char* ToCString(const
base::StringPiece value) {
TOCString() -> more like, DuplicateString() ?

https://codereview.chromium.org/2117713002/diff/220001/printing/printing_cont...
printing/printing_context_chromeos.cc:85: option->name = ToCString(name);
It's probably not obvious to readers that CupsOption is going delete the
name/value here. Even reading the cupsFreeOptions() documentation is not
completely obvious and I had to peek at the cupsFreeOptions() source code to see
what it does. So a comment here to say (Scoped)CupsOption will delete the
strings is helpful.

https://codereview.chromium.org/2117713002/diff/220001/printing/printing_cont...
File printing/printing_context_chromeos.h (right):

https://codereview.chromium.org/2117713002/diff/220001/printing/printing_cont...
printing/printing_context_chromeos.h:53: // id for ongoing print job.  -1 if no
job is active.
printing_context_chromeos.cc checks for 0, what is 0?

The CQ bit was checked by [email protected] 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...

Comments addressed.  PTAL.

https://codereview.chromium.org/2117713002/diff/200001/printing/printing_cont...
File printing/printing_context_chromeos.cc (right):

https://codereview.chromium.org/2117713002/diff/200001/printing/printing_cont...
printing/printing_context_chromeos.cc:81: value.copy(*dst, value_len);
On 2016/07/25 21:06:03, Lei Zhang wrote:
> On 2016/07/22 23:36:14, skau wrote:
> > On 2016/07/20 01:26:56, Lei Zhang wrote:
> > > Is this copying 1 extra byte than necessary?
> > 
> > It is.  I think strlcpy is cleaner so I'm changing it.  Is there a standard
> way
> > to do this?  It seems like there should be...
> 
> How about using base::StringPiece::copy() ?
> 
> char* dst = new char[value.size() + 1];
> value.copy(dst, value.size());
> dst[value.size()] = '\0';
> return dst;

Done.

https://codereview.chromium.org/2117713002/diff/220001/chrome/browser/printin...
File chrome/browser/printing/pdf_to_emf_converter.cc (right):

https://codereview.chromium.org/2117713002/diff/220001/chrome/browser/printin...
chrome/browser/printing/pdf_to_emf_converter.cc:263: bool
LazyEmf::GetDataAsVector(std::vector<char>* buffer) const {
On 2016/07/25 21:06:04, Lei Zhang wrote:
> Thanks for implemeting this... but will this ever get called? i.e. Should this
> just be NOTREACHED(); return false;?

It's not currently called.  NOTREACHED() probably works better since it's not
anticipated to be used.

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

https://codereview.chromium.org/2117713002/diff/220001/printing/backend/cups_...
printing/backend/cups_ipp_util.h:17: extern const char kIppCollate[];  // RFC
3381
On 2016/07/25 21:06:04, Lei Zhang wrote:
> No need for comments here.

Done.

https://codereview.chromium.org/2117713002/diff/220001/printing/printed_docum...
File printing/printed_document_chromeos.cc (right):

https://codereview.chromium.org/2117713002/diff/220001/printing/printed_docum...
printing/printed_document_chromeos.cc:13: #endif  // USE_CUPS
On 2016/07/25 21:06:04, Lei Zhang wrote:
> So some of the printing code does not set the best example. This should be //
> defined(USE_CUPS). i.e. #if $FOO should be mirrored by // $FOO.
> 
> Though in this particular case, I'd say don't even bother because:
> 1) there's no nesting
> 2) the #if is very close, so it's already obvious what this #endif pairs with.
> 
> Line 26 below should have the comment because nesting.
> Line 44 below should have the comment because it's a bit far away from the
#if.

Done.

https://codereview.chromium.org/2117713002/diff/220001/printing/printing_cont...
File printing/printing_context_chromeos.cc (right):

https://codereview.chromium.org/2117713002/diff/220001/printing/printing_cont...
printing/printing_context_chromeos.cc:34: using CupsOption =
std::unique_ptr<cups_option_t, OptionDeleter>;
On 2016/07/25 21:06:04, Lei Zhang wrote:
> Can you name this ScopedCupsOption, so it's more obvious that this is a smart
> pointer?

Done.

https://codereview.chromium.org/2117713002/diff/220001/printing/printing_cont...
printing/printing_context_chromeos.cc:74: // is null terminated and will need to
be deleted.
On 2016/07/25 21:06:04, Lei Zhang wrote:
> The more common phrase here would be "the caller owns the returned string."

Done.

https://codereview.chromium.org/2117713002/diff/220001/printing/printing_cont...
printing/printing_context_chromeos.cc:75: char* ToCString(const
base::StringPiece value) {
On 2016/07/25 21:06:04, Lei Zhang wrote:
> TOCString() -> more like, DuplicateString() ?

Done.

https://codereview.chromium.org/2117713002/diff/220001/printing/printing_cont...
printing/printing_context_chromeos.cc:85: option->name = ToCString(name);
On 2016/07/25 21:06:04, Lei Zhang wrote:
> It's probably not obvious to readers that CupsOption is going delete the
> name/value here. Even reading the cupsFreeOptions() documentation is not
> completely obvious and I had to peek at the cupsFreeOptions() source code to
see
> what it does. So a comment here to say (Scoped)CupsOption will delete the
> strings is helpful.

Done.

https://codereview.chromium.org/2117713002/diff/220001/printing/printing_cont...
File printing/printing_context_chromeos.h (right):

https://codereview.chromium.org/2117713002/diff/220001/printing/printing_cont...
printing/printing_context_chromeos.h:53: // id for ongoing print job.  -1 if no
job is active.
On 2016/07/25 21:06:04, Lei Zhang wrote:
> printing_context_chromeos.cc checks for 0, what is 0?

If the createDestJob fails, job_id is set to 0.  I'll just use 0 for everything
since they shouldn't need differentiation and I appear to have forgotten anyway.

The CQ bit was unchecked by [email protected]

Dry run: This issue passed the CQ dry run.

lgtm

The CQ bit was checked by [email protected]

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

Committed patchset #14 (id:260001)

Description was changed from

==========
Print directly to CUPS using the IPP APIs

Implements printing to CUPS with paper size, color, and duplexing
configurable.

Existing bug where the printer does not initialize the first time it is
selected.

BUG=607668

TEST=Enable --enable-native-cups.  Print a webpage to a desired CUPS
printer.
==========

to

==========
Print directly to CUPS using the IPP APIs

Implements printing to CUPS with paper size, color, and duplexing
configurable.

Existing bug where the printer does not initialize the first time it is
selected.

BUG=607668

TEST=Enable --enable-native-cups.  Print a webpage to a desired CUPS
printer.

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

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