Move margin processing code to the browser process.

Move margin processing code to the browser process.

It seems that this is where it is supposed to live and it was erroneously added to PrintWebViewHelper.

BUG=67091, 92045, 91880, 92000, 92218, 95905
TEST=NONE


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

[vandebo (ex-Chrome), 2011-10-08 01:27:24]

I need to verify this work on Mac, Windows and Linux native flow still, but I
don't expect it to change much.

http://codereview.chromium.org/8201027/diff/1/chrome/browser/printing/printin...
File chrome/browser/printing/printing_message_filter.cc (left):

http://codereview.chromium.org/8201027/diff/1/chrome/browser/printing/printin...
chrome/browser/printing/printing_message_filter.cc:50: params->margin_top =
settings.page_setup_device_units().content_area().x();
This is a long outstanding bug, that apparently hasn't mattered much.

[Lei Zhang, 2011-10-08 02:02:04]

http://codereview.chromium.org/8201027/diff/1/chrome/browser/printing/print_d...
File chrome/browser/printing/print_dialog_gtk.h (right):

http://codereview.chromium.org/8201027/diff/1/chrome/browser/printing/print_d...
chrome/browser/printing/print_dialog_gtk.h:70: // set of |page_ranges|.
nit: update comment?

http://codereview.chromium.org/8201027/diff/1/printing/print_dialog_gtk_inter...
File printing/print_dialog_gtk_interface.h (right):

http://codereview.chromium.org/8201027/diff/1/printing/print_dialog_gtk_inter...
printing/print_dialog_gtk_interface.h:24: // Update the dialog to use |settings|
and |ranges|, where |settings| is a
nit: Comments are confusing now.

http://codereview.chromium.org/8201027/diff/1/printing/printing_context.cc
File printing/printing_context.cc (right):

http://codereview.chromium.org/8201027/diff/1/printing/printing_context.cc#ne...
printing/printing_context.cc:41: ResetSettings();
I see you removed a ResetSettings() call in printing_context_win.cc. Is that
really moving it here. I don't know off the top of my head if this is right.

