Fix issue in print preview where selecting a cloud printer does not generate

Fix issue in print preview where selecting a cloud printer does not generate a new preview.

Adds handling of the request ID to PrintWebViewHelper::UpdatePrintSettingsCloud


BUG=http://code.google.com/p/chromium-os/issues/detail?id=16082
TEST=Run chrome with --enable-cloud-printers and --enable-print-preview.
Open a web page and hit ctrl-p to open print preview
Select a cloud printer from the dropdown.
You should get a preview.


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

[dpapad, 2011-07-20 00:54:11]

LGTM. Two questions again.

1) I get only the first page back. Is that under construction?
2) Copies settings are hidden for the cloud printers, is that intended?

http://codereview.chromium.org/7461014/diff/1/chrome/renderer/print_web_view_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7461014/diff/1/chrome/renderer/print_web_view_...
chrome/renderer/print_web_view_helper.cc:709: return false;
I think a return statement after a NOTREACHED() is not needed (in debug builds
at least). I know I put it there, I just noticed it.

http://codereview.chromium.org/7461014/diff/1/chrome/renderer/print_web_view_...
chrome/renderer/print_web_view_helper.cc:749: if
(!UpdatePrintSettingsRequestId(job_settings, &(settings.params)))
I believe that operator . has higher precedence than &, so parenthesis are not
required.

[Albert Bodenhamer, 2011-07-20 01:12:40]

On Tue, Jul 19, 2011 at 5:54 PM, <dpapad@chromium.org> wrote:

> LGTM. Two questions again.
>
> 1) I get only the first page back. Is that under construction?
>

Odd.  I'll look into that.  Thanks for checking.  I've just been testing
with google.com and the new tab page, so I suspect it's an unrelated bug.


> 2) Copies settings are hidden for the cloud printers, is that intended?
>
>
That's intended.  Long story, but it's not an option we can support right
now.


>
> http://codereview.chromium.**org/7461014/diff/1/chrome/**
>
renderer/print_web_view_**helper.cc<http://codereview.chromium.org/7461014/diff/1/chrome/renderer/print_web_view_helper.cc>
> File chrome/renderer/print_web_**view_helper.cc (right):
>
> http://codereview.chromium.**org/7461014/diff/1/chrome/**
>
renderer/print_web_view_**helper.cc#newcode709<http://codereview.chromium.org/7461014/diff/1/chrome/renderer/print_web_view_helper.cc#newcode709>
> chrome/renderer/print_web_**view_helper.cc:709: return false;
> I think a return statement after a NOTREACHED() is not needed (in debug
> builds at least). I know I put it there, I just noticed it.
>

I think it makes the code easier to read, but if it's generally not used
I'll remove it for consistency.


>
> http://codereview.chromium.**org/7461014/diff/1/chrome/**
>
renderer/print_web_view_**helper.cc#newcode749<http://codereview.chromium.org/7461014/diff/1/chrome/renderer/print_web_view_helper.cc#newcode749>
> chrome/renderer/print_web_**view_helper.cc:749: if
>
> (!**UpdatePrintSettingsRequestId(**job_settings, &(settings.params)))
> I believe that operator . has higher precedence than &, so parenthesis
> are not required.
>
>
Not required, but most readers haven't memorized the precedence tables that
well.  Adding the parens avoids causing people to stop and think about it
(or worse, have to look it up) and it doesn't hurt anything.


>
>
http://codereview.chromium.**org/7461014/<http://codereview.chromium.org/7461...
>


Thanks.

-- 
Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com>
org

[commit-bot: I haz the power, 2011-07-20 17:36:17]

Change committed as 93215