Print Preview: Change getPrinters to cr.sendWithPromise

Print Preview: Change getPrinters to cr.sendWithPromise

Change one more native layer message to cr.sendWithPromise and adjust
tests to use browser proxy for this message instead of triggering an
event.

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

Review-Url: https://codereview.chromium.org/2919693002
Cr-Commit-Position: refs/heads/master@{#476503}
Committed: https://chromium.googlesource.com/chromium/src/+/10c0414243dada240a7fbac8853cb31d6ced5e86

[rbpotter, 2017-06-01 01:25:08]

Description was changed from

==========
Print Preview: Change getPrinters to cr.sendWithPromise (WIP)

Change one more native layer message to cr.sendWithPromise and adjust
tests.

BUG=717296
==========

to

==========
Print Preview: Change getPrinters to cr.sendWithPromise (WIP)

Change one more native layer message to cr.sendWithPromise and adjust
tests.

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

[rbpotter, 2017-06-01 18:41:55]

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

[commit-bot: I haz the power, 2017-06-01 18:42:45]

Dry run: CQ is trying da patch.

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

[rbpotter, 2017-06-01 18:47:46]

Description was changed from

==========
Print Preview: Change getPrinters to cr.sendWithPromise (WIP)

Change one more native layer message to cr.sendWithPromise and adjust
tests.

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

to

==========
Print Preview: Change getPrinters to cr.sendWithPromise

Change one more native layer message to cr.sendWithPromise and adjust
tests to use browser proxy for this message instead of triggering an
event.

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

[rbpotter, 2017-06-01 18:47:46]

rbpotter@chromium.org changed reviewers:
+ dpapad@chromium.org

[dpapad, 2017-06-01 19:01:54]

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

https://codereview.chromium.org/2919693002/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/data/destination_store.js:1081:
this.nativeLayer_.getPrinters().then(
Isn't this equivalent to

this.nativeLayer_.getPrinters().then(this.onLocalDestinationsSet_.bind(this));

https://codereview.chromium.org/2919693002/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/data/destination_store.js:1089:
this.isLocalDestinationSearchInProgress_ = true;
Not for this CL, mostly brainstorming:

I am seeing a few similar booleans in this file

this.isLocalDestinationSearchInProgress_
this.isPrivetDestinationSearchInProgress_
this.isExtensionDestinationSearchInProgress_

and also similar methods like

startLoadLocalDestinations
startLoadPrivetDestinations
startLoadExtensionDestinations

I think these are all remnants of the previous design where the request and the
response where not directly associated to each other via a Promise. I am hoping
that we can eventually improve this design, but at this point I don't know
exactly how. In theory, every  function that spawns an async operation, should
return a Promise. For example

instead of startLoadLocalDestinations it could (maybe?) be

/** @return !Promise<...> Fires when local destinations have been loaded */
loadLocalDestinations: function(...) {...}

and similar for all other such methods in this file.

https://codereview.chromium.org/2919693002/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/data/destination_store.js:1443: * @param
{Array<print_preview.LocalDestinationInfo>} destinationInfos A
Should this be !Array<!print_preview.LocalDestinationInfo> ?

https://codereview.chromium.org/2919693002/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/print_preview/native_layer_stub.js (right):

https://codereview.chromium.org/2919693002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/print_preview/native_layer_stub.js:38: * @private
{Array<print_preview.LocalDestinationInfo>} Local destination
?Array<!

Or set initial value to an empty array, and annotate as "!Array<!..."

https://codereview.chromium.org/2919693002/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/print_preview/print_preview_tests.js (right):

https://codereview.chromium.org/2919693002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/print_preview/print_preview_tests.js:267: return
nativeLayer.whenCalled('getPrinters').then(function() {
No need for the extra nesting, it can be replaced by chaining, which is always
preferred over nesting.

return nativeLayer.whenCalled('getInitialSettings').then(function() {
  setLocalDestinations();
  return nativeLayer.whenCalled('getPrinters');
}).then(function() {
  var recentList = ...;
  ...
});

[commit-bot: I haz the power, 2017-06-01 20:18:44]

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

[commit-bot: I haz the power, 2017-06-01 20:18:45]

Dry run: This issue passed the CQ dry run.

[rbpotter, 2017-06-01 22:31:12]

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

https://codereview.chromium.org/2919693002/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/data/destination_store.js:1081:
this.nativeLayer_.getPrinters().then(
On 2017/06/01 19:01:53, dpapad wrote:
> Isn't this equivalent to
> 
> this.nativeLayer_.getPrinters().then(this.onLocalDestinationsSet_.bind(this));

Done.

https://codereview.chromium.org/2919693002/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/data/destination_store.js:1089:
this.isLocalDestinationSearchInProgress_ = true;
On 2017/06/01 19:01:53, dpapad wrote:
> Not for this CL, mostly brainstorming:
> 
> I am seeing a few similar booleans in this file
> 
> this.isLocalDestinationSearchInProgress_
> this.isPrivetDestinationSearchInProgress_
> this.isExtensionDestinationSearchInProgress_
> 
> and also similar methods like
> 
> startLoadLocalDestinations
> startLoadPrivetDestinations
> startLoadExtensionDestinations
> 
> I think these are all remnants of the previous design where the request and
the
> response where not directly associated to each other via a Promise. I am
hoping
> that we can eventually improve this design, but at this point I don't know
> exactly how. In theory, every  function that spawns an async operation, should
> return a Promise. For example
> 
> instead of startLoadLocalDestinations it could (maybe?) be
> 
> /** @return !Promise<...> Fires when local destinations have been loaded */
> loadLocalDestinations: function(...) {...}
> 
> and similar for all other such methods in this file.

Acknowledged.

https://codereview.chromium.org/2919693002/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/data/destination_store.js:1443: * @param
{Array<print_preview.LocalDestinationInfo>} destinationInfos A
On 2017/06/01 19:01:53, dpapad wrote:
> Should this be !Array<!print_preview.LocalDestinationInfo> ?

Done.

https://codereview.chromium.org/2919693002/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/print_preview/native_layer_stub.js (right):

https://codereview.chromium.org/2919693002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/print_preview/native_layer_stub.js:38: * @private
{Array<print_preview.LocalDestinationInfo>} Local destination
On 2017/06/01 19:01:53, dpapad wrote:
> ?Array<!
> 
> Or set initial value to an empty array, and annotate as "!Array<!..."

Done.

https://codereview.chromium.org/2919693002/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/print_preview/print_preview_tests.js (right):

https://codereview.chromium.org/2919693002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/print_preview/print_preview_tests.js:267: return
nativeLayer.whenCalled('getPrinters').then(function() {
On 2017/06/01 19:01:53, dpapad wrote:
> No need for the extra nesting, it can be replaced by chaining, which is always
> preferred over nesting.
> 
> return nativeLayer.whenCalled('getInitialSettings').then(function() {
>   setLocalDestinations();
>   return nativeLayer.whenCalled('getPrinters');
> }).then(function() {
>   var recentList = ...;
>   ...
> });

Mostly replaced instances of this sequence with a
helper function, since it is repeated in almost all
of the tests. Thanks for the recommendation though;
will use chaining in future instead of nesting.

[rbpotter, 2017-06-01 22:31:21]

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

[commit-bot: I haz the power, 2017-06-01 22:32:12]

Dry run: CQ is trying da patch.

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

[dpapad, 2017-06-01 23:02:33]

https://codereview.chromium.org/2919693002/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/print_preview/print_preview_tests.js (right):

https://codereview.chromium.org/2919693002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/print_preview/print_preview_tests.js:267: return
nativeLayer.whenCalled('getPrinters').then(function() {
On 2017/06/01 at 22:31:12, rbpotter wrote:
> On 2017/06/01 19:01:53, dpapad wrote:
> > No need for the extra nesting, it can be replaced by chaining, which is
always
> > preferred over nesting.
> > 
> > return nativeLayer.whenCalled('getInitialSettings').then(function() {
> >   setLocalDestinations();
> >   return nativeLayer.whenCalled('getPrinters');
> > }).then(function() {
> >   var recentList = ...;
> >   ...
> > });
> 
> Mostly replaced instances of this sequence with a
> helper function, since it is repeated in almost all
> of the tests. Thanks for the recommendation though;
> will use chaining in future instead of nesting.

We should do this for this CL. Nesting and chaining have slightly different
behavior when it comes to errors. The way the code is now an error in the inner
callback will not be surfaced correctly to the outer. You can verify this with a
minimal example in the dev console.

Try the following:
function foo() {
  return Promise.resolve('foo').then(function() {
    Promise.reject('error');
  });
}
foo().then(function() {console.log('resolved success')}, function(e) {
console.log('resolved with error')})

It erroneously thinks that the outer Promise succeeded.

Now try the following

function foo() {
  return Promise.resolve('foo').then(function() {
    return Promise.reject('error');
  });
}
foo().then(function() {console.log('resolved success')}, function(e) {
console.log('resolved with error')})

It correctly detects that foo() failed. In our case, Mocha relies on the Promise
we return from the test to detect whether the test succeeded or failed. If we
don't get that Promise to properly reject for any inner error, we are breaking
Mocha's contract.

[dpapad, 2017-06-01 23:07:28]

On 2017/06/01 at 23:02:33, dpapad wrote:
>
https://codereview.chromium.org/2919693002/diff/40001/chrome/test/data/webui/...
> File chrome/test/data/webui/print_preview/print_preview_tests.js (right):
> 
>
https://codereview.chromium.org/2919693002/diff/40001/chrome/test/data/webui/...
> chrome/test/data/webui/print_preview/print_preview_tests.js:267: return
nativeLayer.whenCalled('getPrinters').then(function() {
> On 2017/06/01 at 22:31:12, rbpotter wrote:
> > On 2017/06/01 19:01:53, dpapad wrote:
> > > No need for the extra nesting, it can be replaced by chaining, which is
always
> > > preferred over nesting.
> > > 
> > > return nativeLayer.whenCalled('getInitialSettings').then(function() {
> > >   setLocalDestinations();
> > >   return nativeLayer.whenCalled('getPrinters');
> > > }).then(function() {
> > >   var recentList = ...;
> > >   ...
> > > });
> > 
> > Mostly replaced instances of this sequence with a
> > helper function, since it is repeated in almost all
> > of the tests. Thanks for the recommendation though;
> > will use chaining in future instead of nesting.
> 
> We should do this for this CL. Nesting and chaining have slightly different
behavior when it comes to errors. The way the code is now an error in the inner
callback will not be surfaced correctly to the outer. You can verify this with a
minimal example in the dev console.
> 
> Try the following:
> function foo() {
>   return Promise.resolve('foo').then(function() {
>     Promise.reject('error');
>   });
> }
> foo().then(function() {console.log('resolved success')}, function(e) {
console.log('resolved with error')})
> 
> It erroneously thinks that the outer Promise succeeded.
> 
> Now try the following
> 
> function foo() {
>   return Promise.resolve('foo').then(function() {
>     return Promise.reject('error');
>   });
> }
> foo().then(function() {console.log('resolved success')}, function(e) {
console.log('resolved with error')})
> 
> It correctly detects that foo() failed. In our case, Mocha relies on the
Promise we return from the test to detect whether the test succeeded or failed.
If we don't get that Promise to properly reject for any inner error, we are
breaking Mocha's contract.

LGTM. Looking at the code, it seems you already addressed what I am talking
about. I did not realize that, since there was not "Done" in the response.

[commit-bot: I haz the power, 2017-06-02 00:32:40]

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

[commit-bot: I haz the power, 2017-06-02 00:32:41]

Dry run: This issue passed the CQ dry run.

[rbpotter, 2017-06-02 00:33:36]

The CQ bit was checked by rbpotter@chromium.org

[commit-bot: I haz the power, 2017-06-02 00:34:21]

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-06-02 00:39:05]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496363616076500,
"parent_rev": "2bfd0fd3e968ff844ca121035ff9fe352375f62d", "commit_rev":
"10c0414243dada240a7fbac8853cb31d6ced5e86"}

[commit-bot: I haz the power, 2017-06-02 00:39:15]

Description was changed from

==========
Print Preview: Change getPrinters to cr.sendWithPromise

Change one more native layer message to cr.sendWithPromise and adjust
tests to use browser proxy for this message instead of triggering an
event.

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

to

==========
Print Preview: Change getPrinters to cr.sendWithPromise

Change one more native layer message to cr.sendWithPromise and adjust
tests to use browser proxy for this message instead of triggering an
event.

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

Review-Url: https://codereview.chromium.org/2919693002
Cr-Commit-Position: refs/heads/master@{#476503}
Committed:
https://chromium.googlesource.com/chromium/src/+/10c0414243dada240a7fbac8853c...
==========

[commit-bot: I haz the power, 2017-06-02 00:39:16]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/10c0414243dada240a7fbac8853c...