Print Preview: Add option to rasterize PDFs and add JPEG compression.

Print Preview: Add option to rasterize PDFs and add JPEG compression.

Add checkbox to print preview to select rasterized PDF output. When
selected, PDF will be rasterized and printed as a series of images.
Images will be JPEG encoded.

Preliminary design doc: https://docs.google.com/a/google.com/document/d/1UTzurMuPeRgx2PcnTMuAq5iM5_R2QBNGp-9lzgtwT2k/edit?usp=sharing

Depends on http://crrev.com/2529543003

Linked to launch bug, should also resolve 534945,550205,480628

BUG=675798
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/0fab356029c629728da6de182da3500661778b3c
Cr-Commit-Position: refs/heads/master@{#440897}

[rbpotter, 2016-11-23 23:08:23]

Description was changed from

==========
Add option to rasterize PDFs and add JPEG compression.

Add checkbox to print preview to select rasterized PDF output. When
selected, PDF will be rasterized and printed as a series of images.
Images will be JPEG encoded.

BUG=534945,550205,480628
==========

to

==========
Add option to rasterize PDFs and add JPEG compression.

Add checkbox to print preview to select rasterized PDF output. When
selected, PDF will be rasterized and printed as a series of images.
Images will be JPEG encoded.

BUG=534945,550205,480628
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[rbpotter, 2016-11-23 23:34:14]

Description was changed from

==========
Add option to rasterize PDFs and add JPEG compression.

Add checkbox to print preview to select rasterized PDF output. When
selected, PDF will be rasterized and printed as a series of images.
Images will be JPEG encoded.

BUG=534945,550205,480628
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add option to rasterize PDFs and add JPEG compression.

Add checkbox to print preview to select rasterized PDF output. When
selected, PDF will be rasterized and printed as a series of images.
Images will be JPEG encoded.

Depends on http://crrev.com/2529543003

BUG=534945,550205,480628
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[rbpotter, 2016-11-23 23:35:32]

Description was changed from

==========
Add option to rasterize PDFs and add JPEG compression.

Add checkbox to print preview to select rasterized PDF output. When
selected, PDF will be rasterized and printed as a series of images.
Images will be JPEG encoded.

Depends on http://crrev.com/2529543003

BUG=534945,550205,480628
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add option to rasterize PDFs and add JPEG compression.

Add checkbox to print preview to select rasterized PDF output. When
selected, PDF will be rasterized and printed as a series of images.
Images will be JPEG encoded.

Depends on http://crrev.com/2529543003

BUG=534945,550205,480628
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
CQ_DEPEND=2529543003
==========

[rbpotter, 2016-11-28 22:55:27]

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

[commit-bot: I haz the power, 2016-11-28 22:56:32]

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

[rbpotter, 2016-11-28 23:33:22]

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

[commit-bot: I haz the power, 2016-11-28 23:34:36]

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, 2016-11-29 00:16:22]

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

[commit-bot: I haz the power, 2016-11-29 00:16:23]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[rbpotter, 2016-11-29 01:11:12]

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

[commit-bot: I haz the power, 2016-11-29 01:12:33]

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

[rbpotter, 2016-11-29 01:51:00]

Description was changed from

==========
Add option to rasterize PDFs and add JPEG compression.

Add checkbox to print preview to select rasterized PDF output. When
selected, PDF will be rasterized and printed as a series of images.
Images will be JPEG encoded.

Depends on http://crrev.com/2529543003

BUG=534945,550205,480628
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
CQ_DEPEND=2529543003
==========

to

==========
Add option to rasterize PDFs and add JPEG compression.

Add checkbox to print preview to select rasterized PDF output. When
selected, PDF will be rasterized and printed as a series of images.
Images will be JPEG encoded.

Preliminary design doc:
https://docs.google.com/a/google.com/document/d/1UTzurMuPeRgx2PcnTMuAq5iM5_R2...

Depends on http://crrev.com/2529543003

BUG=534945,550205,480628
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
CQ_DEPEND=2529543003
==========

[rbpotter, 2016-11-29 01:51:00]

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

[rbpotter, 2016-11-29 01:51:41]

rbpotter@chromium.org changed reviewers:
- dpapad@chromium.org

[rbpotter, 2016-11-29 01:58:37]

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

[commit-bot: I haz the power, 2016-11-29 01:59:14]

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, 2016-11-29 03:20:07]

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

[commit-bot: I haz the power, 2016-11-29 03:20:08]

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

[rbpotter, 2016-11-30 20:30:24]

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

[commit-bot: I haz the power, 2016-11-30 20:30:56]

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, 2016-11-30 22:24:33]

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

[commit-bot: I haz the power, 2016-11-30 22:24:34]

Dry run: This issue passed the CQ dry run.

[rbpotter, 2016-12-13 20:16:46]

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

[commit-bot: I haz the power, 2016-12-13 20:17:33]

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

[rbpotter, 2016-12-13 20:26:33]

rbpotter@chromium.org changed reviewers:
+ dpapad@chromium.org, dsinclair@chromium.org, vitalybuka@chromium.org

[commit-bot: I haz the power, 2016-12-13 21:09:04]

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

