On Fri, Jul 23, 2010 at 2:30 PM, Bruno Aranda <[email protected]> wrote: > Yeah Werner, you should take holidays seriously :)
+1 > 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 >>>> >>>> >>> >>> >>> >> >> > > -- Matthias Wessendorf blog: http://matthiaswessendorf.wordpress.com/ sessions: http://www.slideshare.net/mwessendorf twitter: http://twitter.com/mwessendorf
