Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2516523002:
Fix CrOS reverting to Save as PDF and random PDF preview fail (Closed)
DescriptionFix CrOS reverting to Save as PDF and random PDF preview fail
Increasing number of saved destinations caused a bug on Chrome OS -
destination always reverted to Save as PDF.
Also caused random "PDF failed to load" preview errors. This occurred
since the destinations were modified one after the other too quickly,
which caused a race condition that crashed PDF preview. This error
occurred ~1 in 10 times.
BUG=665455, 666426, 666595
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/b12aba73d3525867511b315a45d2dbc65acd2647
Cr-Commit-Position: refs/heads/master@{#433408}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fix comments #Patch Set 3 : Fix occasional PDF error problem #
Total comments: 2
Patch Set 4 : Add helper #
Total comments: 1
Messages
Total messages: 34 (24 generated)
Description was changed from ========== Fix bug with chromeOS always reverting to the Save as PDF destination. Increasing number of saved destinations caused a bug on Chrome OS - destination always reverted to Save as PDF. Fixed and fixed some ensuing javascript errors. BUG=665455 ========== to ========== Fix bug with chromeOS always reverting to the Save as PDF destination. Increasing number of saved destinations caused a bug on Chrome OS - destination always reverted to Save as PDF. Fixed and fixed some ensuing javascript errors. BUG=665455 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Description was changed from ========== Fix bug with chromeOS always reverting to the Save as PDF destination. Increasing number of saved destinations caused a bug on Chrome OS - destination always reverted to Save as PDF. Fixed and fixed some ensuing javascript errors. BUG=665455 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix bug with chromeOS always reverting to the Save as PDF destination. Increasing number of saved destinations caused a bug on Chrome OS - destination always reverted to Save as PDF. Fixed and fixed some ensuing javascript errors. BUG=665455 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
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...
LGTM. Would be nice to add a test to cover this regression. https://codereview.chromium.org/2516523002/diff/1/chrome/browser/resources/pr... File chrome/browser/resources/print_preview/data/print_ticket_store.js (right): https://codereview.chromium.org/2516523002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/data/print_ticket_store.js:561: if (!this.destinationStore_.selectedDestination) Can you add a comment explaining how this function can executed before selectedDestination is populated?
https://codereview.chromium.org/2516523002/diff/1/chrome/browser/resources/pr... File chrome/browser/resources/print_preview/data/app_state.js (right): https://codereview.chromium.org/2516523002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/data/app_state.js:140: get selectedDestinationId() { I am also wondering why are all these getters (line 140-187). Can't all callers use a single getter (selectedDestination at line 193)? https://codereview.chromium.org/2516523002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/data/app_state.js:302: if (!this.isInitialized_ || !dest) Also per the type annotation !print_preview.Destination above, |dest| should never be null/undefined, so this checks seems odd. Can you either update the annotation or remove the check if not necessary?
https://codereview.chromium.org/2516523002/diff/1/chrome/browser/resources/pr... File chrome/browser/resources/print_preview/data/app_state.js (right): https://codereview.chromium.org/2516523002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/data/app_state.js:140: get selectedDestinationId() { On 2016/11/18 00:33:20, dpapad wrote: > I am also wondering why are all these getters (line 140-187). Can't all callers > use a single getter (selectedDestination at line 193)? Done. They were here because they were here before I made this change, so I left them in and just updated them to work with the new code. Apparently only 2 of them were ever used... https://codereview.chromium.org/2516523002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/data/app_state.js:302: if (!this.isInitialized_ || !dest) On 2016/11/18 00:33:20, dpapad wrote: > Also per the type annotation !print_preview.Destination above, |dest| should > never be null/undefined, so this checks seems odd. Can you either update the > annotation or remove the check if not necessary? Removed this and updated the place it was getting called with a null destination. https://codereview.chromium.org/2516523002/diff/1/chrome/browser/resources/pr... File chrome/browser/resources/print_preview/data/print_ticket_store.js (right): https://codereview.chromium.org/2516523002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/data/print_ticket_store.js:561: if (!this.destinationStore_.selectedDestination) On 2016/11/18 00:29:16, dpapad wrote: > Can you add a comment explaining how this function can executed before > selectedDestination is populated? I was getting an error here previously but after fixing the other issues I have not been able to duplicate it. It also should not be possible so I have removed this check.
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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Fix bug with chromeOS always reverting to the Save as PDF destination. Increasing number of saved destinations caused a bug on Chrome OS - destination always reverted to Save as PDF. Fixed and fixed some ensuing javascript errors. BUG=665455 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix CrOS reverting to Save as PDF and random PDF preview fail Increasing number of saved destinations caused a bug on Chrome OS - destination always reverted to Save as PDF. Also caused random PDF previews as updated the destination too quickly, which caused a race condition that crashed PDF preview loading. This error occurred ~1 in 10 times. BUG=665455,666426,666595 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
rbpotter@chromium.org changed reviewers: + 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...
Added a fix for PDF preview failures.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
Description was changed from ========== Fix CrOS reverting to Save as PDF and random PDF preview fail Increasing number of saved destinations caused a bug on Chrome OS - destination always reverted to Save as PDF. Also caused random PDF previews as updated the destination too quickly, which caused a race condition that crashed PDF preview loading. This error occurred ~1 in 10 times. BUG=665455,666426,666595 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix CrOS reverting to Save as PDF and random PDF preview fail Increasing number of saved destinations caused a bug on Chrome OS - destination always reverted to Save as PDF. Also caused random "PDF failed to load" preview errors. This occurred since the destinations were modified one after the other too quickly, which caused a race condition that crashed PDF preview. This error occurred ~1 in 10 times. BUG=665455,666426,666595 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2516523002/diff/40001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2516523002/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:941: if (this.appState_.selectedDestination && Can you package these three checks in a helper and re-use? They are currently repeated in 3 places in this file (lines 672-674, 684-686, and 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/2516523002/diff/40001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2516523002/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:941: if (this.appState_.selectedDestination && On 2016/11/19 01:37:58, dpapad wrote: > Can you package these three checks in a helper and re-use? They are currently > repeated in 3 places in this file (lines 672-674, 684-686, and here). 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/2516523002/#ps60001 (title: "Add helper")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix CrOS reverting to Save as PDF and random PDF preview fail Increasing number of saved destinations caused a bug on Chrome OS - destination always reverted to Save as PDF. Also caused random "PDF failed to load" preview errors. This occurred since the destinations were modified one after the other too quickly, which caused a race condition that crashed PDF preview. This error occurred ~1 in 10 times. BUG=665455,666426,666595 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix CrOS reverting to Save as PDF and random PDF preview fail Increasing number of saved destinations caused a bug on Chrome OS - destination always reverted to Save as PDF. Also caused random "PDF failed to load" preview errors. This occurred since the destinations were modified one after the other too quickly, which caused a race condition that crashed PDF preview. This error occurred ~1 in 10 times. BUG=665455,666426,666595 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix CrOS reverting to Save as PDF and random PDF preview fail Increasing number of saved destinations caused a bug on Chrome OS - destination always reverted to Save as PDF. Also caused random "PDF failed to load" preview errors. This occurred since the destinations were modified one after the other too quickly, which caused a race condition that crashed PDF preview. This error occurred ~1 in 10 times. BUG=665455,666426,666595 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix CrOS reverting to Save as PDF and random PDF preview fail Increasing number of saved destinations caused a bug on Chrome OS - destination always reverted to Save as PDF. Also caused random "PDF failed to load" preview errors. This occurred since the destinations were modified one after the other too quickly, which caused a race condition that crashed PDF preview. This error occurred ~1 in 10 times. BUG=665455,666426,666595 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/b12aba73d3525867511b315a45d2dbc65acd2647 Cr-Commit-Position: refs/heads/master@{#433408} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b12aba73d3525867511b315a45d2dbc65acd2647 Cr-Commit-Position: refs/heads/master@{#433408}
Message was sent while issue was closed.
https://codereview.chromium.org/2516523002/diff/60001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2516523002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:662: return this.appState_.selectedDestination && Nit: This method only refers to |this.appState_| member variables, so it belongs to the AppState class instead of here. This is known as "feature envy", see https://waog.wordpress.com/2014/08/25/code-smell-feature-envy/. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