[commit-bot: I haz the power, 2016-12-13 21:09:05]

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, 2016-12-13 21:42:52]

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

[commit-bot: I haz the power, 2016-12-13 21:43:18]

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, 2016-12-13 22:08:01]

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

[commit-bot: I haz the power, 2016-12-13 22:08:01]

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

[dpapad, 2016-12-13 23:38:18]

https://codereview.chromium.org/2524143003/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/data/ticket_items/rasterize.js
(right):

https://codereview.chromium.org/2524143003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/print_preview/data/ticket_items/rasterize.js:20: this,
null /*appState*/, null /*field*/, destinationStore,
Nit: Add spaces
/* appState */

https://codereview.chromium.org/2524143003/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/settings/other_options_settings.js
(right):

https://codereview.chromium.org/2524143003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/print_preview/settings/other_options_settings.js:35:
this.ticketItems_ = [headerFooter, fitToPage, duplex, cssBackground,
Can this be simplified even more? We have M arrays describing N items. An item
is fully described by the index position within those arrays

array1[i]
array2[i]
array3[i]
...

Can we instead create a wrapper object (perhaps named TicketItemElement?), that
encapsulates all the info (collapsible, ticketItem, container, checkbox etc)?
Then we can have a single array of TicketItemElement instances.

[dsinclair, 2016-12-14 19:54:37]

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

[dsinclair, 2016-12-14 19:54:39]

tsepez@ can you take a quick look at the bits in pdfium_engine?

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1392: if (pos + size < pos || pos + size >
buf_p->size)
The pos + size < pos seems like it would always be false? Should the second pos
be related to the buffer?

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1394: memcpy(pBuf, buf_p->data, size);
Should this be copying from buf_p->data + pos? Won't this always copy from the
beginning of the buffer?

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1394: memcpy(pBuf, buf_p->data, size);
Is it safe to assume pBuf is always big enough here?

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1396: }
Move up into the anonymous namespace.

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1452: bool encode_success =
gfx::JPEGCodec::Encode(bitmap_data,
Do we always want to do the JPEG encode, or only when the user selected the UI
option?

https://codereview.chromium.org/2524143003/diff/180001/printing/print_settings.h
File printing/print_settings.h (right):

https://codereview.chromium.org/2524143003/diff/180001/printing/print_setting...
printing/print_settings.h:100: rasterize_pdf_ = rasterize_pdf; }
nit: } on the next line.

[Tom Sepez, 2016-12-14 20:32:55]

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1357: if
(!HasPermission(PDFEngine::PERMISSION_PRINT_LOW_QUALITY))
nit: 
  Can a document have high_quality but not low_quality?  If so,
  can we assemble the permissions along the lines of

  uint32_t result = 0;
  if (HasPermission(PDFEngine::PERMISSION_PRINT_LOW_QUALITY))
    result |= PP_PRINTOUTPUTFORMAT_RASTER;
  if (HasPermission(PDFEngine::PERMISSION_PRINT_HIGH_QUALITY))
    result |= PP_PRINTOUTPUTFORMAT_PDF;
  return result;

