Yeah Werner, you should take holidays seriously :) Thanks! Bruno
On 23 July 2010 13:28, Werner Punz <[email protected]> wrote: > Hi I have written a load of tests for the PPR responsewriter to have it > covered, but I will do some additional testing mid next week (and will > commit those tests) if the ppr responsewriter still behaves as it should > after the patch. > Since we are not in the middle of another release I can guess the > additional testing on the ppr responsewriter can wait a little bit. > If anything negative comes out I probably can fix it on the ppr writers > side. > > > > Werner > > > Am 23.07.10 13:05, schrieb Matthias Wessendorf: > > re-tested; works fine with your patch as well >> >> On Fri, Jul 23, 2010 at 11:37 AM, Bruno Aranda<[email protected]> >> wrote: >> >>> 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 >>>> >>> >>> >>> >> >> >> > >
