Print Preview: Hook up the print button to initiate printing without displaying a print dialog.

(1) Hook up the print button to send the pages to the default printer for printing without displaying a native dialog. 
(2) Made code changes to accept a print page range from the user and to print only those specified pages.

BUG=none
TEST=Enable print preview on mac, provide a valid page range and  make sure print button in print preview works.

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

[kmadhusu, 2011-02-16 22:37:35]

thestig, jam: Please review printing code.
jhawkins: Please review preview dom ui code.

Thanks.

[James Hawkins, 2011-02-16 22:58:21]

http://codereview.chromium.org/6533006/diff/47/chrome/browser/dom_ui/print_pr...
File chrome/browser/dom_ui/print_preview_handler.h (right):

http://codereview.chromium.org/6533006/diff/47/chrome/browser/dom_ui/print_pr...
chrome/browser/dom_ui/print_preview_handler.h:30: void HandlePrint(const
ListValue* args);
Document this method.

http://codereview.chromium.org/6533006/diff/47/chrome/browser/printing/print_...
File chrome/browser/printing/print_job_worker.h (right):

http://codereview.chromium.org/6533006/diff/47/chrome/browser/printing/print_...
chrome/browser/printing/print_job_worker.h:98: // Called on a UI thread to
update the print settings.
s/a/the/

http://codereview.chromium.org/6533006/diff/47/chrome/browser/resources/print...
File chrome/browser/resources/print_preview.html (right):

http://codereview.chromium.org/6533006/diff/47/chrome/browser/resources/print...
chrome/browser/resources/print_preview.html:69: <option value="1"
i18n-content="optionColor"></option>
Why is the order reversed for the values?

http://codereview.chromium.org/6533006/diff/47/chrome/browser/resources/print...
File chrome/browser/resources/print_preview.js (right):

http://codereview.chromium.org/6533006/diff/47/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:34: var temp_range =
pageRangeList[i].split("-");
What if the page range text is malformed?

[arv (Not doing code reviews), 2011-02-16 23:54:41]

FYI

http://codereview.chromium.org/6533006/diff/47/chrome/browser/resources/print...
File chrome/browser/resources/print_preview.html (right):

http://codereview.chromium.org/6533006/diff/47/chrome/browser/resources/print...
chrome/browser/resources/print_preview.html:83: console.log('in bottom script');
remove this

http://codereview.chromium.org/6533006/diff/47/chrome/browser/resources/print...
File chrome/browser/resources/print_preview.js (right):

http://codereview.chromium.org/6533006/diff/47/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:32: var pageRangeList =
pageRangeText.split(",");
Use single quotes

http://codereview.chromium.org/6533006/diff/47/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:34: var temp_range =
pageRangeList[i].split("-");
no underscores in js

