Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2962983002:
Print Preview: change getPreview to cr.sendWithPromise (Closed)
DescriptionPrint Preview: change getPreview to cr.sendWithPromise
- Use cr.sendWithPromise for getPreview
- Remove invalidPrinterSettings, printPreviewFailed, and
updatePrintPreview global JS functions
- Update some print preview tests to take advantage of browser proxy
for getPreview.
BUG=717296
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2962983002
Cr-Commit-Position: refs/heads/master@{#484183}
Committed: https://chromium.googlesource.com/chromium/src/+/579f9d6fc5f37f9c1a40894b8a811aad58d01d3b
Patch Set 1 #Patch Set 2 : Simplify test, change to -1 #Patch Set 3 : Re-comment test #
Total comments: 1
Patch Set 4 : Rebase #
Total comments: 4
Patch Set 5 : Fix tests #
Total comments: 18
Patch Set 6 : Address comments #Patch Set 7 : Address comments #2 #
Total comments: 6
Patch Set 8 : Track IDs - for testing on Win #Patch Set 9 : Fix invalid settings and revert IPC changes #Patch Set 10 : Remove debugging intentional failure #Patch Set 11 : Fix string literal #
Total comments: 2
Patch Set 12 : Fix JS and string literal #
Total comments: 2
Patch Set 13 : Fix PrintWebViewHelperBrowsertest #
Total comments: 6
Patch Set 14 : Address comments #
Total comments: 2
Patch Set 15 : Fix check #Messages
Total messages: 69 (51 generated)
Description was changed from ========== Print Preview: change getPreview to cr.sendWithPromise - Use cr.sendWithPromise for getPreview - Change to 1 to 1 relationship between preview requests and updates/ failures - Remove invalidPrinterSettings, printPreviewFailed, and updatePrintPreview global JS functions - Update some print preview tests to take advantage of browser proxy for getPreview. BUG=717296 ========== to ========== Print Preview: change getPreview to cr.sendWithPromise - Use cr.sendWithPromise for getPreview - Change to 1 to 1 relationship between preview requests and updates/ failures - Remove invalidPrinterSettings, printPreviewFailed, and updatePrintPreview global JS functions - Update some print preview tests to take advantage of browser proxy for getPreview. BUG=717296 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== Print Preview: change getPreview to cr.sendWithPromise - Use cr.sendWithPromise for getPreview - Change to 1 to 1 relationship between preview requests and updates/ failures - Remove invalidPrinterSettings, printPreviewFailed, and updatePrintPreview global JS functions - Update some print preview tests to take advantage of browser proxy for getPreview. BUG=717296 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Print Preview: change getPreview to cr.sendWithPromise - Use cr.sendWithPromise for getPreview - Remove invalidPrinterSettings, printPreviewFailed, and updatePrintPreview global JS functions - Update some print preview tests to take advantage of browser proxy for getPreview. BUG=717296 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:20001) has been deleted
rbpotter@chromium.org changed reviewers: + dpapad@chromium.org, thestig@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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/2962983002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/print_preview/print_preview_tests.js (right): https://codereview.chromium.org/2962983002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/print_preview/print_preview_tests.js:1248: // request. Ensure this is also ID1. Note: This always catches the bug linked, as the code at that time fetched ID3, then ID2, then ID1 (reverse order of recency). However it may be possible for a related bug to occur in which ID1 is fetched first, then some other ID is fetched just enough afterward to trigger the second preview request. I have an alternate solution that covers this case, but it requires ~30 lines of code to be added to NativeLayerStub and when I tried to create this failure the test as is caught it every time I ran it locally (i.e. ID3 always arrived before the timeout for the first request and as a result line 1258 failed).
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_...)
There are some red trybots... https://codereview.chromium.org/2962983002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2962983002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1541: ResolveJavascriptCallback(base::Value(preview_callbacks_[request_id]), If |request_id| is negative, or too big, then this goes out of bounds. In general, any data that comes from the initiator renderer needs to be sanitized before we use it. https://codereview.chromium.org/2962983002/diff/100001/components/printing/re... File components/printing/renderer/print_web_view_helper.cc (right): https://codereview.chromium.org/2962983002/diff/100001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:1582: : 0; Why do we use 0 here, but -1 on line 1194?
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 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...
Trybots should hopefully turn green now. https://codereview.chromium.org/2962983002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2962983002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1541: ResolveJavascriptCallback(base::Value(preview_callbacks_[request_id]), On 2017/06/29 19:25:46, Lei Zhang wrote: > If |request_id| is negative, or too big, then this goes out of bounds. > > In general, any data that comes from the initiator renderer needs to be > sanitized before we use it. Done. https://codereview.chromium.org/2962983002/diff/100001/components/printing/re... File components/printing/renderer/print_web_view_helper.cc (right): https://codereview.chromium.org/2962983002/diff/100001/components/printing/re... components/printing/renderer/print_web_view_helper.cc:1582: : 0; On 2017/06/29 19:25:46, Lei Zhang wrote: > Why do we use 0 here, but -1 on line 1194? Should always be -1, fixed.
https://codereview.chromium.org/2962983002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/print_preview/previewarea/preview_area.js (left): https://codereview.chromium.org/2962983002/diff/120001/chrome/browser/resourc... chrome/browser/resources/print_preview/previewarea/preview_area.js:526: if (this.previewGenerator_ && this.previewGenerator_.requestPreview()) { Do we know if checking if previewGenerator_ exists is necessary? Also, it seems a bit odd that in the case where previewGenerator is not there, we still go to the else case. If legitimately |previewGenerator_| can be null/undefined here, should the logic be as follows? if (!this.previewGenerator_) return; if (this.previewGenerator_.requestPreview()) { ... } else { ... } Asking because this complicates a bit the code you are modifying. https://codereview.chromium.org/2962983002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/print_preview/previewarea/preview_area.js (right): https://codereview.chromium.org/2962983002/diff/120001/chrome/browser/resourc... chrome/browser/resources/print_preview/previewarea/preview_area.js:179: this.isBrowserTest_ = false; Is this new boolean necessary? How did the tests work before without it? https://codereview.chromium.org/2962983002/diff/120001/chrome/browser/resourc... chrome/browser/resources/print_preview/previewarea/preview_area.js:531: var requestPreview = this.previewGenerator_ ? Nit (optional): |requestPreview| sounds like a function name. Maybe reverse, as |previewRequest|, or simply |request|. https://codereview.chromium.org/2962983002/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/print_preview/print_preview_tests.js (right): https://codereview.chromium.org/2962983002/diff/120001/chrome/test/data/webui... chrome/test/data/webui/print_preview/print_preview_tests.js:277: return true; Why is this change necessary? https://codereview.chromium.org/2962983002/diff/120001/chrome/test/data/webui... chrome/test/data/webui/print_preview/print_preview_tests.js:1085: }).then(function(args0) { Nit s/args0/args? https://codereview.chromium.org/2962983002/diff/120001/chrome/test/data/webui... chrome/test/data/webui/print_preview/print_preview_tests.js:1110: }).then(function(args1) { Same here. No need to use a different name, since it's a different scope. https://codereview.chromium.org/2962983002/diff/120001/chrome/test/data/webui... chrome/test/data/webui/print_preview/print_preview_tests.js:1269: }).then(function(preview_args) { previewArgs https://codereview.chromium.org/2962983002/diff/120001/chrome/test/data/webui... chrome/test/data/webui/print_preview/print_preview_tests.js:1315: }).then(function() { If you don't need to do anything here, then how about waiting on both promises at once? return Promise.all([ nativeLayer.whenCalled('getPrinterCapabilities'), nativeLayer.whenCalled('getPreview'), ]);
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2962983002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/print_preview/previewarea/preview_area.js (left): https://codereview.chromium.org/2962983002/diff/120001/chrome/browser/resourc... chrome/browser/resources/print_preview/previewarea/preview_area.js:526: if (this.previewGenerator_ && this.previewGenerator_.requestPreview()) { On 2017/06/29 20:58:31, dpapad wrote: > Do we know if checking if previewGenerator_ exists is necessary? Also, it seems > a bit odd that in the case where previewGenerator is not there, we still go to > the else case. > > If legitimately |previewGenerator_| can be null/undefined here, should the logic > be as follows? > > if (!this.previewGenerator_) > return; > > if (this.previewGenerator_.requestPreview()) { > ... > } else { > ... > } > > Asking because this complicates a bit the code you are modifying. Think so, as if the plugin is not compatible preview generator will not exist. See line 349. I suspect showing margin controls in this case was needed for the CustomMargins tests as previewGenerator_ was null for all tests previously. However now that this has changed it appears we can just return immediately in the case of a null generator. https://codereview.chromium.org/2962983002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/print_preview/previewarea/preview_area.js (right): https://codereview.chromium.org/2962983002/diff/120001/chrome/browser/resourc... chrome/browser/resources/print_preview/previewarea/preview_area.js:179: this.isBrowserTest_ = false; On 2017/06/29 20:58:31, dpapad wrote: > Is this new boolean necessary? How did the tests work before without it? This is used for the InvalidSettings test. Most tests do not care about what happens after a preview request is sent, so they do not care if PREVIEW_GENERATION_DONE is ever fired. I wanted to use the new getPreview() function with resolve/reject instead of manually triggering PREVIEW_GENERATION_DONE and SETTINGS_INVALID in this test. onDocumentReady_ is called when getPreview is resolved but it also checks the plugin loading state before issuing PREVIEW_GENERATION_DONE. Since the plugin does not really exist it looked like the preview was never finished so print was never called. Thus this boolean was added so for testing PreviewArea just manually sets the pluginReload state to done as soon as onDocumentReady_ is called. As a result this test now checks the full initialize -> getPreview -> reject and change destination -> getPreview -> resolve logic/event sequences, instead of just creating the events on its own and manually setting parts of the UI (e.g. forcing print header to enabled at the start of the test). The other thought I had was to create a "PluginStub" to fill in all the necessary plugin functions and watch for resolutions of getPreview from the NativeLayerStub to call onLoadCallback each time. Then PreviewArea.createPlugin_ would be overridden to use the constructor of this fake plugin. However, this seemed like it might be more complicated than necessary. Would that be a better approach? https://codereview.chromium.org/2962983002/diff/120001/chrome/browser/resourc... chrome/browser/resources/print_preview/previewarea/preview_area.js:531: var requestPreview = this.previewGenerator_ ? On 2017/06/29 20:58:31, dpapad wrote: > Nit (optional): |requestPreview| sounds like a function name. Maybe reverse, as > |previewRequest|, or simply |request|. Done. https://codereview.chromium.org/2962983002/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/print_preview/print_preview_tests.js (right): https://codereview.chromium.org/2962983002/diff/120001/chrome/test/data/webui... chrome/test/data/webui/print_preview/print_preview_tests.js:277: return true; On 2017/06/29 20:58:31, dpapad wrote: > Why is this change necessary? This function determines if the Preview Generator gets created when PreviewArea is initialized. If it isn't created, any test that simulates a "getPreview" request (currently InvalidSettings, GenerateDraft, and InitIssuesOne...) has to create the PreviewGenerator manually in the test (see previous version of GenerateDraft/InitIssuesOneRequest). If it is created, these tests avoid the need to set the generator, and all other tests are unaffected except for the PDFPluginDisabled test. It seemed simpler to manually set the generator to null in the PluginDisabled test rather than having to set it to non-null in every test that simulates a preview request. We could change this back but then the previewArea.previewGenerator_ = new PreviewGenerator(...) would have to be added back to 3 of the tests. https://codereview.chromium.org/2962983002/diff/120001/chrome/test/data/webui... chrome/test/data/webui/print_preview/print_preview_tests.js:1085: }).then(function(args0) { On 2017/06/29 20:58:31, dpapad wrote: > Nit s/args0/args? Done. https://codereview.chromium.org/2962983002/diff/120001/chrome/test/data/webui... chrome/test/data/webui/print_preview/print_preview_tests.js:1110: }).then(function(args1) { On 2017/06/29 20:58:31, dpapad wrote: > Same here. No need to use a different name, since it's a different scope. Done. https://codereview.chromium.org/2962983002/diff/120001/chrome/test/data/webui... chrome/test/data/webui/print_preview/print_preview_tests.js:1269: }).then(function(preview_args) { On 2017/06/29 20:58:31, dpapad wrote: > previewArgs Done. https://codereview.chromium.org/2962983002/diff/120001/chrome/test/data/webui... chrome/test/data/webui/print_preview/print_preview_tests.js:1315: }).then(function() { On 2017/06/29 20:58:31, dpapad wrote: > If you don't need to do anything here, then how about waiting on both promises > at once? > > return Promise.all([ > nativeLayer.whenCalled('getPrinterCapabilities'), > nativeLayer.whenCalled('getPreview'), > ]); 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...
https://codereview.chromium.org/2962983002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/print_preview/previewarea/preview_area.js (right): https://codereview.chromium.org/2962983002/diff/120001/chrome/browser/resourc... chrome/browser/resources/print_preview/previewarea/preview_area.js:179: this.isBrowserTest_ = false; On 2017/06/29 at 22:31:22, rbpotter wrote: > On 2017/06/29 20:58:31, dpapad wrote: > > Is this new boolean necessary? How did the tests work before without it? > This is used for the InvalidSettings test. Most tests do not care > about what happens after a preview request is sent, so they do not > care if PREVIEW_GENERATION_DONE is ever fired. > > I wanted to use the new getPreview() function with resolve/reject > instead of manually triggering PREVIEW_GENERATION_DONE and > SETTINGS_INVALID in this test. onDocumentReady_ is called when > getPreview is resolved but it also checks the plugin loading state > before issuing PREVIEW_GENERATION_DONE. Since the plugin does not > really exist it looked like the preview was never finished so print > was never called. Thus this boolean was added so for testing > PreviewArea just manually sets the pluginReload state to done as soon > as onDocumentReady_ is called. > > As a result this test now checks the full initialize -> getPreview -> > reject and change destination -> getPreview -> resolve logic/event > sequences, instead of just creating the events on its own and > manually setting parts of the UI (e.g. forcing print header to > enabled at the start of the test). > > The other thought I had was to create a "PluginStub" to fill in all > the necessary plugin functions and watch for resolutions of getPreview > from the NativeLayerStub to call onLoadCallback each time. Then > PreviewArea.createPlugin_ would be overridden to use the constructor > of this fake plugin. However, this seemed like it might be more > complicated than necessary. Would that be a better approach? Thanks for the explanation. I think the way the InvalidSettings test works now is better than before. Regarding PluginStub, probably a good idea, but I think we should attempt it separately (in a follow up). Also not sure if we need to monitor NativeLayerStub to call onLoadCallback. Can't our fake plugin call onLoadCallback immediately? https://codereview.chromium.org/2962983002/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/print_preview/print_preview_tests.js (right): https://codereview.chromium.org/2962983002/diff/120001/chrome/test/data/webui... chrome/test/data/webui/print_preview/print_preview_tests.js:277: return true; On 2017/06/29 at 22:31:22, rbpotter wrote: > On 2017/06/29 20:58:31, dpapad wrote: > > Why is this change necessary? > > This function determines if the Preview Generator gets created when > PreviewArea is initialized. If it isn't created, any test that > simulates a "getPreview" request (currently InvalidSettings, > GenerateDraft, and InitIssuesOne...) has to create the > PreviewGenerator manually in the test (see previous version of > GenerateDraft/InitIssuesOneRequest). If it is created, these tests > avoid the need to set the generator, and all other tests are > unaffected except for the PDFPluginDisabled test. It seemed simpler > to manually set the generator to null in the PluginDisabled test > rather than having to set it to non-null in every test that simulates > a preview request. We could change this back but then the > previewArea.previewGenerator_ = new PreviewGenerator(...) would have > to be added back to 3 of the tests. Thanks. Current version seems fine to me, now that I understand what this boolean was doing. https://codereview.chromium.org/2962983002/diff/160001/chrome/test/data/webui... File chrome/test/data/webui/print_preview/native_layer_stub.js (right): https://codereview.chromium.org/2962983002/diff/160001/chrome/test/data/webui... chrome/test/data/webui/print_preview/native_layer_stub.js:98: if (destination.id == this.badPrinterId_) { Nit (optional): return destination.id == this.badPrinterId_ ? Promise.reject('SETTINGS_INVALID') : Promise.resolve(requestId); Also can we use print_preview.PreviewArea.EventType.SETTINGS_INVALID instead of using the string literal? https://codereview.chromium.org/2962983002/diff/160001/chrome/test/data/webui... File chrome/test/data/webui/print_preview/print_preview_tests.js (right): https://codereview.chromium.org/2962983002/diff/160001/chrome/test/data/webui... chrome/test/data/webui/print_preview/print_preview_tests.js:1282: nativeLayer.setInvalidPrinterId('FooDevice'); Nit: Maybe add a comment that "FooDevice" is the currently selected or default printer. https://codereview.chromium.org/2962983002/diff/160001/chrome/test/data/webui... chrome/test/data/webui/print_preview/print_preview_tests.js:1288: // is not running. However, it should not be the invalid settings I think the 2nd sentence is no longer accurate.
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...
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...
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...
- Addressed comments in JS code - Reverted IPC message changes and changed preview_callbacks_ to a queue in PrintPreviewHandler. - Added functions to notify the print preview UI when preview requests are cancelled so that all promises will always be resolved/rejected.
https://codereview.chromium.org/2962983002/diff/160001/chrome/test/data/webui... File chrome/test/data/webui/print_preview/native_layer_stub.js (right): https://codereview.chromium.org/2962983002/diff/160001/chrome/test/data/webui... chrome/test/data/webui/print_preview/native_layer_stub.js:98: if (destination.id == this.badPrinterId_) { On 2017/06/29 23:28:31, dpapad wrote: > Nit (optional): > > return destination.id == this.badPrinterId_ ? > Promise.reject('SETTINGS_INVALID') : > Promise.resolve(requestId); > > > Also can we use print_preview.PreviewArea.EventType.SETTINGS_INVALID instead of > using the string literal? Done. https://codereview.chromium.org/2962983002/diff/160001/chrome/test/data/webui... File chrome/test/data/webui/print_preview/print_preview_tests.js (right): https://codereview.chromium.org/2962983002/diff/160001/chrome/test/data/webui... chrome/test/data/webui/print_preview/print_preview_tests.js:1282: nativeLayer.setInvalidPrinterId('FooDevice'); On 2017/06/29 23:28:31, dpapad wrote: > Nit: Maybe add a comment that "FooDevice" is the currently selected or default > printer. Done. https://codereview.chromium.org/2962983002/diff/160001/chrome/test/data/webui... chrome/test/data/webui/print_preview/print_preview_tests.js:1288: // is not running. However, it should not be the invalid settings On 2017/06/29 23:28:31, dpapad wrote: > I think the 2nd sentence is no longer accurate. 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...
Print preview UI LGTM with a request. https://codereview.chromium.org/2962983002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/2962983002/diff/240001/chrome/browser/resourc... chrome/browser/resources/print_preview/print_preview.js:958: this.previewArea_.cancelTimeout(); This line and the next are now already happening within previewArea_ itself, before the SETTINGS_INVALID is fired. Can we remove them from here?
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/2962983002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/2962983002/diff/240001/chrome/browser/resourc... chrome/browser/resources/print_preview/print_preview.js:958: this.previewArea_.cancelTimeout(); On 2017/06/30 18:58:39, dpapad wrote: > This line and the next are now already happening within previewArea_ itself, > before the SETTINGS_INVALID is fired. Can we remove them from here? Done. https://codereview.chromium.org/2962983002/diff/260001/chrome/test/data/webui... File chrome/test/data/webui/print_preview/native_layer_stub.js (right): https://codereview.chromium.org/2962983002/diff/260001/chrome/test/data/webui... chrome/test/data/webui/print_preview/native_layer_stub.js:98: var rejectString = print_preview.PreviewArea.EventType.SETTINGS_INVALID; Had to change the way this works as the event name is actually a longer string 'print_preview.PreviewArea.SETTINGS_INVALID'. Is this way preferable to the old string literal or should I revert?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2962983002/diff/260001/chrome/test/data/webui... File chrome/test/data/webui/print_preview/native_layer_stub.js (right): https://codereview.chromium.org/2962983002/diff/260001/chrome/test/data/webui... chrome/test/data/webui/print_preview/native_layer_stub.js:98: var rejectString = print_preview.PreviewArea.EventType.SETTINGS_INVALID; On 2017/06/30 at 19:45:22, rbpotter wrote: > Had to change the way this works as the event name is > actually a longer string 'print_preview.PreviewArea.SETTINGS_INVALID'. > Is this way preferable to the old string literal or should I revert? I think this is preferable, not because it is shorter (it's not), but because it is less redundant. If we did compile our tests, that would be better for more reasons, but unfortunately we don't compile them.
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/2962983002/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2962983002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:791: CHECK(args->GetString(0, &callback_id)); How about instead of doing the CHECK() here, check the end result instead, which is |callback_id| is non-empty. If the check fails, it's still easy enough to figure out what went wrong, ya? https://codereview.chromium.org/2962983002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1535: DCHECK(preview_callbacks_.size() > 0); !preview_callbacks_.empty() https://codereview.chromium.org/2962983002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1535: DCHECK(preview_callbacks_.size() > 0); If I controlled the renderer and made it malicious, all I have to do is send an extra valid looking PrintHostMsg_MetafileReadyForPrinting IPC message, and we would blow past this in a release build and go out of bounds on the next line.
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/2962983002/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2962983002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:791: CHECK(args->GetString(0, &callback_id)); On 2017/06/30 22:11:34, Lei Zhang wrote: > How about instead of doing the CHECK() here, check the end result instead, which > is |callback_id| is non-empty. If the check fails, it's still easy enough to > figure out what went wrong, ya? Done. Also removed the CHECK below, since GetSettingsDictionary checks for an empty string. https://codereview.chromium.org/2962983002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1535: DCHECK(preview_callbacks_.size() > 0); On 2017/06/30 22:11:34, Lei Zhang wrote: > If I controlled the renderer and made it malicious, all I have to do is send an > extra valid looking PrintHostMsg_MetafileReadyForPrinting IPC message, and we > would blow past this in a release build and go out of bounds on the next line. Done. https://codereview.chromium.org/2962983002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1535: DCHECK(preview_callbacks_.size() > 0); On 2017/06/30 22:11:34, Lei Zhang wrote: > !preview_callbacks_.empty() Done.
lgtm https://codereview.chromium.org/2962983002/diff/300001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2962983002/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:798: CHECK(settings->GetInteger(printing::kPreviewRequestID, &request_id) && One more. Just CHECK_GT(request_id, -1).
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/2962983002/diff/300001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2962983002/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:798: CHECK(settings->GetInteger(printing::kPreviewRequestID, &request_id) && On 2017/06/30 22:58:18, Lei Zhang wrote: > One more. Just CHECK_GT(request_id, -1). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_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, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2962983002/#ps320001 (title: "Fix check")
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": 320001, "attempt_start_ts": 1499219455064950,
"parent_rev": "fa77056f089f6e1ab205b556f31ab9e34a37fc5e", "commit_rev":
"579f9d6fc5f37f9c1a40894b8a811aad58d01d3b"}
Message was sent while issue was closed.
Description was changed from ========== Print Preview: change getPreview to cr.sendWithPromise - Use cr.sendWithPromise for getPreview - Remove invalidPrinterSettings, printPreviewFailed, and updatePrintPreview global JS functions - Update some print preview tests to take advantage of browser proxy for getPreview. BUG=717296 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Print Preview: change getPreview to cr.sendWithPromise - Use cr.sendWithPromise for getPreview - Remove invalidPrinterSettings, printPreviewFailed, and updatePrintPreview global JS functions - Update some print preview tests to take advantage of browser proxy for getPreview. BUG=717296 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2962983002 Cr-Commit-Position: refs/heads/master@{#484183} Committed: https://chromium.googlesource.com/chromium/src/+/579f9d6fc5f37f9c1a40894b8a81... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:320001) as https://chromium.googlesource.com/chromium/src/+/579f9d6fc5f37f9c1a40894b8a81... |
