This affects some of the test suite, as it depends on local resources: 8.3.5.b (hint) and 8.3.6.b (alert)

Oh and it fails 8.3.4.b for xf:help element

Appears src on instance behaves the same way as well, see test case 4.5.2.a http://www.w3.org/MarkUp/Forms/Test/XForms1.0/Edition2/Chapt4/4.5/4.5.2/4.5.2.a.xhtml Perhaps fixing this bug will fix this testcase as well

I haven't seen any activity on this bug from Olli and Merle has found out some good stuff debugging it. Assigning to him.

Implement OnStartRequest, OnDataAvailable, and on OnStopRequest. Convert relative urls to absolute in HandleModalAndModlessMessage.

Comment on attachment 221495 [details] [diff] [review] Convert relative urls to absolute >Index: nsXFormsMessageElement.cpp >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsMessageElement.cpp,v >retrieving revision 1.19 >diff -u -8 -p -r1.19 nsXFormsMessageElement.cpp >--- nsXFormsMessageElement.cpp 17 Apr 2006 13:06:08 -0000 1.19 >+++ nsXFormsMessageElement.cpp 9 May 2006 21:07:50 -0000 >@@ -637,17 +637,30 @@ nsXFormsMessageElement::HandleModalAndMo > } > > // order of precedence is single-node binding, linking attribute then > // inline text > nsresult rv; > if (!hasBinding && !src.IsEmpty()) { > // Creating a normal window for messages with src attribute. > options.AppendLiteral(",chrome=no"); >- rv = ConstructMessageWindowURL(src, PR_TRUE, src); >+ >+ // Create a new URI so that we properly convert relative urls to absolute. >+ mElement->GetOwnerDocument(&aDoc); don't need this call to GetOwnerDocument. aDoc is already mElement's owner document. >+ nsCOMPtr<nsIDocument> doc(do_QueryInterface(aDoc)); >+ NS_ENSURE_STATE(doc); >+ nsCOMPtr<nsIURI> uri; >+ NS_NewURI(getter_AddRefs(uri), src, doc->GetDocumentCharacterSet().get(), >+ doc->GetDocumentURI()); >+ NS_ENSURE_STATE(uri); >+ nsCAutoString uriSpec; >+ uri->GetSpec(uriSpec); >+ rv = ConstructMessageWindowURL(NS_ConvertUTF8toUTF16(uriSpec), >+ PR_TRUE, >+ NS_ConvertUTF8toUTF16(uriSpec)); you need to pass in the variable 'src' as the 3rd parameter to ConstructMessageWindowURL or your call to internal->OpenDialog later in the function won't have the correct stuff in its 1st parameter. > NS_ENSURE_SUCCESS(rv, rv); > } else { > // Cloning the content of the xf:message and creating a > // dialog for it. > options.AppendLiteral(",dialog,chrome,dependent,width=200,height=200"); > nsCOMPtr<nsIDOMDocument> ddoc; > nsCOMPtr<nsIDOMDOMImplementation> domImpl; > rv = aDoc->GetImplementation(getter_AddRefs(domImpl)); >@@ -1028,88 +1041,96 @@ nsXFormsMessageElement::OnChannelRedirec > } > > // nsIStreamListener > > NS_IMETHODIMP > nsXFormsMessageElement::OnStartRequest(nsIRequest *aRequest, > nsISupports *aContext) > { >- return NS_OK; >-} >+ // Make sure to null out mChannel before we return. Keep in mind that >+ // if this is the last message channel to be loaded for the xforms >+ // document then when AddRemoveExternalResource is called, it may result >+ // in xforms-ready firing. Should there be a message acting as a handler >+ // for xforms-ready, it will start the logic to display itself >+ // (HandleAction()). So we can't call AddRemoveExternalResource to remove >+ // this channel from the count until we've set the mStopType to be the >+ // proper value. Entering this function, mStopType will be eStopType_None, >+ // so if we need mStopType to be any other value (like in an error >+ // condition), please make sure it is set before AddRemoveExternalResource >+ // is called. nit: the contents of this whole function needs to be moved two columns left to be consistent with the rest of the file. And please make sure that the contents of any other code blocks (like some of the 'if' tests below) are only indented two spaces. >+ NS_ASSERTION(aRequest == mChannel, "unexpected request"); >+ NS_ASSERTION(mChannel, "no channel"); >+ >+ nsresult status; >+ nsresult rv = mChannel->GetStatus(&status); >+ // DNS errors and other obvious problems will return failure status >+ if (NS_FAILED(rv) || NS_FAILED(status)) { >+ // NS_BINDING_ABORTED means that we have been cancelled by a later >+ // AttributeSet() call (which will also reset mStopType), so don't >+ // treat it like an error. >+ if (status != NS_BINDING_ABORTED) { >+ nsAutoString src, tagName; >+ mElement->GetLocalName(tagName); >+ mElement->GetAttribute(NS_LITERAL_STRING("src"), src); >+ const PRUnichar *strings[] = { tagName.get(), src.get() }; >+ nsXFormsUtils::ReportError(NS_LITERAL_STRING("externalLink2Error"), >+ strings, 2, mElement, mElement); >+ mStopType = eStopType_LinkError; >+ AddRemoveExternalResource(PR_FALSE); >+ mChannel = nsnull; >+ } > >-NS_IMETHODIMP >-nsXFormsMessageElement::OnDataAvailable(nsIRequest *aRequest, >- nsISupports *aContext, >- nsIInputStream *aInputStream, >- PRUint32 aOffset, >- PRUint32 aCount) >-{ >- return NS_OK; >-} >+ return NS_BINDING_FAILED; >+ } nit: return doesn't line up right. > >-NS_IMETHODIMP >-nsXFormsMessageElement::OnStopRequest(nsIRequest *aRequest, >- nsISupports *aContext, >- nsresult aStatusCode) >-{ >+ // If status is zero, it might still be an error if it's http: >+ // http has data even when there's an error like a 404. >+ nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(mChannel); >+ if (!httpChannel) >+ return NS_BINDING_ABORTED; don't return from ::OnStartRequest, in any manner, without both setting mChannel to nsnull and calling AddRemoveExternalResource(PR_FALSE). Maybe use a test similar to what currently exists in ::OnStopRequest? Then you only have to do it in two places in this function rather than three.

