Re-use PromiseResolver helper in print preview.

Re-use PromiseResolver helper in print preview.

TBR=tbarzic@chromium.org

Committed: https://crrev.com/0db8c1a80c76832140adfc7c2171fbd6f9067713
Cr-Commit-Position: refs/heads/master@{#379446}

[Dan Beam, 2016-03-03 21:18:23]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2016-03-03 21:18:24]

https://codereview.chromium.org/1755423003/diff/1/chrome/browser/resources/pr...
File
chrome/browser/resources/print_preview/search/provisional_destination_resolver.js
(left):

https://codereview.chromium.org/1755423003/diff/1/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/search/provisional_destination_resolver.js:26:
function PromiseResolver(promise, resolve, reject) {
nit: perhaps we should templatize PromiseResolver

[dpapad, 2016-03-04 18:55:43]

Description was changed from

==========
Re-use PromiseResolver, WIP.

BUG=
==========

to

==========
Re-use PromiseResolver helper from print preview.
==========

[dpapad, 2016-03-04 18:58:42]

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

[dpapad, 2016-03-04 18:58:43]

@tbarzic: You had added this code at https://codereview.chromium.org/1144983002.
I don't know how to trigger this particular codepath, so could you ensure this
codepath still works or provide some instructions for manual testing?

https://codereview.chromium.org/1755423003/diff/1/chrome/browser/resources/pr...
File
chrome/browser/resources/print_preview/search/provisional_destination_resolver.js
(left):

https://codereview.chromium.org/1755423003/diff/1/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/search/provisional_destination_resolver.js:26:
function PromiseResolver(promise, resolve, reject) {
On 2016/03/03 at 21:18:24, Dan Beam wrote:
> nit: perhaps we should templatize PromiseResolver

Done. I templatized it in a bit simpler way compared to
https://github.com/google/closure-library/blob/master/closure/goog/promise/re....
I was not sure why Closure's Resolver is annotated in a more complicated way. We
can update the type annotations to match those, if needed in the future.

[dpapad, 2016-03-04 18:59:04]

Description was changed from

==========
Re-use PromiseResolver helper from print preview.
==========

to

==========
Re-use PromiseResolver helper in print preview.
==========

[Dan Beam, 2016-03-04 19:29:26]

lgtm

https://codereview.chromium.org/1755423003/diff/20001/ui/webui/resources/js/p...
File ui/webui/resources/js/promise_resolver.js (right):

https://codereview.chromium.org/1755423003/diff/20001/ui/webui/resources/js/p...
ui/webui/resources/js/promise_resolver.js:28: this.reject;
maybe you already mentioned this (as I remember vaguely bringing this up
before), but why can't we just do this inside the promise callback? i.e.

/**
 * @constructor
 * @template T
 */
function PromiseResolver() {
  this.promise = new Promise(function(resolve, reject) {
    /** @type {function(T): void} */
    this.resolve = resolve;

    /** @type {function(*=): void} */
    this.reject = reject;
  }.bind(this));
}

(even if we need an @this {PromiseResolver} for the compiler to better
understand the scope in the callback)

[Dan Beam, 2016-03-04 19:29:43]

++code health!

[dpapad, 2016-03-04 20:01:28]

https://codereview.chromium.org/1755423003/diff/20001/ui/webui/resources/js/p...
File ui/webui/resources/js/promise_resolver.js (right):

https://codereview.chromium.org/1755423003/diff/20001/ui/webui/resources/js/p...
ui/webui/resources/js/promise_resolver.js:28: this.reject;
On 2016/03/04 at 19:29:26, Dan Beam wrote:
> maybe you already mentioned this (as I remember vaguely bringing this up
before), but why can't we just do this inside the promise callback? i.e.
> 
> /**
>  * @constructor
>  * @template T
>  */
> function PromiseResolver() {
>   this.promise = new Promise(function(resolve, reject) {
>     /** @type {function(T): void} */
>     this.resolve = resolve;
> 
>     /** @type {function(*=): void} */
>     this.reject = reject;
>   }.bind(this));
> }
> 
> (even if we need an @this {PromiseResolver} for the compiler to better
understand the scope in the callback)

Thanks for asking, this reminded me that I forgot to add the @struct annotation.
The reason for not declaring this.resolve and this.reject in the callback can be
seen by trying the following example (use "Advanced" compilation to get all the
checks, then click compile, then see the "warnings" tab). 

https://closure-compiler.appspot.com/home#code%3D%252F%252F%2520%253D%253DClo...

If you remove the @struct it compiles just fine, but I would like to keep the
@struct annotation because it is very useful, since it catches silly errors
(this.myTyppo = 'foo', instead of this.myTypo= 'foo'). ES6 Classes will be
@struct by default, see
https://github.com/google/closure-compiler/wiki/ECMAScript6. There has been talk
about making @constructor imply @struct, but that work has not been completed
AFAIK.

[Dan Beam, 2016-03-04 20:30:54]

still lgtm, thx for explanation

[dpapad, 2016-03-05 00:59:33]

Description was changed from

==========
Re-use PromiseResolver helper in print preview.
==========

to

==========
Re-use PromiseResolver helper in print preview.

TBR=tbarzic
==========

[dpapad, 2016-03-05 00:59:45]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-03-05 00:59:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1755423003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1755423003/40001

[dpapad, 2016-03-05 01:00:01]

Description was changed from

==========
Re-use PromiseResolver helper in print preview.

TBR=tbarzic
==========

to

==========
Re-use PromiseResolver helper in print preview.

TBR=tbarzic@chromium.org
==========

[commit-bot: I haz the power, 2016-03-05 02:55:12]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-05 02:56:14]

Description was changed from

==========
Re-use PromiseResolver helper in print preview.

TBR=tbarzic@chromium.org
==========

to

==========
Re-use PromiseResolver helper in print preview.

TBR=tbarzic@chromium.org

Committed: https://crrev.com/0db8c1a80c76832140adfc7c2171fbd6f9067713
Cr-Commit-Position: refs/heads/master@{#379446}
==========

[commit-bot: I haz the power, 2016-03-05 02:56:15]

Patchset 3 (id:??) landed as
https://crrev.com/0db8c1a80c76832140adfc7c2171fbd6f9067713
Cr-Commit-Position: refs/heads/master@{#379446}