Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2918243002:
Change getExtensionPrinters to cr.sendWithPromise (Closed)
DescriptionChange getExtensionPrinters to cr.sendWithPromise
Use cr.sendWithPromise for getExtensionPrinters. Resolve the promise
only when all extension printers have been listed. Also add a Web UI
event to fire from C++ as individual lists of printers are ready.
BUG=717296
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2918243002
Cr-Commit-Position: refs/heads/master@{#477826}
Committed: https://chromium.googlesource.com/chromium/src/+/c0f846bdbfa4e685f05c48443923db9dfceaa35d
Patch Set 1 #Patch Set 2 : Fix #
Total comments: 4
Patch Set 3 : Fix tests, address comments #Patch Set 4 : Move add listener into init #
Total comments: 6
Patch Set 5 : Address comments #
Total comments: 2
Patch Set 6 : Fix comment #
Dependent Patchsets: Messages
Total messages: 34 (26 generated)
Description was changed from ========== Change getExtensionPrinters to cr.sendWithPromise Use cr.sendWithPromise for getExtensionPrinters. Resolve the promise only when all extension printers have been listed. Also add a Web UI event to fire from C++ as individual lists of printers are ready. BUG=717296 ========== to ========== Change getExtensionPrinters to cr.sendWithPromise Use cr.sendWithPromise for getExtensionPrinters. Resolve the promise only when all extension printers have been listed. Also add a Web UI event to fire from C++ as individual lists of printers are ready. 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 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_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2918243002/diff/20001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2918243002/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1628: * @param {Array<!{extensionId: string, !Array It does not seem that the function expects a null input, otherwise the call to map() would throw an error. https://codereview.chromium.org/2918243002/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1638: this.insertDestinations_(printers.map(function(printer) { Nit: I think this is equivalent to the shorter this.insertDestinations_( printers.map(print_preview.ExtensionDestinationParser.parse));
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/2918243002/diff/20001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2918243002/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1628: * @param {Array<!{extensionId: string, On 2017/06/03 01:00:00, dpapad wrote: > !Array > > It does not seem that the function expects a null input, otherwise the call to > map() would throw an error. Done. https://codereview.chromium.org/2918243002/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1638: this.insertDestinations_(printers.map(function(printer) { On 2017/06/03 01:00:00, dpapad wrote: > Nit: I think this is equivalent to the shorter > > this.insertDestinations_( > printers.map(print_preview.ExtensionDestinationParser.parse)); 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 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...
dpapad@chromium.org changed reviewers: + dpapad@chromium.org
https://codereview.chromium.org/2918243002/diff/60001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2918243002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1643: * @param {boolean} done Whether printers are done Nit: Maybe update this comment to Whether searching for printers has completed. https://codereview.chromium.org/2918243002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1647: if (done && this.isExtensionDestinationSearchInProgress_) { This is basically same question as in the C++ handler comment. Can 'done' ever be false? https://codereview.chromium.org/2918243002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2918243002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1750: ResolveJavascriptCallback(base::Value(callback_id), base::Value(true)); Is the boolean passed here needed? It seems that we only ever pass "true", so seems unnecessary.
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/2918243002/diff/60001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2918243002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1643: * @param {boolean} done Whether printers are done On 2017/06/07 03:20:11, dpapad wrote: > Nit: Maybe update this comment to > Whether searching for printers has completed. Removed this comment since parameter was removed per comment below. https://codereview.chromium.org/2918243002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1647: if (done && this.isExtensionDestinationSearchInProgress_) { On 2017/06/07 03:20:11, dpapad wrote: > This is basically same question as in the C++ handler comment. Can 'done' ever > be false? Done. https://codereview.chromium.org/2918243002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2918243002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1750: ResolveJavascriptCallback(base::Value(callback_id), base::Value(true)); On 2017/06/07 03:20:11, dpapad wrote: > Is the boolean passed here needed? It seems that we only ever pass "true", so > seems unnecessary. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with nit. https://codereview.chromium.org/2918243002/diff/80001/chrome/browser/resource... File chrome/browser/resources/print_preview/native_layer.js (right): https://codereview.chromium.org/2918243002/diff/80001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:262: * Requests that extension system dispatches an event requesting the list of This comment sounds too indirect (requesting a request to be sent). Can we simplify as follows? Request a list of extension printers. Printers are reported as they are found by a series of 'extension-printers-added' events.
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
https://codereview.chromium.org/2918243002/diff/80001/chrome/browser/resource... File chrome/browser/resources/print_preview/native_layer.js (right): https://codereview.chromium.org/2918243002/diff/80001/chrome/browser/resource... chrome/browser/resources/print_preview/native_layer.js:262: * Requests that extension system dispatches an event requesting the list of On 2017/06/07 18:21:39, dpapad wrote: > This comment sounds too indirect (requesting a request to be sent). Can we > simplify as follows? > > Request a list of extension printers. Printers are reported as they are found by > a series of 'extension-printers-added' events. Done.
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_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) 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
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/2918243002/#ps100001 (title: "Fix comment")
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": 100001, "attempt_start_ts": 1496881114362450,
"parent_rev": "f81c5f0e622cd1eb2bac723744a5a0f5c6b7837d", "commit_rev":
"c0f846bdbfa4e685f05c48443923db9dfceaa35d"}
Message was sent while issue was closed.
Description was changed from ========== Change getExtensionPrinters to cr.sendWithPromise Use cr.sendWithPromise for getExtensionPrinters. Resolve the promise only when all extension printers have been listed. Also add a Web UI event to fire from C++ as individual lists of printers are ready. BUG=717296 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Change getExtensionPrinters to cr.sendWithPromise Use cr.sendWithPromise for getExtensionPrinters. Resolve the promise only when all extension printers have been listed. Also add a Web UI event to fire from C++ as individual lists of printers are ready. BUG=717296 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2918243002 Cr-Commit-Position: refs/heads/master@{#477826} Committed: https://chromium.googlesource.com/chromium/src/+/c0f846bdbfa4e685f05c48443923... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/c0f846bdbfa4e685f05c48443923... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
