329520 - Hook up schemaLoadError to the fatal error dialog box?

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Allan Beaufour]

Should we hook more stuff like:
http://lxr.mozilla.org/seamonkey/source/extensions/xforms/nsXFormsModelElement.cpp#464
up to the error dialog box (or whatever it will be in the future)?

Comment 1

[aaronr]

makes sense that any fatal exception be hooked up to that error dialog.  Please also verify that we spit something out the the JS Console so that the user has something to look at if they click on that link off of the dialog box.

Comment 2

[Merle Sterling]

Attachment: Testcase - schema attribute pointing to non-existent xsd.

Comment 3

[Merle Sterling]

Attachment: Fatal Error dialog for schema load error

Created a new method HandleFatalError to display the error dialog in response to fatal errors.

Comment 4

[aaronr]

Comment on attachment 215185 [details] [diff] [review]
Fatal Error dialog for schema load error

>Index: nsXFormsModelElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsModelElement.cpp,v
>retrieving revision 1.89
>diff -p -u -8 -r1.89 nsXFormsModelElement.cpp
>--- nsXFormsModelElement.cpp	15 Mar 2006 09:18:06 -0000	1.89
>+++ nsXFormsModelElement.cpp	15 Mar 2006 22:37:22 -0000
>@@ -462,16 +462,21 @@ nsXFormsModelElement::InitializeInstance
>           newURI->GetSpec(uriSpec);
>           rv = mSchemas->LoadAsync(NS_ConvertUTF8toUTF16(uriSpec), this);
>         }
>       }
>       if (NS_FAILED(rv)) {
>         // this is a fatal error (XXX)
>         nsXFormsUtils::ReportError(NS_LITERAL_STRING("schemaLoadError"), mElement);
>         nsXFormsUtils::DispatchEvent(mElement, eEvent_LinkException);
>+        nsXFormsUtils::HandleFatalError(mElement, 
>+                                        NS_LITERAL_STRING("chrome://xforms/content/bindingex.xul"),
>+                                        NS_LITERAL_STRING("XFormsLinkException"),
>+                                        NS_LITERAL_STRING("modal,dialog,chrome,dependent"),
>+                                        nsnull);

I think that HandleFatalError should take care of most of this string stuff for us.  We'll only ever have one fatal error dialog in our chrome (I assume), so we shouldn't need to pass in anything other than the name of it.  Could even just pass in the just the event atom and have HandleFatalError figure out the string from that, too.  Though on second thought that is probably going a little far because we might someday decide to have our own non-event-based fatal errors that we might like to report in addition to the XForms spec's fatal errors.

So I'd say move the filename of the dialog chrome, the dialog display options, and the extra argument parameters into the HandleFatalError code itself.

If you want, you could look to see if it is worth generalizing all of the places where we call opendialog in XForms.  If it is worth it, then rename your HandleFatalError to something like nsXFormsUtils::OpenDialog and then have your new HandleFatalError call this new generalized open dialog function.  I don't know that we have that many places that need to open dialogs like this, though.


>
>Index: nsXFormsUtils.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsUtils.cpp,v
>retrieving revision 1.69
>diff -p -u -8 -r1.69 nsXFormsUtils.cpp
>--- nsXFormsUtils.cpp	15 Mar 2006 09:31:13 -0000	1.69
>+++ nsXFormsUtils.cpp	15 Mar 2006 22:37:24 -0000
>@@ -1567,16 +1567,37 @@ nsXFormsUtils::GetElementById(nsIDOMDocu

>+/* static */
>+PRBool
>+nsXFormsUtils::HandleFatalError(nsIDOMElement *aElement, 
>+                                const nsAString& aUrl,
>+                                const nsAString& aName, 
>+                                const nsAString& aOptions,
>+                                nsISupports* aExtraArgument)
>+{

nit: as per the current style in this file, please line up the parameter names (like nsXFormsUtils::GetWindowFromDocument has them, for example...all of the aXxxx in a column).

>+  if (!aElement) {
>+    return PR_FALSE;
>+  }
>   nsCOMPtr<nsIDOMDocument> doc;
>   aElement->GetOwnerDocument(getter_AddRefs(doc));
> 
>   nsCOMPtr<nsIDocument> iDoc(do_QueryInterface(doc));
>   if (!iDoc) {
>     return PR_FALSE;
>   }
> 
>@@ -1604,23 +1625,20 @@ nsXFormsUtils::HandleBindingException(ns
>   rv = nsXFormsUtils::GetWindowFromDocument(doc, getter_AddRefs(internal));
>   if (NS_FAILED(rv) || !internal) {
>     return PR_FALSE;
>   }
> 
> 
>   // Show popup
>   nsCOMPtr<nsIDOMWindow> messageWindow;
>-  rv = internal->OpenDialog(NS_LITERAL_STRING("chrome://xforms/content/bindingex.xul"),
>-                            NS_LITERAL_STRING("XFormsBindingException"),
>-                            NS_LITERAL_STRING("modal,dialog,chrome,dependent"),
>-                            nsnull, getter_AddRefs(messageWindow));
>+  rv = internal->OpenDialog(aUrl, aName, aOptions, aExtraArgument, 
>+                            getter_AddRefs(messageWindow));
>   return NS_SUCCEEDED(rv);
> }
>-

nit: leave this empty line before AreEntitiesEqual, please

> /* static */ PRBool
> nsXFormsUtils::AreEntitiesEqual(nsIDOMNamedNodeMap *aEntities1,
>                                 nsIDOMNamedNodeMap *aEntities2)
> {
>   if (!aEntities1 && !aEntities2) {
>     return PR_TRUE;
>   }
> 
>Index: nsXFormsUtils.h
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsUtils.h,v
>retrieving revision 1.40
>diff -p -u -8 -r1.40 nsXFormsUtils.h
>--- nsXFormsUtils.h	23 Feb 2006 11:25:07 -0000	1.40
>+++ nsXFormsUtils.h	15 Mar 2006 22:37:24 -0000
>@@ -465,16 +465,34 @@ public:
>    * The dialog can be disabled via the |xforms.disablePopup| preference.
>    *
>    * @param aElement         Element the exception occured at
>    * @return                 Whether handling was successful
>    */
>   static PRBool HandleBindingException(nsIDOMElement *aElement);
> 
>   /**
>+   * Shows an error dialog for fatal errors.
>+   *
>+   * The dialog can be disabled via the |xforms.disablePopup| preference.
>+   *
>+   * @param aElement         Element the exception occured at
>+   * @param aURL             the URL to load in the new window
>+   * @param aName            the name to use for the new window
>+   * @param aOptions         the window options to use for the new window
>+   * @param aExtraArgument   Another way to pass arguments in.  This is 
>+   *                         mutually exclusive with the argv/argc approach.
>+   * @return                 Whether handling was successful
>+   */
>+  static PRBool nsXFormsUtils::HandleFatalError(nsIDOMElement *aElement, 
>+                                                const nsAString& aUrl,
>+                                                const nsAString& aName, 
>+                                                const nsAString& aOptions,
>+                                                nsISupports* aExtraArgument);
>+  /**

update this after you get rid of aUrl, aOptions and aExtraArgument.

Comment 5

[Merle Sterling]

Attachment: Made the changes suggested by Aaron

Comment 6

[aaronr]

Comment on attachment 215339 [details] [diff] [review]
Made the changes suggested by Aaron

The code looks great.  Thanks for moving it all into a central function.  Just some styling nits below.


>Index: nsXFormsModelElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsModelElement.cpp,v
>retrieving revision 1.90
>diff -u -p -8 -r1.90 nsXFormsModelElement.cpp
>--- nsXFormsModelElement.cpp	16 Mar 2006 19:17:19 -0000	1.90
>+++ nsXFormsModelElement.cpp	16 Mar 2006 22:35:18 -0000
>@@ -463,16 +463,18 @@ nsXFormsModelElement::InitializeInstance
>           newURI->GetSpec(uriSpec);
>           rv = mSchemas->LoadAsync(NS_ConvertUTF8toUTF16(uriSpec), this);
>         }
>       }
>       if (NS_FAILED(rv)) {
>         // this is a fatal error (XXX)
>         nsXFormsUtils::ReportError(NS_LITERAL_STRING("schemaLoadError"), mElement);
>         nsXFormsUtils::DispatchEvent(mElement, eEvent_LinkException);
>+        nsXFormsUtils::HandleFatalError(mElement, 
>+                                        NS_LITERAL_STRING("XFormsLinkException"));

nit: get rid of the spaces at the end of your changed lines.  Like here with mElement.  You can find simple things like this if you pass your patches through: http://beaufour.dk/jst-review/ first.  I won't mention the other ones, but looks like there are 6 or so lines like this in this patch.

>Index: nsXFormsUtils.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsUtils.cpp,v
>retrieving revision 1.69
>diff -u -p -8 -r1.69 nsXFormsUtils.cpp
>--- nsXFormsUtils.cpp	15 Mar 2006 09:31:13 -0000	1.69
>+++ nsXFormsUtils.cpp	16 Mar 2006 22:35:19 -0000
>@@ -1567,16 +1567,31 @@ nsXFormsUtils::GetElementById(nsIDOMDocu
> 
> /* static */
> PRBool
> nsXFormsUtils::HandleBindingException(nsIDOMElement *aElement)
> {
>   if (!aElement) {
>     return PR_FALSE;
>   }
>+
>+  nsresult rv = 
>+      HandleFatalError(aElement, NS_LITERAL_STRING("XFormsBindingException"));

nit: if wrapping an = due to line length, either indent the HandleFatalError two spaces from the 'n' in nsresult or leave HandleFatalError on the same line and column-ize the parameters.  I know that we aren't that consistent about it in many files, but we are trying :-)


>Index: nsXFormsUtils.h
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsUtils.h,v
>retrieving revision 1.40
>diff -u -p -8 -r1.40 nsXFormsUtils.h
>--- nsXFormsUtils.h	23 Feb 2006 11:25:07 -0000	1.40
>+++ nsXFormsUtils.h	16 Mar 2006 22:35:20 -0000
>@@ -465,16 +465,28 @@ public:
>    * The dialog can be disabled via the |xforms.disablePopup| preference.
>    *
>    * @param aElement         Element the exception occured at
>    * @return                 Whether handling was successful
>    */
>   static PRBool HandleBindingException(nsIDOMElement *aElement);
> 
>   /**
>+   * Shows an error dialog for fatal errors.
>+   *
>+   * The dialog can be disabled via the |xforms.disablePopup| preference.
>+   *
>+   * @param aElement         Element the exception occured at
>+   * @param aName            the name to use for the new window
>+   * @return                 Whether handling was successful
>+   */

nit: capitalize 't' at beginning of the descrition for aName.  I know it sounds crabby, just bugs me if two of them are and one isn't. :-)


With these changes, r=me

Comment 7

[Merle Sterling]

Attachment: Final formatting and code style changes

Comment 8

[Allan Beaufour]

Comment on attachment 215434 [details] [diff] [review]
Final formatting and code style changes

>Index: nsXFormsModelElement.cpp
>===================================================================
>       if (NS_FAILED(rv)) {
>         // this is a fatal error (XXX)

As you actually handle that now, kill the XXX

>         nsXFormsUtils::ReportError(NS_LITERAL_STRING("schemaLoadError"), mElement);
>         nsXFormsUtils::DispatchEvent(mElement, eEvent_LinkException);
>+        nsXFormsUtils::HandleFatalError(mElement,
>+                                        NS_LITERAL_STRING("XFormsLinkException"));

You should not ignore the return value.

>Index: nsXFormsUtils.cpp
>===================================================================
> PRBool
> nsXFormsUtils::HandleBindingException(nsIDOMElement *aElement)
> {
>   if (!aElement) {
>     return PR_FALSE;
>   }
>+
>+  nsresult rv =
>+    HandleFatalError(aElement, NS_LITERAL_STRING("XFormsBindingException"));
>+
>+  return NS_SUCCEEDED(rv);
>+}

HandleFatalError returns PRBool, not nsresult. But kill this function, and change calls to HandleBindingException to use HandleFatalError instead -- there's no use for this now.

>+PRBool
>+nsXFormsUtils::HandleFatalError(nsIDOMElement    *aElement,

>Index: nsXFormsUtils.h
>===================================================================

>+   * Shows an error dialog for fatal errors.
>+   *
>+   * The dialog can be disabled via the |xforms.disablePopup| preference.
>+   *
>+   * @param aElement         Element the exception occured at
>+   * @param aName            The name to use for the new window
>+   * @return                 Whether handling was successful
>+   */
>+  static PRBool nsXFormsUtils::HandleFatalError(nsIDOMElement    *aElement,

Hmmmm. Have you actually tried to compile this?

Comment 9

[Merle Sterling]

Attachment: Changes per Allan's review

Comment 10

[Allan Beaufour]

Comment on attachment 215924 [details] [diff] [review]
Changes per Allan's review

> Index: nsXFormsModelElement.cpp
> ===================================================================

>        if (NS_FAILED(rv)) {
> -        // this is a fatal error (XXX)
> +        // this is a fatal error
>          nsXFormsUtils::ReportError(NS_LITERAL_STRING("schemaLoadError"), mElement);
>          nsXFormsUtils::DispatchEvent(mElement, eEvent_LinkException);
> -        return NS_OK;
> +        if (nsXFormsUtils::HandleFatalError(mElement,
> +                                            NS_LITERAL_STRING("XFormsLinkException"))) {
> +
> +            return NS_OK;
> +        }
> +        else {
> +            return NS_ERROR_FAILURE;
> +        }

Always run your patch through jst-review:
http://beaufour.dk/jst-review/?patch=215924
That would have spotted a problem in the above.

This is a nit, but the else after a return, and two returns like that is hurting my eyes :)
return HandleFa... ? NS_OK : NS_ERROR_FAILURE
or
rv = NS_OK;
if (!HandleF...) {
  rv = NS_ERROR_FAILURE;
}
return rv;


> Index: nsXFormsUtils.cpp
> ===================================================================
> RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsUtils.cpp,v
> retrieving revision 1.70
> diff -u -8 -p -r1.70 nsXFormsUtils.cpp
> --- nsXFormsUtils.cpp	17 Mar 2006 02:19:07 -0000	1.70
> +++ nsXFormsUtils.cpp	22 Mar 2006 19:39:15 -0000
> @@ -1562,17 +1562,18 @@ nsXFormsUtils::GetElementById(nsIDOMDocu
>    NS_ADDREF(*aElement = element);
 
>    return NS_OK;
>  }
 
 
>  /* static */
>  PRBool
> -nsXFormsUtils::HandleBindingException(nsIDOMElement *aElement)
> +nsXFormsUtils::HandleFatalError(nsIDOMElement    *aElement,
> +                                const nsAString&  aName)

"&  aName" -> "  &aName"


> -/* static */ PRBool
> +/* static */
> +PRBool

(Hmm, we are not really consistant with this in this file... whatever.)

> Index: nsXFormsUtils.h
> ===================================================================

> +  
>    /**
> -   * Shows an error dialog for the user the first time an
> -   * xforms-binding-exception event is received by the control.
> +   * Shows an error dialog for fatal errors.
>     *
>     * The dialog can be disabled via the |xforms.disablePopup| preference.
>     *
>     * @param aElement         Element the exception occured at
> +   * @param aName            The name to use for the new window
>     * @return                 Whether handling was successful
>     */
> -  static PRBool HandleBindingException(nsIDOMElement *aElement);
> +  static NS_HIDDEN_(PRBool) HandleFatalError(nsIDOMElement   *aElement,
> +                                             const nsAString& aName);

"& aName" -> " &aName"

r=me with those nits fixed

Comment 11

[Merle Sterling]

(In reply to comment #10)
> (From update of attachment 215924 [details] [diff] [review] [edit])
> > Index: nsXFormsModelElement.cpp
> > ===================================================================

> return HandleFa... ? NS_OK : NS_ERROR_FAILURE
> or
> rv = NS_OK;
> if (!HandleF...) {
>   rv = NS_ERROR_FAILURE;
> }
> return rv;
Using the ternary if form will make the line way too long because of the requirement to place the ? at the end of the line instead of on a new line. That's why I didn't choose that form in the first place. I changed it to the second suggestion, which also makes the line longer than 80 chars but not as much.


> > Index: nsXFormsUtils.cpp
> > ===================================================================
> "&  aName" -> "  &aName"
> > -/* static */ PRBool
> > +/* static */
> > +PRBool
> (Hmm, we are not really consistant with this in this file... whatever.)

Yep, definitely not consistent. AFAIK placing the & next to the type instead of the variable or parameter name is a long-standing c++ convention for references. Changed it to place it next to the parm name anyway.

No comments on the actual *logic* of the patch? Before adding the call to HandleFatalError, the code always returned NS_OK. It will now do the same only if the call to HandleFatalError is successful. If the call fails, it will return NS_ERROR_FAILURE. The real error (checked by NS_FAILED(rv)) is lost either way. I didn't think NS_OK was correct in the original code - I think it should return the original rv regardless of whether or not the popup dialog fails.

Comment 12

[Merle Sterling]

Attachment: More formatting changes

See comments above.

Comment 13

[Allan Beaufour]

(In reply to comment #11)
> (In reply to comment #10)
> Using the ternary if form will make the line way too long because of the
> requirement to place the ? at the end of the line instead of on a new line.
> That's why I didn't choose that form in the first place. I changed it to the
> second suggestion, which also makes the line longer than 80 chars but not as
> much.

Well, please do not let the 80 chars restriction limit your coding. Imho, if it's possible to break the lines before 80 chars, then fine, it not, then be it :)

> > > Index: nsXFormsUtils.cpp
> > > ===================================================================
> > "&  aName" -> "  &aName"
> > > -/* static */ PRBool
> > > +/* static */
> > > +PRBool
> > (Hmm, we are not really consistant with this in this file... whatever.)
> 
> Yep, definitely not consistent. AFAIK placing the & next to the type instead of
> the variable or parameter name is a long-standing c++ convention for
> references.

Well, it depends on the project. The rule here is to follow the Mozilla conventions, or the style of the current component.

Changed it to place it next to the parm name anyway.

> No comments on the actual *logic* of the patch?

I did write r=me, right? :)

> Before adding the call to
> HandleFatalError, the code always returned NS_OK. It will now do the same only
> if the call to HandleFatalError is successful. If the call fails, it will
> return NS_ERROR_FAILURE. The real error (checked by NS_FAILED(rv)) is lost
> either way. I didn't think NS_OK was correct in the original code - I think it
> should return the original rv regardless of whether or not the popup dialog
> fails.

In InitializeInstances() I guess? It's not terribly important, but you are right. It would be a lot cleaner to return the original error, and feel free to do that. Although, in one of the calls we do not even bother to check the rv, so that should be fixed too.  I did not want to keep nit-picking your patch.

If you would like to fix it, please go ahead.

Comment 14

[Allan Beaufour]

Comment on attachment 216123 [details] [diff] [review]
More formatting changes

(In reply to comment #12)
> Created an attachment (id=216123) [edit]
> More formatting changes

Oh, you also asked for review. Well, in comment 10 I wrote "r=me with those nits fixed", so you do not need to ask for a review again if you agree with the nits.

So it's up to you, in with this as it is now, or more changes. If you want to have  more changes, then yes, ask for another review.

Comment 15

[aaronr]

checked into trunk for msterlin