[gwt-contrib] Re: Deal with null (but defined) localStorage and sessionStorage objects. (issue1615807)

[pdr]

On 2011/12/27 18:39:02, pdr wrote:

On 2011/12/20 04:12:58, Jason Terk wrote:



Thank you for the patch!



I vaguely remember there being cross-browser issues with this

approach, but I

just tested IE9, IE7, FF3.6 and FF4 and things look good so far with

your new

approach. Modernizr even wraps this check in a try/catch, but I think

both their

approach and our existing one are for supporting beta versions of

browsers that

no longer exist.




LGTM.



(@rdayal, do you mind applying this patch?)


rdayal is on vacation so I went ahead and applied this:
http://code.google.com/p/google-web-toolkit/source/detail?r=10820
Thanks again for the patch.

http://gwt-code-reviews.appspot.com/1615807/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Deal with null (but defined) localStorage and sessionStorage objects. (issue1615807)

[pdr]

On 2011/12/20 04:12:58, Jason Terk wrote:

Thank you for the patch!

I vaguely remember there being cross-browser issues with this approach,
but I just tested IE9, IE7, FF3.6 and FF4 and things look good so far
with your new approach. Modernizr even wraps this check in a try/catch,
but I think both their approach and our existing one are for supporting
beta versions of browsers that no longer exist.


LGTM.

(@rdayal, do you mind applying this patch?)

http://gwt-code-reviews.appspot.com/1615807/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: change to fix the JavaDoc error during build (issue1478801)

[pdr]

On 2011/07/12 17:50:51, Mark R Russell wrote:

LGTM!

http://gwt-code-reviews.appspot.com/1478801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Change formatter to use 100 col. comments and add a checkstyle check for it (issue1465803)

[pdr]

Thanks for the review! Looks like we might get this to land once and for
all :)


http://gwt-code-reviews.appspot.com/1465803/diff/8/build-tools/customchecks/src/com/google/gwt/checkstyle/GwtHeaderCheck.java
File
build-tools/customchecks/src/com/google/gwt/checkstyle/GwtHeaderCheck.java
(right):

http://gwt-code-reviews.appspot.com/1465803/diff/8/build-tools/customchecks/src/com/google/gwt/checkstyle/GwtHeaderCheck.java#newcode2
build-tools/customchecks/src/com/google/gwt/checkstyle/GwtHeaderCheck.java:2:
* Copyright 2011 Google Inc.
On 2011/06/20 17:59:24, zundel wrote:

If we decide to keep the custom check changes, You will also need to

check-in a

pre-build gwt-checkstyle.jar file to be consistent with past

practices.

Already done (Rietveld doesn't show the binary files though)

http://gwt-code-reviews.appspot.com/1465803/diff/8/eclipse/settings/code-style/gwt-checkstyle.xml
File eclipse/settings/code-style/gwt-checkstyle.xml (right):

http://gwt-code-reviews.appspot.com/1465803/diff/8/eclipse/settings/code-style/gwt-checkstyle.xml#newcode85
eclipse/settings/code-style/gwt-checkstyle.xml:85: http://www\.apache\.org/licenses/LICENSE-2\.0\n \*[
]*\n \* Unless required by applicable law or agreed to in writing,
software\n \* distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT\n \* WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied\. See the\n \* License for the specific
language governing permissions and limitations under\n \* the
License\.\n \*/"/>
On 2011/06/20 17:59:24, zundel wrote:

Instead of coming up with the more complicated check logic, we could

express

this as a regular expression



property name="header" value="/\*\n \* Copyright 20(0[6789]|[12][0-9])

Google

Inc\.\n \*[ ]*\n \* Licensed under the Apache License, Version 2\.0

\(the

"License"\); you may not[\s\*]+use this file except[\s\*]+in
compliance with the License\. You may obtain a copy of[\s\*]+



...


I agree that it would be cleaner, but that wouldn't be as strict as our
current tests. E.g., somebody could get passed a half 80 and half 100
formatted header preamble.

To support both with the same restrictions we currently have, I don't
think there is a simpler way. We could have one enormous regular
expression that slurped up the entire header and accepted either 80 or
100 column headers, but then we wouldn't be able to point to the correct
line number if something didn't match.

http://gwt-code-reviews.appspot.com/1465803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Change formatter to use 100 col. comments and add a checkstyle check for it (issue1465803)

[pdr]

http://gwt-code-reviews.appspot.com/1465803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Change formatter to use 100 col. comments and add a checkstyle check for it (issue1465803)

[pdr]

Reviewers: rjrjr,

Description:
Change formatter to use 100 col. comments and add a checkstyle check for
it


Please review this at http://gwt-code-reviews.appspot.com/1465803/

Affected files:
  A  
build-tools/customchecks/src/com/google/gwt/checkstyle/CustomRegexpHeaderCheck.java
  A  
build-tools/customchecks/src/com/google/gwt/checkstyle/GwtHeaderCheck.java
  A  
build-tools/customchecks/src/com/google/gwt/checkstyle/messages.properties

  M eclipse/settings/code-style/gwt-checkstyle-tests.xml
  M eclipse/settings/code-style/gwt-checkstyle.xml
  M eclipse/settings/code-style/gwt-format.xml


--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: New projects should always have a DTD specified in their template .gwt.xml module file (issue1407803)

[pdr]

On 2011/06/17 15:36:34, fredsa wrote:

http://gwt-code-reviews.appspot.com/1407803/diff/10001/user/src/com/google/gwt/user/tools/WebAppCreator.java

File user/src/com/google/gwt/user/tools/WebAppCreator.java (right):



http://gwt-code-reviews.appspot.com/1407803/diff/10001/user/src/com/google/gwt/user/tools/WebAppCreator.java#newcode474

user/src/com/google/gwt/user/tools/WebAppCreator.java:474: +

"\n
PUBLIC \"-//Google Inc.//DTD Google Web Toolkit "
I prefer to keep the version number on the same line as as "Google Web

Toolkit",

i.e.



How's this look?




2.3.0//EN"


"http://google-web-toolkit.googlecode.com/svn/trunk/distro-source/core/src/gwt-module.dtd";>

LGTM

http://gwt-code-reviews.appspot.com/1407803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: New projects should always have a DTD specified in their template .gwt.xml module file (issue1407803)

[pdr]

LGTM


http://gwt-code-reviews.appspot.com/1407803/diff/10001/user/src/com/google/gwt/user/tools/WebAppCreator.java
File user/src/com/google/gwt/user/tools/WebAppCreator.java (right):

http://gwt-code-reviews.appspot.com/1407803/diff/10001/user/src/com/google/gwt/user/tools/WebAppCreator.java#newcode474
user/src/com/google/gwt/user/tools/WebAppCreator.java:474: +
"\n "...DTD Google Web Toolkit \n"

http://gwt-code-reviews.appspot.com/1407803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: New projects should always have a DTD specified in their template .gwt.xml module file (issue1407803)

[pdr]

http://gwt-code-reviews.appspot.com/1407803/diff/5001/user/src/com/google/gwt/user/tools/WebAppCreator.java
File user/src/com/google/gwt/user/tools/WebAppCreator.java (right):

http://gwt-code-reviews.appspot.com/1407803/diff/5001/user/src/com/google/gwt/user/tools/WebAppCreator.java#newcode461
user/src/com/google/gwt/user/tools/WebAppCreator.java:461: // Public
builds generate a DTD reference.
// Public builds... -> // Generate a DTD reference.

http://gwt-code-reviews.appspot.com/1407803/diff/5001/user/src/com/google/gwt/user/tools/WebAppCreator.java#newcode462
user/src/com/google/gwt/user/tools/WebAppCreator.java:462: String
gwtModuleDtd = "\n"
Nit: Can you make this less than than 100 chars?

http://gwt-code-reviews.appspot.com/1407803/diff/5001/user/src/com/google/gwt/user/tools/WebAppCreator.java#newcode469
user/src/com/google/gwt/user/tools/WebAppCreator.java:469: gwtModuleDtd
= "\n"
Is this requirement practical?

http://gwt-code-reviews.appspot.com/1407803/diff/5001/user/src/com/google/gwt/user/tools/WebAppCreator.java#newcode470
user/src/com/google/gwt/user/tools/WebAppCreator.java:470: +
"\nhttp://gwt-code-reviews.appspot.com/1407803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Autoformat of Widget in preparation for some new additions. (issue1447822)

[pdr]

On 2011/06/08 23:14:41, rjrjr wrote:

LGTM.

http://gwt-code-reviews.appspot.com/1447822/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Format comments to 100 columns to match code format (issue1450808)

[pdr]

I'm not sure of these Google conventions you refer to, but if there were
such conventions I think they have changed :)

On 2011/05/31 14:27:15, rjrjr wrote:

It's not called out in the gwt guidelines, but I don't think I'm

giving away

any secrets by saying 80/100 is the Google convention.
On May 31, 2011 6:21 AM,  wrote:
> I wasn't able to find that in the style guide or discussion when it

was

> changed. 80 code / 100 comments feels inconsistent.
>
>
> On 2011/05/29 23:06:55, zundel wrote:
>> Leaving comments at 80 was intentional as per the style guide, I
> thought.
>> On May 29, 2011 6:43 PM,  wrote:
>> > Reviewers: zundel, rjrjr,
>> >
>> > Description:
>> > Format comments to 100 columns to match code format
>> >
>> >
>> > Please review this at

http://gwt-code-reviews.appspot.com/1450808/

>> >
>> > Affected files:
>> > M eclipse/settings/code-style/gwt-format.xml
>> >
>> >
>> > Index: eclipse/settings/code-style/gwt-format.xml
>> >

===

>> > --- eclipse/settings/code-style/gwt-format.xml (revision 10241)
>> > +++ eclipse/settings/code-style/gwt-format.xml (working copy)
>> > @@ -64,7 +64,7 @@
>> >  id="org.eclipse.jdt.core.formatter.comment.indent_root_tags"
>> > value="true"/>
>> > > >
>
>


id="org.eclipse.jdt.core.formatter.comment.insert_new_line_before_root_tags"

>
>> > value="insert"/>
>> > > >
>

id="org.eclipse.jdt.core.formatter.comment.insert_new_line_for_parameter"

>> > value="do not insert"/>
>> > -> > value="80"/>
>> > +> > value="100"/>
>> > > >
>

id="org.eclipse.jdt.core.formatter.comment.new_lines_at_block_boundaries"

>> > value="true"/>
>> > > >
>
>


id="org.eclipse.jdt.core.formatter.comment.new_lines_at_javadoc_boundaries"

>> > value="true"/>
>> > > value="true"/>
>> >
>> >
>
>
>
> http://gwt-code-reviews.appspot.com/1450808/




http://gwt-code-reviews.appspot.com/1450808/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Format comments to 100 columns to match code format (issue1450808)

[pdr]

I wasn't able to find that in the style guide or discussion when it was
changed. 80 code / 100 comments feels inconsistent.


On 2011/05/29 23:06:55, zundel wrote:

Leaving comments at 80 was intentional as per the style guide, I

thought.

On May 29, 2011 6:43 PM,  wrote:
> Reviewers: zundel, rjrjr,
>
> Description:
> Format comments to 100 columns to match code format
>
>
> Please review this at http://gwt-code-reviews.appspot.com/1450808/
>
> Affected files:
> M eclipse/settings/code-style/gwt-format.xml
>
>
> Index: eclipse/settings/code-style/gwt-format.xml
> ===
> --- eclipse/settings/code-style/gwt-format.xml (revision 10241)
> +++ eclipse/settings/code-style/gwt-format.xml (working copy)
> @@ -64,7 +64,7 @@
> 
id="org.eclipse.jdt.core.formatter.comment.indent_root_tags"

> value="true"/>
> 


id="org.eclipse.jdt.core.formatter.comment.insert_new_line_before_root_tags"


> value="insert"/>
> 

id="org.eclipse.jdt.core.formatter.comment.insert_new_line_for_parameter"

> value="do not insert"/>
> - value="80"/>
> + value="100"/>
> 

id="org.eclipse.jdt.core.formatter.comment.new_lines_at_block_boundaries"

> value="true"/>
> 


id="org.eclipse.jdt.core.formatter.comment.new_lines_at_javadoc_boundaries"

> value="true"/>
> 
>
>




http://gwt-code-reviews.appspot.com/1450808/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Format comments to 100 columns to match code format (issue1450808)

[pdr]

Reviewers: zundel, rjrjr,

Description:
Format comments to 100 columns to match code format


Please review this at http://gwt-code-reviews.appspot.com/1450808/

Affected files:
  M eclipse/settings/code-style/gwt-format.xml