http://codereview.chromium.org/8201027/diff/1/printing/printing_context.cc#ne...
printing/printing_context.cc:42: if
(!job_settings.GetBoolean(printing::kSettingHeaderFooterEnabled,
this block is not needed. It's already in
PrintSettingsInitializer::InitHeaderFooterStrings.

http://codereview.chromium.org/8201027/diff/1/printing/printing_context.cc#ne...
printing/printing_context.cc:78: empty.Clear();
Not needed. Better yet, let's add an is_empty method to PageMargins?

http://codereview.chromium.org/8201027/diff/1/printing/printing_context.h
File printing/printing_context.h (right):

http://codereview.chromium.org/8201027/diff/1/printing/printing_context.h#new...
printing/printing_context.h:103: DCHECK(type != CUSTOM_MARGINS);
Clang probably won't like this. Move into .cc file?

[vandebo (ex-Chrome), 2011-10-15 00:42:50]

All the deps are in and this is now tested on all platforms.

http://codereview.chromium.org/8201027/diff/1/chrome/browser/printing/print_d...
File chrome/browser/printing/print_dialog_gtk.h (right):

http://codereview.chromium.org/8201027/diff/1/chrome/browser/printing/print_d...
chrome/browser/printing/print_dialog_gtk.h:70: // set of |page_ranges|.
On 2011/10/08 02:02:05, Lei Zhang wrote:
> nit: update comment?

Done.

http://codereview.chromium.org/8201027/diff/1/printing/print_dialog_gtk_inter...
File printing/print_dialog_gtk_interface.h (right):

http://codereview.chromium.org/8201027/diff/1/printing/print_dialog_gtk_inter...
printing/print_dialog_gtk_interface.h:24: // Update the dialog to use |settings|
and |ranges|, where |settings| is a
On 2011/10/08 02:02:05, Lei Zhang wrote:
> nit: Comments are confusing now.

Done.

http://codereview.chromium.org/8201027/diff/1/printing/printing_context.cc
File printing/printing_context.cc (right):

http://codereview.chromium.org/8201027/diff/1/printing/printing_context.cc#ne...
printing/printing_context.cc:41: ResetSettings();
On 2011/10/08 02:02:05, Lei Zhang wrote:
> I see you removed a ResetSettings() call in printing_context_win.cc. Is that
> really moving it here. I don't know off the top of my head if this is right.

The previous ResetSettings happened after I set these settings, so yes I moved
it up in the flow.  Linux used to fill out a stack local copy and then copy it
in instead of doing Reset.

http://codereview.chromium.org/8201027/diff/1/printing/printing_context.cc#ne...
printing/printing_context.cc:42: if
(!job_settings.GetBoolean(printing::kSettingHeaderFooterEnabled,
On 2011/10/08 02:02:05, Lei Zhang wrote:
> this block is not needed. It's already in
> PrintSettingsInitializer::InitHeaderFooterStrings.

The value needs to be set for PrintSettings::SetPrinterPrintableAream which is
called from UpdatePrinterSettings, but I can't just swap the order of it and
InitHeaderFooterStrings because that depends on settings set in
UpdatePrinterSettings.  I could remove it from InitHeaderFooterStrings, but
leaving it in makes that method seem more complete.

http://codereview.chromium.org/8201027/diff/1/printing/printing_context.cc#ne...
printing/printing_context.cc:78: empty.Clear();
On 2011/10/08 02:02:05, Lei Zhang wrote:
> Not needed. Better yet, let's add an is_empty method to PageMargins?

Fixed otherwise.

http://codereview.chromium.org/8201027/diff/1/printing/printing_context.h
File printing/printing_context.h (right):

http://codereview.chromium.org/8201027/diff/1/printing/printing_context.h#new...
printing/printing_context.h:103: DCHECK(type != CUSTOM_MARGINS);
On 2011/10/08 02:02:05, Lei Zhang wrote:
> Clang probably won't like this. Move into .cc file?

Done.

http://codereview.chromium.org/8201027/diff/17001/chrome/browser/printing/pri...
File chrome/browser/printing/printing_message_filter.cc (right):

http://codereview.chromium.org/8201027/diff/17001/chrome/browser/printing/pri...
chrome/browser/printing/printing_message_filter.cc:50: params->margin_top =
settings.page_setup_device_units().content_area().y();
This is a pretty old bug.

[Lei Zhang, 2011-10-15 03:42:19]

LGTM with a bunch of nits

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

http://codereview.chromium.org/8201027/diff/21001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:156: // TODO(vandebo) When it's plumbed
through, check if the plugin wants us to
obsolete comment?

http://codereview.chromium.org/8201027/diff/21001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1048: const DictionaryValue&
passed_job_settings, bool generating_preview) {
nit: |passed_job_settings| does not match parameter name in the .h file.

http://codereview.chromium.org/8201027/diff/21001/printing/page_setup.h
File printing/page_setup.h (right):

http://codereview.chromium.org/8201027/diff/21001/printing/page_setup.h#newco...
printing/page_setup.h:68: // a contstraint of |printable_area|
nit: contstraint -> constraint, printable_area -> bounds

http://codereview.chromium.org/8201027/diff/21001/printing/print_settings.cc
File printing/print_settings.cc (right):

http://codereview.chromium.org/8201027/diff/21001/printing/print_settings.cc#...
printing/print_settings.cc:205:
page_setup_device_units_.ForceRequestedMargins(margins);
why does custom margins get to go outside of the printable area?

http://codereview.chromium.org/8201027/diff/21001/printing/printing_context.cc
File printing/printing_context.cc (right):

http://codereview.chromium.org/8201027/diff/21001/printing/printing_context.c...
printing/printing_context.cc:55: (margin_type != DEFAULT_MARGINS &&
Maybe in printing/print_job_constants.h make sure one type of margins is always
first and one is always last. Then you can do:

if (margin_type < first || margin_type > last)

http://codereview.chromium.org/8201027/diff/21001/printing/printing_context.h
File printing/printing_context.h (right):

http://codereview.chromium.org/8201027/diff/21001/printing/printing_context.h...
printing/printing_context.h:12: #include "base/logging.h"
nit: not needed?

[commit-bot: I haz the power, 2011-10-15 20:51:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vandebo@chromium.org/8201027/25001

[vandebo (ex-Chrome), 2011-10-15 20:52:08]

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

http://codereview.chromium.org/8201027/diff/21001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:156: // TODO(vandebo) When it's plumbed
through, check if the plugin wants us to
On 2011/10/15 03:42:19, Lei Zhang wrote:
> obsolete comment?

I'm going to do this in a follow up CL.

http://codereview.chromium.org/8201027/diff/21001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1048: const DictionaryValue&
passed_job_settings, bool generating_preview) {
On 2011/10/15 03:42:19, Lei Zhang wrote:
> nit: |passed_job_settings| does not match parameter name in the .h file.

Done.

http://codereview.chromium.org/8201027/diff/21001/printing/page_setup.h
File printing/page_setup.h (right):

http://codereview.chromium.org/8201027/diff/21001/printing/page_setup.h#newco...
printing/page_setup.h:68: // a contstraint of |printable_area|
On 2011/10/15 03:42:19, Lei Zhang wrote:
> nit: contstraint -> constraint, printable_area -> bounds

Done.

http://codereview.chromium.org/8201027/diff/21001/printing/print_settings.cc
File printing/print_settings.cc (right):

http://codereview.chromium.org/8201027/diff/21001/printing/print_settings.cc#...
printing/print_settings.cc:205:
page_setup_device_units_.ForceRequestedMargins(margins);
On 2011/10/15 03:42:19, Lei Zhang wrote:
> why does custom margins get to go outside of the printable area?

If the user wants to set margins to a specific value, we should honor that.  If
the UI design is that we want to prevent or warn the user when they set the
margins in the printable area, that should be done in the UI code.  From the
point of view of this code, the stack above has requested specific margins, so
we set them to that value.

http://codereview.chromium.org/8201027/diff/21001/printing/printing_context.h
File printing/printing_context.h (right):

http://codereview.chromium.org/8201027/diff/21001/printing/printing_context.h...
printing/printing_context.h:12: #include "base/logging.h"
On 2011/10/15 03:42:19, Lei Zhang wrote:
> nit: not needed?

Oops, this should be in the .cc

[commit-bot: I haz the power, 2011-10-15 22:30:50]

Change committed as 105688