De-duplicate job_settings parsing code.

De-duplicate job_settings parsing code.
Proper set/get methods in PrintSettings.

Objective of this re-factoring is to simplify processing of 
"print_to_pdf || is_cloud_dialog || print_to_cloud" case.
Now all platforms for this case do almost the same by in different way: calculate and set printing area.
In the next CL I'll move that code into common PrintingContext::UpdatePrintSettings and will ask platform specific implementation for desired page size only.

BUG=305731
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=231501

[Noam Samuel, 2013-10-28 21:52:38]

Overall LGTM, though I'm not OWNERS for any of these files.

https://codereview.chromium.org/47823002/diff/280001/printing/print_job_const...
File printing/print_job_constants.h (left):

https://codereview.chromium.org/47823002/diff/280001/printing/print_job_const...
printing/print_job_constants.h:57: #if defined(OS_MACOSX)
Is "Preview" a reference to print preview, or the application Preview in Mac
OSX?

[Vitaly Buka (NO REVIEWS), 2013-10-28 21:58:10]

https://codereview.chromium.org/47823002/diff/280001/printing/print_job_const...
File printing/print_job_constants.h (left):

https://codereview.chromium.org/47823002/diff/280001/printing/print_job_const...
printing/print_job_constants.h:57: #if defined(OS_MACOSX)
Yes. It's MacOSX preview.
I'd like to have code less platform dependent so I remove this ifdef.

On 2013/10/28 21:52:39, Noam Samuel wrote:
> Is "Preview" a reference to print preview, or the application Preview in Mac
> OSX?

[Noam Samuel, 2013-10-28 22:03:01]

https://codereview.chromium.org/47823002/diff/280001/printing/print_job_const...
File printing/print_job_constants.h (left):

https://codereview.chromium.org/47823002/diff/280001/printing/print_job_const...
printing/print_job_constants.h:57: #if defined(OS_MACOSX)
Maybe it's worth a comment explaining it? I could guess what it meant based on
the ifdefs, but without them it's not 100% clear.

On 2013/10/28 21:58:10, Vitaly Buka wrote:
> Yes. It's MacOSX preview.
> I'd like to have code less platform dependent so I remove this ifdef.
> 
> On 2013/10/28 21:52:39, Noam Samuel wrote:
> > Is "Preview" a reference to print preview, or the application Preview in Mac
> > OSX?
>

[Lei Zhang, 2013-10-28 22:10:39]

+1

With the OSX #ifdef, there is at least a hint that this is OS X Preview. Now
there's no context for the reader of the code.

On 2013/10/28 22:03:01, Noam Samuel wrote:
>
https://codereview.chromium.org/47823002/diff/280001/printing/print_job_const...
> File printing/print_job_constants.h (left):
> 
>
https://codereview.chromium.org/47823002/diff/280001/printing/print_job_const...
> printing/print_job_constants.h:57: #if defined(OS_MACOSX)
> Maybe it's worth a comment explaining it? I could guess what it meant based on
> the ifdefs, but without them it's not 100% clear.
> 
> On 2013/10/28 21:58:10, Vitaly Buka wrote:
> > Yes. It's MacOSX preview.
> > I'd like to have code less platform dependent so I remove this ifdef.
> > 
> > On 2013/10/28 21:52:39, Noam Samuel wrote:
> > > Is "Preview" a reference to print preview, or the application Preview in
Mac
> > > OSX?
> >

[Vitaly Buka (NO REVIEWS), 2013-10-28 22:12:52]

https://codereview.chromium.org/47823002/diff/280001/printing/print_job_const...
File printing/print_job_constants.h (left):

https://codereview.chromium.org/47823002/diff/280001/printing/print_job_const...
printing/print_job_constants.h:57: #if defined(OS_MACOSX)
On 2013/10/28 22:03:01, Noam Samuel wrote:
> Maybe it's worth a comment explaining it? I could guess what it meant based on
> the ifdefs, but without them it's not 100% clear.
> 
> On 2013/10/28 21:58:10, Vitaly Buka wrote:
> > Yes. It's MacOSX preview.
> > I'd like to have code less platform dependent so I remove this ifdef.
> > 
> > On 2013/10/28 21:52:39, Noam Samuel wrote:
> > > Is "Preview" a reference to print preview, or the application Preview in
Mac
> > > OSX?
> > 
> 