Index: eclipse/settings/code-style/gwt-format.xml
===
--- eclipse/settings/code-style/gwt-format.xml  (revision 10241)
+++ eclipse/settings/code-style/gwt-format.xml  (working copy)
@@ -64,7 +64,7 @@
 value="true"/>
 id="org.eclipse.jdt.core.formatter.comment.insert_new_line_before_root_tags"  
value="insert"/>
 id="org.eclipse.jdt.core.formatter.comment.insert_new_line_for_parameter"  
value="do not insert"/>
-value="80"/>
+value="100"/>
 id="org.eclipse.jdt.core.formatter.comment.new_lines_at_block_boundaries"  
value="true"/>
 id="org.eclipse.jdt.core.formatter.comment.new_lines_at_javadoc_boundaries"  
value="true"/>

 


--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Adds HTML5 App Cache support to MobileWebApp sample. (issue1428811)

[pdr]

You'll need to add this to the mobile web app .gwt.xml:



http://gwt-code-reviews.appspot.com/1428811/diff/1/dev/core/src/com/google/gwt/core/linker/SimpleAppCacheLinker.java
File dev/core/src/com/google/gwt/core/linker/SimpleAppCacheLinker.java
(right):

http://gwt-code-reviews.appspot.com/1428811/diff/1/dev/core/src/com/google/gwt/core/linker/SimpleAppCacheLinker.java#newcode17
dev/core/src/com/google/gwt/core/linker/SimpleAppCacheLinker.java:17:
This linker should be defined in a gwt.xml file somewhere with this
line:


http://gwt-code-reviews.appspot.com/1428811/diff/1/dev/core/src/com/google/gwt/core/linker/SimpleAppCacheLinker.java#newcode55
dev/core/src/com/google/gwt/core/linker/SimpleAppCacheLinker.java:55: *
and overrides {@code otherCAchedFiles()}, and use it as a linker
instead:
otherCAchedFiles -> otherCachedFiles

http://gwt-code-reviews.appspot.com/1428811/diff/1/dev/core/src/com/google/gwt/core/linker/SimpleAppCacheLinker.java#newcode166
dev/core/src/com/google/gwt/core/linker/SimpleAppCacheLinker.java:166:
// build cache list
This code is essentially a duplicate of the above code. Break it out
into a method?

http://gwt-code-reviews.appspot.com/1428811/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Adds HTML5 App Cache support to MobileWebApp sample. (issue1422815)

[pdr]

Rietveld is giving me a bunch of errors trying to read files such as
web.xml. I reviewed the one file I could access and that is relevant to
this change, but do you mind re-uploading the patch?


http://gwt-code-reviews.appspot.com/1422815/diff/1/samples/mobilewebapp/src/dev/com/google/gwt/sample/mobilewebapp/linker/AppCacheLinker.java
File
samples/mobilewebapp/src/dev/com/google/gwt/sample/mobilewebapp/linker/AppCacheLinker.java
(right):

http://gwt-code-reviews.appspot.com/1422815/diff/1/samples/mobilewebapp/src/dev/com/google/gwt/sample/mobilewebapp/linker/AppCacheLinker.java#newcode57
samples/mobilewebapp/src/dev/com/google/gwt/sample/mobilewebapp/linker/AppCacheLinker.java:57:
private static final String[] STATIC_FILES = {
Is there no way to grab these automatically so that this static files
stuff doesn't have to be here? At a minimum there should be a way to
parameterize this.

http://gwt-code-reviews.appspot.com/1422815/diff/1/samples/mobilewebapp/src/dev/com/google/gwt/sample/mobilewebapp/linker/AppCacheLinker.java#newcode58
samples/mobilewebapp/src/dev/com/google/gwt/sample/mobilewebapp/linker/AppCacheLinker.java:58:
"/MobileWebApp.html",
I believe you don't need to list MobileWebApp.html in the cache manifest
since it will automatically be cached. The css file will need to be
listed though.

http://gwt-code-reviews.appspot.com/1422815/diff/1/samples/mobilewebapp/src/dev/com/google/gwt/sample/mobilewebapp/linker/AppCacheLinker.java#newcode59
samples/mobilewebapp/src/dev/com/google/gwt/sample/mobilewebapp/linker/AppCacheLinker.java:59:
"/MobileWebApp.css",
These resources (in /audio/) can be included by adding a  line to your gwt.xml so you don't have to
list them here.

http://gwt-code-reviews.appspot.com/1422815/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Adding the SourceElement for use with Audio and Video, and adding convenience methods in those w... (issue1423810)

[pdr]

On 2011/04/28 15:57:22, jlabanca wrote:

http://gwt-code-reviews.appspot.com/1423810/diff/1/user/src/com/google/gwt/dom/client/SourceElement.java

File user/src/com/google/gwt/dom/client/SourceElement.java (right):



http://gwt-code-reviews.appspot.com/1423810/diff/1/user/src/com/google/gwt/dom/client/SourceElement.java#newcode67

user/src/com/google/gwt/dom/client/SourceElement.java:67: * Sets the

type of

media represented by the src.
On 2011/04/28 00:18:50, pdr wrote:
> Can you add a mention that codec info can be specified here too?

Maybe provide

> an example in the doc like: type='audio/ogg; codecs=vorbis'



Done.



http://gwt-code-reviews.appspot.com/1423810/diff/1/user/src/com/google/gwt/media/client/MediaBase.java

File user/src/com/google/gwt/media/client/MediaBase.java (right):



http://gwt-code-reviews.appspot.com/1423810/diff/1/user/src/com/google/gwt/media/client/MediaBase.java#newcode67

user/src/com/google/gwt/media/client/MediaBase.java:67: * files until

it finds

one it can play.
On 2011/04/28 00:18:50, pdr wrote:
> "choose download" -> "choose to download"



Done.



Changed to "will request source files from the server"



http://gwt-code-reviews.appspot.com/1423810/diff/1/user/src/com/google/gwt/media/client/MediaBase.java#newcode536

user/src/com/google/gwt/media/client/MediaBase.java:536:
On 2011/04/28 00:18:50, pdr wrote:
> Can you add methods such as getSource(index), getNumSources(), and
> removeSource(index)?



Not easily.  A media element can have elements other than source as a

child.

For example, you can have a div with a message to display if the html5

element

isn't supported.  Calling removeSource(index) would mean that we have

to walk

the children until we find the nth source.



Instead, we return the SourceElement from addSource, so users can keep

track of

it.  I also added removeSource(SourceElement).



http://gwt-code-reviews.appspot.com/1423810/diff/1/user/test/com/google/gwt/media/client/MediaTest.java

File user/test/com/google/gwt/media/client/MediaTest.java (right):



http://gwt-code-reviews.appspot.com/1423810/diff/1/user/test/com/google/gwt/media/client/MediaTest.java#newcode132

user/test/com/google/gwt/media/client/MediaTest.java:132:

media.load();

On 2011/04/28 00:18:50, pdr wrote:
> AFAIK, load isn't synchronous. This seems like it should fail on

most

browsers.



I removed this test and replaced it with

testAddSource()/testRemoveSource(),

which actually tests the API that was added.



http://gwt-code-reviews.appspot.com/1423810/diff/1/user/test/com/google/gwt/media/client/MediaTest.java#newcode141

user/test/com/google/gwt/media/client/MediaTest.java:141:
On 2011/04/28 00:18:50, pdr wrote:
> Can you add a test for Source's getType() and getSrc()?



Done.



I added tests for MediaBase#addSource()/removeSource(), and I check

the src and

type in the addSource() test.


LGTM

http://gwt-code-reviews.appspot.com/1423810/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Adding the SourceElement for use with Audio and Video, and adding convenience methods in those w... (issue1423810)

[pdr]

http://gwt-code-reviews.appspot.com/1423810/diff/1/user/src/com/google/gwt/dom/client/SourceElement.java
File user/src/com/google/gwt/dom/client/SourceElement.java (right):

http://gwt-code-reviews.appspot.com/1423810/diff/1/user/src/com/google/gwt/dom/client/SourceElement.java#newcode67
user/src/com/google/gwt/dom/client/SourceElement.java:67: * Sets the
type of media represented by the src.
Can you add a mention that codec info can be specified here too? Maybe
provide an example in the doc like: type='audio/ogg; codecs=vorbis'

http://gwt-code-reviews.appspot.com/1423810/diff/1/user/src/com/google/gwt/media/client/MediaBase.java
File user/src/com/google/gwt/media/client/MediaBase.java (right):

http://gwt-code-reviews.appspot.com/1423810/diff/1/user/src/com/google/gwt/media/client/MediaBase.java#newcode67
user/src/com/google/gwt/media/client/MediaBase.java:67: * files until it
finds one it can play.
"choose download" -> "choose to download"

http://gwt-code-reviews.appspot.com/1423810/diff/1/user/src/com/google/gwt/media/client/MediaBase.java#newcode536
user/src/com/google/gwt/media/client/MediaBase.java:536:
Can you add methods such as getSource(index), getNumSources(), and
removeSource(index)?

http://gwt-code-reviews.appspot.com/1423810/diff/1/user/test/com/google/gwt/media/client/MediaTest.java
File user/test/com/google/gwt/media/client/MediaTest.java (right):

http://gwt-code-reviews.appspot.com/1423810/diff/1/user/test/com/google/gwt/media/client/MediaTest.java#newcode132
user/test/com/google/gwt/media/client/MediaTest.java:132: media.load();
AFAIK, load isn't synchronous. This seems like it should fail on most
browsers.

http://gwt-code-reviews.appspot.com/1423810/diff/1/user/test/com/google/gwt/media/client/MediaTest.java#newcode141
user/test/com/google/gwt/media/client/MediaTest.java:141:
Can you add a test for Source's getType() and getSrc()?

http://gwt-code-reviews.appspot.com/1423810/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Adding the SourceElement for use with Audio and Video, and adding convenience methods in those w... (issue1423810)

[pdr]

http://gwt-code-reviews.appspot.com/1423810/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Change Audio and Video support detection method for Safari3. (issue1423802)

[pdr]

Reviewers: jlabanca,

Description:
Change Audio and Video support detection method for Safari3.

Safari3 incorrectly reports that audio/video are supported when using
!!element.play, but correctly reports that audio/video are not supported
using
!!element.canPlayType. This was verified to work and pass tests on the
following versions of Safari: Safari 3.2.1, Safari 4.0.5, and Safari
5.0.4.


Please review this at http://gwt-code-reviews.appspot.com/1423802/

Affected files:
  M user/src/com/google/gwt/media/client/Audio.java
  M user/src/com/google/gwt/media/client/Video.java


Index: user/src/com/google/gwt/media/client/Audio.java
===
--- user/src/com/google/gwt/media/client/Audio.java (revision 10005)
+++ user/src/com/google/gwt/media/client/Audio.java (working copy)
@@ -80,7 +80,7 @@
  * @return true if supported, false otherwise.
  */
 static native boolean isSupportedRunTime(AudioElement element) /*-{
-  return !!element.play;
+  return !!element.canPlayType;
 }-*/;

 /**
Index: user/src/com/google/gwt/media/client/Video.java
===
--- user/src/com/google/gwt/media/client/Video.java (revision 10005)
+++ user/src/com/google/gwt/media/client/Video.java (working copy)
@@ -78,7 +78,7 @@
  * @return true if supported, false otherwise.
  */
 static native boolean isSupportedRunTime(VideoElement element) /*-{
-  return !!element.play;
+  return !!element.canPlayType;
 }-*/;

 /**
@@ -143,7 +143,7 @@

   /**
* Creates a Video widget with a given source URL.
-   *
+   *
* @param src a String URL.
* @deprecated use {@link #createIfSupported()}.
*/


--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Removing duplicate entries from User.gwt.xml that were added in r9976 due to an undetected merge... (issue1422801)

[pdr]

On 2011/04/15 13:35:59, jlabanca wrote:

LGTM

http://gwt-code-reviews.appspot.com/1422801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Audio and Video cleanup. (issue1415801)

[pdr]

http://gwt-code-reviews.appspot.com/1415801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Auto-formats the GWT tools projects (excluding api-checker covered in (issue1402803)

[pdr]

On 2011/04/15 15:35:21, zundel wrote:

ping


Despite my minor complaints, but this matches what was proposed so LGTM.

http://gwt-code-reviews.appspot.com/1402803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Fixing a bug in Image where IE throws a native JavaScript exception if an image is attached and ... (issue1421801)

[pdr]

LGTM


http://gwt-code-reviews.appspot.com/1421801/diff/1/user/test/com/google/gwt/user/client/ui/ImageTest.java
File user/test/com/google/gwt/user/client/ui/ImageTest.java (right):

http://gwt-code-reviews.appspot.com/1421801/diff/1/user/test/com/google/gwt/user/client/ui/ImageTest.java#newcode170
user/test/com/google/gwt/user/client/ui/ImageTest.java:170:
}.schedule(500);;
Extra ;

