Server Request Tests and Mock Promise Enhancements

Server Request Tests and Mock Promise Enhancements

Added some server request tests and improved the mock promise to handle
these tests. The Mock Promise now better simulates the full extent of
promises.

BUG=164227
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258239

[rgustafson, 2014-03-19 00:24:31]

https://codereview.chromium.org/202293003/diff/1/chrome/browser/resources/goo...
File chrome/browser/resources/google_now/background_unittest.gtestjs (right):

https://codereview.chromium.org/202293003/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/background_unittest.gtestjs:69:
this.makeAndRegisterMockApis(['authenticationManager.removeToken']);
Just an FYI, 403 being here was a product of a previous server pre-apiserving
that wouldn't do exactly the right thing. I don't believe we are following
correct HTTP spec by reacting this way on 403s, and it doesn't really do
anything to refresh the auth token since the server should respond with another
403. Your choice what to do with that info.

Not a very critical test at the least.

https://codereview.chromium.org/202293003/diff/1/chrome/browser/resources/goo...
File chrome/browser/resources/google_now/common_test_util.js (right):

https://codereview.chromium.org/202293003/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/common_test_util.js:139: if
(isThenable(result)) {
I don't quite understand why this is here (and above). I'm guessing that you've
run these tests and they work, but this is a pretty hard read.

[robliao, 2014-03-19 00:43:38]

https://codereview.chromium.org/202293003/diff/1/chrome/browser/resources/goo...
File chrome/browser/resources/google_now/background_unittest.gtestjs (right):

https://codereview.chromium.org/202293003/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/background_unittest.gtestjs:69:
this.makeAndRegisterMockApis(['authenticationManager.removeToken']);
If we're forbidden to accessing this resource, it's indicative that maybe this
user isn't really a Google account user anymore. The idea here is indicating
token failure should cause Chrome to sign the user out.
On 2014/03/19 00:24:31, rgustafson wrote:
> Just an FYI, 403 being here was a product of a previous server pre-apiserving
> that wouldn't do exactly the right thing. I don't believe we are following
> correct HTTP spec by reacting this way on 403s, and it doesn't really do
> anything to refresh the auth token since the server should respond with
another
> 403. Your choice what to do with that info.
> 
> Not a very critical test at the least.

https://codereview.chromium.org/202293003/diff/1/chrome/browser/resources/goo...
File chrome/browser/resources/google_now/common_test_util.js (right):

https://codereview.chromium.org/202293003/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/common_test_util.js:139: if
(isThenable(result)) {
This code is indeed crazy. This implementation is adapted from the ECMAScript 6
spec
(http://wiki.ecmascript.org/lib/exe/fetch.php?id=harmony%3Aspecification_draft...)
with me adding synchronicity to it.

The craziness stems from Section 25.4.1.8 - UpdatePromiseFromPotentialThenable,
which is used during reaction and resolution.

Page 490 (PDF Page 508, Section 25.4) discusses Promises in all its detail.
On 2014/03/19 00:24:31, rgustafson wrote:
> I don't quite understand why this is here (and above). I'm guessing that
you've
> run these tests and they work, but this is a pretty hard read.

[rgustafson, 2014-03-19 23:02:25]

lgtm

https://codereview.chromium.org/202293003/diff/1/chrome/browser/resources/goo...
File chrome/browser/resources/google_now/common_test_util.js (right):

https://codereview.chromium.org/202293003/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/common_test_util.js:139: if
(isThenable(result)) {
On 2014/03/19 00:43:39, robliao wrote:
> This code is indeed crazy. This implementation is adapted from the ECMAScript
6
> spec
>
(http://wiki.ecmascript.org/lib/exe/fetch.php?id=harmony%253Aspecification_dra...)
> with me adding synchronicity to it.
> 
> The craziness stems from Section 25.4.1.8 -
UpdatePromiseFromPotentialThenable,
> which is used during reaction and resolution.
> 
> Page 490 (PDF Page 508, Section 25.4) discusses Promises in all its detail.
> On 2014/03/19 00:24:31, rgustafson wrote:
> > I don't quite understand why this is here (and above). I'm guessing that
> you've
> > run these tests and they work, but this is a pretty hard read.
> 

I'll take your word on this since that's just taking a small part of a more
complex larger spec. Maybe consider linking to that in comment for the whole
promise implementation, so some future person having to edit this has some idea
what's happening?

[skare_, 2014-03-19 23:20:23]

lgtm

https://codereview.chromium.org/202293003/diff/1/chrome/browser/resources/goo...
File chrome/browser/resources/google_now/common_test_util.js (right):

https://codereview.chromium.org/202293003/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/common_test_util.js:180:
PromisePrototypeObject.resolve = resolve;
this is in the scope of a prior cl, so maybe I/we asked earlier :)

can these be placed inside the class definition above and then returned
alongside then/catch/isPromise?

[robliao, 2014-03-19 23:34:39]

https://codereview.chromium.org/202293003/diff/1/chrome/browser/resources/goo...
File chrome/browser/resources/google_now/common_test_util.js (right):

https://codereview.chromium.org/202293003/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/common_test_util.js:139: if
(isThenable(result)) {
Links will change, so I've added a comment about this on the top.
On 2014/03/19 23:02:25, rgustafson wrote:
> On 2014/03/19 00:43:39, robliao wrote:
> > This code is indeed crazy. This implementation is adapted from the
ECMAScript
> 6
> > spec
> >
>
(http://wiki.ecmascript.org/lib/exe/fetch.php?id=harmony%25253Aspecification_d...)
> > with me adding synchronicity to it.
> > 
> > The craziness stems from Section 25.4.1.8 -
> UpdatePromiseFromPotentialThenable,
> > which is used during reaction and resolution.
> > 
> > Page 490 (PDF Page 508, Section 25.4) discusses Promises in all its detail.
> > On 2014/03/19 00:24:31, rgustafson wrote:
> > > I don't quite understand why this is here (and above). I'm guessing that
> > you've
> > > run these tests and they work, but this is a pretty hard read.
> > 
> 
> I'll take your word on this since that's just taking a small part of a more
> complex larger spec. Maybe consider linking to that in comment for the whole
> promise implementation, so some future person having to edit this has some
idea
> what's happening?

https://codereview.chromium.org/202293003/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/common_test_util.js:180:
PromisePrototypeObject.resolve = resolve;
What do you mean?

Are you asking for direct declarations like this?
PromisePrototypeObject.prototype.all = function(arrayOfPromises) {...}
On 2014/03/19 23:20:23, Travis Skare wrote:
> this is in the scope of a prior cl, so maybe I/we asked earlier :)
> 
> can these be placed inside the class definition above and then returned
> alongside then/catch/isPromise?

[skare_, 2014-03-20 00:27:26]

https://codereview.chromium.org/202293003/diff/1/chrome/browser/resources/goo...
File chrome/browser/resources/google_now/common_test_util.js (right):

https://codereview.chromium.org/202293003/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/common_test_util.js:180:
PromisePrototypeObject.resolve = resolve;
moving all the callable functions/'api' to one place: 
return {then: then, ..., all: all}
and moving functions if necessary, though I don't think that's needed since they
should be in the closure.

but I'm not sure it's a net readability gain, so it was pre-lgtm'ed


On 2014/03/19 23:34:39, robliao wrote:
> What do you mean?
> 
> Are you asking for direct declarations like this?
> PromisePrototypeObject.prototype.all = function(arrayOfPromises) {...}
> On 2014/03/19 23:20:23, Travis Skare wrote:
> > this is in the scope of a prior cl, so maybe I/we asked earlier :)
> > 
> > can these be placed inside the class definition above and then returned
> > alongside then/catch/isPromise?
>

[robliao, 2014-03-20 00:31:55]

xiyuan: Please provide owner approval for this CL. Thanks!

https://codereview.chromium.org/202293003/diff/1/chrome/browser/resources/goo...
File chrome/browser/resources/google_now/common_test_util.js (right):

https://codereview.chromium.org/202293003/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/common_test_util.js:180:
PromisePrototypeObject.resolve = resolve;
Ah. That wouldn't work. We actually need a constructor for this.
new Promise(...) is used by the code, which is why you see this sort of
initialization.

then, catch, and isPromise require instances. all, resolve, and reject are
"statics" and do not exist off of the global Promise object.

On 2014/03/20 00:27:27, Travis Skare wrote:
> moving all the callable functions/'api' to one place: 
> return {then: then, ..., all: all}
> and moving functions if necessary, though I don't think that's needed since
they
> should be in the closure.
> 
> but I'm not sure it's a net readability gain, so it was pre-lgtm'ed
> 
> 
> On 2014/03/19 23:34:39, robliao wrote:
> > What do you mean?
> > 
> > Are you asking for direct declarations like this?
> > PromisePrototypeObject.prototype.all = function(arrayOfPromises) {...}
> > On 2014/03/19 23:20:23, Travis Skare wrote:
> > > this is in the scope of a prior cl, so maybe I/we asked earlier :)
> > > 
> > > can these be placed inside the class definition above and then returned
> > > alongside then/catch/isPromise?
> > 
>

[robliao, 2014-03-20 00:32:49]

https://codereview.chromium.org/202293003/diff/1/chrome/browser/resources/goo...
File chrome/browser/resources/google_now/common_test_util.js (right):

https://codereview.chromium.org/202293003/diff/1/chrome/browser/resources/goo...
chrome/browser/resources/google_now/common_test_util.js:180:
PromisePrototypeObject.resolve = resolve;
s/do not exist/exist/
On 2014/03/20 00:31:55, robliao wrote:
> Ah. That wouldn't work. We actually need a constructor for this.
> new Promise(...) is used by the code, which is why you see this sort of
> initialization.
> 
> then, catch, and isPromise require instances. all, resolve, and reject are
> "statics" and do not exist off of the global Promise object.
> 
> On 2014/03/20 00:27:27, Travis Skare wrote:
> > moving all the callable functions/'api' to one place: 
> > return {then: then, ..., all: all}
> > and moving functions if necessary, though I don't think that's needed since
> they
> > should be in the closure.
> > 
> > but I'm not sure it's a net readability gain, so it was pre-lgtm'ed
> > 
> > 
> > On 2014/03/19 23:34:39, robliao wrote:
> > > What do you mean?
> > > 
> > > Are you asking for direct declarations like this?
> > > PromisePrototypeObject.prototype.all = function(arrayOfPromises) {...}
> > > On 2014/03/19 23:20:23, Travis Skare wrote:
> > > > this is in the scope of a prior cl, so maybe I/we asked earlier :)
> > > > 
> > > > can these be placed inside the class definition above and then returned
> > > > alongside then/catch/isPromise?
> > > 
> > 
>

[xiyuan, 2014-03-20 01:05:23]

lgtm

[robliao, 2014-03-20 03:41:14]

The CQ bit was checked by robliao@chromium.org

[commit-bot: I haz the power, 2014-03-20 03:41:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robliao@chromium.org/202293003/20001

[commit-bot: I haz the power, 2014-03-20 07:01:21]

Change committed as 258239