[gwt-contrib] Re: Add SafeHtml support to ui widgets. (issue829801)
Done.
I apologize about the duplicate/overlapping reviews, but I've rolled the
results of this review into:
http://gwt-code-reviews.appspot.com/847801/show
http://gwt-code-reviews.appspot.com/829801/diff/8020/24004
File user/src/com/google/gwt/user/client/ui/CheckBox.java (right):
http://gwt-code-reviews.appspot.com/829801/diff/8020/24004#newcode265
user/src/com/google/gwt/user/client/ui/CheckBox.java:265: public void
setSafeHtml(SafeHtml html) {
On 2010/09/07 23:08:10, xtof wrote:
Don't we already inherit this from ButtonBase?
Done.
http://gwt-code-reviews.appspot.com/829801/diff/8020/24013
File user/test/com/google/gwt/user/client/ui/CheckBoxTest.java (right):
http://gwt-code-reviews.appspot.com/829801/diff/8020/24013#newcode226
user/test/com/google/gwt/user/client/ui/CheckBoxTest.java:226: CheckBox
box = new CheckBox(html);
On 2010/09/07 23:08:10, xtof wrote:
You could shorten this to
new CheckBox(SafeHtmlUtils.fromSafeConstant(b...
(here and elsewhere)
Done.
http://gwt-code-reviews.appspot.com/829801/diff/8020/24015
File user/test/com/google/gwt/user/client/ui/HTMLTest.java (right):
http://gwt-code-reviews.appspot.com/829801/diff/8020/24015#newcode43
user/test/com/google/gwt/user/client/ui/HTMLTest.java:43:
sb.appendHtmlConstant(html);
On 2010/09/07 23:08:10, xtof wrote:
It's a bit of a discouraged pattern to pass the value of local
variables to
appendHtmlConstant (since in a more complicated method you have to
squint at it
to be sure it's really a constant). Can you make this static final
constants?
(here and elsewhere)
Done.
http://gwt-code-reviews.appspot.com/829801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add SafeHtml support to ui widgets. (issue829801)
LGTM
http://gwt-code-reviews.appspot.com/829801/diff/8020/24004
File user/src/com/google/gwt/user/client/ui/CheckBox.java (right):
http://gwt-code-reviews.appspot.com/829801/diff/8020/24004#newcode265
user/src/com/google/gwt/user/client/ui/CheckBox.java:265: public void
setSafeHtml(SafeHtml html) {
Don't we already inherit this from ButtonBase?
http://gwt-code-reviews.appspot.com/829801/diff/8020/24013
File user/test/com/google/gwt/user/client/ui/CheckBoxTest.java (right):
http://gwt-code-reviews.appspot.com/829801/diff/8020/24013#newcode226
user/test/com/google/gwt/user/client/ui/CheckBoxTest.java:226: CheckBox
box = new CheckBox(html);
You could shorten this to
new CheckBox(SafeHtmlUtils.fromSafeConstant(b...
(here and elsewhere)
http://gwt-code-reviews.appspot.com/829801/diff/8020/24015
File user/test/com/google/gwt/user/client/ui/HTMLTest.java (right):
http://gwt-code-reviews.appspot.com/829801/diff/8020/24015#newcode43
user/test/com/google/gwt/user/client/ui/HTMLTest.java:43:
sb.appendHtmlConstant(html);
It's a bit of a discouraged pattern to pass the value of local variables
to appendHtmlConstant (since in a more complicated method you have to
squint at it to be sure it's really a constant). Can you make this
static final constants?
(here and elsewhere)
http://gwt-code-reviews.appspot.com/829801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add SafeHtml support to ui widgets. (issue829801)
http://gwt-code-reviews.appspot.com/829801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add SafeHtml support to ui widgets. (issue829801)
http://gwt-code-reviews.appspot.com/829801/diff/7001/8002
File user/src/com/google/gwt/user/User.gwt.xml (right):
http://gwt-code-reviews.appspot.com/829801/diff/7001/8002#newcode51
user/src/com/google/gwt/user/User.gwt.xml:51: inherits
name=com.google.gwt.safehtml.SafeHtml /
Maybe alphabetize?
http://gwt-code-reviews.appspot.com/829801/diff/7001/8004
File user/src/com/google/gwt/user/client/ui/CheckBox.java (right):
http://gwt-code-reviews.appspot.com/829801/diff/7001/8004#newcode70
user/src/com/google/gwt/user/client/ui/CheckBox.java:70: * Creates a
check box with the specified text label.
text - html or safe html
http://gwt-code-reviews.appspot.com/829801/diff/7001/8005
File user/src/com/google/gwt/user/client/ui/HTML.java (right):
http://gwt-code-reviews.appspot.com/829801/diff/7001/8005#newcode45
user/src/com/google/gwt/user/client/ui/HTML.java:45: public class HTML
extends Label implements HasDirectionalSafeHtml {
Do you mean to remove HasDirectionalHTML?
In general, changes to interfaces can cause a lot of compatibility
problems -- is there a strategy on how to deprecate the raw HTML
interfaces without breaking all users at once?
http://gwt-code-reviews.appspot.com/829801/diff/7001/8005#newcode85
user/src/com/google/gwt/user/client/ui/HTML.java:85:
setHTML(html.asString());
Shouldn't this call some variant of setSafeHtml?
http://gwt-code-reviews.appspot.com/829801/diff/7001/8010
File user/src/com/google/gwt/user/client/ui/MenuItem.java (right):
http://gwt-code-reviews.appspot.com/829801/diff/7001/8010#newcode42
user/src/com/google/gwt/user/client/ui/MenuItem.java:42: * Similar to
{...@link #MenuItem(String, boolean)}
Maybe change this to:
Similar to {...@link #MenuItem(String, boolean) MenuItem(String, true)}
to indicate that the effect is like setting the flag to true
http://gwt-code-reviews.appspot.com/829801/diff/7001/8010#newcode227
user/src/com/google/gwt/user/client/ui/MenuItem.java:227:
Spaces
http://gwt-code-reviews.appspot.com/829801/diff/7001/8010#newcode231
user/src/com/google/gwt/user/client/ui/MenuItem.java:231:
Spaces
http://gwt-code-reviews.appspot.com/829801/diff/7001/8010#newcode253
user/src/com/google/gwt/user/client/ui/MenuItem.java:253:
Spaces
http://gwt-code-reviews.appspot.com/829801/diff/7001/8011
File user/src/com/google/gwt/user/client/ui/RadioButton.java (right):
http://gwt-code-reviews.appspot.com/829801/diff/7001/8011#newcode77
user/src/com/google/gwt/user/client/ui/RadioButton.java:77: * @param
label this radio button's label
as SafeHtml
http://gwt-code-reviews.appspot.com/829801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add SafeHtml support to ui widgets. (issue829801)
http://gwt-code-reviews.appspot.com/829801/diff/7001/8003
File user/src/com/google/gwt/user/client/ui/ButtonBase.java (right):
http://gwt-code-reviews.appspot.com/829801/diff/7001/8003#newcode50
user/src/com/google/gwt/user/client/ui/ButtonBase.java:50:
extra spaces
http://gwt-code-reviews.appspot.com/829801/diff/7001/8003#newcode52
user/src/com/google/gwt/user/client/ui/ButtonBase.java:52:
getElement().setInnerHTML(html.asString());
I think we should delegate to setHtml(String) in case a subclass
overrides the method:
setHtml(html.asString())
Same for all other files
http://gwt-code-reviews.appspot.com/829801/diff/7001/8004
File user/src/com/google/gwt/user/client/ui/CheckBox.java (right):
http://gwt-code-reviews.appspot.com/829801/diff/7001/8004#newcode72
user/src/com/google/gwt/user/client/ui/CheckBox.java:72: * Similar to
{...@link #CheckBox(String)}
I don't think you need any of these similar to comments.
http://gwt-code-reviews.appspot.com/829801/diff/7001/8004#newcode80
user/src/com/google/gwt/user/client/ui/CheckBox.java:80:
extra spaces
http://gwt-code-reviews.appspot.com/829801/diff/7001/8004#newcode266
user/src/com/google/gwt/user/client/ui/CheckBox.java:266:
extra spaces - also in other files. Auto-format should fix them.
http://gwt-code-reviews.appspot.com/829801/diff/7001/8005
File user/src/com/google/gwt/user/client/ui/HTML.java (right):
http://gwt-code-reviews.appspot.com/829801/diff/7001/8005#newcode45
user/src/com/google/gwt/user/client/ui/HTML.java:45: public class HTML
extends Label implements HasDirectionalSafeHtml {
Agreed - HasDirectionSafeHtml doesn't extend HasDirectionHtml, so you'll
need to leave them both in there.
http://gwt-code-reviews.appspot.com/829801/diff/7001/8005#newcode135
user/src/com/google/gwt/user/client/ui/HTML.java:135: public
HTML(SafeHtml html, boolean wordWrap) {
I argue that we delete this constructor and deprecate the old version.
GWT has a big problem with constructor bloat for every option. In
general, if a class provides a setter and the argument isn't
fundamentally important to the class (such as the HTML to display), then
we shouldn't have a constructor.
http://gwt-code-reviews.appspot.com/829801/diff/7001/8005#newcode164
user/src/com/google/gwt/user/client/ui/HTML.java:164: public String
getHTML() {
Should we also have getSafeHtml?
I'm going to argue no even though I brought it up. It seems like it
would have some value, but not at the cost of having to store the
SafeHtml in a field and manage it if the user calls setText.
http://gwt-code-reviews.appspot.com/829801/diff/7001/8005#newcode199
user/src/com/google/gwt/user/client/ui/HTML.java:199: public void
setSafeHtml(SafeHtml html) {
You can delete the JavaDoc for this method, and it will be inherited
automatically from HasSafeHtml (I think)
http://gwt-code-reviews.appspot.com/829801/diff/7001/8006
File user/src/com/google/gwt/user/client/ui/HTMLPanel.java (right):
http://gwt-code-reviews.appspot.com/829801/diff/7001/8006#newcode86
user/src/com/google/gwt/user/client/ui/HTMLPanel.java:86: public
HTMLPanel(String tag, String html) {
We need to overload this constructor too.
http://gwt-code-reviews.appspot.com/829801/diff/7001/8014
File user/test/com/google/gwt/user/client/ui/HTMLPanelTest.java (right):
http://gwt-code-reviews.appspot.com/829801/diff/7001/8014#newcode255
user/test/com/google/gwt/user/client/ui/HTMLPanelTest.java:255:
Extra newline
http://gwt-code-reviews.appspot.com/829801/diff/7001/8014#newcode284
user/test/com/google/gwt/user/client/ui/HTMLPanelTest.java:284: }
Is there still a newline at the end of this file?
http://gwt-code-reviews.appspot.com/829801/diff/7001/8015
File user/test/com/google/gwt/user/client/ui/HTMLTest.java (right):
http://gwt-code-reviews.appspot.com/829801/diff/7001/8015#newcode45
user/test/com/google/gwt/user/client/ui/HTMLTest.java:45:
assertEquals(html, htmlElement.getHTML());
add .toLowerCase. Some browser capitalize all tags
http://gwt-code-reviews.appspot.com/829801/diff/7001/8015#newcode57
user/test/com/google/gwt/user/client/ui/HTMLTest.java:57:
assertEquals(html, htmlElementWW.getHTML());
.toLowerCase()
http://gwt-code-reviews.appspot.com/829801/diff/7001/8015#newcode72
user/test/com/google/gwt/user/client/ui/HTMLTest.java:72:
assertEquals(html, htmlElementLTR.getHTML());
.toLowerCase
http://gwt-code-reviews.appspot.com/829801/diff/7001/8016
File user/test/com/google/gwt/user/client/ui/InlineHTMLTest.java
(right):
http://gwt-code-reviews.appspot.com/829801/diff/7001/8016#newcode38
user/test/com/google/gwt/user/client/ui/InlineHTMLTest.java:38:
assertEquals(html, htmlElement.getHTML());
.toLowerCase()
http://gwt-code-reviews.appspot.com/829801/diff/7001/8016#newcode50
user/test/com/google/gwt/user/client/ui/InlineHTMLTest.java:50:
assertEquals(html, htmlElementLTR.getHTML());
.toLowerCase()
http://gwt-code-reviews.appspot.com/829801/diff/7001/8016#newcode62
user/test/com/google/gwt/user/client/ui/InlineHTMLTest.java:62:
[gwt-contrib] Re: Add SafeHtml support to ui widgets. (issue829801)
http://gwt-code-reviews.appspot.com/829801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add SafeHtml support to ui widgets. (issue829801)
http://gwt-code-reviews.appspot.com/829801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
