PrintPreview: Hide/Show the header footer option based on print frame margins.

PrintPreview: Hide/Show the header footer option based on print frame margins.

BUG=104080
TEST= Please refer to bug report.


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

[kmadhusu, 2012-03-14 20:33:03]

dpapad: Please review js code.
vandebo: Please review c++ code.

Thanks.

[dpapad, 2012-03-14 21:15:17]

http://codereview.chromium.org/9699040/diff/1/chrome/browser/resources/print_...
File chrome/browser/resources/print_preview/header_footer_settings.js (left):

http://codereview.chromium.org/9699040/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/header_footer_settings.js:62:
this.headerFooterApplies_ = event.selectedMargins !=
Can you explain what happens now when NO_MARGINS is selected? Are you handling
this in the c++ side now?

http://codereview.chromium.org/9699040/diff/1/chrome/browser/resources/print_...
File chrome/browser/resources/print_preview/header_footer_settings.js (right):

http://codereview.chromium.org/9699040/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/header_footer_settings.js:65:
this.headerFooterApplies_ = headerFooterApplies;
This member variable seems to only be used here. Can't it just be a local var?

http://codereview.chromium.org/9699040/diff/1/chrome/test/data/webui/print_pr...
File chrome/test/data/webui/print_preview.js (right):

http://codereview.chromium.org/9699040/diff/1/chrome/test/data/webui/print_pr...
chrome/test/data/webui/print_preview.js:274: // Case 1: Zero margin values. Hide
header footer option.
Maybe these cases should be broken to independent tests.

[kmadhusu, 2012-03-14 22:10:14]

dpapad: Addressed review comments.

Thanks.

http://codereview.chromium.org/9699040/diff/1/chrome/browser/resources/print_...
File chrome/browser/resources/print_preview/header_footer_settings.js (left):

