249366 - Better error handling for webservices

Status: RESOLVED FIXED

(Core Graveyard :: Web Services, enhancement)

Description

[Cédric Chantepie]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7) Gecko/20040614 Firefox/0.9
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7) Gecko/20040614 Firefox/0.9

It would be very usefull to be able to get more precise error from webservice
about what make it fails to load WSDL or schema or why the SOAP response is wrong.

Reproducible: Always
Steps to Reproduce:
1. Use webservices from JS for exemple
2. There is a error when creating proxy
3. You get very generic and not very usefull error message in JS onError

Actual Results:  
Failure loading WSDL

Expected Results:  
Failure loading WSDL, cannot process type "XXX"

Comment 1

[Cédric Chantepie]

Attachment: Error handling proposal

Allow to handle more precisly error about WSDL and Schema, should do something
similar about proxy to be able to handle error when processing SOAP response.

Comment 2

[Cédric Chantepie]

Attachment: Error handling proposal

Handles errors from WSDL or Schema processing but not from SOAP response
processing

Comment 3

[Cédric Chantepie]

Comment on attachment 152175 [details] [diff] [review]
Error handling proposal

Should add some error handling when parsing SOAP response

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 152175 [details] [diff] [review]
Error handling proposal

- In interface nsISchemaComponent:

-  void resolve();
+  //XXXTelemac:void resolve();
+  void resolve(in nsIWebserviceErrorHandler errorHandler);

I've been looking through this change, and I like the changes in general, but
comments like the above need to be removed. If you're changing the signature of
a method, just change it, no need to leave the old signature around in a
comment, and especially not an XXX comment (those are generally used in Mozilla
code as a reminder for people to go back and look at a piece of code later for
some reason, not just to indicate who changed what). And if you're adding a
block of code, no need to add XXXTelemac before and after your changes. We've
got CVS and bonsai to keep track of who wrote what code. No need to clutter our
code with comments like these.

So, I'd suggest you go through this diff and remove all comments like the above
one, and if you're adding an informational comment, just add the comment, and
add your name in it if you want to, but don't add XXX to it...

r- for now, I'll gladly re-review this when these comments have been cleaned
up.

Comment 5

[Cédric Chantepie]

Attachment: Error handling proposal (without invalid comments)

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 152764 [details] [diff] [review]
Error handling proposal (without invalid comments)

- In extensions/webservices/proxy/src/wspproxy.cpp:

+/**
+ * XXXTelemac Asynchronous processing :
+ * 1-> WSPProxy::CallMethod
+ * 2  -> WSPCallContext::CallAsync
...

Loose the XXX in the comment there...

- In extensions/webservices/schema/src/nsSchemaLoader.cpp:

 class LoadListener : public nsIDOMEventListener {
-public:
+ public:

This doesn't seem to be consistent with how the majority of this code is
styled. Undo this change.

-NS_IMETHODIMP 
+  NS_IMETHODIMP 
 LoadListener::HandleEvent(nsIDOMEvent *event)

And same here (there's more than one occurance of this), don't indent the
return type.

- In nsSchemaLoader::GetType():

+    if (NS_FAILED(rv)) {
+      return rv;
+    }
+    else {
+      return NS_OK;
+    }

else-after-return. Remove the else.

...

+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+  else {
+    return NS_OK;
+  }

Same here.

+/**
+ * XXXTelemac
+ * @param aErrorHandler Error handler (in).
+ * @param aSchema Schema in which is the element, not null (in).
+ * @param aElement DOM element <element .../>, not null (in).
+ * @param aSchemaElement Schema element from processing DOM element (out).
+ */
 nsresult 
-nsSchemaLoader::ProcessElement(nsSchema* aSchema, 
...

Remove the XXX

- In extensions/webservices/wsdl/src/nsWSDLLoader.cpp:

 #include "nsIServiceManager.h"
 #include "nsIComponentManager.h"
+//#include "nsIProxyObjectManager.h"

Remove the commented out #include

+//static NS_DEFINE_IID(kIWebserviceErrorHandlerIID,
NS_IWEBSERVICEERRORHANDLER_IID);

And this too.

