[webkit-reviews] review granted: [Bug 6272] XMLHttpRequest freezes on getting a missing document with overridden Content-Length : [Attachment 12416] proposed fix

[bugzilla-request-daemon at macosforge.org]

Darin Adler <darin at apple.com> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 6272: XMLHttpRequest freezes on getting a missing document with overridden
Content-Length
http://bugs.webkit.org/show_bug.cgi?id=6272

Attachment 12416: proposed fix
http://bugs.webkit.org/attachment.cgi?id=12416&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
+	 void loadResourceSynchronously(const ResourceRequest& request,
ResourceError& error, ResourceResponse& r, Vector<char>& data);

Really should leave out those parameter names ("request", "error", "r").

It's a real shame that canSetRequestHeader has to call lower on each string
passed into it. Because of that, we lose much if not all of the performance
advantage of using AtomicString. I could see just using a HashSet<StringImpl*,
CaseInsensitiveHash<StringImpl*> > instead.

I think it's a little ugly and unnecessarily costly to have all those
AtomicString globals. Instead you could just have a little helper function that
makes an atomic string, adds it to the set and ref's the atomic string's impl.
Like this:

    static void addString(HashSet<AtomicStringImpl*>& set, const char* string)
    {
	AtomicString atomicString(string);
	set.add(atomicString.impl());
	atomicString.impl()->ref();
    }

    ...

    if (forbiddenHeaders.isEmpty()) {
	addString(forbiddenHeaders, "accept-charset");
	...

Where did the list of forbidden headers come from? How are you sure you have
the right list?

+	 // FIXME: Report a warning to the user.

We could put something on the console there. Why not do that, instead of having
the FIXME?

Why not add SECURITY_ERR with this patch? Is there a risk that if after add it
to DOM Core, the value will change?

There are enough questions here that I'm going to say review-, but I really
approve of everything in here!