On Tue, Jan 18, 2011 at 11:53 AM, <[email protected]> wrote: > > http://gwt-code-reviews.appspot.com/1296801/diff/7001/8002 > > File user/src/com/google/gwt/canvas/client/Canvas.java (right): > > http://gwt-code-reviews.appspot.com/1296801/diff/7001/8002#newcode42 > user/src/com/google/gwt/canvas/client/Canvas.java:42: public static > Canvas createCanvasIfSupported() { > On 2011/01/18 17:46:05, rjrjr wrote: > >> Naming nit: use of these factory methods might be easier to automate >> > down the > >> road if they are consistent, so I'd suggest >> > Canvas#createIfSupported(). DRY and > >> all that. >> > > Done. > > > http://gwt-code-reviews.appspot.com/1296801/diff/13001/14002#newcode45 > user/src/com/google/gwt/canvas/client/Canvas.java:45: return null; > > On 2011/01/18 18:55:17, rjrjr wrote: > >> Duplicate code, should call isSupported instead. >> > > I was trying to provide a way for developers to avoid creating two > Canvas elements by re-using the one created during the check for the > actual Canvas creation (calling isSupported() requires creating a Canvas > element). Think that's too hacky?
Oh, I didn't notice that. Objection withdrawn. > > > http://gwt-code-reviews.appspot.com/1296801/diff/13001/14002#newcode174 > user/src/com/google/gwt/canvas/client/Canvas.java:174: } > On 2011/01/18 18:55:17, rjrjr wrote: > >> Can this be private? >> > > Done. > > > http://gwt-code-reviews.appspot.com/1296801/diff/13001/14003 > File user/src/com/google/gwt/dom/client/PartialSupport.java (right): > > http://gwt-code-reviews.appspot.com/1296801/diff/13001/14003#newcode23 > user/src/com/google/gwt/dom/client/PartialSupport.java:23: * supported > at runtime. > On 2011/01/18 18:55:17, rjrjr wrote: > >> What about the factory method? Even if it's not universally needed, >> > you can > >> document how it will appear when it's appropriate. >> > > Improved the javadoc two include both methods. Does the new wording > LGTY? > > > http://gwt-code-reviews.appspot.com/1296801/diff/13001/14005 > File user/test/com/google/gwt/canvas/client/CanvasTest.java (right): > > http://gwt-code-reviews.appspot.com/1296801/diff/13001/14005#newcode33 > user/test/com/google/gwt/canvas/client/CanvasTest.java:33: public class > CanvasTest extends GWTTestCase { > On 2011/01/18 18:55:17, rjrjr wrote: > >> Doesn't test isSupported. >> > > Done. > > > http://gwt-code-reviews.appspot.com/1296801/show > -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
