Substitute boolean GDI printing for a mode type

Substitute boolean GDI printing for a mode type

Instead of indicating GDI printing with a boolean add a parameter to the
PdfRenderSettings structure that is an enum. This makes a few function
calls cleaner and will facilitate addition of new modes when postscript
printing is added (see https://codereview.chromium.org/2633573002/).

BUG=None

Review-Url: https://codereview.chromium.org/2640033005
Cr-Commit-Position: refs/heads/master@{#446484}
Committed: https://chromium.googlesource.com/chromium/src/+/c9251407fa9abe958dcbf2d72e7c25f8a422542c

[rbpotter, 2017-01-20 00:46:04]

The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 00:47:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 01:41:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-20 01:41:43]

Dry run: This issue passed the CQ dry run.

[rbpotter, 2017-01-21 01:24:18]

The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run

[rbpotter, 2017-01-21 01:24:34]

Description was changed from

==========
Substitute boolean GDI printing for a mode type

Instead of indicating GDI printing with a boolean add a parameter to the
PdfRenderSettings structure that is an enum. This makes a few function
calls cleaner and will facilitate addition of new modes when postscript
printing is added (see https://codereview.chromium.org/2633573002/).

BUG=None
==========

to

==========
Substitute boolean GDI printing for a mode type

Instead of indicating GDI printing with a boolean add a parameter to the
PdfRenderSettings structure that is an enum. This makes a few function
calls cleaner and will facilitate addition of new modes when postscript
printing is added (see https://codereview.chromium.org/2633573002/).

BUG=None
==========

[rbpotter, 2017-01-21 01:24:34]

rbpotter@chromium.org changed reviewers:
+ vitalybuka@chromium.org

[commit-bot: I haz the power, 2017-01-21 01:24:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-21 02:22:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-21 02:22:35]

Dry run: This issue passed the CQ dry run.

[rbpotter, 2017-01-23 21:01:12]

rbpotter@chromium.org changed reviewers:
+ tsepez@chromium.org

[rbpotter, 2017-01-23 21:01:52]

Switching to an enum vs a boolean to indicate GDI printing.
This is to set up for adding postscript printing path.

[Tom Sepez, 2017-01-23 21:14:08]

https://codereview.chromium.org/2640033005/diff/20001/printing/pdf_render_set...
File printing/pdf_render_settings.h (right):

https://codereview.chromium.org/2640033005/diff/20001/printing/pdf_render_set...
printing/pdf_render_settings.h:59: r->mode =
static_cast<printing::PdfRenderSettings::Mode>(mode);
Need to validate that mode is in range here or use the IPC macros instead to
generate its onw traits.

[Vitaly Buka (NO REVIEWS), 2017-01-23 22:03:30]

https://codereview.chromium.org/2640033005/diff/20001/printing/pdf_render_set...
File printing/pdf_render_settings.h (right):

https://codereview.chromium.org/2640033005/diff/20001/printing/pdf_render_set...
printing/pdf_render_settings.h:59: r->mode =
static_cast<printing::PdfRenderSettings::Mode>(mode);
On 2017/01/23 21:14:08, Tom Sepez wrote:
> Need to validate that mode is in range here or use the IPC macros instead to
> generate its onw traits.

Better to use IPC macros

[rbpotter, 2017-01-24 01:14:58]

The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 01:15:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 01:38:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 01:38:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rbpotter, 2017-01-24 01:55:39]

The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 01:56:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rbpotter, 2017-01-24 02:22:01]

https://codereview.chromium.org/2640033005/diff/20001/printing/pdf_render_set...
File printing/pdf_render_settings.h (right):

https://codereview.chromium.org/2640033005/diff/20001/printing/pdf_render_set...
printing/pdf_render_settings.h:59: r->mode =
static_cast<printing::PdfRenderSettings::Mode>(mode);
On 2017/01/23 22:03:30, Vitaly Buka wrote:
> On 2017/01/23 21:14:08, Tom Sepez wrote:
> > Need to validate that mode is in range here or use the IPC macros instead to
> > generate its onw traits.
> 
> Better to use IPC macros

Done.

I switched the entire structure to use the IPC macros, which seems to work.
Let me know if I should do something differently. Defining only the enum
but not the structure with the macros did not compile correctly.

[commit-bot: I haz the power, 2017-01-24 02:45:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 02:45:59]

Dry run: This issue passed the CQ dry run.

[Vitaly Buka (NO REVIEWS), 2017-01-25 19:51:37]

lgtm

[Tom Sepez, 2017-01-25 22:59:09]

Messages LGTM.

[rbpotter, 2017-01-25 23:00:10]

The CQ bit was checked by rbpotter@chromium.org

[commit-bot: I haz the power, 2017-01-25 23:00:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 23:13:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 23:13:01]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[rbpotter, 2017-01-25 23:17:03]

On 2017/01/25 23:13:01, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Do either of you know who to use as a owner/reviewer for
chrome/utility/printing_handler.*? I thought a security owner (tsepez@)
would be correct here as there are no other owners listed in the OWNERS
file other than ipc/SECURITY_OWNERS, but apparently that is not correct
and the next owners file up is chrome/OWNERS which says it is a last
resort...

[rbpotter, 2017-01-25 23:55:38]

rbpotter@chromium.org changed reviewers:
+ thakis@chromium.org

[rbpotter, 2017-01-26 00:00:20]

thakis@ - I believe the changes to printing_handler.*
require a review from chrome/OWNERS. Could you PTAL? If I
am missing something and this can be reviewed by a more
specific owner please let me know.

[Nico, 2017-01-26 20:46:05]

lgtm, with some optional nits (even if you do want to do them, feel free to do
them in a follow-up if this is urgent)

https://codereview.chromium.org/2640033005/diff/60001/chrome/browser/printing...
File chrome/browser/printing/print_job.cc (right):

https://codereview.chromium.org/2640033005/diff/60001/chrome/browser/printing...
chrome/browser/printing/print_job.cc:288: content_area, kPrinterDpi, true
/*autorotate? */,
nit: spell this like /*autorotate=*/true. For true, it doesn't make a big
difference, but for `false /*autorotate*/` it's not clear if false means
autorotate, or if that sets autorotate to false. (The `?` helps, but the = helps
too and that's fairly widely used in chrome, and clang-format knows not to put a
space after the comment for /*foo=*/ comments)

https://codereview.chromium.org/2640033005/diff/60001/chrome/browser/printing...
File chrome/browser/printing/pwg_raster_converter.cc (right):

https://codereview.chromium.org/2640033005/diff/60001/chrome/browser/printing...
chrome/browser/printing/pwg_raster_converter.cc:304: return
PdfRenderSettings(area, dpi, true /* autorotate */,
same nit

https://codereview.chromium.org/2640033005/diff/60001/printing/pdf_render_set...
File printing/pdf_render_settings.h (right):

https://codereview.chromium.org/2640033005/diff/60001/printing/pdf_render_set...
printing/pdf_render_settings.h:26: #endif
nit: consider

enum Mode {
  NORMAL,
#if defined(OS_WIN)
  GDI_TEXT,
  LAST = GDI_TEXT,
#else
  LAST = NORMAL,
#endif
};

then the logic to compute the last element is right where the enum is defined,
and you can just use LAST in the ipc macro file

[rbpotter, 2017-01-26 22:10:08]

The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 22:10:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rbpotter, 2017-01-26 22:10:50]

https://codereview.chromium.org/2640033005/diff/60001/chrome/browser/printing...
File chrome/browser/printing/print_job.cc (right):

https://codereview.chromium.org/2640033005/diff/60001/chrome/browser/printing...
chrome/browser/printing/print_job.cc:288: content_area, kPrinterDpi, true
/*autorotate? */,
On 2017/01/26 20:46:05, Nico wrote:
> nit: spell this like /*autorotate=*/true. For true, it doesn't make a big
> difference, but for `false /*autorotate*/` it's not clear if false means
> autorotate, or if that sets autorotate to false. (The `?` helps, but the =
helps
> too and that's fairly widely used in chrome, and clang-format knows not to put
a
> space after the comment for /*foo=*/ comments)

Done.

https://codereview.chromium.org/2640033005/diff/60001/chrome/browser/printing...
File chrome/browser/printing/pwg_raster_converter.cc (right):

https://codereview.chromium.org/2640033005/diff/60001/chrome/browser/printing...
chrome/browser/printing/pwg_raster_converter.cc:304: return
PdfRenderSettings(area, dpi, true /* autorotate */,
On 2017/01/26 20:46:05, Nico wrote:
> same nit

Done.

https://codereview.chromium.org/2640033005/diff/60001/printing/pdf_render_set...
File printing/pdf_render_settings.h (right):

https://codereview.chromium.org/2640033005/diff/60001/printing/pdf_render_set...
printing/pdf_render_settings.h:26: #endif
On 2017/01/26 20:46:05, Nico wrote:
> nit: consider
> 
> enum Mode {
>   NORMAL,
> #if defined(OS_WIN)
>   GDI_TEXT,
>   LAST = GDI_TEXT,
> #else
>   LAST = NORMAL,
> #endif
> };
> 
> then the logic to compute the last element is right where the enum is defined,
> and you can just use LAST in the ipc macro file

Done.

[commit-bot: I haz the power, 2017-01-26 22:58:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-26 22:58:22]

Dry run: This issue passed the CQ dry run.

[rbpotter, 2017-01-26 22:59:21]

The CQ bit was checked by rbpotter@chromium.org

[rbpotter, 2017-01-26 22:59:21]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
thakis@chromium.org, vitalybuka@chromium.org
Link to the patchset: https://codereview.chromium.org/2640033005/#ps80001
(title: "Fix nits")

[commit-bot: I haz the power, 2017-01-26 23:00:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 23:06:47]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485471561319670,
"parent_rev": "e3ec364721fe2d782df068cbdd7785e4d47365fa", "commit_rev":
"c9251407fa9abe958dcbf2d72e7c25f8a422542c"}

[commit-bot: I haz the power, 2017-01-26 23:07:17]

Description was changed from

==========
Substitute boolean GDI printing for a mode type

Instead of indicating GDI printing with a boolean add a parameter to the
PdfRenderSettings structure that is an enum. This makes a few function
calls cleaner and will facilitate addition of new modes when postscript
printing is added (see https://codereview.chromium.org/2633573002/).

BUG=None
==========

to

==========
Substitute boolean GDI printing for a mode type

Instead of indicating GDI printing with a boolean add a parameter to the
PdfRenderSettings structure that is an enum. This makes a few function
calls cleaner and will facilitate addition of new modes when postscript
printing is added (see https://codereview.chromium.org/2633573002/).

BUG=None

Review-Url: https://codereview.chromium.org/2640033005
Cr-Commit-Position: refs/heads/master@{#446484}
Committed:
https://chromium.googlesource.com/chromium/src/+/c9251407fa9abe958dcbf2d72e7c...
==========

[commit-bot: I haz the power, 2017-01-26 23:07:22]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/c9251407fa9abe958dcbf2d72e7c...