Printing: Move some printing code out of pdf/ and into printing/.

Printing: Move some printing code out of pdf/ and into printing/.

This code was stuck in pdf/ previously because the PDF plugin was
proprietary.

Add unit tests now that the code has been moved out.

Committed: https://crrev.com/64c8e26697caef0ef26cf604da6747408efd7526
Cr-Commit-Position: refs/heads/master@{#356600}

[Lei Zhang, 2015-10-21 02:39:58]

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

[Lei Zhang, 2015-10-21 03:42:09]

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

[commit-bot: I haz the power, 2015-10-21 03:42:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1421493002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1421493002/1

[commit-bot: I haz the power, 2015-10-21 03:53:11]

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

[commit-bot: I haz the power, 2015-10-21 03:53:11]

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

[raymes, 2015-10-26 04:04:48]

looks good overall, a few minor comments.

https://codereview.chromium.org/1421493002/diff/20001/printing/pdf_transform.cc
File printing/pdf_transform.cc (right):

https://codereview.chromium.org/1421493002/diff/20001/printing/pdf_transform....
printing/pdf_transform.cc:18: if (src_width == 0 || src_height == 0)
I noticed you pulled out the first parameter of this function and the first part
of CalculateClipBoxBoundary but are there any other changes to these functions
that I should review?

https://codereview.chromium.org/1421493002/diff/20001/printing/printing.gyp
File printing/printing.gyp (right):

https://codereview.chromium.org/1421493002/diff/20001/printing/printing.gyp#n...
printing/printing.gyp:241: 'sources': ['pdf_transform_unittest.cc']
Is this missing?

[Lei Zhang, 2015-10-26 04:39:26]

Please note the CL description has changed and this now fixes a bug.

https://codereview.chromium.org/1421493002/diff/20001/printing/pdf_transform.cc
File printing/pdf_transform.cc (right):

https://codereview.chromium.org/1421493002/diff/20001/printing/pdf_transform....
printing/pdf_transform.cc:18: if (src_width == 0 || src_height == 0)
On 2015/10/26 04:04:48, raymes wrote:
> I noticed you pulled out the first parameter of this function and the first
part
> of CalculateClipBoxBoundary but are there any other changes to these functions
> that I should review?

Most of the changes are just refactorings. I'll comment on the non-trivial bits.

https://codereview.chromium.org/1421493002/diff/20001/printing/printing.gyp
File printing/printing.gyp (right):

https://codereview.chromium.org/1421493002/diff/20001/printing/printing.gyp#n...
printing/printing.gyp:241: 'sources': ['pdf_transform_unittest.cc']
On 2015/10/26 04:04:48, raymes wrote:
> Is this missing?

yes, added.

https://codereview.chromium.org/1421493002/diff/60001/printing/pdf_transform.cc
File printing/pdf_transform.cc (right):

https://codereview.chromium.org/1421493002/diff/60001/printing/pdf_transform....
printing/pdf_transform.cc:14: double CalculateScaleFactor(const gfx::Rect&
content_rect,
This has been simplified by removing |scale_to_fit|. It's done by the caller
now.

https://codereview.chromium.org/1421493002/diff/60001/printing/pdf_transform....
printing/pdf_transform.cc:40: void CalculateMediaBoxAndCropBox(bool rotated,
This is new. It's split off from CalculateClipBoxBoundary() to fix bug 543304.

https://codereview.chromium.org/1421493002/diff/60001/printing/pdf_transform....
printing/pdf_transform.cc:56: ClipBox CalculateClipBoxBoundary(const ClipBox&
media_box,
This has been simplified. The rest of the function is in
CalculateMediaBoxAndCropBox() above.

https://codereview.chromium.org/1421493002/diff/60001/printing/pdf_transform....
printing/pdf_transform.cc:72: void ScaleClipBox(double scale_factor, ClipBox*
box) {
Split this out into its own function.

https://codereview.chromium.org/1421493002/diff/60001/printing/pdf_transform....
printing/pdf_transform.cc:79: void CalculateScaledClipBoxOffset(const gfx::Rect&
content_rect,
No changes.

https://codereview.chromium.org/1421493002/diff/60001/printing/pdf_transform....
printing/pdf_transform.cc:93: void CalculateNonScaledClipBoxOffset(const
gfx::Rect& content_rect,
No changes here.

[raymes, 2015-10-27 04:03:20]

Sorry I've been slow on this, I've been a bit swamped :( Would it be possible to
make the functional changes in the first CL (and perhaps add the test) and move
the code out in a separate CL (in either order). That would help make the diff
more useful.

[Lei Zhang, 2015-10-27 04:25:15]

On 2015/10/27 04:03:20, raymes wrote:
> Sorry I've been slow on this, I've been a bit swamped :( Would it be possible
to
> make the functional changes in the first CL (and perhaps add the test) and
move
> the code out in a separate CL (in either order). That would help make the diff
> more useful.

Sure. No rush. Let me do a fix CL first, so we can merge that to M47.

Then I'll come back here and split the code.

[Lei Zhang, 2015-10-28 00:54:38]

Description was changed from

==========
Printing: Move some printing code out of pdf/ and into printing/.

This code was stuck in pdf/ previously because the PDF plugin was
proprietary.

Add unit tests, simplify functions, and fix bugs.

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

to

==========
Printing: Move some printing code out of pdf/ and into printing/.

This code was stuck in pdf/ previously because the PDF plugin was
proprietary.

Add unit tests now that the code has been moved out.
==========

[Lei Zhang, 2015-10-28 00:56:24]

And now we are back to this CL. It should be just a simple move.

[raymes, 2015-10-28 02:26:19]

lgtm

https://codereview.chromium.org/1421493002/diff/100001/printing/pdf_transform.cc
File printing/pdf_transform.cc (right):

https://codereview.chromium.org/1421493002/diff/100001/printing/pdf_transform...
printing/pdf_transform.cc:78: void CalculateScaledClipBoxOffset(const gfx::Rect&
content_rect,
To be honest I don't really understand the purpose of this function and the one
below, but the test looks ok.

https://codereview.chromium.org/1421493002/diff/100001/printing/pdf_transform...
File printing/pdf_transform_unittest.cc (right):

https://codereview.chromium.org/1421493002/diff/100001/printing/pdf_transform...
printing/pdf_transform_unittest.cc:127: ExpectDefaultLandscapeBox(media_box);
nit: perhaps use a non-default size of media box to ensure that it's not hitting
the default size code?

[Lei Zhang, 2015-10-28 05:47:23]

https://codereview.chromium.org/1421493002/diff/100001/printing/pdf_transform.cc
File printing/pdf_transform.cc (right):

https://codereview.chromium.org/1421493002/diff/100001/printing/pdf_transform...
printing/pdf_transform.cc:78: void CalculateScaledClipBoxOffset(const gfx::Rect&
content_rect,
On 2015/10/28 02:26:19, raymes wrote:
> To be honest I don't really understand the purpose of this function and the
one
> below, but the test looks ok.

They calculate how far the PDF's rect needs to be moved to fit on paper
"properly". These were written long ago by kmadhusu@ IIRC. I just want them to
at least get exercised by tests.

https://codereview.chromium.org/1421493002/diff/100001/printing/pdf_transform...
File printing/pdf_transform_unittest.cc (right):

https://codereview.chromium.org/1421493002/diff/100001/printing/pdf_transform...
printing/pdf_transform_unittest.cc:127: ExpectDefaultLandscapeBox(media_box);
On 2015/10/28 02:26:19, raymes wrote:
> nit: perhaps use a non-default size of media box to ensure that it's not
hitting
> the default size code?

Done.

[raymes, 2015-10-28 05:52:53]

https://codereview.chromium.org/1421493002/diff/100001/printing/pdf_transform.cc
File printing/pdf_transform.cc (right):

https://codereview.chromium.org/1421493002/diff/100001/printing/pdf_transform...
printing/pdf_transform.cc:78: void CalculateScaledClipBoxOffset(const gfx::Rect&
content_rect,
Ah yes sorry, I wasn't trying to imply it was your fault. Thank you for adding
tests!!

[Lei Zhang, 2015-10-28 06:02:33]

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

[Lei Zhang, 2015-10-28 06:02:33]

+danakj for ui/OWNERS for pdf/DEPS.

[Lei Zhang, 2015-10-28 06:03:54]

https://codereview.chromium.org/1421493002/diff/100001/printing/pdf_transform.cc
File printing/pdf_transform.cc (right):

https://codereview.chromium.org/1421493002/diff/100001/printing/pdf_transform...
printing/pdf_transform.cc:78: void CalculateScaledClipBoxOffset(const gfx::Rect&
content_rect,
On 2015/10/28 05:52:53, raymes wrote:
> Ah yes sorry, I wasn't trying to imply it was your fault. Thank you for adding
> tests!!

I didn't think you implied that. I just wanted to mention where the code came
from since I knew.

[danakj, 2015-10-28 18:15:40]

deps LGTM

[danakj, 2015-10-28 18:16:04]

https://codereview.chromium.org/1421493002/diff/120001/pdf/DEPS
File pdf/DEPS (right):

https://codereview.chromium.org/1421493002/diff/120001/pdf/DEPS#newcode10
pdf/DEPS:10: "+ui/gfx/geometry/rect.h",
you might wanna include all of geometry/.

[Lei Zhang, 2015-10-28 18:50:26]

https://codereview.chromium.org/1421493002/diff/120001/pdf/DEPS
File pdf/DEPS (right):

https://codereview.chromium.org/1421493002/diff/120001/pdf/DEPS#newcode10
pdf/DEPS:10: "+ui/gfx/geometry/rect.h",
On 2015/10/28 18:16:04, danakj wrote:
> you might wanna include all of geometry/.

This code changes very slowly. I'll bug you again when we need it. :-P

[Lei Zhang, 2015-10-28 18:50:33]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2015-10-28 18:50:33]

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/1421493002/#ps120001
(title: "address comments")

[commit-bot: I haz the power, 2015-10-28 18:52:22]

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

[commit-bot: I haz the power, 2015-10-28 19:04:36]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-10-28 19:05:55]

Patchset 7 (id:??) landed as
https://crrev.com/64c8e26697caef0ef26cf604da6747408efd7526
Cr-Commit-Position: refs/heads/master@{#356600}