Don't let rounding prematurely influence document size when printing

Don't let rounding prematurely influence document size when printing

Don't round away subpixel values when they can still make a difference to the
size of the document. We convert back and forth between different DPIs so we
can lose a pixel or two along the way if we don't retain subpixel values to
the end.

BUG=467579
Committed: https://crrev.com/53c779cbb1a137fd5781422c67f36c3354382bc8
Cr-Commit-Position: refs/heads/master@{#409469}

[rhogan, 2016-07-05 18:32:15]

Description was changed from

==========
Don't let rounding prematurely influence document size when printing

BUG=467579
==========

to

==========
Don't let rounding prematurely influence document size when printing

Don't round away subpixel values when they can still make a difference to the
size of the document. We convert back and forth between different DPIs so we
can lose a pixel or two along the way if we don't retain subpixel values to
the end.

BUG=467579
==========

[rhogan, 2016-07-05 18:32:39]

robhogan@gmail.com changed reviewers:
+ thestig@chromium.org

[Lei Zhang, 2016-07-06 18:26:45]

UnitsTest.Convertions is failing. I'm trying to do some code archaeology to
figure out why the failing test case got added, and who misspelled
"conversions".

https://codereview.chromium.org/2116283002/diff/40001/components/printing/ren...
File components/printing/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/2116283002/diff/40001/components/printing/ren...
components/printing/renderer/print_web_view_helper.cc:170:
ConvertUnit(page_size_in_pixels.height,
Just run: clang-format -i -style=Chromium $file, and call it a day.

https://codereview.chromium.org/2116283002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebFrame.h (right):

https://codereview.chromium.org/2116283002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrame.h:511: WebFloatSize& pageSize,
nit: Is this the correct style for Blink?

[Lei Zhang, 2016-07-06 18:46:43]

So does printing::ConvertUnit() actually need to take a double? If not, leave it
alone and I can deal with it separately? IMO the failing test is a bad test.

[rhogan, 2016-07-07 21:53:18]

On 2016/07/06 at 18:46:43, thestig wrote:
> So does printing::ConvertUnit() actually need to take a double? If not, leave
it alone and I can deal with it separately? IMO the failing test is a bad test.

It does - I need to to include the subpixel values when converting.

[Lei Zhang, 2016-07-07 22:43:36]

On 2016/07/07 21:53:18, rhogan wrote:
> On 2016/07/06 at 18:46:43, thestig wrote:
> > So does printing::ConvertUnit() actually need to take a double? If not,
leave
> it alone and I can deal with it separately? IMO the failing test is a bad
test.
> 
> It does - I need to to include the subpixel values when converting.

Can you add some test code to exercise ConvertUnit() for subpixel values? BTW,
I'm fine with removing the 4000000000U case.

Also, you probably need to rebase again so your CL applies and it can go through
the trybots. I think dcheng@ had a CL that conflicted.

[Lei Zhang, 2016-07-07 22:46:59]

... otherwise I think this is looking good.

[rhogan, 2016-07-11 18:14:09]

On 2016/07/07 at 22:46:59, thestig wrote:
> ... otherwise I think this is looking good.

Ready for another look now I think!

[Lei Zhang, 2016-07-12 17:02:24]

non Blink bits lgtm

https://codereview.chromium.org/2116283002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrame.h (right):

https://codereview.chromium.org/2116283002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrame.h:88: struct WebFloatSize;
Move to WebLocalFrame.h?

[rhogan, 2016-07-12 21:52:24]

robhogan@gmail.com changed reviewers:
+ eae@chromium.org

[rhogan, 2016-07-12 21:52:25]

eae@ - could you give it a look for blink?

[eae, 2016-07-12 22:26:39]

This does not look correct to me. Pages should always have a size in
full-pixels.

https://codereview.chromium.org/2116283002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2116283002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:1998: float width =
pageSize.width();
This does not look right. Pages are always in full pixels. Even if that hadn't
been the case we should not use floats for sizes in layout. That's what
LayoutUnit is for.

https://codereview.chromium.org/2116283002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/2116283002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutView.cpp:837:
floorToInt(overflowRect.width()),
This is incorrect. We do not floor sizes, we pixel snap them.

[rhogan, 2016-07-13 18:27:13]

https://codereview.chromium.org/2116283002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2116283002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:1998: float width =
pageSize.width();
On 2016/07/12 at 22:26:39, eae wrote:
> This does not look right. Pages are always in full pixels. Even if that hadn't
been the case we should not use floats for sizes in layout. That's what
LayoutUnit is for.

The main user of this API is print_web_view_helper.cc, and it does need to
consume the un-snapped value so that it can scale it without losing accuracy.
Maybe I should rename the method to pageSizeAndMarginsInSubpixels?

https://codereview.chromium.org/2116283002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/2116283002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutView.cpp:837:
floorToInt(overflowRect.width()),
On 2016/07/12 at 22:26:39, eae wrote:
> This is incorrect. We do not floor sizes, we pixel snap them.

Yes, this is wrong.

https://codereview.chromium.org/2116283002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/PrintContext.cpp (right):

https://codereview.chromium.org/2116283002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/PrintContext.cpp:300: return "(" +
String::number(floor(pageSize.width())) + ", " +
String::number(floor(pageSize.height())) + ") " +
This is the other user, exposed via V8 bindings. I will snap to pixels here, as
you say flooring is wrong.

[eae, 2016-07-13 20:42:26]

https://codereview.chromium.org/2116283002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2116283002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:1998: float width =
pageSize.width();
On 2016/07/13 18:27:13, rhogan wrote:
> On 2016/07/12 at 22:26:39, eae wrote:
> > This does not look right. Pages are always in full pixels. Even if that
hadn't
> been the case we should not use floats for sizes in layout. That's what
> LayoutUnit is for.
> 
> The main user of this API is print_web_view_helper.cc, and it does need to
> consume the un-snapped value so that it can scale it without losing accuracy.
> Maybe I should rename the method to pageSizeAndMarginsInSubpixels?

In that case we need to use either LayoutUnits or doubles. Floats are not
precise enough.

[rhogan, 2016-07-17 18:40:14]

On 2016/07/13 at 20:42:26, eae wrote:
> 
> In that case we need to use either LayoutUnits or doubles. Floats are not
precise enough.

OK, can you take another look?

[eae, 2016-07-18 17:00:41]

LGTM

Thank you!

[Lei Zhang, 2016-07-18 18:41:37]

On 2016/07/18 17:00:41, eae wrote:
> LGTM
> 
> Thank you!

++lgtm
++thanks

[rhogan, 2016-07-18 21:29:26]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-18 21:30:57]

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-07-18 23:22:09]

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

