LGTM. Nothing controversial I believe. This is a pretty big change though, so maybe an additional pair of eyes would be better (but maybe you also have an internal review ongoing?)
http://gwt-code-reviews.appspot.com/1827804/diff/1/user/src/com/google/gwt/core/client/impl/Impl.java File user/src/com/google/gwt/core/client/impl/Impl.java (right): http://gwt-code-reviews.appspot.com/1827804/diff/1/user/src/com/google/gwt/core/client/impl/Impl.java#newcode279 user/src/com/google/gwt/core/client/impl/Impl.java:279: if (unloadSupport.isUnloadSupported() && Impl.isModuleUnloaded()) { On 2012/10/08 22:27:38, cromwellian wrote:
On 2012/09/14 09:24:16, tbroyer wrote: > "Impl." qualifier is superfluous
Done.
Apparently not. http://gwt-code-reviews.appspot.com/1827804/diff/1/user/src/com/google/gwt/core/client/impl/UnloadSupportEnabled.java File user/src/com/google/gwt/core/client/impl/UnloadSupportEnabled.java (right): http://gwt-code-reviews.appspot.com/1827804/diff/1/user/src/com/google/gwt/core/client/impl/UnloadSupportEnabled.java#newcode93 user/src/com/google/gwt/core/client/impl/UnloadSupportEnabled.java:93: GWT.UncaughtExceptionHandler uncaughtExceptionHandler = GWT.getUncaughtExceptionHandler(); On 2012/09/14 17:23:41, cromwellian wrote:
I agree with the umbrella exception, but this method is also called
from JSNI
directly in some spots. We don't want a bubbling exception to prevent
removal
from disposeables (it could be moved before the call to d.dispose())
but more
generally, probaly don't want all of the try/catch logic in JS.
I could split this into a version that has the try catch (for JSNI
calls) and
leave a version for Java callers without.
Maybe add a comment at least, documenting why it's needed (called directly from JSNI outside $entry scope), so that someone revisiting this code in a few months won't be tempted to remove it. I otherwise like the idea of splitting this method (most probably rather add Impl.disposeWithoutError with the try/catch, called from JSNI, and let Impl.dispose simply call UnloadSupport#dispose, to be called from Java, and remove the try/catch from here; and of course add try/catch in disposeAll to build an UmbrellaException) http://gwt-code-reviews.appspot.com/1827804/diff/1/user/src/com/google/gwt/user/client/ui/PotentialElement.java File user/src/com/google/gwt/user/client/ui/PotentialElement.java (right): http://gwt-code-reviews.appspot.com/1827804/diff/1/user/src/com/google/gwt/user/client/ui/PotentialElement.java#newcode49 user/src/com/google/gwt/user/client/ui/PotentialElement.java:49: $wnd.GwtPotentialElementShim = null; On 2012/10/08 22:27:38, cromwellian wrote:
I removed it for now, but I think I will have to use a global eval/set-timeout/script-tag-inject trick to have it constructed via a
string in
the host page. The shim code is clearly residing in the <IFRAME>'s
module
source, and letting the host page hold a reference to it will likely
cause the
entire source to be retained (because of the way the VM preserves
debugging
info)
Does it really need to be a global in $wnd? Can't it be in 'window' instead and/or possibly use the module name as a prefix/suffix? (similar to DOMImplTrident's window['__gwt_dispatchEvent_' + moduleName]) http://gwt-code-reviews.appspot.com/1827804/diff/10001/user/src/com/google/gwt/core/client/GWT.java File user/src/com/google/gwt/core/client/GWT.java (right): http://gwt-code-reviews.appspot.com/1827804/diff/10001/user/src/com/google/gwt/core/client/GWT.java#newcode88 user/src/com/google/gwt/core/client/GWT.java:88: public static void exportUnloadModule() { How about a com.google.gwt.core.UnloadEnabled module that defines gwt.unloadEnabled to true and defines an entry-point that automatically calls exportUnloadModule? Or possibly simply define the entry-point in Core.gwt.xml (exportUnloadModule will be a no-op when unload support is disabled so it should be compiled out anyway) http://gwt-code-reviews.appspot.com/1827804/diff/10001/user/src/com/google/gwt/core/client/GWT.java#newcode293 user/src/com/google/gwt/core/client/GWT.java:293: * cleaned up. This memory is not typically called by the GWT module itself, but exported so that another module Did you mean 'method' rather than 'memory' here? http://gwt-code-reviews.appspot.com/1827804/diff/10001/user/src/com/google/gwt/core/client/impl/Impl.java File user/src/com/google/gwt/core/client/impl/Impl.java (right): http://gwt-code-reviews.appspot.com/1827804/diff/10001/user/src/com/google/gwt/core/client/impl/Impl.java#newcode248 user/src/com/google/gwt/core/client/impl/Impl.java:248: private static void disposeAll() { There should probably be a comment here explaining where this method is called from? http://gwt-code-reviews.appspot.com/1827804/diff/10001/user/src/com/google/gwt/core/client/impl/UnloadSupportEnabled.java File user/src/com/google/gwt/core/client/impl/UnloadSupportEnabled.java (right): http://gwt-code-reviews.appspot.com/1827804/diff/10001/user/src/com/google/gwt/core/client/impl/UnloadSupportEnabled.java#newcode63 user/src/com/google/gwt/core/client/impl/UnloadSupportEnabled.java:63: var $moduleName = __gwtModuleFunction.__moduleName; Maybe add a comment that unfortunately GWT.getModuleName() isn't assigned yet? http://gwt-code-reviews.appspot.com/1827804/diff/10001/user/src/com/google/gwt/core/client/impl/UnloadSupportEnabled.java#newcode76 user/src/com/google/gwt/core/client/impl/UnloadSupportEnabled.java:76: // Browsers without Object.keys don't benefit from nulling windowj typo on 'windowj' http://gwt-code-reviews.appspot.com/1827804/diff/10001/user/src/com/google/gwt/core/client/impl/UnloadSupportEnabled.java#newcode78 user/src/com/google/gwt/core/client/impl/UnloadSupportEnabled.java:78: var frame = window.frameElement; Should a similar comment be added here about frameElement support? // Browsers without window.frameElement don't benefit from removing the iframe http://gwt-code-reviews.appspot.com/1827804/diff/10001/user/src/com/google/gwt/core/client/impl/UnloadSupportEnabled.java#newcode157 user/src/com/google/gwt/core/client/impl/UnloadSupportEnabled.java:157: final int timerId = UnloadSupport.setTimeout0(func, time, disposable); nit: "UnloadSupport." qualifier is superfluous http://gwt-code-reviews.appspot.com/1827804/diff/10001/user/src/com/google/gwt/user/cellview/client/CellBasedWidgetImplStandard.java File user/src/com/google/gwt/user/cellview/client/CellBasedWidgetImplStandard.java (right): http://gwt-code-reviews.appspot.com/1827804/diff/10001/user/src/com/google/gwt/user/cellview/client/CellBasedWidgetImplStandard.java#newcode140 user/src/com/google/gwt/user/cellview/client/CellBasedWidgetImplStandard.java:140: @com.google.gwt.user.client.impl.DOMImpl::addDisposableEvent(Lcom/google/gwt/dom/client/Element;Ljava/lang/String;Lcom/google/gwt/core/client/JavaScriptObject;Z)(elem, Use "addDisposableEvent(*)(elem, ..." to make it more readable? http://gwt-code-reviews.appspot.com/1827804/diff/10001/user/src/com/google/gwt/user/client/History.java File user/src/com/google/gwt/user/client/History.java (right): http://gwt-code-reviews.appspot.com/1827804/diff/10001/user/src/com/google/gwt/user/client/History.java#newcode79 user/src/com/google/gwt/user/client/History.java:79: impl.dispose(); Couldn't it be simplified to Impl.dispose(impl) here? (and letting HistoryImpl implement Disposable) Or are you expecting to make Disposable compile out completely when unload support is disabled? http://gwt-code-reviews.appspot.com/1827804/diff/10001/user/src/com/google/gwt/user/client/impl/DOMImpl.java File user/src/com/google/gwt/user/client/impl/DOMImpl.java (right): http://gwt-code-reviews.appspot.com/1827804/diff/10001/user/src/com/google/gwt/user/client/impl/DOMImpl.java#newcode24 user/src/com/google/gwt/user/client/impl/DOMImpl.java:24: import com.google.gwt.user.client.DOM; Argl, circular dependency: DOM calls DOMImpl which calls DOM. Anyway, this circular dependency is not needed: all methods from DOM that we all here actually simply defer to DOMImpl, so we can just s/DOM\./this./ http://gwt-code-reviews.appspot.com/1827804/diff/10001/user/src/com/google/gwt/user/client/impl/DOMImpl.java#newcode42 user/src/com/google/gwt/user/client/impl/DOMImpl.java:42: elem.__gwt_disposeEvent.push({event: event, handler: handler, capture: capture}); Note JsArray uses the 'arr[arr.length] = x' idiom as it was benchmarked faster than 'arr.push(x)' IIRC. http://gwt-code-reviews.appspot.com/1827804/diff/10001/user/src/com/google/gwt/user/client/impl/DOMImpl.java#newcode85 user/src/com/google/gwt/user/client/impl/DOMImpl.java:85: private static boolean isJavaObjectFromOurModule(EventListener listener) { We already have isMyListener. Either use it, or rewrite it to use isJavaObjectFromOurModule. http://gwt-code-reviews.appspot.com/1827804/diff/10001/user/src/com/google/gwt/user/client/impl/DOMImpl.java#newcode93 user/src/com/google/gwt/user/client/impl/DOMImpl.java:93: for (var diEvent in diEvents) { diEvents is supposed to be an array, arrays shouldn't be iterated with a for(…in…) loop: for (var i = 0, l = diEvents.length; i < l; i++) { var diEvent = diEvents[i]; http://gwt-code-reviews.appspot.com/1827804/diff/10001/user/src/com/google/gwt/user/client/impl/DOMImplTrident.java File user/src/com/google/gwt/user/client/impl/DOMImplTrident.java (right): http://gwt-code-reviews.appspot.com/1827804/diff/10001/user/src/com/google/gwt/user/client/impl/DOMImplTrident.java#newcode233 user/src/com/google/gwt/user/client/impl/DOMImplTrident.java:233: protected void disposeEventSystem() { Add a TODO? http://gwt-code-reviews.appspot.com/1827804/diff/10001/user/src/com/google/gwt/user/client/impl/HistoryImpl.java File user/src/com/google/gwt/user/client/impl/HistoryImpl.java (right): http://gwt-code-reviews.appspot.com/1827804/diff/10001/user/src/com/google/gwt/user/client/impl/HistoryImpl.java#newcode61 user/src/com/google/gwt/user/client/impl/HistoryImpl.java:61: private JavaScriptObject oldHandler; nit: missing new line above this one http://gwt-code-reviews.appspot.com/1827804/diff/10001/user/src/com/google/gwt/user/client/ui/impl/FocusImplStandard.java File user/src/com/google/gwt/user/client/ui/impl/FocusImplStandard.java (right): http://gwt-code-reviews.appspot.com/1827804/diff/10001/user/src/com/google/gwt/user/client/ui/impl/FocusImplStandard.java#newcode60 user/src/com/google/gwt/user/client/ui/impl/FocusImplStandard.java:60: @com.google.gwt.user.client.impl.DOMImpl::addDisposableEvent(Lcom/google/gwt/dom/client/Element;Ljava/lang/String;Lcom/google/gwt/core/client/JavaScriptObject;Z)(input, addDisposableEvent(*) http://gwt-code-reviews.appspot.com/1827804/ -- -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
