On Wed, Dec 15, 2010 at 5:40 PM, <jlaba...@google.com> wrote: > LGTM > > Don't forget test cases. >
By which I'm sure John meant "before you submit this." > > You guys are doing an awesome job with the HTML5 stuff! I can't wait to > see this stuff in action. > > > http://gwt-code-reviews.appspot.com/1195801/diff/3001/4004 > File user/src/com/google/gwt/dom/client/MediaElement.java (right): > > http://gwt-code-reviews.appspot.com/1195801/diff/3001/4004#newcode178 > user/src/com/google/gwt/dom/client/MediaElement.java:178: return > this.error; > What does this return if there is no error? Is it null or undefined? > Might be better to do return this.error || null just in case. > > http://gwt-code-reviews.appspot.com/1195801/diff/3001/4004#newcode309 > user/src/com/google/gwt/dom/client/MediaElement.java:309: * controls > (e.g., for controlling play./pause, seek position, and volume), > replace "e.g" with "such as". We avoid abbreviations because some > international users don't recognize them. > > Also, play./pause should be play/pause. > > http://gwt-code-reviews.appspot.com/1195801/diff/3001/4004#newcode410 > user/src/com/google/gwt/dom/client/MediaElement.java:410: * Causes > playback of the resource to be (re)started. > Does it restart or resume if it is already started? How about "Causes > playback of the resource to be started or resumed". > > http://gwt-code-reviews.appspot.com/1195801/diff/3001/4004#newcode516 > user/src/com/google/gwt/dom/client/MediaElement.java:516: public final > native void setSrc(String url) /*-{ > instead of setAttribute(), can we use this.src = url > > > http://gwt-code-reviews.appspot.com/1195801/diff/3001/4007 > File user/src/com/google/gwt/media/client/Audio.java (right): > > http://gwt-code-reviews.appspot.com/1195801/diff/3001/4007#newcode68 > user/src/com/google/gwt/media/client/Audio.java:68: public AudioElement > getAudioElement() { > Can we just override the return value of getElement()? > > > http://gwt-code-reviews.appspot.com/1195801/diff/3001/4008 > File user/src/com/google/gwt/media/client/Video.java (right): > > http://gwt-code-reviews.appspot.com/1195801/diff/3001/4008#newcode68 > user/src/com/google/gwt/media/client/Video.java:68: public VideoElement > getVideoElement() { > I suggest overriding the return value of getElement() instead of > creating a new method. > > http://gwt-code-reviews.appspot.com/1195801/diff/3001/4010 > File user/src/com/google/gwt/media/dom/DOM.gwt.xml (right): > > http://gwt-code-reviews.appspot.com/1195801/diff/3001/4010#newcode18 > user/src/com/google/gwt/media/dom/DOM.gwt.xml:18: <inherits > name="com.google.gwt.media.dom.DOM"/> > Should inherit com.google.gwt.dom.DOM, not com.google.gwt.media.dom.DOM > > > http://gwt-code-reviews.appspot.com/1195801/show > > -- > http://groups.google.com/group/Google-Web-Toolkit-Contributors > -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