[commit-bot: I haz the power, 2016-07-18 23:22:10]

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_...)

[rhogan, 2016-07-20 07:02:50]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2016-07-20 07:02:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2116283002/#ps200001
(title: "bug 467579")

[commit-bot: I haz the power, 2016-07-20 07:03:10]

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-07-20 07:14:33]

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

[commit-bot: I haz the power, 2016-07-20 07:14:34]

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...)

[rhogan, 2016-07-20 20:12:40]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2016-07-20 20:12:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2116283002/#ps220001
(title: "bug 467579")

[commit-bot: I haz the power, 2016-07-20 20:13:21]

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-07-20 20:22:23]

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

[commit-bot: I haz the power, 2016-07-20 20:22:25]

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...)

[Lei Zhang, 2016-07-20 20:26:11]

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

[Lei Zhang, 2016-07-20 20:26:12]

Looks like we need more OWNERS? +dglazkov

[rhogan, 2016-07-22 13:38:55]

robhogan@gmail.com changed reviewers:
+ pdr@chromium.org

[rhogan, 2016-07-22 13:38:57]

pdr: have you got time to take a look?

[dglazkov, 2016-07-22 14:25:24]

lgtm

[rhogan, 2016-07-22 14:48:18]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2016-07-22 14:48:33]

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-07-22 16:10:15]

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

