xmlextras cleanup to make DOM LS easier to implement.

RESOLVED FIXED

Status

()

Core
XML
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: jst, Assigned: jst)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

56.64 KB, patch
Christopher Aillon (sabbatical, not receiving bugmail)
: review+
peterv
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

13 years ago
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.
(Assignee)

Comment 1

13 years ago
Created attachment 140128 [details] [diff] [review]
Clean things up in xmlextras.
(Assignee)

Updated

13 years ago
Attachment #140128 - Flags: superreview?(peterv)
Attachment #140128 - Flags: review?(caillon)
> 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?
(Assignee)

Comment 3

13 years ago
Created attachment 142862 [details] [diff] [review]
Updated diff
Attachment #140128 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #140128 - Flags: superreview?(peterv)
Attachment #140128 - Flags: review?(caillon)
(Assignee)

Updated

13 years ago
Attachment #142862 - Flags: superreview?(peterv)
Attachment #142862 - Flags: review?(caillon)
Attachment #142862 - Flags: review?(caillon) → review+
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?
Attachment #142862 - Flags: superreview?(peterv) → superreview+
(Assignee)

Comment 5

13 years ago
Fix checked in.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Blocks: 230310
You need to log in before you can comment on or make changes to this bug.