http://codereview.chromium.org/6533006/diff/47/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:36: pageRangeParams =
JSON.stringify({"from" : parseInt(temp_range[0]),
parseInt(n, 10) please

http://codereview.chromium.org/6533006/diff/47/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:39: pageRangeParams =
JSON.stringify({"from" : parseInt(temp_range[0]),
It seems like you can do the from to assignment outside the if to reduce code
duplication.

http://codereview.chromium.org/6533006/diff/47/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:42:
pageRangesInfo.push(pageRangeParams);
Why are we using strings in here?

http://codereview.chromium.org/6533006/diff/47/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:58: var job_settings =
JSON.stringify({"printerName" : printer_name,
no ws before :

http://codereview.chromium.org/6533006/diff/47/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:59: "pageRange" : page_range,
Won't this lead to double json encoding?

[kmadhusu, 2011-02-17 19:20:31]

Thanks for your comments. Made code changes accordingly. Please review the new
patch.

http://codereview.chromium.org/6533006/diff/47/chrome/browser/dom_ui/print_pr...
File chrome/browser/dom_ui/print_preview_handler.h (right):

http://codereview.chromium.org/6533006/diff/47/chrome/browser/dom_ui/print_pr...
chrome/browser/dom_ui/print_preview_handler.h:30: void HandlePrint(const
ListValue* args);
On 2011/02/16 22:58:21, James Hawkins wrote:
> Document this method.

Done.

http://codereview.chromium.org/6533006/diff/47/chrome/browser/printing/print_...
File chrome/browser/printing/print_job_worker.h (right):

http://codereview.chromium.org/6533006/diff/47/chrome/browser/printing/print_...
chrome/browser/printing/print_job_worker.h:98: // Called on a UI thread to
update the print settings.
On 2011/02/16 22:58:21, James Hawkins wrote:
> s/a/the/

Done.

http://codereview.chromium.org/6533006/diff/47/chrome/browser/resources/print...
File chrome/browser/resources/print_preview.html (right):

http://codereview.chromium.org/6533006/diff/47/chrome/browser/resources/print...
chrome/browser/resources/print_preview.html:69: <option value="1"
i18n-content="optionColor"></option>
On 2011/02/16 22:58:21, James Hawkins wrote:
> Why is the order reversed for the values?

Fixed.

http://codereview.chromium.org/6533006/diff/47/chrome/browser/resources/print...
chrome/browser/resources/print_preview.html:83: console.log('in bottom script');
On 2011/02/16 23:54:41, arv wrote:
> remove this

Done.

http://codereview.chromium.org/6533006/diff/47/chrome/browser/resources/print...
File chrome/browser/resources/print_preview.js (right):

http://codereview.chromium.org/6533006/diff/47/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:32: var pageRangeList =
pageRangeText.split(",");
On 2011/02/16 23:54:41, arv wrote:
> Use single quotes

Done.

http://codereview.chromium.org/6533006/diff/47/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:34: var temp_range =
pageRangeList[i].split("-");
On 2011/02/16 23:54:41, arv wrote:
> no underscores in js

Done.

http://codereview.chromium.org/6533006/diff/47/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:34: var temp_range =
pageRangeList[i].split("-");
On 2011/02/16 22:58:21, James Hawkins wrote:
> What if the page range text is malformed?

Added isValidPageRange() function to validate the page range information.

http://codereview.chromium.org/6533006/diff/47/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:36: pageRangeParams =
JSON.stringify({"from" : parseInt(temp_range[0]),
On 2011/02/16 23:54:41, arv wrote:
> parseInt(n, 10) please

Done.

http://codereview.chromium.org/6533006/diff/47/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:39: pageRangeParams =
JSON.stringify({"from" : parseInt(temp_range[0]),
On 2011/02/16 23:54:41, arv wrote:
> It seems like you can do the from to assignment outside the if to reduce code
> duplication.

Done.

http://codereview.chromium.org/6533006/diff/47/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:42:
pageRangesInfo.push(pageRangeParams);
On 2011/02/16 23:54:41, arv wrote:
> Why are we using strings in here?

Rather than creating json strings, I modified the code to create an array of
Objects.

http://codereview.chromium.org/6533006/diff/47/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:58: var job_settings =
JSON.stringify({"printerName" : printer_name,
On 2011/02/16 23:54:41, arv wrote:
> no ws before :

Done.

http://codereview.chromium.org/6533006/diff/47/chrome/browser/resources/print...
chrome/browser/resources/print_preview.js:59: "pageRange" : page_range,
On 2011/02/16 23:54:41, arv wrote:
> Won't this lead to double json encoding?

Fixed. |pageRange| is now an array of objects. JSON encoding is done only once.

[Lei Zhang, 2011-02-17 19:32:15]

You should have ask me to commit http://codereview.chromium.org/6479004/, rather
than combine that CL with this one. Thus the same code doesn't get reviewed
again, and it's easier to review your changes.

[kmadhusu, 2011-02-17 19:51:24]

On 2011/02/17 19:32:15, Lei Zhang wrote:
> You should have ask me to commit http://codereview.chromium.org/6479004/,
rather
> than combine that CL with this one. Thus the same code doesn't get reviewed
> again, and it's easier to review your changes.

Lei,

I am sorry about that. I had to modify most of that CL code. That is the reason
I didn't ask you to commit that first. I thought it will be better for the
reviewer to understand the sequence with all the changes. In the description of
this CL, I have stated that this CL includes the patch from 6479004. Please
commit your patch and let me know. I will update the CL. 

Thanks.

[kmadhusu, 2011-02-17 23:27:04]

I have updated my CL. Please review my changes.

Thanks.

[jam, 2011-02-18 01:55:06]

http://codereview.chromium.org/6533006/diff/6031/chrome/browser/printing/prin...
File chrome/browser/printing/print_job_worker.cc (right):

http://codereview.chromium.org/6533006/diff/6031/chrome/browser/printing/prin...
chrome/browser/printing/print_job_worker.cc:105:
MessageLoop::current()->SetNestableTasksAllowed(false);
why are you're doing this?

http://codereview.chromium.org/6533006/diff/6031/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_message_filter.cc (right):

http://codereview.chromium.org/6533006/diff/6031/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_message_filter.cc:465:
IPC_MESSAGE_HANDLER_DELAY_REPLY(ViewHostMsg_GetCurrentPrintSettings,
i think it's about time that the print related messages get moved into their own
BrowserMessageFilter, given that you're adding a few more now

http://codereview.chromium.org/6533006/diff/6031/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_message_filter.cc:1182: Send(reply_msg);
why are you using DELAY_REPLY?  see this message
http://groups.google.com/a/chromium.org/group/chromium-dev/msg/b55e8878f6d22197
for more information.

http://codereview.chromium.org/6533006/diff/6031/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_message_filter.cc:1219:
ViewHostMsg_UpdatePrintSettings::WriteReplyParams(reply_msg, params);
ditto

[Lei Zhang, 2011-02-18 05:21:42]

http://codereview.chromium.org/6533006/diff/6031/chrome/browser/dom_ui/print_...
File chrome/browser/dom_ui/print_preview_handler.h (right):

http://codereview.chromium.org/6533006/diff/6031/chrome/browser/dom_ui/print_...
chrome/browser/dom_ui/print_preview_handler.h:31: // Get the job settings from
Web UI and initiate printing.
Document what |args| is.

http://codereview.chromium.org/6533006/diff/6031/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview.js (right):

http://codereview.chromium.org/6533006/diff/6031/chrome/browser/resources/pri...
chrome/browser/resources/print_preview.js:83: "pageRange": pageRanges,
nit: indentation.

http://codereview.chromium.org/6533006/diff/6031/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/6533006/diff/6031/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:163: PrintNode(&element, false, false,
true);
Why change PrintNode()? Why not just call PrintForPrintPreview() directly?

http://codereview.chromium.org/6533006/diff/6031/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/6533006/diff/6031/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:124: DEFAULT,
Do you intend to have more types in this enum? If not, why not just use a bool?

http://codereview.chromium.org/6533006/diff/6031/printing/printing_context.h
File printing/printing_context.h (right):

http://codereview.chromium.org/6533006/diff/6031/printing/printing_context.h#...
printing/printing_context.h:56: virtual Result UpdatePrintSettings(const
DictionaryValue* job_settings,
Why do you have |job_settings| that's unused in all the PrintingContext
implementations?

[kmadhusu, 2011-02-21 01:30:26]

Thanks for your comments. Made code changes accordingly. Please review the new
patch.

jam: I am creating a new BrowserMessageFilter to handle printing related
messages. I will upload those changes asap.

http://codereview.chromium.org/6533006/diff/6031/chrome/browser/dom_ui/print_...
File chrome/browser/dom_ui/print_preview_handler.h (right):

http://codereview.chromium.org/6533006/diff/6031/chrome/browser/dom_ui/print_...
chrome/browser/dom_ui/print_preview_handler.h:31: // Get the job settings from
Web UI and initiate printing.
On 2011/02/18 05:21:42, Lei Zhang wrote:
> Document what |args| is.

Done.

http://codereview.chromium.org/6533006/diff/6031/chrome/browser/printing/prin...
File chrome/browser/printing/print_job_worker.cc (right):

http://codereview.chromium.org/6533006/diff/6031/chrome/browser/printing/prin...
chrome/browser/printing/print_job_worker.cc:105:
MessageLoop::current()->SetNestableTasksAllowed(false);
On 2011/02/18 01:55:07, John Abd-El-Malek wrote:
> why are you're doing this?

Based on our discussion, I removed this statement since it is not required for
this code.

http://codereview.chromium.org/6533006/diff/6031/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_message_filter.cc (right):

http://codereview.chromium.org/6533006/diff/6031/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_message_filter.cc:465:
IPC_MESSAGE_HANDLER_DELAY_REPLY(ViewHostMsg_GetCurrentPrintSettings,
On 2011/02/18 01:55:07, John Abd-El-Malek wrote:
> i think it's about time that the print related messages get moved into their
own
> BrowserMessageFilter, given that you're adding a few more now

Creating a new printing_message_filter to handle print related messages. I will
upload these changes asap.

http://codereview.chromium.org/6533006/diff/6031/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_message_filter.cc:1182: Send(reply_msg);
On 2011/02/18 01:55:07, John Abd-El-Malek wrote:
> why are you using DELAY_REPLY?  see this message
>
http://groups.google.com/a/chromium.org/group/chromium-dev/msg/b55e8878f6d22197
> for more information.

Fixed. Thanks for the message link.

http://codereview.chromium.org/6533006/diff/6031/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_message_filter.cc:1219:
ViewHostMsg_UpdatePrintSettings::WriteReplyParams(reply_msg, params);
On 2011/02/18 01:55:07, John Abd-El-Malek wrote:
> ditto

I need to use DELAY_REPLY for "ViewHostMsg_UpdatePrintSettings" because the
handler doesn't have the output parameter ready for response.

http://codereview.chromium.org/6533006/diff/6031/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview.js (right):

http://codereview.chromium.org/6533006/diff/6031/chrome/browser/resources/pri...
chrome/browser/resources/print_preview.js:83: "pageRange": pageRanges,
On 2011/02/18 05:21:42, Lei Zhang wrote:
> nit: indentation.

Done.

http://codereview.chromium.org/6533006/diff/6031/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/6533006/diff/6031/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:163: PrintNode(&element, false, false,
true);
On 2011/02/18 05:21:42, Lei Zhang wrote:
> Why change PrintNode()? Why not just call PrintForPrintPreview() directly?

Fixed.

http://codereview.chromium.org/6533006/diff/6031/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/6533006/diff/6031/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:124: DEFAULT,
On 2011/02/18 05:21:42, Lei Zhang wrote:
> Do you intend to have more types in this enum? If not, why not just use a
bool?

Just for readability and better understanding of the code, I went for enum. I am
not planning to add more types to this enum.

http://codereview.chromium.org/6533006/diff/6031/printing/printing_context.h
File printing/printing_context.h (right):

http://codereview.chromium.org/6533006/diff/6031/printing/printing_context.h#...
printing/printing_context.h:56: virtual Result UpdatePrintSettings(const
DictionaryValue* job_settings,
On 2011/02/18 05:21:42, Lei Zhang wrote:
> Why do you have |job_settings| that's unused in all the PrintingContext
> implementations?

Removed that argument.

[arv (Not doing code reviews), 2011-02-21 02:00:05]

http://codereview.chromium.org/6533006/diff/17001/chrome/browser/dom_ui/print...
File chrome/browser/dom_ui/print_preview_handler.cc (right):

http://codereview.chromium.org/6533006/diff/17001/chrome/browser/dom_ui/print...
chrome/browser/dom_ui/print_preview_handler.cc:41:
web_ui_util::GetJsonResponseFromFirstArgumentInList(args));
This looks wrong. Why does GetJSONResponseFromFirstArgumentInList return a
string? I would expect its signature to be:

bool GetJGetJSONResponseFromFirstArgumentInList(const ListValue* args,
                                                Value* out);

If you really want a string here I would suggest renaming that function since
JSON != string

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

http://codereview.chromium.org/6533006/diff/17001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:29: if
(printFromText.match(numericExp) && printToText.match(numericExp)) {
match is useful to get the matched groups. If all you care about is if it
matched test seems cleaner.

if (numerucExp.test(printFromText) ... )

http://codereview.chromium.org/6533006/diff/17001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:30: var printFrom =
parseInt(printFromText, 10);
or just Number(printFromText) since you only allow [0-9]+ anyway.

http://codereview.chromium.org/6533006/diff/17001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:32: if ((printFrom > 0) && (printTo >
0) && (printFrom <= printTo))
Too many parentheses

http://codereview.chromium.org/6533006/diff/17001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:32: if ((printFrom > 0) && (printTo >
0) && (printFrom <= printTo))
How can printFrom be less than 0 when the regexp only allows [0-9]?

parseInt returns NaN when it cannot parse an int.

http://codereview.chromium.org/6533006/diff/17001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:46: var pageRangesInfo = new Array();
Use []

http://codereview.chromium.org/6533006/diff/17001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:62: var pageRangeParams = new
Object();
Use an object literal

pageRangesInfo.push({
  from: parseInt(printFrom, 10),
  ...
});

http://codereview.chromium.org/6533006/diff/17001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:82: var jobSettings =
JSON.stringify({"printerName": printerName,
use single quotes

[kmadhusu, 2011-02-22 02:56:34]

arv: Addressed review comments. Please review.

http://codereview.chromium.org/6533006/diff/17001/chrome/browser/dom_ui/print...
File chrome/browser/dom_ui/print_preview_handler.cc (right):

http://codereview.chromium.org/6533006/diff/17001/chrome/browser/dom_ui/print...
chrome/browser/dom_ui/print_preview_handler.cc:41:
web_ui_util::GetJsonResponseFromFirstArgumentInList(args));
On 2011/02/21 02:00:05, arv wrote:
> This looks wrong. Why does GetJSONResponseFromFirstArgumentInList return a
> string? I would expect its signature to be:
> 
> bool GetJGetJSONResponseFromFirstArgumentInList(const ListValue* args,
>                                                 Value* out);
> 
> If you really want a string here I would suggest renaming that function since
> JSON != string

From my understanding, GetJSONResponseFromFirstArgumentInList() is a helper
function to get the first argument of the list as a string. 

In this case, the first argument is a JSONText and not a JSONObject.  So I used
that function to get the string from the list.

Please let me know if you still want me to change the function name to
"GetStringFromFirstArgumentInList()". I will submit a separate CL for that.

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

http://codereview.chromium.org/6533006/diff/17001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:29: if
(printFromText.match(numericExp) && printToText.match(numericExp)) {
On 2011/02/21 02:00:05, arv wrote:
> match is useful to get the matched groups. If all you care about is if it
> matched test seems cleaner.
> 
> if (numerucExp.test(printFromText) ... ) 

Done.

http://codereview.chromium.org/6533006/diff/17001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:30: var printFrom =
parseInt(printFromText, 10);
On 2011/02/21 02:00:05, arv wrote:
> or just Number(printFromText) since you only allow [0-9]+ anyway.

Done.

http://codereview.chromium.org/6533006/diff/17001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:32: if ((printFrom > 0) && (printTo >
0) && (printFrom <= printTo))
On 2011/02/21 02:00:05, arv wrote:
> Too many parentheses

Fixed.

http://codereview.chromium.org/6533006/diff/17001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:32: if ((printFrom > 0) && (printTo >
0) && (printFrom <= printTo))
On 2011/02/21 02:00:05, arv wrote:
> How can printFrom be less than 0 when the regexp only allows [0-9]?
> 
> parseInt returns NaN when it cannot parse an int.

Fixed.

http://codereview.chromium.org/6533006/diff/17001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:46: var pageRangesInfo = new Array();
On 2011/02/21 02:00:05, arv wrote:
> Use []

Done.

http://codereview.chromium.org/6533006/diff/17001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:62: var pageRangeParams = new
Object();
On 2011/02/21 02:00:05, arv wrote:
> Use an object literal
> 
> pageRangesInfo.push({
>   from: parseInt(printFrom, 10),
>   ...
> });

Done.

http://codereview.chromium.org/6533006/diff/17001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:82: var jobSettings =
JSON.stringify({"printerName": printerName,
On 2011/02/21 02:00:05, arv wrote:
> use single quotes

Done.

[arv (Not doing code reviews), 2011-02-23 19:38:52]

http://codereview.chromium.org/6533006/diff/17001/chrome/browser/dom_ui/print...
File chrome/browser/dom_ui/print_preview_handler.cc (right):

http://codereview.chromium.org/6533006/diff/17001/chrome/browser/dom_ui/print...
chrome/browser/dom_ui/print_preview_handler.cc:41:
web_ui_util::GetJsonResponseFromFirstArgumentInList(args));
On 2011/02/22 02:56:34, kmadhusu wrote:
> On 2011/02/21 02:00:05, arv wrote:
> > This looks wrong. Why does GetJSONResponseFromFirstArgumentInList return a
> > string? I would expect its signature to be:
> > 
> > bool GetJGetJSONResponseFromFirstArgumentInList(const ListValue* args,
> >                                                 Value* out);
> > 
> > If you really want a string here I would suggest renaming that function
since
> > JSON != string
> 
> From my understanding, GetJSONResponseFromFirstArgumentInList() is a helper
> function to get the first argument of the list as a string. 
> 
> In this case, the first argument is a JSONText and not a JSONObject.  So I
used
> that function to get the string from the list.
> 
> Please let me know if you still want me to change the function name to
> "GetStringFromFirstArgumentInList()". I will submit a separate CL for that. 

A separate CL is fine.

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

http://codereview.chromium.org/6533006/diff/20001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:49: var pageRangeList =
pageRangeText.split(',');
Should we not support white spaces in the text as well? If so, just remove them
before the split.


pageRangeText.replace(/\s/g, '').split(',');

[kmadhusu, 2011-02-24 02:11:33]

arv: Addressed your comments. Please review print_preview.js and
print_preview_handler.cc

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

http://codereview.chromium.org/6533006/diff/20001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:49: var pageRangeList =
pageRangeText.split(',');
On 2011/02/23 19:38:52, arv wrote:
> Should we not support white spaces in the text as well? If so, just remove
them
> before the split.
> 
> 
> pageRangeText.replace(/\s/g, '').split(',');

Done.

[Lei Zhang, 2011-02-24 03:49:32]

http://codereview.chromium.org/6533006/diff/23001/chrome/browser/printing/pri...
File chrome/browser/printing/print_job_worker.cc (right):

http://codereview.chromium.org/6533006/diff/23001/chrome/browser/printing/pri...
chrome/browser/printing/print_job_worker.cc:112: void
PrintJobWorker::UpdatePrintSettings(const std::string& new_settings) {
Have you considered parsing (and maybe validating) the json string when you
first get it in PrintPreviewHandler::HandlePrint(), and just pass a
DictionaryValue around. It's better to validate first and drop the request on
the floor if it doesn't make any sense, than to do a bunch of work, only then to
realize you can't continue due to bad settings.

http://codereview.chromium.org/6533006/diff/23001/chrome/browser/printing/pri...
chrome/browser/printing/print_job_worker.cc:118: DictionaryValue* settings =
nit: you don't need separate |settings| and |parsed_value| variables.

http://codereview.chromium.org/6533006/diff/23001/chrome/browser/printing/pri...
chrome/browser/printing/print_job_worker.cc:123: ListValue* pageRangeArray =
NULL;
you don't need this assignment or the ones below for |printFrom| and |printTo|,
since you'll never use the variable if the DictionaryValue.GetFoo() call fails.

http://codereview.chromium.org/6533006/diff/23001/chrome/browser/printing/pri...
chrome/browser/printing/print_job_worker.cc:129: int printFrom = 0;
Can you get rid of |printFrom| and |printTo|, instead do:

PageRange range;
if (dict->GetInteger("from", &range.from) &&
    dict->GetInteger("to", &range.to)) {
  // Page numbers are 0-based.
  range.from--;
  range.to--;
  new_ranges.push_back(range);
}

http://codereview.chromium.org/6533006/diff/23001/chrome/browser/printing/pri...
File chrome/browser/printing/printer_query.cc (right):

http://codereview.chromium.org/6533006/diff/23001/chrome/browser/printing/pri...
chrome/browser/printing/printer_query.cc:111: CancelableTask* callback) {
nit: indentation doesn't follow the style in the rest of the file.

http://codereview.chromium.org/6533006/diff/23001/chrome/browser/printing/pri...
chrome/browser/printing/printer_query.cc:112: DCHECK(!callback_.get());
You're duplicating code from PrinterQuery::GetSettings(). It should be trivial
to write a common method.

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

http://codereview.chromium.org/6533006/diff/23001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:144: #if defined(OS_MACOSX)
Why make this Mac only?

http://codereview.chromium.org/6533006/diff/23001/content/browser/renderer_ho...
File content/browser/renderer_host/render_message_filter.cc (right):

http://codereview.chromium.org/6533006/diff/23001/content/browser/renderer_ho...
content/browser/renderer_host/render_message_filter.cc:1182: // If user hasn't
cancelled.
Can't you move this block into the else block above?

[arv (Not doing code reviews), 2011-02-24 23:21:02]

The js/handler parts looks good with nits

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

http://codereview.chromium.org/6533006/diff/23001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:55: printFrom = tempRange[0];
This one could be moved out of the if since it is the same in both branches

http://codereview.chromium.org/6533006/diff/23001/chrome/browser/webui/print_...
File chrome/browser/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/6533006/diff/23001/chrome/browser/webui/print_...
chrome/browser/webui/print_preview_handler.cc:8: #include
"chrome/browser/webui/web_ui_util.h"
This is no longer needed

[kmadhusu, 2011-03-01 01:55:50]

arv: Addressed review comments.
jam, thestig: Addressed review comments. Moved print related messages from
render_message_filter to printing_message_filter. 

Please review. Thanks.

http://codereview.chromium.org/6533006/diff/23001/chrome/browser/printing/pri...
File chrome/browser/printing/print_job_worker.cc (right):

http://codereview.chromium.org/6533006/diff/23001/chrome/browser/printing/pri...
chrome/browser/printing/print_job_worker.cc:112: void
PrintJobWorker::UpdatePrintSettings(const std::string& new_settings) {
On 2011/02/24 03:49:32, Lei Zhang wrote:
> Have you considered parsing (and maybe validating) the json string when you
> first get it in PrintPreviewHandler::HandlePrint(), and just pass a
> DictionaryValue around. It's better to validate first and drop the request on
> the floor if it doesn't make any sense, than to do a bunch of work, only then
to
> realize you can't continue due to bad settings.

Fixed. Now UpdatePrintSettings() has a dictionary value as a parameter.

http://codereview.chromium.org/6533006/diff/23001/chrome/browser/printing/pri...
chrome/browser/printing/print_job_worker.cc:118: DictionaryValue* settings =
On 2011/02/24 03:49:32, Lei Zhang wrote:
> nit: you don't need separate |settings| and |parsed_value| variables.

Done.

http://codereview.chromium.org/6533006/diff/23001/chrome/browser/printing/pri...
chrome/browser/printing/print_job_worker.cc:123: ListValue* pageRangeArray =
NULL;
On 2011/02/24 03:49:32, Lei Zhang wrote:
> you don't need this assignment or the ones below for |printFrom| and
|printTo|,
> since you'll never use the variable if the DictionaryValue.GetFoo() call
fails.

Done.

http://codereview.chromium.org/6533006/diff/23001/chrome/browser/printing/pri...
chrome/browser/printing/print_job_worker.cc:129: int printFrom = 0;
On 2011/02/24 03:49:32, Lei Zhang wrote:
> Can you get rid of |printFrom| and |printTo|, instead do:
> 
> PageRange range;
> if (dict->GetInteger("from", &range.from) &&
>     dict->GetInteger("to", &range.to)) {
>   // Page numbers are 0-based.
>   range.from--;
>   range.to--;
>   new_ranges.push_back(range);
> }

Done.

http://codereview.chromium.org/6533006/diff/23001/chrome/browser/printing/pri...
File chrome/browser/printing/printer_query.cc (right):

http://codereview.chromium.org/6533006/diff/23001/chrome/browser/printing/pri...
chrome/browser/printing/printer_query.cc:111: CancelableTask* callback) {
On 2011/02/24 03:49:32, Lei Zhang wrote:
> nit: indentation doesn't follow the style in the rest of the file.

Fixed.

http://codereview.chromium.org/6533006/diff/23001/chrome/browser/printing/pri...
chrome/browser/printing/printer_query.cc:112: DCHECK(!callback_.get());
On 2011/02/24 03:49:32, Lei Zhang wrote:
> You're duplicating code from PrinterQuery::GetSettings(). It should be trivial
> to write a common method.

Added a common method "StartWorker()".

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

http://codereview.chromium.org/6533006/diff/23001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:55: printFrom = tempRange[0];
On 2011/02/24 23:21:03, arv wrote:
> This one could be moved out of the if since it is the same in both branches

Done.

http://codereview.chromium.org/6533006/diff/23001/chrome/browser/webui/print_...
File chrome/browser/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/6533006/diff/23001/chrome/browser/webui/print_...
chrome/browser/webui/print_preview_handler.cc:8: #include
"chrome/browser/webui/web_ui_util.h"
On 2011/02/24 23:21:03, arv wrote:
> This is no longer needed

Done.

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

http://codereview.chromium.org/6533006/diff/23001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:144: #if defined(OS_MACOSX)
On 2011/02/24 03:49:32, Lei Zhang wrote:
> Why make this Mac only?
As of now, I am implementing only the MAC preview printing workflow. After
implementing PrintingContextCairo::UpdatePrintSettings() and
PrintingContextWin::UpdatePrintSettings() function, I will remove this
condition.

http://codereview.chromium.org/6533006/diff/23001/content/browser/renderer_ho...
File content/browser/renderer_host/render_message_filter.cc (right):

http://codereview.chromium.org/6533006/diff/23001/content/browser/renderer_ho...
content/browser/renderer_host/render_message_filter.cc:1182: // If user hasn't
cancelled.
On 2011/02/24 03:49:32, Lei Zhang wrote:
> Can't you move this block into the else block above?

Fixed.

[jam, 2011-03-02 18:25:53]

lgtm for the higher-level stuff, i defer to others for printing specific code.

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
File chrome/browser/printing/printing_message_filter.cc (right):

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
chrome/browser/printing/printing_message_filter.cc:288: void
PrintingMessageFilter::OnUpdatePrintSettings(int document_cookie,
nit: (here and above) this style of tabbing is against our style guide.  i.e.
can't start the parameters on the same line as the function, and then continue
in the next line with a 4 space tab.  they all need to start from the same
place, so i.e. just move the first one down to the next line and either try to
fit as many as possible on a line, or put each on a separate line

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
File chrome/browser/printing/printing_message_filter.h (right):

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
chrome/browser/printing/printing_message_filter.h:71: // Get the current print
settings right here synchronously.
nit: here and below, no need to comment IPC dispatcher functions.  the messages
are documented already.

[Lei Zhang, 2011-03-02 23:28:53]

I haven't finished reviewing everything yet, but here's a list of items that
need to be addressed.

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
File chrome/browser/printing/print_job_worker.cc (right):

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
chrome/browser/printing/print_job_worker.cc:104: DCHECK_EQ(page_number_,
PageNumber::npos());
Do we need this DCHECK here?

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
chrome/browser/printing/print_job_worker.cc:115: ListValue* pageRangeArray;
nit: pageRangeArray -> page_range_array.

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
chrome/browser/printing/print_job_worker.cc:117: for (size_t index = 0; index <
pageRangeArray->GetSize(); index++) {
index++ -> ++index

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
chrome/browser/printing/print_job_worker.cc:120: if (dict) {
This is not the right way to check if |dict| is valid because you did not
initialize |dict| to NULL in the first place.

The better way to do this is to check to make sure GetDictionary() returned
true, just like you did with |pageRangeArray| above.

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
chrome/browser/printing/print_job_worker.cc:133: if (new_settings)
You don't need this check, you've already assumed |new_settings| is valid when
you called new_settings->GetList().

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

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:41: CHECK(args->GetString(0,
&json_str)) << "Could not read JSON argument";
I would not do a CHECK here. This would imply if the renderer process send us a
bad message, then the browser process would crash! Same for the CHECKs below.
You should not be sprinkling CHECKs in your code so liberally.

http://codereview.chromium.org/6533006/diff/35002/chrome/common/render_messag...
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/6533006/diff/35002/chrome/common/render_messag...
chrome/common/render_messages_internal.h:134:
IPC_MESSAGE_ROUTED1(ViewMsg_PrintForPrintPreview, DictionaryValue /* settings*/)
nit: put the DictionaryValue on a new line to be consistent with the rest of the
file.

http://codereview.chromium.org/6533006/diff/35002/chrome/common/render_messag...
chrome/common/render_messages_internal.h:1717: int /* document_cookie */,
ViewMsg_Print_Params already has a document cookie field. Same for
ViewHostMsg_UpdatePrintSettings below.

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

http://codereview.chromium.org/6533006/diff/35002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:164: // Initialize print page settings
with default settings.
This comment needs to be updated.

[Lei Zhang, 2011-03-03 00:22:44]

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
File chrome/browser/printing/print_job_worker.h (right):

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
chrome/browser/printing/print_job_worker.h:48: void SetSettings(const
DictionaryValue* const new_settings);
You probably should mention SetSettings() takes ownership of |new_settings| and
frees it.

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
File chrome/browser/printing/printer_query.cc (right):

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
chrome/browser/printing/printer_query.cc:102: scoped_ptr<DictionaryValue>
new_settings_copy(new_settings.DeepCopy());
Probably don't need the scoped_ptr if you are allocating it and immediately
releasing it.

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
File chrome/browser/printing/printer_query.h (right):

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
chrome/browser/printing/printer_query.h:60: // Returns true, if worker thread
exists or created.
nit: or has been created.

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
chrome/browser/printing/printer_query.h:61: bool StartWorker(CancelableTask*
callback);
This method should be private.

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
File chrome/browser/printing/printing_message_filter.cc (right):

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
chrome/browser/printing/printing_message_filter.cc:306:
printer_query->last_status() != printing::PrintingContext::OK) {
nit: indentation is wrong.

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
File chrome/browser/printing/printing_message_filter.h (right):

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
chrome/browser/printing/printing_message_filter.h:72: void
OnGetCurrentPrintSettings(int document_cookie,
You don't need |document_cookie| here or below since ViewMsg_Print_Params
already has a document_cookie field.

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

http://codereview.chromium.org/6533006/diff/35002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:160:
InitPrintSettings(element.document().frame(), &element,
What happens when InitPrintSettings() or UpdatePrintSettings() fail?

http://codereview.chromium.org/6533006/diff/35002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:213: if (!InitPrintSettings(frame,
node, PrintWebViewHelper::CURRENT)) {
Didn't we just call InitPrintSettings() and UpdatePrintSettings(). Shouldn't
|print_pages_params_| be up to date? Why do we have to fetch the settings again?

http://codereview.chromium.org/6533006/diff/35002/printing/printing_context.h
File printing/printing_context.h (right):

http://codereview.chromium.org/6533006/diff/35002/printing/printing_context.h...
printing/printing_context.h:54: // In future, update other print job settings.
nit: in the future.

http://codereview.chromium.org/6533006/diff/35002/printing/printing_context_c...
File printing/printing_context_cairo.h (right):

http://codereview.chromium.org/6533006/diff/35002/printing/printing_context_c...
printing/printing_context_cairo.h:43: virtual Result UpdatePrintSettings(const
PageRanges& ranges);
nit: This comes after InitWithSettings(), per printing_context.h. Same for the
other platforms' printing context headers.

[kmadhusu, 2011-03-04 19:39:02]

thestig: Addressed review comments. Please review my new patch.

Thanks.

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
File chrome/browser/printing/print_job_worker.cc (right):

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
chrome/browser/printing/print_job_worker.cc:104: DCHECK_EQ(page_number_,
PageNumber::npos());
On 2011/03/02 23:28:54, Lei Zhang wrote:
> Do we need this DCHECK here?

It is not required. Fixed.

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
chrome/browser/printing/print_job_worker.cc:115: ListValue* pageRangeArray;
On 2011/03/02 23:28:54, Lei Zhang wrote:
> nit: pageRangeArray -> page_range_array.

Done.

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
chrome/browser/printing/print_job_worker.cc:117: for (size_t index = 0; index <
pageRangeArray->GetSize(); index++) {
On 2011/03/02 23:28:54, Lei Zhang wrote:
> index++ -> ++index

Done.

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
chrome/browser/printing/print_job_worker.cc:120: if (dict) {
On 2011/03/02 23:28:54, Lei Zhang wrote:
> This is not the right way to check if |dict| is valid because you did not
> initialize |dict| to NULL in the first place.
> 
> The better way to do this is to check to make sure GetDictionary() returned
> true, just like you did with |pageRangeArray| above.

Fixed.

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
chrome/browser/printing/print_job_worker.cc:133: if (new_settings)
On 2011/03/02 23:28:54, Lei Zhang wrote:
> You don't need this check, you've already assumed |new_settings| is valid when
> you called new_settings->GetList().

Done.

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
File chrome/browser/printing/print_job_worker.h (right):

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
chrome/browser/printing/print_job_worker.h:48: void SetSettings(const
DictionaryValue* const new_settings);
On 2011/03/03 00:22:44, Lei Zhang wrote:
> You probably should mention SetSettings() takes ownership of |new_settings|
and
> frees it.

Done.

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
File chrome/browser/printing/printer_query.cc (right):

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
chrome/browser/printing/printer_query.cc:102: scoped_ptr<DictionaryValue>
new_settings_copy(new_settings.DeepCopy());
On 2011/03/03 00:22:44, Lei Zhang wrote:
> Probably don't need the scoped_ptr if you are allocating it and immediately
> releasing it.

Done.

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
File chrome/browser/printing/printer_query.h (right):

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
chrome/browser/printing/printer_query.h:60: // Returns true, if worker thread
exists or created.
On 2011/03/03 00:22:44, Lei Zhang wrote:
> nit: or has been created.

Done.

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
chrome/browser/printing/printer_query.h:61: bool StartWorker(CancelableTask*
callback);
On 2011/03/03 00:22:44, Lei Zhang wrote:
> This method should be private.

Done.

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
File chrome/browser/printing/printing_message_filter.cc (right):

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
chrome/browser/printing/printing_message_filter.cc:288: void
PrintingMessageFilter::OnUpdatePrintSettings(int document_cookie,
On 2011/03/02 18:25:53, John Abd-El-Malek wrote:
> nit: (here and above) this style of tabbing is against our style guide.  i.e.
> can't start the parameters on the same line as the function, and then continue
> in the next line with a 4 space tab.  they all need to start from the same
> place, so i.e. just move the first one down to the next line and either try to
> fit as many as possible on a line, or put each on a separate line

Done.

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
chrome/browser/printing/printing_message_filter.cc:306:
printer_query->last_status() != printing::PrintingContext::OK) {
On 2011/03/03 00:22:44, Lei Zhang wrote:
> nit: indentation is wrong.

Done.

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
File chrome/browser/printing/printing_message_filter.h (right):

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
chrome/browser/printing/printing_message_filter.h:71: // Get the current print
settings right here synchronously.
On 2011/03/02 18:25:53, John Abd-El-Malek wrote:
> nit: here and below, no need to comment IPC dispatcher functions.  the
messages
> are documented already.

Done.

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
chrome/browser/printing/printing_message_filter.h:72: void
OnGetCurrentPrintSettings(int document_cookie,
On 2011/03/03 00:22:44, Lei Zhang wrote:
> You don't need |document_cookie| here or below since ViewMsg_Print_Params
> already has a document_cookie field.

Removed this message handler.

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/printing/pri...
chrome/browser/printing/printing_message_filter.h:76: void
OnUpdatePrintSettings(int document_cookie,
I need document_cookie here.

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

http://codereview.chromium.org/6533006/diff/35002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:41: CHECK(args->GetString(0,
&json_str)) << "Could not read JSON argument";
On 2011/03/02 23:28:54, Lei Zhang wrote:
> I would not do a CHECK here. This would imply if the renderer process send us
a
> bad message, then the browser process would crash! Same for the CHECKs below.
> You should not be sprinkling CHECKs in your code so liberally.

Fixed.

http://codereview.chromium.org/6533006/diff/35002/chrome/common/render_messag...
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/6533006/diff/35002/chrome/common/render_messag...
chrome/common/render_messages_internal.h:134:
IPC_MESSAGE_ROUTED1(ViewMsg_PrintForPrintPreview, DictionaryValue /* settings*/)
On 2011/03/02 23:28:54, Lei Zhang wrote:
> nit: put the DictionaryValue on a new line to be consistent with the rest of
the
> file.

Done.

http://codereview.chromium.org/6533006/diff/35002/chrome/common/render_messag...
chrome/common/render_messages_internal.h:1717: int /* document_cookie */,
On 2011/03/02 23:28:54, Lei Zhang wrote:
> ViewMsg_Print_Params already has a document cookie field. Same for
> ViewHostMsg_UpdatePrintSettings below.

Removed ViewHostMsg_GetCurrentPrintSettings Info. I need |document_cookie| for
ViewHostMsg_UpdatePrintSettings.

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

http://codereview.chromium.org/6533006/diff/35002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:160:
InitPrintSettings(element.document().frame(), &element,
On 2011/03/03 00:22:44, Lei Zhang wrote:
> What happens when InitPrintSettings() or UpdatePrintSettings() fail?

Fixed.

http://codereview.chromium.org/6533006/diff/35002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:213: if (!InitPrintSettings(frame,
node, PrintWebViewHelper::CURRENT)) {
On 2011/03/03 00:22:44, Lei Zhang wrote:
> Didn't we just call InitPrintSettings() and UpdatePrintSettings(). Shouldn't
> |print_pages_params_| be up to date? Why do we have to fetch the settings
again?

Removed this function. Calling RenderPagesForPrint() directly from
OnPrintForPrintPreview().

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

http://codereview.chromium.org/6533006/diff/35002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:164: // Initialize print page settings
with default settings.
On 2011/03/02 23:28:54, Lei Zhang wrote:
> This comment needs to be updated.

I reverted my changes to this function. So I didn't update this comment.

http://codereview.chromium.org/6533006/diff/35002/printing/printing_context.h
File printing/printing_context.h (right):

http://codereview.chromium.org/6533006/diff/35002/printing/printing_context.h...
printing/printing_context.h:54: // In future, update other print job settings.
On 2011/03/03 00:22:44, Lei Zhang wrote:
> nit: in the future.

Done.

http://codereview.chromium.org/6533006/diff/35002/printing/printing_context_c...
File printing/printing_context_cairo.h (right):

http://codereview.chromium.org/6533006/diff/35002/printing/printing_context_c...
printing/printing_context_cairo.h:43: virtual Result UpdatePrintSettings(const
PageRanges& ranges);
On 2011/03/03 00:22:44, Lei Zhang wrote:
> nit: This comes after InitWithSettings(), per printing_context.h. Same for the
> other platforms' printing context headers.

Moved it before InitPrintSettings in printing_context.h

[Lei Zhang, 2011-03-04 20:01:56]

LGTM

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

http://codereview.chromium.org/6533006/diff/45005/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:526:
(*print_pages_params_).params.document_cookie,
nit: isn't this print_pages_params->params.document_cookie?

[kmadhusu, 2011-03-04 21:20:03]

thestig: Thanks for your comments.

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

http://codereview.chromium.org/6533006/diff/45005/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:526:
(*print_pages_params_).params.document_cookie,
On 2011/03/04 20:01:56, Lei Zhang wrote:
> nit: isn't this print_pages_params->params.document_cookie?

Done.