[Print Preview]: Added code to support pdf fit to page functionality.

[Print Preview]: Added code to support pdf fit to page functionality.

BUG=85132
TEST=none


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

[kmadhusu, 2012-04-19 03:14:13]

As per our discussion, I have modified the workflow such that the initiator
renderer will create a transformed preview document as per the specified print
settings.

You can find the previous review comments at codereview.chromium.org/8968033.

thestig: Please review the c++ code changes.
dpapad: Please review the js code. 

Thanks.

[kmadhusu, 2012-04-19 03:20:31]

You can find the ppapi CL @ http://codereview.chromium.org/10083059/

and Webkit CL @ https://bugs.webkit.org/show_bug.cgi?id=84312

[kmadhusu, 2012-04-19 18:35:40]

Currently working on adding new tests for the fit to page option. Will let you
know once the patch is ready for review.

Thanks.

[dpapad, 2012-04-19 18:38:39]

Should we cc rltoscano@chromium? It seems that this change will affect his
redesign of the code.

http://codereview.chromium.org/10083060/diff/1001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/fit_to_page_settings.js (right):

http://codereview.chromium.org/10083060/diff/1001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/fit_to_page_settings.js:106: setVisible_:
function(visible) {
This is always invoked with this.fitToPageApplies_, why bother passing it as an
argument? Just access it directly and rename this method to updateVisibility or
some other better name.

http://codereview.chromium.org/10083060/diff/1001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/more_options.js (right):

http://codereview.chromium.org/10083060/diff/1001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/more_options.js:82:
updateMoreOptionsVisibility_: function() {
rename to updateVisibility? MoreOptions seems redundant.

[kmadhusu, 2012-04-19 20:44:54]

Added WebUI tests and addressed review comments. 

PTAL.

Thanks.

http://codereview.chromium.org/10083060/diff/1001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/fit_to_page_settings.js (right):

http://codereview.chromium.org/10083060/diff/1001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/fit_to_page_settings.js:106: setVisible_:
function(visible) {
On 2012/04/19 18:38:39, dpapad wrote:
> This is always invoked with this.fitToPageApplies_, why bother passing it as
an
> argument? Just access it directly and rename this method to updateVisibility
or
> some other better name.

setVisible_ -> updateVisibility. Removed the param.

http://codereview.chromium.org/10083060/diff/1001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/more_options.js (right):

http://codereview.chromium.org/10083060/diff/1001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/more_options.js:82:
updateMoreOptionsVisibility_: function() {
On 2012/04/19 18:38:39, dpapad wrote:
> rename to updateVisibility? MoreOptions seems redundant.

Done.

[dpapad, 2012-04-19 21:11:29]

Mostly nits.

http://codereview.chromium.org/10083060/diff/7001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/fit_to_page_settings.js (right):

http://codereview.chromium.org/10083060/diff/7001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/fit_to_page_settings.js:56:
printScalingDisabledUpdateState: function() {
Nit:
The naming of this function describes why it is called, not what it is doing,
this seems a bit weird. I would just name this function
uncheckFitToPageCheckbox().

Alternatively (i think this is better than previous suggestion) I would name it
onPrintScalingDisabled().

http://codereview.chromium.org/10083060/diff/7001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/fit_to_page_settings.js:107: 'none';
Nit: I believe it is more readable as follows.

this.fitToPageOption_.style.display =
    this.fitToPageApplies_ ? 'block' : 'none';

http://codereview.chromium.org/10083060/diff/7001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/more_options.js (right):

http://codereview.chromium.org/10083060/diff/7001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/more_options.js:18:
this.hideHeaderFooterOption_ = true;
Maybe a better name would be headerFooterOptionHidden. Starting the name with a
verb makes it sound like a function name. Same for hideFitToPageOption_.

Edit: I see that you are using those names because you are hiding those at a
later stage, in that case leave the names as they are but update the comments
"True if fit to page option should be hidden." or something like that.

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

http://codereview.chromium.org/10083060/diff/7001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:315: var
selectedPrinterChanged = true;
Why not dispatching the event here, and eliminating the need for
selectedPrinterChanged variable?

http://codereview.chromium.org/10083060/diff/7001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:327:
selectedPrinterChanged = false;
Same here.

http://codereview.chromium.org/10083060/diff/7001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:654: var pageCount =
totalPageCount != undefined ? totalPageCount : -1;
Not related to your changes (optional): I believe this is equivalent to
var pageCount = totalPageCount || -1;
but I would test to make sure.

[kmadhusu, 2012-04-20 23:03:42]

dpapad: Addressed review comments.

PTAL.

Thanks.

http://codereview.chromium.org/10083060/diff/7001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/fit_to_page_settings.js (right):

http://codereview.chromium.org/10083060/diff/7001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/fit_to_page_settings.js:56:
printScalingDisabledUpdateState: function() {
On 2012/04/19 21:11:29, dpapad wrote:
> Nit:
> The naming of this function describes why it is called, not what it is doing,
> this seems a bit weird. I would just name this function
> uncheckFitToPageCheckbox().
> 
> Alternatively (i think this is better than previous suggestion) I would name
it
> onPrintScalingDisabled(). 

+1 for onPrintScalingDisabled().

http://codereview.chromium.org/10083060/diff/7001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/fit_to_page_settings.js:107: 'none';
On 2012/04/19 21:11:29, dpapad wrote:
> Nit: I believe it is more readable as follows.
> 
> this.fitToPageOption_.style.display =
>     this.fitToPageApplies_ ? 'block' : 'none';        

Done.

http://codereview.chromium.org/10083060/diff/7001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/more_options.js (right):

http://codereview.chromium.org/10083060/diff/7001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/more_options.js:18:
this.hideHeaderFooterOption_ = true;
On 2012/04/19 21:11:29, dpapad wrote:
> Maybe a better name would be headerFooterOptionHidden. Starting the name with
a
> verb makes it sound like a function name. Same for hideFitToPageOption_.
> 
> Edit: I see that you are using those names because you are hiding those at a
> later stage, in that case leave the names as they are but update the comments
> "True if fit to page option should be hidden." or something like that.

Modified the comment.

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

http://codereview.chromium.org/10083060/diff/7001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:315: var
selectedPrinterChanged = true;
On 2012/04/19 21:11:29, dpapad wrote:
> Why not dispatching the event here, and eliminating the need for
> selectedPrinterChanged variable?

As we discussed, I am leaving this as it is.

http://codereview.chromium.org/10083060/diff/7001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:327:
selectedPrinterChanged = false;
On 2012/04/19 21:11:29, dpapad wrote:
> Same here.

As we discussed, I am leaving this as it is.

http://codereview.chromium.org/10083060/diff/7001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:654: var pageCount =
totalPageCount != undefined ? totalPageCount : -1;
On 2012/04/19 21:11:29, dpapad wrote:
> Not related to your changes (optional): I believe this is equivalent to
> var pageCount = totalPageCount || -1;
> but I would test to make sure.

Done.

[dpapad, 2012-04-21 01:36:06]

js LGTM

http://codereview.chromium.org/10083060/diff/18009/chrome/test/data/webui/pri...
File chrome/test/data/webui/print_preview.js (right):

http://codereview.chromium.org/10083060/diff/18009/chrome/test/data/webui/pri...
chrome/test/data/webui/print_preview.js:268: assertNotEquals(null, section);
Isn't there a assertNotNull() method?

[kmadhusu, 2012-04-25 20:52:41]

thestig: When you get a chance, please review the latest patch.

Thanks.

[Lei Zhang, 2012-04-26 00:19:51]

http://codereview.chromium.org/10083060/diff/30001/chrome/browser/printing/pr...
File chrome/browser/printing/print_preview_message_handler.cc (right):

http://codereview.chromium.org/10083060/diff/30001/chrome/browser/printing/pr...
chrome/browser/printing/print_preview_message_handler.cc:27: #include
"ui/gfx/rect.h"
nit: not needed since all we do is pass the Rect to another method.

http://codereview.chromium.org/10083060/diff/30001/chrome/browser/printing/pr...
chrome/browser/printing/print_preview_message_handler.cc:219:
TabContentsWrapper* print_preview_tab = GetPrintPreviewTab();
Just rewrite this method as:

PrintPreviewUI* print_preview_ui = GetPrintPreviewUI();
if (!print_preview_ui)
  return;
print_preview_ui->OnPrintPreviewScalingDisabled();

http://codereview.chromium.org/10083060/diff/30001/chrome/common/print_messag...
File chrome/common/print_messages.h (right):

http://codereview.chromium.org/10083060/diff/30001/chrome/common/print_messag...
chrome/common/print_messages.h:420: // Notify the browser that the previewing
pdf has disable print scaling
"previewing pdf" is not clear. Can you say "PDF in the initiator renderer"
instead?

disable -> disabled.

http://codereview.chromium.org/10083060/diff/30001/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (left):

http://codereview.chromium.org/10083060/diff/30001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1313: // - On Windows, we don't add a
margin until we turn it into an EMF when
Is this change related to this CL?

http://codereview.chromium.org/10083060/diff/30001/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/10083060/diff/30001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1379: // TODO(kmadhusu): Update this
code.
I would move this into its own method with a clear explanation of what's going
on.

http://codereview.chromium.org/10083060/diff/30001/printing/print_job_constan...
File printing/print_job_constants.cc (right):

http://codereview.chromium.org/10083060/diff/30001/printing/print_job_constan...
printing/print_job_constants.cc:45: // Key that specifies the fit to page option
value.
Let's not state the obvious. Something like the |kSettingHeaderFooterEnabled|
comment is better.

[kmadhusu, 2012-04-26 22:18:51]

thestig: Addressed review comments.

Thanks.

http://codereview.chromium.org/10083060/diff/30001/chrome/browser/printing/pr...
File chrome/browser/printing/print_preview_message_handler.cc (right):

http://codereview.chromium.org/10083060/diff/30001/chrome/browser/printing/pr...
chrome/browser/printing/print_preview_message_handler.cc:27: #include
"ui/gfx/rect.h"
On 2012/04/26 00:19:51, Lei Zhang wrote:
> nit: not needed since all we do is pass the Rect to another method.

Done.

http://codereview.chromium.org/10083060/diff/30001/chrome/browser/printing/pr...
chrome/browser/printing/print_preview_message_handler.cc:219:
TabContentsWrapper* print_preview_tab = GetPrintPreviewTab();
On 2012/04/26 00:19:51, Lei Zhang wrote:
> Just rewrite this method as:
> 
> PrintPreviewUI* print_preview_ui = GetPrintPreviewUI();
> if (!print_preview_ui)
>   return;
> print_preview_ui->OnPrintPreviewScalingDisabled();

Done.

http://codereview.chromium.org/10083060/diff/30001/chrome/common/print_messag...
File chrome/common/print_messages.h (right):

http://codereview.chromium.org/10083060/diff/30001/chrome/common/print_messag...
chrome/common/print_messages.h:420: // Notify the browser that the previewing
pdf has disable print scaling
On 2012/04/26 00:19:51, Lei Zhang wrote:
> "previewing pdf" is not clear. Can you say "PDF in the initiator renderer"
> instead?
> 
> disable -> disabled.

Done.

http://codereview.chromium.org/10083060/diff/30001/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (left):

http://codereview.chromium.org/10083060/diff/30001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1313: // - On Windows, we don't add a
margin until we turn it into an EMF when
On 2012/04/26 00:19:51, Lei Zhang wrote:
> Is this change related to this CL?

Yes.

http://codereview.chromium.org/10083060/diff/30001/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/10083060/diff/30001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1379: // TODO(kmadhusu): Update this
code.
On 2012/04/26 00:19:51, Lei Zhang wrote:
> I would move this into its own method with a clear explanation of what's going
> on.

Done.

http://codereview.chromium.org/10083060/diff/30001/printing/print_job_constan...
File printing/print_job_constants.cc (right):

http://codereview.chromium.org/10083060/diff/30001/printing/print_job_constan...
printing/print_job_constants.cc:45: // Key that specifies the fit to page option
value.
On 2012/04/26 00:19:51, Lei Zhang wrote:
> Let's not state the obvious. Something like the |kSettingHeaderFooterEnabled|
> comment is better.

Done.

[kmadhusu, 2012-04-26 22:34:36]

http://codereview.chromium.org/10083060/diff/40001/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/10083060/diff/40001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.h:165: void
UpdatePrintParamsFitToPageInfo(WebKit::WebFrame* frame,
Forgot to update this function name in the previous patch.

[Lei Zhang, 2012-04-26 22:52:23]

http://codereview.chromium.org/10083060/diff/52001/chrome/common/print_messag...
File chrome/common/print_messages.h (right):

http://codereview.chromium.org/10083060/diff/52001/chrome/common/print_messag...
chrome/common/print_messages.h:420: // Notify the browser that the PDF in
initiator renderer has disabled print
nit: "in initiator" -> "in the initiator"

http://codereview.chromium.org/10083060/diff/52001/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (left):

http://codereview.chromium.org/10083060/diff/52001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1313: // - On Windows, we don't add a
margin until we turn it into an EMF when
BTW, why are we removing this?

http://codereview.chromium.org/10083060/diff/52001/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/10083060/diff/52001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.h:163: // Updates |settings| fit to page
information based on |job_settings| and
I think this comment can use some improvements to be less vague.

As an example, the following comment is useless. I cannot tell what this
function is suppose to do without reading the implementation. Even then, I don't
know if the implementation is doing what it is designed to do.

// Returns a value based on |a| and |b|.
int MagicalMathFunction(a, b);

Compare with the following. Here, I know exactly what the function does, and I
can even verify the implementation is correct.

// Returns the largest prime number between |a| and |b|.
int MagicalMathFunction(a, b);

[Lei Zhang, 2012-04-26 22:57:53]

http://codereview.chromium.org/10083060/diff/52001/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/10083060/diff/52001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:848: bool source_is_html = true;
This first bit of logic is the same as
PrintWebViewHelper::UpdatePrintSettings(). Can you split this off into its own
function?

http://codereview.chromium.org/10083060/diff/52001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:855: source_is_html =
!PrintingNodeOrFrame(frame, node);
I don't think this will compile.

[kmadhusu, 2012-04-27 01:28:58]

http://codereview.chromium.org/10083060/diff/52001/chrome/common/print_messag...
File chrome/common/print_messages.h (right):

http://codereview.chromium.org/10083060/diff/52001/chrome/common/print_messag...
chrome/common/print_messages.h:420: // Notify the browser that the PDF in
initiator renderer has disabled print
On 2012/04/26 22:52:23, Lei Zhang wrote:
> nit: "in initiator" -> "in the initiator"

Done.

http://codereview.chromium.org/10083060/diff/52001/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (left):

http://codereview.chromium.org/10083060/diff/52001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1313: // - On Windows, we don't add a
margin until we turn it into an EMF when
On 2012/04/26 22:52:23, Lei Zhang wrote:
> BTW, why are we removing this?

While printing pdf for print preview, we have already taken the print scaling
option of the plugin into consideration. Therefore, we don't need to set the
margins type explicitly.

http://codereview.chromium.org/10083060/diff/52001/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/10083060/diff/52001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:848: bool source_is_html = true;
On 2012/04/26 22:57:53, Lei Zhang wrote:
> This first bit of logic is the same as
> PrintWebViewHelper::UpdatePrintSettings(). Can you split this off into its own
> function?

Done.

http://codereview.chromium.org/10083060/diff/52001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:855: source_is_html =
!PrintingNodeOrFrame(frame, node);
On 2012/04/26 22:57:53, Lei Zhang wrote:
> I don't think this will compile.

oops.. My commit is messed up while resolving a conflict. Fixed now.

http://codereview.chromium.org/10083060/diff/52001/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/10083060/diff/52001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.h:163: // Updates |settings| fit to page
information based on |job_settings| and
On 2012/04/26 22:52:23, Lei Zhang wrote:
> I think this comment can use some improvements to be less vague.
> 
> As an example, the following comment is useless. I cannot tell what this
> function is suppose to do without reading the implementation. Even then, I
don't
> know if the implementation is doing what it is designed to do.
> 
> // Returns a value based on |a| and |b|.
> int MagicalMathFunction(a, b);
> 
> Compare with the following. Here, I know exactly what the function does, and I
> can even verify the implementation is correct.
> 
> // Returns the largest prime number between |a| and |b|.
> int MagicalMathFunction(a, b);

As we discussed, I have listed the details and changed the function signature to
return a bool.

[Lei Zhang, 2012-04-27 04:32:46]

http://codereview.chromium.org/10083060/diff/52001/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (left):

http://codereview.chromium.org/10083060/diff/52001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1313: // - On Windows, we don't add a
margin until we turn it into an EMF when
On 2012/04/27 01:28:58, kmadhusu wrote:
> On 2012/04/26 22:52:23, Lei Zhang wrote:
> > BTW, why are we removing this?
> 
> While printing pdf for print preview, we have already taken the print scaling
> option of the plugin into consideration. Therefore, we don't need to set the
> margins type explicitly.

Ok, can you also get rid of |margin_type| then?

http://codereview.chromium.org/10083060/diff/54002/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/10083060/diff/54002/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:847: if (print_for_preview_ ||
params.print_to_pdf) {
Do you need to check |print_for_preview_|? Right now, you only call
IsFitToPaperSizeRequested() when |print_for_preview_| is false.

In fact, if you are going to use |print_preview_context_| below, you should
DCHECK(!print_for_preview); and mention this is only used for print preview.

http://codereview.chromium.org/10083060/diff/54002/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:870: !FitToPageEnabled(job_settings)) {
You can check the result of FitToPageEnabled() first, which is relatively cheap,
and return false without calling isPrintScalingDisabledForPlugin() above.

http://codereview.chromium.org/10083060/diff/54002/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1364: bool source_is_html =
IsSourceHTML(frame, node, *job_settings);
It occurred to me that you can pass this to IsFitToPaperSizeRequested() below,
so it won't have to compute it again. If you do that, then you don't have to
split IsSourceHTML() into a separate function.

[kmadhusu, 2012-05-01 16:43:14]

thestig: Addressed review comments.

Thanks.

http://codereview.chromium.org/10083060/diff/52001/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (left):

http://codereview.chromium.org/10083060/diff/52001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1313: // - On Windows, we don't add a
margin until we turn it into an EMF when
On 2012/04/27 04:32:47, Lei Zhang wrote:
> On 2012/04/27 01:28:58, kmadhusu wrote:
> > On 2012/04/26 22:52:23, Lei Zhang wrote:
> > > BTW, why are we removing this?
> > 
> > While printing pdf for print preview, we have already taken the print
scaling
> > option of the plugin into consideration. Therefore, we don't need to set the
> > margins type explicitly.
> 
> Ok, can you also get rid of |margin_type| then?

Done.

http://codereview.chromium.org/10083060/diff/54002/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/10083060/diff/54002/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:847: if (print_for_preview_ ||
params.print_to_pdf) {
On 2012/04/27 04:32:47, Lei Zhang wrote:
> Do you need to check |print_for_preview_|? Right now, you only call
> IsFitToPaperSizeRequested() when |print_for_preview_| is false.
> 
> In fact, if you are going to use |print_preview_context_| below, you should
> DCHECK(!print_for_preview); and mention this is only used for print preview.

Changed the default value of |fit_to_paper_size| to false in
chrome/common/print_messages.cc (Ref: Patch set 11). We override that value
during preview workflow and normal printing workflow.

http://codereview.chromium.org/10083060/diff/54002/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:870: !FitToPageEnabled(job_settings)) {
On 2012/04/27 04:32:47, Lei Zhang wrote:
> You can check the result of FitToPageEnabled() first, which is relatively
cheap,
> and return false without calling isPrintScalingDisabledForPlugin() above.

Done.

http://codereview.chromium.org/10083060/diff/54002/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1364: bool source_is_html =
IsSourceHTML(frame, node, *job_settings);
On 2012/04/27 04:32:47, Lei Zhang wrote:
> It occurred to me that you can pass this to IsFitToPaperSizeRequested() below,
> so it won't have to compute it again. If you do that, then you don't have to
> split IsSourceHTML() into a separate function.

Done.

[Lei Zhang, 2012-05-02 06:42:36]

lgtm

http://codereview.chromium.org/10083060/diff/64005/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/10083060/diff/64005/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:913:
print_pages_params_->params.is_first_request &&
nit: if you check this first, it's a much cheaper check and you can potentially
short-circuit the rest of the evaluation.

[kmadhusu, 2012-05-06 20:10:14]

http://codereview.chromium.org/10083060/diff/64005/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/10083060/diff/64005/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:913:
print_pages_params_->params.is_first_request &&
On 2012/05/02 06:42:36, Lei Zhang wrote:
> nit: if you check this first, it's a much cheaper check and you can
potentially
> short-circuit the rest of the evaluation.

Done.

http://codereview.chromium.org/10083060/diff/75002/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/10083060/diff/75002/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1465:
print_pages_params_->params.fit_to_paper_size = fit_to_paper_size;
While testing the native printing workflow, I found that the new code failed to
honor the html print css, that's because we are getting a new print settings
(with fit_to_paper_size value set to false) from the print dialog. So I need to
restore the calculated fit to paper size value in the new print params.

[Lei Zhang, 2012-05-07 18:54:05]

http://codereview.chromium.org/10083060/diff/75002/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/10083060/diff/75002/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1465:
print_pages_params_->params.fit_to_paper_size = fit_to_paper_size;
On 2012/05/06 20:10:15, kmadhusu wrote:
> While testing the native printing workflow, I found that the new code failed
to
> honor the html print css, that's because we are getting a new print settings
> (with fit_to_paper_size value set to false) from the print dialog. So I need
to
> restore the calculated fit to paper size value in the new print params.

You may want to add a comment here to explain that.

[kmadhusu, 2012-05-16 18:45:11]

thestig: Fixed nit. CL is ready to commit. I am going to check the CQ checkbox.

https://chromiumcodereview.appspot.com/10083060/diff/75002/chrome/renderer/pr...
File chrome/renderer/print_web_view_helper.cc (right):

https://chromiumcodereview.appspot.com/10083060/diff/75002/chrome/renderer/pr...
chrome/renderer/print_web_view_helper.cc:1465:
print_pages_params_->params.fit_to_paper_size = fit_to_paper_size;
On 2012/05/07 18:54:05, Lei Zhang wrote:
> On 2012/05/06 20:10:15, kmadhusu wrote:
> > While testing the native printing workflow, I found that the new code failed
> to
> > honor the html print css, that's because we are getting a new print settings
> > (with fit_to_paper_size value set to false) from the print dialog. So I need
> to
> > restore the calculated fit to paper size value in the new print params.
> 
> You may want to add a comment here to explain that.

Done.

[commit-bot: I haz the power, 2012-05-16 18:49:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/10083060/89004

[commit-bot: I haz the power, 2012-05-16 20:32:17]

Change committed as 137498