http://gwt-code-reviews.appspot.com/771801/diff/25001/26004 File user/src/com/google/gwt/safehtml/shared/OnlyToBeUsedInGeneratedCodeStringBlessedAsSafeHtml.java (right):
http://gwt-code-reviews.appspot.com/771801/diff/25001/26004#newcode31 user/src/com/google/gwt/safehtml/shared/OnlyToBeUsedInGeneratedCodeStringBlessedAsSafeHtml.java:31: this.html = html; Enforce html != null per class contract (throw an IAE or NPE) http://gwt-code-reviews.appspot.com/771801/diff/25001/26004#newcode45 user/src/com/google/gwt/safehtml/shared/OnlyToBeUsedInGeneratedCodeStringBlessedAsSafeHtml.java:45: if (this == obj) { Missing obj == null case http://gwt-code-reviews.appspot.com/771801/diff/25001/26004#newcode47 user/src/com/google/gwt/safehtml/shared/OnlyToBeUsedInGeneratedCodeStringBlessedAsSafeHtml.java:47: } else if (getClass() != obj.getClass()) { It's better to do if (!(obj instanceof SafeHtml): 1) it avoids the overhead of creating class objects 2) it deals with subclassing correctly -- the SafeHtml contract allows instances of different implementing classes to compare as equal 3) it includes a null check for free (since null instanceof X always returns false) http://gwt-code-reviews.appspot.com/771801/diff/25001/26004#newcode49 user/src/com/google/gwt/safehtml/shared/OnlyToBeUsedInGeneratedCodeStringBlessedAsSafeHtml.java:49: } else if (!html.equals(((SafeHtml) obj).asString())) { Replace last else if with: } else { return html.equals(((SafeHtml) obj).asString()) } http://gwt-code-reviews.appspot.com/771801/diff/25001/26004#newcode53 user/src/com/google/gwt/safehtml/shared/OnlyToBeUsedInGeneratedCodeStringBlessedAsSafeHtml.java:53: } So the complete method looks like: public boolean equals(Object obj) { if (this == obj) { return true; } if (!(obj instanceof SafeHtml)) { return false; } return html.equals(((SafeHtml) obj).asString()); } The first == test could be skipped without affecting correctness -- it's hard to say whether it will be a net performance win in practice. http://gwt-code-reviews.appspot.com/771801/diff/25001/26006 File user/src/com/google/gwt/safehtml/shared/SafeHtmlBuilder.java (right): http://gwt-code-reviews.appspot.com/771801/diff/25001/26006#newcode28 user/src/com/google/gwt/safehtml/shared/SafeHtmlBuilder.java:28: * {...@link #appendHtmlConstant(String)}) requires a convention of use to be specify the convention here or at least summarize it http://gwt-code-reviews.appspot.com/771801/diff/25001/26007 File user/src/com/google/gwt/safehtml/shared/SafeHtmlString.java (right): http://gwt-code-reviews.appspot.com/771801/diff/25001/26007#newcode40 user/src/com/google/gwt/safehtml/shared/SafeHtmlString.java:40: this.html = html; Enforce non-null http://gwt-code-reviews.appspot.com/771801/diff/25001/26007#newcode61 user/src/com/google/gwt/safehtml/shared/SafeHtmlString.java:61: if (this == obj) { See previous comment on style for equals methods http://gwt-code-reviews.appspot.com/771801/diff/25001/26008 File user/src/com/google/gwt/safehtml/shared/SafeHtmlUtils.java (right): http://gwt-code-reviews.appspot.com/771801/diff/25001/26008#newcode80 user/src/com/google/gwt/safehtml/shared/SafeHtmlUtils.java:80: * {...@link gwt-user/core/super/com/google/gwt/emul/java/lang/String} Be more specific here or don't say anything -- the comment is not really useful as-is. http://gwt-code-reviews.appspot.com/771801/diff/25001/26008#newcode87 user/src/com/google/gwt/safehtml/shared/SafeHtmlUtils.java:87: if (s.indexOf("&") != -1) { You might want to profile using com.google.gwt.regexp.shared.RegExp to allow you to pre-compile the constant regular expressions (even thought they are single chars they still go through a regex code path). This would look something like: com.google.gwt.regexp.shared.RegExp; private static final RegExp AMP_REGEX = RegExp.compile("&", "g"); // ... s = AMP_REGEX.replace(s, "&"); This turns into very straightforward JavaScript. Another approach would be to scan the input character by character and append to an output StringBuilder when one of the special input chars is detected. This would avoid a lot of new string creation and multiple passes through the input, like: StringBuilder sb = new StringBuilder(); for (int i = 0, len = s.length(); i < len; i++) { char c = s.charAt(i); switch (c) { case '&': sb.append("&"); break; case '\"': sb.append("""); break; case '\'': sb.append("'"); break; case '<': sb.append("<"); break; case '>': sb.append(">"); break; default: sb.append(c); } } return sb.toString(); if you want to be a bit slicker, instead of appending each char you could keep track of sections that don't contain special chars and append the whole chunk at once. I guess I would profile a bit to see if it matters. http://gwt-code-reviews.appspot.com/771801/diff/25001/26008#newcode120 user/src/com/google/gwt/safehtml/shared/SafeHtmlUtils.java:120: for (String segment : text.split("&", -1)) { Again, RegExp can perform split more efficiently where the split regex is constant. http://gwt-code-reviews.appspot.com/771801/diff/25001/26009 File user/src/com/google/gwt/safehtml/shared/SimpleHtmlSanitizer.java (right): http://gwt-code-reviews.appspot.com/771801/diff/25001/26009#newcode54 user/src/com/google/gwt/safehtml/shared/SimpleHtmlSanitizer.java:54: return new SafeHtmlString(simpleSanitize(html)); Enforce html != null http://gwt-code-reviews.appspot.com/771801/diff/25001/26009#newcode78 user/src/com/google/gwt/safehtml/shared/SimpleHtmlSanitizer.java:78: // input string starts Use /* comments and Eclipse formatting for multi-line comments http://gwt-code-reviews.appspot.com/771801/diff/25001/26010 File user/src/com/google/gwt/safehtml/shared/UriUtils.java (right): http://gwt-code-reviews.appspot.com/771801/diff/25001/26010#newcode62 user/src/com/google/gwt/safehtml/shared/UriUtils.java:62: String scheme = extractScheme(uri); There's a subtle problem with equalsIgnoreCase that can cause it to fail for strings containing the letter "i" running in a Turkish locale (see http://cafe.elharo.com/blogroll/turkish/). You might need to use a trick to get "mailto" to work correctly. http://gwt-code-reviews.appspot.com/771801/diff/25001/26015 File user/test/com/google/gwt/safehtml/shared/GwtSafeHtmlStringTest.java (right): http://gwt-code-reviews.appspot.com/771801/diff/25001/26015#newcode40 user/test/com/google/gwt/safehtml/shared/GwtSafeHtmlStringTest.java:40: assertFalse(safe1.hashCode() == safe3.hashCode()); Assert safe1.hashCode() == "stringsame".hashCode? Also, it's better to use: assertEquals(expected, actual) rather than: assertTrue(expected == actual) because the failure message will provide better information http://gwt-code-reviews.appspot.com/771801/diff/25001/26016 File user/test/com/google/gwt/safehtml/shared/GwtSafeHtmlUtilsTest.java (right): http://gwt-code-reviews.appspot.com/771801/diff/25001/26016#newcode23 user/test/com/google/gwt/safehtml/shared/GwtSafeHtmlUtilsTest.java:23: public class GwtSafeHtmlUtilsTest extends GWTTestCase { It would be a good idea to use Emma to improve the code coverage of this test. See: http://code.google.com/p/google-web-toolkit/wiki/EmmaSupport http://gwt-code-reviews.appspot.com/771801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
