Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2948723002:
Print Preview: Change print to cr.sendWithPromise (Closed)
DescriptionPrint Preview: Change print to cr.sendWithPromise
Change the print message in print preview to send with promise, and use
this to eliminate some global JS functions/CallJavascriptFunctionUnsafe:
- onFileSelectionCompleted
- onFileSelectionCancelled
- printToCloud
Also remove the print-failed webui event.
BUG=717296
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2948723002
Cr-Commit-Position: refs/heads/master@{#482095}
Committed: https://chromium.googlesource.com/chromium/src/+/d29fc0d18dc459eb543f50860d7d244694bebc5d
Patch Set 1 #Patch Set 2 : Fix tests #Patch Set 3 : nit #
Total comments: 3
Patch Set 4 : Address comments #
Total comments: 25
Patch Set 5 : Address comments #Patch Set 6 : Fix #Patch Set 7 : nits #
Total comments: 2
Patch Set 8 : Address comments and add close_ param #
Total comments: 5
Messages
Total messages: 53 (36 generated)
Description was changed from ========== Print Preview: Change print to cr.sendWithPromise Change the print message in print preview to send with promise, and use this to eliminate some global JS functions/CallJavascriptFunctionUnsafe: - onFileSelectionCompleted - onFileSelectionCancelled - printToCloud Also remove the print-failed webui event. Cases: PDF: If successful, goes to file selection complete or goes to preset file name. Call onFileSelectionCompleted in either case. If fails, calls FileSelectionCancelled, dialog stays open. Extension/Privet: Success-> Closes the dialog. Failure-> print-fail event. Cloud: Calls print to cloud. Other than a bad ticket, no way to fail since actual printing is done later. Local: Hides dialog either way. Previously dialog attempted to hide twice, once in JS, and once via C++. Consolidate to 1 hide in JS. BUG=717296 ========== to ========== Print Preview: Change print to cr.sendWithPromise Change the print message in print preview to send with promise, and use this to eliminate some global JS functions/CallJavascriptFunctionUnsafe: - onFileSelectionCompleted - onFileSelectionCancelled - printToCloud Also remove the print-failed webui event. Cases: PDF: If successful, goes to file selection complete or goes to preset file name. Call onFileSelectionCompleted in either case. If fails, calls FileSelectionCancelled, dialog stays open. Extension/Privet: Success-> Closes the dialog. Failure-> print-fail event. Cloud: Calls print to cloud. Other than a bad ticket, no way to fail since actual printing is done later. Local: Hides dialog either way. Previously dialog attempted to hide twice, once in JS, and once via C++. Consolidate to 1 hide in JS. BUG=717296 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Description was changed from ========== Print Preview: Change print to cr.sendWithPromise Change the print message in print preview to send with promise, and use this to eliminate some global JS functions/CallJavascriptFunctionUnsafe: - onFileSelectionCompleted - onFileSelectionCancelled - printToCloud Also remove the print-failed webui event. Cases: PDF: If successful, goes to file selection complete or goes to preset file name. Call onFileSelectionCompleted in either case. If fails, calls FileSelectionCancelled, dialog stays open. Extension/Privet: Success-> Closes the dialog. Failure-> print-fail event. Cloud: Calls print to cloud. Other than a bad ticket, no way to fail since actual printing is done later. Local: Hides dialog either way. Previously dialog attempted to hide twice, once in JS, and once via C++. Consolidate to 1 hide in JS. BUG=717296 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Print Preview: Change print to cr.sendWithPromise Change the print message in print preview to send with promise, and use this to eliminate some global JS functions/CallJavascriptFunctionUnsafe: - onFileSelectionCompleted - onFileSelectionCancelled - printToCloud Also remove the print-failed webui event. Cases: PDF: If successful, goes to file selection complete or goes to preset file name. Call onFileSelectionCompleted in either case. If fails, calls FileSelectionCancelled, dialog stays open. Extension/Privet: Success-> Closes the dialog. Failure-> print-fail event. Cloud: Calls print to cloud. Other than a bad ticket, no way to fail since actual printing is done later. Local: Hides dialog either way. Previously dialog attempted to hide twice, once in JS, and once via C++. Consolidate to 1 hide in JS. BUG=717296 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
rbpotter@chromium.org changed reviewers: + dpapad@chromium.org, thestig@chromium.org
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rbpotter@chromium.org changed reviewers: - thestig@chromium.org
Move the print message to cr.sendWithPromise to eliminate some global JS functions. Note that resolving promise happens at different points in the printing process for different types of destinations. It happens after selecting the file for PDF, retrieving the data (but before submitting job) for cloud, just before starting the print job for local printers, and after a successful print is reported for privet/extension. cc thestig@ since, while this is all webui code, the logic for how print requests are handled for different destinations is very related to the printing side. https://codereview.chromium.org/2948723002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (left): https://codereview.chromium.org/2948723002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:996: print_preview_ui()->OnHidePreviewDialog(); Moved this to JS side - hide will be called from there when the promise is resolved. https://codereview.chromium.org/2948723002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1738: ClosePreviewDialog(); Now done from JS https://codereview.chromium.org/2948723002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1811: ClosePreviewDialog(); Now done from JS
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2948723002/diff/80001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/2948723002/diff/80001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:588: * @param {?string=} data Data for cloud print if this is a cloud I don't think @param annotation does anything if you place it before the .then(). IIUC this should be after (right before the "function" keyword). .then( /** @param .... */ function() {...}) https://codereview.chromium.org/2948723002/diff/80001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:592: function(data) { Would lines 591-624 be more readable if we untangle the separate cases as follows: var whenPrintDone = this.nativeLayer_.print(....); if (!destination.isLocal) { whenPrintDone.then( this.onPrintToCloud_.bind(this), this.onSettingsInvalid_.bind(this)); } else if (destination.isPrivet || destination.isExtension) { whenPrintDone.then( this.close_.bind(this), this.onPrintFailed_.bind(this)); } else if (...) { ... } else { var boundHideDialog = function() { this.nativeLayer_.startHideDialog() }.bind(this); whenPrintDone.then(boundHideDialog, boundHideDialog); } https://codereview.chromium.org/2948723002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/print_preview/native_layer_stub.js (right): https://codereview.chromium.org/2948723002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/print_preview/native_layer_stub.js:138: isPrintStarted: function() { return this.printStarted_; }, Is this method and the boolean used anymore? It seems that printStarted_ will always be false from now on.
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...
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
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...
Accidentally deleted the wrong patchset (the one with comments) when trying to remove one I uploaded too early. However, all comments are addressed. Also, the diff with patchset 3 is identical to what the diff with patchset 4 would have been except for the changes to print_preview_handler.cc, which were made between PS 3 and PS4. On 2017/06/22 01:15:22, dpapad wrote: > https://codereview.chromium.org/2948723002/diff/80001/chrome/browser/resource... > File chrome/browser/resources/print_preview/print_preview.js (right): > > https://codereview.chromium.org/2948723002/diff/80001/chrome/browser/resource... > chrome/browser/resources/print_preview/print_preview.js:588: * @param {?string=} > data Data for cloud print if this is a cloud > I don't think @param annotation does anything if you place it before the > .then(). IIUC this should be after (right before the "function" keyword). > > .then( > /** @param .... */ > function() {...}) Done. Not needed any more with changes suggested below. > https://codereview.chromium.org/2948723002/diff/80001/chrome/browser/resource... > chrome/browser/resources/print_preview/print_preview.js:592: function(data) { > Would lines 591-624 be more readable if we untangle the separate cases as > follows: > > var whenPrintDone = this.nativeLayer_.print(....); > > if (!destination.isLocal) { > whenPrintDone.then( > this.onPrintToCloud_.bind(this), > this.onSettingsInvalid_.bind(this)); > } else if (destination.isPrivet || destination.isExtension) { > whenPrintDone.then( > this.close_.bind(this), > this.onPrintFailed_.bind(this)); > } else if (...) { > ... > } else { > var boundHideDialog = function() { > this.nativeLayer_.startHideDialog() > }.bind(this); > whenPrintDone.then(boundHideDialog, boundHideDialog); > } Done. > https://codereview.chromium.org/2948723002/diff/80001/chrome/test/data/webui/... > File chrome/test/data/webui/print_preview/native_layer_stub.js (right): > > https://codereview.chromium.org/2948723002/diff/80001/chrome/test/data/webui/... > chrome/test/data/webui/print_preview/native_layer_stub.js:138: isPrintStarted: > function() { return this.printStarted_; }, > Is this method and the boolean used anymore? It seems that printStarted_ will > always be false from now on. Not used; removed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM! https://codereview.chromium.org/2948723002/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/print_preview/native_layer_stub.js (right): https://codereview.chromium.org/2948723002/diff/120001/chrome/test/data/webui... chrome/test/data/webui/print_preview/native_layer_stub.js:102: this.methodCalled('print'); Nit (for a potential follow CL): Depending on how thorough are tests are, we could forward all the parameters passed to print() to methodCalled() as a 2nd param, and assert that the JS sends the expected params to C++. https://codereview.chromium.org/2948723002/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/print_preview/print_preview_tests.js (right): https://codereview.chromium.org/2948723002/diff/120001/chrome/test/data/webui... chrome/test/data/webui/print_preview/print_preview_tests.js:1312: return nativeLayer.whenCalled('print'); Related to comment in native_layer_stub.js, here we could be asserting that nativeLayer.print() was called correctly.
Thanks! Will wait to land this until I have a chance to double check the final patch with Extension/Privet printing (hopefully tomorrow morning). We don't have any automated test coverage for those cases so need to manually verify this does not break anything. https://codereview.chromium.org/2948723002/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/print_preview/native_layer_stub.js (right): https://codereview.chromium.org/2948723002/diff/120001/chrome/test/data/webui... chrome/test/data/webui/print_preview/native_layer_stub.js:102: this.methodCalled('print'); On 2017/06/23 00:13:57, dpapad wrote: > Nit (for a potential follow CL): Depending on how thorough are tests are, we > could forward all the parameters passed to print() to methodCalled() as a 2nd > param, and assert that the JS sends the expected params to C++. That would be useful, so will add it. The existing test is intended to verify that the call to print doesn't get blocked somewhere in JS by logic that checks the preview state, so it does not really care what most parameters are. However, can add verification that they at least seem reasonable.
thestig@chromium.org changed reviewers: + thestig@chromium.org
https://codereview.chromium.org/2948723002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2948723002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:205: // Get the print job settings dictionary from |args|. The caller takes Still referring to |args|. https://codereview.chromium.org/2948723002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:206: // ownership of the returned DictionaryValue. Returns NULL on failure. BTW, no need to mention ownership since unique_ptr implies it now. https://codereview.chromium.org/2948723002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:208: std::string json_str) { Pass by const-ref, yet again. Does "git cl lint" warn about this? My guess is no. https://codereview.chromium.org/2948723002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:868: CHECK(args->GetString(0, &callback_id) && !callback_id.empty()); CHECK() the 2 checks separately, so if this ever fails, we can tell which CHECK() failed. https://codereview.chromium.org/2948723002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:986: RejectJavascriptCallback(base::Value(callback_id), base::Value()); Is this missing a return? https://codereview.chromium.org/2948723002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1008: // This tries to activate the initiator as well, so do not clear the Is this comment still valid? https://codereview.chromium.org/2948723002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1445: pdf_callback_id_.clear(); Is this suppose to return? https://codereview.chromium.org/2948723002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1838: ResolveJavascriptCallback(base::Value(callback_id), base::Value()); We are calling: ResolveJavascriptCallback(base::Value(callback_id), ...) either way. Merge into 1 call? https://codereview.chromium.org/2948723002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_handler.h (right): https://codereview.chromium.org/2948723002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.h:416: std::string privet_search_callback_id_ = ""; You don't need to explicitly initialize std::string to "". It's not a Plain Old Datatype. https://codereview.chromium.org/2948723002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.h:416: std::string privet_search_callback_id_ = ""; Wrap with #if BUILDFLAG(ENABLE_SERVICE_DISCOVERY) ? https://codereview.chromium.org/2948723002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.h:418: std::string privet_print_callback_id_ = ""; Might as well add comments for consistency.
Patchset #6 (id:160001) has been deleted
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/2948723002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2948723002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:205: // Get the print job settings dictionary from |args|. The caller takes On 2017/06/23 08:30:01, Lei Zhang wrote: > Still referring to |args|. Done. https://codereview.chromium.org/2948723002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:206: // ownership of the returned DictionaryValue. Returns NULL on failure. On 2017/06/23 08:30:01, Lei Zhang wrote: > BTW, no need to mention ownership since unique_ptr implies it now. Done. https://codereview.chromium.org/2948723002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:208: std::string json_str) { On 2017/06/23 08:30:01, Lei Zhang wrote: > Pass by const-ref, yet again. Does "git cl lint" warn about this? My guess is > no. Done. No, git cl lint doesn't warn about this. https://codereview.chromium.org/2948723002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:868: CHECK(args->GetString(0, &callback_id) && !callback_id.empty()); On 2017/06/23 08:30:01, Lei Zhang wrote: > CHECK() the 2 checks separately, so if this ever fails, we can tell which > CHECK() failed. Done. https://codereview.chromium.org/2948723002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:986: RejectJavascriptCallback(base::Value(callback_id), base::Value()); On 2017/06/23 08:30:01, Lei Zhang wrote: > Is this missing a return? Done. https://codereview.chromium.org/2948723002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1008: // This tries to activate the initiator as well, so do not clear the On 2017/06/23 08:30:01, Lei Zhang wrote: > Is this comment still valid? Yes, because OnHidePreviewDialog will get called after JS calls chrome.send("hidePreview") - see HandleHidePreview(). Previously it was actually getting called twice, since JS did this anyway... To be safer with the ClearInitiatorDetails() below, I am moving that into the HandleHidePreview function. https://codereview.chromium.org/2948723002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1445: pdf_callback_id_.clear(); On 2017/06/23 08:30:01, Lei Zhang wrote: > Is this suppose to return? Return type is void. ClosePreviewDialog() does not return anything either, so I think previously it was just using that as a return. https://codereview.chromium.org/2948723002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1838: ResolveJavascriptCallback(base::Value(callback_id), base::Value()); On 2017/06/23 08:30:01, Lei Zhang wrote: > We are calling: ResolveJavascriptCallback(base::Value(callback_id), ...) either > way. Merge into 1 call? Below is RejectJavascriptCallback, not Resolve, so they need to be separate calls. https://codereview.chromium.org/2948723002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_handler.h (right): https://codereview.chromium.org/2948723002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.h:416: std::string privet_search_callback_id_ = ""; On 2017/06/23 08:30:01, Lei Zhang wrote: > You don't need to explicitly initialize std::string to "". It's not a Plain Old > Datatype. Done. https://codereview.chromium.org/2948723002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.h:418: std::string privet_print_callback_id_ = ""; On 2017/06/23 08:30:01, Lei Zhang wrote: > Might as well add comments for consistency. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2948723002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2948723002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1445: pdf_callback_id_.clear(); On 2017/06/23 17:34:30, rbpotter wrote: > On 2017/06/23 08:30:01, Lei Zhang wrote: > > Is this suppose to return? > > Return type is void. ClosePreviewDialog() does not return anything either, > so I think previously it was just using that as a return. But now execution continues onwards, whereas before it didn't. https://codereview.chromium.org/2948723002/diff/200001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_handler.h (right): https://codereview.chromium.org/2948723002/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.h:421: #endif // ENABLE_SERVICE_DISCOVERY I wouldn't bother with the comment when the #if block is very short. If there is a comment, it should match the #if verbatim. e.g. // BUILDFLAG(ENABLE_SERVICE_DISCOVERY) But I know there's a lot of code that doesn't do it right.
FYI, modified the close_ function in print preview. The "closePrintPreviewDialog" message is sent only to update some UMA statistics about how many times the user tried to regenerate print preview before cancelling/otherwise exiting the dialog without printing - see HandleClosePreviewDialog. There is no reason to send this message either in the new case of calling close_ due to successful print or in the case of calling it after a cloud print job has been submitted successfully. Have changed things to reflect this. https://codereview.chromium.org/2948723002/diff/200001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_handler.h (right): https://codereview.chromium.org/2948723002/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.h:421: #endif // ENABLE_SERVICE_DISCOVERY On 2017/06/23 20:39:57, Lei Zhang wrote: > I wouldn't bother with the comment when the #if block is very short. If there is > a comment, it should match the #if verbatim. e.g. // > BUILDFLAG(ENABLE_SERVICE_DISCOVERY) > > But I know there's a lot of code that doesn't do it right. 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...
Description was changed from ========== Print Preview: Change print to cr.sendWithPromise Change the print message in print preview to send with promise, and use this to eliminate some global JS functions/CallJavascriptFunctionUnsafe: - onFileSelectionCompleted - onFileSelectionCancelled - printToCloud Also remove the print-failed webui event. Cases: PDF: If successful, goes to file selection complete or goes to preset file name. Call onFileSelectionCompleted in either case. If fails, calls FileSelectionCancelled, dialog stays open. Extension/Privet: Success-> Closes the dialog. Failure-> print-fail event. Cloud: Calls print to cloud. Other than a bad ticket, no way to fail since actual printing is done later. Local: Hides dialog either way. Previously dialog attempted to hide twice, once in JS, and once via C++. Consolidate to 1 hide in JS. BUG=717296 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Print Preview: Change print to cr.sendWithPromise Change the print message in print preview to send with promise, and use this to eliminate some global JS functions/CallJavascriptFunctionUnsafe: - onFileSelectionCompleted - onFileSelectionCancelled - printToCloud Also remove the print-failed webui event. BUG=717296 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2948723002/diff/220001/chrome/browser/resourc... File chrome/browser/resources/print_preview/native_layer.js (right): https://codereview.chromium.org/2948723002/diff/220001/chrome/browser/resourc... chrome/browser/resources/print_preview/native_layer.js:515: chrome.send('closePrintPreviewDialog'); Nit (optional): chrome.send(isCancel ? 'closePrintPreviewDialog', 'dialogClose');
lgtm
https://codereview.chromium.org/2948723002/diff/220001/chrome/browser/resourc... File chrome/browser/resources/print_preview/native_layer.js (right): https://codereview.chromium.org/2948723002/diff/220001/chrome/browser/resourc... chrome/browser/resources/print_preview/native_layer.js:515: chrome.send('closePrintPreviewDialog'); On 2017/06/23 23:17:26, dpapad wrote: > Nit (optional): > > chrome.send(isCancel ? 'closePrintPreviewDialog', 'dialogClose'); We want to send dialogClose either way though, so this wouldn't this require adding if (!isCancel) below?
https://codereview.chromium.org/2948723002/diff/220001/chrome/browser/resourc... File chrome/browser/resources/print_preview/native_layer.js (right): https://codereview.chromium.org/2948723002/diff/220001/chrome/browser/resourc... chrome/browser/resources/print_preview/native_layer.js:515: chrome.send('closePrintPreviewDialog'); On 2017/06/23 at 23:34:35, rbpotter wrote: > On 2017/06/23 23:17:26, dpapad wrote: > > Nit (optional): > > > > chrome.send(isCancel ? 'closePrintPreviewDialog', 'dialogClose'); > > We want to send dialogClose either way though, so this wouldn't this > require adding if (!isCancel) below? Ah right, I misread the original code. Ignore my suggestion.
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/2948723002/#ps220001 (title: "Address comments and add close_ param")
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": 220001, "attempt_start_ts": 1498264959446690,
"parent_rev": "7e19dfa32df45d1541fde8f76bcf8eb819889c0c", "commit_rev":
"d29fc0d18dc459eb543f50860d7d244694bebc5d"}
Message was sent while issue was closed.
Description was changed from ========== Print Preview: Change print to cr.sendWithPromise Change the print message in print preview to send with promise, and use this to eliminate some global JS functions/CallJavascriptFunctionUnsafe: - onFileSelectionCompleted - onFileSelectionCancelled - printToCloud Also remove the print-failed webui event. BUG=717296 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Print Preview: Change print to cr.sendWithPromise Change the print message in print preview to send with promise, and use this to eliminate some global JS functions/CallJavascriptFunctionUnsafe: - onFileSelectionCompleted - onFileSelectionCancelled - printToCloud Also remove the print-failed webui event. BUG=717296 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2948723002 Cr-Commit-Position: refs/heads/master@{#482095} Committed: https://chromium.googlesource.com/chromium/src/+/d29fc0d18dc459eb543f50860d7d... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:220001) as https://chromium.googlesource.com/chromium/src/+/d29fc0d18dc459eb543f50860d7d...
Message was sent while issue was closed.
https://codereview.chromium.org/2948723002/diff/220001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2948723002/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:872: RejectJavascriptCallback(base::Value(callback_id), base::Value(-1)); Wait, don't we need to return after a failure like this?
Message was sent while issue was closed.
https://codereview.chromium.org/2948723002/diff/220001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2948723002/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:872: RejectJavascriptCallback(base::Value(callback_id), base::Value(-1)); On 2017/07/11 05:11:10, Lei Zhang wrote: > Wait, don't we need to return after a failure like this? Yes for the case below. Should probably just crash here. The ticket should never be empty unless something is wrong with the print preview JS code. https://codereview.chromium.org/2980603002/ |
