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?

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

Reply via email to