| Summary: | [ES6] Implement the latest Promise spec in JS | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jordan Harband <ljharb> | ||||||||||||||||||||
| Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||
| Severity: | Normal | CC: | buildbot, rniwa, sam | ||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||
| Bug Depends on: | 146555 | ||||||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||
Since the current Promise is based on a little bit old spec, there's no one-on-one correspondence between the spec and the implementation. Adjusting the implementation to the spec is one solution to solve these problem like this issue. Sam, what do you think about it? And what do you think about writing the part of Promise implementation in JS builtins? (In reply to comment #1) > Since the current Promise is based on a little bit old spec, there's no > one-on-one correspondence between the spec and the implementation. > Adjusting the implementation to the spec is one solution to solve these > problem like this issue. > > Sam, what do you think about it? I think we need to update to match the latest spec. > And what do you think about writing the part of Promise implementation in JS builtins? I am in favor of it. Created attachment 256118 [details]
WIP, implemented Promise abstract operations in JS
WIP
Created attachment 256157 [details]
WIP
WIP; implemented. need to add the test for the original reported issue
Created attachment 256159 [details]
WIP
WIP
Created attachment 256173 [details]
Patch
Created attachment 256174 [details]
Patch
Comment on attachment 256174 [details] Patch Attachment 256174 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4724177877598208 Number of test failures exceeded the failure limit. Created attachment 256175 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 256174 [details] Patch Attachment 256174 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5521457757028352 Number of test failures exceeded the failure limit. Created attachment 256176 [details]
Archive of layout-test-results from ews101 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 256182 [details]
Patch
Comment on attachment 256182 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256182&action=review > Source/JavaScriptCore/ChangeLog:24 > + * CMakeLists.txt: > + * JavaScriptCore.vcxproj/JavaScriptCore.vcxproj: > + * JavaScriptCore.vcxproj/JavaScriptCore.vcxproj.filters: > + * JavaScriptCore.xcodeproj/project.pbxproj: > + * builtins/GlobalObject.js: > + (isObject): > + (isPromise): > + (newPromiseReaction): > + (newPromiseDeferred): This ChangeLog could use some per-function comments. > Source/JavaScriptCore/builtins/GlobalObject.js:55 > +function isObject(object) > +{ > + "use strict"; > + > + return (object !== null && typeof object === "object") || typeof object === "function"; > +} It looks like we are being inconsistent with the naming in this file. The functions above all start with a capital letter, but these below do not. Should they be different? They also have the starting brace on different lines. > Source/JavaScriptCore/builtins/GlobalObject.js:74 > +function newPromiseReaction(capability, handler) > +{ > + "use strict"; > + > + return { > + @capabilities: capability, > + @handler: handler > + }; > +} It seems like it is going to become a problem in the future if we have all the helper functions living in GlobalObject.js. Do we have some way to split it up? > Source/JavaScriptCore/builtins/Promise.prototype.js:40 > + // TODO: Fix this code when @@species well-known symbol is landed. TODO: This should probably be a FIXME. > Source/JavaScriptCore/builtins/PromiseConstructor.js:33 > + // TODO: Use @@species. TODO: This should probably be a FIXME and match the one in then(). Even better, if there is a bug about species support, note it in the FIXME. > Source/JavaScriptCore/builtins/PromiseConstructor.js:89 > + // TODO: Use @@species. > + var constructor = this; Same as above. Comment on attachment 256182 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256182&action=review Thank you for your review! >> Source/JavaScriptCore/ChangeLog:24 >> + (newPromiseDeferred): > > This ChangeLog could use some per-function comments. Added comments. >> Source/JavaScriptCore/builtins/GlobalObject.js:55 >> +} > > It looks like we are being inconsistent with the naming in this file. The functions above all start with a capital letter, but these below do not. Should they be different? They also have the starting brace on different lines. Changed ToLength to toLength. And ToInteger to toInteger. And adjust the brace rules. >> Source/JavaScriptCore/builtins/GlobalObject.js:74 >> +} > > It seems like it is going to become a problem in the future if we have all the helper functions living in GlobalObject.js. Do we have some way to split it up? Yup. I've splitted Promise related abstract operations to Operations.Promise.js. >> Source/JavaScriptCore/builtins/Promise.prototype.js:40 >> + // TODO: Fix this code when @@species well-known symbol is landed. > > TODO: This should probably be a FIXME. Fixed. >> Source/JavaScriptCore/builtins/PromiseConstructor.js:33 >> + // TODO: Use @@species. > > TODO: This should probably be a FIXME and match the one in then(). Even better, if there is a bug about species support, note it in the FIXME. Fixed the comment and noted https://bugs.webkit.org/show_bug.cgi?id=146624 >> Source/JavaScriptCore/builtins/PromiseConstructor.js:89 >> + var constructor = this; > > Same as above. Fixed. Committed r186298: <http://trac.webkit.org/changeset/186298> several tests are failing, I'll fix this. https://build.webkit.org/builders/Apple%20Mavericks%20Debug%20WK1%20%28Tests%29/builds/13985/steps/layout-test/logs/stdio Reopening to attach new patch. Created attachment 256191 [details]
Patch
Committed r186300: <http://trac.webkit.org/changeset/186300> |
`var count = 0, p = Promise.resolve(Object.defineProperty({}, 'then', { get() { count++; throw new RangeError(); } })); assert(count === 1, 'count'); p.catch(function (err) { assert(err instanceof RangeError, 'error'); assert(false, 'should fail here'); });` should reject synchronously, per http://www.ecma-international.org/ecma-262/6.0/#sec-promise.resolve step 6 which leads to http://www.ecma-international.org/ecma-262/6.0/#sec-promise-resolve-functions step 8 (step 12 is when it becomes asynchronous). Thus, the promise should reject synchronously, and the first two assertions should be true, and the third should fail. What happens now in nightly is that the first assertion fails, and that the promise ends up pending - it never resolves. However, the console inspection of p seems to flip back and forth between "pending" and "resolved". So, I'm not really sure what's going on here - the console inspection stuff isn't specced, so it's just weird, but the assertions should work as described.