Return-Path: Delivered-To: apmail-geronimo-dev-archive@www.apache.org Received: (qmail 96262 invoked from network); 10 Dec 2010 15:44:55 -0000 Received: from unknown (HELO mail.apache.org) (140.211.11.3) by 140.211.11.9 with SMTP; 10 Dec 2010 15:44:55 -0000 Received: (qmail 36351 invoked by uid 500); 10 Dec 2010 15:44:54 -0000 Delivered-To: apmail-geronimo-dev-archive@geronimo.apache.org Received: (qmail 35998 invoked by uid 500); 10 Dec 2010 15:44:54 -0000 Mailing-List: contact dev-help@geronimo.apache.org; run by ezmlm Precedence: bulk list-help: list-unsubscribe: List-Post: Reply-To: dev@geronimo.apache.org List-Id: Delivered-To: mailing list dev@geronimo.apache.org Received: (qmail 35984 invoked by uid 99); 10 Dec 2010 15:44:53 -0000 Received: from athena.apache.org (HELO athena.apache.org) (140.211.11.136) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 10 Dec 2010 15:44:53 +0000 X-ASF-Spam-Status: No, hits=-0.7 required=10.0 tests=FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS X-Spam-Check-By: apache.org Received-SPF: pass (athena.apache.org: domain of sethfromaustria@gmail.com designates 74.125.82.50 as permitted sender) Received: from [74.125.82.50] (HELO mail-ww0-f50.google.com) (74.125.82.50) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 10 Dec 2010 15:44:48 +0000 Received: by wwf26 with SMTP id 26so3602022wwf.31 for ; Fri, 10 Dec 2010 07:44:27 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:sender:received :in-reply-to:references:date:x-google-sender-auth:message-id:subject :from:to:cc:content-type:content-transfer-encoding; bh=QOUoLJaTNAVYWADGQ+ofVutppnLEXyhJ3jnjainPVag=; b=uHIeLsL8f6BmzVRtV5tMUR7/VPS9oUh4SjrlZzizQ0vnY1ADroY6YVjdZGRKUKFw3h yYDrjxweot/PTjCHnkTT6JvzYfJlVOjQJY+4IGnW05q3donBzw5BoVZvlu75EKOmGbSz +FfBEpXcdOjzWSVsxjc2vxZOtXQIg53aZdI2g= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :content-transfer-encoding; b=hdd9QwAuE5OqzVcS79B98A2zQoWx2e8FJ4FQSUy/9X5w3qAYYBiJDUvVyKa6GIfM/X lFt2Zq3lgMCtTSJT4a4wq/NI85Btb8BQGznMclmcH2ipG3XVKmzexQ3oofy4o8guMmJx 5QapnVcUilX6xl/zHSXkjamI0WEWZnHsCF2A0= MIME-Version: 1.0 Received: by 10.216.172.206 with SMTP id t56mr2470902wel.66.1291995866532; Fri, 10 Dec 2010 07:44:26 -0800 (PST) Sender: sethfromaustria@gmail.com Received: by 10.216.53.78 with HTTP; Fri, 10 Dec 2010 07:44:25 -0800 (PST) In-Reply-To: References: <0E901BAB-10FB-4263-ACBE-6247F61F41AF@yahoo.com> <2B5C75DA-830D-4343-8DC9-F9DDE57A1AAA@yahoo.com> <9E004281-2991-406F-82A1-D3497681E366@yahoo.com> Date: Fri, 10 Dec 2010 16:44:25 +0100 X-Google-Sender-Auth: _LFh4Xs8m2N75KnebMizSJeK9m4 Message-ID: Subject: Re: ordering tck failures in geronimo From: Jakob Korherr To: MyFaces Development Cc: dev@geronimo.apache.org Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Hi, OK, great! I added another comment to MYFACES-2945 explaining why I did it this way. Please tell me your final opinion after reading this comment and I'll commit an appropriate version of the proposed patch! Regards, Jakob 2010/12/9 Leonardo Uribe : > Hi > > I think it is not necessary to pass FacesConfigurationProvider as param f= or > getFacesConfigData, because in theory we don't do anything with it before > pass it and wrappers will not do anything with it later. I think it looks > good to load it using > FacesConfigurationProviderFactory.getFacesConfigurationProvider(ExternalC= ontext). > > The other parts of the patch looks good. > > regards, > > Leonardo Uribe > > 2010/12/9 Jakob Korherr >> >> Hi guys, >> >> I just uploaded a patch for a FacesConfigurationMerger SPI: >> MYFACES-2945-FacesConfigurationMerger.patch >> >> Furthermore I added a quick code sample as comment on the MYFACES-2945 >> issue about how Geronimo can use this new SPI. >> >> Please take a look at the patch and if there are no objections, I will >> commit it soon! >> >> Regards, >> Jakob >> >> 2010/12/9 Jakob Korherr : >> > Hi, >> > >> > I called it ugly, because of its implementation code in >> > DefaultFacesConfigurationProvider: The method is already inside of a >> > FacesConfigurationProvider, but it does this: >> > >> > FacesConfigurationProvider provider =3D FacesConfigurationProviderFact= ory. >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0getFacesConfigurationProvider= Factory(_externalContext). >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0getFacesConfigu= rationProvider(_externalContext); >> > >> > ...and then calls all the other methods of FacesConfigurationProvider >> > on this provider instance. >> > >> > As I said, I know this is this way, because FacesConfigurationProvider >> > can be wrapped, but IMHO it is really ugly. >> > >> > >> > The method used on MYFACES-2998 was a first approach to solve this >> > problem in a better way. However, I really like those two of your >> > suggestions: >> > >> > 1) Leo #2: Create another SPI interface for getFacesConfigData() >> > (please suggest a name for it, maybe >> > FacesConfigurationMergerProvider?) and remove this method form >> > FacesConfigurationProvider. >> > 2) Ivan: In a few words: let getFacesConfigData() on >> > FacesConfigurationProvider, but provide an >> > AbstractFacesConfigurationProvider which implements the merging and >> > sorting to let custom SPI impls take advantage of it. >> > >> > At first, I really liked Ivan's proposal, but after thinking more >> > about it, it is not consistent with what we have right now (we do not >> > provide any other Abstract-SPI impl) and we would have the huge >> > merging and sorting code all in the SPI(-api) package, but IMO it >> > should really go into o.a.m.config. >> > >> > Thus I think that a new FacesConfigurationMergerProvider-SPI as Leo >> > proposed is the best choice here. Note that for this SPI it is good >> > practice to use other SPI impls. >> > >> > I will provide a patch for it soon and then we can discuss it further! >> > >> > Regards, >> > Jakob >> > >> > 2010/12/9 Ivan : >> >> my 2 cents, it seems for me less urgly ... >> >> a. For the FacesConfigurationProvider , it is better to have only one >> >> method >> >> getFacesConfigData() >> >> b. Create another abstract class AbstractFacesConfigurationProvider >> >> which >> >> extends FacesConfigurationProvider, and define those proctected metho= ds >> >> of >> >> get***, also place those sorting/merging codes there. >> >> c. In the DefaultFacesConfigurationProvider, it only implements those >> >> get*** >> >> methods. >> >> thanks. >> >> >> >> 2010/12/9 Leonardo Uribe >> >>> >> >>> Hi >> >>> >> >>> I agree with Jakob about faces-config merging and ordering algorithm >> >>> should not be exposed by MyFaces. Why is it not enough?. At this poi= nt >> >>> it is >> >>> not clear the reasons. Note in this moment ordering and sorting >> >>> algoritm it >> >>> is not being exposed by FacesConfigurationProvider interface. >> >>> >> >>> Other different thing is >> >>> FacesConfigurationProvider.getFacesConfigData(). >> >>> The intention of this method is provide a way to get a Serializable >> >>> object >> >>> that represents all config information required to initialize MyFace= s >> >>> and >> >>> allow container to store it temporally somewere. In this way it is >> >>> possible >> >>> to deploy and undeploy an application more quickly, because if >> >>> "nothing >> >>> changes"(no added dependencies, no update from faces-config.xml file= s >> >>> or >> >>> classes) this information is always the same. >> >>> >> >>> On MYFACES-2945 and previous discussions it was proposed two differe= nt >> >>> options: >> >>> >> >>> 1. Add getFacesConfigData() >> >>> 2. Add a set of methods to retrieve FacesConfig objects instead. >> >>> >> >>> =C2=A0=C2=A0=C2=A0 public abstract FacesConfig getStandardFacesConfi= g(ExternalContext >> >>> ectx); >> >>> =C2=A0=C2=A0=C2=A0 public abstract FacesConfig >> >>> getMetaInfServicesFacesConfig(ExternalContext ectx); >> >>> =C2=A0=C2=A0=C2=A0 public abstract FacesConfig >> >>> getAnnotationsFacesConfig(ExternalContext >> >>> ectx, boolean metadataComplete); >> >>> =C2=A0=C2=A0=C2=A0 public abstract List >> >>> getClassloaderFacesConfig(ExternalContext ectx); >> >>> =C2=A0=C2=A0=C2=A0 public abstract List >> >>> getContextSpecifiedFacesConfig(ExternalContext ectx); >> >>> =C2=A0=C2=A0=C2=A0 public abstract FacesConfig getWebAppFacesConfig(= ExternalContext >> >>> ectx); >> >>> >> >>> The first option has the advantage that it fill the initial >> >>> requeriment >> >>> without add more complexity to the problem. The second one seems to = be >> >>> more >> >>> structured and opens the possibility to do other things like how >> >>> override >> >>> MyFaces parsing for faces-config.xml files like it is being discusse= d. >> >>> If >> >>> the container parse faces-config.xml files, with the previous method= s >> >>> it is >> >>> possible to prevent parse the same files twice. >> >>> >> >>> My first intention, as is shown on MYFACES-2945 was that >> >>> FacesConfigurationProvider does not included getFacesConfigData(), >> >>> because >> >>> it is possible to fill the initial objective with these methods, but >> >>> finally >> >>> it was agreed the first option looks better, right? >> >>> >> >>> Now I see that on MYFACES-2998 that fact is questioned: >> >>> >> >>> JK>> Unfortunately it also provides a method that should combine all >> >>> these >> >>> data: getFacesConfigData(). >> >>> JK>> This method is here due to refactorings, but IMHO this is the >> >>> last >> >>> place where it should be put, >> >>> JK>> because it gets "its own implementation" via its Factory and th= en >> >>> calls all of the above methods on >> >>> JK>> it. I know this was introduced to support wrapping the default >> >>> impl, >> >>> but it really is very, very ugly. >> >>> >> >>> In few words, why does it looks ugly? or with other words, what can = we >> >>> do >> >>> to make it cleaner? remove it? or just provide another SPI interface >> >>> and put >> >>> that method there? In practice, getFacesConfigData() merges all >> >>> FacesConfig >> >>> information, and "on the way" it does order applicationFacesConfig >> >>> files >> >>> (the ones obtained from getClassloaderFacesConfig() and >> >>> getContextSpecifiedFacesConfig() ) . To do that it requires to call >> >>> all six >> >>> methods from FacesConfigurationProvider, there is no other way, so I >> >>> don't >> >>> see why do that is considered ugly. >> >>> >> >>> At this moment we have the following courses of action: >> >>> >> >>> 1. Remove FacesConfigurationResource interface partially, because it >> >>> is >> >>> still too inmature and let it for myfaces core 2.0.4. >> >>> 2. Create another SPI interface for getFacesConfigData() (please >> >>> suggest a >> >>> name for it, maybe FacesConfigurationMergerProvider?) and remove thi= s >> >>> method >> >>> form FacesConfigurationResource. Apply the patch on MYFACES-2998 see= ms >> >>> to be >> >>> in this direction, but forget the reason why it is wanted to expose >> >>> getFacesConfigData() to the container. >> >>> 3. Apply something like MYFACES-2998 patch, and refactor this one >> >>> later in >> >>> myfaces core 2.0.4 >> >>> >> >>> Suggestions are welcome. >> >>> >> >>> regards, >> >>> >> >>> Leonardo Uribe >> >>> >> >>> >> >>> >> >>> >> >> >> >> >> >> >> >> -- >> >> Ivan >> >> >> > >> > >> > >> > -- >> > Jakob Korherr >> > >> > blog: http://www.jakobk.com >> > twitter: http://twitter.com/jakobkorherr >> > work: http://www.irian.at >> > >> >> >> >> -- >> Jakob Korherr >> >> blog: http://www.jakobk.com >> twitter: http://twitter.com/jakobkorherr >> work: http://www.irian.at > > --=20 Jakob Korherr blog: http://www.jakobk.com twitter: http://twitter.com/jakobkorherr work: http://www.irian.at