Also, can you bump this time a bit (here, and elsewhere)? Maybe 1s?

http://gwt-code-reviews.appspot.com/1421801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Audio and Video cleanup. (issue1415801)

[pdr]

http://gwt-code-reviews.appspot.com/1415801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Audio and Video cleanup. (issue1415801)

[pdr]

http://gwt-code-reviews.appspot.com/1415801/diff/1/user/src/com/google/gwt/media/client/Video.java
File user/src/com/google/gwt/media/client/Video.java (right):

http://gwt-code-reviews.appspot.com/1415801/diff/1/user/src/com/google/gwt/media/client/Video.java#newcode88
user/src/com/google/gwt/media/client/Video.java:88: return
(VideoElement) getMediaElement();
On 2011/04/13 17:46:08, jlabanca wrote:

I liked cast() better than a cast if it works.


Done (curious, why? Does it save a cast check?)

http://gwt-code-reviews.appspot.com/1415801/diff/1/user/test/com/google/gwt/canvas/client/CanvasTest.java
File user/test/com/google/gwt/canvas/client/CanvasTest.java (right):

http://gwt-code-reviews.appspot.com/1415801/diff/1/user/test/com/google/gwt/canvas/client/CanvasTest.java#newcode41
user/test/com/google/gwt/canvas/client/CanvasTest.java:41: private
native boolean isFirefox35OrLater() /*-{
On 2011/04/13 17:46:08, jlabanca wrote:

public, private, and protected methods appear out of order.  Also, you

can make

these isXXX() checks static.


Done.

http://gwt-code-reviews.appspot.com/1415801/diff/1/user/test/com/google/gwt/media/client/AudioTest.java
File user/test/com/google/gwt/media/client/AudioTest.java (right):

http://gwt-code-reviews.appspot.com/1415801/diff/1/user/test/com/google/gwt/media/client/AudioTest.java#newcode84
user/test/com/google/gwt/media/client/AudioTest.java:84: public
MediaBase getMedia() {
On 2011/04/13 17:46:08, jlabanca wrote:

public method after protected method


Done.

http://gwt-code-reviews.appspot.com/1415801/diff/1/user/test/com/google/gwt/media/client/MediaTest.java
File user/test/com/google/gwt/media/client/MediaTest.java (right):

http://gwt-code-reviews.appspot.com/1415801/diff/1/user/test/com/google/gwt/media/client/MediaTest.java#newcode110
user/test/com/google/gwt/media/client/MediaTest.java:110: // wait an
additional 1000ms, then check that the seek was successful
On 2011/04/13 17:46:08, jlabanca wrote:

1000ms => 5000ms


Done.

http://gwt-code-reviews.appspot.com/1415801/diff/1/user/test/com/google/gwt/media/client/VideoTest.java
File user/test/com/google/gwt/media/client/VideoTest.java (right):

http://gwt-code-reviews.appspot.com/1415801/diff/1/user/test/com/google/gwt/media/client/VideoTest.java#newcode71
user/test/com/google/gwt/media/client/VideoTest.java:71:
assertEquals(height + "px", video.getOffsetHeight());
On 2011/04/13 17:46:08, jlabanca wrote:

Does this pass?  getOffsetHeight/Width() returns an int, so this test

should

always fail.


It worked better in my head. Fixed.

http://gwt-code-reviews.appspot.com/1415801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Improving TouchScroller to allow native document level scrolling when appropriate. If the scroll... (issue1410803)

[pdr]

On 2011/04/12 20:17:42, jlabanca wrote:

LGTM. Much better!

http://gwt-code-reviews.appspot.com/1410803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Audio and Video cleanup. (issue1415801)

[pdr]

Reviewers: jlabanca,

Description:
Audio and Video cleanup.

Add a MediaBase widget backing Audio and Video, and clean up tests.
Also, address comments by reviewer and re-encode test media files for a
smaller size and to fix an h264 video issue.


Please review this at http://gwt-code-reviews.appspot.com/1415801/

Affected files:
  M user/src/com/google/gwt/dom/client/DOMImplMozilla.java
  M user/src/com/google/gwt/media/client/Audio.java
  A user/src/com/google/gwt/media/client/MediaBase.java
  M user/src/com/google/gwt/media/client/Video.java
  M user/test/com/google/gwt/canvas/client/CanvasTest.java
  M user/test/com/google/gwt/media/MediaSuite.java
  M user/test/com/google/gwt/media/client/AudioTest.java
  M user/test/com/google/gwt/media/client/MediaTest.java
  M user/test/com/google/gwt/media/client/VideoTest.java


--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Escape single characters in SafeHtmlBuilder/SafeHtmlUtils (external issue 6222) (issue1413802)

[pdr]

On 2011/04/12 18:35:43, xtof wrote:

LGTM.


LGTM2

http://gwt-code-reviews.appspot.com/1413802/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Updating Showcase to use LayoutPanels throughout the hierarchy, thus implementing ProvidesResize... (issue1409801)

[pdr]

LGTM pending a check of showcase on mobile/tablet devices.


http://gwt-code-reviews.appspot.com/1409801/diff/1/samples/showcase/src/com/google/gwt/sample/showcase/client/ContentWidget.java
File
samples/showcase/src/com/google/gwt/sample/showcase/client/ContentWidget.java
(right):

http://gwt-code-reviews.appspot.com/1409801/diff/1/samples/showcase/src/com/google/gwt/sample/showcase/client/ContentWidget.java#newcode47
samples/showcase/src/com/google/gwt/sample/showcase/client/ContentWidget.java:47:
* This {@link Widget} uses a lazy initialization mechanism so that the
content
Do you need to update this after switching from LazyPanel to
SimpleLayoutPanel?

http://gwt-code-reviews.appspot.com/1409801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Add the beginnings of new HTML5 drag and drop events (issue1398802)

[pdr]

LGTM

On 2011/04/05 17:35:36, fredsa wrote:

http://gwt-code-reviews.appspot.com/1398802/diff/1/user/src/com/google/gwt/event/dom/client/DragExitEvent.java

File user/src/com/google/gwt/event/dom/client/DragExitEvent.java

(right):


http://gwt-code-reviews.appspot.com/1398802/diff/1/user/src/com/google/gwt/event/dom/client/DragExitEvent.java#newcode41

user/src/com/google/gwt/event/dom/client/DragExitEvent.java:41: *

{@link

DomEvent#fireNativeEvent(com.google.gwt.dom.client.NativeEvent,
com.google.gwt.event.shared.HasHandlers)}
On 2011/04/04 14:43:23, pdr wrote:
> Line > 100 chars.



Done.



http://gwt-code-reviews.appspot.com/1398802/diff/1/user/test/com/google/gwt/user/client/DragAndDropEventsSinkTest.java

File

user/test/com/google/gwt/user/client/DragAndDropEventsSinkTest.java

(right):



http://gwt-code-reviews.appspot.com/1398802/diff/1/user/test/com/google/gwt/user/client/DragAndDropEventsSinkTest.java#newcode176

user/test/com/google/gwt/user/client/DragAndDropEventsSinkTest.java:176:

delayTestFinish(100);
On 2011/04/04 14:43:23, pdr wrote:
> This is just 100ms, any reason not to bump that to 1000ms / 1s?



Done.




http://gwt-code-reviews.appspot.com/1398802/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Autoformat the api-checker tool source (issue1405801)

[pdr]

http://gwt-code-reviews.appspot.com/1405801/diff/1/tools/api-checker/test/com/google/gwt/tools/apichecker/ApiContainerTest.java
File
tools/api-checker/test/com/google/gwt/tools/apichecker/ApiContainerTest.java
(right):

http://gwt-code-reviews.appspot.com/1405801/diff/1/tools/api-checker/test/com/google/gwt/tools/apichecker/ApiContainerTest.java#newcode233
tools/api-checker/test/com/google/gwt/tools/apichecker/ApiContainerTest.java:233:
.getApiClass("test.apicontainer.OneMoreApiClass");
Quadruple spaces are pretty here, but push the code too far to the
right. As far as I can tell, it breaks with Google's normal Java style.
I would prefer 2 spaces.

http://gwt-code-reviews.appspot.com/1405801/diff/1/tools/api-checker/test/com/google/gwt/tools/apichecker/ApiContainerTest.java#newcode256
tools/api-checker/test/com/google/gwt/tools/apichecker/ApiContainerTest.java:256:
.size());
This is an example of where the builder pattern rules makes normal code
ugly. This should be:
assertEquals(1, oneMoreApiClass.getApiMembersBySet(methodNames,
  ApiClass.MethodType.METHOD).size());

http://gwt-code-reviews.appspot.com/1405801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Add the beginnings of new HTML5 drag and drop events (issue1398802)

[pdr]

LGTM


http://gwt-code-reviews.appspot.com/1398802/diff/1/user/src/com/google/gwt/event/dom/client/DragExitEvent.java
File user/src/com/google/gwt/event/dom/client/DragExitEvent.java
(right):

http://gwt-code-reviews.appspot.com/1398802/diff/1/user/src/com/google/gwt/event/dom/client/DragExitEvent.java#newcode41
user/src/com/google/gwt/event/dom/client/DragExitEvent.java:41: * {@link
DomEvent#fireNativeEvent(com.google.gwt.dom.client.NativeEvent,
com.google.gwt.event.shared.HasHandlers)}
Line > 100 chars.

http://gwt-code-reviews.appspot.com/1398802/diff/1/user/test/com/google/gwt/user/client/DragAndDropEventsSinkTest.java
File user/test/com/google/gwt/user/client/DragAndDropEventsSinkTest.java
(right):

http://gwt-code-reviews.appspot.com/1398802/diff/1/user/test/com/google/gwt/user/client/DragAndDropEventsSinkTest.java#newcode176
user/test/com/google/gwt/user/client/DragAndDropEventsSinkTest.java:176:
delayTestFinish(100);
This is just 100ms, any reason not to bump that to 1000ms / 1s?

http://gwt-code-reviews.appspot.com/1398802/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Add the beginnings of new HTML5 drag and drop events (issue1398802)

[pdr]

On 2011/03/31 17:50:22, fredsa wrote:

LGTM

http://gwt-code-reviews.appspot.com/1398802/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Adding a SuppressWarnings to prevent the Google Plugin for Eclipse from marking this class as error. (issue1386805)

[pdr]

On 2011/03/31 18:35:21, schenney wrote:

LGTM

http://gwt-code-reviews.appspot.com/1386805/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Add media events for audio/video elements (issue1385804)

[pdr]

On 2011/03/30 19:06:49, fredsa wrote:

The code review tool is being a little funny but LGTM for the change to
DOMImplStandard.java

http://gwt-code-reviews.appspot.com/1385804/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Fixes build break - unit test expected non-existent dir to throw an exception (issue1396802)

[pdr]

LGTM


http://gwt-code-reviews.appspot.com/1396802/diff/2001/dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java
File dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java
(right):

http://gwt-code-reviews.appspot.com/1396802/diff/2001/dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java#newcode44
dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java:44:
public void testNewDir() throws IOException, UnableToCompleteException {
Would it be cleaner to wrap these exceptions and explicitly call
fail(exception.getMessage())?

http://gwt-code-reviews.appspot.com/1396802/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Roll-back due to test breakage. (issue1396801)

[pdr]

On 2011/03/29 21:54:47, rchandia wrote:

LGTM

http://gwt-code-reviews.appspot.com/1396801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Fixing HasDataPresenter#scheduleFinally to schedule the command in the argument, not the pending... (issue1391801)

[pdr]

On 2011/03/24 20:18:47, jlabanca wrote:

LGTM but this bug means there's no test coverage of this functionality.
Can you add it?

http://gwt-code-reviews.appspot.com/1391801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Fixing HasDataPresenter#scheduleFinally to schedule the command in the argument, not the pending... (issue1391801)

[pdr]

On 2011/03/24 20:18:47, jlabanca wrote:

LGTM but this bug means there's no test coverage of this functionality.
Can you add it?

http://gwt-code-reviews.appspot.com/1391801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Cells should only fill a CellWidget if they render exactly one topmost element, or the first chi... (issue1385806)

[pdr]

On 2011/03/24 15:59:37, jlabanca wrote:

LGTM, thanks for catching that tbroyer :)

http://gwt-code-reviews.appspot.com/1385806/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Making rendered Cells in CellWidgets fill the root element of the widget so that setting the hei... (issue1383807)

[pdr]

On 2011/03/23 19:28:33, jlabanca wrote:

LGTM if you add a test of the sizing changes for CellWidget

http://gwt-code-reviews.appspot.com/1383807/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Adding a new TextButton widget to replace the existing Button widget. The new widget provides th... (issue1383806)

[pdr]

LGTM2

On 2011/03/23 18:07:02, rjrjr wrote:

LGTM



This is pretty exciting.



On Wed, Mar 23, 2011 at 11:05 AM,   wrote:
> Moving the Default/Negative/Primary Decorations into

DefaultAppearance

> makes the API much cleaner.
>
> First, I removed all of the static factory methods (but left some
> JavaDoc explaining how to use the different constructors). Users can
> just use the default constructor, then call setDecoration() to make

the

> button a primary/negative button.
>
> Second, we now GWT.create(Appearance.class) instead of
> GWT.create(DefaultAppearance.class).  This is much better

because users

> should be able to provide skins by implementing Appearance, without
> having to subclass DefaultAppearance.  In general, we should

always push

> configuration into ButtonCellBase, and leave the Appearance to just
> render the configuration.
>
>
>


http://gwt-code-reviews.appspot.com/1383806/diff/13/user/src/com/google/gwt/cell/client/ButtonCellBase.java

> File user/src/com/google/gwt/cell/client/ButtonCellBase.java

(right):

>
>


http://gwt-code-reviews.appspot.com/1383806/diff/13/user/src/com/google/gwt/cell/client/ButtonCellBase.java#newcode53

> user/src/com/google/gwt/cell/client/ButtonCellBase.java:53: public
> static interface Appearance {
> On 2011/03/23 17:27:30, rjrjr wrote:
>>
>> all interfaces are static, that's noise. Please remove static from

all

>
> these
>>
>> interfaces
>
> Done.
>
>


http://gwt-code-reviews.appspot.com/1383806/diff/13/user/src/com/google/gwt/cell/client/TextButtonCell.java

> File user/src/com/google/gwt/cell/client/TextButtonCell.java

(right):

>
>


http://gwt-code-reviews.appspot.com/1383806/diff/13/user/src/com/google/gwt/cell/client/TextButtonCell.java#newcode71

> user/src/com/google/gwt/cell/client/TextButtonCell.java:71:
> super(SimpleSafeHtmlRenderer.getInstance());
> On 2011/03/23 17:27:30, rjrjr wrote:
>>
>> Are you sure that this shared instance of SimpleSafeHtmlRenderer is
>
> worth the
>>
>> bother? I wonder about unnecessary clinits and effects on inlining.
>
> Did you
>>
>> actually check that SimpleSafeHtmlRenderer.getInstance() is cheaper
>
> than new
>>
>> SimpleSafeHtmlRenderer()?
>
> I doubt it makes any difference since Cells are singletons, so their
> won't be too many on a page and one extra object creation is
> insignificant.
>
> Can we remember to take a look into the general question of

singletons

> versus new instance of simple objects?  It seems like thats a

bigger

> question than this change, and I need to submit this asap.
>
>


http://gwt-code-reviews.appspot.com/1383806/diff/13/user/src/com/google/gwt/user/widget/client/ButtonBase.java

> File user/src/com/google/gwt/user/widget/client/ButtonBase.java

(right):

>
>


http://gwt-code-reviews.appspot.com/1383806/diff/13/user/src/com/google/gwt/user/widget/client/ButtonBase.java#newcode98

> user/src/com/google/gwt/user/widget/client/ButtonBase.java:98:

return

> addHandler(handler, BlurEvent.getType());
> On 2011/03/23 17:03:21, tbroyer wrote:
>>
>> Shouldn't this be addDomHandler?
>
> Done.
>
>


http://gwt-code-reviews.appspot.com/1383806/diff/13/user/src/com/google/gwt/user/widget/client/package-info.java

> File user/src/com/google/gwt/user/widget/client/package-info.java
> (right):
>
>


http://gwt-code-reviews.appspot.com/1383806/diff/13/user/src/com/google/gwt/user/widget/client/package-info.java#newcode21

> user/src/com/google/gwt/user/widget/client/package-info.java:21:

package

> com.google.gwt.user.widget.client;
> On 2011/03/23 17:27:30, rjrjr wrote:
>>
>> Any reason not to make this com.google.gwt.widget?
>
> Done.
>
> http://gwt-code-reviews.appspot.com/1383806/
>




http://gwt-code-reviews.appspot.com/1383806/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Update partial support elements for ie9. (issue1389802)

[pdr]

Reviewers: jlabanca,

Description:
Update partial support elements for ie9.


Please review this at http://gwt-code-reviews.appspot.com/1389802/

Affected files:
  M user/src/com/google/gwt/canvas/Canvas.gwt.xml
  M user/src/com/google/gwt/media/Media.gwt.xml


Index: user/src/com/google/gwt/canvas/Canvas.gwt.xml
===
--- user/src/com/google/gwt/canvas/Canvas.gwt.xml   (revision 9873)
+++ user/src/com/google/gwt/canvas/Canvas.gwt.xml   (working copy)
@@ -20,14 +20,14 @@
   
   

-  
-  
+  
+  

-  
+  
+  
 
-  
-  
-  
+  
+  
 
   

Index: user/src/com/google/gwt/media/Media.gwt.xml
===
--- user/src/com/google/gwt/media/Media.gwt.xml (revision 9873)
+++ user/src/com/google/gwt/media/Media.gwt.xml (working copy)
@@ -21,23 +21,21 @@
   
   

-  
-  
-  
+  
+  
+  

-  
+  
 
-  
-  
-  
+  
+  
 
   

-  
+  
 
-  
-  
-  
+  
+  
 
   

@@ -53,7 +51,7 @@

   class="com.google.gwt.media.client.Audio.AudioElementSupportDetectedMaybe">
 class="com.google.gwt.media.client.Audio.AudioElementSupportDetector" />

-
+
   

   class="com.google.gwt.media.client.Audio.AudioElementSupportDetectedNo">



--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Adding a new TextButton widget to replace the existing Button widget. The new widget provides th... (issue1383806)

[pdr]

A small review--larger one coming up tomorrow.


http://gwt-code-reviews.appspot.com/1383806/diff/3001/user/src/com/google/gwt/cell/client/ButtonCellBase.java
File user/src/com/google/gwt/cell/client/ButtonCellBase.java (right):

http://gwt-code-reviews.appspot.com/1383806/diff/3001/user/src/com/google/gwt/cell/client/ButtonCellBase.java#newcode103
user/src/com/google/gwt/cell/client/ButtonCellBase.java:103:
ImageResource buttonCellBaseBackground();
Forgot to add buttonCellBaseBackground.png to the review.

http://gwt-code-reviews.appspot.com/1383806/diff/3001/user/src/com/google/gwt/user/User.gwt.xml
File user/src/com/google/gwt/user/User.gwt.xml (right):

http://gwt-code-reviews.appspot.com/1383806/diff/3001/user/src/com/google/gwt/user/User.gwt.xml#newcode55
user/src/com/google/gwt/user/User.gwt.xml:55: 
Can you alphabetize these guys?

http://gwt-code-reviews.appspot.com/1383806/diff/3001/user/src/com/google/gwt/user/widget/client/ButtonBase.java
File user/src/com/google/gwt/user/widget/client/ButtonBase.java (right):

http://gwt-code-reviews.appspot.com/1383806/diff/3001/user/src/com/google/gwt/user/widget/client/ButtonBase.java#newcode56
user/src/com/google/gwt/user/widget/client/ButtonBase.java:56: */
There is another class called com.google.gwt.user.client.ui.ButtonBase
and Eclipse loves picking the wrong one just for fun. Is there a way to
avoid that name overlap?

