Fix printing of PDFs. They should not be cut off.

Fix printing of PDFs. They should not be cut off.

r351729 fixed the case of a crop box being larger than a media box, but
introduced a regression where it assumed non-existent crop boxes are the
default size of 8.5 x 11. Instead, non-existent crop boxes should imply
they are the same size as the media box.

BUG=543304
Committed: https://crrev.com/8c6f10e481a5c2af50ac58d2971a171806015bc9
Cr-Commit-Position: refs/heads/master@{#356249}

[Lei Zhang, 2015-10-27 04:48:45]

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

[Lei Zhang, 2015-10-27 04:48:46]

See the diffs between patch sets for the step by step.

[raymes, 2015-10-27 05:59:07]

lgtm - thanks for breaking this up :) 

Could we expand the CL description a bit? 

Was the bug the fact that prior to this CL the media box and crop box would not
be set to the same value if one of them was available?

https://codereview.chromium.org/1415413008/diff/40001/pdf/pdfium/pdfium_engin...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/1415413008/diff/40001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:372: // Set the media box and/or crop box as needed.
nit: fill 80chars

https://codereview.chromium.org/1415413008/diff/40001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:390: *media_box = *crop_box;
This is a little confusing to me. Perhaps we just do the more straightforward
approach?:

if (!has_media_box && !has_crop_box) {
  SetDefaultClipBox(rotated, crop_box);
  SetDefaultClipBox(rotated, media_box);
} else if (has_crop_box && !has_media_box) {
  *media_box = *crop_box;
} else if (has_media_box && !has_crop_box) {
  *crop_box = *media_box;
}

[Lei Zhang, 2015-10-27 06:48:01]

Description was changed from

==========
Fix printing of PDFs. They should not be cut off.

BUG=543304
==========

to

==========
Fix printing of PDFs. They should not be cut off.

r351729 fixed the case of a crop box being larger than a media box, but
introduced a regression where it assumed non-existent crop boxes are the
default size of 8.5 x 11. Instead, non-existent crop boxes should imply
they are the same size as the media box.

BUG=543304
==========

[Lei Zhang, 2015-10-27 06:51:48]

On 2015/10/27 05:59:07, raymes wrote:
> lgtm - thanks for breaking this up :) 
> 
> Could we expand the CL description a bit? 

Done.

> Was the bug the fact that prior to this CL the media box and crop box would
not
> be set to the same value if one of them was available?

Right, it was wrong to assume 8.5 x 11 for the missing box if only one of the
two boxes were missing.

[Lei Zhang, 2015-10-27 06:52:13]

https://codereview.chromium.org/1415413008/diff/40001/pdf/pdfium/pdfium_engin...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/1415413008/diff/40001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:372: // Set the media box and/or crop box as needed.
On 2015/10/27 05:59:07, raymes wrote:
> nit: fill 80chars

Done.

https://codereview.chromium.org/1415413008/diff/40001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:390: *media_box = *crop_box;
On 2015/10/27 05:59:07, raymes wrote:
> This is a little confusing to me. Perhaps we just do the more straightforward
> approach?:
> 
> if (!has_media_box && !has_crop_box) {
>   SetDefaultClipBox(rotated, crop_box);
>   SetDefaultClipBox(rotated, media_box);
> } else if (has_crop_box && !has_media_box) {
>   *media_box = *crop_box;
> } else if (has_media_box && !has_crop_box) {
>   *crop_box = *media_box;
> }

Done.

[Lei Zhang, 2015-10-27 06:52:40]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2015-10-27 06:52:40]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/1415413008/#ps60001
(title: "Address comments")

[commit-bot: I haz the power, 2015-10-27 06:52:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1415413008/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1415413008/60001

[commit-bot: I haz the power, 2015-10-27 07:35:39]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-27 07:36:41]

Patchset 4 (id:??) landed as
https://crrev.com/8c6f10e481a5c2af50ac58d2971a171806015bc9
Cr-Commit-Position: refs/heads/master@{#356249}