Re: cvs commit: cocoon-2.1/src/java/org/apache/cocoon/sitemap SitemapComponentSelector.java

[Sylvain Wallez]

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 }