LGTM (BTW why was the GWT-Contrib group removed from CC?)
http://gwt-code-reviews.appspot.com/1880803/diff/1/user/src/com/google/gwt/resources/rg/ImageBundleBuilder.java File user/src/com/google/gwt/resources/rg/ImageBundleBuilder.java (right): http://gwt-code-reviews.appspot.com/1880803/diff/1/user/src/com/google/gwt/resources/rg/ImageBundleBuilder.java#newcode778 user/src/com/google/gwt/resources/rg/ImageBundleBuilder.java:778: if (imageStream != null) { On 2013/01/04 01:15:25, Colin Alworth wrote:
On 2013/01/03 23:46:58, tbroyer wrote: > There should be a try/catch for each stream; and we should probably
close the
> MemoryCacheImageInputStream before the underlying InputStream. Using Utility.close deals with the separate try/catch issue, and I
believe they
are already in the correct order?
Ah yes sorry, maybe the variable names should be changed then, I thought 'imageStream' would be the ImageInputStream and 'input' were the InputStream. How about 'is' for the InputStream? http://gwt-code-reviews.appspot.com/1880803/diff/7001/dev/core/src/com/google/gwt/util/tools/Utility.java File dev/core/src/com/google/gwt/util/tools/Utility.java (right): http://gwt-code-reviews.appspot.com/1880803/diff/7001/dev/core/src/com/google/gwt/util/tools/Utility.java#newcode54 dev/core/src/com/google/gwt/util/tools/Utility.java:54: } catch (IOException e) { On 2013/01/04 01:31:40, jens wrote:
How about logging this exception as it is an I/O error and could
indicate
failing hardware? Personally I think completely ignoring this exception just because you
can not
recover from it is bad practice.
While I agree on principle, I think this is best left for another CL. Guava has a nice Closeables utility [1] that we might want to use, but that means replacing all uses of Utility.close(Closeable) with Closeables.close(Closeable,boolean) and adding a boolean in these try/finally to track whether to swallow the exception. [1] http://docs.guava-libraries.googlecode.com/git-history/release/javadoc/com/google/common/io/Closeables.html#close(java.io.Closeable, boolean) http://gwt-code-reviews.appspot.com/1880803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
