Use DPI from Print Preview on Windows, handle non square

Use DPI from Print Preview on Windows, handle non square

- On Windows, use DPI set in print preview to set the printer DPI. Print
  preview DPI was previously only used for extension/cloud printers.
- Fix handling of non-square DPI - use x and y DPI to compute the
  correct page size to report from the printer instead of assuming
  both resolutions are the same when computing page size. Assuming both
  are identical results in incorrect page sizes for non square DPI.
- Note that we have to use only 1 DPI for rendering in both
  dimensions, this CL just fixes the bad page size behavior.

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

Review-Url: https://codereview.chromium.org/2795453002
Cr-Commit-Position: refs/heads/master@{#461799}
Committed: https://chromium.googlesource.com/chromium/src/+/116c2e17ec19ced7c6700c34661017ee81d0d4ab

[rbpotter, 2017-03-31 19:26:34]

Description was changed from

==========
Use DPI from Print Preview on Windows, handle non square

- On Windows, use DPI set in print preview to set the printer DPI. Print
  preview DPI was previously only used for extension/cloud printers.
- Fix handling of non-square DPI - use x and y DPI to compute the
  correct page size to report from the printer instead of assuming
  both resolutions are the same when computing page size. Assuming both
  are identical results in incorrect page sizes for non square DPI.
- Note that we have to use only 1 DPI for rendering in both
  dimensions, this CL just fixes the bad page size behavior.

BUG=648774
==========

to

==========
Use DPI from Print Preview on Windows, handle non square

- On Windows, use DPI set in print preview to set the printer DPI. Print
  preview DPI was previously only used for extension/cloud printers.
- Fix handling of non-square DPI - use x and y DPI to compute the
  correct page size to report from the printer instead of assuming
  both resolutions are the same when computing page size. Assuming both
  are identical results in incorrect page sizes for non square DPI.
- Note that we have to use only 1 DPI for rendering in both
  dimensions, this CL just fixes the bad page size behavior.

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

[rbpotter, 2017-03-31 19:27:45]

Description was changed from

==========
Use DPI from Print Preview on Windows, handle non square

- On Windows, use DPI set in print preview to set the printer DPI. Print
  preview DPI was previously only used for extension/cloud printers.
- Fix handling of non-square DPI - use x and y DPI to compute the
  correct page size to report from the printer instead of assuming
  both resolutions are the same when computing page size. Assuming both
  are identical results in incorrect page sizes for non square DPI.
- Note that we have to use only 1 DPI for rendering in both
  dimensions, this CL just fixes the bad page size behavior.

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

to

==========
Use DPI from Print Preview on Windows, handle non square

- On Windows, use DPI set in print preview to set the printer DPI. Print
  preview DPI was previously only used for extension/cloud printers.
- Fix handling of non-square DPI - use x and y DPI to compute the
  correct page size to report from the printer instead of assuming
  both resolutions are the same when computing page size. Assuming both
  are identical results in incorrect page sizes for non square DPI.
- Note that we have to use only 1 DPI for rendering in both
  dimensions, this CL just fixes the bad page size behavior.

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

[rbpotter, 2017-03-31 19:27:45]

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

[Lei Zhang, 2017-03-31 22:21:13]

https://codereview.chromium.org/2795453002/diff/1/components/printing/browser...
File components/printing/browser/print_manager_utils.cc (right):

https://codereview.chromium.org/2795453002/diff/1/components/printing/browser...
components/printing/browser/print_manager_utils.cc:28: : 72;
Can we leave this file with only getter calls, and move the 72 DPI value into
printing/ ?

https://codereview.chromium.org/2795453002/diff/1/components/printing/rendere...
File components/printing/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/2795453002/diff/1/components/printing/rendere...
components/printing/renderer/print_web_view_helper.cc:293:
ConvertUnit(print_params.content_size.width(), dpi, kPointsPerInch);
By doing this, I don't think there are any more users of the desired_dpi field
in PrintMsg_Print_Params.

https://codereview.chromium.org/2795453002/diff/1/printing/print_settings.h
File printing/print_settings.h (left):

https://codereview.chromium.org/2795453002/diff/1/printing/print_settings.h#o...
printing/print_settings.h:186: // Desired visible dots per inch rendering for
output. Printing should be
Observation: From what I can tell, this was only used to tell the renderer how
to scale the image in ComputeWebKitPrintParamsInDesiredDpi(), and it was hard
coded to 72.

https://codereview.chromium.org/2795453002/diff/1/printing/print_settings.h
File printing/print_settings.h (right):

https://codereview.chromium.org/2795453002/diff/1/printing/print_settings.h#n...
printing/print_settings.h:194: // Desired visible dots per inch rendering for
output. Printing should be
Observation: Now it's being repurposed for something else, which is setting the
DPI when printing on Windows. In principle, |dpi_| below on line 236 should be
used for that, but I feel it probably gets used in many places, so this is being
used instead to avoid changing existing behavior.

https://codereview.chromium.org/2795453002/diff/1/printing/print_settings_ini...
File printing/print_settings_initializer_win.cc (right):

https://codereview.chromium.org/2795453002/diff/1/printing/print_settings_ini...
printing/print_settings_initializer_win.cc:116: // No printer device is known to
advertise different dpi in X and Y axis; even
Don't we have a printer that refutes this statement? Can we update the comment
to explain our new strategy?

https://codereview.chromium.org/2795453002/diff/1/printing/print_settings_ini...
printing/print_settings_initializer_win.cc:124: // DPI ratio should be an
integer.
As in, we don't expect a printer to have a DPI of 450x300.

[rbpotter, 2017-04-01 00:02:49]

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

[commit-bot: I haz the power, 2017-04-01 00:03:17]

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

[rbpotter, 2017-04-01 00:08:02]

https://codereview.chromium.org/2795453002/diff/1/components/printing/browser...
File components/printing/browser/print_manager_utils.cc (right):

https://codereview.chromium.org/2795453002/diff/1/components/printing/browser...
components/printing/browser/print_manager_utils.cc:28: : 72;
On 2017/03/31 22:21:13, Lei Zhang wrote:
> Can we leave this file with only getter calls, and move the 72 DPI value into
> printing/ ?

Removed this parameter entirely and used kPointsPerInch instead,
which is what this was actually used for.

https://codereview.chromium.org/2795453002/diff/1/components/printing/rendere...
File components/printing/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/2795453002/diff/1/components/printing/rendere...
components/printing/renderer/print_web_view_helper.cc:293:
ConvertUnit(print_params.content_size.width(), dpi, kPointsPerInch);
On 2017/03/31 22:21:13, Lei Zhang wrote:
> By doing this, I don't think there are any more users of the desired_dpi field
> in PrintMsg_Print_Params.

Acknowledged.

https://codereview.chromium.org/2795453002/diff/1/printing/print_settings.h
File printing/print_settings.h (left):

https://codereview.chromium.org/2795453002/diff/1/printing/print_settings.h#o...
printing/print_settings.h:186: // Desired visible dots per inch rendering for
output. Printing should be
On 2017/03/31 22:21:13, Lei Zhang wrote:
> Observation: From what I can tell, this was only used to tell the renderer how
> to scale the image in ComputeWebKitPrintParamsInDesiredDpi(), and it was hard
> coded to 72.

Removed it.

https://codereview.chromium.org/2795453002/diff/1/printing/print_settings.h
File printing/print_settings.h (right):

https://codereview.chromium.org/2795453002/diff/1/printing/print_settings.h#n...
printing/print_settings.h:194: // Desired visible dots per inch rendering for
output. Printing should be
On 2017/03/31 22:21:13, Lei Zhang wrote:
> Observation: Now it's being repurposed for something else, which is setting
the
> DPI when printing on Windows. In principle, |dpi_| below on line 236 should be
> used for that, but I feel it probably gets used in many places, so this is
being
> used instead to avoid changing existing behavior.

I think just using DPI is working. I added some functions so that we
can still used dpi the way it was before (as if it is the same in
both dimensions) but it actually stores the two dimensional dpi from
print preview if the user selected one. If not, it just starts out at
zero until we fetch the default value from the printer.

https://codereview.chromium.org/2795453002/diff/1/printing/print_settings_ini...
File printing/print_settings_initializer_win.cc (right):

https://codereview.chromium.org/2795453002/diff/1/printing/print_settings_ini...
printing/print_settings_initializer_win.cc:116: // No printer device is known to
advertise different dpi in X and Y axis; even
On 2017/03/31 22:21:13, Lei Zhang wrote:
> Don't we have a printer that refutes this statement? Can we update the comment
> to explain our new strategy?

Done

https://codereview.chromium.org/2795453002/diff/1/printing/print_settings_ini...
printing/print_settings_initializer_win.cc:124: // DPI ratio should be an
integer.
On 2017/03/31 22:21:13, Lei Zhang wrote:
> As in, we don't expect a printer to have a DPI of 450x300.

Actually fixed this. I have not seen any non-integer examples, but
realized I probably shouldn't assume that is the case.

https://codereview.chromium.org/2795453002/diff/20001/printing/print_settings.h
File printing/print_settings.h (right):

https://codereview.chromium.org/2795453002/diff/20001/printing/print_settings...
printing/print_settings.h:107: int dpi() const { return std::min(dpi_[0],
dpi_[1]); }
What do you think makes sense here? The previous idea was to use the
horizontal dpi always (so dpi_[0]). We could do that, or min, or max of
the two dimensions. Not sure what makes the most sense.

[commit-bot: I haz the power, 2017-04-01 00:53:49]

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

[commit-bot: I haz the power, 2017-04-01 00:53:49]

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

[Lei Zhang, 2017-04-01 00:55:44]

There are some trybot failures.

We may want to remove desired_dpi first in a separate CL. That may make for 2
simpler CLs.

[Lei Zhang, 2017-04-01 01:14:13]

https://codereview.chromium.org/2795453002/diff/20001/printing/print_settings.h
File printing/print_settings.h (right):

https://codereview.chromium.org/2795453002/diff/20001/printing/print_settings...
printing/print_settings.h:107: int dpi() const { return std::min(dpi_[0],
dpi_[1]); }
On 2017/04/01 00:08:02, rbpotter wrote:
> What do you think makes sense here? The previous idea was to use the
> horizontal dpi always (so dpi_[0]). We could do that, or min, or max of
> the two dimensions. Not sure what makes the most sense.

Well, for most printers, it doesn't matter right? I feel like this should be min
or max. Not sure which one makes the printer with different DPIs happier though.

[rbpotter, 2017-04-03 17:49:12]

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

[commit-bot: I haz the power, 2017-04-03 17:49: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, 2017-04-03 18:00:05]

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

[commit-bot: I haz the power, 2017-04-03 18:00:06]

Dry run: Try jobs failed on following builders:
  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_...)

