Better error handling for webservices

RESOLVED FIXED

Status

Core Graveyard
Web Services
--
enhancement
RESOLVED FIXED
14 years ago
6 months ago

People

(Reporter: Cédric Chantepie, Assigned: Cédric Chantepie)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 11 obsolete attachments)

146.38 KB, patch
Harshal Pradhan
: review+
Details | Diff | Splinter Review
146.35 KB, patch
Details | Diff | Splinter Review
9.11 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

14 years ago
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"
(Assignee)

Comment 1

14 years ago
Created attachment 152101 [details] [diff] [review]
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.
(Assignee)

Comment 2

14 years ago
Created attachment 152175 [details] [diff] [review]
Error handling proposal

Handles errors from WSDL or Schema processing but not from SOAP response
processing
Attachment #152101 - Attachment is obsolete: true
(Assignee)

Comment 3

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

Should add some error handling when parsing SOAP response
Attachment #152175 - Flags: review?(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.
Attachment #152175 - Flags: review?(jst) → review-
(Assignee)

Updated

14 years ago
Attachment #152175 - Attachment is obsolete: true
(Assignee)

Comment 5

14 years ago
Created attachment 152764 [details] [diff] [review]
Error handling proposal (without invalid comments)
(Assignee)

Updated

14 years ago
Attachment #152764 - Flags: review?(jst)
(Assignee)

Updated

14 years ago
Blocks: 250777
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=.
Attachment #152764 - Flags: superreview+
Attachment #152764 - Flags: review?(keeda)
Attachment #152764 - Flags: review?(jst)

Updated

14 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 7

14 years ago
Created attachment 153171 [details] [diff] [review]
Invalid
(Assignee)

Updated

14 years ago
Attachment #153171 - Attachment description: Changed according review comment → Invalid
Attachment #153171 - Attachment is obsolete: true
(Assignee)

Comment 8

14 years ago
Created attachment 153176 [details] [diff] [review]
Proposal changed according review
(Assignee)

Updated

14 years ago
Attachment #153176 - Flags: review?(keeda)
(Assignee)

Updated

14 years ago
Attachment #152764 - Flags: review?(keeda)
(Assignee)

Updated

14 years ago
Attachment #153176 - Flags: review?(keeda)
(Assignee)

Comment 9

14 years ago
Created attachment 153348 [details] [diff] [review]
Corrected proposal

Maid from aviary 2004/07/16
Attachment #153176 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #153348 - Flags: review?(keeda)
(Assignee)

Updated

14 years ago
Depends on: 244952

Comment 10

14 years ago
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 <vidur@netscape.com> (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 <vidur@netscape.com> (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 <vidur@netscape.com> (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 <vidur@netscape.com> (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().
Attachment #153348 - Flags: review?(keeda) → review-
(Assignee)

Comment 11

14 years ago
Created attachment 154690 [details] [diff] [review]
Patch proposal for the trunk
Attachment #153348 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #154690 - Flags: review?(keeda)

Comment 12

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

Comment 13

14 years ago
Created attachment 154777 [details] [diff] [review]
Patch proposal for the trunk
Attachment #154690 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #154690 - Flags: review?(keeda)
(Assignee)

Updated

14 years ago
Attachment #154777 - Flags: review?(jst)

Comment 14

14 years ago
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.
Attachment #154777 - Flags: review?(jst) → review+
(Assignee)

Comment 15

14 years ago
Created attachment 155110 [details] [diff] [review]
Patch proposal for the trunk
(Assignee)

Comment 16

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

Updated patch
Attachment #155110 - Flags: review?(keeda)
(Assignee)

Updated

14 years ago
Attachment #152764 - Attachment is obsolete: true

Updated

14 years ago
Attachment #155110 - Flags: superreview?(jst)
Attachment #155110 - Flags: review?(keeda)
Attachment #155110 - Flags: review+

Comment 17

14 years ago
Created attachment 155156 [details] [diff] [review]
Additional patch that is needed when building tests.

This needs to go in along with the patch above.

Updated

14 years ago
Attachment #155156 - Flags: superreview?(jst)
Attachment #155156 - Flags: review?(chantepie)
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

14 years ago
Assignee: web-services → chantepie
Status: ASSIGNED → NEW
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

14 years ago
Attachment #155156 - Flags: review?(chantepie) → review+
(Assignee)

Comment 18

14 years ago
Created attachment 155249 [details] [diff] [review]
Patch including changes for test
(Assignee)

Updated

14 years ago
Attachment #155249 - Flags: review?(keeda)
(Assignee)

Updated

14 years ago
OS: MacOS X → All
Hardware: Macintosh → All
(Assignee)

Updated

14 years ago
Attachment #155156 - Flags: superreview?(jst)
(Assignee)

Updated

14 years ago
Attachment #155110 - Flags: superreview?(jst)
(Assignee)

Updated

14 years ago
Attachment #154777 - Attachment is obsolete: true

Comment 19

14 years ago
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.
Attachment #155249 - Flags: review?(keeda) → review+
(Assignee)

Updated

14 years ago
Attachment #155249 - Flags: superreview?(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.
Attachment #155249 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 21

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

Updated

14 years ago
Attachment #155110 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #155156 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #155249 - Flags: approval1.7.x?

Comment 22

14 years ago
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?
(Assignee)

Updated

14 years ago
Attachment #155249 - Flags: approval1.7.x?
(Assignee)

Comment 23

14 years ago
Created attachment 159072 [details] [diff] [review]
Patch with licence boilerplate change [trunk]
(Assignee)

Updated

14 years ago
Attachment #159072 - Flags: approval1.7.x?

Comment 24

14 years ago
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

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

Updated

14 years ago
Blocks: 259820

Comment 26

14 years ago
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

14 years ago
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

14 years ago
-  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

14 years ago
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 ...

Updated

14 years ago
Attachment #165774 - Flags: review?(chantepie)
(Assignee)

Updated

14 years ago
Attachment #165774 - Flags: review?(chantepie) → review+

Updated

14 years ago
Attachment #165774 - Flags: superreview?(jst)
Comment on attachment 165774 [details] [diff] [review]
fix

sr=jst
Attachment #165774 - Flags: superreview?(jst) → superreview+

Comment 32

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

This doesn't look baked enough for 1.7
Attachment #159072 - Flags: approval1.7.x? → approval1.7.x-

Comment 33

14 years ago
Created attachment 170447 [details] [diff] [review]
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.
Attachment #165774 - Attachment is obsolete: true
Attachment #170447 - Flags: review?(jst)
(Assignee)

Comment 34

14 years ago
(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...
Fix landed on the trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Updated

13 years ago
Attachment #170447 - Flags: review?(jst)

Updated

6 months ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.