Hi, But the patch had not been checked it yet? Or did you patch it before trying the tests?
In any case, I have committed it just now, all myfaces tests pass. Cheers! Bruno On 23 July 2010 08:26, Matthias Wessendorf <[email protected]> wrote: > I just tested 2.0.2-SNAPSHOT with our internal version of ADF Faces, > that runs on JSF2. > > My jetty powered environment gives me no errors with our latest trunk... > > -Matthias > > On Thu, Jul 22, 2010 at 9:36 PM, Werner Punz <[email protected]> > wrote: > > Btw. one issue about this, check if your fix does not break anything in > the > > ppr case. > > The problem is that the ppr responseWriter simply delegates the > > HtmlResponseWriterImpl, but the issue is > > that CDATA blocks are automatically opened before any html is rendered, > any > > valid CDATA block inside should not be swallowed but escaped in that > case. > > > > > > So a ppr response looks like this > > > > <changes> > > <update id="bla"><![CDATA[<div id="bla"> .... </div> ]]></update> > > > > The problem is that per spec definition the xhr response must be xml > > and any html must be embedded within CDATA blocks, now if there is > another > > CDATA block within the valid html it must be preserved at all costs > because > > it is vital that it is properly rendered at the ppr update time (hence > the > > complicated escaping in my code) > > > > Werner > > > > > > Am 22.07.10 21:31, schrieb Werner Punz: > >> > >> Ok point taken, yes the HTMLResponseWriterImpl definitely is html so a > >> fix there is valid. > >> > >> > >> Werner > >> > >> > >> Am 22.07.10 20:11, schrieb Bruno Aranda: > >>> > >>> But we are talking about the HtmlResponseWriterImpl, which outputs > HTML. > >>> The fix I have done is in that class and it only checks if there is a > >>> CDATA already started before starting one when writing the scripts. It > >>> is slightly different to the original problem (about the HtmlResponse, > >>> which is different from the one in Mojarra). The fix simply checks if > >>> there is the CDATA around the script before opening another one inside > >>> the script. I think it is OK if we just do not nest CDATAs in the HTML > >>> response, even if it was allowed. > >>> > >>> And this fixes the problems with PrimeFaces too, without having to > >>> change the ResponseWriter or the PartialResponseWriterImpl... > >>> > >>> Bruno > >>> > >>> On 22 July 2010 16:59, Werner Punz <[email protected] > >>> <mailto:[email protected]>> wrote: > >>> > >>> Hia guys please also read up on my jira response. > >>> The entire thing is not as easy as it seems, because there are > >>> various ways a cdata block can be opened, first you can do it via > >>> startCDATA secondly you can do it via a direct write. > >>> > >>> I did some kind of double buffering in case of a cdata block was > >>> opened and then escaped the ]]> as multiple cdata blocks (the jira > >>> response explains the technique exactly) > >>> > >>> And yes there is somewhat a speed hit because of this, but in case > >>> of the partial response writer I did not have a chance because: > >>> > >>> But the partial response writer is somewhat different, because it > >>> has to press html code in a valid xml response format, so nested > >>> cdata blocks can occur naturally, the normal response writer is > >>> somewhat different because nested cdata blocks are only forbidden > >>> for xmlish output dialects others might allow it. > >>> > >>> Werner > >>> > >>> > >>> > >>> Am 22.07.10 17:47, schrieb Mark Struberg: > >>> > >>> > >>> But isn't the patch of Marcus Büttner doing this by maintaining > >>> a reference > >>> counter? > >>> > >>> Another question: how is the performance of all this > >>> scanning/dynamic > >>> replacement? > >>> > >>> LieGrue, > >>> strub > >>> > >>> > >>> From: Bruno Aranda<[email protected] > >>> <mailto:[email protected]>> > >>> To: MyFaces Development<[email protected] > >>> <mailto:[email protected]>> > >>> Sent: Thu, July 22, 2010 5:26:35 PM > >>> Subject: Re: Fixing ResponseWriter.startCDATA/endCDATA > >>> > >>> Further investigation of this incompatibility problem with > >>> myfaces leads me to > >>> the fact that in the HtmlResponseWriterImpl, when we write > >>> the content of a > >>> script, we create a CDATA element without checking if is > >>> nested at all. That is > >>> > >>> > >>> a problem, because if we use the standard response writer > >>> and we write a script > >>> > >>> > >>> section inside a CDATA section, the problem will be triggered... > >>> > >>> We need a way in HtmlResponseWriterImpl to check nested > >>> CDATA calls to the > >>> startCDATA or endCDATA methods I guess. > >>> > >>> Cheers, > >>> > >>> Bruno > >>> > >>> > >>> On 22 July 2010 15:15, Bruno Aranda<[email protected] > >>> <mailto:[email protected]>> wrote: > >>> > >>> Just clicked on sent and Werner had answered in the JIRA > >>> issue explaining the > >>> partial approach... > >>> > >>> > >>> Cheers, > >>> > >>> Bruno > >>> > >>> > >>> > >>> On 22 July 2010 15:12, Bruno > >>> Aranda<[email protected] > >>> <mailto:[email protected]>> wrote: > >>> > >>> As you can see in my black box tests with Mojarra, the > >>> behaviour is different in > >>> > >>> both implementations. In the base ResponseWriter class, > >>> they don't do anything > >>> > >>> > >>> in the startCDATA method and throw an undocumented > >>> exception in the endCDATA. > >>> > >>> > >>> In both implementations of the base class, they > >>> throw an exception if the > >>> startCDATA method is called and it had been called > >>> already... > >>> > >>> I don't quite understand our implementation of the > >>> PartialResponseWriterImpl. We > >>> > >>> do buffer nested CDATAs and write them when closing > >>> the parent one? This would > >>> > >>> > >>> still create nested CDATAs... I still need to > >>> understand this bit properly, > >>> > >>> Cheers, > >>> > >>> Bruno > >>> > >>> > >>> > >>> On 22 July 2010 13:58, Bruno > >>> Aranda<[email protected] > >>> <mailto:[email protected]>> wrote: > >>> > >>> yeah, sorry, my problem was running only the API > >>> tests :) > >>> > >>> > >>> Bruno > >>> > >>> > >>> > >>> On 22 July 2010 13:48, Matthias > >>> Wessendorf<[email protected] > >>> <mailto:[email protected]>> wrote: > >>> > >>> On Thu, Jul 22, 2010 at 2:14 PM, Matthias > >>> Wessendorf<[email protected] > >>> <mailto:[email protected]>> > >>> > >>> wrote: > >>> > >>> so, maybe there are now regressions? > >>> > >>> hrm. have you done some testing? > >>> > >>> > >>> Ah, the discussion is on the JIRA.. > >>> > >>> please run tests, before committing ;-) > >>> > >>> > >>> > >>> -M > >>> > >>> On Thu, Jul 22, 2010 at 2:07 PM, > >>> Matthias Wessendorf<[email protected] > >>> <mailto:[email protected]>> > >>> > >>> wrote: > >>> > >>> sounds right. > >>> > >>> does blame say more why it does not > >>> do nothing? > >>> > >>> It is also kinda strange since the > >>> TCK was successfully executed for > >>> 2.0.0 and 2.0.1; > >>> > >>> -Matthias > >>> > >>> On Thu, Jul 22, 2010 at 1:48 PM, > >>> Bruno Aranda<[email protected] > >>> <mailto:[email protected]>> > >>> > >>> wrote: > >>> > >>> Hi, > >>> > >>> Having problems with Primefaces > >>> again I have realised that something > >>> > >>> was > >>> > >>> working with Mojarra, but not > >>> with MyFaces. Again, is the > >>> ResponseWriter.startCDATA stuff > >>> which Primefaces invokes directly on > >>> > >>> its > >>> > >>> main phase listener. > >>> > >>> However, reading the javadocs: > >>> > >>> > https://javaserverfaces.dev.java.net/nonav/docs/2.0/javadocs/index.html > >>> > >>> It says that method "should > >>> take no action when invoked"... > >>> which > >>> > >>> means > >>> > >>> that it should be completely > >>> empty as far as I understand. If > >>> that was > >>> > >>> the > >>> > >>> case, we would get the same > >>> behaviour in both implementations... > >>> > >>> Cheers, > >>> > >>> Bruno > >>> > >>> > >>> > >>> > >>> -- > >>> Matthias Wessendorf > >>> > >>> blog: > >>> http://matthiaswessendorf.wordpress.com/ > >>> sessions: > >>> http://www.slideshare.net/mwessendorf > >>> twitter: http://twitter.com/mwessendorf > >>> > >>> > >>> > >>> > >>> -- > >>> Matthias Wessendorf > >>> > >>> blog: > >>> http://matthiaswessendorf.wordpress.com/ > >>> sessions: > >>> http://www.slideshare.net/mwessendorf > >>> twitter: http://twitter.com/mwessendorf > >>> > >>> > >>> > >>> > >>> -- > >>> > >>> Matthias Wessendorf > >>> > >>> blog: http://matthiaswessendorf.wordpress.com/ > >>> sessions: http://www.slideshare.net/mwessendorf > >>> twitter: http://twitter.com/mwessendorf > >>> > >>> > >>> > >>> > >>> > >>> > >>> > >>> > >>> > >>> > >>> > >>> > >> > >> > >> > > > > > > > > > > -- > Matthias Wessendorf > > blog: http://matthiaswessendorf.wordpress.com/ > sessions: http://www.slideshare.net/mwessendorf > twitter: http://twitter.com/mwessendorf >
