Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2697683004:
Add automated testing to check for page size with scaling (Closed)
DescriptionAdd automated testing to check for page size with scaling
Ensure correct page size is still passed back with non default scaling
values - see bug 686384. Also, modify print web view helper to prevent
any rounding errors from converting dpi -> pixels -> dpi and scaling
and unscaling.
BUG=686384
Review-Url: https://codereview.chromium.org/2697683004
Cr-Commit-Position: refs/heads/master@{#452737}
Committed: https://chromium.googlesource.com/chromium/src/+/230c3f1c820ccb738e9715cd45b2b5e049d782d7
Patch Set 1 #Patch Set 2 : Fix git cl format issue #
Total comments: 27
Patch Set 3 : Address comments #
Total comments: 8
Patch Set 4 : Split verification fn #
Messages
Total messages: 31 (19 generated)
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add automated testing to check for page size with scaling Ensure correct page size is still passed back with non default scaling values - see bug 686384. Also, modify print web view helper to prevent any rounding errors from converting dpi -> pixels -> dpi and scaling and unscaling. BUG=None ========== to ========== Add automated testing to check for page size with scaling Ensure correct page size is still passed back with non default scaling values - see bug 686384. Also, modify print web view helper to prevent any rounding errors from converting dpi -> pixels -> dpi and scaling and unscaling. BUG=None ==========
rbpotter@chromium.org changed reviewers: + vitalybuka@chromium.org
rbpotter@chromium.org changed reviewers: + thestig@chromium.org - vitalybuka@chromium.org
Mostly nits. https://codereview.chromium.org/2697683004/diff/20001/components/printing/ren... File components/printing/renderer/print_web_view_helper.cc (right): https://codereview.chromium.org/2697683004/diff/20001/components/printing/ren... components/printing/renderer/print_web_view_helper.cc:1850: // Pull the original page size here. Windows uses the page_size_in_dpi s/Pull/Save/ ? "Windows uses |page_size_in_dpi| ..." https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... File components/printing/test/mock_printer.cc (right): https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... components/printing/test/mock_printer.cc:43: void UpdatePageSizeAndScaling(gfx::Size page_size, Pass by cosnt ref again. https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... components/printing/test/mock_printer.cc:181: if (page_size.width() > 0 && page_size.height() > 0) if (!page_size.IsEmpty()) https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... components/printing/test/mock_printer.cc:182: UpdatePageSizeAndScaling(page_size, scale_factor, &(params->params)); You can probably omit the parenthesis around |params->params|. https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... File components/printing/test/mock_printer.h (right): https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... components/printing/test/mock_printer.h:89: gfx::Size page_size, Pass by const ref? https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... File components/printing/test/print_mock_render_thread.cc (right): https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... components/printing/test/print_mock_render_thread.cc:168: const base::DictionaryValue* media_size_value = NULL; nullptr https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... components/printing/test/print_mock_render_thread.cc:169: gfx::Size page_size = gfx::Size(0, 0); The default constructor already sets the size of (0, 0). https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... components/printing/test/print_mock_render_thread.cc:178: float deviceMicronsPerUnit = device_microns_per_unit https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... File components/printing/test/print_web_view_helper_browsertest.cc (right): https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... components/printing/test/print_web_view_helper_browsertest.cc:240: gfx::Size page_size_received = Can this be a separate VerifyPrintedPageSize() function? Since the caller passes in |printed|, it should know whether to call just VerifyPagesPrinted(), or VerifyPagesPrinted() + VerifyPrintedPageSize(). Or is checking the page number not desired? https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... components/printing/test/print_web_view_helper_browsertest.cc:242: EXPECT_EQ(page_size_received.height(), page_size.height()); EXPECT_EQ(expected, actual) -> the arguments passed in are reversed. https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... components/printing/test/print_web_view_helper_browsertest.cc:995: // Tests that when printing non default scaling values the page size returned non-default comma after values https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... components/printing/test/print_web_view_helper_browsertest.cc:1006: gfx::Size page_size_in = gfx::Size(240, 240); Maybe pick different values for width/height? https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... components/printing/test/print_web_view_helper_browsertest.cc:1013: base::DictionaryValue* mediaSize = new base::DictionaryValue; auto dict = base::MakeUnique<base::DictionaryValue>(); |media_size|
And BUG=686384 in the CL description and just "see bug" instead of "see bug N."
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Description was changed from ========== Add automated testing to check for page size with scaling Ensure correct page size is still passed back with non default scaling values - see bug 686384. Also, modify print web view helper to prevent any rounding errors from converting dpi -> pixels -> dpi and scaling and unscaling. BUG=None ========== to ========== Add automated testing to check for page size with scaling Ensure correct page size is still passed back with non default scaling values - see bug 686384. Also, modify print web view helper to prevent any rounding errors from converting dpi -> pixels -> dpi and scaling and unscaling. BUG=686384 ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2697683004/diff/20001/components/printing/ren... File components/printing/renderer/print_web_view_helper.cc (right): https://codereview.chromium.org/2697683004/diff/20001/components/printing/ren... components/printing/renderer/print_web_view_helper.cc:1850: // Pull the original page size here. Windows uses the page_size_in_dpi On 2017/02/22 21:21:37, Lei Zhang (super slow) wrote: > s/Pull/Save/ ? > > "Windows uses |page_size_in_dpi| ..." Done. https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... File components/printing/test/mock_printer.cc (right): https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... components/printing/test/mock_printer.cc:43: void UpdatePageSizeAndScaling(gfx::Size page_size, On 2017/02/22 21:21:37, Lei Zhang (super slow) wrote: > Pass by cosnt ref again. Done. https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... components/printing/test/mock_printer.cc:181: if (page_size.width() > 0 && page_size.height() > 0) On 2017/02/22 21:21:37, Lei Zhang (super slow) wrote: > if (!page_size.IsEmpty()) Done. https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... components/printing/test/mock_printer.cc:182: UpdatePageSizeAndScaling(page_size, scale_factor, &(params->params)); On 2017/02/22 21:21:37, Lei Zhang (super slow) wrote: > You can probably omit the parenthesis around |params->params|. Done. https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... File components/printing/test/mock_printer.h (right): https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... components/printing/test/mock_printer.h:89: gfx::Size page_size, On 2017/02/22 21:21:37, Lei Zhang (super slow) wrote: > Pass by const ref? Done. https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... File components/printing/test/print_mock_render_thread.cc (right): https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... components/printing/test/print_mock_render_thread.cc:168: const base::DictionaryValue* media_size_value = NULL; On 2017/02/22 21:21:38, Lei Zhang (super slow) wrote: > nullptr Done. https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... components/printing/test/print_mock_render_thread.cc:169: gfx::Size page_size = gfx::Size(0, 0); On 2017/02/22 21:21:38, Lei Zhang (super slow) wrote: > The default constructor already sets the size of (0, 0). Done. https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... components/printing/test/print_mock_render_thread.cc:178: float deviceMicronsPerUnit = On 2017/02/22 21:21:38, Lei Zhang (super slow) wrote: > device_microns_per_unit Done. https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... File components/printing/test/print_web_view_helper_browsertest.cc (right): https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... components/printing/test/print_web_view_helper_browsertest.cc:240: gfx::Size page_size_received = On 2017/02/22 21:21:38, Lei Zhang (super slow) wrote: > Can this be a separate VerifyPrintedPageSize() function? Since the caller passes > in |printed|, it should know whether to call just VerifyPagesPrinted(), or > VerifyPagesPrinted() + VerifyPrintedPageSize(). Or is checking the page number > not desired? Done. https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... components/printing/test/print_web_view_helper_browsertest.cc:242: EXPECT_EQ(page_size_received.height(), page_size.height()); On 2017/02/22 21:21:38, Lei Zhang (super slow) wrote: > EXPECT_EQ(expected, actual) -> the arguments passed in are reversed. Done. https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... components/printing/test/print_web_view_helper_browsertest.cc:995: // Tests that when printing non default scaling values the page size returned On 2017/02/22 21:21:38, Lei Zhang (super slow) wrote: > non-default > > comma after values Done. https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... components/printing/test/print_web_view_helper_browsertest.cc:1006: gfx::Size page_size_in = gfx::Size(240, 240); On 2017/02/22 21:21:38, Lei Zhang (super slow) wrote: > Maybe pick different values for width/height? Done. https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... components/printing/test/print_web_view_helper_browsertest.cc:1013: base::DictionaryValue* mediaSize = new base::DictionaryValue; On 2017/02/22 21:21:38, Lei Zhang (super slow) wrote: > auto dict = base::MakeUnique<base::DictionaryValue>(); > > |media_size| Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... File components/printing/test/print_web_view_helper_browsertest.cc (right): https://codereview.chromium.org/2697683004/diff/20001/components/printing/tes... components/printing/test/print_web_view_helper_browsertest.cc:240: gfx::Size page_size_received = On 2017/02/23 22:27:11, rbpotter wrote: > On 2017/02/22 21:21:38, Lei Zhang (super slow) wrote: > > Can this be a separate VerifyPrintedPageSize() function? Since the caller > passes > > in |printed|, it should know whether to call just VerifyPagesPrinted(), or > > VerifyPagesPrinted() + VerifyPrintedPageSize(). Or is checking the page number > > not desired? > > Done. Looking at the code again, did patch set 3 get written that way because we can only call PrintHostMsg_DidPrintPage::Read() once on |print_msg| ? i.e. not possible to separate it the way I suggested. https://codereview.chromium.org/2697683004/diff/40001/components/printing/ren... File components/printing/renderer/print_web_view_helper.cc (right): https://codereview.chromium.org/2697683004/diff/40001/components/printing/ren... components/printing/renderer/print_web_view_helper.cc:1851: // actual page size, and rounding error is incurred by converting to pixels It might be clearer to say "Save the original page size here to avoid rounding errors incurred by..." because this may be interpreted as though we are going to incur a rounding error. https://codereview.chromium.org/2697683004/diff/40001/components/printing/tes... File components/printing/test/print_web_view_helper_browsertest.cc (right): https://codereview.chromium.org/2697683004/diff/40001/components/printing/tes... components/printing/test/print_web_view_helper_browsertest.cc:217: gfx::Size page_size, One more pass by const ref. https://codereview.chromium.org/2697683004/diff/40001/components/printing/tes... components/printing/test/print_web_view_helper_browsertest.cc:218: PrintHostMsg_DidPrintPage::Param post_did_print_page_param) { Make that 2? https://codereview.chromium.org/2697683004/diff/40001/components/printing/tes... components/printing/test/print_web_view_helper_browsertest.cc:221: EXPECT_EQ(page_size.height(), page_size_received.height()); Fun fact, gfx::Size integrates well with gtest. Assuming we have a dependency on the //ui/gfx:test_support target, which we probably already do, one can write: gfx::Size a(100, 200); gfx::Size b(300, 400); EXPECT_EQ(a, b); and at run time, that prints out: Failure Value of: b Actual: 300x400 Expected: a Which is: 100x200
I thought it wasn't possible to read off the IPC message twice, but apparently that works, so I switched it to be a completely separate function. Think that is closer to your first suggestion. https://codereview.chromium.org/2697683004/diff/40001/components/printing/ren... File components/printing/renderer/print_web_view_helper.cc (right): https://codereview.chromium.org/2697683004/diff/40001/components/printing/ren... components/printing/renderer/print_web_view_helper.cc:1851: // actual page size, and rounding error is incurred by converting to pixels On 2017/02/23 23:13:54, Lei Zhang (super slow) wrote: > It might be clearer to say "Save the original page size here to avoid rounding > errors incurred by..." because this may be interpreted as though we are going to > incur a rounding error. Done. https://codereview.chromium.org/2697683004/diff/40001/components/printing/tes... File components/printing/test/print_web_view_helper_browsertest.cc (right): https://codereview.chromium.org/2697683004/diff/40001/components/printing/tes... components/printing/test/print_web_view_helper_browsertest.cc:217: gfx::Size page_size, On 2017/02/23 23:13:54, Lei Zhang (super slow) wrote: > One more pass by const ref. Done. https://codereview.chromium.org/2697683004/diff/40001/components/printing/tes... components/printing/test/print_web_view_helper_browsertest.cc:218: PrintHostMsg_DidPrintPage::Param post_did_print_page_param) { On 2017/02/23 23:13:54, Lei Zhang (super slow) wrote: > Make that 2? Done. https://codereview.chromium.org/2697683004/diff/40001/components/printing/tes... components/printing/test/print_web_view_helper_browsertest.cc:221: EXPECT_EQ(page_size.height(), page_size_received.height()); On 2017/02/23 23:13:54, Lei Zhang (super slow) wrote: > Fun fact, gfx::Size integrates well with gtest. Assuming we have a dependency on > the //ui/gfx:test_support target, which we probably already do, one can write: > > gfx::Size a(100, 200); > gfx::Size b(300, 400); > EXPECT_EQ(a, b); > > and at run time, that prints out: > > Failure > Value of: b > Actual: 300x400 > Expected: a > Which is: 100x200 Done.
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/02/24 00:34:28, rbpotter wrote: > I thought it wasn't possible to read off the IPC message > twice, but apparently that works, so I switched it to be > a completely separate function. Think that is closer to > your first suggestion. I didn't know it was possible either, but now looking at ipc/ipc_message_templates_impl.h:33 I can see it is.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rbpotter@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487905959608410,
"parent_rev": "3c20e2d031c75de77442d5dbb6cca41adaf390f6", "commit_rev":
"230c3f1c820ccb738e9715cd45b2b5e049d782d7"}
Message was sent while issue was closed.
Description was changed from ========== Add automated testing to check for page size with scaling Ensure correct page size is still passed back with non default scaling values - see bug 686384. Also, modify print web view helper to prevent any rounding errors from converting dpi -> pixels -> dpi and scaling and unscaling. BUG=686384 ========== to ========== Add automated testing to check for page size with scaling Ensure correct page size is still passed back with non default scaling values - see bug 686384. Also, modify print web view helper to prevent any rounding errors from converting dpi -> pixels -> dpi and scaling and unscaling. BUG=686384 Review-Url: https://codereview.chromium.org/2697683004 Cr-Commit-Position: refs/heads/master@{#452737} Committed: https://chromium.googlesource.com/chromium/src/+/230c3f1c820ccb738e9715cd45b2... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/230c3f1c820ccb738e9715cd45b2...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2718513003/ by hajimehoshi@chromium.org. The reason for reverting is: Looks like this causes build bot errors: crbug.com/695760. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
