Bug 6642 – Split XMLHttpRequest class into JS binding and implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Alexey Proskuryakov]

Split XMLHttpRequest class into JS binding and implementation

[Alexey Proskuryakov]

Created attachment 5755 [details]
first cut

A minimal patch that attempts no cleanup, to make it more readable
(ironically).

Which I don't like here is that the impl still has to know about the JS object
to gcProtect/gcUnprotect it while an async request is in progress. I couldn't
find a proper way to communicate this state back to the JS binding, and didn't
want to devise an ad-hoc broadcaster/listener scheme. OTOH, maybe the JS object
doesn't even need to be protected, and it's enough to bump the refcount of the
impl?

[Darin Adler]

Comment on attachment 5755 [details]
first cut

Namespace should be WebCore::, not DOM:: for XMLHttpRequestImpl and everywhere.
DOM is now just a synonym for WebCore, and an obsolete one.

We're not going to use the "Impl" suffix any more going forward, so you should
omit it and name the class XMLHttpRequest.

xmlhttprequestimpl.h should be XMLHttpRequest.h (to match the class name) and
should not be in the WebCore/kjs/ecma directory. I suggest the WebCore/xml
directory (which wouldbe a new one).

Please leave the commented-out lines out altogether.

In the code for StatusText, I think you should just use jsStringOrNull. I think
it's OK for a null string to become a JavaScript null rather than undefined.
Same for GetAllResponseHeaders.

I'd like to see the listeners move into the DOM too, but I can see that might
be tricky. In general this still leaves a number of things in the bindings that
we should get into the DOM.

Looks good, the right direction. This is going to be great!

[Alexey Proskuryakov]

Created attachment 5760 [details]
revised patch

- Renamed the namespace to WebCore.
- Removed commented-out lines.
- Moved KURL completion logic into the impl.
- (mostly) renamed the files. Kept xmlhttprequest.{h,cpp} in khtml/ecma to help
svn produce readable diffs; will move to WebCore/xml when that's no longer
needed.

Actually, I thought I have already moved listeners to the impl - or did I do it
incorrectly?

Later, I'm going to investigate how Safari null/undefined/throw responses match
over browsers and specifications. So, I don't think that arbitrarily changing
the behavior would be appropriate in this patch.

I still dislike it that gcProtect/gcUnprotect are in the impl... Do we really
need to protect the JSXMLHttpRequest object?

[Darin Adler]

(In reply to comment #3)
> Do we really need to protect the JSXMLHttpRequest object?

We don't. We just need to ref the underlying DOM object. It's just that gcProtect/gcUnprotect was the only 
way to do that when it wasn't a separate object. Now that it's separate, a simple ref/deref will do.

[Darin Adler]

Comment on attachment 5760 [details]
revised patch

Not sure I like the "JS" naming here, since it's novel and doesn't match the
rest of the ecma directory. But perhaps it's forward looking and all of this is
temporary since we hope to auto-generate the files.

We need to eliminate that gcProtect/Unprotect as you suggested.

All the places that say DOM:: should be WebCore:: instead.

I really dislike the special case for undefiined. So if you want to be
conservative and use the behavior, how about making a jsStringOrUndefined
alongside jsStringOrNull? When we autogenerate headers we're going to need
syntax to say whether a 0 should change to a null or an undefined or an empty
string, which is annoying. I'm hoping we won't ever need the undefined case.

[Alexey Proskuryakov]

Created attachment 5787 [details]
revised patch

[Alexey Proskuryakov]

Yes, the JS prefix was suggested by MacDome as forward looking (Maciej also was there).

jsStringOrUndefined was already defined and used in kjs_events.cpp, I moved it to kjs_binding.h.

[Alexey Proskuryakov]

Created attachment 5820 [details]
revised patch

Maciej pointed out that the previous version changed the semantics of URL completion - it would complete the URL for open() against the document the XMLHttpRequest was created with instead of based on the currently executing frame. So, returned URL completion back to JSXMLHttpRequest.

[Lucas Forschler]

Mass moving XML DOM bugs to the "DOM" Component.