Last Comment Bug 232493 - xmlextras cleanup to make DOM LS easier to implement.
: xmlextras cleanup to make DOM LS easier to implement.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Johnny Stenback (:jst, jst@mozilla.com)
: Ashish Bhatt
Mentors:
Depends on:
Blocks: 230310
  Show dependency treegraph
 
Reported: 2004-01-28 19:23 PST by Johnny Stenback (:jst, jst@mozilla.com)
Modified: 2005-10-08 05:14 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Clean things up in xmlextras. (50.75 KB, patch)
2004-01-28 19:24 PST, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Review
Updated diff (56.64 KB, patch)
2004-03-03 15:18 PST, Johnny Stenback (:jst, jst@mozilla.com)
caillon: review+
peterv: superreview+
Details | Diff | Review

Description Johnny Stenback (:jst, jst@mozilla.com) 2004-01-28 19:23:49 PST
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, jst@mozilla.com) 2004-01-28 19:24:50 PST
Created attachment 140128 [details] [diff] [review]
Clean things up in xmlextras.
Comment 2 Peter Van der Beken [:peterv] 2004-02-01 09:13:08 PST
> 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, jst@mozilla.com) 2004-03-03 15:18:21 PST
Created attachment 142862 [details] [diff] [review]
Updated diff
Comment 4 Peter Van der Beken [:peterv] 2004-03-08 11:15:54 PST
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, jst@mozilla.com) 2004-03-08 15:56:35 PST
Fix checked in.

Note You need to log in before you can comment on or make changes to this bug.