[commit-bot: I haz the power, 2016-07-22 16:10:17]

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_...)

[pdr., 2016-07-22 16:34:19]

Why are the non-printing pixel tests different? Particularly zoom-hixie-mixed; I
wouldn't have expected that to change from a printing change, and it's now
different from firefox.

[esprehn, 2016-07-22 17:03:07]

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

[esprehn, 2016-07-22 17:03:09]

Can we just pixel inflate instead? We usually just round up. Please fix comments
before landing if we do this though.

https://codereview.chromium.org/2116283002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/2116283002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.h:464: void
pageSizeAndMarginsInPixels(int pageIndex, DoubleSize& pageSize, int& marginTop,
int& marginRight, int& marginBottom, int& marginLeft);
Does this file include DoubleSize.h ?

https://codereview.chromium.org/2116283002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/2116283002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.h:43: #include
"public/platform/WebDoubleSize.h"
Why add this here if nothing uses it?

https://codereview.chromium.org/2116283002/diff/220001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebDoubleSize.h (right):

https://codereview.chromium.org/2116283002/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebDoubleSize.h:2: * Copyright (C) 2013
Google Inc. All rights reserved.
Wrong year also please use the short copyright all new files use not this big
one.

https://codereview.chromium.org/2116283002/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebDoubleSize.h:36: #include <algorithm>
Why do you need algorithm?

https://codereview.chromium.org/2116283002/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebDoubleSize.h:41: #include <cmath>
Why do you need cmath?

https://codereview.chromium.org/2116283002/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebDoubleSize.h:48: struct WebDoubleSize {
class and then put the fields at the bottom

https://codereview.chromium.org/2116283002/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebDoubleSize.h:60: WebDoubleSize(float
width, float height)
Why does the constructor not take doubles?

https://codereview.chromium.org/2116283002/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebDoubleSize.h:73: WebDoubleSize&
operator=(const DoubleSize& s)
Don't abbreviate, size

[rhogan, 2016-07-23 10:58:59]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-23 10:59:07]

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-07-23 11:10:05]

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

[commit-bot: I haz the power, 2016-07-23 11:10:06]

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

[rhogan, 2016-07-23 11:21:59]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-23 11:22:05]

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-07-23 12:30:23]

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

[commit-bot: I haz the power, 2016-07-23 12:30:24]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rhogan, 2016-07-24 12:02:55]

On 2016/07/22 at 17:03:09, esprehn wrote:
> Can we just pixel inflate instead? We usually just round up. Please fix
comments before landing if we do this though.
I don't think so, unless I've misunderstood what you mean. This is about losing
fractional values when we convert from pixels to a dpi system then back again.
We need to keep the precision all the way through so we don't drop a pixel.

I've applied your other comments!

[rhogan, 2016-07-24 12:05:25]

On 2016/07/22 at 16:34:19, pdr wrote:
> Why are the non-printing pixel tests different? Particularly zoom-hixie-mixed;
I wouldn't have expected that to change from a printing change, and it's now
different from firefox.

Yup, that was cruft from the trybots I applied unthinkingly!  :/ There are no
differences to apply here!

[rhogan, 2016-07-24 12:05:45]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-24 12:05:58]

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-07-24 16:11:08]

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

[commit-bot: I haz the power, 2016-07-24 16:11:09]

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_...)

[pdr., 2016-07-25 16:09:26]

On 2016/07/24 at 12:05:25, robhogan wrote:
> On 2016/07/22 at 16:34:19, pdr wrote:
> > Why are the non-printing pixel tests different? Particularly
zoom-hixie-mixed; I wouldn't have expected that to change from a printing
change, and it's now different from firefox.
> 
> Yup, that was cruft from the trybots I applied unthinkingly!  :/ There are no
differences to apply here!

