Print Preview: Fixing behavior of event listeners.

Print Preview: Fixing behavior of event listeners.

Now that pending preview requests are being canceled when a new request is issued, the event listeners of the printing controls should not check for |hasPendingPreviewRequest| before issuing new requests, instead they should immediately request a new preview so that the previous one is canceled.

Also all functions that are associated with a specific print request should get the requestID as a parameter, in order to ignore obsolete responses.

I found out that when the print preview tab is reloaded, responses from requests before the reload are reaching the ui code. If reloading the preview tab repeatedly all these request have id=0 and are not being ignored as they should. Therefore I changed the id of the initial request to have a random variable.

BUG=91556, 91769
TEST=See bug description

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

[dpapad, 2011-08-03 18:38:45]

I have not removed the console.log statements from print_previw.js yet to make
it easier for the reviewers to test.

Also I believe that once we make sure this works ok, it should be merged to M14.

[kmadhusu, 2011-08-03 18:39:45]

I am yet to test your code.

http://codereview.chromium.org/7550022/diff/5005/chrome/browser/printing/prin...
File chrome/browser/printing/print_preview_message_handler.cc (right):

http://codereview.chromium.org/7550022/diff/5005/chrome/browser/printing/prin...
chrome/browser/printing/print_preview_message_handler.cc:86: bool is_modifiable,
I think its worth to create a new Struct for this message.

[kmadhusu, 2011-08-05 16:01:12]

dpapad: Can you rebase your CL?

[Lei Zhang, 2011-08-06 00:52:57]

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

http://codereview.chromium.org/7550022/diff/7024/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:86:
initialPreviewRequestID = getRandomIntegerWithinRange(0, 32000);
Is this so if we reload the page, we have a unique starting point, whereas 0 can
be confused with the preview tab's request from before the reload?

http://codereview.chromium.org/7550022/diff/7024/chrome/common/print_messages.h
File chrome/common/print_messages.h (right):

http://codereview.chromium.org/7550022/diff/7024/chrome/common/print_messages...
chrome/common/print_messages.h:53: IPC_STRUCT_MEMBER(bool, is_first_request)
nit: can you comment these two struct members and mention they are only used for
print preview?

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

