Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2833993004:
Print Preview: Make generate draft mode work again. (Closed)
DescriptionPrint Preview: Make generate draft mode work again.
This makes updating the print preview when changing the page range
faster. The updated print preview will be stitched together from existing
data, rather than re-rendered.
BUG=713860
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2833993004
Cr-Commit-Position: refs/heads/master@{#469749}
Committed: https://chromium.googlesource.com/chromium/src/+/2d62082dec8785c54220a0295033cf12a0edf2d4
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 4
Patch Set 3 : nits #Patch Set 4 : Add test #Patch Set 5 : rebase #
Total comments: 1
Patch Set 6 : rebase #Patch Set 7 : rebase #Patch Set 8 : Check NativeLayerStub #
Total comments: 1
Messages
Total messages: 34 (22 generated)
Description was changed from ========== Print Preview: Make generate draft mode work again. This makes updating the print preview when changing the page range faster. The updated print preview will be stitched together from existing data, rather than re-rendered. BUG=713860 ========== to ========== Print Preview: Make generate draft mode work again. This makes updating the print preview when changing the page range faster. The updated print preview will be stitched together from existing data, rather than re-rendered. BUG=713860 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by thestig@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: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by thestig@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...
thestig@chromium.org changed reviewers: + dpapad@chromium.org, rbpotter@chromium.org
This obviously needs more testing since some refactoring in the last 6 years broke this. I'm thinking of adding JS tests, since that's what decides whether to generate drafts or not.
LGTM. We definitely need more tests :| https://codereview.chromium.org/2833993004/diff/20001/chrome/browser/resource... File chrome/browser/resources/print_preview/preview_generator.js (right): https://codereview.chromium.org/2833993004/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/preview_generator.js:64: * @type {boolean} Nit: @private {boolean} https://codereview.chromium.org/2833993004/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/preview_generator.js:324: this.pageRanges_); Nit (optional): There used to be a so called "box rule" in one of our styleguides, but can't find it anymore. Meaning that one should be able to draw a box around a subtree of the code. !areRangesEqual( this.printTicketStore_.pageRange.getPageRanges(), this.pageRanges_);
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/22 01:32:49, dpapad wrote: > LGTM. We definitely need more tests :| > > https://codereview.chromium.org/2833993004/diff/20001/chrome/browser/resource... > File chrome/browser/resources/print_preview/preview_generator.js (right): > > https://codereview.chromium.org/2833993004/diff/20001/chrome/browser/resource... > chrome/browser/resources/print_preview/preview_generator.js:64: * @type > {boolean} > Nit: @private {boolean} > > https://codereview.chromium.org/2833993004/diff/20001/chrome/browser/resource... > chrome/browser/resources/print_preview/preview_generator.js:324: > this.pageRanges_); > Nit (optional): There used to be a so called "box rule" in one of our > styleguides, but can't find it anymore. Meaning that one should be able to draw > a box around a subtree of the code. > > !areRangesEqual( > this.printTicketStore_.pageRange.getPageRanges(), > this.pageRanges_); lgtm
Still working on JS tests... https://codereview.chromium.org/2833993004/diff/20001/chrome/browser/resource... File chrome/browser/resources/print_preview/preview_generator.js (right): https://codereview.chromium.org/2833993004/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/preview_generator.js:64: * @type {boolean} On 2017/04/22 01:32:49, dpapad wrote: > Nit: @private {boolean} Done. https://codereview.chromium.org/2833993004/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/preview_generator.js:324: this.pageRanges_); On 2017/04/22 01:32:49, dpapad wrote: > Nit (optional): There used to be a so called "box rule" in one of our > styleguides, but can't find it anymore. Meaning that one should be able to draw > a box around a subtree of the code. > > !areRangesEqual( > this.printTicketStore_.pageRange.getPageRanges(), > this.pageRanges_); Done.
The CQ bit was checked by thestig@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: 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_...)
Now with tests.
https://codereview.chromium.org/2833993004/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/print_preview.js (right): https://codereview.chromium.org/2833993004/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/print_preview.js:1378: expectTrue(printPreview.previewArea_.previewGenerator_.generateDraft_); Pasting my comment from another recent print preview CL (https://codereview.chromium.org/2849733002#msg5). Is there a way to avoid referring to a private variable from this test? Tests that assert on private variables tend to test a specific implementation instead of the observed behavior of the code they are testing. Maybe assert that something that is observable by the user on the DOM (some button being enabled, some message being shown etc).
On 2017/05/01 18:08:31, dpapad wrote: > https://codereview.chromium.org/2833993004/diff/80001/chrome/test/data/webui/... > File chrome/test/data/webui/print_preview.js (right): > > https://codereview.chromium.org/2833993004/diff/80001/chrome/test/data/webui/... > chrome/test/data/webui/print_preview.js:1378: > expectTrue(printPreview.previewArea_.previewGenerator_.generateDraft_); > Pasting my comment from another recent print preview CL > (https://codereview.chromium.org/2849733002#msg5). > > Is there a way to avoid referring to a private variable from this test? Tests > that assert on private variables tend to test a specific implementation instead > of the observed behavior of the code they are testing. > > Maybe assert that something that is observable by the user on the DOM (some > button being enabled, some message being shown etc). The variable is being sent in a message to C++ code. That C++ code is not running in this test, right? In which case, there may not be any reaction in the JS code to observe. I can check and see.
> The variable is being sent in a message to C++ code. That C++ code is not running in this test, right? In which case, there may not be any reaction in the JS code to observe. I can check and see. I see. I think a better way to test this is to intercept the message and see that the expected value is sent to the C++ (see https://cs.chromium.org/chromium/src/chrome/test/data/webui/print_preview.js?...), instead of asserting on a private member var and assuming (without checking) that eventually it makes its way to C++. FWIW, the changes tracked by issue 717296 should make intercepting messages to C++ easier (with https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/test_bro...), but that is going to take a while.
The CQ bit was checked by thestig@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.
On 2017/05/02 21:42:58, dpapad wrote: > > The variable is being sent in a message to C++ code. That C++ code is not > running in this test, right? In which case, there may not be any reaction in the > JS code to observe. I can check and see. > > I see. I think a better way to test this is to intercept the message and see > that the expected value is sent to the C++ (see > https://cs.chromium.org/chromium/src/chrome/test/data/webui/print_preview.js?...), > instead of asserting on a private member var and assuming (without checking) > that eventually it makes its way to C++. I changed the NativeLayerStub. One more look?
https://codereview.chromium.org/2833993004/diff/140001/chrome/test/data/webui... File chrome/test/data/webui/print_preview.js (right): https://codereview.chromium.org/2833993004/diff/140001/chrome/test/data/webui... chrome/test/data/webui/print_preview.js:1486: expectFalse(this.generateDraft()); Thanks. This looks good for now. Eventually when we switch NativeLayerStub to inherit from TestBrowserProxy you could do something as follows this.setCapabilities(getCddTemplate("FooDevice")); return stub.whenCalled('startGetPreview').then(function(args) { // args[3] is |generatePreview| assertFalse(args[3]); stub.resetResolver('startGetPreview'); printPreview.printTicketStore_.marginsType.updateValue( print_preview.ticket_items.MarginsTypeValue.NO_MARGINS); return stub.whenCalled('startGetPreview') }).then(function(args) { assertTrue(args[3]); });
The CQ bit was checked by thestig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org, rbpotter@chromium.org Link to the patchset: https://codereview.chromium.org/2833993004/#ps140001 (title: "Check NativeLayerStub")
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": 140001, "attempt_start_ts": 1494011401151640,
"parent_rev": "eff03640eb47562e8a461dd950a42e469a77e842", "commit_rev":
"2d62082dec8785c54220a0295033cf12a0edf2d4"}
Message was sent while issue was closed.
Description was changed from ========== Print Preview: Make generate draft mode work again. This makes updating the print preview when changing the page range faster. The updated print preview will be stitched together from existing data, rather than re-rendered. BUG=713860 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Print Preview: Make generate draft mode work again. This makes updating the print preview when changing the page range faster. The updated print preview will be stitched together from existing data, rather than re-rendered. BUG=713860 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2833993004 Cr-Commit-Position: refs/heads/master@{#469749} Committed: https://chromium.googlesource.com/chromium/src/+/2d62082dec8785c54220a0295033... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/2d62082dec8785c54220a0295033... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
