Bug 39125 – XMLHttpRequest.getResponseHeader doesn't need to be custom

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Adam Barth]

XMLHttpRequest.getResponseHeader doesn't need to be custom

[Adam Barth]

Created attachment 56087 [details]
Patch

[Adam Barth]

Comment on attachment 56087 [details]
Patch

Thanks.

[Alexey Proskuryakov]

+        // FIXME: RequiresAllArguments is probably bogus here.

I don't know what to do with this FIXME when I see it. On what grounds can the FIXME be removed, if it's not bogus?

I think that a FIXME should contain a specific claim that can be addressed or rejected, but "bogus" is open ended.

[Adam Barth]

We should make a project-wide decision about whether our bindings require all arguments by default.  In making that decision, we should consider the specs, other browsers, and that vast majority of our bindings do not require all arguments.  I added these comments to ensure that future developers don't think that I put the attribute there for a reason other than inertia.

[Alexey Proskuryakov]

I'd say it's enough to mention that in ChangeLog - there is very little in JS bindings that's never supposed to be reconsidered.

Generally, if not passing an argument makes no sense (like here), and we could get away with detecting that and reporting a useful error for all those years, then it's probably best to keep this developer-friendly behavior.

[Adam Barth]

If that's the case, then we should enforce a minimum number of arguments on all the functions in the bindings.  Having some enforce it and some not is a bunch of entropy for no reason.

The last time I discussed this with mjs, my impression was that he favored not requiring all the arguments because that's how JavaScript works natively.

Honestly, I don't much care.  I just care that we're consistent.

[Alexey Proskuryakov]

I don't really see this as an "all or none" case. We should be compatible above all, and helpful to developers when possible.

[Adam Barth]

(In reply to comment #7)
> I don't really see this as an "all or none" case. We should be compatible above all, and helpful to developers when possible.

Is our current behavior a product of compatibility considerations or just whatever someone happened to copy/paste?  In any case, it's something I'd like to worry about in the future, hence the FIXME.

[Alexey Proskuryakov]

Firefox and IE behave differently, so compatibility isn't a product of consideration, but a product of testing and adjustment. We've had this behavior since 2006, so it's vetted pretty well.

The arguments against detecting and reporting errors seem to be purely aesthetic, while the argument for detecting them is about making the platform easier to develop for. Calling getResponseHeader() without arguments makes no sense, and pretty much the only conceivable case when this could happen is a wrong copy/paste for getAllResponseHeaders(). Why make that harder to debug?

[Adam Barth]

I feel like you're not listening to what I'm saying.  Please re-read the above comments for the answer to your question.

[Alexey Proskuryakov]

I re-read your comments, and don't see an answer.

[Adam Barth]

(In reply to comment #9)
> Why make that harder to debug?

(In reply to comment #6)
> Honestly, I don't much care.  I just care that we're consistent.

[Alexey Proskuryakov]

Oh, that's what I called "purely aesthetic" in the same paragraph you quoted. I don't see it as an important counter-argument, web developers' convenience trumps our convenience and our aesthetic feelings.

[Adam Barth]

(In reply to comment #13)
> Oh, that's what I called "purely aesthetic" in the same paragraph you quoted. I don't see it as an important counter-argument, web developers' convenience trumps our convenience and our aesthetic feelings.

In that case, we should enforce minimum argument lengths everywhere that compatibility doesn't force our hand to do something else.

[Adam Barth]

(In reply to comment #4)
> We should make a project-wide decision about whether our bindings require all arguments by default.  In making that decision, we should consider the specs, other browsers, and that vast majority of our bindings do not require all arguments.

It seems your saying that we should add "developer convenience" to that list.

In any case, we should make a project-wide decision for what our default stance should be, especially for new code that doesn't have any compat baggage.  Currently, our practice is to randomly enforce minimum arguments depending on what code is used as a template.  That's clearly lame.

[Alexey Proskuryakov]

I agree that having a default stance would be good, but I'm not sure if it's achievable at this point. The arguments are all known, and it seems that we basically deferred this decision to WebIDL.

[WebKit Commit Bot]

Comment on attachment 56087 [details]
Patch

Rejecting patch 56087 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Darin Adler', u'--force']" exit_code: 1
Last 500 characters of output:
(s).
patching file WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebCore/bindings/js/JSXMLHttpRequestCustom.cpp
Hunk #1 FAILED at 133.
1 out of 1 hunk FAILED -- saving rejects to file WebCore/bindings/js/JSXMLHttpRequestCustom.cpp.rej
patching file WebCore/bindings/v8/custom/V8XMLHttpRequestCustom.cpp
Hunk #1 FAILED at 157.
1 out of 1 hunk FAILED -- saving rejects to file WebCore/bindings/v8/custom/V8XMLHttpRequestCustom.cpp.rej
patching file WebCore/xml/XMLHttpRequest.idl

Full output: http://webkit-commit-queue.appspot.com/results/2313120

[Adam Barth]

Created attachment 56165 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 56165 [details]
Patch for landing

Clearing flags on attachment: 56165

Committed r59559: <http://trac.webkit.org/changeset/59559>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.