Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2001283006:
Promises: Rename functions/parameters to match spec (Closed)
DescriptionThis patch updates certain functions and parameters to match the Promise spec.
Committed: https://crrev.com/ffdd76e61be3010d7fad2f280fa09d8abfe298f6
Cr-Commit-Position: refs/heads/master@{#36535}
Patch Set 1 #Patch Set 2 : Rename executor #
Total comments: 8
Patch Set 3 : fix anchor link #
Total comments: 4
Patch Set 4 : Address comments #
Total comments: 1
Patch Set 5 : Remove quotes #
Total comments: 2
Messages
Total messages: 30 (11 generated)
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2001283006/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2001283006/1
Dry run: None
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2001283006/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2001283006/20001
Description was changed from ========== Promises: Rename certain functions to match spec ========== to ========== Promises: Rename functions/variables to match spec ==========
gsathya@chromium.org changed reviewers: + adamk@chromium.org, littledan@chromium.org
Description was changed from ========== Promises: Rename functions/variables to match spec ========== to ========== This patch updates certain functions and parameters to match the Promise spec. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Dry run: None
https://codereview.chromium.org/2001283006/diff/20001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2001283006/diff/20001/src/js/promise.js#newco... src/js/promise.js:165: function PromiseNoopResolver() {} This one doesn't seem spec related? https://codereview.chromium.org/2001283006/diff/20001/src/js/promise.js#newco... src/js/promise.js:181: // ES#sec-resolvepromise This section doesn't exist. The format of these is "ES#actual-anchor-in-spec" https://codereview.chromium.org/2001283006/diff/20001/src/js/promise.js#newco... src/js/promise.js:182: // ResolvePromise ( promise, value) Nor does this abstract operation. What does it correspond to in the spec? Looks like it's most of the steps of https://tc39.github.io/ecma262/#sec-promise-resolve-functions?
Thanks for the review! PTAL https://codereview.chromium.org/2001283006/diff/20001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2001283006/diff/20001/src/js/promise.js#newco... src/js/promise.js:165: function PromiseNoopResolver() {} On 2016/05/25 20:57:26, adamk wrote: > This one doesn't seem spec related? Yeah, this isn't. I thought nop was a typo https://codereview.chromium.org/2001283006/diff/20001/src/js/promise.js#newco... src/js/promise.js:181: // ES#sec-resolvepromise On 2016/05/25 20:57:26, adamk wrote: > This section doesn't exist. The format of these is "ES#actual-anchor-in-spec" Aha. Changed. https://codereview.chromium.org/2001283006/diff/20001/src/js/promise.js#newco... src/js/promise.js:182: // ResolvePromise ( promise, value) On 2016/05/25 20:57:26, adamk wrote: > Nor does this abstract operation. What does it correspond to in the spec? Looks > like it's most of the steps of > https://tc39.github.io/ecma262/#sec-promise-resolve-functions? Yeah, I've updated the anchor to link to this. Let me know if there's a better name for this
https://codereview.chromium.org/2001283006/diff/20001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2001283006/diff/20001/src/js/promise.js#newco... src/js/promise.js:165: function PromiseNoopResolver() {} On 2016/05/25 23:09:24, gsathya wrote: > On 2016/05/25 20:57:26, adamk wrote: > > This one doesn't seem spec related? > > Yeah, this isn't. I thought nop was a typo Ah, understood. I believe "nop" is an accepted spelling, I'd leave that change out of this patch. https://codereview.chromium.org/2001283006/diff/40001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2001283006/diff/40001/src/js/promise.js#newco... src/js/promise.js:182: // ResolvePromise ( promise, value) I'd just remove this line. Maybe replace it with: "Promise Resolve Functions, steps 6-13" https://codereview.chromium.org/2001283006/diff/40001/src/js/promise.js#newco... src/js/promise.js:183: function ResolvePromise(promise, value) { Isn't "value" called "resolution" in the spec?
https://codereview.chromium.org/2001283006/diff/20001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2001283006/diff/20001/src/js/promise.js#newco... src/js/promise.js:165: function PromiseNoopResolver() {} On 2016/05/25 23:19:18, adamk wrote: > On 2016/05/25 23:09:24, gsathya wrote: > > On 2016/05/25 20:57:26, adamk wrote: > > > This one doesn't seem spec related? > > > > Yeah, this isn't. I thought nop was a typo > > Ah, understood. I believe "nop" is an accepted spelling, I'd leave that change > out of this patch. Done. https://codereview.chromium.org/2001283006/diff/40001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2001283006/diff/40001/src/js/promise.js#newco... src/js/promise.js:182: // ResolvePromise ( promise, value) On 2016/05/25 23:19:18, adamk wrote: > I'd just remove this line. Maybe replace it with: > > "Promise Resolve Functions, steps 6-13" Done. https://codereview.chromium.org/2001283006/diff/40001/src/js/promise.js#newco... src/js/promise.js:183: function ResolvePromise(promise, value) { On 2016/05/25 23:19:18, adamk wrote: > Isn't "value" called "resolution" in the spec? Done.
lgtm % smallest possible nit below https://codereview.chromium.org/2001283006/diff/60001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2001283006/diff/60001/src/js/promise.js#newco... src/js/promise.js:182: // "Promise Resolve Functions, steps 6-13" Sorry, didn't mean literal quotes. Pls remove thx.
On 2016/05/25 23:55:25, adamk wrote: > lgtm % smallest possible nit below > > https://codereview.chromium.org/2001283006/diff/60001/src/js/promise.js > File src/js/promise.js (right): > > https://codereview.chromium.org/2001283006/diff/60001/src/js/promise.js#newco... > src/js/promise.js:182: // "Promise Resolve Functions, steps 6-13" > Sorry, didn't mean literal quotes. Pls remove thx. Oops, removed.
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2001283006/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2001283006/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Dry run: None
The CQ bit was checked by gsathya@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org Link to the patchset: https://codereview.chromium.org/2001283006/#ps80001 (title: "Remove quotes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2001283006/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2001283006/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== This patch updates certain functions and parameters to match the Promise spec. ========== to ========== This patch updates certain functions and parameters to match the Promise spec. Committed: https://crrev.com/ffdd76e61be3010d7fad2f280fa09d8abfe298f6 Cr-Commit-Position: refs/heads/master@{#36535} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ffdd76e61be3010d7fad2f280fa09d8abfe298f6 Cr-Commit-Position: refs/heads/master@{#36535}
Message was sent while issue was closed.
I like how this patch fixes up the names of some local variables, but I'm not a big fan of the function renamings. Our Promise code is simply factored differently from the spec, ultimately to reduce code duplication. If we want it to correspond better, then we either need to refactor this code or the spec; I think these function renamings just add to the confusion. https://codereview.chromium.org/2001283006/diff/80001/src/js/promise.js File src/js/promise.js (left): https://codereview.chromium.org/2001283006/diff/80001/src/js/promise.js#oldco... src/js/promise.js:121: function PromiseDone(promise, status, value, promiseQueue) { I'm not a big fan of this name change. This is a function which doesn't directly correspond to anything in the spec, as it takes the PromiseQueue as an argument. Maybe you could make a helper function called FulfillPromise which calls PromiseDone with the right promiseQueue. https://codereview.chromium.org/2001283006/diff/80001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2001283006/diff/80001/src/js/promise.js#newco... src/js/promise.js:182: // Promise Resolve Functions, steps 6-13 As your comment says, the contents of this function don't correspond to promise resolve functions either--only part of it. The code here is simply factored differently from the spec. I am not a big fan of the name ResolvePromise as it sounds a lot like PromiseResolve, which is Promise.prototype.resolve, whereas FulfillPromise is something that appears in the spec which this largely corresponds to. |
