Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2920153004:
Print Preview: Change getPrivetPrinters to cr.sendWithPromise (Closed)
DescriptionPrint Preview: Change getPrivetPrinters to cr.sendWithPromise
Change the getPrivetPrinters message to use cr.sendWithPromise. Also
move the timeout to C++ from Javascript, and resolve the promise on
timeout instead of sending a separate stop message from JS.
BUG=717296
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2920153004
Cr-Commit-Position: refs/heads/master@{#478053}
Committed: https://chromium.googlesource.com/chromium/src/+/242c8082ae66fee2667469a64c3e4b91205fe75a
Patch Set 1 #Patch Set 2 : Clean up #Patch Set 3 : Fix closure compile error #
Total comments: 6
Patch Set 4 : Address comments #Patch Set 5 : Remove unnecessary callback argument #
Total comments: 5
Patch Set 6 : Address comments #Patch Set 7 : Change notation #Patch Set 8 : Change notation #
Messages
Total messages: 40 (30 generated)
Description was changed from ========== Print Preview: Change getPrivetPrinters to cr.sendWithPromise Change the getPrivetPrinters message to use cr.sendWithPromise. Also move the timeout to C++ from Javascript, and resolve the promise on timeout instead of sending a separate stop message from JS. BUG=717296 ========== to ========== Print Preview: Change getPrivetPrinters to cr.sendWithPromise Change the getPrivetPrinters message to use cr.sendWithPromise. Also move the timeout to C++ from Javascript, and resolve the promise on timeout instead of sending a separate stop message from JS. BUG=717296 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
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: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
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.
rbpotter@chromium.org changed reviewers: + dpapad@chromium.org
https://codereview.chromium.org/2920153004/diff/40001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2920153004/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:688: cr.addWebUIListener('privet-printer-added', Is fetchPreselectedDestination_ only called once? Is there any chance that we are registering this listener more than once by planing this call here? Usually listeners are registered during some obvious "initialization" step that is only executed once. In this file maybe the init_ method? https://codereview.chromium.org/2920153004/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1059: cr.addWebUIListener('privet-printer-added', Same question here. Do we need to register this listener in multiple places? It seems that if both startLoadPrivetDestinations() and fetchPreselectedDestination_() it will cause onPrivetPrinterAdded_ to execute twice for a single event occurrence. https://codereview.chromium.org/2920153004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2920153004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:691: privet_callback_id_ = callback_id; Should we assert that privet_callback_id_ is not already before assigning a new value?
https://codereview.chromium.org/2920153004/diff/40001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2920153004/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:688: cr.addWebUIListener('privet-printer-added', On 2017/06/06 21:16:33, dpapad wrote: > Is fetchPreselectedDestination_ only called once? Is there any chance that we > are registering this listener more than once by planing this call here? Usually > listeners are registered during some obvious "initialization" step that is only > executed once. In this file maybe the init_ method? Done. This can be called once or not at all, and startLoad... can be called multiple times. Have moved both listeners into init. https://codereview.chromium.org/2920153004/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1059: cr.addWebUIListener('privet-printer-added', On 2017/06/06 21:16:33, dpapad wrote: > Same question here. Do we need to register this listener in multiple places? It > seems that if both startLoadPrivetDestinations() and > fetchPreselectedDestination_() it will cause onPrivetPrinterAdded_ to execute > twice for a single event occurrence. Done. https://codereview.chromium.org/2920153004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2920153004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:691: privet_callback_id_ = callback_id; On 2017/06/06 21:16:33, dpapad wrote: > Should we assert that privet_callback_id_ is not already before assigning a new > value? 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...
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.
https://codereview.chromium.org/2920153004/diff/80001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2920153004/diff/80001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:98: * @private {!Object<Array<print_preview.DestinationOrigin>>} Should this be !Object<!Array<!print_preview.DestinationOrigin>> https://codereview.chromium.org/2920153004/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.h (right): https://codereview.chromium.org/2920153004/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.h:147: void HandleStopGetPrivetPrinters(const base::ListValue* args); Is this still used?
https://codereview.chromium.org/2920153004/diff/80001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2920153004/diff/80001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:98: * @private {!Object<Array<print_preview.DestinationOrigin>>} On 2017/06/07 20:58:41, dpapad wrote: > Should this be > !Object<!Array<!print_preview.DestinationOrigin>> It looks like everywhere this is used follows the pattern: var origins = this.loadedCloudOrigins_["some key"] || [] so it is okay if the array does not exist, and there is also handling for the array being empty, so it seems like it should be okay if there are no DestinationOrigins in the array. https://codereview.chromium.org/2920153004/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.h (right): https://codereview.chromium.org/2920153004/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.h:147: void HandleStopGetPrivetPrinters(const base::ListValue* args); On 2017/06/07 20:58:41, dpapad wrote: > Is this still used? Done. Not used.
LGTM with nit. https://codereview.chromium.org/2920153004/diff/80001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2920153004/diff/80001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:98: * @private {!Object<Array<print_preview.DestinationOrigin>>} On 2017/06/07 at 21:30:19, rbpotter wrote: > On 2017/06/07 20:58:41, dpapad wrote: > > Should this be > > !Object<!Array<!print_preview.DestinationOrigin>> > > It looks like everywhere this is used follows the pattern: > var origins = this.loadedCloudOrigins_["some key"] || [] > so it is okay if the array does not exist, and there is also handling > for the array being empty, so it seems like it should be okay if there > are no DestinationOrigins in the array. !Array<Foo> indicates that a non-null array is always expected which might contain null values. !Array<!Foo> indicates that a non-null array is always expected which will NOT contain null values. None of the two statements above rule out the case of the array being empty.
Patchset #7 (id:120001) has been deleted
On 2017/06/07 21:34:48, dpapad wrote: > LGTM with nit. > > https://codereview.chromium.org/2920153004/diff/80001/chrome/browser/resource... > File chrome/browser/resources/print_preview/data/destination_store.js (right): > > https://codereview.chromium.org/2920153004/diff/80001/chrome/browser/resource... > chrome/browser/resources/print_preview/data/destination_store.js:98: * @private > {!Object<Array<print_preview.DestinationOrigin>>} > On 2017/06/07 at 21:30:19, rbpotter wrote: > > On 2017/06/07 20:58:41, dpapad wrote: > > > Should this be > > > !Object<!Array<!print_preview.DestinationOrigin>> > > > > It looks like everywhere this is used follows the pattern: > > var origins = this.loadedCloudOrigins_["some key"] || [] > > so it is okay if the array does not exist, and there is also handling > > for the array being empty, so it seems like it should be okay if there > > are no DestinationOrigins in the array. > > !Array<Foo> indicates that a non-null array is always expected which might > contain null values. > !Array<!Foo> indicates that a non-null array is always expected which will NOT > contain null values. > > None of the two statements above rule out the case of the array being empty. Updated to !Object<Array<!Origin>> as there should be no null elements in the array. However the object may or may not have any defined arrays. array, however
> Updated to !Object<Array<!Origin>> as there should be no null elements > in the array. However the object may or may not have any defined arrays. > array, however SG, thanks.
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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
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/2920153004/#ps160001 (title: "Change notation")
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": 160001, "attempt_start_ts": 1496949895008250,
"parent_rev": "5671d4fe7016d6a2306db39021fb16d51c6ac543", "commit_rev":
"242c8082ae66fee2667469a64c3e4b91205fe75a"}
Message was sent while issue was closed.
Description was changed from ========== Print Preview: Change getPrivetPrinters to cr.sendWithPromise Change the getPrivetPrinters message to use cr.sendWithPromise. Also move the timeout to C++ from Javascript, and resolve the promise on timeout instead of sending a separate stop message from JS. BUG=717296 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Print Preview: Change getPrivetPrinters to cr.sendWithPromise Change the getPrivetPrinters message to use cr.sendWithPromise. Also move the timeout to C++ from Javascript, and resolve the promise on timeout instead of sending a separate stop message from JS. BUG=717296 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2920153004 Cr-Commit-Position: refs/heads/master@{#478053} Committed: https://chromium.googlesource.com/chromium/src/+/242c8082ae66fee2667469a64c3e... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001) as https://chromium.googlesource.com/chromium/src/+/242c8082ae66fee2667469a64c3e...
Message was sent while issue was closed.
Patchset #9 (id:180001) has been deleted |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
