726434 - Return DOMString for XMLHttpRequest.getAllResponseHeaders()

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

There is no point in doing silly things like ToNewCString(EmptyString()), and we'll probably want this for the DOM bindings work in bug 580070.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 596476 [details] [diff] [review]
Patch v1

I think this should be ok, but we need to return a void string rather than an empty string to make sure we don't break pages (they could currently test for null, who knows).

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

You saw

-  if (!*_retval) {
-    *_retval = ToNewCString(EmptyString());
-  }

right?

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 596476 [details] [diff] [review]
Patch v1

Review of attachment 596476 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, I didn't look very closely before. This seems good.

::: content/base/public/nsIXMLHttpRequest.idl
@@ +187,2 @@
>     */
> +  DOMString getAllResponseHeaders();

Needs an iid bump

::: content/base/src/nsXMLHttpRequest.cpp
@@ +1320,5 @@
>    }
> +
> +  nsRefPtr<nsHeaderVisitor> visitor = new nsHeaderVisitor();
> +  nsresult rv = httpChannel->VisitResponseHeaders(visitor);
> +  NS_ENSURE_SUCCESS(rv, NS_OK);

This is weird (and will warn when it didn't before... unclear if that's good or bad). How about:

  if (NS_SUCCEEDED(httpChannel->VisitResponseHeaders(visitor))) {
    aResponseHeaders = NS_ConvertUTF8toUTF16(visitor->Headers());
  }

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Will do.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/be00940ec645