http://gwt-code-reviews.appspot.com/1383806/diff/3001/user/src/com/google/gwt/user/widget/client/TextButton.java
File user/src/com/google/gwt/user/widget/client/TextButton.java (right):

http://gwt-code-reviews.appspot.com/1383806/diff/3001/user/src/com/google/gwt/user/widget/client/TextButton.java#newcode25
user/src/com/google/gwt/user/widget/client/TextButton.java:25: * A
button that displays text and an option icon.
option -> optional

http://gwt-code-reviews.appspot.com/1383806/diff/3001/user/src/com/google/gwt/user/widget/client/TextButton.java#newcode41
user/src/com/google/gwt/user/widget/client/TextButton.java:41: public
static TextButton createDefaultTextButton(String value) {
Subclasses of TextButton won't like these static methods. Should
TextButton just be final?

http://gwt-code-reviews.appspot.com/1383806/diff/3001/user/src/com/google/gwt/user/widget/client/TextButton.java#newcode83
user/src/com/google/gwt/user/widget/client/TextButton.java:83: *
Construct a new {@link TextButton}.
Need to javadoc this. Probably want to include a hint that users can
call createXYZTextButton(), etc.

http://gwt-code-reviews.appspot.com/1383806/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Add media events for audio/video elements (issue1385804)

[pdr]

LGTM


http://gwt-code-reviews.appspot.com/1385804/diff/1014/user/src/com/google/gwt/media/client/Audio.java
File user/src/com/google/gwt/media/client/Audio.java (right):

http://gwt-code-reviews.appspot.com/1385804/diff/1014/user/src/com/google/gwt/media/client/Audio.java#newcode93
user/src/com/google/gwt/media/client/Audio.java:93:
On 2011/03/21 20:53:19, fredsa wrote:

These are documented on the HasCanPlayThroughHandlers,

HasEndedHandlers and

HasProgressHandlers interfaces. Would you still like me to duplicate

the docs

here?



Oops--missed that. No need to duplicate.

I think there's an extra space here now though.

http://gwt-code-reviews.appspot.com/1385804/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Add media events for audio/video elements (issue1385804)

[pdr]

Per our discussion, an overhaul of the event system is required to get
all the media events in, but I think these look good for now. I think
the Error and LoadedData events would also be useful if you have the
time.

Lastly, could you add tests for these events? You may be blocked on the
Audio and Video tests being disabled--if that's so, I am about to put up
a review that re-enables the Audio and Video tests, do you mind waiting
for that to land?


http://gwt-code-reviews.appspot.com/1385804/diff/1014/user/src/com/google/gwt/event/dom/client/EndedHandler.java
File user/src/com/google/gwt/event/dom/client/EndedHandler.java (right):

http://gwt-code-reviews.appspot.com/1385804/diff/1014/user/src/com/google/gwt/event/dom/client/EndedHandler.java#newcode26
user/src/com/google/gwt/event/dom/client/EndedHandler.java:26: * Called
when EndEdEvent is fired.
EndEdEvent -> EndedEvent

http://gwt-code-reviews.appspot.com/1385804/diff/1014/user/src/com/google/gwt/event/dom/client/HasAllMediaHandlers.java
File user/src/com/google/gwt/event/dom/client/HasAllMediaHandlers.java
(right):

http://gwt-code-reviews.appspot.com/1385804/diff/1014/user/src/com/google/gwt/event/dom/client/HasAllMediaHandlers.java#newcode2
user/src/com/google/gwt/event/dom/client/HasAllMediaHandlers.java:2: *
Copyright 2010 Google Inc.
2010 -> 2011

http://gwt-code-reviews.appspot.com/1385804/diff/1014/user/src/com/google/gwt/event/dom/client/HasAllMediaHandlers.java#newcode25
user/src/com/google/gwt/event/dom/client/HasAllMediaHandlers.java:25: *
to it. Therefore, updates can cause breaking API changes.
For consistency, could this be replaced with the experimental warning we
use in some of the other html5 apis?

http://gwt-code-reviews.appspot.com/1385804/diff/1014/user/src/com/google/gwt/event/dom/client/HasCanPlayThroughHandlers.java
File
user/src/com/google/gwt/event/dom/client/HasCanPlayThroughHandlers.java
(right):

http://gwt-code-reviews.appspot.com/1385804/diff/1014/user/src/com/google/gwt/event/dom/client/HasCanPlayThroughHandlers.java#newcode2
user/src/com/google/gwt/event/dom/client/HasCanPlayThroughHandlers.java:2:
* Copyright 2010 Google Inc.
2010 -> 2011

http://gwt-code-reviews.appspot.com/1385804/diff/1014/user/src/com/google/gwt/event/dom/client/HasEndedHandlers.java
File user/src/com/google/gwt/event/dom/client/HasEndedHandlers.java
(right):

http://gwt-code-reviews.appspot.com/1385804/diff/1014/user/src/com/google/gwt/event/dom/client/HasEndedHandlers.java#newcode2
user/src/com/google/gwt/event/dom/client/HasEndedHandlers.java:2: *
Copyright 2010 Google Inc.
2010 -> 2011

http://gwt-code-reviews.appspot.com/1385804/diff/1014/user/src/com/google/gwt/event/dom/client/HasProgressHandlers.java
File user/src/com/google/gwt/event/dom/client/HasProgressHandlers.java
(right):

http://gwt-code-reviews.appspot.com/1385804/diff/1014/user/src/com/google/gwt/event/dom/client/HasProgressHandlers.java#newcode2
user/src/com/google/gwt/event/dom/client/HasProgressHandlers.java:2: *
Copyright 2010 Google Inc.
2010 -> 2011

http://gwt-code-reviews.appspot.com/1385804/diff/1014/user/src/com/google/gwt/media/client/Audio.java
File user/src/com/google/gwt/media/client/Audio.java (right):

http://gwt-code-reviews.appspot.com/1385804/diff/1014/user/src/com/google/gwt/media/client/Audio.java#newcode93
user/src/com/google/gwt/media/client/Audio.java:93:
Can you add javadoc to these?

http://gwt-code-reviews.appspot.com/1385804/diff/1014/user/src/com/google/gwt/media/client/Video.java
File user/src/com/google/gwt/media/client/Video.java (right):

http://gwt-code-reviews.appspot.com/1385804/diff/1014/user/src/com/google/gwt/media/client/Video.java#newcode100
user/src/com/google/gwt/media/client/Video.java:100:
Can you add javadoc to these?

http://gwt-code-reviews.appspot.com/1385804/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Fix Javadoc for gesture/touch events (issue1383805)

[pdr]

On 2011/03/20 04:45:50, fredsa wrote:

LGTM

http://gwt-code-reviews.appspot.com/1383805/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Fixing touch scrolling support for devices that support it natively. Our current touch support o... (issue1369809)

[pdr]

On 2011/03/14 18:29:43, jlabanca wrote:

http://gwt-code-reviews.appspot.com/1369809/diff/4001/user/src/com/google/gwt/event/dom/DomEvent.gwt.xml

File user/src/com/google/gwt/event/dom/DomEvent.gwt.xml (right):



http://gwt-code-reviews.appspot.com/1369809/diff/4001/user/src/com/google/gwt/event/dom/DomEvent.gwt.xml#newcode3

user/src/com/google/gwt/event/dom/DomEvent.gwt.xml:3: 
On 2011/03/14 18:09:14, pdr wrote:
> Can you alphabetize these?



Done.



http://gwt-code-reviews.appspot.com/1369809/diff/4001/user/src/com/google/gwt/event/dom/client/TouchEvent.java

File user/src/com/google/gwt/event/dom/client/TouchEvent.java (right):



http://gwt-code-reviews.appspot.com/1369809/diff/4001/user/src/com/google/gwt/event/dom/client/TouchEvent.java#newcode35

user/src/com/google/gwt/event/dom/client/TouchEvent.java:35: private

static

class TouchSupportDetector {
This would return true for a tablet Android device.  TouchScroller has

the

second check for Android, as you described.



http://gwt-code-reviews.appspot.com/1369809/diff/4001/user/src/com/google/gwt/user/tools/templates/eclipse/_moduleShortName_.launch

File


user/src/com/google/gwt/user/tools/templates/eclipse/_moduleShortName_.launch

(left):



http://gwt-code-reviews.appspot.com/1369809/diff/4001/user/src/com/google/gwt/user/tools/templates/eclipse/_moduleShortName_.launch#oldcode1

user/src/com/google/gwt/user/tools/templates/eclipse/_moduleShortName_.launch:1:


On 2011/03/14 18:09:14, pdr wrote:
> Was deleting this a mistake?



Yes, reverting



http://gwt-code-reviews.appspot.com/1369809/diff/4001/user/test/com/google/gwt/touch/client/TouchScrollTest.java

File user/test/com/google/gwt/touch/client/TouchScrollTest.java

(right):


http://gwt-code-reviews.appspot.com/1369809/diff/4001/user/test/com/google/gwt/touch/client/TouchScrollTest.java#newcode513

user/test/com/google/gwt/touch/client/TouchScrollTest.java:513: }
isTouchSupported() was a helper method to determine if TouchScroller

should be

supported in this test. However, now that the detection is more

complicated,

copying TouchScroller.isSupport() seems redundant. I agree in

principle that it

would be nice to verify that TouchScroller.isSupported() returns the

correct

value on the correct device, but if we just copy the implementation of
TouchScroller.isSupported(), then it isn't really testing anything.


LGTM

http://gwt-code-reviews.appspot.com/1369809/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Fixing touch scrolling support for devices that support it natively. Our current touch support o... (issue1369809)

[pdr]

I had reviewed this after all, but forgot to click send. D'oh! Sorry for
the delay.


http://gwt-code-reviews.appspot.com/1369809/diff/4001/user/src/com/google/gwt/event/dom/DomEvent.gwt.xml
File user/src/com/google/gwt/event/dom/DomEvent.gwt.xml (right):

http://gwt-code-reviews.appspot.com/1369809/diff/4001/user/src/com/google/gwt/event/dom/DomEvent.gwt.xml#newcode3
user/src/com/google/gwt/event/dom/DomEvent.gwt.xml:3: 
Can you alphabetize these?

http://gwt-code-reviews.appspot.com/1369809/diff/4001/user/src/com/google/gwt/event/dom/client/TouchEvent.java
File user/src/com/google/gwt/event/dom/client/TouchEvent.java (right):

http://gwt-code-reviews.appspot.com/1369809/diff/4001/user/src/com/google/gwt/event/dom/client/TouchEvent.java#newcode35
user/src/com/google/gwt/event/dom/client/TouchEvent.java:35: private
static class TouchSupportDetector {
This name is a bit confusing since it will return false for a tablet
Android device, which does fire touch events. If that fact is
javadoc'ed, I think things would be ok. Alternatively, have
TouchSupportDetector and a separate check for the touch-based scrolling.

http://gwt-code-reviews.appspot.com/1369809/diff/4001/user/src/com/google/gwt/user/tools/templates/eclipse/_moduleShortName_.launch
File
user/src/com/google/gwt/user/tools/templates/eclipse/_moduleShortName_.launch
(left):

http://gwt-code-reviews.appspot.com/1369809/diff/4001/user/src/com/google/gwt/user/tools/templates/eclipse/_moduleShortName_.launch#oldcode1
user/src/com/google/gwt/user/tools/templates/eclipse/_moduleShortName_.launch:1:

Was deleting this a mistake?

http://gwt-code-reviews.appspot.com/1369809/diff/4001/user/test/com/google/gwt/touch/client/TouchScrollTest.java
File user/test/com/google/gwt/touch/client/TouchScrollTest.java (right):

http://gwt-code-reviews.appspot.com/1369809/diff/4001/user/test/com/google/gwt/touch/client/TouchScrollTest.java#newcode513
user/test/com/google/gwt/touch/client/TouchScrollTest.java:513: }
You removed a test called isTouchSupported() that I was fond of. I'd
recommend adding a test to double-check that your support check works.
Something like if(browser == android && tablet && elem.ontouchstart ==
"function") { assertTrue(TouchScroller.isSupported()); }

http://gwt-code-reviews.appspot.com/1369809/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Fix bug on IE where onload events don't fire for IFrames. (issue1294801)

[pdr]

http://gwt-code-reviews.appspot.com/1294801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Fix bug on IE where onload events don't fire for IFrames. (issue1294801)

[pdr]

http://gwt-code-reviews.appspot.com/1294801/diff/17001/user/src/com/google/gwt/user/client/ui/Frame.java
File user/src/com/google/gwt/user/client/ui/Frame.java (right):

http://gwt-code-reviews.appspot.com/1294801/diff/17001/user/src/com/google/gwt/user/client/ui/Frame.java#newcode78
user/src/com/google/gwt/user/client/ui/Frame.java:78:
sinkEvents(Event.ONLOAD);
On 2011/03/14 15:10:05, jlabanca wrote:

Don't sink in the constructor unless its used by the Widget itself.
addDomHandler will lazily sink the load event automatically.  This is

important

because sinking events is expensive.


Done.

http://gwt-code-reviews.appspot.com/1294801/diff/17001/user/src/com/google/gwt/user/client/ui/Frame.java#newcode110
user/src/com/google/gwt/user/client/ui/Frame.java:110: return
addHandler(handler, LoadEvent.getType());
On 2011/03/14 15:10:05, jlabanca wrote:

Use addDomHandler() for events that wrap native events.  It will sink

ONLOAD the

first time it is called.


Done.

http://gwt-code-reviews.appspot.com/1294801/diff/17001/user/test/com/google/gwt/dom/public-test/iframetest.html
File user/test/com/google/gwt/dom/public-test/iframetest.html (right):

http://gwt-code-reviews.appspot.com/1294801/diff/17001/user/test/com/google/gwt/dom/public-test/iframetest.html#newcode3
user/test/com/google/gwt/dom/public-test/iframetest.html:3: 
On 2011/03/14 15:10:05, jlabanca wrote:

This doesn't look right

svn diff!! >:(

fixed :)

http://gwt-code-reviews.appspot.com/1294801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Fix bug on IE where onload events don't fire for IFrames. (issue1294801)

[pdr]

http://gwt-code-reviews.appspot.com/1294801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Fix bug on IE where onload events don't fire for IFrames. (issue1294801)

[pdr]

http://gwt-code-reviews.appspot.com/1294801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Fixing touch scrolling support for devices that support it natively. Our current touch support o... (issue1369809)

[pdr]

http://gwt-code-reviews.appspot.com/1369809/diff/1/user/src/com/google/gwt/touch/client/TouchScroller.java
File user/src/com/google/gwt/touch/client/TouchScroller.java (right):

http://gwt-code-reviews.appspot.com/1369809/diff/1/user/src/com/google/gwt/touch/client/TouchScroller.java#newcode173
user/src/com/google/gwt/touch/client/TouchScroller.java:173: }-*/;
Can this be broken out into a module file, or a method in DomImpl?
Also, can you guarantee an Android 3.0 / Honeycomb device will always
have a touchscreen?