http://codereview.chromium.org/9699040/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/header_footer_settings.js:57:
document.addEventListener(customEvents.MARGINS_SELECTION_CHANGED,
This event is no longer required. So removed the corresponding code from
margin_settings.js

http://codereview.chromium.org/9699040/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/header_footer_settings.js:62:
this.headerFooterApplies_ = event.selectedMargins !=
On 2012/03/14 21:15:17, dpapad wrote:
> Can you explain what happens now when NO_MARGINS is selected? Are you handling
> this in the c++ side now?

Yes. It is handled on C++ side. When NO_MARGINS is selected, we always hide the
header footer option.

http://codereview.chromium.org/9699040/diff/1/chrome/browser/resources/print_...
File chrome/browser/resources/print_preview/header_footer_settings.js (right):

http://codereview.chromium.org/9699040/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/header_footer_settings.js:65:
this.headerFooterApplies_ = headerFooterApplies;
On 2012/03/14 21:15:17, dpapad wrote:
> This member variable seems to only be used here. Can't it just be a local var?

oops... Fixed. Removed the member variable.

http://codereview.chromium.org/9699040/diff/1/chrome/test/data/webui/print_pr...
File chrome/test/data/webui/print_preview.js (right):

http://codereview.chromium.org/9699040/diff/1/chrome/test/data/webui/print_pr...
chrome/test/data/webui/print_preview.js:274: // Case 1: Zero margin values. Hide
header footer option.
On 2012/03/14 21:15:17, dpapad wrote:
> Maybe these cases should be broken to independent tests.

sure. Done.

[dpapad, 2012-03-14 23:16:24]

http://codereview.chromium.org/9699040/diff/3004/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/header_footer_settings.js (right):

http://codereview.chromium.org/9699040/diff/3004/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/header_footer_settings.js:63:
setVisibility: function(headerFooterApplies) {
There is already a method setVisible_ in this class. Just make it public and use
it directly. setVisibility() is not needed.

[kmadhusu, 2012-03-15 00:19:14]

http://codereview.chromium.org/9699040/diff/3004/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/header_footer_settings.js (right):

http://codereview.chromium.org/9699040/diff/3004/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/header_footer_settings.js:63:
setVisibility: function(headerFooterApplies) {
On 2012/03/14 23:16:25, dpapad wrote:
> There is already a method setVisible_ in this class. Just make it public and
use
> it directly. setVisibility() is not needed.

Done.

[dpapad, 2012-03-15 17:01:17]

http://codereview.chromium.org/9699040/diff/3007/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/header_footer_settings.js (right):

http://codereview.chromium.org/9699040/diff/3007/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/header_footer_settings.js:73:
this.setVisible_(false);
This is throwing an error.

s/setVisible_/setVisible

[kmadhusu, 2012-03-15 18:46:24]

http://codereview.chromium.org/9699040/diff/3007/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/header_footer_settings.js (right):

http://codereview.chromium.org/9699040/diff/3007/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/header_footer_settings.js:73:
this.setVisible_(false);
On 2012/03/15 17:01:18, dpapad wrote:
> This is throwing an error.
> 
> s/setVisible_/setVisible

Fixed.

[dpapad, 2012-03-15 21:18:50]

On 2012/03/15 18:46:24, kmadhusu wrote:
>
http://codereview.chromium.org/9699040/diff/3007/chrome/browser/resources/pri...
> File chrome/browser/resources/print_preview/header_footer_settings.js (right):
> 
>
http://codereview.chromium.org/9699040/diff/3007/chrome/browser/resources/pri...
> chrome/browser/resources/print_preview/header_footer_settings.js:73:
> this.setVisible_(false);
> On 2012/03/15 17:01:18, dpapad wrote:
> > This is throwing an error.
> > 
> > s/setVisible_/setVisible
> 
> Fixed.

LGTM for js.

[vandebo (ex-Chrome), 2012-03-15 21:47:17]

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

http://codereview.chromium.org/9699040/diff/2017/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:889: bool header_footer_applies = true;
It seems like this is solely concerned with the presentation of the print
preview and therefore should live entirely in the javascript if possible.   From
our discussion, that means sending the printable are to javascript, which seems
reasonable.

[kmadhusu, 2012-03-17 00:29:12]

Addressed review comments.

Thanks.

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

http://codereview.chromium.org/9699040/diff/2017/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:889: bool header_footer_applies = true;
On 2012/03/15 21:47:17, vandebo wrote:
> It seems like this is solely concerned with the presentation of the print
> preview and therefore should live entirely in the javascript if possible.  
From
> our discussion, that means sending the printable are to javascript, which
seems
> reasonable.

Done.

[dpapad, 2012-03-17 00:36:41]

http://codereview.chromium.org/9699040/diff/11001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/header_footer_settings.js (right):

http://codereview.chromium.org/9699040/diff/11001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/header_footer_settings.js:50: * @param
{printing::PageSizeMargins} pageLayout The default layout of the
Fix comment.

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

http://codereview.chromium.org/9699040/diff/11001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:876: function
onDidGetDefaultPageLayout(pageLayout,
Since the logic for hiding showing the header and footer was moved back to js,
you should restore the event at line 129 (and the logic associated with this
event). As it is now, the header/footer checkbox will only get updated when the
c++ side sends the layout, whereas before it was updated immediately when the
user changed the margins.

[vandebo (ex-Chrome), 2012-03-19 17:11:30]

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

http://codereview.chromium.org/9699040/diff/11001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:27: 
#include "ui/gfx/rect.h"

http://codereview.chromium.org/9699040/diff/11001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right):

http://codereview.chromium.org/9699040/diff/11001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_ui.cc:228:
base::DictionaryValue printable_area_info;
Seems like you could put this in the layout dictionary.  Is there any advantage
to putting it in a separate dictionary?

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

http://codereview.chromium.org/9699040/diff/11001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:37: #include "ui/base/l10n/l10n_util.h"
#include "ui/gfx/rect.h"

http://codereview.chromium.org/9699040/diff/11001/printing/print_job_constant...
File printing/print_job_constants.cc (right):

http://codereview.chromium.org/9699040/diff/11001/printing/print_job_constant...
printing/print_job_constants.cc:117: const char kSettingPrintableAreaX[] = "x";
Seems like these constants should say what they are i.e. x -> printableAreaX

[kmadhusu, 2012-03-19 18:45:31]

Addressed review comments.

Thanks.

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

http://codereview.chromium.org/9699040/diff/11001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:27: 
On 2012/03/19 17:11:30, vandebo wrote:
> #include "ui/gfx/rect.h"

(repeating our conversation) Header file inclusion is not required here since we
are just passing the reference around.

http://codereview.chromium.org/9699040/diff/11001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/header_footer_settings.js (right):

http://codereview.chromium.org/9699040/diff/11001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/header_footer_settings.js:50: * @param
{printing::PageSizeMargins} pageLayout The default layout of the
On 2012/03/17 00:36:41, dpapad wrote:
> Fix comment.

Done.

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

http://codereview.chromium.org/9699040/diff/11001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:876: function
onDidGetDefaultPageLayout(pageLayout,
On 2012/03/17 00:36:41, dpapad wrote:
> Since the logic for hiding showing the header and footer was moved back to js,
> you should restore the event at line 129 (and the logic associated with this
> event). As it is now, the header/footer checkbox will only get updated when
the
> c++ side sends the layout, whereas before it was updated immediately when the
> user changed the margins.

As per our discussion, I am going to leave this as it is. Because the time gap
between the margin selection and the DefaultPageLayout event is very small. So
it should be fine.

http://codereview.chromium.org/9699040/diff/11001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right):

http://codereview.chromium.org/9699040/diff/11001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview/print_preview_ui.cc:228:
base::DictionaryValue printable_area_info;
On 2012/03/19 17:11:30, vandebo wrote:
> Seems like you could put this in the layout dictionary.  Is there any
advantage
> to putting it in a separate dictionary?

Added printable area details in page layout dictionary.

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

http://codereview.chromium.org/9699040/diff/11001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:37: #include "ui/base/l10n/l10n_util.h"
On 2012/03/19 17:11:30, vandebo wrote:
> #include "ui/gfx/rect.h"

Done.

http://codereview.chromium.org/9699040/diff/11001/printing/print_job_constant...
File printing/print_job_constants.cc (right):

http://codereview.chromium.org/9699040/diff/11001/printing/print_job_constant...
printing/print_job_constants.cc:117: const char kSettingPrintableAreaX[] = "x";
On 2012/03/19 17:11:30, vandebo wrote:
> Seems like these constants should say what they are i.e. x -> printableAreaX

Done

[dpapad, 2012-03-19 19:50:53]

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

http://codereview.chromium.org/9699040/diff/11001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:876: function
onDidGetDefaultPageLayout(pageLayout,
On 2012/03/19 18:45:31, kmadhusu wrote:
> On 2012/03/17 00:36:41, dpapad wrote:
> > Since the logic for hiding showing the header and footer was moved back to
js,
> > you should restore the event at line 129 (and the logic associated with this
> > event). As it is now, the header/footer checkbox will only get updated when
> the
> > c++ side sends the layout, whereas before it was updated immediately when
the
> > user changed the margins.
> 
> As per our discussion, I am going to leave this as it is. Because the time gap
> between the margin selection and the DefaultPageLayout event is very small. So
> it should be fine.

SG. Lets monitor the tracker for any complaints about this. I understand the
reasons for leaving it as is, but i don't think that the time gap is that small,
it can get to a few secs for a large doc while using a slow machine.

[vandebo (ex-Chrome), 2012-03-19 22:29:04]

C++ LGTM

[commit-bot: I haz the power, 2012-03-20 00:21:51]

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

[commit-bot: I haz the power, 2012-03-20 03:10:23]

Change committed as 127631