Initial support for cloudprint in print preview

Initial support for cloudprint in print preview.

To enable in Windows/Mac/Linux pass "--enable-cloud-print --enable-print-preview" on the command line.

To enable in ChromeOS open about:flags and enable print preview.

This first pass extends the print preview UI to retrieve a printer list from cloud print and to allow printing to a cloud print printer.

Limitations/known issues:
Sign in opens a new tab and requires the print preview page to be refreshed after sign in.
Only pulls the first 10 GCP printers.
Job settings are very limited.  Only sets color and only for a limited set of printers.

BUG=80004
TEST=
When running with default flags the only visible change should be that "Manage Printers" is now "Manage Local Printers"
With flags set as above there should be new entries in the printer selection dropdown allowing the use of cloud printers.  It should be possible to sign in to cloud print, open the management page, and be able to print documents to cloud printers.

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

[Lei Zhang, 2011-05-24 22:33:08]

Hi Albert,

Thanks for working on this. There's a lot of churn in the print preview code for
M13. Can we look at this next week after the M13 branch point?

[Albert Bodenhamer, 2011-05-24 22:40:13]

Sure thing.  I'm working on a second CL based on this one.  I may merge
them.  I'll let you know.

If you could spend 5 minutes just making sure I'm not doing anything
horribly and obviously idiotic I'd really appreciate it.

Thanks.

On Tue, May 24, 2011 at 3:33 PM, <thestig@chromium.org> wrote:

> Hi Albert,
>
> Thanks for working on this. There's a lot of churn in the print preview
> code for
> M13. Can we look at this next week after the M13 branch point?
>
>
> http://codereview.chromium.org/7038028/
>

[Lei Zhang, 2011-05-24 22:46:45]

At a quick glance, nothing fired off an alarm with the exception of the change
to content/renderer/render_thread.cc, mainly because I'm not a security expert.

On 2011/05/24 22:40:13, Albert Bodenhamer wrote:
> Sure thing.  I'm working on a second CL based on this one.  I may merge
> them.  I'll let you know.
> 
> If you could spend 5 minutes just making sure I'm not doing anything
> horribly and obviously idiotic I'd really appreciate it.
> 
> Thanks.
> 
> On Tue, May 24, 2011 at 3:33 PM, <mailto:thestig@chromium.org> wrote:
> 
> > Hi Albert,
> >
> > Thanks for working on this. There's a lot of churn in the print preview
> > code for
> > M13. Can we look at this next week after the M13 branch point?
> >
> >
> > http://codereview.chromium.org/7038028/
> >

[sanjeevr, 2011-05-24 23:21:25]

Looks mostly OK, some minor comments.

http://codereview.chromium.org/7038028/diff/17001/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/7038028/diff/17001/chrome/browser/about_flags....
chrome/browser/about_flags.cc:177: kOsMac | kOsWin | kOsLinux | kOsCrOS,
You can probably use kOsAll :)

http://codereview.chromium.org/7038028/diff/17001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview_cloud.js (right):

http://codereview.chromium.org/7038028/diff/17001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview_cloud.js:64: if (xhr.status == 200) {
Don't we need to update some error status if it fails?

http://codereview.chromium.org/7038028/diff/17001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7038028/diff/17001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:541:
"capabilities=%%7B%%22capabilities%%22%%3A%%5B%%5D%%7D&"
Hmm, might it be more readable if the string was not encoded and then you called
an encode function on it?

http://codereview.chromium.org/7038028/diff/17001/printing/backend/print_back...
File printing/backend/print_backend_chromeos.cc (right):

