see http://tc39.github.io/ecma262/#sec-function.prototype.bind
Created attachment 270510 [details] patch
Comment on attachment 270510 [details] patch r=me
Comment on attachment 270510 [details] patch Attachment 270510 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/773879 New failing tests: http/tests/media/media-source/SourceBuffer-abort-removed.html http/tests/media/media-source/SourceBuffer-abort-readyState.html inspector/runtime/getProperties.html imported/w3c/web-platform-tests/XMLHttpRequest/interfaces.html http/tests/media/media-source/mediasource-sourcebuffer-mode.html
Created attachment 270512 [details] Archive of layout-test-results from ews117 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 270510 [details] patch Attachment 270510 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/773942 New failing tests: http/tests/media/media-source/mediasource-sourcebuffer-mode.html imported/w3c/web-platform-tests/XMLHttpRequest/interfaces.html http/tests/media/media-source/SourceBuffer-abort-removed.html http/tests/media/media-source/SourceBuffer-abort-readyState.html imported/w3c/web-platform-tests/html/dom/interfaces.html imported/w3c/web-platform-tests/dom/interfaces.html inspector/runtime/getProperties.html
Created attachment 270514 [details] Archive of layout-test-results from ews102 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 270510 [details] patch Attachment 270510 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/773944 New failing tests: http/tests/media/media-source/mediasource-sourcebuffer-mode.html imported/w3c/web-platform-tests/XMLHttpRequest/interfaces.html http/tests/media/media-source/SourceBuffer-abort-removed.html http/tests/media/media-source/SourceBuffer-abort-readyState.html imported/w3c/web-platform-tests/html/dom/interfaces.html imported/w3c/web-platform-tests/dom/interfaces.html inspector/runtime/getProperties.html
Created attachment 270516 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Created attachment 270522 [details] patch updated toString() of bound functions.
Attachment 270522 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/JSBoundFunction.cpp:140: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 270522 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=270522&action=review > Source/JavaScriptCore/runtime/JSBoundFunction.cpp:142 > + String result = name(exec); > + ASSERT(result.find(ASCIILiteral(boundPrefix)) == 0); > + static const size_t boundPrefixLength = 6; > + return result.substring(boundPrefixLength); Can't we just get the name out of m_targetFunction?
Comment on attachment 270522 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=270522&action=review r=me > Source/JavaScriptCore/runtime/JSBoundFunction.cpp:90 > +static const char* boundPrefix = "bound "; nit: this can be static const char* const boundPrefix = "bound "; There's no need for the pointer to not be const.
(In reply to comment #11) > Comment on attachment 270522 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=270522&action=review > > > Source/JavaScriptCore/runtime/JSBoundFunction.cpp:142 > > + String result = name(exec); > > + ASSERT(result.find(ASCIILiteral(boundPrefix)) == 0); > > + static const size_t boundPrefixLength = 6; > > + return result.substring(boundPrefixLength); > > Can't we just get the name out of m_targetFunction? It's because m_targetFunction might not be a JSFunction. It could be an InternalFunction too. I think it's cleaner to just do it this way.
Created attachment 270525 [details] patch for landing updated test results.
Attachment 270525 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/JSBoundFunction.cpp:140: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
landed in: http://trac.webkit.org/changeset/196033
Comment on attachment 270525 [details] patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=270525&action=review > Source/JavaScriptCore/runtime/JSBoundFunction.cpp:90 > +static const char* const boundPrefix = "bound "; We normally write this: static const char boundPrefix[] = "bound "; > Source/JavaScriptCore/runtime/JSBoundFunction.cpp:100 > + function->finishCreation(vm, executable, length, makeString(ASCIILiteral(boundPrefix), name)); I think that stating ASCIILiteral here explicitly might actually make things slower by causing a string to be allocated. It’s an optimization for creating a string, not needed when concatenating to make a string. Please just write "boundPrefix + name" or "makeString(boundPrefix, name)", both of which should be efficient. > Source/JavaScriptCore/runtime/JSBoundFunction.cpp:140 > + ASSERT(result.find(ASCIILiteral(boundPrefix)) == 0); This should be: ASSERT(result.startsWith(boundPrefix)); > Source/JavaScriptCore/runtime/JSBoundFunction.cpp:141 > + static const size_t boundPrefixLength = 6; The substring function takes an unsigned, not a size_t. Might be nice to just use strlen(boundPrefix) instead of boundPrefixLength. I believe modern compilers do constant folding in this kind of case.
(In reply to comment #13) > (In reply to comment #11) > > Comment on attachment 270522 [details] > > patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=270522&action=review > > > > > Source/JavaScriptCore/runtime/JSBoundFunction.cpp:142 > > > + String result = name(exec); > > > + ASSERT(result.find(ASCIILiteral(boundPrefix)) == 0); > > > + static const size_t boundPrefixLength = 6; > > > + return result.substring(boundPrefixLength); > > > > Can't we just get the name out of m_targetFunction? > > It's because m_targetFunction might not be a JSFunction. > It could be an InternalFunction too. I think it's cleaner > to just do it this way. Maybe more expedient. Not cleaner!
Comment on attachment 270522 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=270522&action=review > Source/JavaScriptCore/ChangeLog:15 > + See http://tc39.github.io/ecma262/#sec-function.prototype.bind for details. > + What the spec says: > + ``` > + function foo() { } > + foo.bind(null).name === "bound foo" > + > + (function bar() { }).bind(null).name === "bound bar" > + ``` It would be nice to have more tests for this. Some examples: // Nested (function foo() {}).bind(null).bind(null).name === "bound bound foo" // Anonymous (function(){}).bind(null).name === "bound "
Comment on attachment 270525 [details] patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=270525&action=review I'll come up w/ a follow-up patch to address your comments. >> Source/JavaScriptCore/runtime/JSBoundFunction.cpp:100 >> + function->finishCreation(vm, executable, length, makeString(ASCIILiteral(boundPrefix), name)); > > I think that stating ASCIILiteral here explicitly might actually make things slower by causing a string to be allocated. It’s an optimization for creating a string, not needed when concatenating to make a string. Please just write "boundPrefix + name" or "makeString(boundPrefix, name)", both of which should be efficient. I'll go with makeString(.) >> Source/JavaScriptCore/runtime/JSBoundFunction.cpp:140 >> + ASSERT(result.find(ASCIILiteral(boundPrefix)) == 0); > > This should be: > > ASSERT(result.startsWith(boundPrefix)); neat. I didn't know we had this function. >> Source/JavaScriptCore/runtime/JSBoundFunction.cpp:141 >> + static const size_t boundPrefixLength = 6; > > The substring function takes an unsigned, not a size_t. > > Might be nice to just use strlen(boundPrefix) instead of boundPrefixLength. I believe modern compilers do constant folding in this kind of case. I'll go with strlen
(In reply to comment #18) > (In reply to comment #13) > > (In reply to comment #11) > > > Comment on attachment 270522 [details] > > > patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=270522&action=review > > > > > > > Source/JavaScriptCore/runtime/JSBoundFunction.cpp:142 > > > > + String result = name(exec); > > > > + ASSERT(result.find(ASCIILiteral(boundPrefix)) == 0); > > > > + static const size_t boundPrefixLength = 6; > > > > + return result.substring(boundPrefixLength); > > > > > > Can't we just get the name out of m_targetFunction? > > > > It's because m_targetFunction might not be a JSFunction. > > It could be an InternalFunction too. I think it's cleaner > > to just do it this way. > > Maybe more expedient. Not cleaner! Expediency wasn't my goal here. I spent more time debating about whether or not to do it this way than the time it would have taken me to write the extra ~3 lines of code. My thinking about this is as follows: If we case on everything that could be bound as the target of a bound function, any time we add a new thing that can be bound by a JSBoundFunction, we have yet even one more function to change. There is something nice about an implementer not worrying about this function, and this function not worrying about the rest of the runtime.
(In reply to comment #19) > Comment on attachment 270522 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=270522&action=review > > > Source/JavaScriptCore/ChangeLog:15 > > + See http://tc39.github.io/ecma262/#sec-function.prototype.bind for details. > > + What the spec says: > > + ``` > > + function foo() { } > > + foo.bind(null).name === "bound foo" > > + > > + (function bar() { }).bind(null).name === "bound bar" > > + ``` > > It would be nice to have more tests for this. > > Some examples: > > // Nested > (function foo() {}).bind(null).bind(null).name === "bound bound foo" This is a good test to have, I'll add it in a follow-up patch. > > // Anonymous > (function(){}).bind(null).name === "bound " We already have a test for this inside the ES6 test suite.
(In reply to comment #21) > (In reply to comment #18) > > (In reply to comment #13) > > > (In reply to comment #11) > > > > Comment on attachment 270522 [details] > > > > patch > > > > > > > > View in context: > > > > https://bugs.webkit.org/attachment.cgi?id=270522&action=review > > > > > > > > > Source/JavaScriptCore/runtime/JSBoundFunction.cpp:142 > > > > > + String result = name(exec); > > > > > + ASSERT(result.find(ASCIILiteral(boundPrefix)) == 0); > > > > > + static const size_t boundPrefixLength = 6; > > > > > + return result.substring(boundPrefixLength); > > > > > > > > Can't we just get the name out of m_targetFunction? > > > > > > It's because m_targetFunction might not be a JSFunction. > > > It could be an InternalFunction too. I think it's cleaner > > > to just do it this way. > > > > Maybe more expedient. Not cleaner! > > Expediency wasn't my goal here. I spent more time debating > about whether or not to do it this way than the time it would > have taken me to write the extra ~3 lines of code. > My thinking about this is as follows: > If we case on everything that could be bound as the target of a bound > function, > any time we add a new thing that can be bound by a JSBoundFunction, we have > yet even one more function to change. There is something nice about an > implementer > not worrying about this function, and this function not worrying about the > rest > of the runtime. Actually, this is really silly. We can just do m_target->get(exec, exec->vm().propertyNames->name).toWTFString(exec); This is what JSFunction::name does, and it's to inline here because it's expected that function-like things to store to that property.
(In reply to comment #23) > (In reply to comment #21) > > (In reply to comment #18) > > > (In reply to comment #13) > > > > (In reply to comment #11) > > > > > Comment on attachment 270522 [details] > > > > > patch > > > > > > > > > > View in context: > > > > > https://bugs.webkit.org/attachment.cgi?id=270522&action=review > > > > > > > > > > > Source/JavaScriptCore/runtime/JSBoundFunction.cpp:142 > > > > > > + String result = name(exec); > > > > > > + ASSERT(result.find(ASCIILiteral(boundPrefix)) == 0); > > > > > > + static const size_t boundPrefixLength = 6; > > > > > > + return result.substring(boundPrefixLength); > > > > > > > > > > Can't we just get the name out of m_targetFunction? > > > > > > > > It's because m_targetFunction might not be a JSFunction. > > > > It could be an InternalFunction too. I think it's cleaner > > > > to just do it this way. > > > > > > Maybe more expedient. Not cleaner! > > > > Expediency wasn't my goal here. I spent more time debating > > about whether or not to do it this way than the time it would > > have taken me to write the extra ~3 lines of code. > > My thinking about this is as follows: > > If we case on everything that could be bound as the target of a bound > > function, > > any time we add a new thing that can be bound by a JSBoundFunction, we have > > yet even one more function to change. There is something nice about an > > implementer > > not worrying about this function, and this function not worrying about the > > rest > > of the runtime. > Actually, this is really silly. > We can just do m_target->get(exec, > exec->vm().propertyNames->name).toWTFString(exec); > This is what JSFunction::name does, and it's to inline here > because it's expected that function-like things to store to that property. "it's to" => "it's appropriate to"
Reopening because I'm going to post follow-ups under this bug.
Created attachment 270563 [details] follow-up patch
Created attachment 270566 [details] follow-up patch change test output
Comment on attachment 270566 [details] follow-up patch View in context: https://bugs.webkit.org/attachment.cgi?id=270566&action=review > Source/JavaScriptCore/runtime/JSBoundFunction.cpp:137 > + return m_targetFunction->get(exec, exec->vm().propertyNames->name).toWTFString(exec); What happens if m_targetFunction->get() throws an exception? Can you add a test case where the bound function has a getter on name that throws?
(In reply to comment #28) > Comment on attachment 270566 [details] > follow-up patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=270566&action=review > > > Source/JavaScriptCore/runtime/JSBoundFunction.cpp:137 > > + return m_targetFunction->get(exec, exec->vm().propertyNames->name).toWTFString(exec); > > What happens if m_targetFunction->get() throws an exception? Can you add a > test case where the bound function has a getter on name that throws? Unfortunately it's not that easy. "name" is a non-writable, non-configurable property. I'm sure there are really weird/rare situations where we might throw an error because of OOM or something when getting this property (for any number of reasons), but I'm not sure how to write a straight-forward test that does that.
Comment on attachment 270566 [details] follow-up patch Clearing flags on attachment: 270566 Committed r196243: <http://trac.webkit.org/changeset/196243>
All reviewed patches have been landed. Closing bug.