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: assertEquals(html, htmlElement.getHTML()); .toLowerCase() http://gwt-code-reviews.appspot.com/829801/diff/7001/8017 File user/test/com/google/gwt/user/client/ui/MenuBarTest.java (right): http://gwt-code-reviews.appspot.com/829801/diff/7001/8017#newcode322 user/test/com/google/gwt/user/client/ui/MenuBarTest.java:322: assertEquals(html1.asString(), item1.getHTML()); .toLowerCase() http://gwt-code-reviews.appspot.com/829801/diff/7001/8017#newcode332 user/test/com/google/gwt/user/client/ui/MenuBarTest.java:332: assertEquals(html2.asString(), item2.getHTML()); .toLowerCase() http://gwt-code-reviews.appspot.com/829801/diff/7001/8018 File user/test/com/google/gwt/user/client/ui/MenuItemTest.java (right): http://gwt-code-reviews.appspot.com/829801/diff/7001/8018#newcode43 user/test/com/google/gwt/user/client/ui/MenuItemTest.java:43: assertEquals(html.asString(), item.getHTML()); .toLowerCase() http://gwt-code-reviews.appspot.com/829801/diff/7001/8018#newcode70 user/test/com/google/gwt/user/client/ui/MenuItemTest.java:70: assertEquals(html.asString(), item.getHTML()); .toLowerCase() http://gwt-code-reviews.appspot.com/829801/diff/7001/8018#newcode103 user/test/com/google/gwt/user/client/ui/MenuItemTest.java:103: assertEquals(html2.asString(), item.getHTML()); .toLowerCase() http://gwt-code-reviews.appspot.com/829801/diff/7001/8019 File user/test/com/google/gwt/user/client/ui/RadioButtonTest.java (right): http://gwt-code-reviews.appspot.com/829801/diff/7001/8019#newcode228 user/test/com/google/gwt/user/client/ui/RadioButtonTest.java:228: assertEquals(label1.asString(), radio.getHTML()); .toLowerCase() http://gwt-code-reviews.appspot.com/829801/diff/7001/8019#newcode235 user/test/com/google/gwt/user/client/ui/RadioButtonTest.java:235: assertEquals(label2.asString(), radio.getHTML()); .toLowerCase() http://gwt-code-reviews.appspot.com/829801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