Comment on attachment 221740 [details] [diff] [review] Changes to OnStartRequest >Index: nsXFormsMessageElement.cpp >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsMessageElement.cpp,v >retrieving revision 1.19 >diff -u -8 -p -r1.19 nsXFormsMessageElement.cpp >--- nsXFormsMessageElement.cpp 17 Apr 2006 13:06:08 -0000 1.19 >+++ nsXFormsMessageElement.cpp 11 May 2006 20:33:44 -0000 >@@ -1028,88 +1040,105 @@ nsXFormsMessageElement::OnChannelRedirec > } > > // nsIStreamListener > > NS_IMETHODIMP > nsXFormsMessageElement::OnStartRequest(nsIRequest *aRequest, > nsISupports *aContext) > { >- return NS_OK; >-} >- >-NS_IMETHODIMP >-nsXFormsMessageElement::OnDataAvailable(nsIRequest *aRequest, >- nsISupports *aContext, >- nsIInputStream *aInputStream, >- PRUint32 aOffset, >- PRUint32 aCount) >-{ >- return NS_OK; >-} >- >-NS_IMETHODIMP >-nsXFormsMessageElement::OnStopRequest(nsIRequest *aRequest, >- nsISupports *aContext, >- nsresult aStatusCode) >-{ >- >- // We are done with the load request, whatever the result, so make sure to >- // null out mChannel before we return. Keep in mind that if this is the last >- // message channel to be loaded for the xforms document then when >- // AddRemoveExternalResource is called, it may result in xforms-ready firing. >- // Should there be a message acting as a handler for xforms-ready, it will >- // start the logic to display itself (HandleAction()). So we can't call >- // AddRemoveExternalResource to remove this channel from the count until we've >- // set the mStopType to be the proper value. Entering this function, >- // mStopType will be eStopType_None, so if we need mStopType to be any other >- // value (like in an error condition), please make sure it is set before >- // AddRemoveExternalResource is called. >- >- if (NS_FAILED(aStatusCode)) { >+ // Make sure to null out mChannel before we return. Keep in mind that >+ // if this is the last message channel to be loaded for the xforms >+ // document then when AddRemoveExternalResource is called, it may result >+ // in xforms-ready firing. Should there be a message acting as a handler >+ // for xforms-ready, it will start the logic to display itself >+ // (HandleAction()). So we can't call AddRemoveExternalResource to remove >+ // this channel from the count until we've set the mStopType to be the >+ // proper value. Entering this function, mStopType will be eStopType_None, >+ // so if we need mStopType to be any other value (like in an error >+ // condition), please make sure it is set before AddRemoveExternalResource >+ // is called. >+ NS_ASSERTION(aRequest == mChannel, "unexpected request"); >+ NS_ASSERTION(mChannel, "no channel"); >+ >+ nsresult status; >+ nsresult rv = mChannel->GetStatus(&status); >+ // DNS errors and other obvious problems will return failure status >+ if (NS_FAILED(rv) || NS_FAILED(status)) { > // NS_BINDING_ABORTED means that we have been cancelled by a later >- // AttributeSet() call (which will also reset mStopType). So don't treat >- // like an error. >- if (aStatusCode != NS_BINDING_ABORTED) { >+ // AttributeSet() call (which will also reset mStopType), so don't >+ // treat it like an error. >+ if (status != NS_BINDING_ABORTED) { > nsAutoString src, tagName; > mElement->GetLocalName(tagName); > mElement->GetAttribute(NS_LITERAL_STRING("src"), src); > const PRUnichar *strings[] = { tagName.get(), src.get() }; > nsXFormsUtils::ReportError(NS_LITERAL_STRING("externalLink2Error"), > strings, 2, mElement, mElement); > mStopType = eStopType_LinkError; >- AddRemoveExternalResource(PR_FALSE); >- mChannel = nsnull; >- >- return NS_OK; > } >+ >+ AddRemoveExternalResource(PR_FALSE); >+ mChannel = nsnull; >+ >+ return NS_BINDING_FAILED; > } > >- PRUint32 responseStatus; >+ // If status is zero, it might still be an error if it's http: >+ // http has data even when there's an error like a 404. > nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(mChannel); >- if (NS_SUCCEEDED(aStatusCode) && httpChannel) { >- nsresult rv = httpChannel->GetResponseStatus(&responseStatus); >+ if (!httpChannel) { >+ AddRemoveExternalResource(PR_TRUE); >+ mChannel = nsnull; >+ return NS_BINDING_ABORTED; >+ } >+ this should be AddRemoveExternalResource(PR_FALSE). PR_TRUE is for when you are starting a request, I believe. >+ // If responseStatus is 2xx, it is valid. >+ // 3xx (various flavors of redirection) COULD be successful. Can't really >+ // follow those to conclusion so we'll assume they were successful. >+ // If responseStatus is 4xx or 5xx, it is an error. >+ PRUint32 responseStatus; >+ rv = httpChannel->GetResponseStatus(&responseStatus); >+ if (NS_FAILED(rv) || (responseStatus >= 400)) { >+ nsAutoString src, tagName; >+ mElement->GetLocalName(tagName); >+ mElement->GetAttribute(NS_LITERAL_STRING("src"), src); >+ const PRUnichar *strings[] = { tagName.get(), src.get() }; >+ nsXFormsUtils::ReportError(NS_LITERAL_STRING("externalLink2Error"), >+ strings, 2, mElement, mElement); >+ mStopType = eStopType_LinkError; > >- // If responseStatus is 4xx or 5xx, it is an error. 2xx is success. >- // 3xx (various flavors of redirection) COULD be successful. Can't really >- // follow those to conclusion so we'll assume they were successful. >- if (NS_FAILED(rv) || (responseStatus >= 400)) { >- nsAutoString src, tagName; >- mElement->GetLocalName(tagName); >- mElement->GetAttribute(NS_LITERAL_STRING("src"), src); >- const PRUnichar *strings[] = { tagName.get(), src.get() }; >- nsXFormsUtils::ReportError(NS_LITERAL_STRING("externalLink2Error"), >- strings, 2, mElement, mElement); >- mStopType = eStopType_LinkError; >- } >+ AddRemoveExternalResource(PR_FALSE); >+ mChannel = nsnull; >+ >+ return NS_BINDING_FAILED; > } > > AddRemoveExternalResource(PR_FALSE); > mChannel = nsnull; > >+ return NS_BINDING_ABORTED; >+} >+ Sorry that I missed this in the first review...you can return NS_BINDING_ABORTED in every situation in ::OnStartRequest. That should help simplify your code a little down to only two returns in the function, probably. You shouldn't need to worry about differentiating between NS_BINDING_FAILED and NS_BINDING_ABORTED. Once we make our test we just want to stop the channel. I don't think that it matters how we do it since we aren't checking the status anywhere down the line (not in ::OnDataAvailable or ::OnStopRequest, for example). With those, r=me

