232493 - xmlextras cleanup to make DOM LS easier to implement.

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[Johnny Stenback (:jst)]

I've been prototyping some DOM LS work in extensions/xmlextras/ls and I need to
make some changes to our xmlextras code (specifically base URI handling in
nsXMLHttpRequest when not called directly from script), and there were some old
lame string handling (bad string APIs) that I cleaned up while I was at it too,
and some other general cleanup.

Comment 1

[Johnny Stenback (:jst)]

Attachment: Clean things up in xmlextras.

Comment 2

[Peter Van der Beken [:peterv]]

> Index: extensions/p3p/src/nsPolicyReference.cpp
> ===================================================================

> @@ -234,22 +234,24 @@ nsPolicyReference::Load(const char* aURI
>      mXMLHttpRequest = do_CreateInstance(NS_XMLHTTPREQUEST_CONTRACTID, &result);
>      NS_ENSURE_SUCCESS(result, result); 
>      
>      nsCOMPtr<nsIDOMEventTarget> target(do_QueryInterface(mXMLHttpRequest,
&result));
>      NS_ENSURE_SUCCESS(result, result);
>  
>      target->AddEventListener(NS_LITERAL_STRING("load"), this, PR_FALSE);
>    }
>  
> -  result = mXMLHttpRequest->OpenRequest("GET", aURI, PR_TRUE, nsnull, nsnull);
> +  result = mXMLHttpRequest->OpenRequest(NS_LITERAL_CSTRING("GET"),
> +                                        nsDependentCString(aURI), PR_TRUE,
> +                                        EmptyString(), EmptyString());

You should change nsPolicyReference::Load to take a nsACString, afaict all the
callers could be easily converted. You could then drop the nsDependentCString.

> Index: extensions/webservices/schema/src/nsSchemaLoader.cpp
> ===================================================================