http://codereview.chromium.org/7038028/diff/17001/printing/backend/print_back...
printing/backend/print_backend_chromeos.cc:11: class PrintBackendChromeOS :
public PrintBackend {
Please add some comments for this class.

[Scott Byer, 2011-05-24 23:40:31]

And I'd add arv@ as a reviewer for the WebUI portions.

http://codereview.chromium.org/7038028/diff/17001/base/json/json_writer.cc
File base/json/json_writer.cc (right):

http://codereview.chromium.org/7038028/diff/17001/base/json/json_writer.cc#ne...
base/json/json_writer.cc:117: case Value::TYPE_BINARY:
If StringValue works with the constructed form string, this shouldn't be needed.

http://codereview.chromium.org/7038028/diff/17001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7038028/diff/17001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:561: memcpy(buffer +
prolog.length(), base64_data.c_str(),
Hmm, what about just adding the base64 encoded stuff to the end of the
SStringPrintf?

http://codereview.chromium.org/7038028/diff/17001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:563: scoped_ptr<BinaryValue>
data_value(BinaryValue::Create(buffer, buffer_size));
Shouldn't need to be a BinaryValue anymore with the base64 encoding -
StringValue should suffice.

http://codereview.chromium.org/7038028/diff/17001/content/renderer/render_thr...
File content/renderer/render_thread.cc (right):

http://codereview.chromium.org/7038028/diff/17001/content/renderer/render_thr...
content/renderer/render_thread.cc:442:
WebKit::WebSecurityPolicy::addOriginAccessWhitelistEntry(
This is going to need to be fixed up (sorry, it was on my to-do list). We
shouldn't call it until we need it - so adding some mechanism or a field to
params to have it only added when print preview is first invoked. It also will
need to be removed when the print preview tab goes away. And after that, we'll
want someone with more security knowledge to review it.

http://codereview.chromium.org/7038028/diff/17001/printing/backend/print_back...
File printing/backend/print_backend_chromeos.cc (right):

http://codereview.chromium.org/7038028/diff/17001/printing/backend/print_back...
printing/backend/print_backend_chromeos.cc:11: class PrintBackendChromeOS :
public PrintBackend {
We will want a comment here explaining that it's supposed to be a dummy, I
think.

[Albert Bodenhamer, 2011-06-09 17:33:11]

I've responded to the original comments and added arv as a reviewer.  I've also
added simple handling for capabilities and print ticket generation.

Please take another look.

http://codereview.chromium.org/7038028/diff/17001/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/7038028/diff/17001/chrome/browser/about_flags....
chrome/browser/about_flags.cc:177: kOsMac | kOsWin | kOsLinux | kOsCrOS,
On 2011/05/24 23:21:25, sanjeevr wrote:
> You can probably use kOsAll :)

Done.

http://codereview.chromium.org/7038028/diff/17001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview_cloud.js (right):

http://codereview.chromium.org/7038028/diff/17001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview_cloud.js:64: if (xhr.status == 200) {
On 2011/05/24 23:21:25, sanjeevr wrote:
> Don't we need to update some error status if it fails?

Some sort of feedback would definitely be good.  I don't want to gate this on
proper handling of that though.  For now I'm just adding a TODO.

http://codereview.chromium.org/7038028/diff/17001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7038028/diff/17001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:541:
"capabilities=%%7B%%22capabilities%%22%%3A%%5B%%5D%%7D&"
On 2011/05/24 23:21:25, sanjeevr wrote:
> Hmm, might it be more readable if the string was not encoded and then you
called
> an encode function on it?

This is just copied/pasted from a successful print job.  Handling caps/print
ticket properly is my next task.  I'll take care of this at the same time. 
Until then, I'm added a TODO.

http://codereview.chromium.org/7038028/diff/17001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:561: memcpy(buffer +
prolog.length(), base64_data.c_str(),
On 2011/05/24 23:40:32, Scott Byer wrote:
> Hmm, what about just adding the base64 encoded stuff to the end of the
> SStringPrintf?

Good call.  That makes the code much cleaner.  Done.

http://codereview.chromium.org/7038028/diff/17001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:563: scoped_ptr<BinaryValue>
data_value(BinaryValue::Create(buffer, buffer_size));
On 2011/05/24 23:40:32, Scott Byer wrote:
> Shouldn't need to be a BinaryValue anymore with the base64 encoding -
> StringValue should suffice.

Done.

[Lei Zhang, 2011-06-09 21:23:30]

FYI, I think you'll need to rebase again. The print preview code is still a
moving target, but we should be settling down soon.

http://codereview.chromium.org/7038028/diff/26005/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chrome_web_ui_factory.cc (right):

http://codereview.chromium.org/7038028/diff/26005/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chrome_web_ui_factory.cc:205: if (url.host() ==
chrome::kChromeUISettingsHost)
isn't this already on line 172?

http://codereview.chromium.org/7038028/diff/26005/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7038028/diff/26005/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:10: #include
"base/command_line.h"
move to inside a #if !defined(OS_CHROMEOS) block. Ditto for chrome_switches.h
below.

http://codereview.chromium.org/7038028/diff/26005/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:42: #include
"webkit/glue/webpreferences.h"
not used, ditto for escape.h above.

http://codereview.chromium.org/7038028/diff/26005/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:501: rvh->Send(new
PrintMsg_PrintForPrintPreview(rvh->routing_id(),
nit: this is an unnecessary change.

http://codereview.chromium.org/7038028/diff/26005/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.h (right):

http://codereview.chromium.org/7038028/diff/26005/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.h:69: // Bring up a web page to
allow the user to configure cloud print.
nit: indentation, describe what |args| does.

http://codereview.chromium.org/7038028/diff/26005/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7038028/diff/26005/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:195:
WebKit::WebSecurityPolicy::addOriginAccessWhitelistEntry(
I'm not a web security expert. Is this ok to do?

http://codereview.chromium.org/7038028/diff/26005/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:196: GURL("chrome://print"),
chrome://print -> chrome::kChromeUIPrintHost

http://codereview.chromium.org/7038028/diff/26005/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:197:
WebKit::WebString::fromUTF8("HTTPS"),
HTTPS -> chrome::kHttpsScheme

http://codereview.chromium.org/7038028/diff/26005/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7038028/diff/26005/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:104: // Setup or teardown a whitelist
allowing the
nit: indentation

http://codereview.chromium.org/7038028/diff/26005/printing/print_job_constants.h
File printing/print_job_constants.h (right):

http://codereview.chromium.org/7038028/diff/26005/printing/print_job_constant...
printing/print_job_constants.h:21: extern const char kSettingCloudPrintId[];
nit: alphabetical order.

[Albert Bodenhamer, 2011-06-10 01:29:36]

Thanks for the feedback.  I've been doing a pull/merge about once a day so I
don't get too far out of sync.

http://codereview.chromium.org/7038028/diff/26005/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chrome_web_ui_factory.cc (right):

http://codereview.chromium.org/7038028/diff/26005/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chrome_web_ui_factory.cc:205: if (url.host() ==
chrome::kChromeUISettingsHost)
On 2011/06/09 21:23:30, Lei Zhang wrote:
> isn't this already on line 172?

Good catch.  I think something went wrong with a merge and I missed it.

http://codereview.chromium.org/7038028/diff/26005/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7038028/diff/26005/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:10: #include
"base/command_line.h"
On 2011/06/09 21:23:30, Lei Zhang wrote:
> move to inside a #if !defined(OS_CHROMEOS) block. Ditto for chrome_switches.h
> below.

Done.

http://codereview.chromium.org/7038028/diff/26005/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:42: #include
"webkit/glue/webpreferences.h"
On 2011/06/09 21:23:30, Lei Zhang wrote:
> not used, ditto for escape.h above.

Done.

http://codereview.chromium.org/7038028/diff/26005/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:501: rvh->Send(new
PrintMsg_PrintForPrintPreview(rvh->routing_id(),
On 2011/06/09 21:23:30, Lei Zhang wrote:
> nit: this is an unnecessary change.

Done.

http://codereview.chromium.org/7038028/diff/26005/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.h (right):

http://codereview.chromium.org/7038028/diff/26005/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.h:69: // Bring up a web page to
allow the user to configure cloud print.
On 2011/06/09 21:23:30, Lei Zhang wrote:
> nit: indentation, describe what |args| does.

Done.

http://codereview.chromium.org/7038028/diff/26005/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7038028/diff/26005/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:195:
WebKit::WebSecurityPolicy::addOriginAccessWhitelistEntry(
On 2011/06/09 21:23:30, Lei Zhang wrote:
> I'm not a web security expert. Is this ok to do?

I'm still trying to figure out the best person to ask.  I'll be sure to get an
expert opinion before submit.

Extensions use the same construct much more aggressively.  I expect this will be
fine.

http://codereview.chromium.org/7038028/diff/26005/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7038028/diff/26005/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:104: // Setup or teardown a whitelist
allowing the
On 2011/06/09 21:23:30, Lei Zhang wrote:
> nit: indentation

Done.

http://codereview.chromium.org/7038028/diff/26005/printing/print_job_constants.h
File printing/print_job_constants.h (right):

http://codereview.chromium.org/7038028/diff/26005/printing/print_job_constant...
printing/print_job_constants.h:21: extern const char kSettingCloudPrintId[];
On 2011/06/09 21:23:30, Lei Zhang wrote:
> nit: alphabetical order.

Done.

[Scott Byer, 2011-06-10 17:40:04]

LGTM

On 2011/06/10 01:29:36, Albert Bodenhamer wrote:
> Thanks for the feedback.  I've been doing a pull/merge about once a day so I
> don't get too far out of sync.
> 
>

[sanjeevr, 2011-06-10 18:13:48]

http://codereview.chromium.org/7038028/diff/27005/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview.js (right):

http://codereview.chromium.org/7038028/diff/27005/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:171: return;
Nit: Is there a way to restructure this so we don't have returns inside the if
blocks? Makes for more readable code, methinks.

http://codereview.chromium.org/7038028/diff/27005/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:435: [getSettingsJSON(),
cloudprint.getPrintTicketJSON(printer)]); }, 1000);
You probably need to check whether this is a cloud printer before calling
getPrintTicketJSON?