-#define SET_AND_CHECK_ATOM(_atom, _val)   \
-  PR_BEGIN_MACRO			   \
-    _atom = NS_NewAtom(_val);		   \
-    if (!_atom)			   \
-      return NS_ERROR_OUT_OF_MEMORY;	   \
+#define SET_AND_CHECK_ATOM(_atom, _val)	 \
+  PR_BEGIN_MACRO				 \
+  _atom = NS_NewAtom(_val);			 \
+  if (!_atom)					 \
+    return NS_ERROR_OUT_OF_MEMORY;		 \
   PR_END_MACRO

I don't see any code change there. Undo this.

-nsresult
+  nsresult
 nsWSDLLoader::Init()

Don't indent the return type.

-static inline PRBool
+  static inline PRBool
 IsElementOfNamespace(nsIDOMElement* aElement, const nsAString& aNamespace)

Same thing.

- In nsWSDLLoadRequest::HandleError():

+  if (mErrorHandler) {
+    mErrorHandler->OnError(status, statusMessage);
+
+    return NS_OK;
+  } // end of if
+
+  else {

else-after-return, loose the else...

sr=jst with those changes, over to keeda for r=.

Comment 7

[Cédric Chantepie]

Attachment: Invalid

Comment 8

[Cédric Chantepie]

Attachment: Proposal changed according review

Comment 9

[Cédric Chantepie]

Attachment: Corrected proposal

Maid from aviary 2004/07/16

Comment 10

[Harshal Pradhan]

Comment on attachment 153348 [details] [diff] [review]
Corrected proposal

First of all, thanks for doing this. Something like this is badly
needed and overall I like the changes. 

I am -ing this patch primarily because:

1) It needs quite a lot of changes to get it to compile on VC++
   because the string API usage is broken. (explained below.)
2) This kind of stuff needs to be against trunk. As far as I can see
   this is going to need a bit of work to do that; the patch currently
   rejects badly against trunk. You can of course try to get the aviary
   folks to consider a backport after this has been tested on trunk for
   some length of time.

There are other issues detailed below that I would like to be addressed, but
they
are relatively minor. 

> Index: extensions/webservices/public/nsISchema.idl
> ===================================================================
> RCS file: /cvsroot/mozilla/extensions/webservices/public/nsISchema.idl,v
> retrieving revision 1.8
> diff -u -8 -p -r1.8 nsISchema.idl
> --- extensions/webservices/public/nsISchema.idl	27 Feb 2003 22:36:37 -0000	1.8
> +++ extensions/webservices/public/nsISchema.idl	16 Jul 2004 00:06:20 -0000
> @@ -17,16 +17,17 @@
>   * Copyright (C) 2001 by Netscape Communications.  All
>   * Rights Reserved.
>   * 
>   * Contributor(s): 
>   *   Vidur Apparao <[e-mail]> (original author)
>   */
>  
>  #include "nsISupports.idl"
> +#include "nsIWebScriptsAccessService.idl"
>  
>  %{ C++
>  #include "nsAString.h"
>  %} 
>  
> 

While you are touching these idl files, could you please remove the C++
nsAString
includes like these. They are left over from the time AString was not an idl
type
I think. They are certainly not necessary now.


> Index: extensions/webservices/public/nsISchemaLoader.idl
> ===================================================================
> RCS file: /cvsroot/mozilla/extensions/webservices/public/nsISchemaLoader.idl,v
> retrieving revision 1.4
> diff -u -8 -p -r1.4 nsISchemaLoader.idl
> --- extensions/webservices/public/nsISchemaLoader.idl	6 Feb 2002 22:06:05 -0000	1.4
> +++ extensions/webservices/public/nsISchemaLoader.idl	16 Jul 2004 00:06:20 -0000
> @@ -17,35 +17,35 @@
>   * Copyright (C) 2001 by Netscape Communications.  All
>   * Rights Reserved.
>   * 
>   * Contributor(s): 
>   *   Vidur Apparao <[e-mail]> (original author)
>   */
>  
>  #include "nsISupports.idl"
> +#include "nsIWebScriptsAccessService.idl"
>  
>  interface nsISchema;
>  interface nsISchemaType;
>  interface nsIDOMElement;
>  interface nsISchemaLoadListener;
>  
>  [scriptable, uuid(3c14a032-6f4e-11d5-9b46-000064657374)]
>  interface nsISchemaLoader : nsISupports {
>    nsISchema load(in AString schemaURI);
>    void loadAsync(in AString schemaURI, 
>                   in nsISchemaLoadListener listener);
> -  nsISchema processSchemaElement(in nsIDOMElement element);
> +  nsISchema processSchemaElement(in nsIWebserviceErrorHandler errorHandler,
> +                                 in nsIDOMElement element);
>  };
>  