Done.

[Vitaly Buka (NO REVIEWS), 2013-10-28 22:29:40]

On 2013/10/28 22:12:52, Vitaly Buka wrote:
>
https://codereview.chromium.org/47823002/diff/280001/printing/print_job_const...
> File printing/print_job_constants.h (left):
> 
>
https://codereview.chromium.org/47823002/diff/280001/printing/print_job_const...
> printing/print_job_constants.h:57: #if defined(OS_MACOSX)
> On 2013/10/28 22:03:01, Noam Samuel wrote:
> > Maybe it's worth a comment explaining it? I could guess what it meant based
on
> > the ifdefs, but without them it's not 100% clear.
> > 
> > On 2013/10/28 21:58:10, Vitaly Buka wrote:
> > > Yes. It's MacOSX preview.
> > > I'd like to have code less platform dependent so I remove this ifdef.
> > > 
> > > On 2013/10/28 21:52:39, Noam Samuel wrote:
> > > > Is "Preview" a reference to print preview, or the application Preview in
> Mac
> > > > OSX?
> > > 
> > 
> 
> Done.

update CL description

[Vitaly Buka (NO REVIEWS), 2013-10-28 22:29:59]

On 2013/10/28 22:29:40, Vitaly Buka wrote:
> On 2013/10/28 22:12:52, Vitaly Buka wrote:
> >
>
https://codereview.chromium.org/47823002/diff/280001/printing/print_job_const...
> > File printing/print_job_constants.h (left):
> > 
> >
>
https://codereview.chromium.org/47823002/diff/280001/printing/print_job_const...
> > printing/print_job_constants.h:57: #if defined(OS_MACOSX)
> > On 2013/10/28 22:03:01, Noam Samuel wrote:
> > > Maybe it's worth a comment explaining it? I could guess what it meant
based
> on
> > > the ifdefs, but without them it's not 100% clear.
> > > 
> > > On 2013/10/28 21:58:10, Vitaly Buka wrote:
> > > > Yes. It's MacOSX preview.
> > > > I'd like to have code less platform dependent so I remove this ifdef.
> > > > 
> > > > On 2013/10/28 21:52:39, Noam Samuel wrote:
> > > > > Is "Preview" a reference to print preview, or the application Preview
in
> > Mac
> > > > > OSX?
> > > > 
> > > 
> > 
> > Done.
> 
> update CL description

I have updated CL description

[Lei Zhang, 2013-10-28 23:40:23]

https://codereview.chromium.org/47823002/diff/280001/printing/print_dialog_gt...
File printing/print_dialog_gtk_interface.h (right):

https://codereview.chromium.org/47823002/diff/280001/printing/print_dialog_gt...
printing/print_dialog_gtk_interface.h:27: // |pdf_only| is true if
implementation needs to generate PDF without actual
|pdf_only| -> |target_is_pdf|

https://codereview.chromium.org/47823002/diff/280001/printing/print_settings.h
File printing/print_settings.h (right):

https://codereview.chromium.org/47823002/diff/280001/printing/print_settings....
printing/print_settings.h:78: void set_ranges(const PageRanges& ranges) {
ranges_ = ranges; };
nit: Can you be consistent with the order you define the setter/getter?

https://codereview.chromium.org/47823002/diff/280001/printing/print_settings_...
File printing/print_settings_initializer.cc (left):

