Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2961773003:
Print Preview: Update test to check parameters sent to print() (Closed)
DescriptionPrint Preview: Update test to check parameters sent to print()
Update the native layer stub to pass on the parameters sent to print(),
and update the InvalidSettings test to sanity check some of these
parameters to verify print is being called correctly.
BUG=717296
Review-Url: https://codereview.chromium.org/2961773003
Cr-Commit-Position: refs/heads/master@{#483021}
Committed: https://chromium.googlesource.com/chromium/src/+/6c220aac9f218d0274a6bed442cce6211599811a
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address comments #
Total comments: 8
Patch Set 3 : Address comments #
Messages
Total messages: 23 (16 generated)
Description was changed from ========== Print Preview: Update test to check parameters sent to print() Update the native layer stub to pass on the parameters sent to print(), and update the InvalidSettings test to sanity check some of these parameters to verify print is being called correctly. BUG=717296 ========== to ========== Print Preview: Update test to check parameters sent to print() Update the native layer stub to pass on the parameters sent to print(), and update the InvalidSettings test to sanity check some of these parameters to verify print is being called correctly. BUG=717296 ==========
rbpotter@chromium.org changed reviewers: + dpapad@chromium.org
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...
Followup to https://codereview.chromium.org/2948723002/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2961773003/diff/1/chrome/test/data/webui/prin... File chrome/test/data/webui/print_preview/native_layer_stub.js (right): https://codereview.chromium.org/2961773003/diff/1/chrome/test/data/webui/prin... chrome/test/data/webui/print_preview/native_layer_stub.js:103: var args = {"destination": destination, Nit1: Prefer single quotes instead of double quotes in JS. Nit2: The quotes are not necessary at all here. Nit3: Perhaps inline as followls this.methodCalled('print', { destination: destination, printTicketStore: printTicketStore, ... }); https://codereview.chromium.org/2961773003/diff/1/chrome/test/data/webui/prin... File chrome/test/data/webui/print_preview/print_preview_tests.js (right): https://codereview.chromium.org/2961773003/diff/1/chrome/test/data/webui/prin... chrome/test/data/webui/print_preview/print_preview_tests.js:1329: 215900); Can we store the result of getCddTemplate() at line 1297 and refer to it here, instead of repeating number literal values?
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...
https://codereview.chromium.org/2961773003/diff/1/chrome/test/data/webui/prin... File chrome/test/data/webui/print_preview/native_layer_stub.js (right): https://codereview.chromium.org/2961773003/diff/1/chrome/test/data/webui/prin... chrome/test/data/webui/print_preview/native_layer_stub.js:103: var args = {"destination": destination, On 2017/06/27 18:49:00, dpapad wrote: > Nit1: Prefer single quotes instead of double quotes in JS. > Nit2: The quotes are not necessary at all here. > Nit3: Perhaps inline as followls > > this.methodCalled('print', { > destination: destination, > printTicketStore: printTicketStore, > ... > }); Done. https://codereview.chromium.org/2961773003/diff/1/chrome/test/data/webui/prin... File chrome/test/data/webui/print_preview/print_preview_tests.js (right): https://codereview.chromium.org/2961773003/diff/1/chrome/test/data/webui/prin... chrome/test/data/webui/print_preview/print_preview_tests.js:1329: 215900); On 2017/06/27 18:49:00, dpapad wrote: > Can we store the result of getCddTemplate() at line 1297 and refer to it here, > instead of repeating number literal values? Done.
LGTM with nits. https://codereview.chromium.org/2961773003/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/print_preview/native_layer_stub.js (right): https://codereview.chromium.org/2961773003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/print_preview/native_layer_stub.js:107: documentInfo: documentInfo Per styleguide, always use a trailing comma when the closing brace is on the next line, see https://google.github.io/styleguide/jsguide.html#features-object-literals. BTW, we have not turned on automated linting on test code yet, which is a bit annoying. https://codereview.chromium.org/2961773003/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/print_preview/print_preview_tests.js (right): https://codereview.chromium.org/2961773003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/print_preview/print_preview_tests.js:150: * @return {width_microns: number, Nit: return {{...}} I think there is a set of brackets here, and at line 1339. (We don't compile tests, so it is not actually causing any problem). https://codereview.chromium.org/2961773003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/print_preview/print_preview_tests.js:1350: expectEquals(printTicketStore.landscape.getValue(), Nit: According to the docs, expected value goes first, then actual value, see [1] https://cs.chromium.org/chromium/src/chrome/test/data/webui/test_api.js?l=886... (expectEquals is just a wrapper around assertEquals). https://codereview.chromium.org/2961773003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/print_preview/print_preview_tests.js:1353: var media_default = getDefaultMediaSize(barDevice); s/media_default/mediaDefault
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...
https://codereview.chromium.org/2961773003/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/print_preview/native_layer_stub.js (right): https://codereview.chromium.org/2961773003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/print_preview/native_layer_stub.js:107: documentInfo: documentInfo On 2017/06/28 01:28:34, dpapad wrote: > Per styleguide, always use a trailing comma when the closing brace is on the > next line, see > https://google.github.io/styleguide/jsguide.html#features-object-literals. > > BTW, we have not turned on automated linting on test code yet, which is a bit > annoying. Done. https://codereview.chromium.org/2961773003/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/print_preview/print_preview_tests.js (right): https://codereview.chromium.org/2961773003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/print_preview/print_preview_tests.js:150: * @return {width_microns: number, On 2017/06/28 01:28:34, dpapad wrote: > Nit: return {{...}} > > I think there is a set of brackets here, and at line 1339. (We don't compile > tests, so it is not actually causing any problem). Done. https://codereview.chromium.org/2961773003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/print_preview/print_preview_tests.js:1350: expectEquals(printTicketStore.landscape.getValue(), On 2017/06/28 01:28:34, dpapad wrote: > Nit: According to the docs, expected value goes first, then actual value, see > [1] > > https://cs.chromium.org/chromium/src/chrome/test/data/webui/test_api.js?l=886... > (expectEquals is just a wrapper around assertEquals). Done. https://codereview.chromium.org/2961773003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/print_preview/print_preview_tests.js:1353: var media_default = getDefaultMediaSize(barDevice); On 2017/06/28 01:28:34, dpapad wrote: > s/media_default/mediaDefault Done.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by rbpotter@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2961773003/#ps40001 (title: "Address comments")
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": 40001, "attempt_start_ts": 1498668492953080,
"parent_rev": "ccd4c67b0f7c5418926e32c873181f285a2c5354", "commit_rev":
"6c220aac9f218d0274a6bed442cce6211599811a"}
Message was sent while issue was closed.
Description was changed from ========== Print Preview: Update test to check parameters sent to print() Update the native layer stub to pass on the parameters sent to print(), and update the InvalidSettings test to sanity check some of these parameters to verify print is being called correctly. BUG=717296 ========== to ========== Print Preview: Update test to check parameters sent to print() Update the native layer stub to pass on the parameters sent to print(), and update the InvalidSettings test to sanity check some of these parameters to verify print is being called correctly. BUG=717296 Review-Url: https://codereview.chromium.org/2961773003 Cr-Commit-Position: refs/heads/master@{#483021} Committed: https://chromium.googlesource.com/chromium/src/+/6c220aac9f218d0274a6bed442cc... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/6c220aac9f218d0274a6bed442cc... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
