FormData.append() should throw TypeError if the number of arguments is less than 2. The spec: http://www.w3.org/TR/WebIDL/#dfn-overload-resolution-algorithm http://dvcs.w3.org/hg/xhr/raw-file/tip/Overview.html#the-append()-method
Created attachment 138459 [details] Patch
jianli: review?
Comment on attachment 138459 [details] Patch The test does not need anything related to setting http server, so it should put under fast/.
Created attachment 138586 [details] Patch
(In reply to comment #3) > (From update of attachment 138459 [details]) > The test does not need anything related to setting http server, so it should put under fast/. Done. Thanks.
Comment on attachment 138586 [details] Patch Please make sure the test passes in both JSC and V8.
Created attachment 138590 [details] patch for landing
Comment on attachment 138590 [details] patch for landing Wait, is spec compliance the only reason to make this change? Raising exceptions where we didn't use to do that is extremely dangerous for web compatibility, and shouldn't be treated lightly. Why do we have this behavior? What do other browsers do? Is there a specific reason why it's unlikely for WebKit specific content to rely on this quirk?
(In reply to comment #8) > (From update of attachment 138590 [details]) > Wait, is spec compliance the only reason to make this change? The spec compliance and the fact that almost all "not enough arguments" are throwing TypeError. At least, the current behavior that JSC does not throw any error but V8 throws SyntaxError is a bug. > Raising exceptions where we didn't use to do that is extremely dangerous for web compatibility, and shouldn't be treated lightly. Why do we have this behavior? What do other browsers do? Is there a specific reason why it's unlikely for WebKit specific content to rely on this quirk? Let me confirm other browsers' behavior. jianli: Was there any reason why you implemented SyntaxError in V8 and no error in JSC?
Created attachment 138604 [details] test case
Opera 11.62: FormData is not defined. Firefox 11.0: FormData.append(): [Exception... "Not enough arguments [nsIDOMFormData.append]" nsresult: "0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)" location: "JS frame :: http://www.corp.google.com/~haraken/null/typeerror.html :: :: line 12" data: no] I do not have IE now...
+1 for ap's comment as a basic standpoint. I think it's OK to change the error type thrown. But switching from non-throw to throw should be careful, especially for long standing APIs. Note that we sometimes selected non-standard behaviors for a compatibility reason in the past. I think this specific change is acceptable considering the Firefox behavior. It would be great if each differences is written down in the ChangeLog. It's a part of our accountability to webdev folks.
(In reply to comment #12) > +1 for ap's comment as a basic standpoint. > I think it's OK to change the error type thrown. > But switching from non-throw to throw should be careful, especially for long standing APIs. > Note that we sometimes selected non-standard behaviors for a compatibility reason in the past. > > I think this specific change is acceptable considering the Firefox behavior. > It would be great if each differences is written down in the ChangeLog. > It's a part of our accountability to webdev folks. (In reply to comment #9) > (In reply to comment #8) > > (From update of attachment 138590 [details] [details]) > > Wait, is spec compliance the only reason to make this change? > > The spec compliance and the fact that almost all "not enough arguments" are throwing TypeError. At least, the current behavior that JSC does not throw any error but V8 throws SyntaxError is a bug. > > > Raising exceptions where we didn't use to do that is extremely dangerous for web compatibility, and shouldn't be treated lightly. Why do we have this behavior? What do other browsers do? Is there a specific reason why it's unlikely for WebKit specific content to rely on this quirk? > > Let me confirm other browsers' behavior. > > jianli: Was there any reason why you implemented SyntaxError in V8 and no error in JSC? Per the comment from Sam (https://bugs.webkit.org/show_bug.cgi?id=36024), I changed the JSC implementation to throw no error. We can keep the V8 change if JSC wants to stick with the current behavior for consistency.
I got the result of IE. IE 9: FormData is not defined.
(In reply to comment #13) > Per the comment from Sam (https://bugs.webkit.org/show_bug.cgi?id=36024), I changed the JSC implementation to throw no error. We can keep the V8 change if JSC wants to stick with the current behavior for consistency. Sam: do you have an opinion to change no error to TypeError? (Leaving aside the compatibility problem, ) the first argument and the second argument of FormData.append() are not optional in the spec: http://dvcs.w3.org/hg/xhr/raw-file/tip/Overview.html#the-append()-method
Sam: Do you have any concern to fix this bug?
Created attachment 139480 [details] rebased patch
Created attachment 139482 [details] rebased patch
Created attachment 143259 [details] rebased patch
Comment on attachment 143259 [details] rebased patch Since Sam wrote that comment in the other bug, we've changed our default behavior to throwing exception when web sites supply too few arguments. Given that we're already throwing in this case in V8 and Firefox throws, this patch seems relatively safe from a web compatibly point of view. You might want to give Sam a chance to comment before landing. (I would also add the information about what other browsers do to the ChangeLog in case we look into the history of this behavior later.)
Created attachment 143412 [details] rebased patch for landing
Sam: Any comment for landing the patch?
*** Bug 103221 has been marked as a duplicate of this bug. ***
Sam: Sorry for revoking the old discussion... any objection to landing this patch?
I'm disappointed that the behavior of WebKit changed to throw on too many arguments. Can you confirm that both JSC and V8 do that for all of WebKit now?
(In reply to comment #25) > I'm disappointed that the behavior of WebKit changed to throw on too many arguments. Can you confirm that both JSC and V8 do that for all of WebKit now? I don't understand. The patch makes it throw on too few arguments (not too many). For this method, WebKit will not throw if too many arguments are provided.
(In reply to comment #26) > (In reply to comment #25) > > I'm disappointed that the behavior of WebKit changed to throw on too many arguments. Can you confirm that both JSC and V8 do that for all of WebKit now? > > I don't understand. The patch makes it throw on too few arguments (not too many). For this method, WebKit will not throw if too many arguments are provided. Sorry, I meant too few.
Sam, I'm very confused by your message. 1) This patch does not appear to have landed, so WebKit's behavior has not yet changed. 2) Can you explain why you would be disappointed if we landed this patch? Your comment above isn't very helpful because it just states your conclusion without letting us follow your reasoning.
(In reply to comment #28) > Sam, I'm very confused by your message. > > 1) This patch does not appear to have landed, so WebKit's behavior has not yet changed. > > 2) Can you explain why you would be disappointed if we landed this patch? Your comment above isn't very helpful because it just states your conclusion without letting us follow your reasoning. I apologize for not being clear. I was not disappointed that the patch was landed (as it hasn't) but rather that we had changed our behavior elsewhere to throw more often (something I don't think is a good idea as it is likely to break legacy content, for little gain). I was (am) trying to determine exactly what our current policy for throwing on incorrect number of arguments being provided, as I don't remember the specifics.
> I was (am) trying to determine exactly what our current policy for throwing on incorrect number of arguments being provided, as I don't remember the specifics. Our current policy is that new APIs should throw when given too few arguments (as required by the specs) but that we shouldn't change existing APIs to throw if there is a risk of compatibility problems. The way we've done this is to make the code generator default to throwing when too few arguments are supplied and to mark the parameters for APIs where the arguments are optional with the [Optional] attribute.
Sam: Thanks. As far as I checked a few months ago, FormData.append() was one of a few methods that don't throw TypeError. So we'd like to make the change for consistency if there is no compatibility concern. Given that Firefox and Chrome throw TypeError and that IE and Opera don't support FormData, it would make sense to throw TypeError in JSC.
Updated results from all browsers: *** Safari Technology Preview 152 *** FormData.append(): TypeError: Not enough arguments *** Firefox Nightly 106 *** FormData.append(): TypeError: FormData.append: 0 is not a valid argument count for any overload. *** Chrome Canary 107 *** FormData.append(): TypeError: Failed to execute 'append' on 'FormData': 2 arguments required, but only 0 present. ________ Just wanted to share updated status. Is anything needed here because all of them are giving same error but in different details? Thanks!
That confirms that this is resolved. Likely many years ago.