LGTM

Don't forget test cases.

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

Reply via email to