which reads clearer to me -- and double check that I've got the right
permissions paired up above as I'm confused reading it.

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1384: uint8_t *data;
nit: uint8_t* data; per style guide (several places, e.g. 1391, 1437).

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1387: int GetBlockForJpeg(void *param,
the return type should probably be unsigned long to match |size|.

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1389: unsigned char* pBuf,
nit: uint8_t consistently throughout, hungarian notation only in pdfium itself
(eg. buf, not pBuf here), also this is probably const uint8_t* buf;

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1392: if (pos + size < pos || pos + size >
buf_p->size)
On 2016/12/14 19:54:38, dsinclair wrote:
> The pos + size < pos seems like it would always be false? Should the second
pos
> be related to the buffer?


This is checking for overflow, and is allowed in this manner so long as the
types are unsigned, which they are.

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1450: std::vector <unsigned char>
compressed_bitmap_data;
nit: std::vector<uint8_t>

Did you "git cl format" this?  I thought format would take care of these style
issues.

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1452: bool encode_success =
gfx::JPEGCodec::Encode(bitmap_data,
nit: local not needed

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1462: buffer.size = compressed_bitmap_data.size();
beware, this truncates a size_t to an uint32_t.  You might want to make the
lenghts size_t throughout.

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1474: delete buffer.data;
delete[] buffer.data?

[Tom Sepez, 2016-12-14 20:36:28]

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1463: buffer.data = new uint8_t[buffer.size];
actually, we can set buffer.data to compressed_bitmap_data.data(), since vector
is guaranteed to be contiguous and avoid the alloc/copy/free.

[rbpotter, 2016-12-15 00:50:59]

https://codereview.chromium.org/2524143003/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/data/ticket_items/rasterize.js
(right):

https://codereview.chromium.org/2524143003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/print_preview/data/ticket_items/rasterize.js:20: this,
null /*appState*/, null /*field*/, destinationStore,
On 2016/12/13 23:38:18, dpapad wrote:
> Nit: Add spaces
> /* appState */

Done.

https://codereview.chromium.org/2524143003/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/settings/other_options_settings.js
(right):

https://codereview.chromium.org/2524143003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/print_preview/settings/other_options_settings.js:35:
this.ticketItems_ = [headerFooter, fitToPage, duplex, cssBackground,
On 2016/12/13 23:38:18, dpapad wrote:
> Can this be simplified even more? We have M arrays describing N items. An item
> is fully described by the index position within those arrays
> 
> array1[i]
> array2[i]
> array3[i]
> ...
> 
> Can we instead create a wrapper object (perhaps named TicketItemElement?),
that
> encapsulates all the info (collapsible, ticketItem, container, checkbox etc)?
> Then we can have a single array of TicketItemElement instances.

Done.

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1357: if
(!HasPermission(PDFEngine::PERMISSION_PRINT_LOW_QUALITY))
On 2016/12/14 20:32:54, Tom Sepez wrote:
> nit: 
>   Can a document have high_quality but not low_quality?  If so,
>   can we assemble the permissions along the lines of
> 
>   uint32_t result = 0;
>   if (HasPermission(PDFEngine::PERMISSION_PRINT_LOW_QUALITY))
>     result |= PP_PRINTOUTPUTFORMAT_RASTER;
>   if (HasPermission(PDFEngine::PERMISSION_PRINT_HIGH_QUALITY))
>     result |= PP_PRINTOUTPUTFORMAT_PDF;
>   return result;
> 
> which reads clearer to me -- and double check that I've got the right
> permissions paired up above as I'm confused reading it.
Based on the logic in HasPermission, if a document has high quality it will
always also have low quality. I changed this anyway to make it more readable -
let me know if it is any better now.

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1384: uint8_t *data;
On 2016/12/14 20:32:54, Tom Sepez wrote:
> nit: uint8_t* data; per style guide (several places, e.g. 1391, 1437).

Done.

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1387: int GetBlockForJpeg(void *param,
On 2016/12/14 20:32:54, Tom Sepez wrote:
> the return type should probably be unsigned long to match |size|.

Unfortunately the signature is determined by the FPDF_FILEACCESS structure, so
it has to return an int. However it just needs to return something non-zero for
success, so I changed this to 1 to prevent the unsigned long to int cast.

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1389: unsigned char* pBuf,
On 2016/12/14 20:32:55, Tom Sepez wrote:
> nit: uint8_t consistently throughout, hungarian notation only in pdfium itself
> (eg. buf, not pBuf here), also this is probably const uint8_t* buf;

The signature for this function is defined by the FPDF_FILEACCESS structure as
using unsigned char* (and unsigned long). It will compile at least on Linux with
uint8_t* and size_t, but not sure if this is guaranteed to compile on all
platforms this way. Should I stick to the function signature from PDFium or
change it to be consistent with other variables?

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1394: memcpy(pBuf, buf_p->data, size);
On 2016/12/14 19:54:38, dsinclair wrote:
> Should this be copying from buf_p->data + pos? Won't this always copy from the
> beginning of the buffer?

Done.

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1394: memcpy(pBuf, buf_p->data, size);
On 2016/12/14 19:54:38, dsinclair wrote:
> Is it safe to assume pBuf is always big enough here?

Since the buffer is provided by the caller I don't think there is any way to
check from here; the assumption for the FPDF_FILEACCESS seems to be that the
caller will ensure the size of the buffer is at least equal to size.

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1396: }
On 2016/12/14 19:54:38, dsinclair wrote:
> Move up into the anonymous namespace.

Done.

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1450: std::vector <unsigned char>
compressed_bitmap_data;
On 2016/12/14 20:32:54, Tom Sepez wrote:
> nit: std::vector<uint8_t>
> 
> Did you "git cl format" this?  I thought format would take care of these style
> issues.

Done.

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1452: bool encode_success =
gfx::JPEGCodec::Encode(bitmap_data,
On 2016/12/14 19:54:38, dsinclair wrote:
> Do we always want to do the JPEG encode, or only when the user selected the UI
> option?

Not sure. The JPEG encode saves space for big/image heavy documents. However, it
is probably safer to keep the existing behavior unless the UI option is
selected, as a JPEG image printout will probably be expected in that case, while
it might not be if the document permissions are what is preventing a standard
PDF print. So I have changed it to only encode when the UI option is selected,
thanks for the suggestion.

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1462: buffer.size = compressed_bitmap_data.size();
On 2016/12/14 20:32:54, Tom Sepez wrote:
> beware, this truncates a size_t to an uint32_t.  You might want to make the
> lenghts size_t throughout.

Done.

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1463: buffer.data = new uint8_t[buffer.size];
On 2016/12/14 20:36:28, Tom Sepez wrote:
> actually, we can set buffer.data to compressed_bitmap_data.data(), since
vector
> is guaranteed to be contiguous and avoid the alloc/copy/free.

Done.

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1474: delete buffer.data;
On 2016/12/14 20:32:54, Tom Sepez wrote:
> delete[] buffer.data?

N/A

https://codereview.chromium.org/2524143003/diff/180001/printing/print_settings.h
File printing/print_settings.h (right):

https://codereview.chromium.org/2524143003/diff/180001/printing/print_setting...
printing/print_settings.h:100: rasterize_pdf_ = rasterize_pdf; }
On 2016/12/14 19:54:38, dsinclair wrote:
> nit: } on the next line.

Done.

[dpapad, 2016-12-15 01:17:50]

https://codereview.chromium.org/2524143003/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/settings/other_options_settings.js
(right):

https://codereview.chromium.org/2524143003/diff/200001/chrome/browser/resourc...
chrome/browser/resources/print_preview/settings/other_options_settings.js:8:
function CheckboxTicketItemElement(ticketItem, collapsible, className) {
This looks like a solid improvement (removes a lot of duplicated code). How
about spinning this off as a separate CL that will get reviewed and landed
before this one?

[Vitaly Buka (NO REVIEWS), 2016-12-15 08:27:34]

https://codereview.chromium.org/2524143003/diff/1/pdf/pdfium/pdfium_engine.cc
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/2524143003/diff/1/pdf/pdfium/pdfium_engine.cc...
pdf/pdfium/pdfium_engine.cc:1494: //  encode_success = false;
please remove

https://codereview.chromium.org/2524143003/diff/1/pdf/pdfium/pdfium_engine.cc...
pdf/pdfium/pdfium_engine.cc:1572: for (; i < pages_to_print.size(); ++i) {
Seems total_doc_size is not used.

https://codereview.chromium.org/2524143003/diff/200001/components/printing/re...
File components/printing/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/2524143003/diff/200001/components/printing/re...
components/printing/renderer/print_web_view_helper.cc:1729:
job_settings->GetBoolean(kSettingRasterizePdf, &raster);
value is not used?

[dsinclair, 2016-12-15 18:57:32]

pdf/ changes lgtm

[Tom Sepez, 2016-12-15 21:09:23]

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1357: if
(!HasPermission(PDFEngine::PERMISSION_PRINT_LOW_QUALITY))
On 2016/12/15 00:50:59, rbpotter wrote:
> On 2016/12/14 20:32:54, Tom Sepez wrote:
> > nit: 
> >   Can a document have high_quality but not low_quality?  If so,
> >   can we assemble the permissions along the lines of
> > 
> >   uint32_t result = 0;
> >   if (HasPermission(PDFEngine::PERMISSION_PRINT_LOW_QUALITY))
> >     result |= PP_PRINTOUTPUTFORMAT_RASTER;
> >   if (HasPermission(PDFEngine::PERMISSION_PRINT_HIGH_QUALITY))
> >     result |= PP_PRINTOUTPUTFORMAT_PDF;
> >   return result;
> > 
> > which reads clearer to me -- and double check that I've got the right
> > permissions paired up above as I'm confused reading it.
> Based on the logic in HasPermission, if a document has high quality it will
> always also have low quality. I changed this anyway to make it more readable -
> let me know if it is any better now.
Lots better, thanks.

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1394: memcpy(pBuf, buf_p->data, size);
On 2016/12/14 19:54:38, dsinclair wrote:
> Should this be copying from buf_p->data + pos? Won't this always copy from the
> beginning of the buffer?

I think this is still an issue.

https://codereview.chromium.org/2524143003/diff/200001/pdf/pdfium/pdfium_engi...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/2524143003/diff/200001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:621: struct simple_buf {
actually, can we avoid this altogether by passing a pointer to the std::vector<>
itself?

[rbpotter, 2016-12-16 00:05:48]

rbpotter@chromium.org changed reviewers:
+ bbudge@chromium.org, dcheng@chromium.org

[rbpotter, 2016-12-16 00:06:31]

https://codereview.chromium.org/2524143003/diff/1/pdf/pdfium/pdfium_engine.cc
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/2524143003/diff/1/pdf/pdfium/pdfium_engine.cc...
pdf/pdfium/pdfium_engine.cc:1494: //  encode_success = false;
On 2016/12/15 08:27:34, Vitaly Buka wrote:
> please remove

This is already removed in the latest patchset.

https://codereview.chromium.org/2524143003/diff/1/pdf/pdfium/pdfium_engine.cc...
pdf/pdfium/pdfium_engine.cc:1572: for (; i < pages_to_print.size(); ++i) {
On 2016/12/15 08:27:34, Vitaly Buka wrote:
> Seems total_doc_size is not used.

This is already removed in the latest patchset.

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/2524143003/diff/180001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1394: memcpy(pBuf, buf_p->data, size);
On 2016/12/15 21:09:23, Tom Sepez wrote:
> On 2016/12/14 19:54:38, dsinclair wrote:
> > Should this be copying from buf_p->data + pos? Won't this always copy from
the
> > beginning of the buffer?
> 
> I think this is still an issue.
This is now copying size bytes from buf_p->data.size + pos, so an offset of pos
bytes into the "file", to the user provided buffer. I believe this is correct
based on the description of this function in the definition of FPDF_FILEACCESS,
but let me know if I am missing something.

https://codereview.chromium.org/2524143003/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/settings/other_options_settings.js
(right):

https://codereview.chromium.org/2524143003/diff/200001/chrome/browser/resourc...
chrome/browser/resources/print_preview/settings/other_options_settings.js:8:
function CheckboxTicketItemElement(ticketItem, collapsible, className) {
On 2016/12/15 01:17:50, dpapad wrote:
> This looks like a solid improvement (removes a lot of duplicated code). How
> about spinning this off as a separate CL that will get reviewed and landed
> before this one?
Done in http://crrev.com/2576233003. Will rebase this once it lands.

https://codereview.chromium.org/2524143003/diff/200001/components/printing/re...
File components/printing/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/2524143003/diff/200001/components/printing/re...
components/printing/renderer/print_web_view_helper.cc:1729:
job_settings->GetBoolean(kSettingRasterizePdf, &raster);
On 2016/12/15 08:27:34, Vitaly Buka wrote:
> value is not used?

Done.

https://codereview.chromium.org/2524143003/diff/200001/pdf/pdfium/pdfium_engi...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/2524143003/diff/200001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:621: struct simple_buf {
On 2016/12/15 21:09:23, Tom Sepez wrote:
> actually, can we avoid this altogether by passing a pointer to the
std::vector<>
> itself?

Yes, done

[Vitaly Buka (NO REVIEWS), 2016-12-16 00:25:22]

are there any tests which could be extended?

[Vitaly Buka (NO REVIEWS), 2016-12-16 00:54:38]

https://codereview.chromium.org/2524143003/diff/220001/content/renderer/peppe...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/2524143003/diff/220001/content/renderer/peppe...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1784: *format =
PP_PRINTOUTPUTFORMAT_RASTER;
It should be PP_PRINTOUTPUTFORMAT_PDF as it's still PDF with embedded image.

https://codereview.chromium.org/2524143003/diff/220001/content/renderer/peppe...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1871:
current_print_settings_.format == PP_PRINTOUTPUTFORMAT_RASTER)
I believe we should keep PP_PRINTOUTPUTFORMAT_PDF

https://codereview.chromium.org/2524143003/diff/220001/pdf/pdfium/pdfium_engi...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/2524143003/diff/220001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1452: FPDF_FILEACCESS file_access;
no need to do memset, just:
FPDF_FILEACCESS file_access = {};


even with memset: '\0' -> 0

[rbpotter, 2016-12-16 01:14:33]

Re: extending tests, I have already extended the relevant javascript
capabilities tests to check the UI behavior. I do not see any tests in
components/printing that would make sense to extend as they are all related to
printing HTML and this is a PDF only modification.

https://codereview.chromium.org/2524143003/diff/220001/content/renderer/peppe...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/2524143003/diff/220001/content/renderer/peppe...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1784: *format =
PP_PRINTOUTPUTFORMAT_RASTER;
On 2016/12/16 00:54:38, Vitaly Buka wrote:
> It should be PP_PRINTOUTPUTFORMAT_PDF as it's still PDF with embedded image.

thestig@ specifically told me to do it this way before he left to avoid
modifying the PP_PrintSettings_Dev structure. Not sure how to resolve this since
he is OOO currently... can we defer to bbudge@ and raymes@  from
content/renderer/pepper/OWNERS?

https://codereview.chromium.org/2524143003/diff/220001/content/renderer/peppe...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1871:
current_print_settings_.format == PP_PRINTOUTPUTFORMAT_RASTER)
On 2016/12/16 00:54:38, Vitaly Buka wrote:
> I believe we should keep PP_PRINTOUTPUTFORMAT_PDF

See above.

https://codereview.chromium.org/2524143003/diff/220001/pdf/pdfium/pdfium_engi...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/2524143003/diff/220001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1452: FPDF_FILEACCESS file_access;
On 2016/12/16 00:54:38, Vitaly Buka wrote:
> no need to do memset, just:
> FPDF_FILEACCESS file_access = {};
> 
> 
> even with memset: '\0' -> 0

Done.

[rbpotter, 2016-12-16 01:15:10]

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

[Vitaly Buka (NO REVIEWS), 2016-12-16 01:33:54]

Please point attention of bbudge@ and raymes@ to PP_PRINTOUTPUTFORMAT_RASTER vs
PP_PRINTOUTPUTFORMAT_PDF.
I am not sure but it looks suspicious.

components/printing/ printing/ LGTM

[dcheng, 2016-12-16 01:44:27]

I'm not sure what I'm supposed to review. Under the assumption that it's just
WebKit, LGTM.

(For CLs with many reviewers, it helps to explicitly tell each reviewer which
portion to look at. Thanks!)

https://codereview.chromium.org/2524143003/diff/240001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebPrintParams.h (right):

https://codereview.chromium.org/2524143003/diff/240001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebPrintParams.h:55: bool rasterizePDF;
Consider just writing:

bool rasterizePDF = false;

So we don't have to duplicate it into every initializer list.

[rbpotter, 2016-12-16 01:55:53]

On 2016/12/16 01:44:27, dcheng wrote:
> I'm not sure what I'm supposed to review. Under the assumption that it's just
> WebKit, LGTM.
> 
> (For CLs with many reviewers, it helps to explicitly tell each reviewer which
> portion to look at. Thanks!)
> 
>
https://codereview.chromium.org/2524143003/diff/240001/third_party/WebKit/pub...
> File third_party/WebKit/public/web/WebPrintParams.h (right):
> 
>
https://codereview.chromium.org/2524143003/diff/240001/third_party/WebKit/pub...
> third_party/WebKit/public/web/WebPrintParams.h:55: bool rasterizePDF;
> Consider just writing:
> 
> bool rasterizePDF = false;
> 
> So we don't have to duplicate it into every initializer list.

Sorry about that. Could you also take a look at
components/printing/common/printing_messages.*? Feel free suggest another
security reviewer if you do not have time.

To clarify in general:
vitalybuka@ --> printing/, components/printing
dpapad@ --> chrome/browser/resources/print_preview, chrome/app/,
chrome/test/webui/
dsinclair@/tsepez@ --> pdf/
bbudge@/raymes@ --> content/renderer/pepper

[bbudge, 2016-12-16 01:58:40]

It looks like the intent is to force the Pepper plugin to rasterize rather than
print as PDF in some cases.

LGTM

[dcheng, 2016-12-16 08:51:08]

ipc lgtm as well

[Tom Sepez, 2016-12-16 18:32:21]

lgtm

[rbpotter, 2016-12-16 22:00:36]

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

[rbpotter, 2016-12-16 22:01:14]

Merged in the newly landed other_options_settings.js modifications (changes only
print preview javascript files).

[commit-bot: I haz the power, 2016-12-16 22:03:08]

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, 2016-12-16 23:53:28]

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

[commit-bot: I haz the power, 2016-12-16 23:53:29]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[dpapad, 2016-12-17 01:19:03]

LGTM.

https://codereview.chromium.org/2524143003/diff/280001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/settings/other_options_settings.html
(right):

https://codereview.chromium.org/2524143003/diff/280001/chrome/browser/resourc...
chrome/browser/resources/print_preview/settings/other_options_settings.html:6:
<div class="right-column checkbox">
I don't think that "checkbox" here has any effect. Is it necessary?

https://codereview.chromium.org/2524143003/diff/280001/chrome/browser/resourc...
chrome/browser/resources/print_preview/settings/other_options_settings.html:34:
<input class="checkbox" type="checkbox">
Nit (optional cleanup for a future cleanup): If the previous comment is correct,
then CSS class "checkbox" is not necessary. The only elements that have it would
be <input type="checkbox"> and those can be selected using the
input[type="checkbox"] CSS selector, which is already being used at 
https://cs.chromium.org/chromium/src/chrome/browser/resources/print_preview/p....

[rbpotter, 2016-12-20 22:14:33]

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

[commit-bot: I haz the power, 2016-12-20 22:15:17]

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

[rbpotter, 2016-12-20 22:17:50]

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

[rbpotter, 2016-12-20 22:32:57]

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

[rbpotter, 2016-12-20 22:33:04]

Patchset #16 (id:300001) has been deleted

[commit-bot: I haz the power, 2016-12-20 22:33:49]

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

[rbpotter, 2016-12-20 22:41:05]

Added a flag. Flag modifications affect only JS/webui and the
histograms/chrome_features/testing files. Also followed suggestion to initialize
WebKit printing parameter in declaration instead of in every initializer list.

https://codereview.chromium.org/2524143003/diff/240001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebPrintParams.h (right):

https://codereview.chromium.org/2524143003/diff/240001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebPrintParams.h:55: bool rasterizePDF;
On 2016/12/16 01:44:27, dcheng wrote:
> Consider just writing:
> 
> bool rasterizePDF = false;
> 
> So we don't have to duplicate it into every initializer list.

Done.

https://codereview.chromium.org/2524143003/diff/280001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/settings/other_options_settings.html
(right):

https://codereview.chromium.org/2524143003/diff/280001/chrome/browser/resourc...
chrome/browser/resources/print_preview/settings/other_options_settings.html:6:
<div class="right-column checkbox">
On 2016/12/17 01:19:03, dpapad wrote:
> I don't think that "checkbox" here has any effect. Is it necessary?

It does actually have an effect. There is formatting applied to ".right-column
.checkbox label" in print_preview.css that spaces out these checkboxes
vertically. With this removed the checkboxes end up right on top of each other.
It would be possible to add "checkbox" to each of the "containers" instead, or
to change the formatting to work differently. Would either of these options be
preferable or should I leave it as is?

[rbpotter, 2016-12-20 22:46:13]

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

[commit-bot: I haz the power, 2016-12-21 02:04:22]

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

[commit-bot: I haz the power, 2016-12-21 02:04:25]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2016-12-21 03:25:54]

histograms.xml and fieldtrial_testing_config LGTM.  FWIW, you don't need to add
the fieldtrial_testing_config until you are closer to launching the feature...
but it doesn't hurt, either =)

[rbpotter, 2016-12-22 01:39:13]

rbpotter@chromium.org changed reviewers:
- jwd@chromium.org

[rbpotter, 2016-12-22 01:55:06]

Description was changed from

==========
Add option to rasterize PDFs and add JPEG compression.

Add checkbox to print preview to select rasterized PDF output. When
selected, PDF will be rasterized and printed as a series of images.
Images will be JPEG encoded.

Preliminary design doc:
https://docs.google.com/a/google.com/document/d/1UTzurMuPeRgx2PcnTMuAq5iM5_R2...

Depends on http://crrev.com/2529543003

BUG=534945,550205,480628
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
CQ_DEPEND=2529543003
==========

to

==========
Print Preview: Add option to rasterize PDFs and add JPEG compression.

Add checkbox to print preview to select rasterized PDF output. When
selected, PDF will be rasterized and printed as a series of images.
Images will be JPEG encoded.

Preliminary design doc:
https://docs.google.com/a/google.com/document/d/1UTzurMuPeRgx2PcnTMuAq5iM5_R2...

Depends on http://crrev.com/2529543003

Linked to launch bug, should also resolve 534945,550205,480628

BUG=676263
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[rbpotter, 2016-12-22 01:55:57]

Description was changed from

==========
Print Preview: Add option to rasterize PDFs and add JPEG compression.

Add checkbox to print preview to select rasterized PDF output. When
selected, PDF will be rasterized and printed as a series of images.
Images will be JPEG encoded.

Preliminary design doc:
https://docs.google.com/a/google.com/document/d/1UTzurMuPeRgx2PcnTMuAq5iM5_R2...

Depends on http://crrev.com/2529543003

Linked to launch bug, should also resolve 534945,550205,480628

BUG=676263
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Print Preview: Add option to rasterize PDFs and add JPEG compression.

Add checkbox to print preview to select rasterized PDF output. When
selected, PDF will be rasterized and printed as a series of images.
Images will be JPEG encoded.

Preliminary design doc:
https://docs.google.com/a/google.com/document/d/1UTzurMuPeRgx2PcnTMuAq5iM5_R2...

Depends on http://crrev.com/2529543003

Linked to launch bug, should also resolve 534945,550205,480628

BUG=675798
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-12-22 19:27:00]

printPdfAsImageEnabled JS changes: still LGTM.

https://codereview.chromium.org/2524143003/diff/320001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/settings/other_options_settings.js
(right):

https://codereview.chromium.org/2524143003/diff/320001/chrome/browser/resourc...
chrome/browser/resources/print_preview/settings/other_options_settings.js:151:
new CheckboxTicketItemElement(headerFooter, true,
Nit: Indentation should be 2 less.

https://codereview.chromium.org/2524143003/diff/320001/chrome/browser/resourc...
chrome/browser/resources/print_preview/settings/other_options_settings.js:162:
this.elements_.splice(4, 0, new CheckboxTicketItemElement(rasterize,
Nit: This type of line breaking violates the "rectangle rule", see
https://google.github.io/styleguide/jsguide.html#formatting-rectangle-rule.

https://codereview.chromium.org/2524143003/diff/320001/chrome/browser/resourc...
chrome/browser/resources/print_preview/settings/other_options_settings.js:221:
$('rasterize-container').style.display = 'none';
Nit (optional): I think the following would also work (did not try it though).

$('rasterize-container').hidden = !this.rasterizeEnabled_;

[rbpotter, 2016-12-22 22:05:01]

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

[commit-bot: I haz the power, 2016-12-22 22:05:26]

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

[rbpotter, 2016-12-22 22:58:46]

https://codereview.chromium.org/2524143003/diff/320001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/settings/other_options_settings.js
(right):

https://codereview.chromium.org/2524143003/diff/320001/chrome/browser/resourc...
chrome/browser/resources/print_preview/settings/other_options_settings.js:151:
new CheckboxTicketItemElement(headerFooter, true,
On 2016/12/22 19:27:00, dpapad wrote:
> Nit: Indentation should be 2 less.

Done.

https://codereview.chromium.org/2524143003/diff/320001/chrome/browser/resourc...
chrome/browser/resources/print_preview/settings/other_options_settings.js:162:
this.elements_.splice(4, 0, new CheckboxTicketItemElement(rasterize,
On 2016/12/22 19:27:00, dpapad wrote:
> Nit: This type of line breaking violates the "rectangle rule", see
> https://google.github.io/styleguide/jsguide.html#formatting-rectangle-rule.

Done.

https://codereview.chromium.org/2524143003/diff/320001/chrome/browser/resourc...
chrome/browser/resources/print_preview/settings/other_options_settings.js:221:
$('rasterize-container').style.display = 'none';
On 2016/12/22 19:27:00, dpapad wrote:
> Nit (optional): I think the following would also work (did not try it though).
> 
> $('rasterize-container').hidden = !this.rasterizeEnabled_;

Done.

[commit-bot: I haz the power, 2016-12-23 01:16:46]

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

[commit-bot: I haz the power, 2016-12-23 01:16:48]

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, 2016-12-28 19:06:25]

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

[commit-bot: I haz the power, 2016-12-28 19:06:34]

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, 2016-12-28 20:53:42]

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

[commit-bot: I haz the power, 2016-12-28 20:53:43]

Dry run: This issue passed the CQ dry run.

[rbpotter, 2016-12-28 21:55:46]

The CQ bit was checked by rbpotter@chromium.org

[rbpotter, 2016-12-28 21:55:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
dsinclair@chromium.org, bbudge@chromium.org, dcheng@chromium.org,
tsepez@chromium.org, vitalybuka@chromium.org, dpapad@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2524143003/#ps340001
(title: "Clean up JS")

[commit-bot: I haz the power, 2016-12-28 21:55:58]

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, 2016-12-28 22:00:27]

CQ is committing da patch.

Bot data: {"patchset_id": 340001, "attempt_start_ts": 1482962146246530,
"parent_rev": "a7da38d07517d491a874d79e0dc29b46a63f8422", "commit_rev":
"099e6c006040fdc220015fdaf040d03961984751"}

[commit-bot: I haz the power, 2016-12-28 22:01:00]

Description was changed from

==========
Print Preview: Add option to rasterize PDFs and add JPEG compression.

Add checkbox to print preview to select rasterized PDF output. When
selected, PDF will be rasterized and printed as a series of images.
Images will be JPEG encoded.

Preliminary design doc:
https://docs.google.com/a/google.com/document/d/1UTzurMuPeRgx2PcnTMuAq5iM5_R2...

Depends on http://crrev.com/2529543003

Linked to launch bug, should also resolve 534945,550205,480628

BUG=675798
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Print Preview: Add option to rasterize PDFs and add JPEG compression.

Add checkbox to print preview to select rasterized PDF output. When
selected, PDF will be rasterized and printed as a series of images.
Images will be JPEG encoded.

Preliminary design doc:
https://docs.google.com/a/google.com/document/d/1UTzurMuPeRgx2PcnTMuAq5iM5_R2...

Depends on http://crrev.com/2529543003

Linked to launch bug, should also resolve 534945,550205,480628

BUG=675798
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2524143003
==========

[commit-bot: I haz the power, 2016-12-28 22:01:02]

Committed patchset #17 (id:340001)

[commit-bot: I haz the power, 2017-01-02 15:50:26]

Description was changed from

==========
Print Preview: Add option to rasterize PDFs and add JPEG compression.

Add checkbox to print preview to select rasterized PDF output. When
selected, PDF will be rasterized and printed as a series of images.
Images will be JPEG encoded.

Preliminary design doc:
https://docs.google.com/a/google.com/document/d/1UTzurMuPeRgx2PcnTMuAq5iM5_R2...

Depends on http://crrev.com/2529543003

Linked to launch bug, should also resolve 534945,550205,480628

BUG=675798
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2524143003
==========

to

==========
Print Preview: Add option to rasterize PDFs and add JPEG compression.

Add checkbox to print preview to select rasterized PDF output. When
selected, PDF will be rasterized and printed as a series of images.
Images will be JPEG encoded.

Preliminary design doc:
https://docs.google.com/a/google.com/document/d/1UTzurMuPeRgx2PcnTMuAq5iM5_R2...

Depends on http://crrev.com/2529543003

Linked to launch bug, should also resolve 534945,550205,480628

BUG=675798
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/0fab356029c629728da6de182da3500661778b3c
Cr-Commit-Position: refs/heads/master@{#440897}
==========

[commit-bot: I haz the power, 2017-01-02 15:50:28]

Patchset 17 (id:??) landed as
https://crrev.com/0fab356029c629728da6de182da3500661778b3c
Cr-Commit-Position: refs/heads/master@{#440897}

[Lei Zhang, 2017-02-25 01:01:14]

thestig@chromium.org changed reviewers:
+ thestig@chromium.org

[Lei Zhang, 2017-02-25 01:01:17]

Just being nitty.

https://codereview.chromium.org/2524143003/diff/340001/components/printing/co...
File components/printing/common/print_messages.h (right):

https://codereview.chromium.org/2524143003/diff/340001/components/printing/co...
components/printing/common/print_messages.h:132: // Whether to rasterize a PDF
for printing
Move this down below line 144 somewhere?

https://codereview.chromium.org/2524143003/diff/340001/content/renderer/peppe...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/2524143003/diff/340001/content/renderer/peppe...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1793: params.rasterizePDF
= false;
BTW, this is already initialized to false in the member declaration.

https://codereview.chromium.org/2524143003/diff/340001/pdf/pdfium/DEPS
File pdf/pdfium/DEPS (right):

https://codereview.chromium.org/2524143003/diff/340001/pdf/pdfium/DEPS#newcode8
pdf/pdfium/DEPS:8: "+ui/gfx/codec/jpeg_codec.h",
nit: sort

https://codereview.chromium.org/2524143003/diff/340001/pdf/pdfium/pdfium_engi...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/2524143003/diff/340001/pdf/pdfium/pdfium_engi...
pdf/pdfium/pdfium_engine.cc:1446: int quality = 40;
const + add comment to mention why we chose 40.

https://codereview.chromium.org/2524143003/diff/340001/printing/print_job_con...
File printing/print_job_constants.cc (right):

https://codereview.chromium.org/2524143003/diff/340001/printing/print_job_con...
printing/print_job_constants.cc:175: // Scaling factor
Copy pasta