cocoon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Marc Portier <...@outerthought.org>
Subject Re: cvs commit: cocoon-2.1/src/blocks/forms/java/org/apache/cocoon/forms/transformation EffectPipe.java
Date Fri, 09 Apr 2004 16:40:27 GMT


mpo@apache.org wrote:

> mpo         2004/04/09 09:36:00
> 
>   Modified:    src/blocks/forms/java/org/apache/cocoon/forms/transformation
>                         EffectPipe.java
>   Log:
>   Fixing the Element.addAttrubute(s). (in its various life-forms)
>   The implicit understanding of the 'add' of this method was to me:
>   - ADD if not present yet and
>   - OVERWRITE if already present.
>   the previous implementation delegated to AttributesImpl.setAttributes which is in fact
>   throwing away all existing attributes before adding the new ones.
>   
>   In combination with the previous patch this was breaking all samples that did not use
the form-action and
>   form-method parameters on forms-transformer (almost all of them)
>   
>   Revision  Changes    Path
>   1.4       +63 -30    cocoon-2.1/src/blocks/forms/java/org/apache/cocoon/forms/transformation/EffectPipe.java
>   
>   Index: EffectPipe.java
>   ===================================================================
>   RCS file: /home/cvs/cocoon-2.1/src/blocks/forms/java/org/apache/cocoon/forms/transformation/EffectPipe.java,v
>   retrieving revision 1.3
>   retrieving revision 1.4
>   diff -u -r1.3 -r1.4
>   --- EffectPipe.java	8 Apr 2004 09:38:55 -0000	1.3
>   +++ EffectPipe.java	9 Apr 2004 16:36:00 -0000	1.4
>   @@ -94,53 +94,86 @@
>                }
>            }
>    


Please also notice this important msg:

>    
>   -        public void claimAttributes() {
>   -            if (!mine) {
>   -                attrs = new AttributesImpl(attrs);
>   -                mine = true;
>   -            }
>   -        }
>   +        //TODO: IMPORTANT!!! 
>   +        // check this: when commenting out this stuff everything still compiles!
>   +        // this means that attributes on these objects are never explicitely "claimed"
>   +        // which means that these Element objects should never get referenced 
>   +        // outside the scope of the SAX event that creates them.
>   +        // Is that really the case?
>   +//        public void claimAttributes() {
>   +//            if (!mine) {
>   +//                attrs = new AttributesImpl(attrs);
>   +//                mine = true;
>   +//            }
>   +//        }
>        }
>    


IMHO it could not be the case: AFAIU the whole purpose of the efectpipe 
is to build a stack of these elements
that lives across SAX-EVENTS, this would mean that the current impl only 
works with a SAX parser impl that
allocates a new Attributes instance for each sax event (lucky us?)

In that light I'm doubthing if the added complexity of the late 
attribute-cloning offers us that much.
(doesn't it need to happen anyway?)

IMHO we should consider making the attrs final and thus cloning the lot 
at Element-constructot-time.

wdot?
-marc=
-- 
Marc Portier                            http://outerthought.org/
Outerthought - Open Source, Java & XML Competence Support Center
Read my weblog at                http://blogs.cocoondev.org/mpo/
mpo@outerthought.org                              mpo@apache.org

Mime
View raw message