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;
On 2010/08/19 18:35:55, Dan Rice wrote:
Enforce html != null per class contract (throw an IAE or NPE)
Done.
http://gwt-code-reviews.appspot.com/771801/diff/25001/26004#newcode45
user/src/com/google/gwt/safehtml/shared/OnlyToBeUsedInGeneratedCodeStringBlessedAsSafeHtml.java:45:
if (this == obj) {
On 2010/08/19 18:35:55, Dan Rice wrote:
Missing obj == null case
Done.
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()) {
On 2010/08/19 18:35:55, Dan Rice wrote:
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)
Done.
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())) {
On 2010/08/19 18:35:55, Dan Rice wrote:
Replace last else if with:
} else {
return html.equals(((SafeHtml) obj).asString())
}
Done.
http://gwt-code-reviews.appspot.com/771801/diff/25001/26004#newcode53
user/src/com/google/gwt/safehtml/shared/OnlyToBeUsedInGeneratedCodeStringBlessedAsSafeHtml.java:53:
}
On 2010/08/19 18:35:55, Dan Rice wrote:
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.
Done.
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
On 2010/08/19 18:35:55, Dan Rice wrote:
specify the convention here or at least summarize it
Done.
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;
On 2010/08/19 18:35:55, Dan Rice wrote:
Enforce non-null
Done.
http://gwt-code-reviews.appspot.com/771801/diff/25001/26007#newcode61
user/src/com/google/gwt/safehtml/shared/SafeHtmlString.java:61: if (this
== obj) {
On 2010/08/19 18:35:55, Dan Rice wrote:
See previous comment on style for equals methods
Done.
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}
On 2010/08/19 18:35:55, Dan Rice wrote:
Be more specific here or don't say anything -- the comment is not
really useful
as-is.
Done.
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) {
On 2010/08/19 18:35:55, Dan Rice wrote:
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.
Done.
You were correct, pre-compiled checks turned out to be faster. I
benchmarked the step-through-string method but it ended up being much
slower.
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)) {
On 2010/08/19 18:35:55, rice wrote:
Again, RegExp can perform split more efficiently where the split regex
is
constant.
Done.
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));
On 2010/08/19 18:35:55, Dan Rice wrote:
Enforce html != null
Done.
http://gwt-code-reviews.appspot.com/771801/diff/25001/26009#newcode78
user/src/com/google/gwt/safehtml/shared/SimpleHtmlSanitizer.java:78: //
input string starts
On 2010/08/19 18:35:55, Dan Rice wrote:
Use /* comments and Eclipse formatting for multi-line comments
Done.
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);
On 2010/08/19 18:35:55, Dan Rice wrote:
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.
I do not think this a problem for us. When we go to javascript, we end
up using javascript's toLowerCase(), which just does a character mapping
(ignoring locale... I->i.) Because the mailto string must be in ascii,
"maılto:" (with the Turkish 'ı') would be correctly rejected. Javascript
has a special method for locale-specific case conversion:
toLocaleLowerCase(), unlike Java, where toLowerCase() considers locale.
Given this, I think we can leave the current method.
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());
On 2010/08/19 18:35:55, Dan Rice wrote:
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
Done.
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());
On 2010/08/20 14:58:04, rice wrote:
You can avoid the assertFalse by rewriting the tests like this:
assertEquals("stringsame".hashCode(), safe1.hashCode());
assertEquals(safe1.hashCode(), safe2.hashCode());
assertEquals("stringdiff".hashCode(), safe3.hashCode());
You don't really need to assert that "stringsame" and "stringdiff"
have
different hash codes.
On 2010/08/19 18:35:55, rice wrote:
> 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
Done.
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 {
On 2010/08/19 18:35:55, rice wrote:
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
Done, and thank you for the link--Emma looks very useful!
The tests cover all paths of both htmlEscape() and
htmlEscapeAllowEntities().
I have no coverage of the SimpleHtmlSanitizer class though. Writing that
asap...
http://gwt-code-reviews.appspot.com/771801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