Always return NS_BINDING_ABORTED since we don't care to read the data anyway. Simplify the logic a bit to reduce the number of places from which we return.

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

nsXFormsMessageElement.cpp: In member function ‘nsresult nsXFormsMessageElement::HandleModalAndModelessMessage(nsIDOMDocument*, nsAString_internal&)’: nsXFormsMessageElement.cpp:657: error: no matching function for call to ‘nsXFormsMessageElement::ConstructMessageWindowURL(NS_ConvertUTF8toUTF16, int, nsAutoString&)’ nsXFormsMessageElement.cpp:183: note: candidates are: nsresult nsXFormsMessageElement::ConstructMessageWindowURL(nsAString_internal&, PRBool, nsAString_internal&) gmake[1]: *** [nsXFormsMessageElement.o] Error 1

The issue here is that NS_ConvertUTF8toUTF16 creates a const nsAString&, yet ConstructMessageWindowURL just uses nsAString&. jag: Temporary objects can only bind to const references jag: MS VC lets them bind to non-const refs too But Visual C++ seems to allow non-const refs, which linux doesn't.

Built with Doron's latest patch and here are the results running W3C test suite: Test cases now pass: 8.3.5.b hint 8.3.6.b alert 8.3.4.b help 10.1.12.a valid external message 10.1.12.b source precedence for message element Test cases that still fail: 8.3.5.b linking precedence for hint element 4.5.2.a xforms-link-exception ...investigating these a bit more

checked into trunk for msterlin