Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2741483002:
Check for RPCS printers and return error if Win8+ (Closed)
DescriptionCheck for RPCS printers and return error if Win8+
RPCS printers will cause a crash in DocumentProperties on Windows 8 and
above. Detect these printers before calling DocumentProperties and
return invalid settings so that the preview displays an error message.
Change preview area so that "Loading.." does not overwrite the error
message when invalid printer settings are obtained.
BUG=679160
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2741483002
Cr-Commit-Position: refs/heads/master@{#455789}
Committed: https://chromium.googlesource.com/chromium/src/+/f197528d286b7ecb15e1d0f8d6f05609d8c4b6a4
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fix buffer #Patch Set 3 : Fix preview area #
Total comments: 6
Patch Set 4 : Move timeout cancellation to print_preview, disable button #Patch Set 5 : Format #
Messages
Total messages: 27 (18 generated)
Description was changed from ========== Check for RPCS printers and return error if Win8+ RPCS printers will cause a crash in DocumentProperties on Windows 8 and above. Detect these printers before calling DocumentProperties and return invalid settings so that the preview displays an error message. Change preview area so that "Loading.." does not overwrite the error message when invalid printer settings are obtained. BUG=679160 ========== to ========== Check for RPCS printers and return error if Win8+ RPCS printers will cause a crash in DocumentProperties on Windows 8 and above. Detect these printers before calling DocumentProperties and return invalid settings so that the preview displays an error message. Change preview area so that "Loading.." does not overwrite the error message when invalid printer settings are obtained. BUG=679160 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Check for RPCS printers and return error if Win8+ RPCS printers will cause a crash in DocumentProperties on Windows 8 and above. Detect these printers before calling DocumentProperties and return invalid settings so that the preview displays an error message. Change preview area so that "Loading.." does not overwrite the error message when invalid printer settings are obtained. BUG=679160 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Check for RPCS printers and return error if Win8+ RPCS printers will cause a crash in DocumentProperties on Windows 8 and above. Detect these printers before calling DocumentProperties and return invalid settings so that the preview displays an error message. Change preview area so that "Loading.." does not overwrite the error message when invalid printer settings are obtained. BUG=679160 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
rbpotter@chromium.org changed reviewers: + 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...
https://codereview.chromium.org/2741483002/diff/1/chrome/browser/resources/pr... File chrome/browser/resources/print_preview/previewarea/preview_area.js (right): https://codereview.chromium.org/2741483002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/previewarea/preview_area.js:309: if (message == loadTimeData.getString('invalidPrinterSettings') && Is there ever a case where we want to show a message, and then let "preview loading..." overwrite the error message? https://codereview.chromium.org/2741483002/diff/1/printing/backend/win_helper.cc File printing/backend/win_helper.cc (right): https://codereview.chromium.org/2741483002/diff/1/printing/backend/win_helper... printing/backend/win_helper.cc:483: wchar_t* buf = static_cast<wchar_t*>(calloc(33 * sizeof(wchar_t), 1)); Did we forget to free |buf| ? Maybe use std::vector<wchar_t> instead and pass buf.data() to DeviceCapabilities() ? https://codereview.chromium.org/2741483002/diff/1/printing/backend/win_helper... printing/backend/win_helper.cc:487: if (wcscmp(buf, kRPCSLanguage) == 0) Just: return wcscmp(buf, kRPCSLanguage) == 0;
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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...
https://codereview.chromium.org/2741483002/diff/1/chrome/browser/resources/pr... File chrome/browser/resources/print_preview/previewarea/preview_area.js (right): https://codereview.chromium.org/2741483002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/previewarea/preview_area.js:309: if (message == loadTimeData.getString('invalidPrinterSettings') && On 2017/03/08 20:27:22, Lei Zhang (super slow) wrote: > Is there ever a case where we want to show a message, and then let "preview > loading..." overwrite the error message? Probably not. This shouldn't occur for the other 2 calls to this function though, as one is after we save to PDF and the other is when we display the PDF in PDF viewer. The specific issue here is that there is a race between the "Destination Capabilities" event (which sets a timeout to display "Loading Preview") and the Invalid Settings event, which leads to this function being called. Occasionally the invalid settings event gets there after the timeout has expired and displays as expected but most of the time it seems to arrive after the capabilities event but before the timeout expires, and therefore gets overwritten. Adding dpapad to verify this addition makes sense as a fix for that race. https://codereview.chromium.org/2741483002/diff/1/printing/backend/win_helper.cc File printing/backend/win_helper.cc (right): https://codereview.chromium.org/2741483002/diff/1/printing/backend/win_helper... printing/backend/win_helper.cc:483: wchar_t* buf = static_cast<wchar_t*>(calloc(33 * sizeof(wchar_t), 1)); On 2017/03/08 20:27:22, Lei Zhang (super slow) wrote: > Did we forget to free |buf| ? Maybe use std::vector<wchar_t> instead and pass > buf.data() to DeviceCapabilities() ? Done. https://codereview.chromium.org/2741483002/diff/1/printing/backend/win_helper... printing/backend/win_helper.cc:487: if (wcscmp(buf, kRPCSLanguage) == 0) On 2017/03/08 20:27:22, Lei Zhang (super slow) wrote: > Just: return wcscmp(buf, kRPCSLanguage) == 0; Done.
lgtm https://codereview.chromium.org/2741483002/diff/40001/printing/backend/win_he... File printing/backend/win_helper.cc (right): https://codereview.chromium.org/2741483002/diff/40001/printing/backend/win_he... printing/backend/win_helper.cc:480: int numLanguages = DeviceCapabilities(name, port, DC_PERSONALITY, NULL, NULL); num_languages https://codereview.chromium.org/2741483002/diff/40001/printing/backend/win_he... printing/backend/win_helper.cc:484: memset(buf.data(), 0, 33 * sizeof(wchar_t)); std::vector<wchar_t> buf(33, 0);
https://codereview.chromium.org/2741483002/diff/40001/chrome/browser/resource... File chrome/browser/resources/print_preview/previewarea/preview_area.js (right): https://codereview.chromium.org/2741483002/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/previewarea/preview_area.js:309: if (this.loadingTimeout_) { The same logic to clear the timeout is repeated in other places in this file, see onPreviewGenerationFailed and onPluginLoaded. Can we create a cancelTimeout() helper method instead and use it in all places? Having said that, I am still trying to understand the case where we need to cancel the timeout here. I don't fully understand why it is now the responsibility of showCustomMessage() to do anything about the timeout. Is there a more appropriate place we could move this, outside showCustomMessage?
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/2741483002/diff/40001/chrome/browser/resource... File chrome/browser/resources/print_preview/previewarea/preview_area.js (right): https://codereview.chromium.org/2741483002/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/previewarea/preview_area.js:309: if (this.loadingTimeout_) { On 2017/03/08 23:37:01, dpapad wrote: > The same logic to clear the timeout is repeated in other places in this file, > see onPreviewGenerationFailed and onPluginLoaded. Can we create a > cancelTimeout() helper method instead and use it in all places? > > Having said that, I am still trying to understand the case where we need to > cancel the timeout here. I don't fully understand why it is now the > responsibility of showCustomMessage() to do anything about the timeout. Is there > a more appropriate place we could move this, outside showCustomMessage? Refactored. I moved this into print_preview in onSettingsInvalid_, the function that calls this. Let me know if this seems like a more reasonable place for it. https://codereview.chromium.org/2741483002/diff/40001/printing/backend/win_he... File printing/backend/win_helper.cc (right): https://codereview.chromium.org/2741483002/diff/40001/printing/backend/win_he... printing/backend/win_helper.cc:480: int numLanguages = DeviceCapabilities(name, port, DC_PERSONALITY, NULL, NULL); On 2017/03/08 23:32:20, Lei Zhang (super slow) wrote: > num_languages Done. https://codereview.chromium.org/2741483002/diff/40001/printing/backend/win_he... printing/backend/win_helper.cc:484: memset(buf.data(), 0, 33 * sizeof(wchar_t)); On 2017/03/08 23:32:20, Lei Zhang (super slow) wrote: > std::vector<wchar_t> buf(33, 0); Done.
lgtm
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 thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2741483002/#ps80001 (title: "Format")
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": 1489081576539390,
"parent_rev": "cf1efee24a5d9363e8bed7d45950ead20eff2cda", "commit_rev":
"f197528d286b7ecb15e1d0f8d6f05609d8c4b6a4"}
Message was sent while issue was closed.
Description was changed from ========== Check for RPCS printers and return error if Win8+ RPCS printers will cause a crash in DocumentProperties on Windows 8 and above. Detect these printers before calling DocumentProperties and return invalid settings so that the preview displays an error message. Change preview area so that "Loading.." does not overwrite the error message when invalid printer settings are obtained. BUG=679160 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Check for RPCS printers and return error if Win8+ RPCS printers will cause a crash in DocumentProperties on Windows 8 and above. Detect these printers before calling DocumentProperties and return invalid settings so that the preview displays an error message. Change preview area so that "Loading.." does not overwrite the error message when invalid printer settings are obtained. BUG=679160 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2741483002 Cr-Commit-Position: refs/heads/master@{#455789} Committed: https://chromium.googlesource.com/chromium/src/+/f197528d286b7ecb15e1d0f8d6f0... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/f197528d286b7ecb15e1d0f8d6f0... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