Please change use a new uuid for the nsISchemaLoader interface.
Any interface that change in a binary-incompatible way from
previous releases needs to have its uuid revved. This applies to
all other places where you have added parameters to old methods.


>  [scriptable, function, uuid(3c14a033-6f4e-11d5-9b46-000064657374)]
> -interface nsISchemaLoadListener : nsISupports {
> +interface nsISchemaLoadListener : nsIWebserviceErrorHandler {
>    void onLoad(in nsISchema schema);
> -  void onError(in PRInt32 status,
> -               in AString statusMessage);
>  };
>  

Well, this change will not really change the vtable, so I think
this is ok.

>  %{ C++
>  #define NS_SCHEMALOADER_CID                           \
>  { /* 5adc10f0-74e1-11d5-9b49-00104bdf5339 */          \
>   0x5adc10f0, 0x74e1, 0x11d5,                          \
>  {0x9b, 0x49, 0x00, 0x10, 0x4b, 0xdf, 0x53, 0x39}}     
>  
> Index: extensions/webservices/public/nsIWSDLLoader.idl
> ===================================================================
> RCS file: /cvsroot/mozilla/extensions/webservices/public/nsIWSDLLoader.idl,v
> retrieving revision 1.10
> diff -u -8 -p -r1.10 nsIWSDLLoader.idl
> --- extensions/webservices/public/nsIWSDLLoader.idl	18 Apr 2003 20:58:42 -0000	1.10
> +++ extensions/webservices/public/nsIWSDLLoader.idl	16 Jul 2004 00:06:20 -0000
> @@ -17,16 +17,17 @@
>   * Copyright (C) 2001 by Netscape Communications.  All
>   * Rights Reserved.
>   *
>   * Contributor(s):
>   *   Vidur Apparao <[e-mail]> (original author)
>   */
>  
>  #include "nsISupports.idl"
> +#include "nsIWebScriptsAccessService.idl"
>  
>  %{ C++
>  #include "nsAString.h"
>  %}

Remove this.

> Index: extensions/webservices/public/nsIWebScriptsAccessService.idl
> ===================================================================
> RCS file: /cvsroot/mozilla/extensions/webservices/public/nsIWebScriptsAccessService.idl,v
> retrieving revision 1.1
> diff -u -8 -p -r1.1 nsIWebScriptsAccessService.idl
> --- extensions/webservices/public/nsIWebScriptsAccessService.idl	22 Mar 2003 01:44:36 -0000	1.1
> +++ extensions/webservices/public/nsIWebScriptsAccessService.idl	16 Jul 2004 00:06:21 -0000
> @@ -50,16 +50,22 @@ interface nsIWebScriptsAccessService : n
>    boolean canAccess(in nsIURI aTransportURI, in AString aType);
>    /**
>     * This method will invalidate the cached entry for the transport uri.
>     * Also one can clear the _entire_ cache by passing in a null string.
>     */ 
>    void invalidateCache(in string aTransportURI);
>  };
>  
> +[scriptable, uuid(57E2860C-4266-4a85-BFDE-AE39D945B014)]
> +interface nsIWebserviceErrorHandler : nsISupports
> +{
> +  void onError(in nsresult status, in AString statusMessage);
> +};
> +

This interface does not really belong in this file. It is not even used
by the webscripts access service. I think it is best if you create
a new idl file for it and include the corresponding header directly
instead of including nsIWebScriptsAccessService.h in various places.

> Index: extensions/webservices/schema/src/nsSchema.cpp
> ===================================================================
> RCS file: /cvsroot/mozilla/extensions/webservices/schema/src/nsSchema.cpp,v
> retrieving revision 1.9
> diff -u -8 -p -r1.9 nsSchema.cpp
> --- extensions/webservices/schema/src/nsSchema.cpp	29 Jan 2004 22:04:31 -0000	1.9
> +++ extensions/webservices/schema/src/nsSchema.cpp	16 Jul 2004 00:06:23 -0000
> @@ -71,79 +71,131 @@ nsSchema::GetTargetNamespace(nsAString& 
>  /* readonly attribute wstring schemaNamespace; */
>  NS_IMETHODIMP 
>  nsSchema::GetSchemaNamespace(nsAString& aSchemaNamespace)
>  {
>    aSchemaNamespace.Assign(mSchemaNamespace);
>    return NS_OK;
>  }
>  
> -/* void resolve (); */
> +/* void resolve(in nsIWebserviceErrorHandler errorHandler); */
>  NS_IMETHODIMP 
> -nsSchema::Resolve()
> +nsSchema::Resolve(nsIWebserviceErrorHandler* aErrorHandler)
>  {
>    nsresult rv;
>    PRUint32 i, count;
>  
>    mTypes.Count(&count);
>    for (i = 0; i < count; i++) {
>      nsCOMPtr<nsISchemaType> type;
>      
>      rv = mTypes.QueryElementAt(i, NS_GET_IID(nsISchemaType),
>                                 getter_AddRefs(type));
>      if (NS_SUCCEEDED(rv)) {
> -      rv = type->Resolve();
> +      rv = type->Resolve(aErrorHandler);
> +      if (NS_FAILED(rv)) {
> +        nsAutoString name;
> +        nsresult res = type->GetName(name);
> +        NS_ENSURE_SUCCESS(res, res);
> +
> +        NS_NAMED_LITERAL_STRING(errorMsg, "Failure resolving schema, "
> +                                "cannot resolve schema type \"");
> +        errorMsg.Append(name);
> +        errorMsg.Append(NS_LITERAL_STRING("\""));
> +
> +        NS_SCHEMALOADER_FIRE_ERROR(rv, errorMsg);
> +      }
>        NS_ENSURE_SUCCESS(rv, rv);
>      }
>    }
>  

This will break on platforms that define HAVE_CPP_2BYTE_WCHAR_T see
http://lxr.mozilla.org/mozilla/source/xpcom/string/public/nsLiteralString.h#81
This includes all supported versions of VC++. A literal string is
supposed to be used as a const and should not be appended to.

You are better off using a concatenation like this:
	nsAutoString errorMsg (NS_LITERAL_STRING("Failure resolving schema") +
			       NS_LITERAL_STRING("cannot resolve schema type
\"") +
			       name +
			       NS_LITERAL_STRING("\"");


Or you could use the newer non-bloaty AppendLiteral API's like this:  
	nsAutoString errorMsg;
	errorMsg.AppendLiteral("Failure resolving schema");
	errorMsg.AppendLiteral("cannot resolve schema type \"");
	errorMsg.Append(name);
	errorMsg.AppendLiteral("\"");

I think I would prefer the latter.

Also, typically rv, rv1, rv2 ... are used in newer code. Not res.
But this is just a minor nit.

There are lots of lots of places in the patch which have this problem
and they will all need to be changed. There are other place where you
use Append(NS_LITERAL_STRING("foo")) where you could use
AppendLiteral("foo").

Also, I am slightly concerned about the bloat that this will cause.
Since this pattern is used over and over again and will add a
significant addition of codesize. Maybe you can make the messages a
bit more standardized in format and then add a method like you added
in the wsdl code for handling errors.

static ReportSchemaError(nsIWebserviceErrorHandler* aErrorHandler,
			 nsAString& msg, nsAString& typeName,
			 nsAString& extraMessage = EmptyString())
{
....
}

That would save quite a bit of code I think, so please consider
if that would be possible. (Maybe it could become a static protected
method on nsSchemaComponentBase so that it could be used from 
different files/classes.)


> Index: extensions/webservices/schema/src/nsSchemaLoader.cpp
> ===================================================================
> RCS file: /cvsroot/mozilla/extensions/webservices/schema/src/nsSchemaLoader.cpp,v
> retrieving revision 1.25
> diff -u -8 -p -r1.25 nsSchemaLoader.cpp
> --- extensions/webservices/schema/src/nsSchemaLoader.cpp	8 Mar 2004 23:52:25 -0000	1.25
> +++ extensions/webservices/schema/src/nsSchemaLoader.cpp	16 Jul 2004 00:06:37 -0000
> @@ -18,16 +18,17 @@
>   * Rights Reserved.
>   * 
>   * Contributor(s): 
>   *   Vidur Apparao <[e-mail]> (original author)
>   */
>  
>  #include "nsSchemaPrivate.h"
>  #include "nsSchemaLoader.h"
> +#include "nsIWebScriptsAccessService.h"
>  
>  // content includes
>  #include "nsIContent.h"
>  #include "nsINodeInfo.h"
>  #include "nsIDOMDocument.h"
>  #include "nsIDOM3Node.h"
>  
>  // loading includes
> @@ -356,29 +357,26 @@ LoadListener::HandleEvent(nsIDOMEvent *e
>      rv = mRequest->GetResponseXML(getter_AddRefs(document));
>      if (NS_SUCCEEDED(rv)) {
>        nsCOMPtr<nsIDOMElement> element;
>        
>        if (document)
>          document->GetDocumentElement(getter_AddRefs(element));
>  
>        if (element) {
> -        rv = mLoader->ProcessSchemaElement(element, getter_AddRefs(schema));
> +        rv = mLoader->ProcessSchemaElement(nsnull, element, getter_AddRefs(schema)); //Todo: Have an error handler there instead or nsnull
>        }
>        else {
>          rv = NS_ERROR_SCHEMA_NOT_SCHEMA_ELEMENT;
>        }
>      }
>  
>      if (NS_SUCCEEDED(rv)) {
>        mListener->OnLoad(schema);
>      }
> -    else {
> -      mListener->OnError(rv, NS_LITERAL_STRING("Failure processing schema document"));
> -    }
>    }


If I understand this correctly, you are completely removing existing
error reporting in this case without replacing it with the newer
method.  This is bad. This OnError() call should remain till you are
done dealing with the Todo above.

[snip]


>  
> @@ -954,112 +987,141 @@ nsSchemaLoader::LoadAsync(const nsAStrin
>    
>    return rv;
>  }
>  
>  static const char* kSchemaNamespaces[] = {NS_SCHEMA_1999_NAMESPACE, 
>                                            NS_SCHEMA_2001_NAMESPACE};
>  static PRUint32 kSchemaNamespacesLength = sizeof(kSchemaNamespaces) / sizeof(const char*);
>  
> -/* nsISchema processSchemaElement (in nsIDOMElement element); */
> +/*
> +  nsISchema processSchemaElement(in nsIWebserviceErrorHandler errorHandler,
> +  in nsIDOMElement element);
> +*/
>  NS_IMETHODIMP 
> -nsSchemaLoader::ProcessSchemaElement(nsIDOMElement* aElement, 
> +nsSchemaLoader::ProcessSchemaElement(nsIWebserviceErrorHandler* aErrorHandler,
> +                                     nsIDOMElement* aElement,
>                                       nsISchema **_retval)

The changes in this method are from the other crasher bug. Right?
Its ok to carry them in this patch since its difficult to separate
them out. But it would be even better to get these checked in first
since it already has r+sr, and there is no real reason to clutter
up this patch.


> Index: extensions/webservices/schema/src/nsSchemaLoader.h
> ===================================================================
> RCS file: /cvsroot/mozilla/extensions/webservices/schema/src/nsSchemaLoader.h,v
> retrieving revision 1.8
> diff -u -8 -p -r1.8 nsSchemaLoader.h
> --- extensions/webservices/schema/src/nsSchemaLoader.h	23 Mar 2002 21:13:57 -0000	1.8
> +++ extensions/webservices/schema/src/nsSchemaLoader.h	16 Jul 2004 00:06:38 -0000
> @@ -157,83 +158,102 @@ public:
>    nsSchemaLoader();
>    virtual ~nsSchemaLoader();
>  
>    NS_DECL_ISUPPORTS
>    NS_DECL_NSISCHEMALOADER
>    NS_DECL_NSISCHEMACOLLECTION
>  
>  protected:
> -  nsresult ProcessElement(nsSchema* aSchema, 
> +  nsresult HandleError(nsresult status, const nsAString & statusMessage);
> +


This method is not defined/used in this class right? Get rid of the
declaration.

[snip]

> @@ -261,11 +281,13 @@ protected:
>                             nsISchemaType** aArrayType,
>                             PRUint32* aDimension);
>    void ConstructArrayName(nsISchemaType* aType,
>                            nsAString& aName);
>  
>  protected:
>    nsSupportsHashtable mSchemas;
>    nsCOMPtr<nsISchemaCollection> mBuiltinCollection;
> +
> +  nsSupportsHashtable mSchemaPendingErrors;
>  };
>  


I dont see this hashtable being used anywhere. Probably
a left over from some old code? Please remove it.



> Index: extensions/webservices/wsdl/src/nsWSDLLoader.h
> ===================================================================
> RCS file: /cvsroot/mozilla/extensions/webservices/wsdl/src/nsWSDLLoader.h,v
> retrieving revision 1.7
> diff -u -8 -p -r1.7 nsWSDLLoader.h
> --- extensions/webservices/wsdl/src/nsWSDLLoader.h	18 Apr 2003 20:58:44 -0000	1.7
> +++ extensions/webservices/wsdl/src/nsWSDLLoader.h	16 Jul 2004 00:06:48 -0000

> @@ -158,16 +159,18 @@ class nsWSDLLoadRequest : public nsIDOME
>  public:
>    nsWSDLLoadRequest(PRBool aIsSync, nsIWSDLLoadListener* aListener,
>                      const nsAString& aPortName);
>    virtual ~nsWSDLLoadRequest();
>  
>    NS_DECL_ISUPPORTS
>    NS_DECL_NSIDOMEVENTLISTENER
>  
> +  nsresult HandleError(nsresult status, const nsAString & statusMessage);
> +



This method is used but is not part of the patch somehow. Probably got left
out in hand-editing?



>    nsresult LoadDefinition(const nsAString& aURI);
>    nsresult ResumeProcessing();
>    nsresult ContineProcessingTillDone();
>    nsresult GetPort(nsIWSDLPort** aPort);
>  
>    nsresult PushContext(nsIDOMDocument* aDocument, const nsAString& aLocation);
>    nsWSDLLoadingContext* GetCurrentContext();
>    void PopContext();
> @@ -201,16 +204,17 @@ public:
>    nsresult ProcessServiceElement(nsIDOMElement* aElement);
>    
>  protected:
>    nsCOMPtr<nsIWSDLLoadListener> mListener;
>    nsCOMPtr<nsIXMLHttpRequest> mRequest;
>    nsCOMPtr<nsISchemaLoader> mSchemaLoader;
>    nsCOMPtr<nsIWSDLPort> mPort;
>    nsCOMArray<nsIURI> mImportList;
> +  nsCOMPtr<nsIWebserviceErrorHandler> mErrorHandler;
>    


You don't need a new member since nsIWSDLLoadListener inherits
from nsIWSDLLoadListener right? You can just use the same
poiter to call OnError().

Comment 11

[Cédric Chantepie]

Attachment: Patch proposal for the trunk

Comment 12

[Harshal Pradhan]

Compiling with the current patch with VC7 fails with the following error. 

nsSchema.cpp
d:/trunk\mozilla\extensions\webservices\schema\src\nsSchema.cpp(176) : error C23
08: concatenating mismatched wide strings
d:/trunk\mozilla\extensions\webservices\schema\src\nsSchema.cpp(176) : error C23
08: concatenating mismatched wide strings
d:/trunk\mozilla\extensions\webservices\schema\src\nsSchema.cpp(188) : error C23
08: concatenating mismatched wide strings
d:/trunk\mozilla\extensions\webservices\schema\src\nsSchema.cpp(188) : error C23
08: concatenating mismatched wide strings

Basically you can't do 
NS_LITERAL_STRING("Failure resolving schema, "
                  "cannot resolve attribute groups")

.... since wchar is 2-byte, that gets expanded to the L"" form. 


Can you please fix that. I haven't really had time to look at the rest of the
patch. Hopefully, I will get to it early next week. 

Comment 13

[Cédric Chantepie]

Attachment: Patch proposal for the trunk

Comment 14

[Harshal Pradhan]

Comment on attachment 154777 [details] [diff] [review]
Patch proposal for the trunk

The looks good. r=me if you take care of
the following minor problems.

Index: schema/src/nsSchemaLoader.cpp
===================================================================
RCS file:
/cvsroot/mozilla/extensions/webservices/schema/src/nsSchemaLoader.cpp,v
retrieving revision 1.30
diff -u -8 -p -r1.30 nsSchemaLoader.cpp
--- schema/src/nsSchemaLoader.cpp	22 May 2004 22:15:07 -0000	1.30
+++ schema/src/nsSchemaLoader.cpp	30 Jul 2004 16:00:14 -0000

[...]

   nsCOMPtr<nsISchema> schema;
-  nsresult rv = GetSchema(aNamespace, getter_AddRefs(schema));
+  rv = GetSchema(aNamespace, getter_AddRefs(schema));
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+
+  rv = schema->GetTypeByName(aName, _retval);
+
   if (NS_FAILED(rv)) {
+    NS_NAMED_LITERAL_STRING(msg, "nsSchemaLoader::GetType: "
+			     "Failure processing schema: "
+			     "cannot get schema type \"");
+
+    msg.Append(aName);
+    msg.Append(NS_LITERAL_STRING("\""));
+    NS_ERROR(NS_ConvertUTF16toUTF8(msg).get());
+
     return rv;
   }

You missed one spot :) This needs to be fixed up like all
the others as well.

This is the only remaining syntax issue. I checked that
the patch builds with my VC7.1 build with this fixed.

[...]

+  nsAutoString targetNamespace;
+  schema->GetTargetNamespace(targetNamespace);
+  nsStringKey key(targetNamespace);
+  nsCOMPtr<nsISupports> old = getter_AddRefs(mSchemas.Get(&key));
+  nsCOMPtr<nsISchema> os = do_QueryInterface(old);
+  
+  if (os) {
+    *_retval = os;
+    NS_ADDREF(*_retval);
+    
+    return NS_OK;
+  }
+

Please update your tree to the latest trunk before you submit your
final patch. Just do a |cvs update| in the schema directory and clean up
conflicts if any before diffing. These changes from bug 244952 are
already checked in and cause rejects when applying to a clean trunk tree.

Index: wsdl/src/nsWSDLLoader.h
===================================================================
RCS file: /cvsroot/mozilla/extensions/webservices/wsdl/src/nsWSDLLoader.h,v
retrieving revision 1.9
diff -u -8 -p -r1.9 nsWSDLLoader.h
--- wsdl/src/nsWSDLLoader.h	1 May 2004 09:31:27 -0000	1.9
+++ wsdl/src/nsWSDLLoader.h	30 Jul 2004 16:00:28 -0000
[snip]
@@ -197,16 +199,19 @@ public:
   nsresult ProcessServiceElement(nsIDOMElement* aElement);

 protected:
   nsCOMPtr<nsIWSDLLoadListener> mListener;
   nsCOMPtr<nsIXMLHttpRequest> mRequest;
   nsCOMPtr<nsISchemaLoader> mSchemaLoader;
   nsCOMPtr<nsIWSDLPort> mPort;
   nsCOMArray<nsIURI> mImportList;
+  nsCOMPtr<nsIWebServiceErrorHandler> mErrorHandler; // XXXTelemac let it
different
+  //XXXTelemac from mListener so that error could be reported to some else
from
+  //XXXTelemac wsdl load listener


Its ok if you want to keep the pointer, but
I'd prefer if you don't keep the XXX comments.

Comment 15

[Cédric Chantepie]

Attachment: Patch proposal for the trunk

Comment 16

[Cédric Chantepie]

Comment on attachment 155110 [details] [diff] [review]
Patch proposal for the trunk

Updated patch

Comment 17

[Harshal Pradhan]

Attachment: Additional patch that is needed when building tests.

This needs to go in along with the patch above.

Comment 18

[Cédric Chantepie]

Attachment: Patch including changes for test

Comment 19

[Harshal Pradhan]

Comment on attachment 155249 [details] [diff] [review]
Patch including changes for test

Do you want me to go through this again. This is simply the last patch that I
reviewed + test code changes ... right? r=me obviously still applies in that
case. 

Please do point out if there are other changes that you want me to look at
again.

Comment 20

[Johnny Stenback (:jst)]

Comment on attachment 155249 [details] [diff] [review]
Patch including changes for test

sr=jst.

We should probably file a followup bug to make these errors localizeable at
some point though.

Comment 21

[Cédric Chantepie]

(In reply to comment #20)
> We should probably file a followup bug to make these errors localizeable at
> some point though.
> 

An another bug for error handling in soap encoding/decoding

Comment 22

[Darin Fisher]

the license in public/nsIWebServiceErrorHandler.idl claims that the initial
developer is Netscape Communications Corp, and that the code was written in
2002.  copy-paste snafu, right?

Comment 23

[Cédric Chantepie]

Attachment: Patch with licence boilerplate change [trunk]

Comment 24

[Harshal Pradhan]

Comment on attachment 159072 [details] [diff] [review]
Patch with licence boilerplate change [trunk]

I checked this into trunk. It has caused a rather large codesize spike.

Comment 25

[Benjamin Smedberg]

Whoa, that is a dramatic codesize spike. <mvl> it might want to use nsPrintfCString

Where are these errors reported to? Non-localizable messages that can get
displayed to the user (even in the JS console) are a significant problem that
really shouldn't have gotten past superreview (JST!). Unless these errors are
*only* exposed to onerror script handlers... then I'm not sure what the correct
behavior would be.

Comment 26

[Harshal Pradhan]

Well, this code has been in dire need of *any* sort of error handling for ages,
and people are not exactly queing up to work on it. The present patch did make
things somewhat better and I was reluctant to hold it up if jst was ok with it.
Also, Cedric did agree to follow up on localization ....

I had not anticipated the size of codesize spike. I agree that its pretty awful.
Given that, it might just be better to back the whole thing out and let Cedric
fix it properly. If that is the consensus, I'll do the backout tomorrow.

Comment 27

[Darin Fisher]

it seems like it would be fairly straightforward to move all of the message
formatting code into a common subroutine (or possibly a couple subroutines
depending on how you want to handle variable length argument lists).

Comment 28

[Neil Deakin]

-  nsISchema processSchemaElement(in nsIDOMElement element);
+  nsISchema processSchemaElement(in nsIWebServiceErrorHandler aErrorHandler, 
+                                 in nsIDOMElement element);

Is there a reason why the error handler was added as the first argument? Could
these arguments be reversed so as to not intentionally break backwards
compatibility with js code?

Comment 29

[Harshal Pradhan]

Reversing things might be the best thing to do since so far we have only have
had alpha releases with the current interface. Cederic?

Sorry, we did not realize earlier that this would be quite a pain deal with in
js code... or that there actually were any users of this stuff outside the tree ...

Comment 30

[Harshal Pradhan]

Attachment: fix

Comment 31

[Johnny Stenback (:jst)]

Comment on attachment 165774 [details] [diff] [review]
fix

sr=jst

Comment 32

[Mike Kaply [:mkaply]]

Comment on attachment 159072 [details] [diff] [review]
Patch with licence boilerplate change [trunk]

This doesn't look baked enough for 1.7

Comment 33

[Harshal Pradhan]

Attachment: processSchemaElement API fix patch (updated for a bit of bitrot)

Oops ... I have been away from mozilla hacking and completely forgot that this
patch had not landed. In the meantime a couple more uses of
processSchemaElement have crept into the tree in xforms code. I've updated
things to fix those too. I hope its ok to carry over the review from the patch
that this obsoletes.

I don't have a proper build to checkin from right now, so it would be really
nice if someone could check this in for me.

Comment 34

[Cédric Chantepie]

(In reply to comment #33)
> Created an attachment (id=170447) [edit]
> processSchemaElement API fix patch (updated for a bit of bitrot)
> 
> Oops ... I have been away from mozilla hacking and completely forgot that this
> patch had not landed. 

Me too, I've some spare time so if I can help...

Comment 35

[Johnny Stenback (:jst)]

Fix landed on the trunk.