Added CUPS options to the printer information. Options get uploaded to the cl...

On behalf of gene@chromium.org.  Added CUPS options to the printer information. Options get uploaded to the cloud print server.
Also added some logging for cups proxy.

BUG=none
TEST=Run linux CP proxy and verify that CUPS options gets uploaded to the cloud print server.

[gene, 2010-05-26 00:01:46]

Added CUPS options to the printer information

[sanjeevr, 2010-05-26 03:55:12]

Some nits and one suggestion.

http://codereview.chromium.org/2232002/diff/1/6
File chrome/service/cloud_print/cloud_print_proxy_backend.cc (right):

http://codereview.chromium.org/2232002/diff/1/6#newcode383
chrome/service/cloud_print/cloud_print_proxy_backend.cc:383: std::string
msg(it->first);
Until you add the code to escape can you add a DCHECK and LOG(WARNING) if you
find a name containing an '='.

Also, there is another way you can do this. You can actually name the multi-part
value itself as tag_<name>, for example tag_location. The server can simply
process all names with a tag_ prefix as tags.

http://codereview.chromium.org/2232002/diff/1/5
File chrome/service/cloud_print/printer_info_cups.cc (right):

http://codereview.chromium.org/2232002/diff/1/5#newcode50
chrome/service/cloud_print/printer_info_cups.cc:50: for (int j = 0; j <
destinations[i].num_options; j++) {
Can we use better names than i and j? How about printer_index and option_index?

[gene, 2010-05-26 18:19:25]

On 2010/05/26 03:55:12, sanjeevr wrote:
> Some nits and one suggestion.
> 
> http://codereview.chromium.org/2232002/diff/1/6
> File chrome/service/cloud_print/cloud_print_proxy_backend.cc (right):
> 
> http://codereview.chromium.org/2232002/diff/1/6#newcode383
> chrome/service/cloud_print/cloud_print_proxy_backend.cc:383: std::string
> msg(it->first);
> Until you add the code to escape can you add a DCHECK and LOG(WARNING) if you
> find a name containing an '='.
Done.

> 
> Also, there is another way you can do this. You can actually name the
multi-part
> value itself as tag_<name>, for example tag_location. The server can simply
> process all names with a tag_ prefix as tags.
I'll check with server if this is ok. I am not sure how tag is separated from
the value (separator may contains another special character)

> 
> http://codereview.chromium.org/2232002/diff/1/5
> File chrome/service/cloud_print/printer_info_cups.cc (right):
> 
> http://codereview.chromium.org/2232002/diff/1/5#newcode50
> chrome/service/cloud_print/printer_info_cups.cc:50: for (int j = 0; j <
> destinations[i].num_options; j++) {
> Can we use better names than i and j? How about printer_index and
option_index?
Done.

[sanjeevr, 2010-05-26 18:32:14]

http://codereview.chromium.org/2232002/diff/9001/10005
File chrome/service/cloud_print/cloud_print_proxy_backend.cc (right):

http://codereview.chromium.org/2232002/diff/9001/10005#newcode385
chrome/service/cloud_print/cloud_print_proxy_backend.cc:385: DCHECK(false);
NOTREACHED(); instead of DCHECK(false).

[sanjeevr, 2010-05-26 18:35:42]

On 2010/05/26 18:19:25, gene wrote:
> On 2010/05/26 03:55:12, sanjeevr wrote:
> > Some nits and one suggestion.
> > 
> > http://codereview.chromium.org/2232002/diff/1/6
> > File chrome/service/cloud_print/cloud_print_proxy_backend.cc (right):
> > 
> > http://codereview.chromium.org/2232002/diff/1/6#newcode383
> > chrome/service/cloud_print/cloud_print_proxy_backend.cc:383: std::string
> > msg(it->first);
> > Until you add the code to escape can you add a DCHECK and LOG(WARNING) if
you
> > find a name containing an '='.
> Done.
> 
> > 
> > Also, there is another way you can do this. You can actually name the
> multi-part
> > value itself as tag_<name>, for example tag_location. The server can simply
> > process all names with a tag_ prefix as tags.
> I'll check with server if this is ok. I am not sure how tag is separated from
> the value (separator may contains another special character)

You can see this in AddMultipartValueForUpload, Tag name is part of the
Content-Disposition string and it is separated from the value by \r\n. Yeah let
us make sure the server is OK with this.

> 
> > 
> > http://codereview.chromium.org/2232002/diff/1/5
> > File chrome/service/cloud_print/printer_info_cups.cc (right):
> > 
> > http://codereview.chromium.org/2232002/diff/1/5#newcode50
> > chrome/service/cloud_print/printer_info_cups.cc:50: for (int j = 0; j <
> > destinations[i].num_options; j++) {
> > Can we use better names than i and j? How about printer_index and
> option_index?
> Done.

[gene, 2010-05-26 18:38:26]

Done. Please take a look.

On 2010/05/26 18:32:14, sanjeevr wrote:
> http://codereview.chromium.org/2232002/diff/9001/10005
> File chrome/service/cloud_print/cloud_print_proxy_backend.cc (right):
> 
> http://codereview.chromium.org/2232002/diff/9001/10005#newcode385
> chrome/service/cloud_print/cloud_print_proxy_backend.cc:385: DCHECK(false);
> NOTREACHED(); instead of DCHECK(false).

[gene, 2010-05-26 18:39:06]

On 2010/05/26 18:35:42, sanjeevr wrote:
> On 2010/05/26 18:19:25, gene wrote:
> > On 2010/05/26 03:55:12, sanjeevr wrote:
> > > Some nits and one suggestion.
> > > 
> > > http://codereview.chromium.org/2232002/diff/1/6
> > > File chrome/service/cloud_print/cloud_print_proxy_backend.cc (right):
> > > 
> > > http://codereview.chromium.org/2232002/diff/1/6#newcode383
> > > chrome/service/cloud_print/cloud_print_proxy_backend.cc:383: std::string
> > > msg(it->first);
> > > Until you add the code to escape can you add a DCHECK and LOG(WARNING) if
> you
> > > find a name containing an '='.
> > Done.
> > 
> > > 
> > > Also, there is another way you can do this. You can actually name the
> > multi-part
> > > value itself as tag_<name>, for example tag_location. The server can
simply
> > > process all names with a tag_ prefix as tags.
> > I'll check with server if this is ok. I am not sure how tag is separated
from
> > the value (separator may contains another special character)
> 
> You can see this in AddMultipartValueForUpload, Tag name is part of the
> Content-Disposition string and it is separated from the value by \r\n. Yeah
let
> us make sure the server is OK with this.
Cool. I'll change this if server is OK.

> 
> > 
> > > 
> > > http://codereview.chromium.org/2232002/diff/1/5
> > > File chrome/service/cloud_print/printer_info_cups.cc (right):
> > > 
> > > http://codereview.chromium.org/2232002/diff/1/5#newcode50
> > > chrome/service/cloud_print/printer_info_cups.cc:50: for (int j = 0; j <
> > > destinations[i].num_options; j++) {
> > > Can we use better names than i and j? How about printer_index and
> > option_index?
> > Done.

[sanjeevr, 2010-05-26 20:14:55]

LGTM

[William Hesse, 2010-05-28 08:41:47]

This change causes compilation errors on my Linux system.  /usr/include/cups
only contains driver.h, not cups.h.
If this is an additional dependency upon a new package that has been
introduced, I think we need to add it to the Wiki page
http://code.google.com/p/chromium/wiki/LinuxBuildInstructionsPrerequisites, as a
software requirement, and update the scripts on that page to automatically
install the package.  I just tried installing the package libcupsys2-dev, which
includes cups/cups.h, and rebuilding chrome, and it seems to work.

[William Hesse, 2010-05-28 08:50:54]

OK, I found that somebody else has added the library to the
src/build/install-build-deps.sh.  So rerunning that should fix things.  It is
not listed on the Wiki page, though.  Perhaps the page should be changed to say
that install-build-deps.sh is the most up-to-date list of build requirements,
and the lists on the page are outdated.

On 2010/05/28 08:41:47, William Hesse wrote:
> This change causes compilation errors on my Linux system.  /usr/include/cups
> only contains driver.h, not cups.h.
> If this is an additional dependency upon a new package that has been
> introduced, I think we need to add it to the Wiki page
> http://code.google.com/p/chromium/wiki/LinuxBuildInstructionsPrerequisites, as
a
> software requirement, and update the scripts on that page to automatically
> install the package.  I just tried installing the package libcupsys2-dev,
which
> includes cups/cups.h, and rebuilding chrome, and it seems to work.

[gene1, 2010-05-28 17:32:54]

yes you are correct. I've modified install-build-deps.sh to include cups and
you need to rereun it.
I'll try to update wiki page.

On Fri, May 28, 2010 at 1:50 AM, <whesse@chromium.org> wrote:

> OK, I found that somebody else has added the library to the
> src/build/install-build-deps.sh.  So rerunning that should fix things.  It
> is
> not listed on the Wiki page, though.  Perhaps the page should be changed to
> say
> that install-build-deps.sh is the most up-to-date list of build
> requirements,
> and the lists on the page are outdated.
>
>
> On 2010/05/28 08:41:47, William Hesse wrote:
>
>> This change causes compilation errors on my Linux system.
>>  /usr/include/cups
>> only contains driver.h, not cups.h.
>> If this is an additional dependency upon a new package that has been
>> introduced, I think we need to add it to the Wiki page
>> http://code.google.com/p/chromium/wiki/LinuxBuildInstructionsPrerequisites,
>> as
>>
> a
>
>> software requirement, and update the scripts on that page to automatically
>> install the package.  I just tried installing the package libcupsys2-dev,
>>
> which
>
>> includes cups/cups.h, and rebuilding chrome, and it seems to work.
>>
>
>
>
> http://codereview.chromium.org/2232002/show
>