Mailing-List: contact dev-help@cocoon.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@cocoon.apache.org
Message-ID: <40F69627.8070108@apache.org>
Date: Thu, 15 Jul 2004 16:35:19 +0200
From: Sylvain Wallez <sylvain@apache.org>
User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US;
 rv:1.7) Gecko/20040616
MIME-Version: 1.0
To: dev@cocoon.apache.org
Subject: Re: cvs commit: cocoon-2.1/src/java/org/apache/cocoon/sitemap
 SitemapComponentSelector.java
References: <OFD74F173F.D96F76D8-ONC1256ED2.004DE179@sundn.de>
In-Reply-To: <OFD74F173F.D96F76D8-ONC1256ED2.004DE179@sundn.de>
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Transfer-Encoding: 7bit

Carsten Ziegeler wrote:

>Sylvain Wallez wrote: 
>  
>
>>Carsten Ziegeler wrote:
>>
>>    
>>
>>>Great work, Sylvain.
>>> 
>>>
>>>      
>>>
>>Thanks. Hard job as there were explicit dependencies to 
>>extensions of ComponentSelector (SitemapComponentSelector and 
>>OutputComponentSelector) in many places.
>>
>>These extensions were actually used only to store information 
>>internal to the sitemap (components mime-types and labels), 
>>so I handled this internally (see ProcessorComponentInfo) 
>>which allowed the removal of these extensions.
>>    
>>
>Sounds good, will look at it.
>  
>
>>There still is an ugly hack in ComponentsSelector to manage 
>>the parent/child chaining since WrapperServiceSelector and 
>>WrapperComponentSelector lack some public access to the 
>>wrapped selector. I had no time to locate where is the actual 
>>code (is it Avalon or Excalibur) nor start discussions about 
>>the patch.
>>    
>>
>I think we can leave it as it is. I plan to make the migration
>to Fortress sometime next week. Then this will be changed anyway.
>
>  
>
>>>Only one question: you also changed the mime-type setting (again):
>>>
>>>http://cvs.apache.org/viewcvs/cocoon-2.1/src/java/org/apache/
>>>      
>>>
>>cocoon/com
>>    
>>
>>>ponen
>>>ts/pipeline/AbstractProcessingPipeline.java.diff?r1=1.25&r2=1.26
>>>
>>>According to bug 10277 and to the votes we did, we only 
>>>      
>>>
>>discussed the 
>>    
>>
>>>mime type of the reader, not the mime-type of the serializer.
>>> 
>>>
>>>      
>>>
>>Well, how could the mime-type handling be different for 
>>reader and serializer?
>>
>>    
>>
>Yes, I totally agree, but we never voted about it and this change
>is mentioned nowhere! So, at least it should be added to the
>updating documentation and in the status file.
>  
>

Ok.

>>Additionally, this priorty order seems to me the only one that really makes sense:
>>- first check on the <map:read> or <map:serialize> statement (most specific setting)
>>- then check on the <map:reader> or <map:serializer> component declaration (sitemap-wide setting)
>>- then ask the serializer instance (builtin, system-wide setting)
>>
>>I also wondered why the pipeline raises a ProcessingException if no mime type exist on the serializer. Shouldn't we let the servlet engine decide?
>>
>>    
>>
>>> <>In addition, it seems that you removed one case for the mime-type 
>>> setting of the reader
>>
>>Sorry, what are you referring to?
>>
>>    
>>
>To this change in AbstractProcessingPipeline:
>---------------
>@@ -557,8 +553,7 @@
> 
>this.reader.setup(this.processor.getSourceResolver(),environment.getObjectMo
>del(),readerSource,readerParam);
>             // Set the mime-type
>             // the behaviour has changed from 2.1.x to 2.2 according to bug
>#10277:
>-            // MIME type declared on the reader instance
>-            // MIME type declared for the reader component
>+            // MIME type declared in the sitemap (instance or declaration,
>in this order)
>             // Ask the Reader for a MIME type:
>             //     A *.doc reader could peek into the file
>             //     and return either text/plain or application/vnd.msword
>or
>@@ -566,8 +561,6 @@
>             //     by the server.
>             if ( this.readerMimeType != null ) {
>                 environment.setContentType(this.readerMimeType);
>-            } else if ( this.sitemapReaderMimeType != null ) {
>-                environment.setContentType(this.sitemapReaderMimeType);
>
>             } else {
>                 final String mimeType = this.reader.getMimeType();
>                 if (mimeType != null) {
>--------------
>
>
>
>You removed one else-if statement. I haven't looked at all your changes, perhaps you're doing the correct thing somewhere else. I just want to make sure that the implementation is still correct :)
>  
>

Actually, this is a side effect of the removal of 
OutputComponentSelector: the mimeType given to addReader and 
addSerializer is the one defined in the sitemap, be it on the particular 
statement of the default setting of the component declaration. This is 
implemented in SerializeNodeBuilder and ReadNodeBuilder.

Previously, the pipeline had to do the additional job of getting the 
mimeType from the OutputComponentSelector it none was present on the 
statement, while conceptually this really was (and is now!) the sole 
responsibility of the sitemap engine.

And if you look at the diff, the same if/else disappeared for the 
serializer ;-)

Sylvain

-- 
Sylvain Wallez                                  Anyware Technologies
http://www.apache.org/~sylvain           http://www.anyware-tech.com
{ XML, Java, Cocoon, OpenSource }*{ Training, Consulting, Projects }