Return-Path: 
 <dev-return-42622-apmail-myfaces-dev-archive=myfaces.apache.org@myfaces.apache.org>
Delivered-To: apmail-myfaces-dev-archive@www.apache.org
Received: (qmail 95765 invoked from network); 29 Dec 2009 01:12:07 -0000
Received: from hermes.apache.org (HELO mail.apache.org) (140.211.11.3)
  by minotaur.apache.org with SMTP; 29 Dec 2009 01:12:07 -0000
Received: (qmail 52163 invoked by uid 500); 29 Dec 2009 01:12:06 -0000
Delivered-To: apmail-myfaces-dev-archive@myfaces.apache.org
Received: (qmail 52077 invoked by uid 500); 29 Dec 2009 01:12:06 -0000
Mailing-List: contact dev-help@myfaces.apache.org; run by ezmlm
Precedence: bulk
List-Help: <mailto:dev-help@myfaces.apache.org>
List-Unsubscribe: <mailto:dev-unsubscribe@myfaces.apache.org>
List-Post: <mailto:dev@myfaces.apache.org>
List-Id: <dev.myfaces.apache.org>
Reply-To: "MyFaces Development" <dev@myfaces.apache.org>
Delivered-To: mailing list dev@myfaces.apache.org
Received: (qmail 52069 invoked by uid 99); 29 Dec 2009 01:12:06 -0000
Received: from athena.apache.org (HELO athena.apache.org) (140.211.11.136)
    by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 29 Dec 2009 01:12:06 +0000
X-ASF-Spam-Status: No, hits=-6.6 required=5.0
	tests=AWL,BAYES_00,RCVD_IN_DNSWL_MED
X-Spam-Check-By: apache.org
Received-SPF: pass (athena.apache.org: local policy)
Received: from [141.146.126.233] (HELO acsinet11.oracle.com) (141.146.126.233)
    by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 29 Dec 2009 01:11:57 +0000
Received: from acsinet15.oracle.com (acsinet15.oracle.com [141.146.126.227])
	by acsinet11.oracle.com (Switch-3.4.2/Switch-3.4.1) with ESMTP id
 nBT1BYiE023828
	(version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK);
	Tue, 29 Dec 2009 01:11:35 GMT
Received: from acsmt356.oracle.com (acsmt356.oracle.com [141.146.40.156])
	by acsinet15.oracle.com (Switch-3.4.2/Switch-3.4.1) with ESMTP id
 nBT0oVAH017261;
	Tue, 29 Dec 2009 01:11:32 GMT
Received: from abhmt009.oracle.com by acsmt355.oracle.com
	with ESMTP id 1251116901262049075; Mon, 28 Dec 2009 17:11:15 -0800
Received: from [130.35.103.98] (/130.35.103.98)
	by default (Oracle Beehive Gateway v4.0)
	with ESMTP ; Mon, 28 Dec 2009 17:11:15 -0800
Message-ID: <4B395725.1020403@oracle.com>
Date: Mon, 28 Dec 2009 17:11:01 -0800
From: Blake Sullivan <blake.sullivan@oracle.com>
User-Agent: Thunderbird 2.0.0.23 (Windows/20090812)
MIME-Version: 1.0
To: MyFaces Development <dev@myfaces.apache.org>
CC: Marius Petoi <marius.petoi@codebeat.ro>
Subject: Re: [Trinidad][Skinning]Support @locale rules (TRINIDAD-1041)
References: <16f5365c0912090457n3aca3b84g800e37641697bcfc@mail.gmail.com>
	 <4B206FF7.3050205@oracle.com>
 <16f5365c0912092345l21441f16qbb24fe6795521171@mail.gmail.com>
 <4B39445B.5060706@oracle.com>
In-Reply-To: <4B39445B.5060706@oracle.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
X-Source-IP: acsmt356.oracle.com [141.146.40.156]
X-Auth-Type: Internal IP
X-CT-RefId: str=0001.0A090203.4B395745.0165:SCFMA4539814,ss=1,fgs=0

Also, this code in SkinCSSDocumentHandler.java:

      _locales = locales != null ? new HashSet<Locale>(locales)
            : new HashSet<Locale>();

If we aren't going to modify the HashSet(), use Collections.emptySet().  
When JIRA-1547 is applied, CollectionUtils will ahve a method that 
optimizes this case and the single element cases.

SkinStyleSheetNode--it doesn't make sense to have a getLocales() that 
converts the Set to an array just so that StyleSheetNode can convert the 
array back into a set--we should chanage StyleSheetNode to take a 
Set<Locale>.

-- Blake Sullivan