http://gwt-code-reviews.appspot.com/1369809/diff/1/user/src/com/google/gwt/touch/client/TouchScroller.java#newcode253
user/src/com/google/gwt/touch/client/TouchScroller.java:253: * @return
true if touch events are supported, false it not
it -> if

http://gwt-code-reviews.appspot.com/1369809/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Adding vertical padding to hborder.png in the new Clean style theme to handle large Buttons/Date... (issue1368808)

[pdr]

On 2011/03/09 15:55:18, jlabanca wrote:

LGTM

http://gwt-code-reviews.appspot.com/1368808/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Replacing the testng-5.14.1.jar with testng-5.14.1-nojunit.jar in the gwt-user .classpath file. ... (issue1377803)

[pdr]

On 2011/03/09 15:23:11, jlabanca wrote:

LGTM

http://gwt-code-reviews.appspot.com/1377803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Fixing ActionCell and ButtonCell clicks outside of the Button element are ignored. (issue1371810)

[pdr]

On 2011/03/07 16:43:50, jlabanca wrote:

adapted from http://gwt-code-reviews.appspot.com/1183801/show


LGTM

http://gwt-code-reviews.appspot.com/1371810/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: HTML5 Storage API in GWT. (issue1374803)

[pdr]

http://gwt-code-reviews.appspot.com/1374803/diff/7004/user/src/com/google/gwt/storage/client/Storage.java
File user/src/com/google/gwt/storage/client/Storage.java (right):

http://gwt-code-reviews.appspot.com/1374803/diff/7004/user/src/com/google/gwt/storage/client/Storage.java#newcode107
user/src/com/google/gwt/storage/client/Storage.java:107: protected
static final StorageImpl impl = GWT.create(StorageImpl.class);
On 2011/03/07 16:06:30, jlabanca wrote:

Protected means that subclasses of Storage can access this. Can you

make is

package protected instead?


Done.

http://gwt-code-reviews.appspot.com/1374803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: HTML5 Storage API in GWT. (issue1374803)

[pdr]

http://gwt-code-reviews.appspot.com/1374803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: HTML5 Storage API in GWT. (issue1374803)

[pdr]

http://gwt-code-reviews.appspot.com/1374803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: HTML5 Storage API in GWT. (issue1374803)

[pdr]

http://gwt-code-reviews.appspot.com/1374803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: HTML5 Storage API in GWT. (issue1374803)

[pdr]

http://gwt-code-reviews.appspot.com/1374803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: HTML5 Storage API in GWT. (issue1374803)

[pdr]

http://gwt-code-reviews.appspot.com/1374803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: HTML5 Storage API in GWT. (issue1374803)

[pdr]

http://gwt-code-reviews.appspot.com/1374803/diff/20/user/src/com/google/gwt/storage/client/StorageImplNonNativeEvents.java
File
user/src/com/google/gwt/storage/client/StorageImplNonNativeEvents.java
(right):