> @@ -850,27 +850,27 @@ nsSchemaLoader::Load(const nsAString& sc

> +  rv = request->OpenRequest(NS_LITERAL_CSTRING("GET"),
> +                            spec,
> +                            PR_FALSE, EmptyString(), EmptyString());

Rewrap and maybe use a local emptyString variable.

> @@ -906,27 +906,27 @@ nsSchemaLoader::LoadAsync(const nsAStrin

> +  rv = request->OpenRequest(NS_LITERAL_CSTRING("GET"),
> +                            spec,
> +                            PR_TRUE, EmptyString(), EmptyString());

Same here.

> Index: extensions/webservices/security/src/nsWebScriptsAccess.cpp
> ===================================================================
> RCS file:
/cvsroot/mozilla/extensions/webservices/security/src/nsWebScriptsAccess.cpp,v
> retrieving revision 1.14
> diff -u -9 -p -r1.14 nsWebScriptsAccess.cpp
> --- extensions/webservices/security/src/nsWebScriptsAccess.cpp	23 Jan 2004
13:31:05 -0000	1.14
> +++ extensions/webservices/security/src/nsWebScriptsAccess.cpp	29 Jan 2004
03:22:20 -0000
> @@ -213,22 +213,24 @@ nsWebScriptsAccess::GetDocument(const ch
>                                  nsIDOMDocument** aDocument)
>  {
>    nsresult rv = NS_OK;
>    
>    if (!mRequest) {
>      mRequest = do_CreateInstance(NS_XMLHTTPREQUEST_CONTRACTID, &rv);
>      NS_ENSURE_SUCCESS(rv, rv);
>    }
>   
> -  rv = mRequest->OpenRequest("GET", aDeclFilePath, PR_FALSE, nsnull, nsnull);
> +  rv = mRequest->OpenRequest(NS_LITERAL_CSTRING("GET"),
> +                             nsDependentCString(aDeclFilePath), PR_FALSE,
> +                             EmptyString(), EmptyString());

Make nsWebScriptsAccess::GetDocument take a nsACString, the one caller does a
.get() now.

> Index: extensions/webservices/soap/src/nsHTTPSOAPTransport.cpp
> ===================================================================
> @@ -68,19 +68,19 @@ nsHTTPSOAPTransport::~nsHTTPSOAPTranspor

>  #define DEBUG_DUMP_DOCUMENT(message,doc) \
>    { \
>            nsresult rcc;\
>            nsXPIDLString serial;\
>            nsCOMPtr<nsIDOMSerializer>
serializer(do_CreateInstance(NS_XMLSERIALIZER_CONTRACTID, &rcc));\
>            if (NS_FAILED(rcc)) return rcc;  \
> -          rcc = serializer->SerializeToString(doc, getter_Copies(serial));\
> +          rcc = serializer->SerializeToString(doc, serial);\
>            if (NS_FAILED(rcc)) return rcc;\
>            nsAutoString result(serial);\

Can't you make serial a nsAutoString now and drop result?

> @@ -335,41 +335,42 @@ NS_IMETHODIMP nsHTTPSOAPTransport::SyncC

> -  rv = request->OpenRequest("POST", NS_ConvertUCS2toUTF8(uri).get(),
> -                            PR_FALSE, nsnull, nsnull);
> +  rv = request->OpenRequest(NS_LITERAL_CSTRING("POST"),
> +                            NS_ConvertUCS2toUTF8(uri),
> +                            PR_FALSE, EmptyString(), EmptyString());

Pfff, webservices needs a string useage cleanup. Why is uri not a UTF8 string in
the first place? While you're touching this, make that NS_ConvertUTF16toUTF8 and
maybe an emptyString local.

> +    rv = request->SetRequestHeader(NS_LITERAL_CSTRING("SOAPAction"),
> +                                   NS_ConvertUCS2toUTF8(action));

Sigh, another uri that should be UTF8 in the first place. NS_ConvertUTF16toUTF8.

> +  rv = request->OpenRequest(NS_LITERAL_CSTRING("POST"),
> +                            NS_ConvertUCS2toUTF8(uri),
> +                            PR_TRUE, EmptyString(), EmptyString());

Sigh. NS_ConvertUTF16toUTF8 and emptyString local?

> +    rv = request->SetRequestHeader(NS_LITERAL_CSTRING("SOAPAction"),
> +                                   NS_ConvertUCS2toUTF8(action));

... <speechless>. NS_ConvertUTF16toUTF8.

> Index: extensions/webservices/wsdl/src/nsWSDLLoader.cpp
> ===================================================================

> @@ -337,26 +337,27 @@ nsWSDLLoadRequest::LoadDefinition(const 

> +  rv = mRequest->OpenRequest(NS_LITERAL_CSTRING("GET"),
> +                             NS_ConvertUCS2toUTF8(aURI), !mIsSync,
> +                             EmptyString(), EmptyString());

NS_ConvertUTF16toUTF8 and emptyString local?

> Index: extensions/xmlextras/base/src/nsDOMSerializer.cpp
> ===================================================================

> @@ -67,67 +67,71 @@ NS_INTERFACE_MAP_BEGIN(nsDOMSerializer)

> +static nsresult
> +SetUpEncoder(nsIDOMNode *aRoot, const nsACString& aCharset,
> +             nsIDocumentEncoder **aEncoder)

> -  nsCAutoString charset;
> -  if (aCharset) {
> -    charset = aCharset;
> -  } else {
> +  nsCAutoString charset(aCharset);
> +  if (charset.IsEmpty()) {
>      charset = document->GetDocumentCharacterSet();
>    }
>    rv = encoder->SetCharset(charset);

if (aCharset.IsEmpty) {
    rv = encoder->SetCharset(aCharset);
}
else {
    rv = encoder->SetCharset(document->GetDocumentCharacterSet());
}

I think (aCharset.IsEmpty ? ... : ...) might not work because of the different
string types.

> Index: extensions/xmlextras/base/src/nsXMLHttpRequest.cpp
> ===================================================================

>  NS_IMETHODIMP 
> -nsXMLHttpRequest::GetResponseHeader(const char *header, char **_retval)
> +nsXMLHttpRequest::GetResponseHeader(const nsACString& header,
> +                                    nsACString& _retval)
>  {
> -  NS_ENSURE_ARG(header);
> -  NS_ENSURE_ARG_POINTER(_retval);
>    nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(mChannel));
>  
> -  *_retval = nsnull;
> +  _retval.Truncate();
>    if (httpChannel) {
> -    nsCAutoString buf;
> -    nsresult rv = httpChannel->GetResponseHeader(nsDependentCString(header),
buf);
> +    nsresult rv = httpChannel->GetResponseHeader(header, _retval);
>      if (NS_FAILED(rv)) return rv;
> -    *_retval = ToNewCString(buf);
> -    return *_retval ? NS_OK : NS_ERROR_OUT_OF_MEMORY;
>    }
>    
>    return NS_OK;

Just return rv at the end and drop the line |if (NS_FAILED(rv)) return rv;|


> @@ -678,27 +659,27 @@ nsXMLHttpRequest::OpenRequest(const char

> +    CopyUTF16toUTF8(user, userpass);
> +    if (!password.IsEmpty()) {
>        userpass.Append(":");

Make that Append(':');

> @@ -775,35 +753,35 @@ nsXMLHttpRequest::Open(const char *metho

> +          user.Assign((jschar *)::JS_GetStringChars(userStr),
> +                      ::JS_GetStringLength(userStr));

Why the cast? Or did you want to cast to PRUnichar*?

>          }
>  
>          if (argc > 4) {
> -          JSString* passwordStr;
> +          JSString* passwordStr = JS_ValueToString(cx, argv[4]);
>  
> -          passwordStr = JS_ValueToString(cx, argv[4]);
>            if (passwordStr) {
> -            password = JS_GetStringBytes(passwordStr);
> +            password.Assign((jschar *)::JS_GetStringBytes(passwordStr),
> +                            ::JS_GetStringLength(passwordStr));

And here? JS_GetStringBytes returns char*

> @@ -1261,60 +1240,62 @@ nsXMLHttpRequest::Send(nsIVariant *aBody

> +  nsIURI *baseURI = GetBaseURI();
> +
> +  if (baseURI) {
> +    nsCOMPtr<nsIPrivateDOMImplementation> privImpl =
> +      do_QueryInterface(implementation);
>      if (privImpl) {
> -      privImpl->Init(mBaseURI);
> +      privImpl->Init(baseURI);

You could just do |privImpl->Init(GetBaseURI());| I think.

> Index: extensions/xmlextras/tests/TestXMLExtras.cpp
> ===================================================================

> @@ -206,22 +206,22 @@ int main (int argc, char* argv[]) 

> -        rv = pXMLHttpRequest->OpenRequest( "GET",
> -                                           argv[2],
> +        rv = pXMLHttpRequest->OpenRequest( NS_LITERAL_CSTRING("GET"),
> +                                           nsDependentCString(argv[2]),
>                                             PR_FALSE,
> -                                           nsnull, nsnull );
> +                                           EmptyString(), EmptyString() );

emptyString local?

Comment 3

[Johnny Stenback (:jst)]

Attachment: Updated diff

Comment 4

[Peter Van der Beken [:peterv]]

Comment on attachment 142862 [details] [diff] [review]
Updated diff

>Index: extensions/xmlextras/base/public/nsIXMLHttpRequest.idl
>===================================================================

>@@ -136,19 +137,19 @@ interface nsIXMLHttpRequest : nsISupport

>+  ACString getResponseHeader(in AUTF8String header);

This getter returns ACString

>@@ -218,19 +219,19 @@ interface nsIXMLHttpRequest : nsISupport

>+  void   setRequestHeader(in AUTF8String header, in AUTF8String value);  

but setting takes AUTF8String?
Remove the trailing spaces.

>Index: extensions/xmlextras/base/src/nsXMLHttpRequest.cpp
>===================================================================

>@@ -600,59 +585,56 @@ nsXMLHttpRequest::GetLoadGroup(nsILoadGr

>+nsIURI *
>+nsXMLHttpRequest::GetBaseURI()
> {
>-  NS_ENSURE_ARG_POINTER(aBaseURI);
>-  *aBaseURI = nsnull;
>-
>   if (!mScriptContext) {
>     mScriptContext = GetCurrentContext();
>     if (!mScriptContext) {
>-      return NS_OK;
>+      return nsnull;
>     }
>   }
> 
>   nsCOMPtr<nsIDocument> doc = GetDocumentFromScriptContext(mScriptContext);
>-  if (doc) {
>-    NS_IF_ADDREF(*aBaseURI = doc->GetBaseURI());
>+  if (!doc) {
>+    nsnull;

|return nsnull;|?

>@@ -768,35 +747,35 @@ nsXMLHttpRequest::Open(const char *metho

>       if (argc > 3) {
>-        JSString* userStr;
>+        JSString* userStr = ::JS_ValueToString(cx, argv[3]);
> 
>-        userStr = JS_ValueToString(cx, argv[3]);
>         if (userStr) {
>-          user = JS_GetStringBytes(userStr);
>+          user.Assign((PRUnichar *)::JS_GetStringChars(userStr),

NS_REINTERPRET_CAST?

>+                      ::JS_GetStringLength(userStr));
>         }
> 
>         if (argc > 4) {
>-          JSString* passwordStr;
>+          JSString* passwordStr = JS_ValueToString(cx, argv[4]);
> 
>-          passwordStr = JS_ValueToString(cx, argv[4]);
>           if (passwordStr) {
>-            password = JS_GetStringBytes(passwordStr);
>+            password.Assign((PRUnichar *)::JS_GetStringChars(passwordStr),

NS_REINTERPRET_CAST?

Comment 5

[Johnny Stenback (:jst)]

Fix checked in.