User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20051212 Firefox/1.6a1 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20051212 Firefox/1.6a1 description will be with proposed patch. Reproducible: Always

Comment on attachment 212548 [details] [diff] [review] patch > nsXFormsDelegateStub::Refresh() > { > if (mRepeatState == eType_Template) >- return NS_OK; >+ return NS_ERROR_FAILURE; > > const nsVoidArray* list = nsPostRefresh::PostRefreshList(); > if (list && list->IndexOf(this) >= 0) { > // This control will be refreshed later. >- return NS_OK; >+ return NS_ERROR_FAILURE; > } > > SetMozTypeAttribute(); > > nsCOMPtr<nsIXFormsUIWidget> widget = do_QueryInterface(mElement); > if (!widget) >- return NS_OK; >+ return NS_ERROR_FAILURE; > > return widget->Refresh(); > } Refresh() returns error when control had not been refreshed. >@@ -213,6 +213,10 @@ > mRepeatState = eType_Template; > break; > } >+ if (nsXFormsUtils::IsXFormsElement(parent, NS_LITERAL_STRING("itemset"))) { >+ mRepeatState = eType_Template; >+ break; >+ } I guess controls inside of itemset is also template controls and they shouldn't be refreshed too. > NS_IMETHODIMP > nsXFormsLabelElement::Refresh() > { >- nsXFormsDelegateStub::Refresh(); >+ nsresult rv = nsXFormsDelegateStub::Refresh(); >+ if (NS_FAILED(rv)) >+ return rv; >+ > nsCOMPtr<nsIDOMNode> parent; > mElement->GetParentNode(getter_AddRefs(parent)); > Label shouldn't do own refresh if delegate didn't pass. > // Refresh controls >- rv = control->Refresh(); >- NS_ENSURE_SUCCESS(rv, rv); >+ control->Refresh(); > } Now model shouldn't throw warning if refresh didn't pass. >@@ -137,20 +138,24 @@ > NS_IMETHODIMP > nsXFormsOutputElement::Refresh() > { >- if (mRepeatState == eType_Template) >- return NS_OK; >+ mShouldBeRefreshed = PR_TRUE; >+ nsresult rv = nsXFormsDelegateStub::Refresh(); >+ mShouldBeRefreshed = PR_FALSE; >+ return rv; >+} Output control refresh is much of code dublicate of delegate refresh. nsXFormsDelegateStub::Refresh(); >- NS_ENSURE_SUCCESS(rv, rv); >+ if (NS_FAILED(rv)) >+ return rv; > > if (!mBoundNode) >- return NS_OK; >+ return NS_ERROR_FAILURE; Refresh behaviour changes for upload control

Comment on attachment 212548 [details] [diff] [review] patch Two things: * you seem to misuse the return value of refresh. If a control is refreshed later, it returns NS_ERROR_FAILURE, but if there's no widget it also returns NS_ERROR_FAILURE. One is a failure, one is not. The model should bail and show an error on one of them. So you need to distinguish between those two. * for output, please use a bool (mValueIsDirty), set true by refresh, and set false when the value is successfully updated in getValue(). Much nicer.

(In reply to comment #3) > * you seem to misuse the return value of refresh. If a control is refreshed > later, it returns NS_ERROR_FAILURE, but if there's no widget it also returns > NS_ERROR_FAILURE. One is a failure, one is not. The model should bail and show > an error on one of them. So you need to distinguish between those two. > Agree. Is the proper way replacing void nsIXFormsControlBase::refresh() method on boolean nsIXFormsControlBase::refresh() method? But I get a problem. nsXFormsModelElement is inherited from nsIXFormsContextControl interface what involves realization of nsIXFormsControlBase::refresh() method. By other side nsXFormsModelElement realizes nsIXFormsModelElement::refresh() method. Should nsXFormsModelElement have two refresh() methods: one is for nsIXFormsControlBase, other is for nsIXFormsModelElement?

(In reply to comment #4) > (In reply to comment #3) > > > * you seem to misuse the return value of refresh. If a control is refreshed > > later, it returns NS_ERROR_FAILURE, but if there's no widget it also returns > > NS_ERROR_FAILURE. One is a failure, one is not. The model should bail and show > > an error on one of them. So you need to distinguish between those two. > > > > Agree. Is the proper way replacing > void nsIXFormsControlBase::refresh() > method on > boolean nsIXFormsControlBase::refresh() > method? > > But I get a problem. nsXFormsModelElement is inherited from > nsIXFormsContextControl interface what involves realization of > nsIXFormsControlBase::refresh() method. By other side nsXFormsModelElement > realizes nsIXFormsModelElement::refresh() method. Should nsXFormsModelElement > have two refresh() methods: one is for nsIXFormsControlBase, other is for > nsIXFormsModelElement? Ouch. I had not realized that we actually have overlapping functions there... You have to define your own return value then.

Look here for info. on generating your own return types: http://lxr.mozilla.org/seamonkey/source/xpcom/base/nsError.h#134

(In reply to comment #5) > Ouch. I had not realized that we actually have overlapping functions there... Probably we should exclude nsIXFormsContextControl interface from inheritance chain. How do you look at that?

(In reply to comment #6) > Look here for info. on generating your own return types: > http://lxr.mozilla.org/seamonkey/source/xpcom/base/nsError.h#134 > Should I define new module or use NS_ERROR_MODULE_GENERAL?

(In reply to comment #7) > (In reply to comment #5) > > > Ouch. I had not realized that we actually have overlapping functions there... > > Probably we should exclude nsIXFormsContextControl interface from inheritance > chain. How do you look at that? No, we need it for model... why was that. I think it's because the bind elements need to find their model, or was it submission. Whatever triggered it, it's nice to have the model as a normal "context control" object for it's children.

(In reply to comment #8) > (In reply to comment #6) > > Look here for info. on generating your own return types: > > http://lxr.mozilla.org/seamonkey/source/xpcom/base/nsError.h#134 > > Should I define new module or use NS_ERROR_MODULE_GENERAL? A new model has to change stuff that we need approval for. Just use MODULE_GENERAL. Then we can always create new bug for adding our own module, if we want.

Comment on attachment 214864 [details] [diff] [review] patch2 Stash the error in nsXFormsUtils.h instead of a new file. Almost all classes include utils.h anyway. With that r=me

(In reply to comment #12) > (From update of attachment 214864 [details] [diff] [review] [edit]) > Stash the error in nsXFormsUtils.h instead of a new file. Almost all classes > include utils.h anyway. > > With that r=me > I thought about it. In one hand I didn't like "almost all classes" and many error modules do it in separate file. In other hand we have only one own error code and separate file is luxuriously for it. Probably later we should move them from nsXFormsUtils file to nsXFormsError.

Comment on attachment 214960 [details] [diff] [review] patch3 I'm r-'ing the patch, because it fails the testcase I am about to attach. From my debugging, I think that the problem is that you removed SetDOMStringToNull, yet GetStringValue won't overwrite a string...it appears to append to the end of it. So if mValueIsDirty, you should empty out mValue first. Also, please use the 'p' flag when creating cvs diffs. For instance, I use -u8pN. It is nice to see the function names where the changes are located. Thanks.

Checked into trunk.

*** Bug 333044 has been marked as a duplicate of this bug. ***