Closed
Bug 249366
Opened 20 years ago
Closed 20 years ago
Better error handling for webservices
Categories
(Core Graveyard :: Web Services, enhancement)
Core Graveyard
Web Services
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: chantepie, Assigned: chantepie)
References
Details
Attachments
(3 files, 11 obsolete files)
146.38 KB,
patch
|
keeda
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
146.35 KB,
patch
|
mkaply
:
approval1.7.5-
|
Details | Diff | Splinter Review |
9.11 KB,
patch
|
Details | Diff | Splinter Review |
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•20 years ago
|
||
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•20 years ago
|
||
Handles errors from WSDL or Schema processing but not from SOAP response processing
Attachment #152101 -
Attachment is obsolete: true
Assignee | ||
Comment 3•20 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 4•20 years ago
|
||
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•20 years ago
|
Attachment #152175 -
Attachment is obsolete: true
Assignee | ||
Comment 5•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #152764 -
Flags: review?(jst)
Comment 6•20 years ago
|
||
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•20 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 7•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #153171 -
Attachment description: Changed according review comment → Invalid
Attachment #153171 -
Attachment is obsolete: true
Assignee | ||
Comment 8•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #153176 -
Flags: review?(keeda)
Assignee | ||
Updated•20 years ago
|
Attachment #152764 -
Flags: review?(keeda)
Assignee | ||
Updated•20 years ago
|
Attachment #153176 -
Flags: review?(keeda)
Assignee | ||
Comment 9•20 years ago
|
||
Maid from aviary 2004/07/16
Attachment #153176 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #153348 -
Flags: review?(keeda)
Comment 10•20 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•20 years ago
|
||
Attachment #153348 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #154690 -
Flags: review?(keeda)
Comment 12•20 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•20 years ago
|
||
Attachment #154690 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #154690 -
Flags: review?(keeda)
Assignee | ||
Updated•20 years ago
|
Attachment #154777 -
Flags: review?(jst)
Comment 14•20 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•20 years ago
|
||
Assignee | ||
Comment 16•20 years ago
|
||
Comment on attachment 155110 [details] [diff] [review] Patch proposal for the trunk Updated patch
Attachment #155110 -
Flags: review?(keeda)
Assignee | ||
Updated•20 years ago
|
Attachment #152764 -
Attachment is obsolete: true
Updated•20 years ago
|
Attachment #155110 -
Flags: superreview?(jst)
Attachment #155110 -
Flags: review?(keeda)
Attachment #155110 -
Flags: review+
Comment 17•20 years ago
|
||
This needs to go in along with the patch above.
Updated•20 years ago
|
Attachment #155156 -
Flags: superreview?(jst)
Attachment #155156 -
Flags: review?(chantepie)
Assignee | ||
Updated•20 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•20 years ago
|
Assignee: web-services → chantepie
Status: ASSIGNED → NEW
Assignee | ||
Updated•20 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•20 years ago
|
Attachment #155156 -
Flags: review?(chantepie) → review+
Assignee | ||
Comment 18•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #155249 -
Flags: review?(keeda)
Assignee | ||
Updated•20 years ago
|
OS: MacOS X → All
Hardware: Macintosh → All
Assignee | ||
Updated•20 years ago
|
Attachment #155156 -
Flags: superreview?(jst)
Assignee | ||
Updated•20 years ago
|
Attachment #155110 -
Flags: superreview?(jst)
Assignee | ||
Updated•20 years ago
|
Attachment #154777 -
Attachment is obsolete: true
Comment 19•20 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•20 years ago
|
Attachment #155249 -
Flags: superreview?(jst)
Comment 20•20 years ago
|
||
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•20 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•20 years ago
|
Attachment #155110 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #155156 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #155249 -
Flags: approval1.7.x?
Comment 22•20 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•20 years ago
|
Attachment #155249 -
Flags: approval1.7.x?
Assignee | ||
Comment 23•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #159072 -
Flags: approval1.7.x?
Comment 24•20 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•20 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.
Comment 26•20 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•20 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•20 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•20 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 ...
Comment 30•20 years ago
|
||
Updated•20 years ago
|
Attachment #165774 -
Flags: review?(chantepie)
Assignee | ||
Updated•20 years ago
|
Attachment #165774 -
Flags: review?(chantepie) → review+
Updated•20 years ago
|
Attachment #165774 -
Flags: superreview?(jst)
Comment 31•20 years ago
|
||
Comment on attachment 165774 [details] [diff] [review] fix sr=jst
Attachment #165774 -
Flags: superreview?(jst) → superreview+
Comment 32•20 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•20 years ago
|
||
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•20 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...
Comment 35•20 years ago
|
||
Fix landed on the trunk.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Updated•19 years ago
|
Attachment #170447 -
Flags: review?(jst)
Updated•7 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•