http://codereview.chromium.org/7038028/diff/27005/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview_cloud.js (right):

http://codereview.chromium.org/7038028/diff/27005/chrome/browser/resources/pr...
chrome/browser/resources/print_preview_cloud.js:142: {'X-CloudPrint-Proxy':
'ChromePrintPreview'},
You should probably define a constant for header values like
'ChromePrintPreview'.

[dpapad, 2011-06-10 20:29:06]

http://codereview.chromium.org/7038028/diff/27005/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview.js (right):

http://codereview.chromium.org/7038028/diff/27005/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:245:
fadeInElement($('landscape-option'));
nit: Could you use the more compressed form?

disableLandscapeOption ? fadeOutElement($('landscape-option')) :
    fadeInElement($('landscape-option'));

http://codereview.chromium.org/7038028/diff/27005/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:287: function
setControlsDisabled(disabled) {
This function was removed in earlier revisions, do you need it, or should you
rebase? I do not see it being called and we no longer disable any of the
controls, we just remove event listeners when needed.

http://codereview.chromium.org/7038028/diff/27005/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:525: * @param {boolean} isDisabled is
true if the option needs to be disabled.
Update comments since it is now returning a value.

http://codereview.chromium.org/7038028/diff/27005/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:556: i == 0,
I believe this should be 
printers[i]['name'] == defaultOrLastUsedPrinterName
otherwise the default or last used printer will not be pre-selected.

[Albert Bodenhamer, 2011-06-10 23:13:45]

I've responded to comments, but I'm going to need to address the whitelisting
issues with a server side change before I submit this.

http://codereview.chromium.org/7038028/diff/27005/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview.js (right):

http://codereview.chromium.org/7038028/diff/27005/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:171: return;
On 2011/06/10 18:13:48, sanjeevr wrote:
> Nit: Is there a way to restructure this so we don't have returns inside the if
> blocks? Makes for more readable code, methinks.

Done.

http://codereview.chromium.org/7038028/diff/27005/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:245:
fadeInElement($('landscape-option'));
I have a strong dislike of the ternary operator for that sort of thing.  It
makes the code much less readable.

On 2011/06/10 20:29:07, dpapad wrote:
> nit: Could you use the more compressed form?
> 
> disableLandscapeOption ? fadeOutElement($('landscape-option')) :
>     fadeInElement($('landscape-option'));

http://codereview.chromium.org/7038028/diff/27005/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:287: function
setControlsDisabled(disabled) {
On 2011/06/10 20:29:07, dpapad wrote:
> This function was removed in earlier revisions, do you need it, or should you
> rebase? I do not see it being called and we no longer disable any of the
> controls, we just remove event listeners when needed.

Done.

http://codereview.chromium.org/7038028/diff/27005/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:435: [getSettingsJSON(),
cloudprint.getPrintTicketJSON(printer)]); }, 1000);
getPrintTicketJSON now returns null for non-cloud printers.