https://codereview.chromium.org/47823002/diff/280001/printing/print_settings_...
printing/print_settings_initializer.cc:46: print_settings->url =
gfx::ElideUrl(GURL(url), default_fonts, 0,
Did the elide functionality move elsewhere or is it being removed?

https://codereview.chromium.org/47823002/diff/280001/printing/print_settings_...
File printing/print_settings_initializer.cc (right):

https://codereview.chromium.org/47823002/diff/280001/printing/print_settings_...
printing/print_settings_initializer.cc:27:
job_settings.GetBoolean(kSettingHeaderFooterEnabled,
Should we save the result and check it with a DCHECK() to preserve the previous
behavior? Ditto below.

https://codereview.chromium.org/47823002/diff/280001/printing/print_settings_...
File printing/print_settings_initializer.h (right):

https://codereview.chromium.org/47823002/diff/280001/printing/print_settings_...
printing/print_settings_initializer.h:10: #include "printing/page_range.h"
You can just forward declare instead.

https://codereview.chromium.org/47823002/diff/280001/printing/printing_context.h
File printing/printing_context.h (right):

https://codereview.chromium.org/47823002/diff/280001/printing/printing_contex...
printing/printing_context.h:59: // preview.
You may want to give Preview.app as an example.

[Vitaly Buka (NO REVIEWS), 2013-10-29 00:31:21]

https://codereview.chromium.org/47823002/diff/280001/printing/print_dialog_gt...
File printing/print_dialog_gtk_interface.h (right):

https://codereview.chromium.org/47823002/diff/280001/printing/print_dialog_gt...
printing/print_dialog_gtk_interface.h:27: // |pdf_only| is true if
implementation needs to generate PDF without actual
On 2013/10/28 23:40:23, Lei Zhang wrote:
> |pdf_only| -> |target_is_pdf|

Done.

https://codereview.chromium.org/47823002/diff/280001/printing/print_settings.h
File printing/print_settings.h (right):

https://codereview.chromium.org/47823002/diff/280001/printing/print_settings....
printing/print_settings.h:78: void set_ranges(const PageRanges& ranges) {
ranges_ = ranges; };
On 2013/10/28 23:40:23, Lei Zhang wrote:
> nit: Can you be consistent with the order you define the setter/getter?

Done.

https://codereview.chromium.org/47823002/diff/280001/printing/print_settings_...
File printing/print_settings_initializer.cc (left):

https://codereview.chromium.org/47823002/diff/280001/printing/print_settings_...
printing/print_settings_initializer.cc:46: print_settings->url =
gfx::ElideUrl(GURL(url), default_fonts, 0,
On 2013/10/28 23:40:23, Lei Zhang wrote:
> Did the elide functionality move elsewhere or is it being removed?

It should be removed before when headers/footers switched to webkit rendering.
It's elided by CSS

https://codereview.chromium.org/47823002/diff/280001/printing/print_settings_...
File printing/print_settings_initializer.cc (right):

https://codereview.chromium.org/47823002/diff/280001/printing/print_settings_...
printing/print_settings_initializer.cc:27:
job_settings.GetBoolean(kSettingHeaderFooterEnabled,
Maybe it's better to return error here.

On 2013/10/28 23:40:23, Lei Zhang wrote:
> Should we save the result and check it with a DCHECK() to preserve the
previous
> behavior? Ditto below.

https://codereview.chromium.org/47823002/diff/280001/printing/printing_context.h
File printing/printing_context.h (right):

https://codereview.chromium.org/47823002/diff/280001/printing/printing_contex...
printing/printing_context.h:59: // preview.
On 2013/10/28 23:40:23, Lei Zhang wrote:
> You may want to give Preview.app as an example.

Done.

[Vitaly Buka (NO REVIEWS), 2013-10-29 00:36:00]

https://codereview.chromium.org/47823002/diff/280001/printing/print_settings_...
File printing/print_settings_initializer.h (right):

https://codereview.chromium.org/47823002/diff/280001/printing/print_settings_...
printing/print_settings_initializer.h:10: #include "printing/page_range.h"
On 2013/10/28 23:40:23, Lei Zhang wrote:
> You can just forward declare instead.

Only PageRange. PageRanges is "typedef vector<>"

[Lei Zhang, 2013-10-29 00:45:22]

lgtm

[commit-bot: I haz the power, 2013-10-29 00:48:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/47823002/510001

[commit-bot: I haz the power, 2013-10-29 03:16:39]

Change committed as 231501