Print Preview: Associating preview requests and responses using an identifier

Print Preview: Associating preview requests and responses using an identifier

BUG=88807, 89106
TEST=NONE


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

[Lei Zhang, 2011-07-12 22:36:31]

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

http://codereview.chromium.org/7313035/diff/8002/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:381: settings.GetInteger("requestID",
&preview_params.preview_request_id);
can you put requestID in printing/print_job_constants.h?

http://codereview.chromium.org/7313035/diff/8002/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:658: if (!job_settings.GetInteger(
You can do this check first before the Send() to fail early without having to
send an IPC to the browser.

I'd also add a NOTREACHED() since this should never fail.

http://codereview.chromium.org/7313035/diff/8002/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/7313035/diff/8002/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_linux.cc:46:
PrintHostMsg_DidPreviewDocument_Params preview_params;
We can probably do a little bit of refactoring to combine some of the code here
with the same code in the Win/Mac versions.

[dpapad, 2011-07-12 23:02:11]

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

http://codereview.chromium.org/7313035/diff/8002/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:381: settings.GetInteger("requestID",
&preview_params.preview_request_id);
On 2011/07/12 22:36:31, Lei Zhang wrote:
> can you put requestID in printing/print_job_constants.h?

Done.

http://codereview.chromium.org/7313035/diff/8002/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:658: if (!job_settings.GetInteger(
On 2011/07/12 22:36:31, Lei Zhang wrote:
> You can do this check first before the Send() to fail early without having to
> send an IPC to the browser.
> 
> I'd also add a NOTREACHED() since this should never fail.

Done.

http://codereview.chromium.org/7313035/diff/8002/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/7313035/diff/8002/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_linux.cc:46:
PrintHostMsg_DidPreviewDocument_Params preview_params;
On 2011/07/12 22:36:31, Lei Zhang wrote:
> We can probably do a little bit of refactoring to combine some of the code
here
> with the same code in the Win/Mac versions.

Yes it seems that some code is simply duplicated. Did you mean to do it in this
CL?

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

http://codereview.chromium.org/7313035/diff/12008/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:666:
&settings.params.preview_request_id);
I still have to do this, because it seems that |settings| is getting overwritten
in line 660.

[Lei Zhang, 2011-07-12 23:19:25]

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

http://codereview.chromium.org/7313035/diff/12008/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:382:
settings.GetInteger(printing::kPreviewRequestID,
Actually, why do we have to do this again here? Didn't we fetch the request id
and stored it in |print_pages_params| on line 668?

http://codereview.chromium.org/7313035/diff/12008/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:666:
&settings.params.preview_request_id);
On 2011/07/12 23:02:11, dpapad wrote:
> I still have to do this, because it seems that |settings| is getting
overwritten
> in line 660.

Oh, in that case just ignore my comment about doing the check early and put this
back the way it was.

http://codereview.chromium.org/7313035/diff/12008/printing/print_job_constants.h
File printing/print_job_constants.h (right):

http://codereview.chromium.org/7313035/diff/12008/printing/print_job_constant...
printing/print_job_constants.h:21: extern const char kPreviewRequestID[];
alphabetical order?

[dpapad, 2011-07-12 23:42:29]

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

http://codereview.chromium.org/7313035/diff/12008/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:382:
settings.GetInteger(printing::kPreviewRequestID,
On 2011/07/12 23:19:30, Lei Zhang wrote:
> Actually, why do we have to do this again here? Didn't we fetch the request id
> and stored it in |print_pages_params| on line 668?

This handles the case when there is no rerendering. At this point
|print_pages_params_| contains the id of the previous request (if any), right?

In any case |preview_params| is the struct being sent to the js code that
results in a call to updatePrintPreview(). We need to inject the request ID in
that struct. That is why I had to add preview_request_id in 2 places in
chrome/common/print_messages.h.

http://codereview.chromium.org/7313035/diff/12008/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:666:
&settings.params.preview_request_id);
On 2011/07/12 23:19:30, Lei Zhang wrote:
> On 2011/07/12 23:02:11, dpapad wrote:
> > I still have to do this, because it seems that |settings| is getting
> overwritten
> > in line 660.
> 
> Oh, in that case just ignore my comment about doing the check early and put
this
> back the way it was.

Done.

http://codereview.chromium.org/7313035/diff/12008/printing/print_job_constants.h
File printing/print_job_constants.h (right):

http://codereview.chromium.org/7313035/diff/12008/printing/print_job_constant...
printing/print_job_constants.h:21: extern const char kPreviewRequestID[];
On 2011/07/12 23:19:30, Lei Zhang wrote:
> alphabetical order?

Done.

[Lei Zhang, 2011-07-12 23:58:40]

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

http://codereview.chromium.org/7313035/diff/12008/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:382:
settings.GetInteger(printing::kPreviewRequestID,
On 2011/07/12 23:42:29, dpapad wrote:
> On 2011/07/12 23:19:30, Lei Zhang wrote:
> > Actually, why do we have to do this again here? Didn't we fetch the request
id
> > and stored it in |print_pages_params| on line 668?
> 
> This handles the case when there is no rerendering. At this point
> |print_pages_params_| contains the id of the previous request (if any), right?
> 
> In any case |preview_params| is the struct being sent to the js code that
> results in a call to updatePrintPreview(). We need to inject the request ID in
> that struct. That is why I had to add preview_request_id in 2 places in
> chrome/common/print_messages.h.

We just called UpdatePrintSettings(), which should have the most recent request
id in print_pages_params_, right? What I'm suggesting is to just assign it
directly:

preview_params.preview_request_id =
print_pages_params_.params.preview_request_id;

[dpapad, 2011-07-13 00:15:45]

I still need to rebase because of revision 92275.

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

http://codereview.chromium.org/7313035/diff/12008/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:382:
settings.GetInteger(printing::kPreviewRequestID,
On 2011/07/12 23:58:40, Lei Zhang wrote:
> On 2011/07/12 23:42:29, dpapad wrote:
> > On 2011/07/12 23:19:30, Lei Zhang wrote:
> > > Actually, why do we have to do this again here? Didn't we fetch the
request
> id
> > > and stored it in |print_pages_params| on line 668?
> > 
> > This handles the case when there is no rerendering. At this point
> > |print_pages_params_| contains the id of the previous request (if any),
right?
> > 
> > In any case |preview_params| is the struct being sent to the js code that
> > results in a call to updatePrintPreview(). We need to inject the request ID
in
> > that struct. That is why I had to add preview_request_id in 2 places in
> > chrome/common/print_messages.h.
> 
> We just called UpdatePrintSettings(), which should have the most recent
request
> id in print_pages_params_, right? What I'm suggesting is to just assign it
> directly:
> 
> preview_params.preview_request_id =
> print_pages_params_.params.preview_request_id;

Done, you are right, I confused the order of execution.

[Lei Zhang, 2011-07-13 00:24:08]

LGTM

[dpapad, 2011-07-13 15:59:00]

Fixed unit tests failures and resolved conflicts with
http://codereview.chromium.org/7038028.

[Lei Zhang, 2011-07-13 16:06:14]

LGTM still.

[commit-bot: I haz the power, 2011-07-13 20:16:27]

Change committed as 92402