On 2011/06/10 18:13:48, sanjeevr wrote:
> You probably need to check whether this is a cloud printer before calling
> getPrintTicketJSON?

http://codereview.chromium.org/7038028/diff/27005/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:525: * @param {boolean} isDisabled is
true if the option needs to be disabled.
On 2011/06/10 20:29:07, dpapad wrote:
> Update comments since it is now returning a value.

Done.

http://codereview.chromium.org/7038028/diff/27005/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:556: i == 0,
On 2011/06/10 20:29:07, dpapad wrote:
> I believe this should be 
> printers[i]['name'] == defaultOrLastUsedPrinterName
> otherwise the default or last used printer will not be pre-selected.

Done.

http://codereview.chromium.org/7038028/diff/27005/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview_cloud.js (right):

http://codereview.chromium.org/7038028/diff/27005/chrome/browser/resources/pr...
chrome/browser/resources/print_preview_cloud.js:142: {'X-CloudPrint-Proxy':
'ChromePrintPreview'},
On 2011/06/10 18:13:48, sanjeevr wrote:
> You should probably define a constant for header values like
> 'ChromePrintPreview'.

Done.

[Albert Bodenhamer, 2011-06-14 00:52:44]

I've removed the whitelisting code for allowing XHRs to cloudprint servers and
done another merge.  I think I've hit everyone's comments at this point.

