Perform printer setup on Chrome OS before selecting printer.

Perform printer setup on Chrome OS before selecting printer.

In Chrome OS, printers might need to be configured before they can be
used.  For local printers with origin CROS, perform configuration before
continuing to print preview.  The chosen approach is similar to that of
the extension printers.

BUG=670819
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2606043004
Cr-Commit-Position: refs/heads/master@{#443735}
Committed: https://chromium.googlesource.com/chromium/src/+/f14c7a220a083a7fd6870a10f35fdcf9a8c0b01b

[skau, 2016-12-29 23:12:47]

Description was changed from

==========
Perform printer setup on Chrome OS before selecting printer.

In Chrome OS, printers might need to be configured before they can be
used.  For local printers with origin CROS, perform configuration before
continuing to print preview.  The chosen approach is similar to that of
the extension printers.

BUG=670819
==========

to

==========
Perform printer setup on Chrome OS before selecting printer.

In Chrome OS, printers might need to be configured before they can be
used.  For local printers with origin CROS, perform configuration before
continuing to print preview.  The chosen approach is similar to that of
the extension printers.

BUG=670819
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[skau, 2016-12-29 23:17:29]

skau@chromium.org changed reviewers:
+ dpapad@chromium.org, vitalybuka@chromium.org, xdai@chromium.org

[skau, 2016-12-29 23:17:30]

vitalybuka@: chrome/browser/resources/print_preview/*
xdai@: Everything
dpapad@: webui owner

justincarlson@: fyi

[skau, 2016-12-29 23:19:34]

The CQ bit was checked by skau@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-29 23:19:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Vitaly Buka (NO REVIEWS), 2016-12-29 23:20:09]

chrome/browser/resources/print_preview/ LGTM

[xdai1, 2016-12-30 00:38:13]

Generally looks good.

https://codereview.chromium.org/2606043004/diff/1/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/data/destination_store.js (right):

https://codereview.chromium.org/2606043004/diff/1/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/data/destination_store.js:1287:
DestinationStore.EventType.PRINTER_CONFIGURED);
Seems there is no DestinationStore.EventType.PRINTER_CONFIGURED type?

https://codereview.chromium.org/2606043004/diff/1/chrome/browser/resources/pr...
File
chrome/browser/resources/print_preview/search/cros_destination_resolver.html
(right):

https://codereview.chromium.org/2606043004/diff/1/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/search/cros_destination_resolver.html:5:
Setting up your printer.
i18n?

https://codereview.chromium.org/2606043004/diff/1/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/search/destination_search.js
(right):

https://codereview.chromium.org/2606043004/diff/1/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/search/destination_search.js:606: })
indent off

https://codereview.chromium.org/2606043004/diff/1/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/search/destination_search.js:610:
}.bind(this));
indent off

[commit-bot: I haz the power, 2016-12-30 00:53:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-30 00:53:14]

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_...)

[skau, 2017-01-04 18:45:06]

The CQ bit was checked by skau@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 18:45:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skau, 2017-01-04 18:57:06]

Thanks for the review.  PTAL.

https://codereview.chromium.org/2606043004/diff/1/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/data/destination_store.js (right):

https://codereview.chromium.org/2606043004/diff/1/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/data/destination_store.js:1287:
DestinationStore.EventType.PRINTER_CONFIGURED);
On 2016/12/30 00:38:13, xdai1 wrote:
> Seems there is no DestinationStore.EventType.PRINTER_CONFIGURED type?

Looks like it was left out of the CL somehow.  It's there now.

https://codereview.chromium.org/2606043004/diff/1/chrome/browser/resources/pr...
File
chrome/browser/resources/print_preview/search/cros_destination_resolver.html
(right):

https://codereview.chromium.org/2606043004/diff/1/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/search/cros_destination_resolver.html:5:
Setting up your printer.
On 2016/12/30 00:38:13, xdai1 wrote:
> i18n?

Done.

https://codereview.chromium.org/2606043004/diff/1/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/search/destination_search.js
(right):

https://codereview.chromium.org/2606043004/diff/1/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/search/destination_search.js:606: })
On 2016/12/30 00:38:13, xdai1 wrote:
> indent off

Done.

https://codereview.chromium.org/2606043004/diff/1/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/search/destination_search.js:610:
}.bind(this));
On 2016/12/30 00:38:13, xdai1 wrote:
> indent off

Done.

[commit-bot: I haz the power, 2017-01-04 19:50:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-04 19:50:42]

Dry run: This issue passed the CQ dry run.

[xdai1, 2017-01-05 00:18:56]

lgtm

https://codereview.chromium.org/2606043004/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/print_preview/search/cros_destination_resolver.html
(right):

https://codereview.chromium.org/2606043004/diff/60001/chrome/browser/resource...
chrome/browser/resources/print_preview/search/cros_destination_resolver.html:5:
<div class="resolving-message" i18n-content="resolveCrosPrinterMessage"></div>
nit: put </div> on a separate line

[skau, 2017-01-05 20:25:39]

https://codereview.chromium.org/2606043004/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/print_preview/search/cros_destination_resolver.html
(right):

https://codereview.chromium.org/2606043004/diff/60001/chrome/browser/resource...
chrome/browser/resources/print_preview/search/cros_destination_resolver.html:5:
<div class="resolving-message" i18n-content="resolveCrosPrinterMessage"></div>
On 2017/01/05 00:18:56, xdai1 wrote:
> nit: put </div> on a separate line

Done.

[skau, 2017-01-11 18:57:32]

dpapad@: Could you take a look?  I spoke with rbpotter@ and she has no
objections.

[dpapad, 2017-01-11 20:36:31]

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
File chrome/browser/resources/print_preview/data/destination_store.js (right):

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
chrome/browser/resources/print_preview/data/destination_store.js:1108:
resolveCrosDestination: function(destination) {
@param annotation missing.

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
chrome/browser/resources/print_preview/data/destination_store.js:1502:
this.handleCrosDestinationResolved_.bind(this));
This breaks naming convention within this file.
foo.bar.EVENT_NAME -> onEventName_

should be onPrinterSetup_

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
File chrome/browser/resources/print_preview/native_layer.js (right):

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
chrome/browser/resources/print_preview/native_layer.js:241:
chrome.send('setupPrinter', [printerId]);
Nit (optional): Instead of using chrome.send, exposing a global JS function,
calling that global JS function by name from C++, can you use the newer
cr.sendWithPromise() mechanism instead?

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/print_preview/search/cros_destination_resolver.html
(right):

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
chrome/browser/resources/print_preview/search/cros_destination_resolver.html:5:
<div class="resolving-message" i18n-content="resolveCrosPrinterMessage">
Use i18n{} instead of i18n-content, see other examples in print_preview,
https://cs.chromium.org/search/?q=%22i18n%7B%22+file:%5Esrc/chrome/browser/re...

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
File chrome/browser/resources/print_preview/search/cros_destination_resolver.js
(right):

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
chrome/browser/resources/print_preview/search/cros_destination_resolver.js:36:
/** @private {ResolverState} */
!ResolverState

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
chrome/browser/resources/print_preview/search/cros_destination_resolver.js:57: 
Unnecessary blank line.

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
chrome/browser/resources/print_preview/search/cros_destination_resolver.js:86:
assert(this.promiseResolver_, "Promise resolver collected while waiting");
What do you mean by "collected"? Garbage collected? This seems impossible since
this class is holding a reference to this.promiseResolver_. Probably less
confusing to simply restate to "Promise resolver not found"

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
chrome/browser/resources/print_preview/search/cros_destination_resolver.js:119:
// TOOD(crbug.com/677567): Replace with meaningful UI.
s/TOOD/TODO

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
chrome/browser/resources/print_preview/search/cros_destination_resolver.js:127:
* fulfilled when the destination resolving is finished.
Indentation off

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
File chrome/browser/resources/print_preview/search/destination_search.js
(right):

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
chrome/browser/resources/print_preview/search/destination_search.js:594: .then(
From JS styleguide "Always put the operator on the preceding line." and "This
includes the dot operator.",
https://google.github.io/styleguide/javascriptguide.xml.

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
chrome/browser/resources/print_preview/search/destination_search.js:606:
console.log('Failed to setup destination: ' + destination.id);
console.error?

[skau, 2017-01-12 00:32:24]

The CQ bit was checked by skau@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 00:34:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skau, 2017-01-12 00:36:16]

Thanks for the review.  All comments have been addressed.

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
File chrome/browser/resources/print_preview/data/destination_store.js (right):

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
chrome/browser/resources/print_preview/data/destination_store.js:1108:
resolveCrosDestination: function(destination) {
On 2017/01/11 20:36:30, dpapad wrote:
> @param annotation missing.

Done.

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
chrome/browser/resources/print_preview/data/destination_store.js:1502:
this.handleCrosDestinationResolved_.bind(this));
On 2017/01/11 20:36:30, dpapad wrote:
> This breaks naming convention within this file.
> foo.bar.EVENT_NAME -> onEventName_
> 
> should be onPrinterSetup_

Done.

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
File chrome/browser/resources/print_preview/native_layer.js (right):

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
chrome/browser/resources/print_preview/native_layer.js:241:
chrome.send('setupPrinter', [printerId]);
On 2017/01/11 20:36:30, dpapad wrote:
> Nit (optional): Instead of using chrome.send, exposing a global JS function,
> calling that global JS function by name from C++, can you use the newer
> cr.sendWithPromise() mechanism instead?

Done.

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/print_preview/search/cros_destination_resolver.html
(right):

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
chrome/browser/resources/print_preview/search/cros_destination_resolver.html:5:
<div class="resolving-message" i18n-content="resolveCrosPrinterMessage">
On 2017/01/11 20:36:30, dpapad wrote:
> Use i18n{} instead of i18n-content, see other examples in print_preview,
>
https://cs.chromium.org/search/?q=%22i18n%7B%22+file:%5Esrc/chrome/browser/re...

Thanks.  That's nicer.

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
File chrome/browser/resources/print_preview/search/cros_destination_resolver.js
(right):

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
chrome/browser/resources/print_preview/search/cros_destination_resolver.js:36:
/** @private {ResolverState} */
On 2017/01/11 20:36:30, dpapad wrote:
> !ResolverState

Done.

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
chrome/browser/resources/print_preview/search/cros_destination_resolver.js:57: 
On 2017/01/11 20:36:30, dpapad wrote:
> Unnecessary blank line.

Done.

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
chrome/browser/resources/print_preview/search/cros_destination_resolver.js:86:
assert(this.promiseResolver_, "Promise resolver collected while waiting");
On 2017/01/11 20:36:30, dpapad wrote:
> What do you mean by "collected"? Garbage collected? This seems impossible
since
> this class is holding a reference to this.promiseResolver_. Probably less
> confusing to simply restate to "Promise resolver not found"

The promise resolver is set to null after it has been fulfilled.  I've updated
the message per your suggestion.

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
chrome/browser/resources/print_preview/search/cros_destination_resolver.js:119:
// TOOD(crbug.com/677567): Replace with meaningful UI.
On 2017/01/11 20:36:30, dpapad wrote:
> s/TOOD/TODO

Done.

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
chrome/browser/resources/print_preview/search/cros_destination_resolver.js:127:
* fulfilled when the destination resolving is finished.
On 2017/01/11 20:36:30, dpapad wrote:
> Indentation off

Is there a linter that would catch this?

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
File chrome/browser/resources/print_preview/search/destination_search.js
(right):

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
chrome/browser/resources/print_preview/search/destination_search.js:594: .then(
On 2017/01/11 20:36:31, dpapad wrote:
> From JS styleguide "Always put the operator on the preceding line." and "This
> includes the dot operator.",
> https://google.github.io/styleguide/javascriptguide.xml.

Weird.  I never noticed that.  I've fixed the similar block below for
consistency.

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
chrome/browser/resources/print_preview/search/destination_search.js:606:
console.log('Failed to setup destination: ' + destination.id);
On 2017/01/11 20:36:31, dpapad wrote:
> console.error?

I've made it a warning.  We'll want to know if it failed since we don't expect
it to fail. However, the dialog will still be usable in the event that the call
does fail so it's not an error.  More importantly, if it's an error, the unit
test won't pass.

[dpapad, 2017-01-12 01:18:10]

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
File chrome/browser/resources/print_preview/search/cros_destination_resolver.js
(right):

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
chrome/browser/resources/print_preview/search/cros_destination_resolver.js:127:
* fulfilled when the destination resolving is finished.
On 2017/01/12 at 00:36:16, skau wrote:
> On 2017/01/11 20:36:30, dpapad wrote:
> > Indentation off
> 
> Is there a linter that would catch this?

Not to my knowledge. And to be fair, this is an ambitious goal (having a well
working linter for JSDoc comments, which are not "standard" by any means). We
are not even at the point where we have a well working linter for JS itself (in
Chromium), but we are making progress on this front.

Having said that, you might want to try running clang-format and see if that
would fix it automatically.

https://codereview.chromium.org/2606043004/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/data/destination_store.js (right):

https://codereview.chromium.org/2606043004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/print_preview/data/destination_store.js:1124:
this.nativeLayer_.setupPrinter(destination.id).
There is no need to call both then and catch. then() accepts a 2nd parameter.
Also I think this would be simpler if you simply inline the two handling
functions here, as follows.

this.nativeLayer_.setupPrinter(destination.id).then(
  function(response) {
    var event = ...;
    ...
    this.dispatchEvent(event);
  }.bind(this),
  function() { // You don't even need the parameter anymore, just use
destination.id
    ...
  }.bind(this));

https://codereview.chromium.org/2606043004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/print_preview/data/destination_store.js:1323: var event
= new Event(
Technically, we should not be augmenting a native Event with extra properties. A
much simpler (and robust) way is to use a class specifically made for custom
events, called CustomEvent
(https://developer.mozilla.org/en-US/docs/Web/API/CustomEvent/CustomEvent).

For example this could look as simple as follows

var event = new CustomEvent(
  DestinationStore.EventType.PRINTER_CONFIGURED,
  {detail: response});
this.dispatchEvent(event);

Then consumers of the event can simply refer to the "payload" of the event as
"event.detail".

I am not suggesting to do this now. We should probably cleanup all "new Event"
occurrences in print_preview/ at some point.

https://codereview.chromium.org/2606043004/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/native_layer.js (right):

https://codereview.chromium.org/2606043004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/print_preview/native_layer.js:240: return
cr.sendWithPromise('setupPrinter', [printerId]);
Why do you need to wrap |printerId| in an array? Should not be necessary.

https://codereview.chromium.org/2606043004/diff/120001/chrome/test/data/webui...
File chrome/test/data/webui/print_preview.js (right):

https://codereview.chromium.org/2606043004/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/print_preview.js:392:
this.initialSettings_.serializedAppStateStr_ =
Nit (optional): Instead of painfully constructing a JSON string manually, how
about the following?

var settings = {
  version: 2,
  recentDestinations: [1, 2, 3].map(function(i) {
    return {
      id: 'ID' + i, origin: origin, account: '', capabilities: 0,
      name: '', extensionId: '', extensionName: ''
    };
  }),
};

this.initialSettings_.serializedAppStateStr_ = JSON.stringify(settings);

[commit-bot: I haz the power, 2017-01-12 02:03:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 02:03:10]

Dry run: This issue passed the CQ dry run.

[skau, 2017-01-12 02:15:55]

The CQ bit was checked by skau@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 02:16:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skau, 2017-01-12 02:17:17]

Incorporated the second round of comments.  PTAL.  Thanks for the tips.

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
File chrome/browser/resources/print_preview/search/cros_destination_resolver.js
(right):

https://codereview.chromium.org/2606043004/diff/80001/chrome/browser/resource...
chrome/browser/resources/print_preview/search/cros_destination_resolver.js:127:
* fulfilled when the destination resolving is finished.
On 2017/01/12 01:18:10, dpapad wrote:
> On 2017/01/12 at 00:36:16, skau wrote:
> > On 2017/01/11 20:36:30, dpapad wrote:
> > > Indentation off
> > 
> > Is there a linter that would catch this?
> 
> Not to my knowledge. And to be fair, this is an ambitious goal (having a well
> working linter for JSDoc comments, which are not "standard" by any means). We
> are not even at the point where we have a well working linter for JS itself
(in
> Chromium), but we are making progress on this front.
> 
> Having said that, you might want to try running clang-format and see if that
> would fix it automatically.

Good to know.  I just tried clang-format and it definitely ignores JSDoc.

https://codereview.chromium.org/2606043004/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/data/destination_store.js (right):

https://codereview.chromium.org/2606043004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/print_preview/data/destination_store.js:1124:
this.nativeLayer_.setupPrinter(destination.id).
On 2017/01/12 01:18:10, dpapad wrote:
> There is no need to call both then and catch. then() accepts a 2nd parameter.
> Also I think this would be simpler if you simply inline the two handling
> functions here, as follows.
> 
> this.nativeLayer_.setupPrinter(destination.id).then(
>   function(response) {
>     var event = ...;
>     ...
>     this.dispatchEvent(event);
>   }.bind(this),
>   function() { // You don't even need the parameter anymore, just use
> destination.id
>     ...
>   }.bind(this));

Done.

https://codereview.chromium.org/2606043004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/print_preview/data/destination_store.js:1323: var event
= new Event(
On 2017/01/12 01:18:10, dpapad wrote:
> Technically, we should not be augmenting a native Event with extra properties.
A
> much simpler (and robust) way is to use a class specifically made for custom
> events, called CustomEvent
> (https://developer.mozilla.org/en-US/docs/Web/API/CustomEvent/CustomEvent).
> 
> For example this could look as simple as follows
> 
> var event = new CustomEvent(
>   DestinationStore.EventType.PRINTER_CONFIGURED,
>   {detail: response});
> this.dispatchEvent(event);
> 
> Then consumers of the event can simply refer to the "payload" of the event as
> "event.detail".
> 
> I am not suggesting to do this now. We should probably cleanup all "new Event"
> occurrences in print_preview/ at some point.

It seems to lead to much cleaner code.  I'll use it for this.

https://codereview.chromium.org/2606043004/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/native_layer.js (right):

https://codereview.chromium.org/2606043004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/print_preview/native_layer.js:240: return
cr.sendWithPromise('setupPrinter', [printerId]);
On 2017/01/12 01:18:10, dpapad wrote:
> Why do you need to wrap |printerId| in an array? Should not be necessary.

I've unwrapped it.  Send needed it to be wrapped.

https://codereview.chromium.org/2606043004/diff/120001/chrome/test/data/webui...
File chrome/test/data/webui/print_preview.js (right):

https://codereview.chromium.org/2606043004/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/print_preview.js:392:
this.initialSettings_.serializedAppStateStr_ =
On 2017/01/12 01:18:10, dpapad wrote:
> Nit (optional): Instead of painfully constructing a JSON string manually, how
> about the following?
> 
> var settings = {
>   version: 2,
>   recentDestinations: [1, 2, 3].map(function(i) {
>     return {
>       id: 'ID' + i, origin: origin, account: '', capabilities: 0,
>       name: '', extensionId: '', extensionName: ''
>     };
>   }),
> };
> 
> this.initialSettings_.serializedAppStateStr_ = JSON.stringify(settings);

Done.

[commit-bot: I haz the power, 2017-01-12 02:29:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 02:29:07]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[dpapad, 2017-01-12 19:32:50]

https://codereview.chromium.org/2606043004/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/data/destination_store.js (right):

https://codereview.chromium.org/2606043004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/print_preview/data/destination_store.js:1125: then(
Nit: You would avoid a lot of indentation if you put "then(" in the previous
line.

this.nativeLayer_.setupPrinter(destination.id).then(
    function(response) {
      ...
    }.bind(this),
    function() {
      ...
    }.bind(this));

https://codereview.chromium.org/2606043004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/print_preview/data/destination_store.js:1133: detail: {
Why not the following?

detail: response

Instead of copying fields one by one over?

https://codereview.chromium.org/2606043004/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/native_layer.js (right):

https://codereview.chromium.org/2606043004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/print_preview/native_layer.js:237: * @return
{!Promise<Object>}
Can you document the returned object with a typedef, instead of throwing in the
towel (Object)? For example

/**
 *  @typedef {{foo: string, bar: boolean, num: number}}
 */
var MyResponseType;

and then

@return {!Promise<!MyResponseType>}

[skau, 2017-01-12 22:39:38]

The CQ bit was checked by skau@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 22:40:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skau, 2017-01-12 23:24:04]

Comments addressed PTAL

https://codereview.chromium.org/2606043004/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/data/destination_store.js (right):

https://codereview.chromium.org/2606043004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/print_preview/data/destination_store.js:1125: then(
On 2017/01/12 19:32:50, dpapad wrote:
> Nit: You would avoid a lot of indentation if you put "then(" in the previous
> line.
> 
> this.nativeLayer_.setupPrinter(destination.id).then(
>     function(response) {
>       ...
>     }.bind(this),
>     function() {
>       ...
>     }.bind(this));

Done.

https://codereview.chromium.org/2606043004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/print_preview/data/destination_store.js:1133: detail: {
On 2017/01/12 19:32:50, dpapad wrote:
> Why not the following?
> 
> detail: response
> 
> Instead of copying fields one by one over?

No particular reason.

https://codereview.chromium.org/2606043004/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/native_layer.js (right):

https://codereview.chromium.org/2606043004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/print_preview/native_layer.js:237: * @return
{!Promise<Object>}
On 2017/01/12 19:32:50, dpapad wrote:
> Can you document the returned object with a typedef, instead of throwing in
the
> towel (Object)? For example
> 
> /**
>  *  @typedef {{foo: string, bar: boolean, num: number}}
>  */
> var MyResponseType;
> 
> and then
> 
> @return {!Promise<!MyResponseType>}

Done.

[skau, 2017-01-13 22:45:26]

dpapad@: Ping?

[dpapad, 2017-01-13 22:50:50]

LGTM with nits.

https://codereview.chromium.org/2606043004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/search/cros_destination_resolver.js
(right):

https://codereview.chromium.org/2606043004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/print_preview/search/cros_destination_resolver.js:81: *
@param {Event} event
s/Event/CustomEvent

https://codereview.chromium.org/2606043004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/print_preview/search/cros_destination_resolver.js:90:
// detail contains printerId and capabilities.
Nit: |detail| contains ....

https://codereview.chromium.org/2606043004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/print_preview/search/cros_destination_resolver.js:103:
* @param {ResolverState} state
!ResolverState

[skau, 2017-01-13 22:57:43]

Thanks!

https://codereview.chromium.org/2606043004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/print_preview/search/cros_destination_resolver.js
(right):

https://codereview.chromium.org/2606043004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/print_preview/search/cros_destination_resolver.js:81: *
@param {Event} event
On 2017/01/13 22:50:49, dpapad wrote:
> s/Event/CustomEvent

Done.

https://codereview.chromium.org/2606043004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/print_preview/search/cros_destination_resolver.js:90:
// detail contains printerId and capabilities.
On 2017/01/13 22:50:50, dpapad wrote:
> Nit: |detail| contains ....

Done.

https://codereview.chromium.org/2606043004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/print_preview/search/cros_destination_resolver.js:103:
* @param {ResolverState} state
On 2017/01/13 22:50:50, dpapad wrote:
> !ResolverState

Done.

[skau, 2017-01-13 22:58:05]

The CQ bit was checked by skau@chromium.org

[skau, 2017-01-13 22:58:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
vitalybuka@chromium.org, xdai@chromium.org, dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2606043004/#ps200001
(title: "fix nits")

[commit-bot: I haz the power, 2017-01-13 22:58:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-14 00:07:03]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1484348285866270,
"parent_rev": "c711312d40ea1f9294385ae6fafdb2ceb20737c4", "commit_rev":
"f14c7a220a083a7fd6870a10f35fdcf9a8c0b01b"}

[commit-bot: I haz the power, 2017-01-14 00:07:37]

Description was changed from

==========
Perform printer setup on Chrome OS before selecting printer.

In Chrome OS, printers might need to be configured before they can be
used.  For local printers with origin CROS, perform configuration before
continuing to print preview.  The chosen approach is similar to that of
the extension printers.

BUG=670819
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Perform printer setup on Chrome OS before selecting printer.

In Chrome OS, printers might need to be configured before they can be
used.  For local printers with origin CROS, perform configuration before
continuing to print preview.  The chosen approach is similar to that of
the extension printers.

BUG=670819
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2606043004
Cr-Commit-Position: refs/heads/master@{#443735}
Committed:
https://chromium.googlesource.com/chromium/src/+/f14c7a220a083a7fd6870a10f35f...
==========

[commit-bot: I haz the power, 2017-01-14 00:07:38]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/f14c7a220a083a7fd6870a10f35f...