Mostly LGTM. Aside from individuaal comments, can you clarify how these different estimators will be used? It seems like most code is just going to use the default.
http://gwt-code-reviews.appspot.com/338801/diff/1/4 File user/src/com/google/gwt/i18n/shared/DirectionEstimator.java (right): http://gwt-code-reviews.appspot.com/338801/diff/1/4#newcode32 user/src/com/google/gwt/i18n/shared/DirectionEstimator.java:32: * Estimates the direction of a string, assuming it's clean (doesn't contain Instead of "clean", how about: Estimates the direction of a plain-text string. http://gwt-code-reviews.appspot.com/338801/diff/1/4#newcode44 user/src/com/google/gwt/i18n/shared/DirectionEstimator.java:44: * @param isHtml Whether {...@code str} is HTML / HTML-escaped. Should clarify that it is plain text if isHtml is false. http://gwt-code-reviews.appspot.com/338801/diff/1/6 File user/src/com/google/gwt/i18n/shared/HasDirectionEstimator.java (right): http://gwt-code-reviews.appspot.com/338801/diff/1/6#newcode26 user/src/com/google/gwt/i18n/shared/HasDirectionEstimator.java:26: DirectionEstimator getDirectionEstimator(); Add @return. http://gwt-code-reviews.appspot.com/338801/diff/1/6#newcode37 user/src/com/google/gwt/i18n/shared/HasDirectionEstimator.java:37: void setDirectionEstimator(boolean enabled); Add @param enabled. http://gwt-code-reviews.appspot.com/338801/diff/1/6#newcode42 user/src/com/google/gwt/i18n/shared/HasDirectionEstimator.java:42: * For {...@code null} input, this should turn off direction estimation. Make this an @param entry. http://gwt-code-reviews.appspot.com/338801/diff/1/7 File user/src/com/google/gwt/i18n/shared/WordCountDirectionEstimator.java (right): http://gwt-code-reviews.appspot.com/338801/diff/1/7#newcode28 user/src/com/google/gwt/i18n/shared/WordCountDirectionEstimator.java:28: private static final WordCountDirectionEstimator instance = GWT style is to use all-caps INSTANCE here. http://gwt-code-reviews.appspot.com/338801/diff/1/9 File user/test/com/google/gwt/i18n/shared/AnyRtlDirectionEstimatorTest.java (right): http://gwt-code-reviews.appspot.com/338801/diff/1/9#newcode25 user/test/com/google/gwt/i18n/shared/AnyRtlDirectionEstimatorTest.java:25: private static AnyRtlDirectionEstimator estimator = Would it be worth moving estimator to DirectionEstimatorTestBase (set in a super constructor call) and make each individual test just a set of expected results for various strings? http://gwt-code-reviews.appspot.com/338801/diff/1/9#newcode28 user/test/com/google/gwt/i18n/shared/AnyRtlDirectionEstimatorTest.java:28: private String containsRtlChar = enWord + digitsWord + wordWithOneRtlChar; final http://gwt-code-reviews.appspot.com/338801/diff/1/10 File user/test/com/google/gwt/i18n/shared/DirectionEstimatorTestBase.java (right): http://gwt-code-reviews.appspot.com/338801/diff/1/10#newcode25 user/test/com/google/gwt/i18n/shared/DirectionEstimatorTestBase.java:25: protected String enWord = " abc"; Make these final and named like EN_WORD? http://gwt-code-reviews.appspot.com/338801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors To unsubscribe, reply using "remove me" as the subject.