http://gwt-code-reviews.appspot.com/1374803/diff/20/user/src/com/google/gwt/storage/client/StorageImplNonNativeEvents.java#newcode69
user/src/com/google/gwt/storage/client/StorageImplNonNativeEvents.java:69:
protected native void addStorageEventHandler0() /*-{
On 2011/03/04 23:28:56, jlabanca wrote:

make non-native


Done.

http://gwt-code-reviews.appspot.com/1374803/diff/20/user/src/com/google/gwt/storage/client/StorageImplNonNativeEvents.java#newcode74
user/src/com/google/gwt/storage/client/StorageImplNonNativeEvents.java:74:
protected native void removeStorageEventHandler0() /*-{
On 2011/03/04 23:28:56, jlabanca wrote:

make non-native


Done.

http://gwt-code-reviews.appspot.com/1374803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Adding touch scrolling support to ScrollPanel for mobile support. TouchScroller augments an exis... (issue1370801)

[pdr]

LGTM with nits


http://gwt-code-reviews.appspot.com/1370801/diff/7001/user/src/com/google/gwt/touch/client/Momentum.java
File user/src/com/google/gwt/touch/client/Momentum.java (right):

http://gwt-code-reviews.appspot.com/1370801/diff/7001/user/src/com/google/gwt/touch/client/Momentum.java#newcode34
user/src/com/google/gwt/touch/client/Momentum.java:34:
Extra newline

http://gwt-code-reviews.appspot.com/1370801/diff/7001/user/src/com/google/gwt/touch/client/Point.java
File user/src/com/google/gwt/touch/client/Point.java (right):

http://gwt-code-reviews.appspot.com/1370801/diff/7001/user/src/com/google/gwt/touch/client/Point.java#newcode108
user/src/com/google/gwt/touch/client/Point.java:108: * @param c the
value to addF
addF -> add

http://gwt-code-reviews.appspot.com/1370801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: HTML5 Storage API in GWT. (issue1374803)

[pdr]

http://gwt-code-reviews.appspot.com/1374803/diff/1/user/src/com/google/gwt/storage/client/StorageImpl.java
File user/src/com/google/gwt/storage/client/StorageImpl.java (right):

http://gwt-code-reviews.appspot.com/1374803/diff/1/user/src/com/google/gwt/storage/client/StorageImpl.java#newcode193
user/src/com/google/gwt/storage/client/StorageImpl.java:193:
@com.google.gwt.storage.client.StorageImpl::jsHandler = function(event)
{
On 2011/03/04 18:50:34, jlabanca wrote:

This function assignment should be wrapped with $entry to ensure that

it GWT can

capture events properly.  See DOMImplStandard for an example.



@com.google.gwt.storage.client.StorageImpl::jsHandler =

$entry(function(event) {

...
}}});


Done.

http://gwt-code-reviews.appspot.com/1374803/diff/1/user/src/com/google/gwt/storage/client/StorageImpl.java#newcode193
user/src/com/google/gwt/storage/client/StorageImpl.java:193:
@com.google.gwt.storage.client.StorageImpl::jsHandler = function(event)
{
On 2011/03/04 18:50:34, jlabanca wrote:

This function assignment should be wrapped with $entry to ensure that

it GWT can

capture events properly.  See DOMImplStandard for an example.



@com.google.gwt.storage.client.StorageImpl::jsHandler =

$entry(function(event) {

...
}}});


Done.

http://gwt-code-reviews.appspot.com/1374803/diff/1/user/src/com/google/gwt/storage/client/StorageImplIE8.java
File user/src/com/google/gwt/storage/client/StorageImplIE8.java (right):

http://gwt-code-reviews.appspot.com/1374803/diff/1/user/src/com/google/gwt/storage/client/StorageImplIE8.java#newcode33
user/src/com/google/gwt/storage/client/StorageImplIE8.java:33: public
native String key(String storage, int index) /*-{
On 2011/03/04 18:50:34, jlabanca wrote:

This is that same as StorageImplNonNativeEvents.key().  We can just

get ride of

this Impl version.

It was a mistake for them to be the same--StorageImplNonNativeEvents
shouldn't have had the index bounds check. I've removed it from there.

http://gwt-code-reviews.appspot.com/1374803/diff/1/user/src/com/google/gwt/storage/client/StorageImplMozilla.java
File user/src/com/google/gwt/storage/client/StorageImplMozilla.java
(right):

http://gwt-code-reviews.appspot.com/1374803/diff/1/user/src/com/google/gwt/storage/client/StorageImplMozilla.java#newcode34
user/src/com/google/gwt/storage/client/StorageImplMozilla.java:34:
public native String key(String storage, int index) /*-{
On 2011/03/04 18:50:34, jlabanca wrote:

This is that same as StorageImplNonNativeEvents.key().  We can just

get ride of

this Impl version.


StorageImplNonNativeEvents shouldn't have the index bounds check, that
was a mistake. I've removed the check from StorageImplNonNativeEvents so
this is needed after all.

http://gwt-code-reviews.appspot.com/1374803/diff/1/user/src/com/google/gwt/storage/client/StorageImplNonNativeEvents.java
File
user/src/com/google/gwt/storage/client/StorageImplNonNativeEvents.java
(right):

http://gwt-code-reviews.appspot.com/1374803/diff/1/user/src/com/google/gwt/storage/client/StorageImplNonNativeEvents.java#newcode29
user/src/com/google/gwt/storage/client/StorageImplNonNativeEvents.java:29:
class StorageImplNonNativeEvents extends StorageImpl {
On 2011/03/04 18:50:34, jlabanca wrote:

Most of the overrides in this class could be made non-native and use

calls to

the super.method() plus some other stuff.


Was able to remove tons of duplicated code, thanks for pointing that
out.

http://gwt-code-reviews.appspot.com/1374803/diff/1/user/src/com/google/gwt/storage/client/StorageImplNonNativeEvents.java#newcode53
user/src/com/google/gwt/storage/client/StorageImplNonNativeEvents.java:53:
final StorageEvent.Handler handler) {
On 2011/03/04 18:50:34, jlabanca wrote:

It might be cleaner to just override
addStorageEventHandler0/removeStorageEventHandler0 to be no-op

methods.

Done.

http://gwt-code-reviews.appspot.com/1374803/diff/1/user/src/com/google/gwt/storage/client/StorageImplNonNativeEvents.java#newcode64
user/src/com/google/gwt/storage/client/StorageImplNonNativeEvents.java:64:
$wnd[storage].clear();
On 2011/03/04 18:50:34, jlabanca wrote:

Make non-native:
super.clear();
fireStorageEvent(...);


Done.

http://gwt-code-reviews.appspot.com/1374803/diff/1/user/src/com/google/gwt/storage/client/StorageImplNonNativeEvents.java#newcode87
user/src/com/google/gwt/storage/client/StorageImplNonNativeEvents.java:87:
StorageEvent.Handler handler) {
On 2011/03/04 18:50:35, jlabanca wrote:

Override  removeStorageEventHandler0 instead of this method.


Done.

http://gwt-code-reviews.appspot.com/1374803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: HTML5 Storage API in GWT. (issue1374803)

[pdr]

http://gwt-code-reviews.appspot.com/1374803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] HTML5 Storage API in GWT. (issue1374803)

[pdr]

Reviewers: jlabanca,

Description:
HTML5 Storage API in GWT.

This change adds the HTML5 local and session storage APIs, and a Map
interface
backed by storage. This is a contribution from an external project,
the gwt-mobile-webkit project, and is a copy of issue#1290802 with some
additional changes.

Review by: jlaba...@google.com

Please review this at http://gwt-code-reviews.appspot.com/1374803/

Affected files:
  A user/src/com/google/gwt/storage/Storage.gwt.xml
  A user/src/com/google/gwt/storage/client/Storage.java
  A user/src/com/google/gwt/storage/client/StorageEvent.java
  A user/src/com/google/gwt/storage/client/StorageImpl.java
  A user/src/com/google/gwt/storage/client/StorageImplIE8.java
  A user/src/com/google/gwt/storage/client/StorageImplMozilla.java
  A user/src/com/google/gwt/storage/client/StorageImplNonNativeEvents.java
  A user/src/com/google/gwt/storage/client/StorageMap.java
  A user/src/com/google/gwt/storage/client/package.html
  A user/test/com/google/gwt/storage/StorageSuite.java
  A user/test/com/google/gwt/storage/client/LocalStorageMapTest.java
  A user/test/com/google/gwt/storage/client/LocalStorageTest.java
  A user/test/com/google/gwt/storage/client/MapInterfaceTest.java
  A user/test/com/google/gwt/storage/client/SessionStorageMapTest.java
  A user/test/com/google/gwt/storage/client/SessionStorageTest.java
  A user/test/com/google/gwt/storage/client/StorageMapTest.java
  A user/test/com/google/gwt/storage/client/StorageTest.java


--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Fix bug for IE frames where onload events don't fire. (issue1353801)

[pdr]

Step-by-step how I was running the tests:
1) Open Eclipse and go to Run->Run Configurations
2) Select "GWT JUnit Test" on the left and click the new test icon.
3) Put anything for the name, but for the Project put gwt-user and for
the Test Class put com.google.gwt.user.client.ui.ImageTest
4) Click the Arguments tab and under the VM Arguments put:
-Dgwt.args="-runStyle Manual:1 -web -standardsMode"
5) Click Apply, then click Run.
6) After a minute or so, you should see the following in your console:
"Please navigate your browser to this URL:
http://XXYYZZ:58926/com.google.gwt.user.UserTest.JUnit/junit-standards.html";
7) Open IE8 and navigate to the URL, then switch back to Eclipse and
watch the tests run.
8) There will be several failures.

http://gwt-code-reviews.appspot.com/1353801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Adding touch scrolling support to ScrollPanel for mobile support. TouchScroller augments an exis... (issue1370801)

[pdr]

http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/Touch.gwt.xml
File user/src/com/google/gwt/touch/Touch.gwt.xml (right):

http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/Touch.gwt.xml#newcode25
user/src/com/google/gwt/touch/Touch.gwt.xml:25: 
FF4 and IE9 may support touch events for tablets/touch devices. Could
you check for the other browsers as well (FF3.5, etc.)?

It may be best to set the default to "maybe", and set the support of IE6
and other known-not-to-support browsers to "no" so that a user who has
defined a permutation for "mobile-webkit" will get "maybe" as expected.

http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/DefaultMomentum.java
File user/src/com/google/gwt/touch/client/DefaultMomentum.java (right):

http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/DefaultMomentum.java#newcode39
user/src/com/google/gwt/touch/client/DefaultMomentum.java:39: public
Snapshot updateSnapshot(Snapshot snapshot) {
This design feels a little odd. Why not have:
public boolean update(Snapshot snapshot);
That returns false if there is no more momentum, and true if snapshot is
successfully updated?

http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/Momentum.java
File user/src/com/google/gwt/touch/client/Momentum.java (right):

http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/Momentum.java#newcode26
user/src/com/google/gwt/touch/client/Momentum.java:26: * A snapshot of
the current state.
I think users will find this name confusing.

Could State work? Or MomentumState?

http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/Momentum.java#newcode153
user/src/com/google/gwt/touch/client/Momentum.java:153: * This method
can modify the existing snapshot by called
called -> calling

http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/Point.java
File user/src/com/google/gwt/touch/client/Point.java (right):

http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/Point.java#newcode19
user/src/com/google/gwt/touch/client/Point.java:19: * A simple point
class.
This is a generally useful class. Could it be renamed Vector or Vec2 and
moved somewhere more general?

A mul(double c) method would be useful for cleaning up the
DefaultMomentum code.

http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/Point.java#newcode24
user/src/com/google/gwt/touch/client/Point.java:24: private final double
y;
Is it faster to set these as final and create lots of intermediate Point
objects, or to set these as non-final and have methods like void
point.sub(Point p). ?

http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/TouchScroller.java
File user/src/com/google/gwt/touch/client/TouchScroller.java (right):

http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/TouchScroller.java#newcode44
user/src/com/google/gwt/touch/client/TouchScroller.java:44: * Adds touch
based scrolling to a scroll panel.
On the Android browser, dragging feels "choppier" than coasting on
momentum. Can you repro this?

http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/TouchScroller.java#newcode90
user/src/com/google/gwt/touch/client/TouchScroller.java:90:
schedule((int) MS_PER_FRAME);
May be cleaner to use schedulerepeating((int) MS_PER_FRAME) once, then
call cancel() if snapshot == null to stop the timer.

http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/TouchScroller.java#newcode181
user/src/com/google/gwt/touch/client/TouchScroller.java:181: * Runtime
check for whether the canvas element is supported in this browser.
canvas -> scrollable widget

http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/TouchScroller.java#newcode183
user/src/com/google/gwt/touch/client/TouchScroller.java:183: * @return
whether the canvas element is supported
canvas -> scrollable widget

http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/TouchScroller.java#newcode254
user/src/com/google/gwt/touch/client/TouchScroller.java:254: * If the
gesture takes too long, we update the recentTuochPosition to the
recentTuochPosition -> recentTouchPosition

http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/TouchScroller.java#newcode256
user/src/com/google/gwt/touch/client/TouchScroller.java:256: * do this
so that we don't based the velocity on two touch events that
based -> base

http://gwt-code-reviews.appspot.com/1370801/diff/1016/user/src/com/google/gwt/touch/client/TouchScroller.java#newcode537
user/src/com/google/gwt/touch/client/TouchScroller.java:537

[gwt-contrib] Re: Fix bug for IE frames where onload events don't fire. (issue1353801)

[pdr]

During testing, I found this patch causes several timeout errors in
ImageTest.java on IE8:
testChangeImageToClipped()
testCreateImage()
testSetUrlAndVisiblerectOnUnclippedImage()
testNoEventOnReattachInHandler()

http://gwt-code-reviews.appspot.com/1353801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Fix a codegen bug for use of {0,localtime,predef:FOO} (issue1373801)

[pdr]

On 2011/02/28 19:42:53, jat wrote:

LGTM

http://gwt-code-reviews.appspot.com/1373801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Adding new style theme called Clean, and using the new theme in Showcase and in the default GWT ... (issue1330801)

[pdr]

On 2011/02/25 16:51:09, jlabanca wrote:

LGTM

http://gwt-code-reviews.appspot.com/1330801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Fix bug for IE frames where onload events don't fire. (issue1353801)

[pdr]

Thank you for the patch!

LGTM with a few nits. Lets get this into 2.3 and end this travesty of
broken onload events forever :)


http://gwt-code-reviews.appspot.com/1353801/diff/1/user/src/com/google/gwt/user/client/ui/Frame.java
File user/src/com/google/gwt/user/client/ui/Frame.java (right):

http://gwt-code-reviews.appspot.com/1353801/diff/1/user/src/com/google/gwt/user/client/ui/Frame.java#newcode124
user/src/com/google/gwt/user/client/ui/Frame.java:124:
Add javadoc and move this method and move it below the protected
Frame(Element element) method. (If Eclipse is set up to do it,
Source->Sort Members will do the trick.)

http://gwt-code-reviews.appspot.com/1353801/diff/1/user/test/com/google/gwt/dom/client/FrameTests.java
File user/test/com/google/gwt/dom/client/FrameTests.java (right):

http://gwt-code-reviews.appspot.com/1353801/diff/1/user/test/com/google/gwt/dom/client/FrameTests.java#newcode68
user/test/com/google/gwt/dom/client/FrameTests.java:68: @Override
Remove this @Override. Sadly, until GWT2.3 is released, Java 1.5
compatibility is required which doesn't like @Override's on interfaces.

http://gwt-code-reviews.appspot.com/1353801/diff/1/user/test/com/google/gwt/dom/client/FrameTests.java#newcode85
user/test/com/google/gwt/dom/client/FrameTests.java:85: @Override
Remove this @Override (see above)

http://gwt-code-reviews.appspot.com/1353801/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Eclipse autoformatter update: allow assignments to wrap. (issue1354803)

[pdr]

On 2011/02/11 18:51:18, zundel wrote:

Well, I personally like the way it looks:



  this.myPackage = StringInterner.get().intern(
  (myPackage.length() == 0) ? "" : (myPackage + '.'));



becomes:



  this.myPackage =
  StringInterner.get().intern(
  (myPackage.length() == 0) ? "" : (myPackage + '.'));



This is going to cause a lot of churn...


Would it be reasonable to format the entire source tree at once, and
have a single distasteful SVN commit?

http://gwt-code-reviews.appspot.com/1354803/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Fixing a bug in DeckLayoutPanel where animating the current widget causes the current widget to ... (issue1354801)

[pdr]

LGTM

http://gwt-code-reviews.appspot.com/1354801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Adding a LoadingStateEvent to CellList and CellTable so users can receive an event when the data... (issue1338809)

[pdr]

On 2011/02/07 23:45:33, jlabanca wrote:


LGTM


http://gwt-code-reviews.appspot.com/1338809/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Update getGeckoVersion() to support beta versions of Firefox. (issue1343803)

[pdr]

Reviewers: jlabanca,

Description:
Update getGeckoVersion() to support beta versions of Firefox.

Previously, gecko versions of the form 2.0b10 would cause the regex to
get mad because it is missing a second period. This change updates the
gecko version detector to support gecko versions such as 1.2.3, 1.2, and
2.0b10.


Please review this at http://gwt-code-reviews.appspot.com/1343803/show

Affected files:
  M user/src/com/google/gwt/dom/client/DOMImplMozilla.java


Index: user/src/com/google/gwt/dom/client/DOMImplMozilla.java
===
--- user/src/com/google/gwt/dom/client/DOMImplMozilla.java  (revision 9677)
+++ user/src/com/google/gwt/dom/client/DOMImplMozilla.java  (working copy)
@@ -19,12 +19,12 @@
  * Mozilla implementation of StandardBrowser.
  */
 class DOMImplMozilla extends DOMImplStandard {
-
+
   private static native int getGeckoVersion() /*-{
-var result =  
/rv:([0-9]+)\.([0-9]+)\.([0-9]+)?/.exec(navigator.userAgent.toLowerCase());
+var result =  
/rv:([0-9]+)\.([0-9]+)(\.([0-9]+))?.*?/.exec(navigator.userAgent.toLowerCase());

 if (result && result.length >= 3) {
-  var version = (parseInt(result[1]) * 100) + (parseInt(result[2])  
* 1000) +

-parseInt(result.length == 4 ? result[3] : 0);
+  var version = (parseInt(result[1]) * 100) + (parseInt(result[2])  
* 1000) +

+parseInt(result.length >= 5 && !isNaN(result[4]) ? result[4] : 0);
   return version;
 }
 return -1; // not gecko
@@ -40,7 +40,7 @@
 int geckoVersion = getGeckoVersion();
 return (geckoVersion != -1) && (geckoVersion <= 1009000);
   }
-
+
   /**
* Return true if using Gecko 1.9.1 (Firefox 3.5) or earlier.
*


--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Remove unnecessary "script src=..." from MediaTest.gwt.xml to avoid virus scanner false positives (issue1341801)

[pdr]

Reviewers: rice,

Description:
Remove unnecessary "script src=..." from MediaTest.gwt.xml to avoid
virus scanner false positives

A user reported that Sophos antivirus was flagging MediaTest.gwt.xml
(see: http://savmac7-20.p.link.sophos.com/t/en/Mal/Badsrc-D). The script
src=... line isn't necessary anyway, so I'm removing those lines.


Please review this at http://gwt-code-reviews.appspot.com/1341801/show

Affected files:
  M user/test/com/google/gwt/media/MediaTest.gwt.xml


Index: user/test/com/google/gwt/media/MediaTest.gwt.xml
===
--- user/test/com/google/gwt/media/MediaTest.gwt.xml(revision 9659)
+++ user/test/com/google/gwt/media/MediaTest.gwt.xml(working copy)
@@ -16,11 +16,5 @@
   
   

-  

   
-  
-  
-  
-  
-  
 


--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Setting the line-height of the splitter in Vertical/HorizontalSplitPanel to 0px so the font size... (issue1335801)

[pdr]

On 2011/01/28 14:53:05, jlabanca wrote:


LGTM

http://gwt-code-reviews.appspot.com/1335801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Initial version of HTML5 Audio and Video (issue1318803)

[pdr]

Reviewers: rice, jlabanca,

Description:
Initial version of HTML5 Audio and Video
Original patch by rice, updated by pdr.


Please review this at http://gwt-code-reviews.appspot.com/1318803/show

Affected files:
  A user/src/com/google/gwt/dom/client/AudioElement.java
  M user/src/com/google/gwt/dom/client/DOMImplMozilla.java
  M user/src/com/google/gwt/dom/client/Document.java
  A user/src/com/google/gwt/dom/client/MediaElement.java
  A user/src/com/google/gwt/dom/client/VideoElement.java
  A user/src/com/google/gwt/media/Media.gwt.xml
  A user/src/com/google/gwt/media/client/Audio.java
  A user/src/com/google/gwt/media/client/Video.java
  A user/src/com/google/gwt/media/client/package-info.java
  A user/src/com/google/gwt/media/dom/DOM.gwt.xml
  A user/src/com/google/gwt/media/dom/client/MediaError.java
  A user/src/com/google/gwt/media/dom/client/TimeRanges.java
  A user/src/com/google/gwt/media/dom/client/package-info.java
  M user/src/com/google/gwt/user/User.gwt.xml
  A user/test/com/google/gwt/media/MediaSuite.java
  A user/test/com/google/gwt/media/MediaTest.gwt.xml
  A user/test/com/google/gwt/media/client/AudioTest.java
  A user/test/com/google/gwt/media/client/MediaTest.java
  A user/test/com/google/gwt/media/client/VideoTest.java


--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Adding new style theme called Clean, and using the new theme in Showcase and in the default GWT ... (issue1330801)

[pdr]

Few nits about the styles: button image is off by 1px in showcase button
example, and a few blue border elements remain.


http://gwt-code-reviews.appspot.com/1330801/diff/1/8
File
user/src/com/google/gwt/user/theme/chrome/public/gwt/chrome/chrome.css
(right):

http://gwt-code-reviews.appspot.com/1330801/diff/1/8#newcode233
user/src/com/google/gwt/user/theme/chrome/public/gwt/chrome/chrome.css:233:
line-height: 0px;
This seems like a breaking change. Why not just put it in clean.css to
avoid breaking existing implementations? (Here, below, in
chrome_rtl.css, standards*.css, and dark*.css)

http://gwt-code-reviews.appspot.com/1330801/diff/1/13
File user/src/com/google/gwt/user/theme/clean/public/gwt/clean/clean.css
(right):

http://gwt-code-reviews.appspot.com/1330801/diff/1/13#newcode52
user/src/com/google/gwt/user/theme/clean/public/gwt/clean/clean.css:52:
Nit: Be consistent in newlines after }'s.

http://gwt-code-reviews.appspot.com/1330801/diff/1/13#newcode549
user/src/com/google/gwt/user/theme/clean/public/gwt/clean/clean.css:549:
filter: alpha(opacity=40);
Why not opacity=50, since the standards version is .5?

http://gwt-code-reviews.appspot.com/1330801/diff/1/13#newcode573
user/src/com/google/gwt/user/theme/clean/public/gwt/clean/clean.css:573:
filter: alpha(opacity=40);
Why not opacity=50, since the standards version is .5?

http://gwt-code-reviews.appspot.com/1330801/diff/1/13#newcode1044
user/src/com/google/gwt/user/theme/clean/public/gwt/clean/clean.css:1044:
filter: alpha(opacity=40);
Why not opacity=50, since the standards version is .5?

http://gwt-code-reviews.appspot.com/1330801/diff/1/13#newcode1123
user/src/com/google/gwt/user/theme/clean/public/gwt/clean/clean.css:1123:
background: url(images/hborder2.png) repeat-x 0px -30px;;
extra ; at the end.

http://gwt-code-reviews.appspot.com/1330801/diff/1/13#newcode1148
user/src/com/google/gwt/user/theme/clean/public/gwt/clean/clean.css:1148:
background: url(images/hborder2.png) repeat-x 0px -30px;;
extra ; at the end.

http://gwt-code-reviews.appspot.com/1330801/diff/1/13#newcode1158
user/src/com/google/gwt/user/theme/clean/public/gwt/clean/clean.css:1158:
vertical-align: center;
"center" isn't valid for vertical-align, did you mean "middle"?

http://gwt-code-reviews.appspot.com/1330801/diff/1/14
File
user/src/com/google/gwt/user/theme/clean/public/gwt/clean/clean_rtl.css
(right):

http://gwt-code-reviews.appspot.com/1330801/diff/1/14#newcode549
user/src/com/google/gwt/user/theme/clean/public/gwt/clean/clean_rtl.css:549:
filter: alpha(opacity=40);
Why not opacity=50, since the standards version is .5?

http://gwt-code-reviews.appspot.com/1330801/diff/1/14#newcode573
user/src/com/google/gwt/user/theme/clean/public/gwt/clean/clean_rtl.css:573:
filter: alpha(opacity=40);
Why not opacity=50, since the standards version is .5?

http://gwt-code-reviews.appspot.com/1330801/diff/1/14#newcode1018
user/src/com/google/gwt/user/theme/clean/public/gwt/clean/clean_rtl.css:1018:
filter: alpha(opacity=40);
Why not opacity=50, since the standards version is .5?

http://gwt-code-reviews.appspot.com/1330801/diff/1/14#newcode1044
user/src/com/google/gwt/user/theme/clean/public/gwt/clean/clean_rtl.css:1044:
filter: alpha(opacity=40);
Why not opacity=50, since the standards version is .5?

http://gwt-code-reviews.appspot.com/1330801/diff/1/14#newcode1124
user/src/com/google/gwt/user/theme/clean/public/gwt/clean/clean_rtl.css:1124:
background: url(images/hborder2.png) repeat-x 0px -30px;;
Extra ;

http://gwt-code-reviews.appspot.com/1330801/diff/1/14#newcode1149
user/src/com/google/gwt/user/theme/clean/public/gwt/clean/clean_rtl.css:1149:
background: url(images/hborder2.png) repeat-x 0px -30px;;
Extra ;

http://gwt-code-reviews.appspot.com/1330801/diff/1/14#newcode1159
user/src/com/google/gwt/user/theme/clean/public/gwt/clean/clean_rtl.css:1159:
vertical-align: center;
"center" isn't valid for vertical-align, did you mean "middle"?

http://gwt-code-reviews.appspot.com/1330801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Remove experimental warning from Canvas and clean up javadoc. (issue1286802)

[pdr]

Reviewers: rjrjr,

Description:
Remove experimental warning from Canvas and clean up javadoc.


Please review this at http://gwt-code-reviews.appspot.com/1286802/show

Affected files:
  M user/src/com/google/gwt/canvas/client/Canvas.java
  M user/src/com/google/gwt/canvas/dom/client/CanvasGradient.java
  M user/src/com/google/gwt/canvas/dom/client/CanvasPattern.java
  M user/src/com/google/gwt/canvas/dom/client/CanvasPixelArray.java
  M user/src/com/google/gwt/canvas/dom/client/Context.java
  M user/src/com/google/gwt/canvas/dom/client/Context2d.java
  M user/src/com/google/gwt/canvas/dom/client/CssColor.java
  M user/src/com/google/gwt/canvas/dom/client/FillStrokeStyle.java
  M user/src/com/google/gwt/canvas/dom/client/ImageData.java
  M user/src/com/google/gwt/canvas/dom/client/TextMetrics.java
  M user/src/com/google/gwt/dom/client/CanvasElement.java


--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Adding a new widget ResizeLayoutPanel that can trigger a resize event when it changes size. The ... (issue1301801)

[pdr]

LGTM


http://gwt-code-reviews.appspot.com/1301801/diff/5001/6001
File user/src/com/google/gwt/user/ResizeLayoutPanel.gwt.xml (right):

http://gwt-code-reviews.appspot.com/1301801/diff/5001/6001#newcode23
user/src/com/google/gwt/user/ResizeLayoutPanel.gwt.xml:23: 
Instead of explicitly listing out the browsers that do not support
onresize, can you provide a default and override that default for
IE/IE6? For example:
http://gwt-code-reviews.appspot.com/1296801/patch/8006/39001

http://gwt-code-reviews.appspot.com/1301801/diff/5001/6003
File user/src/com/google/gwt/user/client/ui/HeaderPanel.java (right):

http://gwt-code-reviews.appspot.com/1301801/diff/5001/6003#newcode42
user/src/com/google/gwt/user/client/ui/HeaderPanel.java:42: private
final ResizeLayoutPanel.Impl footerImpl =
GWT.create(ResizeLayoutPanel.Impl.class);

80 chars


http://gwt-code-reviews.appspot.com/1301801/diff/5001/6003#newcode45
user/src/com/google/gwt/user/client/ui/HeaderPanel.java:45: private
final ResizeLayoutPanel.Impl headerImpl =
GWT.create(ResizeLayoutPanel.Impl.class);

80 chars


http://gwt-code-reviews.appspot.com/1301801/diff/5001/6007
File user/test/com/google/gwt/user/client/ui/ResizeLayoutPanelTest.java
(right):

http://gwt-code-reviews.appspot.com/1301801/diff/5001/6007#newcode292
user/test/com/google/gwt/user/client/ui/ResizeLayoutPanelTest.java:292:
handler.assertResizeFired(false);
Is the second call to assertResizeFired() useful?

http://gwt-code-reviews.appspot.com/1301801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Adding a new widget ResizeLayoutPanel that can trigger a resize event when it changes size. The ... (issue1301801)

[pdr]

http://gwt-code-reviews.appspot.com/1301801/diff/1/4
File user/src/com/google/gwt/user/client/ui/HeaderPanel.java (right):

http://gwt-code-reviews.appspot.com/1301801/diff/1/4#newcode37
user/src/com/google/gwt/user/client/ui/HeaderPanel.java:37:
Rendering bug: vertical scrollbars will be hidden in the header and
footer widgets.

http://gwt-code-reviews.appspot.com/1301801/diff/1/4#newcode135
user/src/com/google/gwt/user/client/ui/HeaderPanel.java:135: public
Iterator iterator() {
It seems odd to have an iterator over a 3-element panel. Why is it
useful?

Also, JavaDoc.

http://gwt-code-reviews.appspot.com/1301801/diff/1/4#newcode346
user/src/com/google/gwt/user/client/ui/HeaderPanel.java:346: int
remainingHeight = getOffsetHeight();
Should this be getElement().getClientHeight() instead of
getOffsetHeight() so that borders/padding don't cause the middle element
to overlap the footer panel?

http://gwt-code-reviews.appspot.com/1301801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Fixing HasWidgetsTester#testAll to actually call testDoDetachChildrenWithError. Currently, it c... (issue1300801)

[pdr]

On 2011/01/18 14:46:40, jlabanca wrote:


LGTM

http://gwt-code-reviews.appspot.com/1300801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Improve canvas for browsers (and permutations) with partial canvas support. (issue1296801)

[pdr]

http://gwt-code-reviews.appspot.com/1296801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Improve canvas for browsers (and permutations) with partial canvas support. (issue1296801)

[pdr]

http://gwt-code-reviews.appspot.com/1296801/diff/28001/29002
File user/src/com/google/gwt/canvas/client/Canvas.java (right):

http://gwt-code-reviews.appspot.com/1296801/diff/28001/29002#newcode64
user/src/com/google/gwt/canvas/client/Canvas.java:64:
CanvasElementSupportDetector.class);
On 2011/01/19 00:30:28, jlabanca wrote:

You shouldn't GWT.create() on every call to isSupported.  Create a

static

variable and do a null check here, GWT.creating only the first time.

In fact,

you might just cache the return value after its called once.



Sorry I missed this before.


Done.

http://gwt-code-reviews.appspot.com/1296801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Improve canvas for browsers (and permutations) with partial canvas support. (issue1296801)

[pdr]

http://gwt-code-reviews.appspot.com/1296801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Improve canvas for browsers (and permutations) with partial canvas support. (issue1296801)

[pdr]

http://gwt-code-reviews.appspot.com/1296801/diff/21001/22002
File user/src/com/google/gwt/canvas/client/Canvas.java (right):

http://gwt-code-reviews.appspot.com/1296801/diff/21001/22002#newcode61
user/src/com/google/gwt/canvas/client/Canvas.java:61: public static
final boolean isSupported() {
On 2011/01/18 20:30:22, jat wrote:

final is meaningless on static methods, right?


Yes. Removed.

http://gwt-code-reviews.appspot.com/1296801/diff/21001/22002#newcode74
user/src/com/google/gwt/canvas/client/Canvas.java:74: * Protected
constructor. Use {@link #createIfSupported()} to create a Canvas.
On 2011/01/18 20:30:22, jat wrote:

If this is protected, you should also talk about how subclasses should

use it.

For example, you might want to say that they should never have a

public

constructor, or else you need to specify that this can throw

exceptions if

called when not supported.



If you don't expect subclasses, it might be better to make this

private for now,

and then you can define what subclasses do when it becomes useful to

subclass.

Decided to put it as private. We discussed leaving it as protected but
the subclass route opens up a can of worms factory that I think would be
more harm than good.

http://gwt-code-reviews.appspot.com/1296801/diff/21001/22003
File user/src/com/google/gwt/dom/client/PartialSupport.java (right):

http://gwt-code-reviews.appspot.com/1296801/diff/21001/22003#newcode22
user/src/com/google/gwt/dom/client/PartialSupport.java:22: * By
convention, classes annotated with PartialSupport will provide two
static methods:
On 2011/01/18 20:30:22, jat wrote:

>80 chars, throughout the change.


Done.

http://gwt-code-reviews.appspot.com/1296801/diff/21001/22003#newcode26
user/src/com/google/gwt/dom/client/PartialSupport.java:26: *   
static createIfSupported() factory method that returns a
new feature
On 2011/01/18 20:30:22, jat wrote:

Should this mention the return type?


Done.

http://gwt-code-reviews.appspot.com/1296801/diff/21001/22004
File user/src/com/google/gwt/user/client/IsSupported.java (left):

http://gwt-code-reviews.appspot.com/1296801/diff/21001/22004#oldcode32
user/src/com/google/gwt/user/client/IsSupported.java:32: public
interface IsSupported {
On 2011/01/18 21:14:25, jlabanca wrote:

Is this already in GWT 2.1.1?  If so, we probably don't want to pull

it,

especially if we are just replacing it with another empty interface.



I don't think we need PartialSupport, but if we do, can it just extend
IsSupported?


IsSupported isn't in 2.1.1 so we're free to change it as needed. I think
some sort of annotation/marker is good since this is such a large break
from GWT's norm.

http://gwt-code-reviews.appspot.com/1296801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Improve canvas for browsers (and permutations) with partial canvas support. (issue1296801)

[pdr]

http://gwt-code-reviews.appspot.com/1296801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Fix bug on IE where onload events don't fire for IFrames. (issue1294801)

[pdr]

http://gwt-code-reviews.appspot.com/1294801/diff/3001/4002
File user/test/com/google/gwt/dom/client/FrameTests.java (right):

http://gwt-code-reviews.appspot.com/1294801/diff/3001/4002#newcode49
user/test/com/google/gwt/dom/client/FrameTests.java:49: final Timer
timer = new Timer() {
On 2011/01/14 19:14:48, jlabanca wrote:

Do you need this timer?  The test will time out if onload is never

triggered.

Done.

http://gwt-code-reviews.appspot.com/1294801/diff/3001/4002#newcode75
user/test/com/google/gwt/dom/client/FrameTests.java:75:
delayTestFinish(2 * delayMillis);
On 2011/01/14 19:14:48, jlabanca wrote:

Move this above the line where you create the iframe to avoid timing

issues,

especially in HtmlUnit.  If the iframe happens to load synchronously

or very

quickly, the onload event might fire before delayTestFinish.  In

general, always

call delayTestFinish() before calling the thing that will trigger an
asynchronous event.


Done.

http://gwt-code-reviews.appspot.com/1294801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Fix bug on IE where onload events don't fire for IFrames. (issue1294801)

[pdr]

http://gwt-code-reviews.appspot.com/1294801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Improve canvas for browsers (and permutations) with partial canvas support. (issue1296801)

[pdr]

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

[gwt-contrib] Re: Improve canvas for browsers (and permutations) with partial canvas support. (issue1296801)

[pdr]

http://gwt-code-reviews.appspot.com/1296801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Improve canvas for browsers (and permutations) with partial canvas support. (issue1296801)

[pdr]

http://gwt-code-reviews.appspot.com/1296801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Improve canvas for browsers (and permutations) with partial canvas support. (issue1296801)

[pdr]

http://gwt-code-reviews.appspot.com/1296801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Improve canvas for browsers (and permutations) with partial canvas support. (issue1296801)

[pdr]

Reviewers: jlabanca, rice,

Description:
Improve canvas for browsers (and permutations) with partial canvas
support.


Please review this at http://gwt-code-reviews.appspot.com/1296801/show

Affected files:
  M user/src/com/google/gwt/canvas/client/Canvas.java
  M user/src/com/google/gwt/canvas/dom/DOM.gwt.xml
  A  
user/src/com/google/gwt/canvas/dom/client/CanvasElementSupportDetectedMaybe.java
  A  
user/src/com/google/gwt/canvas/dom/client/CanvasElementSupportDetectedNo.java
  A  
user/src/com/google/gwt/canvas/dom/client/CanvasElementSupportDetector.java

  M user/test/com/google/gwt/canvas/client/CanvasTest.java
  M user/test/com/google/gwt/canvas/dom/client/Context2dTest.java


--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Fix bug on IE where onload events don't fire for IFrames. (issue1294801)

[pdr]

http://gwt-code-reviews.appspot.com/1294801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Fix bug on IE where onload events don't fire for IFrames. (issue1294801)

[pdr]

Reviewers: jlabanca, rjrjr,

Description:
Fix bug on IE where onload events don't fire for IFrames.

This is a bugfix for
http://code.google.com/p/google-web-toolkit/issues/detail?id=1720


Please review this at http://gwt-code-reviews.appspot.com/1294801/show

Affected files:
  M user/src/com/google/gwt/user/client/impl/DOMImplTrident.java
  M user/test/com/google/gwt/dom/client/FrameTests.java
  A user/test/com/google/gwt/dom/public-test/iframetest.html


--
http://groups.google.com/group/Google-Web-Toolkit-Contributors