[Streams API] Shield implementation from mangling then and catch promise methods
Created attachment 264866 [details] Patch
Created attachment 264931 [details] Patch Added another test for Object.setPrototypeOf. The test was already working before but it might be interesting to keep it just in case.
Comment on attachment 264931 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264931&action=review > LayoutTests/streams/streams-promises.html:60 > + Object.setPrototypeOf(promise, { then: function() { assert_unreached("streams should not use this promise then method"); } }); I am surprised the test is working before the patch. Can you try: Object.setPrototypeOf(promise, { constructor: Promise, then: ... }); It may be the case that Promise.resolve is wrapping the "promise" object because its constructor is not Promise.
Comment on attachment 264931 [details] Patch Additional comments. View in context: https://bugs.webkit.org/attachment.cgi?id=264931&action=review > LayoutTests/streams/streams-promises.html:65 > +}, 'Streams implementation is not affected if an attempt to replace promise prototype is made'); The two tests have the same name. Might be good to precise it here. > LayoutTests/streams/streams-promises.html:87 > + rs.tee(); Are we sure closed.catch is actually triggered? We should probably beef up the test here so that we ensure this in some way. > LayoutTests/streams/streams-promises.html:101 > +}, 'Streams implementation is not affected if promise then method is replaced'); Same name as above test. May be good to precise it.
Created attachment 265040 [details] Patch
Comment on attachment 265040 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265040&action=review r=me Test names look much better to me. Some small suggestions below. > LayoutTests/streams/streams-promises-expected.txt:5 > +PASS Streams and promises: attempt to replace Promise prototype Not sure we should keep this test > LayoutTests/streams/streams-promises-expected.txt:6 > +FAIL Streams and promises: replace a single promise prototype undefined is not a function replace prototype of a Promise object > LayoutTests/streams/streams-promises-expected.txt:7 > +PASS Streams and promises: replace then prototype method replace then function in Promise prototype > LayoutTests/streams/streams-promises-expected.txt:8 > +PASS Streams and promises: replace catch prototype method replace catch function in Promise prototype > LayoutTests/streams/streams-promises-expected.txt:9 > +PASS Streams and promises: replace then method in a single promise replace then function in Promise object Also, should we have a test for "replace catch function in Promise object"?
Created attachment 265046 [details] Patch for landing
(In reply to comment #6) > > LayoutTests/streams/streams-promises-expected.txt:5 > > +PASS Streams and promises: attempt to replace Promise prototype > > Not sure we should keep this test Me neither, removed. > replace prototype of a Promise object > > replace then function in Promise prototype > > replace catch function in Promise prototype > > replace then function in Promise object Changed all this names too, used method instead of function to be more OO. > Also, should we have a test for "replace catch function in Promise object"? Currently we don't need it because the only place were we use catch is in tee, with a promise that never leaves the code so currently there is not way to test it either.
Comment on attachment 265046 [details] Patch for landing Clearing flags on attachment: 265046 Committed r192157: <http://trac.webkit.org/changeset/192157>
All reviewed patches have been landed. Closing bug.