Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
158003003:
Convert Google Now's State Change Gathering Mechanism to use Promises (Closed)
|
Created:
6 years, 10 months ago by robliao Modified:
6 years, 10 months ago CC:
chromium-reviews, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionConvert Google Now's State Change Gathering Mechanism to use Promises
Now that Promises have arrived in stable, we can start using them.
This change converts onStateChange to use promises and adds rudimentary
unit test support to handle this new feature.
V8's Promises are not used because the Javascript test harness expects
tests to complete synchronously. Promises, which run asynchronously, may
still be pending at the completion of a test, causing the test case to fail.
BUG=164227
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250992
Patch Set 1 #Patch Set 2 : Fix Typo #
Total comments: 5
Patch Set 3 : CR Feedback #Patch Set 4 : Fix Presubmit #
Total comments: 10
Patch Set 5 : Fix Comment #Patch Set 6 : Update Terminology #Patch Set 7 : Missed a spot #
Total comments: 2
Patch Set 8 : Change from .apply to .call #
Messages
Total messages: 29 (0 generated)
I suggest this order: first, share@ and rgustafson@ review this. I provide OWNERs review at the end.
Sounds good.
Removing vadimt for now. Review away!
https://codereview.chromium.org/158003003/diff/50001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/158003003/diff/50001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:1107: return new Promise(function(onSuccess) { Just out of curiosity, is onSuccess normal promise terminology? It just seems a bit strange since there isn't really failure/error here. I've seen people using "resolve". https://codereview.chromium.org/158003003/diff/50001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:1156: * Gets the previous Google Now opt-in state add . https://codereview.chromium.org/158003003/diff/50001/chrome/browser/resources... File chrome/browser/resources/google_now/common_test_util.js (right): https://codereview.chromium.org/158003003/diff/50001/chrome/browser/resources... chrome/browser/resources/google_now/common_test_util.js:89: function() {}); // Errors are unsupported. Two spaces between ; and comment.
https://codereview.chromium.org/158003003/diff/50001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/158003003/diff/50001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:1107: return new Promise(function(onSuccess) { On 2014/02/11 03:18:59, rgustafson wrote: > Just out of curiosity, is onSuccess normal promise terminology? It just seems a > bit strange since there isn't really failure/error here. I've seen people using > "resolve". We've got terminology to use onSuccess in the cards code and this is simply an extension of that. The success and failure terminology refer to the state of an evaluation: Were we able to evaluate isSignedIn? Were we able to evaluate if geolocation is enabled? If these evaluations occurred without problem, then it is a success evaluation. If there is any failure, it's a failure evaluation. Plus resolve and reject are awkward expressions for success and fail evaluations. https://codereview.chromium.org/158003003/diff/50001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:1156: * Gets the previous Google Now opt-in state On 2014/02/11 03:18:59, rgustafson wrote: > add . Done.
https://codereview.chromium.org/158003003/diff/200001/chrome/browser/resource... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/158003003/diff/200001/chrome/browser/resource... chrome/browser/resources/google_now/background.js:1091: isSignedIn(), consider .all([ new Promise(isSignedIn), new Promise(isGeolocationEnabled), ...]); where the functions are along the lines of function isX(onSuccess) { onSuccess(...); } ? IMO from a first pass, that's fewer lines of code, a little easier to read/write and maybe a little easier to unit test individual functions though that's not a big deal in this case. if not, there's is a comment below. https://codereview.chromium.org/158003003/diff/200001/chrome/browser/resource... chrome/browser/resources/google_now/background.js:1118: function isGeolocationEnabled() { I don't know what the best chrome/google promises style is, maybe because this might be the first component extension to use them, but would var isGeolocationEnabled = new Promise(function(foo) { }); work to remove the wrapper and isGeolocationEnabled() in the caller? https://codereview.chromium.org/158003003/diff/200001/chrome/browser/resource... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/158003003/diff/200001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:442: **/ tiny tiny nit: */ to close https://codereview.chromium.org/158003003/diff/200001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:443: Promise.prototype.then = function() { extending prototypes of builtins is warned against, though this is probably in the less-strict section of the rule vs. e.g. Array or String. https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#Modify...
https://codereview.chromium.org/158003003/diff/200001/chrome/browser/resource... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/158003003/diff/200001/chrome/browser/resource... chrome/browser/resources/google_now/background.js:1091: isSignedIn(), An async operation hands out promises to encapsulate the async behavior. This way you can group them together (like this code does) or chain them (which we will see later). Wrapping everything with 'new Promise' will get out of hand with chain comes along because you'll have to do this: new Promise(calculateResultFunc) .then(function(result) { return new Promise(consumeResultFunc(result)); } .then(function(result) { return new Promise(setResultFunc(result)); } instead of calculateResult() .then(consumeResult) .then(setResult); On 2014/02/11 20:07:44, Travis Skare wrote: > consider > .all([ > new Promise(isSignedIn), > new Promise(isGeolocationEnabled), > ...]); > > where the functions are along the lines of > function isX(onSuccess) { > onSuccess(...); > } > > ? > > IMO from a first pass, that's fewer lines of code, a little easier to read/write > and maybe a little easier to unit test individual functions though that's not a > big deal in this case. > > if not, there's is a comment below. https://codereview.chromium.org/158003003/diff/200001/chrome/browser/resource... chrome/browser/resources/google_now/background.js:1118: function isGeolocationEnabled() { A promise only provides one evaluation. Each new evaluation will require a new promise. On 2014/02/11 20:07:44, Travis Skare wrote: > I don't know what the best chrome/google promises style is, maybe because this > might be the first component extension to use them, but would > > var isGeolocationEnabled = new Promise(function(foo) { > > }); > > work to remove the wrapper and isGeolocationEnabled() in the caller? https://codereview.chromium.org/158003003/diff/200001/chrome/browser/resource... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/158003003/diff/200001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:442: **/ On 2014/02/11 20:07:44, Travis Skare wrote: > tiny tiny nit: */ to close Done. https://codereview.chromium.org/158003003/diff/200001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:443: Promise.prototype.then = function() { Unfortunately, this is the only way to get Promises to participate safely in the tasks system. Since DOM Objects don't support object extension, we can't subclass. On 2014/02/11 20:07:44, Travis Skare wrote: > extending prototypes of builtins is warned against, though this is probably in > the less-strict section of the rule vs. e.g. Array or String. > > https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#Modify...
Did a bit more digging on the terminology. ECMAScript uses resolve and reject. Windows uses complete/done and error Scheme uses force but doesn't handle errors inside a promise. So... we shall use resolve/reject.
lgtm
vadimt: Please provide owner approval for this CL.
https://codereview.chromium.org/158003003/diff/200001/chrome/browser/resource... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/158003003/diff/200001/chrome/browser/resource... chrome/browser/resources/google_now/background.js:1091: isSignedIn(), ok if it makes things easier later; the suggestion was just based on this patch in isolation since this is new and there are different implementations, what docs are you referencing? I was looking at https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje... and since that's on MDN want to make sure my comments are compatible :) On 2014/02/11 21:53:21, robliao wrote: > An async operation hands out promises to encapsulate the async behavior. This > way you can group them together (like this code does) or chain them (which we > will see later). > > Wrapping everything with 'new Promise' will get out of hand with chain comes > along because you'll have to do this: > new Promise(calculateResultFunc) > .then(function(result) { return new Promise(consumeResultFunc(result)); } > .then(function(result) { return new Promise(setResultFunc(result)); } > > instead of > calculateResult() > .then(consumeResult) > .then(setResult); > > > On 2014/02/11 20:07:44, Travis Skare wrote: > > consider > > .all([ > > new Promise(isSignedIn), > > new Promise(isGeolocationEnabled), > > ...]); > > > > where the functions are along the lines of > > function isX(onSuccess) { > > onSuccess(...); > > } > > > > ? > > > > IMO from a first pass, that's fewer lines of code, a little easier to > read/write > > and maybe a little easier to unit test individual functions though that's not > a > > big deal in this case. > > > > if not, there's is a comment below. >
https://codereview.chromium.org/158003003/diff/200001/chrome/browser/resource... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/158003003/diff/200001/chrome/browser/resource... chrome/browser/resources/google_now/background.js:1091: isSignedIn(), That's one of them. The other is here: http://wiki.ecmascript.org/doku.php?id=harmony:specification_drafts http://wiki.ecmascript.org/lib/exe/fetch.php?id=harmony%3Aspecification_draft... Section 25.4 covers promises. On 2014/02/12 19:55:58, Travis Skare wrote: > ok if it makes things easier later; the suggestion was just based on this patch > in isolation > > since this is new and there are different implementations, what docs are you > referencing? I was looking at > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje... > and since that's on MDN want to make sure my comments are compatible :) > > On 2014/02/11 21:53:21, robliao wrote: > > An async operation hands out promises to encapsulate the async behavior. This > > way you can group them together (like this code does) or chain them (which we > > will see later). > > > > Wrapping everything with 'new Promise' will get out of hand with chain comes > > along because you'll have to do this: > > new Promise(calculateResultFunc) > > .then(function(result) { return new Promise(consumeResultFunc(result)); } > > .then(function(result) { return new Promise(setResultFunc(result)); } > > > > instead of > > calculateResult() > > .then(consumeResult) > > .then(setResult); > > > > > > On 2014/02/11 20:07:44, Travis Skare wrote: > > > consider > > > .all([ > > > new Promise(isSignedIn), > > > new Promise(isGeolocationEnabled), > > > ...]); > > > > > > where the functions are along the lines of > > > function isX(onSuccess) { > > > onSuccess(...); > > > } > > > > > > ? > > > > > > IMO from a first pass, that's fewer lines of code, a little easier to > > read/write > > > and maybe a little easier to unit test individual functions though that's > not > > a > > > big deal in this case. > > > > > > if not, there's is a comment below. > > >
On 2014/02/12 18:54:29, robliao wrote: > vadimt: Please provide owner approval for this CL. After skare :)
On 2014/02/12 21:05:48, vadimt wrote: > On 2014/02/12 18:54:29, robliao wrote: > > vadimt: Please provide owner approval for this CL. > > After skare :) We've not required unanimous lgtm's in the past for both server and client side reviews. What's changed?
To clarify: are expecting skare@ to provide LGTM at some point?
If so, it makes sense to request OWNER's approval after the first ("local") wave
of reviews is over. This is how it works with other reviews, and how it worked
when arv@ was the reviewer.
On 2014/02/12 21:19:21, vadimt wrote:
> To clarify: are expecting skare@ to provide LGTM at some point?
> If so, it makes sense to request OWNER's approval after the first ("local")
wave
> of reviews is over. This is how it works with other reviews, and how it worked
> when arv@ was the reviewer.
Please ignore my LGTM :) it was just a keyword recognized by the tool :)
On 2014/02/12 21:19:21, vadimt wrote:
> To clarify: are expecting skare@ to provide LGTM at some point?
> If so, it makes sense to request OWNER's approval after the first ("local")
wave
> of reviews is over. This is how it works with other reviews, and how it worked
> when arv@ was the reviewer.
Working to get things done in parallel here. skare's comments are pretty much
all resolved.
lgtm https://codereview.chromium.org/158003003/diff/360001/chrome/browser/resource... File chrome/browser/resources/google_now/common_test_util.js (right): https://codereview.chromium.org/158003003/diff/360001/chrome/browser/resource... chrome/browser/resources/google_now/common_test_util.js:92: callback.apply(null, result); this works, just curious - could use result=asyncResult and alter callback.call(null, result)?
REAL LGTM
https://codereview.chromium.org/158003003/diff/360001/chrome/browser/resource... File chrome/browser/resources/google_now/common_test_util.js (right): https://codereview.chromium.org/158003003/diff/360001/chrome/browser/resource... chrome/browser/resources/google_now/common_test_util.js:92: callback.apply(null, result); Done. On 2014/02/12 22:39:03, Travis Skare wrote: > this works, just curious - could use result=asyncResult and alter > callback.call(null, result)?
The CQ bit was checked by robliao@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/158003003/520001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) app_list_unittests, ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests, components_unittests, compositor_unittests, content_browsertests, content_unittests, crypto_unittests, events_unittests, google_apis_unittests, gpu_unittests, installer_util_unittests, ipc_tests, jingle_unittests, media_unittests, mini_installer_test, nacl_integration, ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_perf_unittests, telemetry_unittests, views_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by robliao@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/158003003/520001
Message was sent while issue was closed.
Change committed as 250992 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
