Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2628393002:
Print Preview: Add test to ensure only 1 preview is started at init (Closed)
DescriptionAdd print preview test to ensure only 1 preview is started at init
This adds a test to ensure only one DOCUMENT_CHANGE and one
CAPABILITIES_CHANGE event are issued on initialization of print preview.
This ensures that only one preview will be started. Multiple preview
starts cause a race condition that created bug 666595.
BUG=680447
Patch Set 1 #
Total comments: 6
Patch Set 2 : Clarify comment and change settings construction #
Total comments: 2
Patch Set 3 : Remove extra stringify #Patch Set 4 : Count startPreviews not requestPreviews #
Total comments: 4
Patch Set 5 : Fix nits #Messages
Total messages: 31 (19 generated)
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
rbpotter@chromium.org changed reviewers: + dpapad@chromium.org
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/2628393002/diff/1/chrome/test/data/webui/prin... File chrome/test/data/webui/print_preview.js (right): https://codereview.chromium.org/2628393002/diff/1/chrome/test/data/webui/prin... chrome/test/data/webui/print_preview.js:1260: this.initialSettings_.serializedAppStateStr_ = Let's simplify how serializedAppStateStr_ is populated. I already gave a suggestion at a different CL, see my comment at https://codereview.chromium.org/2606043004/diff/120001/chrome/test/data/webui.... https://codereview.chromium.org/2628393002/diff/1/chrome/test/data/webui/prin... chrome/test/data/webui/print_preview.js:1279: // Initializing print preview initializes documentInfo. This triggers This wall of text is a bit hard to read. Can you add some bullet points to make it easier? - 2 calls for ... - 3 calls for ... ... Total 18....
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/2628393002/diff/1/chrome/test/data/webui/prin... File chrome/test/data/webui/print_preview.js (right): https://codereview.chromium.org/2628393002/diff/1/chrome/test/data/webui/prin... chrome/test/data/webui/print_preview.js:1260: this.initialSettings_.serializedAppStateStr_ = On 2017/01/13 19:55:10, dpapad wrote: > Let's simplify how serializedAppStateStr_ is populated. I already gave a > suggestion at a different CL, see my comment at > https://codereview.chromium.org/2606043004/diff/120001/chrome/test/data/webui.... Done. https://codereview.chromium.org/2628393002/diff/1/chrome/test/data/webui/prin... chrome/test/data/webui/print_preview.js:1279: // Initializing print preview initializes documentInfo. This triggers On 2017/01/13 19:55:10, dpapad wrote: > This wall of text is a bit hard to read. Can you add some bullet points to make > it easier? > > - 2 calls for ... > - 3 calls for ... > ... > Total 18.... Done. Let me know if it is still unclear or should be trimmed down.
https://codereview.chromium.org/2628393002/diff/1/chrome/test/data/webui/prin... File chrome/test/data/webui/print_preview.js (right): https://codereview.chromium.org/2628393002/diff/1/chrome/test/data/webui/prin... chrome/test/data/webui/print_preview.js:1279: // Initializing print preview initializes documentInfo. This triggers On 2017/01/13 at 21:25:02, rbpotter wrote: > On 2017/01/13 19:55:10, dpapad wrote: > > This wall of text is a bit hard to read. Can you add some bullet points to make > > it easier? > > > > - 2 calls for ... > > - 3 calls for ... > > ... > > Total 18.... > > Done. Let me know if it is still unclear or should be trimmed down. The text is fine now, I can read it with less effort. But there is one thing I still don't understand. The CL description says that there is only 1 preview at init, but there are 18 calls to requestPreview. That confuses me. Is there a requestPreview() and startPreview() and if so, what is their difference? https://codereview.chromium.org/2628393002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/print_preview.js (right): https://codereview.chromium.org/2628393002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/print_preview.js:1265: capabilities: JSON.stringify(getCddTemplate('ID' + i)), name: '', capabilities is JSONified twice now? I would expect that you don't need to call JSON.stringify here at all anymore, the one at line 1270 should cover it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Am looking to try to find a way to count startPreview calls instead of requestPreview. However, have not been successful so far. https://codereview.chromium.org/2628393002/diff/1/chrome/test/data/webui/prin... File chrome/test/data/webui/print_preview.js (right): https://codereview.chromium.org/2628393002/diff/1/chrome/test/data/webui/prin... chrome/test/data/webui/print_preview.js:1279: // Initializing print preview initializes documentInfo. This triggers On 2017/01/13 21:37:40, dpapad wrote: > On 2017/01/13 at 21:25:02, rbpotter wrote: > > On 2017/01/13 19:55:10, dpapad wrote: > > > This wall of text is a bit hard to read. Can you add some bullet points to > make > > > it easier? > > > > > > - 2 calls for ... > > > - 3 calls for ... > > > ... > > > Total 18.... > > > > Done. Let me know if it is still unclear or should be trimmed down. > > The text is fine now, I can read it with less effort. > > But there is one thing I still don't understand. The CL description says that > there is only 1 preview at init, but there are 18 calls to requestPreview. That > confuses me. > > Is there a requestPreview() and startPreview() and if so, what is their > difference? Yes. requestPreview() checks a number of conditions (if the preview will have changed due to the ticket item changes, if the ticket store has been initialized, and if a destination is selected) to see if a preview request should actually be sent. If so, it calls startPreview() which is handled by the native layer and sends the preview request. So in this case all but one request is filtered out, so only one call is made to startPreview(). The bug occurred because after a destination had been set and everything was initialized, another destination selection event was fired, and because the destination changed, requestPreview called startPreview again. Ideally it would be better to look at the number of calls to startPreview but the native layer cannot be replaced with a mock here because it is needed to send various events in setInitialSettings. https://codereview.chromium.org/2628393002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/print_preview.js (right): https://codereview.chromium.org/2628393002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/print_preview.js:1265: capabilities: JSON.stringify(getCddTemplate('ID' + i)), name: '', On 2017/01/13 21:37:41, dpapad wrote: > capabilities is JSONified twice now? I would expect that you don't need to call > JSON.stringify here at all anymore, the one at line 1270 should cover it. Done.
Am looking to try to find a way to count startPreview calls instead of requestPreview. However, have not been successful so far.
On 2017/01/13 at 22:52:21, rbpotter wrote:
> Am looking to try to find a way to count startPreview calls instead of
requestPreview. However, have not been successful so far.
LGTM.
Ok, thanks for the explanation. It makes sense now.
As for counting calls to startPreview only I think is a good idea, since it more
directly tests what we are actually covering with this test. How about directly
replacing that function with your own, something like the following snippet.
var startPreviewCounter = 0;
function mockStartPreview() {
startPreviewCounter++;
}
nativeLayer.startPreview = mockStartPreview;
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...
Realized there was already a inFlightRequestId that could be used to determine the number of startPreviews called. Needed to add in the real generator to the preview area instead of the mock to make this work. This should now count the number of startPreview calls rather than looking at the number of requestPreview calls.
That test looks more direct than the previous one. LGTM with nits. https://codereview.chromium.org/2628393002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/print_preview.js (right): https://codereview.chromium.org/2628393002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/print_preview.js:1288: origin = cr.isChromeOS ? 'chrome_os' : 'local'; var origin https://codereview.chromium.org/2628393002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/print_preview.js:1306: new print_preview.PreviewGenerator(printPreview.destinationStore_, indent off by -2
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 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/2628393002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/print_preview.js (right): https://codereview.chromium.org/2628393002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/print_preview.js:1288: origin = cr.isChromeOS ? 'chrome_os' : 'local'; On 2017/01/14 02:37:00, dpapad wrote: > var origin Done. https://codereview.chromium.org/2628393002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/print_preview.js:1306: new print_preview.PreviewGenerator(printPreview.destinationStore_, On 2017/01/14 02:37:00, dpapad wrote: > indent off by -2 Done.
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
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/2628393002/#ps80001 (title: "Fix nits")
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": 80001, "attempt_start_ts": 1484683329526730,
"parent_rev": "bfeb98a98d8495492d476d851b896fb09cf1a6ab", "commit_rev":
"0b28591edaa17ce6bc1e2c951c568066dd2e8ae2"}
The CQ bit was unchecked by commit-bot@chromium.org
Prior attempt to commit was detected, but we were not able to check whether the issue was successfully committed. Please check Git history manually and re-check CQ or close this issue as needed. |