[rbpotter, 2017-04-03 18:38:46]

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

[commit-bot: I haz the power, 2017-04-03 18:39:24]

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-04-03 20:23:05]

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

[commit-bot: I haz the power, 2017-04-03 20:23:05]

Dry run: This issue passed the CQ dry run.

[rbpotter, 2017-04-03 22:00:47]

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

[commit-bot: I haz the power, 2017-04-03 22:01: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, 2017-04-03 23:10:06]

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

[commit-bot: I haz the power, 2017-04-03 23:10:07]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2017-04-04 00:03:04]

lgtm

https://codereview.chromium.org/2795453002/diff/80001/printing/print_settings.h
File printing/print_settings.h (right):

https://codereview.chromium.org/2795453002/diff/80001/printing/print_settings...
printing/print_settings.h:106: void set_dpi(int dpi_horizontal, int
dpi_vertical) {
Can we name this something else to avoid overloading set_dpi()?

https://codereview.chromium.org/2795453002/diff/80001/printing/print_settings...
printing/print_settings.h:167: int dpi_horizontal() const { return dpi_[0]; }
Can we keep these grouped with dpi() above?

https://codereview.chromium.org/2795453002/diff/80001/printing/print_settings...
printing/print_settings.h:231: // Printer's device effective dots per inch in
both axis.
Can we add some notes to say the two values can only differ on Windows, and even
then it's rare?

[rbpotter, 2017-04-04 00:36:26]

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

[rbpotter, 2017-04-04 00:36:34]

https://codereview.chromium.org/2795453002/diff/80001/printing/print_settings.h
File printing/print_settings.h (right):

https://codereview.chromium.org/2795453002/diff/80001/printing/print_settings...
printing/print_settings.h:106: void set_dpi(int dpi_horizontal, int
dpi_vertical) {
On 2017/04/04 00:03:03, Lei Zhang wrote:
> Can we name this something else to avoid overloading set_dpi()?

Done.

https://codereview.chromium.org/2795453002/diff/80001/printing/print_settings...
printing/print_settings.h:167: int dpi_horizontal() const { return dpi_[0]; }
On 2017/04/04 00:03:03, Lei Zhang wrote:
> Can we keep these grouped with dpi() above?

Done.

https://codereview.chromium.org/2795453002/diff/80001/printing/print_settings...
printing/print_settings.h:231: // Printer's device effective dots per inch in
both axis.
On 2017/04/04 00:03:03, Lei Zhang wrote:
> Can we add some notes to say the two values can only differ on Windows, and
even
> then it's rare?

Done.

[commit-bot: I haz the power, 2017-04-04 00:37:06]

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-04-04 02:17:49]

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

[commit-bot: I haz the power, 2017-04-04 02:17:51]

Dry run: This issue passed the CQ dry run.

[rbpotter, 2017-04-04 19:01:34]

The CQ bit was checked by rbpotter@chromium.org

[rbpotter, 2017-04-04 19:01:34]

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

[commit-bot: I haz the power, 2017-04-04 19:03:24]

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-04-04 19:21:31]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491332494221920,
"parent_rev": "dd463aa3da242c236addb4fdf26ee4500bd429f5", "commit_rev":
"116c2e17ec19ced7c6700c34661017ee81d0d4ab"}