LGTM

[rhogan, 2016-07-27 12:43:45]

On 2016/07/24 at 12:02:55, rhogan wrote:
> On 2016/07/22 at 17:03:09, esprehn wrote:
> > Can we just pixel inflate instead? We usually just round up. Please fix
comments before landing if we do this though.
> I don't think so, unless I've misunderstood what you mean. This is about
losing fractional values when we convert from pixels to a dpi system then back
again. We need to keep the precision all the way through so we don't drop a
pixel.
> 
> I've applied your other comments!

Hi esprehn, got time to take another look?

[rhogan, 2016-07-27 12:43:47]

On 2016/07/24 at 12:02:55, rhogan wrote:
> On 2016/07/22 at 17:03:09, esprehn wrote:
> > Can we just pixel inflate instead? We usually just round up. Please fix
comments before landing if we do this though.
> I don't think so, unless I've misunderstood what you mean. This is about
losing fractional values when we convert from pixels to a dpi system then back
again. We need to keep the precision all the way through so we don't drop a
pixel.
> 
> I've applied your other comments!

Hi esprehn, got time to take another look?

[rhogan, 2016-07-29 14:18:58]

On 2016/07/27 at 12:43:47, rhogan wrote:
> On 2016/07/24 at 12:02:55, rhogan wrote:
> > On 2016/07/22 at 17:03:09, esprehn wrote:
> > > Can we just pixel inflate instead? We usually just round up. Please fix
comments before landing if we do this though.
> > I don't think so, unless I've misunderstood what you mean. This is about
losing fractional values when we convert from pixels to a dpi system then back
again. We need to keep the precision all the way through so we don't drop a
pixel.
> > 
> > I've applied your other comments!
> 
> Hi esprehn, got time to take another look?

Hi esprehn - OK for me to land?

[esprehn, 2016-08-02 18:01:59]

I hope we can move the printing code into blink eventually and stop with all the
plumbing all over :)

Lgtm

[rhogan, 2016-08-02 18:02:49]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2016-08-02 18:02:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, dglazkov@chromium.org, eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2116283002/#ps280001
(title: "bug 467579")

[commit-bot: I haz the power, 2016-08-02 18:03:39]

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-08-02 18:07:42]

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

[commit-bot: I haz the power, 2016-08-02 18:07:44]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rhogan, 2016-08-02 20:57:10]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-02 20:58:19]

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-08-02 22:53:27]

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

[commit-bot: I haz the power, 2016-08-02 22:53:28]

Dry run: This issue passed the CQ dry run.

[rhogan, 2016-08-03 06:22:59]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2016-08-03 06:22:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, pdr@chromium.org, esprehn@chromium.org,
dglazkov@chromium.org, eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2116283002/#ps300001
(title: "bug 467579")

[commit-bot: I haz the power, 2016-08-03 06:23:17]

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-08-03 06:35:10]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-08-03 06:36:39]

Description was changed from

==========
Don't let rounding prematurely influence document size when printing

Don't round away subpixel values when they can still make a difference to the
size of the document. We convert back and forth between different DPIs so we
can lose a pixel or two along the way if we don't retain subpixel values to
the end.

BUG=467579
==========

to

==========
Don't let rounding prematurely influence document size when printing

Don't round away subpixel values when they can still make a difference to the
size of the document. We convert back and forth between different DPIs so we
can lose a pixel or two along the way if we don't retain subpixel values to
the end.

BUG=467579

Committed: https://crrev.com/53c779cbb1a137fd5781422c67f36c3354382bc8
Cr-Commit-Position: refs/heads/master@{#409469}
==========

[commit-bot: I haz the power, 2016-08-03 06:36:41]

Patchset 16 (id:??) landed as
https://crrev.com/53c779cbb1a137fd5781422c67f36c3354382bc8
Cr-Commit-Position: refs/heads/master@{#409469}