Please take another look.

Thanks

[Lei Zhang, 2011-06-14 01:08:08]

LGTM with nits below.

http://codereview.chromium.org/7038028/diff/32001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7038028/diff/32001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:567: void
PrintPreviewHandler::HandleManageCloudPrint(const ListValue* args) {
nit: leave off args since it's unused.

http://codereview.chromium.org/7038028/diff/32001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7038028/diff/32001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:30: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebSecurityPolicy.h"
no longer needed for this CL

http://codereview.chromium.org/7038028/diff/32001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7038028/diff/32001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:104: // Setup or teardown a whitelist
allowing the
Remove?

[dpapad, 2011-06-14 01:14:46]

There has been a refactoring of print_preview.js and it is in the commit queue
http://codereview.chromium.org/7003153/. Please rebase after it lands.

http://codereview.chromium.org/7038028/diff/32001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview.js (right):

http://codereview.chromium.org/7038028/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:10: var maxCloudPrinters = 10;
Document these variables.

http://codereview.chromium.org/7038028/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:308: * @param {enable} true if cloud
print should be used.
@param {boolean} enable True if could print should be used.

http://codereview.chromium.org/7038028/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:318: * @param {pdfData} data to send
as the print job.
Here and elsewhere
@param {string} pdfData ....

http://codereview.chromium.org/7038028/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:499: var printerList =
$('printer-list')
Add ; at the end.

http://codereview.chromium.org/7038028/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:592: * returns the created option.
For consistency could you use the following format? 
@return {variableType} comments
See other functions for examples.

http://codereview.chromium.org/7038028/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:616: var l = printers.length;
Nit: This makes it shorter to write but harder to read. Maybe rename to
displayedPrinterCount or something that is more relevant?

[Albert Bodenhamer, 2011-06-14 19:43:54]

I've updated based on review feedback.

http://codereview.chromium.org/7038028/diff/32001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview.js (right):

http://codereview.chromium.org/7038028/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:10: var maxCloudPrinters = 10;
On 2011/06/14 01:14:46, dpapad wrote:
> Document these variables.

Done.

http://codereview.chromium.org/7038028/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:499: var printerList =
$('printer-list')
On 2011/06/14 01:14:46, dpapad wrote:
> Add ; at the end.

Done.

http://codereview.chromium.org/7038028/diff/32001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:592: * returns the created option.
On 2011/06/14 01:14:46, dpapad wrote:
> For consistency could you use the following format? 
> @return {variableType} comments
> See other functions for examples.

Done.

http://codereview.chromium.org/7038028/diff/32001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7038028/diff/32001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:567: void
PrintPreviewHandler::HandleManageCloudPrint(const ListValue* args) {
On 2011/06/14 01:08:08, Lei Zhang wrote:
> nit: leave off args since it's unused.

Done.

http://codereview.chromium.org/7038028/diff/32001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7038028/diff/32001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:30: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebSecurityPolicy.h"
On 2011/06/14 01:08:08, Lei Zhang wrote:
> no longer needed for this CL

Done.

http://codereview.chromium.org/7038028/diff/32001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7038028/diff/32001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:104: // Setup or teardown a whitelist
allowing the
On 2011/06/14 01:08:08, Lei Zhang wrote:
> Remove?

Done.

[dpapad, 2011-06-14 20:36:52]

LGTM for the js part.

[sanjeevr, 2011-06-15 20:24:36]

LGTM

http://codereview.chromium.org/7038028/diff/24007/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview.js (right):

http://codereview.chromium.org/7038028/diff/24007/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:222: return;
You forgot setting skip_refresh to true here instead of the return.

[Albert Bodenhamer, 2011-07-11 23:39:27]

Server changes are now live.  I've also added support for a cloud printer to be
"sticky" in the same way as a regular printer.

Can folks take another quick look at my changes?  If all looks OK, I'll submit
tomorrow.

Thanks.

[Scott Byer, 2011-07-12 00:04:34]

LGTM

On 2011/07/11 23:39:27, Albert Bodenhamer wrote:
> Server changes are now live.  I've also added support for a cloud printer to
be
> "sticky" in the same way as a regular printer.
> 
> Can folks take another quick look at my changes?  If all looks OK, I'll submit
> tomorrow.
> 
> Thanks.

[Albert Bodenhamer, 2011-07-12 17:06:50]

If there are no objections I'll submit after lunch

[commit-bot: I haz the power, 2011-07-12 20:05:35]

Can't apply patch for file chrome/browser/ui/webui/print_preview_handler.h.
While running patch -p1 --forward --force;
patching file chrome/browser/ui/webui/print_preview_handler.h
Hunk #2 succeeded at 80 with fuzz 2 (offset 3 lines).
Hunk #3 succeeded at 94 with fuzz 2 (offset 3 lines).
Hunk #4 FAILED at 108.
Hunk #5 succeeded at 141 (offset 3 lines).
1 out of 5 hunks FAILED -- saving rejects to file
chrome/browser/ui/webui/print_preview_handler.h.rej

[commit-bot: I haz the power, 2011-07-13 00:01:30]

Change committed as 92275