[commit-bot: I haz the power, 2017-04-04 19:22:46]

Description was changed from

==========
Use DPI from Print Preview on Windows, handle non square

- On Windows, use DPI set in print preview to set the printer DPI. Print
  preview DPI was previously only used for extension/cloud printers.
- Fix handling of non-square DPI - use x and y DPI to compute the
  correct page size to report from the printer instead of assuming
  both resolutions are the same when computing page size. Assuming both
  are identical results in incorrect page sizes for non square DPI.
- Note that we have to use only 1 DPI for rendering in both
  dimensions, this CL just fixes the bad page size behavior.

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

to

==========
Use DPI from Print Preview on Windows, handle non square

- On Windows, use DPI set in print preview to set the printer DPI. Print
  preview DPI was previously only used for extension/cloud printers.
- Fix handling of non-square DPI - use x and y DPI to compute the
  correct page size to report from the printer instead of assuming
  both resolutions are the same when computing page size. Assuming both
  are identical results in incorrect page sizes for non square DPI.
- Note that we have to use only 1 DPI for rendering in both
  dimensions, this CL just fixes the bad page size behavior.

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

Review-Url: https://codereview.chromium.org/2795453002
Cr-Commit-Position: refs/heads/master@{#461799}
Committed:
https://chromium.googlesource.com/chromium/src/+/116c2e17ec19ced7c6700c346610...
==========

[commit-bot: I haz the power, 2017-04-04 19:22:47]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/116c2e17ec19ced7c6700c346610...