Permalink
Browse files

Makes sure no unload handler is bound when not in IE. Also simplifies…

… the whole "on unload abort" code. Also avoids the declaration of yet another variables in the jQuery main closure for the temporary XHR used to assess support properties.
  • Loading branch information...
1 parent 60cfab3 commit a28eadff48d5e56bb529fc7a5c7e1a74e46398ed @jaubourg jaubourg committed Apr 21, 2011
Showing with 26 additions and 30 deletions.
  1. +26 −30 src/ajax/xhr.js
View
@@ -1,21 +1,14 @@
(function( jQuery ) {
-var // #5280: next active xhr id and list of active xhrs' callbacks
- xhrId = jQuery.now(),
- xhrCallbacks,
-
- // XHR used to determine supports properties
- testXHR;
-
-// #5280: Internet Explorer will keep connections alive if we don't abort on unload
-function xhrOnUnloadAbort() {
- jQuery( window ).unload(function() {
+var // #5280: Internet Explorer will keep connections alive if we don't abort on unload
+ xhrOnUnloadAbort = window.ActiveXObject ? function() {
@cmcnulty

cmcnulty Oct 5, 2011

Contributor

Why are you obfuscating your browser sniffing. Better to be honest about it with .browser

@rwaldron

rwaldron Oct 6, 2011

Member

We don't use jQuery.browser in the core

@cmcnulty

cmcnulty Oct 6, 2011

Contributor

Of course not, because the core isn't supposed to contain browser sniffing, which is what this clearly is. To say that you don't use jQuery.browser in the core is completely without meaning if it's just going to replaced with a "window.ActiveXObject" every time you want to clean up an IE bug. It's better to just use jQuery.browser, because then at least it's obvious what's going on.

@jdalton

jdalton Oct 6, 2011

Member

Ya this is a pretty weak inference because it's if (x) { assumeY() }
I would dig it if more research went into the problem to narrow down which versions of IE have this problem and then re-focusing the weak inference a little more than just window.ActiveXObject (if I remember correctly some old versions of Opera had an ActiveXObject).

@rwaldron

rwaldron Oct 6, 2011

Member

@jdalton I'll ping you on Irc to discuss solutions :)

@jaubourg

jaubourg Oct 6, 2011

Member
  1. we most probably don't support these old versions of Opera
  2. it's impossible to feature test something that has a side-effect AFTER a page reload/load

Weak inference, for sure, but, unless we wanna use $.browser to control version, that's probably the best we can do.

Also, remember this is just here to avoid some silly side-effect in other browsers.

@jdalton

jdalton Oct 6, 2011

Member
  1. we most probably don't support these old versions of Opera

It was just an example of how weak that inference is... too weak

  1. it's impossible to feature test something that has a side-effect AFTER a page reload/load
    Weak inference, for sure, but, unless we wanna use $.browser to control version, that's probably the best we can do.
    Also, remember this is just here to avoid some silly side-effect in other browsers.

Yap it's gonna have to be a weak inference for sure but it should be reworked to be less greedy.

// Abort all pending requests
for ( var key in xhrCallbacks ) {
xhrCallbacks[ key ]( 0, 1 );
}
- });
-}
+ } : false,
+ xhrId = 0,
+ xhrCallbacks;
// Functions to create xhrs
function createStandardXHR() {
@@ -45,15 +38,13 @@ jQuery.ajaxSettings.xhr = window.ActiveXObject ?
// For all other browsers, use the standard XMLHttpRequest object
createStandardXHR;
-// Test if we can create an xhr object
-testXHR = jQuery.ajaxSettings.xhr();
-jQuery.support.ajax = !!testXHR;
-
-// Does this browser support crossDomain XHR requests
-jQuery.support.cors = testXHR && ( "withCredentials" in testXHR );
-
-// No need for the temporary xhr anymore
-testXHR = undefined;
+// Determine support properties
+(function( xhr ) {
+ jQuery.extend( jQuery.support, {
+ ajax: !!xhr,
+ cors: !!xhr && ( "withCredentials" in xhr )
+ });
+})( jQuery.ajaxSettings.xhr() );
// Create transport if the browser can provide an xhr
if ( jQuery.support.ajax ) {
@@ -136,7 +127,9 @@ if ( jQuery.support.ajax ) {
// Do not keep as active anymore
if ( handle ) {
xhr.onreadystatechange = jQuery.noop;
- delete xhrCallbacks[ handle ];
+ if ( xhrOnUnloadAbort ) {
+ delete xhrCallbacks[ handle ];
+ }
}
// If it's an abort
@@ -197,15 +190,18 @@ if ( jQuery.support.ajax ) {
if ( !s.async || xhr.readyState === 4 ) {
callback();
} else {
- // Create the active xhrs callbacks list if needed
- // and attach the unload handler
- if ( !xhrCallbacks ) {
- xhrCallbacks = {};
- xhrOnUnloadAbort();
+ handle = ++xhrId;
+ if ( xhrOnUnloadAbort ) {
+ // Create the active xhrs callbacks list if needed
+ // and attach the unload handler
+ if ( !xhrCallbacks ) {
+ xhrCallbacks = {};
+ jQuery( window ).unload( xhrOnUnloadAbort );
+ }
+ // Add to list of active xhrs callbacks
+ xhrCallbacks[ handle ] = callback;
}
- // Add to list of active xhrs callbacks
- handle = xhrId++;
- xhr.onreadystatechange = xhrCallbacks[ handle ] = callback;
+ xhr.onreadystatechange = callback;
}
},

0 comments on commit a28eadf

Please sign in to comment.