Jeanne Waldman said the following On 12/28/2009 3:50 PM PT:
> Here is a code review that is based on formatting only. The code itself looks 
> fine, but I need to review it more thoroughly.
> Also, you'll need to document this feature in the skinning.xml file.
>
> *SkinCSSDocumentHandler:*
>
> 1. Keep the imports:
>  import java.util.ArrayList;
> import java.util.HashMap;
> import java.util.HashSet;
> import java.util.LinkedList;
> import java.util.List;
> import java.util.Map;
> import java.util.Set;
> This is convention. We don't use * in our imports
>
> 2. Check indentation, throughout the code you added, the indentation doesn't 
> match the rest of the file.
>       } else if (atRule.startsWith(_AT_LOCALE))
>       {
>         _parseCustomAtRule(_AT_LOCALE, atRule);
>       }
>
> 3. Check indentation:
>     else if (_AT_LOCALE.equals(type))
>       _locales = null;
>
>       else if (_AT_LOCALE.equals(type)) {
>         _locales = new HashSet<Locale>();
>         for (int i = 0; i < typeArray.length; i++) {
>             Locale locale = LocaleUtils
>                 .getLocaleForIANAString(typeArray[i].replace('_',
>                     '-').trim());
>             _locales.add(locale);
>         }
>       }
>
> 4. brackets '{' '}' need to be on their own line to fit the style we use in all 
> the files
>
> 5. add parens around (locales != null)
>       _locales = locales != null ? new HashSet<Locale>(locales)
>             : new HashSet<Locale>();
>
> 6. indentation
>     public Set<Locale> getLocales()
>     {
>       return _locales;
>     }
>
> *SkinStyleSheetNode.java*
>
> 1. java.util imports, do not use *
>
> 2. indentation and brackets on their own line:
>           boolean localeMatch = _setsEqual(locales, _locales);
>           if (localeMatch) {
>             boolean accMatch = _setsEqual(accProperties, _accProperties);
>
>             if (accMatch)
>               return true;
>           }
> *
> SkinStyleSheetParserUtils.java*
> 1. remove the comment that says locales are not supported.
>
> Marius Petoi wrote, On 12/9/2009 11:45 PM PT:
> > Hi Jeanne,
> >
> > Everything that you mentioned here is done, so I think it will be ok. Please 
> > have a look over the code and tell me whether it is fine and then I shall send 
> > another email with the voting proposal for the public API.
> >
> > Marius
> >
> > On Thu, Dec 10, 2009 at 5:50 AM, Jeanne Waldman <jeanne.waldman@oracle.com 
> > <mailto:jeanne.waldman@oracle.com>> wrote:
> >
> >     I'll take a look. We should get approval from the community for @locale,
> >     because this is a public API.
> >     Could you send out another email with the subject set so people know they
> >     should vote on the public API? Or you can wait until I review this first.
> >
> >     +1 from me for @locale.
> >
> >     In XSS we had this syntax:
> >     <styleSheet locales="en">
> >     <styleSheet browsers="ie">
> >
> >     We converted the <styleSheet browsers..> syntax to @agent ie or @agent ie,
> >     gecko, so I think we should convert the locales to @locale with commas
> >     separating the locales. We should support the locales the same way we do
> >     for xss, so the code change should be pretty simple, since we already have
> >     the support for xss to follow.
> >     The locale should be included for the hashcode in the css filename
> >     (stylesheetdocumentid)
> >
> >     Jeanne
> >
> >     Marius Petoi wrote, On 12/9/2009 4:57 AM PT:
> >
> >         Hello,
> >
> >         I have added a patch that implements the support for skin selectors
> >         depending on the locale
> >         (https://issues.apache.org/jira/browse/TRINIDAD-1041). This is done in
> >         the CSS using @locale rules. The syntax of the @locale rules is
> >         similar to the syntax of the @agent and @platform rules:
> >
> >         @locale en-us, fr
> >         {
> >             /* skin selectors definitions go here */
> >         }
> >
> >         The set of supported locales is afterwards stored in each
> >         StyleSheetNode. I also implemented an example in the trinidad-demo.
> >         There is a skin (localeDemo.css) which contains @locale rules.
> >         Afterwards, the localeDemo.jspx page displays a textInput with a
> >         different color, depending on the locale. In order to configure the
> >         proper skin, a "LocaleDemoBean" is used, which is used for configuring
> >         the proper skin-family (localeDemo), in the same way in which the
> >         accessibility profile is configured for accessibilityProfileDemo.jspx.
> >
> >         Could you please have a look over the patch and see if this resolves
> >         the issue.
> >
> >         Regards,
> >         Marius
> >
> >
>