http://codereview.chromium.org/7550022/diff/7024/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:778: if
(!job_settings.GetBoolean(printing::kIsFirstRequest,
Why do we need this check here?

[dpapad, 2011-08-06 01:21:20]

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

http://codereview.chromium.org/7550022/diff/7024/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:86:
initialPreviewRequestID = getRandomIntegerWithinRange(0, 32000);
On 2011/08/06 00:52:57, Lei Zhang wrote:
> Is this so if we reload the page, we have a unique starting point, whereas 0
can
> be confused with the preview tab's request from before the reload?

Exactly. This was happening when reloading the page repeatedly (several requests
with the same id=0 in flight). Also I stop the range to 32000, because I want to
make sure that it fits in a c integer (I could increase it a little bit more
though).

http://codereview.chromium.org/7550022/diff/7024/chrome/common/print_messages.h
File chrome/common/print_messages.h (right):

http://codereview.chromium.org/7550022/diff/7024/chrome/common/print_messages...
chrome/common/print_messages.h:53: IPC_STRUCT_MEMBER(bool, is_first_request)
On 2011/08/06 00:52:57, Lei Zhang wrote:
> nit: can you comment these two struct members and mention they are only used
for
> print preview?

Done.

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

http://codereview.chromium.org/7550022/diff/7024/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:778: if
(!job_settings.GetBoolean(printing::kIsFirstRequest,
On 2011/08/06 00:52:57, Lei Zhang wrote:
> Why do we need this check here?

Is this method used from the old print flow? Then, this check should be removed.
I added the check because I saw that it is performed for all other settings.

[dpapad, 2011-08-08 18:25:35]

Addressed kmadhusu's comment.

http://codereview.chromium.org/7550022/diff/5005/chrome/browser/printing/prin...
File chrome/browser/printing/print_preview_message_handler.cc (right):

http://codereview.chromium.org/7550022/diff/5005/chrome/browser/printing/prin...
chrome/browser/printing/print_preview_message_handler.cc:86: bool is_modifiable,
On 2011/08/03 18:39:45, kmadhusu wrote:
> I think its worth to create a new Struct for this message.

Done.

[kmadhusu, 2011-08-08 22:20:45]

http://codereview.chromium.org/7550022/diff/20001/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_message_handler.cc (right):

http://codereview.chromium.org/7550022/diff/20001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:93:
print_preview_ui->OnDidGetPreviewPageCount(
You can change PrintPreviewUI::OnDidGetPreviewPageCount() to take 
PrintHostMsg_DidGetPreviewPageCount_Params as an argument.

http://codereview.chromium.org/7550022/diff/20001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:160: params.modifiable,
nit: Can you make this line style similar to line #94?

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

http://codereview.chromium.org/7550022/diff/20001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/color_settings.js:51:
this.colorRadioButton_.onclick = function() {
Can you make this this.colorRadioButton_.onclick = setColor(true);

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

http://codereview.chromium.org/7550022/diff/20001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/layout_settings.js:51:
this.landscapeRadioButton_.onclick = function() {
How about:
this.landscapeRadioButton_.onclick = this.onLayoutButtonClick_().bind(this); ? 

It will be consistent with page_settings.js line #338.

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

http://codereview.chromium.org/7550022/diff/20001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:86:
initialPreviewRequestID = getRandomIntegerWithinRange(0, 32000);
Use constants for range min & max values.

http://codereview.chromium.org/7550022/diff/20001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:301: 'isFirstRequst' :
false,
Typo "isFirstRequst" -> "isFirstRequest"

http://codereview.chromium.org/7550022/diff/20001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:328: * @return {boolean}
True iff this is the |lastPreviewRequestID| corresponds
Typo "iff" => "if"

http://codereview.chromium.org/7550022/diff/20001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:429:
settings.isFirstRequest = isFirstPreviewRequest();
Can't you call isFirstPreviewRequest() and generatePreviewRequestID() in line
#301 & #302?

http://codereview.chromium.org/7550022/diff/20001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:792:
console.log("onDidGetPreviewPageCount " + previewResponseId);
Do you still need these debug statements?

http://codereview.chromium.org/7550022/diff/20001/chrome/common/print_messages.h
File chrome/common/print_messages.h (right):

http://codereview.chromium.org/7550022/diff/20001/chrome/common/print_message...
chrome/common/print_messages.h:56: IPC_STRUCT_MEMBER(bool, is_first_request)
We have an instance of this struct in
src/chrome/browser/printing/print_dialog_cloud.cc and mock_printer.cc. Please
make sure we set the default values for these member variables.

http://codereview.chromium.org/7550022/diff/20001/chrome/renderer/mock_render...
File chrome/renderer/mock_render_thread.cc (right):

http://codereview.chromium.org/7550022/diff/20001/chrome/renderer/mock_render...
chrome/renderer/mock_render_thread.cc:241:
!job_settings.GetInteger(printing::kSettingCopies, NULL)) {
Make sure job_settings has "IsFirstRequest" and "requestID" keys.

http://codereview.chromium.org/7550022/diff/20001/chrome/renderer/mock_render...
File chrome/renderer/mock_render_thread.h (right):

http://codereview.chromium.org/7550022/diff/20001/chrome/renderer/mock_render...
chrome/renderer/mock_render_thread.h:128: void OnDidGetPreviewPageCount(int
document_cookie, int number_pages,
When you compile browser_tests, I think you will see an compile error. This
function should take "PrintHostMsg_DidGetPreviewPageCount_Params params" as
argument.

[dpapad, 2011-08-08 23:41:43]

http://codereview.chromium.org/7550022/diff/20001/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_message_handler.cc (right):

http://codereview.chromium.org/7550022/diff/20001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:93:
print_preview_ui->OnDidGetPreviewPageCount(
On 2011/08/08 22:20:45, kmadhusu wrote:
> You can change PrintPreviewUI::OnDidGetPreviewPageCount() to take 
> PrintHostMsg_DidGetPreviewPageCount_Params as an argument. 

I could, but I did not do it for consistency. None of the other methods in
PrintPreviewUI use the structs that are being used for IPC messages.

http://codereview.chromium.org/7550022/diff/20001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:160: params.modifiable,
On 2011/08/08 22:20:45, kmadhusu wrote:
> nit: Can you make this line style similar to line #94?

Done.

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

http://codereview.chromium.org/7550022/diff/20001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/color_settings.js:51:
this.colorRadioButton_.onclick = function() {
On 2011/08/08 22:20:45, kmadhusu wrote:
> Can you make this this.colorRadioButton_.onclick = setColor(true);

No this is syntactically wrong. setColor(true) does not return a function in
order to assign it to onclick.

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

http://codereview.chromium.org/7550022/diff/20001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/layout_settings.js:51:
this.landscapeRadioButton_.onclick = function() {
On 2011/08/08 22:20:45, kmadhusu wrote:
> How about:
> this.landscapeRadioButton_.onclick = this.onLayoutButtonClick_().bind(this); ?

> 
> It will be consistent with page_settings.js line #338.

Done.

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

http://codereview.chromium.org/7550022/diff/20001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:86:
initialPreviewRequestID = getRandomIntegerWithinRange(0, 32000);
On 2011/08/08 22:20:45, kmadhusu wrote:
> Use constants for range min & max values.

Done.

http://codereview.chromium.org/7550022/diff/20001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:301: 'isFirstRequst' :
false,
On 2011/08/08 22:20:45, kmadhusu wrote:
> Typo "isFirstRequst" -> "isFirstRequest"

Done. Good catch.

http://codereview.chromium.org/7550022/diff/20001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:328: * @return {boolean}
True iff this is the |lastPreviewRequestID| corresponds
On 2011/08/08 22:20:45, kmadhusu wrote:
> Typo "iff" => "if"

Done. It is not a typo though, iff meand "if and only if"
(http://en.wikipedia.org/wiki/If_and_only_if).

http://codereview.chromium.org/7550022/diff/20001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:429:
settings.isFirstRequest = isFirstPreviewRequest();
On 2011/08/08 22:20:45, kmadhusu wrote:
> Can't you call isFirstPreviewRequest() and generatePreviewRequestID() in line
> #301 & #302?

No. Doing so would affect the Ids every time you call getSettings() making it
impossible to call it (even just for debugging). Just imagine adding a
console.log(getSettings()), it would break the code.

http://codereview.chromium.org/7550022/diff/20001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:792:
console.log("onDidGetPreviewPageCount " + previewResponseId);
On 2011/08/08 22:20:45, kmadhusu wrote:
> Do you still need these debug statements?

Done.

http://codereview.chromium.org/7550022/diff/20001/chrome/common/print_messages.h
File chrome/common/print_messages.h (right):

http://codereview.chromium.org/7550022/diff/20001/chrome/common/print_message...
chrome/common/print_messages.h:56: IPC_STRUCT_MEMBER(bool, is_first_request)
On 2011/08/08 22:20:45, kmadhusu wrote:
> We have an instance of this struct in
> src/chrome/browser/printing/print_dialog_cloud.cc and mock_printer.cc. Please
> make sure we set the default values for these member variables.

Done for print_dialog_cloud.cc, still investigating how to initialize those
parameters in mock_printer.cc.

http://codereview.chromium.org/7550022/diff/20001/chrome/renderer/mock_render...
File chrome/renderer/mock_render_thread.cc (right):

http://codereview.chromium.org/7550022/diff/20001/chrome/renderer/mock_render...
chrome/renderer/mock_render_thread.cc:241:
!job_settings.GetInteger(printing::kSettingCopies, NULL)) {
On 2011/08/08 22:20:45, kmadhusu wrote:
> Make sure job_settings has "IsFirstRequest" and "requestID" keys.

Done.

http://codereview.chromium.org/7550022/diff/20001/chrome/renderer/mock_render...
File chrome/renderer/mock_render_thread.h (right):

http://codereview.chromium.org/7550022/diff/20001/chrome/renderer/mock_render...
chrome/renderer/mock_render_thread.h:128: void OnDidGetPreviewPageCount(int
document_cookie, int number_pages,
On 2011/08/08 22:20:45, kmadhusu wrote:
> When you compile browser_tests, I think you will see an compile error. This
> function should take "PrintHostMsg_DidGetPreviewPageCount_Params params" as
> argument.

Done.

[kmadhusu, 2011-08-09 22:30:40]

http://codereview.chromium.org/7550022/diff/20001/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_message_handler.cc (right):

http://codereview.chromium.org/7550022/diff/20001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:93:
print_preview_ui->OnDidGetPreviewPageCount(
On 2011/08/08 23:41:43, dpapad wrote:
> On 2011/08/08 22:20:45, kmadhusu wrote:
> > You can change PrintPreviewUI::OnDidGetPreviewPageCount() to take 
> > PrintHostMsg_DidGetPreviewPageCount_Params as an argument. 
> 
> I could, but I did not do it for consistency. None of the other methods in
> PrintPreviewUI use the structs that are being used for IPC messages.

No other method in PrintPreviewUI uses all of the struct member variables as
parameters. I think you can make this change now.

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

http://codereview.chromium.org/7550022/diff/20001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:429:
settings.isFirstRequest = isFirstPreviewRequest();
On 2011/08/08 23:41:43, dpapad wrote:
> On 2011/08/08 22:20:45, kmadhusu wrote:
> > Can't you call isFirstPreviewRequest() and generatePreviewRequestID() in
line
> > #301 & #302?
> 
> No. Doing so would affect the Ids every time you call getSettings() making it
> impossible to call it (even just for debugging). Just imagine adding a
> console.log(getSettings()), it would break the code.

For debugging purpose you can always call console.log(settings). Why do you need
to call console.log(getSettings()) if you very well know that the requestID and
isFirstRequest of the settings is always '0' and 'false'?

http://codereview.chromium.org/7550022/diff/23002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:298: 'landscape':
layoutSettings.isLandscape(),
Looks like we do not use 'printAll' setting value. Can you remove that?

http://codereview.chromium.org/7550022/diff/23002/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview_utils.js (right):

http://codereview.chromium.org/7550022/diff/23002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview_utils.js:201: function
getRandomIntegerWithinRange(from, to) {
This function can return negative value. Please validate from & to and make sure
Math.floor returns a positive integer.

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

http://codereview.chromium.org/7550022/diff/23002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:779: return false;
nit: We don't want to return if we are unable to get this setting. Just do a
NOTREACHED() and proceed further.

[dpapad, 2011-08-10 00:17:10]

http://codereview.chromium.org/7550022/diff/20001/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_message_handler.cc (right):

http://codereview.chromium.org/7550022/diff/20001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:93:
print_preview_ui->OnDidGetPreviewPageCount(
On 2011/08/09 22:30:40, kmadhusu wrote:
> On 2011/08/08 23:41:43, dpapad wrote:
> > On 2011/08/08 22:20:45, kmadhusu wrote:
> > > You can change PrintPreviewUI::OnDidGetPreviewPageCount() to take 
> > > PrintHostMsg_DidGetPreviewPageCount_Params as an argument. 
> > 
> > I could, but I did not do it for consistency. None of the other methods in
> > PrintPreviewUI use the structs that are being used for IPC messages.
> 
> No other method in PrintPreviewUI uses all of the struct member variables as
> parameters. I think you can make this change now. 

Done.

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

http://codereview.chromium.org/7550022/diff/20001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:429:
settings.isFirstRequest = isFirstPreviewRequest();
On 2011/08/09 22:30:40, kmadhusu wrote:
> On 2011/08/08 23:41:43, dpapad wrote:
> > On 2011/08/08 22:20:45, kmadhusu wrote:
> > > Can't you call isFirstPreviewRequest() and generatePreviewRequestID() in
> line
> > > #301 & #302?
> > 
> > No. Doing so would affect the Ids every time you call getSettings() making
it
> > impossible to call it (even just for debugging). Just imagine adding a
> > console.log(getSettings()), it would break the code.
> 
> For debugging purpose you can always call console.log(settings). Why do you
need
> to call console.log(getSettings()) if you very well know that the requestID
and
> isFirstRequest of the settings is always '0' and 'false'?

Done, even though I am not sure that this makes the code better. Here is why:

1) There is no need for a "settings" local var (so you cant just print it) as
you suggest. 
2) A function getXXXX() should not have any side effects (violates the principle
of least surprise). Also now a specific note has to be made in the documentation
to prevent any misuse. (Maybe renaming the function to something like
generateSettingsTicket would solve that).
3) Now even when sending a request for printing and not for previewing these
fields are populated (and ignored by the backend).

Pls respond if you still prefer to keep it as suggested.

http://codereview.chromium.org/7550022/diff/23002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:298: 'landscape':
layoutSettings.isLandscape(),
On 2011/08/09 22:30:40, kmadhusu wrote:
> Looks like we do not use 'printAll' setting value. Can you remove that?

Done.

http://codereview.chromium.org/7550022/diff/23002/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview_utils.js (right):

http://codereview.chromium.org/7550022/diff/23002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview_utils.js:201: function
getRandomIntegerWithinRange(from, to) {
On 2011/08/09 22:30:40, kmadhusu wrote:
> This function can return negative value. Please validate from & to and make
sure
> Math.floor returns a positive integer. 

This function does what it says in the documentation. It can return negative
numbers if |from| is negative. It is up to the caller to use it appropriately.
Why constraining it to return a positive integer?

I did fix it though so that the order of arguments does not matter
(getRandomIntegerWithinRange(0,10) and getRandomIntegerWithinRange(10,0) both
work as expected now).
I am also planning to a new test for this function.

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

http://codereview.chromium.org/7550022/diff/23002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:779: return false;
On 2011/08/09 22:30:40, kmadhusu wrote:
> nit: We don't want to return if we are unable to get this setting. Just do a
> NOTREACHED() and proceed further.

Done.

[Lei Zhang, 2011-08-10 03:26:07]

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

http://codereview.chromium.org/7550022/diff/20001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:429:
settings.isFirstRequest = isFirstPreviewRequest();
On 2011/08/10 00:17:10, dpapad wrote:
> On 2011/08/09 22:30:40, kmadhusu wrote:
> > On 2011/08/08 23:41:43, dpapad wrote:
> > > On 2011/08/08 22:20:45, kmadhusu wrote:
> > > > Can't you call isFirstPreviewRequest() and generatePreviewRequestID() in
> > line
> > > > #301 & #302?
> > > 
> > > No. Doing so would affect the Ids every time you call getSettings() making
> it
> > > impossible to call it (even just for debugging). Just imagine adding a
> > > console.log(getSettings()), it would break the code.
> > 
> > For debugging purpose you can always call console.log(settings). Why do you
> need
> > to call console.log(getSettings()) if you very well know that the requestID
> and
> > isFirstRequest of the settings is always '0' and 'false'?
> 
> Done, even though I am not sure that this makes the code better. Here is why:
> 
> 1) There is no need for a "settings" local var (so you cant just print it) as
> you suggest. 
> 2) A function getXXXX() should not have any side effects (violates the
principle
> of least surprise). Also now a specific note has to be made in the
documentation
> to prevent any misuse. (Maybe renaming the function to something like
> generateSettingsTicket would solve that).
> 3) Now even when sending a request for printing and not for previewing these
> fields are populated (and ignored by the backend).
> 
> Pls respond if you still prefer to keep it as suggested.

I would be surprised if I am new to the code and used getSettings() only to find
it actually changes the state. If you were to stick with patchset 14, I'd rename
this to something like getSettingsWithNextRequestID().

http://codereview.chromium.org/7550022/diff/30002/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview_utils.js (right):

http://codereview.chromium.org/7550022/diff/30002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview_utils.js:201: function
getRandomIntegerWithinRange(endPointA, endPointB) {
I think most people call this function "randInt()".

[dpapad, 2011-08-10 15:47:09]

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

http://codereview.chromium.org/7550022/diff/20001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:429:
settings.isFirstRequest = isFirstPreviewRequest();
Done. I restored getSettings() to have no side effects and added a function
getSettingsWithRequestID() that adds the side effects. When requesting to print
(or when debugging) getSettings() is called, when requesting a preview
getSettingsWithRequestID() should be called.

http://codereview.chromium.org/7550022/diff/30002/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview_utils.js (right):

http://codereview.chromium.org/7550022/diff/30002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview_utils.js:201: function
getRandomIntegerWithinRange(endPointA, endPointB) {
On 2011/08/10 03:26:07, Lei Zhang wrote:
> I think most people call this function "randInt()".

Renamed to randomInteger() instead of randInt(). According to the C++ styleguide
"Function names, variable names, and filenames should be descriptive; eschew
abbreviation". I think it is a good practice to apply in JS too.

[kmadhusu, 2011-08-10 17:12:40]

I assume you are still investigating on how to initialize those parameters in
mock_printer.cc.

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

http://codereview.chromium.org/7550022/diff/33002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:287: * Note:
|lastPreviewRequestID| is being modified every time this function is
How |lastPreviewRequestID| change when you call getSettings()?

http://codereview.chromium.org/7550022/diff/33002/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_ui.h (right):

http://codereview.chromium.org/7550022/diff/33002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui.h:46: // pages. |params.is_modifiable|
indicates if the preview can be rerendered
nit: I think the struct definition has a detailed description about the member
variables. We don't want to repeat that here.

[dpapad, 2011-08-10 17:21:39]

I am still looking on mock_printer.cc.

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

http://codereview.chromium.org/7550022/diff/33002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:287: * Note:
|lastPreviewRequestID| is being modified every time this function is
On 2011/08/10 17:12:40, kmadhusu wrote:
> How |lastPreviewRequestID| change when you call getSettings()?

Obviously that is an obsolete comment belonging to the previous patch (but was
not uploaded before). This comment has been pasted to
getSettingsWithWithRequestID(). Removed from here.

http://codereview.chromium.org/7550022/diff/33002/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_ui.h (right):

http://codereview.chromium.org/7550022/diff/33002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui.h:46: // pages. |params.is_modifiable|
indicates if the preview can be rerendered
On 2011/08/10 17:12:40, kmadhusu wrote:
> nit: I think the struct definition has a detailed description about the member
> variables. We don't want to repeat that here.

Done. Removed the details from this comment.

[dpapad, 2011-08-10 17:39:35]

MockPrinter is only used in print_web_view_helper_browsertest.cc
PrintWebViewHelperTest.PrintWithIframe. I dont see how this is related to the
new added parameters. I am running some tryjobs to confirm.

[kmadhusu1, 2011-08-10 18:03:03]

On Wed, Aug 10, 2011 at 10:39 AM, <dpapad@chromium.org> wrote:

> MockPrinter is only used in print_web_view_helper_**browsertest.cc
> PrintWebViewHelperTest.**PrintWithIframe. I dont see how this is related
> to the
> new added parameters. I am running some tryjobs to confirm.
>
>
MockPrinter is also used by preview tests.
MockRenderThread::OnUpdatePrintSettings() uses this mock printer and this
message handler is called during the preview workflow.


>
>
http://codereview.chromium.**org/7550022/<http://codereview.chromium.org/7550...
>

[dpapad, 2011-08-11 21:21:37]

Remove some checks that made sense when 1 pending request was allowed, but do
not apply anymore.

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

http://codereview.chromium.org/7550022/diff/49001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:437:
layoutSettings.updateState();
I could send an event here to update the state so that all settings can receive
it (and have updateState as a private method), but for the moment only
layoutSettings do that.

[dpapad, 2011-08-11 22:52:34]

Resolved conflicts with ToT.

[Lei Zhang, 2011-08-12 02:11:34]

http://codereview.chromium.org/7550022/diff/39006/chrome/renderer/mock_printe...
File chrome/renderer/mock_printer.cc (right):

http://codereview.chromium.org/7550022/diff/39006/chrome/renderer/mock_printe...
chrome/renderer/mock_printer.cc:39: is_first_request_(false),
Why don't we just default to true and 0 and be done with it? I think that should
work; correct me if I'm wrong.

[dpapad, 2011-08-12 19:01:28]

http://codereview.chromium.org/7550022/diff/39006/chrome/renderer/mock_printe...
File chrome/renderer/mock_printer.cc (right):

http://codereview.chromium.org/7550022/diff/39006/chrome/renderer/mock_printe...
chrome/renderer/mock_printer.cc:39: is_first_request_(false),
On 2011/08/12 02:11:34, Lei Zhang wrote:
> Why don't we just default to true and 0 and be done with it? I think that
should
> work; correct me if I'm wrong.

Done.

[Lei Zhang, 2011-08-12 19:41:47]

LGTM

[kmadhusu, 2011-08-12 19:53:19]

LGTM