ronantakizawa/codereview-bench huggingface dataset:
Repository: jquery/jquery
File: src/core/isAttached.js (JavaScript)

Code before:
define( [
	"../core",
	"../var/getShadowRoot",
	"../var/document",
	"../selector" // jQuery.contains
], function( jQuery, getShadowRoot, document ) {
	"use strict";

	// Replace the use of contains function to check attachment (gh-3504)

	var isAttached;

	// Check attachment across shadow DOM boundaries when possible (gh-3504)
	if ( document.head.createShadowRoot ) {
		isAttached = function( elem ) {
			return jQuery.contains( elem.ownerDocument, elem ) ||
				getShadowRoot( elem ) === elem.ownerDocument;
		};
	} else {
		isAttached = function( elem ) {
			return jQuery.contains( elem.ownerDocument, elem );
		};
	}
	return isAttached;
} );


Review comment (suggestion):
I've just noticed this method belongs to Shadow DOM v0, not v1 and we want to support the v1 version. This should be changed to `documentElement.attachShadow` where `documentElement` is defined in `src/var/documentElement.js`.

Code after:
define( [
	"../core",
	"../var/documentElement",
	"../selector" // jQuery.contains
], function( jQuery, documentElement ) {
	"use strict";

	var isAttached = function( elem ) {
			return jQuery.contains( elem.ownerDocument, elem );
		},
		composed = { composed: true };

	// Check attachment across shadow DOM boundaries when possible (gh-3504)
	if ( documentElement.attachShadow ) {
		isAttached = function( elem ) {
			return jQuery.contains( elem.ownerDocument, elem ) ||
				elem.getRootNode( composed ) === elem.ownerDocument;
		};
	}

	return isAttached;
} );
Repository: jquery/jquery
File: src/core.js (JavaScript)

Code before:
	if ( i === length ) {
		target = this;
		i--;
	}

	for ( ; i < length; i++ ) {

		// Only deal with non-null/undefined values
		if ( ( options = arguments[ i ] ) != null ) {

			// Extend the base object
			for ( name in options ) {
				copy = options[ name ];

				// Prevent never-ending loop
				if ( target === copy ) {
					continue;
				}

				// Recurse if we're merging plain objects or arrays
				if ( deep && copy && ( jQuery.isPlainObject( copy ) ||
					( copyIsArray = Array.isArray( copy ) ) ) ) {
					src = target[ name ];

					if ( copyIsArray ) {
						copyIsArray = false;
						clone = src && Array.isArray( src ) ? src : [];

					} else {
						clone = src && jQuery.isPlainObject( src ) ? src : {};
					}

					// Never move original objects, clone them
					target[ name ] = jQuery.extend( deep, clone, copy );

				// Don't bring in undefined values
				} else if ( copy !== undefined ) {
					target[ name ] = copy;
				}
			}
		}
	}

	// Return the modified object
	return target;
};

jQuery.extend( {

	// Unique for each copy of jQuery on the page
	expando: "jQuery" + ( version + Math.random() ).replace( /\D/g, "" ),

Review comment (suggestion):
While we're rearranging this code without changing functionality, we can make a small size/clarity optimization as well:
```js
// Ensure proper type for the source value
if ( copyIsArray && !Array.isArray( src ) ) {
	clone = [];
} else if ( !copyIsArray && !jQuery.isPlainObject( src ) ) {
	clone = {};
} else {
	clone = src;
}
copyIsArray = false;
```

Code after:
	if ( i === length ) {
		target = this;
		i--;
	}

	for ( ; i < length; i++ ) {

		// Only deal with non-null/undefined values
		if ( ( options = arguments[ i ] ) != null ) {

			// Extend the base object
			for ( name in options ) {
				copy = options[ name ];

				// Prevent never-ending loop
				if ( target === copy ) {
					continue;
				}

				// Recurse if we're merging plain objects or arrays
				if ( deep && copy && ( jQuery.isPlainObject( copy ) ||
					( copyIsArray = Array.isArray( copy ) ) ) ) {
					src = target[ name ];

					// Ensure proper type for the source value
					if ( copyIsArray && !Array.isArray( src ) ) {
						clone = [];
					} else if ( !copyIsArray && !jQuery.isPlainObject( src ) ) {
						clone = {};
					} else {
						clone = src;
					}
					copyIsArray = false;

					// Never move original objects, clone them
					target[ name ] = jQuery.extend( deep, clone, copy );

				// Don't bring in undefined values
				} else if ( copy !== undefined ) {
					target[ name ] = copy;
				}
			}
		}
	}

	// Return the modified object
	return target;
};

jQuery.extend( {
Repository: jquery/jquery
File: src/core/DOMEval.js (JavaScript)

Code before:
define( [
	"../var/document"
], function( document ) {
	"use strict";

	var preservedScriptAttributes = {
		type: true,
		src: true,
		noModule: true
	};

	function DOMEval( code, doc, node ) {
		doc = doc || document;

		var i,
			script = doc.createElement( "script" );

		script.text = code;
		if ( node ) {
			for ( i in preservedScriptAttributes ) {
				if ( node[ i ] ) {
					script[ i ] = node[ i ];
				}
			}
		}
		doc.head.appendChild( script ).parentNode.removeChild( script );
	}

	return DOMEval;
} );


Review comment (suggestion):
This will only work if the property & an attribute is named with identical casing but from our fixed list only `nonce` may get to this line so we should be safe for now.

Code after:
		type: true,
		src: true,
		nonce: true,
		noModule: true
	};

	function DOMEval( code, doc, node ) {
		doc = doc || document;

		var i,
			script = doc.createElement( "script" );

		script.text = code;
		if ( node ) {
			for ( i in preservedScriptAttributes ) {
				if ( node[ i ] ) {
					script[ i ] = node[ i ];
				} else if ( node.getAttribute( i ) ) {

					// Support: Firefox 64+, Edge 18+
					// Some browsers don't support the "nonce" property on scripts.
					// On the other hand, just using `setAttribute` & `getAttribute`
					// is not enough as `nonce` is no longer exposed as an attribute
					// in the latest standard.
					// See https://github.com/whatwg/html/issues/2369
					script.setAttribute( i, node.getAttribute( i ) );
				}
			}
		}
		doc.head.appendChild( script ).parentNode.removeChild( script );
	}

	return DOMEval;
} );
Repository: jquery/jquery
File: src/event.js (JavaScript)

Code before:
], function( jQuery, document, documentElement, isFunction, rnothtmlwhite,
	rcheckableType, slice, dataPriv, nodeName ) {

"use strict";

var
	rkeyEvent = /^key/,
	rmouseEvent = /^(?:mouse|pointer|contextmenu|drag|drop)|click/,
	rtypenamespace = /^([^.]*)(?:\.(.+)|)/;

function returnTrue() {
	return true;
}

function returnFalse() {
	return false;
}

// Support: IE <=9 only
// https://bugs.jquery.com/ticket/13393
function expectSync( elem, type ) {

	// Expect focus to be synchronous when the element is already active,
	// and blur to be synchronous when the element is not already active
	// (even in IE)
	return ( elem === safeActiveElement() ) === ( type === "focus" );
}
function safeActiveElement() {
	try {
		return document.activeElement;
	} catch ( err ) { }
}

function on( elem, types, selector, data, fn, one ) {
	var origFn, type;

	// Types can be a map of types/handlers
	if ( typeof types === "object" ) {

		// ( types-Object, selector, data )
		if ( typeof selector !== "string" ) {

			// ( types-Object, data )
			data = data || selector;
			selector = undefined;
		}
		for ( type in types ) {
			on( elem, type, selector, data, types[ type ], one );
		}
		return elem;
	}

Review comment (refactor):
You could perhaps add a support comment for IE <=11 here as other browsers fire those events synchronously so this helper wouldn't be needed?

It seems to me the `Support: IE <=9 only` part should be kept directly above `safeActiveElement` and `expectSync` might get a `Support: IE <=11 only` comment (or `Support: IE <=11+`, whatever we decide the convention to be).

What do you think?

Code after:
], function( jQuery, document, documentElement, isFunction, rnothtmlwhite,
	rcheckableType, slice, dataPriv, nodeName ) {

"use strict";

var
	rkeyEvent = /^key/,
	rmouseEvent = /^(?:mouse|pointer|contextmenu|drag|drop)|click/,
	rtypenamespace = /^([^.]*)(?:\.(.+)|)/;

function returnTrue() {
	return true;
}

function returnFalse() {
	return false;
}

// Support: IE <=9 - 11+
// focus() and blur() are asynchronous, except when they are no-op.
// So expect focus to be synchronous when the element is already active,
// and blur to be synchronous when the element is not already active.
// (focus and blur are always synchronous in other supported browsers,
// this just defines when we can count on it).
function expectSync( elem, type ) {
	return ( elem === safeActiveElement() ) === ( type === "focus" );
}

// Support: IE <=9 only
// Accessing document.activeElement can throw unexpectedly
// https://bugs.jquery.com/ticket/13393
function safeActiveElement() {
	try {
		return document.activeElement;
	} catch ( err ) { }
}

function on( elem, types, selector, data, fn, one ) {
	var origFn, type;

	// Types can be a map of types/handlers
	if ( typeof types === "object" ) {

		// ( types-Object, selector, data )
		if ( typeof selector !== "string" ) {

			// ( types-Object, data )
			data = data || selector;
			selector = undefined;
		}
		for ( type in types ) {
Repository: jquery/jquery
File: src/selector.js (JavaScript)

Code before:
				( excess = tokenize( unquoted, true ) ) &&

				// advance to the next closing parenthesis
				( excess = unquoted.indexOf( ")", unquoted.length - excess ) - unquoted.length ) ) {

				// excess is a negative index
				match[ 0 ] = match[ 0 ].slice( 0, excess );
				match[ 2 ] = unquoted.slice( 0, excess );
			}

			// Return only captures needed by the pseudo filter method (type and argument)
			return match.slice( 0, 3 );
		}
	},

	filter: {
		ID: function( id ) {
			var attrId = id.replace( runescape, funescape );
			return function( elem ) {
				return elem.getAttribute( "id" ) === attrId;
			};
		},

		TAG: function( nodeNameSelector ) {
			var nodeName = nodeNameSelector.replace( runescape, funescape ).toLowerCase();
			return nodeNameSelector === "*" ?
				function() {
 return true;
} :
				function( elem ) {
					return elem.nodeName && elem.nodeName.toLowerCase() === nodeName;
				};
		},

		CLASS: function( className ) {
			var pattern = classCache[ className + " " ];

			return pattern ||
				( pattern = new RegExp( "(^|" + whitespace + ")" + className +
					"(" + whitespace + "|$)" ) ) &&
				classCache( className, function( elem ) {
					return pattern.test(
						typeof elem.className === "string" && elem.className ||
							typeof elem.getAttribute !== "undefined" &&
								elem.getAttribute( "class" ) ||
							""
					);
				} );
		},

		ATTR: function( name, operator, check ) {

Review comment (suggestion):
I thought it might save a bit more to name this `nodeNameExpected` but it was the same. One day I will understand gzip.

Code after:
				// Get excess from tokenize (recursively)
				( excess = tokenize( unquoted, true ) ) &&

				// advance to the next closing parenthesis
				( excess = unquoted.indexOf( ")", unquoted.length - excess ) - unquoted.length ) ) {

				// excess is a negative index
				match[ 0 ] = match[ 0 ].slice( 0, excess );
				match[ 2 ] = unquoted.slice( 0, excess );
			}

			// Return only captures needed by the pseudo filter method (type and argument)
			return match.slice( 0, 3 );
		}
	},

	filter: {
		ID: function( id ) {
			var attrId = id.replace( runescape, funescape );
			return function( elem ) {
				return elem.getAttribute( "id" ) === attrId;
			};
		},

		TAG: function( nodeNameSelector ) {
			var expectedNodeName = nodeNameSelector.replace( runescape, funescape ).toLowerCase();
			return nodeNameSelector === "*" ?
				function() {
 return true;
} :
				function( elem ) {
					return nodeName( elem, expectedNodeName );
				};
		},

		CLASS: function( className ) {
			var pattern = classCache[ className + " " ];

			return pattern ||
				( pattern = new RegExp( "(^|" + whitespace + ")" + className +
					"(" + whitespace + "|$)" ) ) &&
				classCache( className, function( elem ) {
					return pattern.test(
						typeof elem.className === "string" && elem.className ||
							typeof elem.getAttribute !== "undefined" &&
								elem.getAttribute( "class" ) ||
							""
					);
				} );
		},
Repository: jquery/jquery
File: src/selector/rbuggyQSA.js (JavaScript)

Code before:
"use strict";

var rbuggyQSA = [],
	testEl = document.createElement( "div" ),
	input = document.createElement( "input" );

testEl.innerHTML = "<a href=''></a>";

// Support: Chrome 38 - 77 only
// Chrome considers anchor elements with href to match ":enabled"
// See https://bugs.chromium.org/p/chromium/issues/detail?id=993387
if ( testEl.querySelectorAll( ":enabled" ).length ) {
	rbuggyQSA.push( ":enabled" );
}

// Support: IE 9 - 11+
// IE's :disabled selector does not pick up the children of disabled fieldsets
if ( isIE ) {
	rbuggyQSA.push( ":enabled", ":disabled" );
}

// Support: IE 11+, Edge 15 - 18+
// IE 11/Edge don't find elements on a `[name='']` query in some cases.
// Adding a temporary attribute to the document before the selection works
// around the issue.
// Interestingly, IE 10 & older don't seem to have the issue.
input.setAttribute( "name", "" );
testEl.appendChild( input );
if ( !testEl.querySelectorAll( "[name='']" ).length ) {
	rbuggyQSA.push( "\\[" + whitespace + "*name" + whitespace + "*=" +
		whitespace + "*(?:''|\"\")" );
}

rbuggyQSA = rbuggyQSA.length && new RegExp( rbuggyQSA.join( "|" ) );

return rbuggyQSA;

} );


Review comment (suggestion):
Does the `=` here also need to cover other operators (e.g., `[*^$]?=`)? Cf. checks for [empty strings after ^= or $= or *=](https://github.com/jquery/sizzle/blob/a528ae520245b13a6a879041e4a2b135c7d6e522/src/sizzle.js#L816) and [case-sensitivity of name attribute](https://github.com/jquery/sizzle/blob/a528ae520245b13a6a879041e4a2b135c7d6e522/src/sizzle.js#L858).

Code after:
	input = document.createElement( "input" );

testEl.innerHTML = "<a href=''></a>";

// Support: Chrome 38 - 77 only
// Chrome considers anchor elements with href to match ":enabled"
// See https://bugs.chromium.org/p/chromium/issues/detail?id=993387
if ( testEl.querySelectorAll( ":enabled" ).length ) {
	rbuggyQSA.push( ":enabled" );
}

// Support: IE 9 - 11+
// IE's :disabled selector does not pick up the children of disabled fieldsets
if ( isIE ) {
	rbuggyQSA.push( ":enabled", ":disabled" );
}

// Support: IE 11+, Edge 15 - 18+
// IE 11/Edge don't find elements on a `[name='']` query in some cases.
// Adding a temporary attribute to the document before the selection works
// around the issue.
// Interestingly, IE 10 & older don't seem to have the issue.
input.setAttribute( "name", "" );
testEl.appendChild( input );
if ( !testEl.querySelectorAll( "[name='']" ).length ) {
	rbuggyQSA.push( "\\[" + whitespace + "*name" + whitespace + "*=" +
		whitespace + "*(?:''|\"\")" );
}

rbuggyQSA = rbuggyQSA.length && new RegExp( rbuggyQSA.join( "|" ) );

export default rbuggyQSA;
Repository: jquery/jquery
File: src/ajax/script.js (JavaScript)

Code before:
jQuery.ajaxTransport( "script", function( s ) {

	// This transport only deals with cross domain or forced-by-attrs requests
	if ( s.crossDomain || s.scriptAttrs ) {
		var script, callback;
		return {
			send: function( _, complete ) {
				script = jQuery( "<script>" )
					.attr( "crossOrigin", s.crossOrigin || null )
					.attr( s.scriptAttrs || {} )
					.prop( { charset: s.scriptCharset, src: s.url } )
					.on( "load error", callback = function( evt ) {
						script.remove();
						callback = null;
						if ( evt ) {
							complete( evt.type === "error" ? 404 : 200, evt.type );
						}
					} );

				// Use native DOM manipulation to avoid our domManip AJAX trickery
				document.head.appendChild( script[ 0 ] );
			},
			abort: function() {
				if ( callback ) {
					callback();
				}
			}
		};
	}
} );


Review comment (suggestion):
That's what the `scriptAttrs` option if for. You can add any attribute in there with no need to introduce a new option. It was designed specifically to avoid option-creep.

The correct route would be to add `crossOrigin` into the `scriptAttrs` object.

Code after:
			return text;
		}
	}
} );

// Handle cache's special case and crossDomain
jQuery.ajaxPrefilter( "script", function( s ) {
	if ( s.cache === undefined ) {
		s.cache = false;
	}

	// These types of requests are handled via a script tag
	// so force their methods to GET.
	if ( canUseScriptTag( s ) ) {
		s.type = "GET";
	}
} );

// Bind script tag hack transport
jQuery.ajaxTransport( "script", function( s ) {
	if ( canUseScriptTag( s ) ) {
		var script, callback;
		return {
			send: function( _, complete ) {
				script = jQuery( "<script>" )
					.attr( s.scriptAttrs || {} )
					.prop( { charset: s.scriptCharset, src: s.url } )
					.on( "load error", callback = function( evt ) {
						script.remove();
						callback = null;
						if ( evt ) {
							complete( evt.type === "error" ? 404 : 200, evt.type );
						}
					} );

				// Use native DOM manipulation to avoid our domManip AJAX trickery
				document.head.appendChild( script[ 0 ] );
			},
			abort: function() {
				if ( callback ) {
					callback();
				}
			}
		};
	}
} );
Repository: jquery/jquery
File: test/unit/ajax.js (JavaScript)

Code before:
							assert.strictEqual( jQuery( "result", this ).text(), "3", "Check for XML" );
						} );
					}
				} );
			} )
		).always( function() {
			done();
		} );
	} );

	QUnit.test( "jQuery.append with crossOrigin attribute", function( assert ) {
		var done = assert.async();
		assert.expect( 1 );

		var timeout = null;

		Globals.register( "corsCallback" );
		window.corsCallback = function( response ) {
			assert.ok( response === true, "Cors" );
			window.clearTimeout( timeout );
			done();
		};

		var src = baseURL + "mock.php?action=cors";

		// To simulate a cross-origin request
		src = src.replace( "localhost", "127.0.0.1" );
		var html = "<script type=\"text/javascript\" src=\"" + src + "\" crossorigin=\"anonymous\"><\/script>";

		jQuery( document.body ).append( html );
		timeout = window.setTimeout( function() {
			assert.ok( false, "Cors" );
			done();
		}, 2000 );
	} );

//----------- jQuery.active

	QUnit.test( "jQuery.active", function( assert ) {
		assert.expect( 1 );
		assert.ok( jQuery.active === 0, "ajax active counter should be zero: " + jQuery.active );
	} );

} )();


Review comment (suggestion):
```suggestion
		// Simulate a cross-origin request
```

Code after:
			done();
		} );
	} );

	QUnit.test( "jQuery.fn.load( String, Function ) - dataFilter in ajaxSettings", function( assert ) {
		assert.expect( 2 );
		var done = assert.async();
		jQuery.ajaxSetup( {
			dataFilter: function() {
				return "Hello World";
			}
		} );
		jQuery( "<div></div>" ).load( url( "name.html" ), function( responseText ) {
			assert.strictEqual( jQuery( this ).html(), "Hello World", "Test div was filled with filtered data" );
			assert.strictEqual( responseText, "Hello World", "Test callback receives filtered data" );
			done();
		} );
	} );

	QUnit.test( "jQuery.fn.load( String, Object, Function )", function( assert ) {
		assert.expect( 2 );
		var done = assert.async();
		jQuery( "<div></div>" ).load( url( "mock.php?action=echoHtml" ), {
			"bar": "ok"
		}, function() {
			var $node = jQuery( this );
			assert.strictEqual( $node.find( "#method" ).text(), "POST", "Check method" );
			assert.strictEqual( $node.find( "#data" ).text(), "bar=ok", "Check if data is passed correctly" );
			done();
		} );
	} );

	QUnit.test( "jQuery.fn.load( String, String, Function )", function( assert ) {
		assert.expect( 2 );
		var done = assert.async();

		jQuery( "<div></div>" ).load( url( "mock.php?action=echoHtml" ), "foo=3&bar=ok", function() {
			var $node = jQuery( this );
			assert.strictEqual( $node.find( "#method" ).text(), "GET", "Check method" );
			assert.ok( $node.find( "#query" ).text().match( /foo=3&bar=ok/ ), "Check if a string of data is passed correctly" );
			done();
		} );
	} );

	QUnit.test( "jQuery.fn.load() - callbacks get the correct parameters", function( assert ) {
		assert.expect( 8 );
		var completeArgs = {},
			done = assert.async();

		jQuery.ajaxSetup( {
			success: function( _, status, jqXHR ) {
Repository: jquery/jquery
File: test/unit/ajax.js (JavaScript)

Code before:
			done();
		} );
	} );

	QUnit.test( "jQuery.append with crossOrigin attribute", function( assert ) {
		var done = assert.async();
		assert.expect( 1 );

		var timeout = null;

		Globals.register( "corsCallback" );
		window.corsCallback = function( response ) {
			assert.ok( response === true, "Cors" );
			window.clearTimeout( timeout );
			done();
		};

		var src = baseURL + "mock.php?action=cors";

		// To simulate a cross-origin request
		src = src.replace( "localhost", "127.0.0.1" );
		var html = "<script type=\"text/javascript\" src=\"" + src + "\" crossorigin=\"anonymous\"><\/script>";

		jQuery( document.body ).append( html );
		timeout = window.setTimeout( function() {
			assert.ok( false, "Cors" );
			done();
		}, 2000 );
	} );

//----------- jQuery.active

	QUnit.test( "jQuery.active", function( assert ) {
		assert.expect( 1 );
		assert.ok( jQuery.active === 0, "ajax active counter should be zero: " + jQuery.active );
	} );

} )();


Review comment (suggestion):
A bit more descriptive:
```suggestion
			assert.ok( false, "Callback should have fired" );
```

Code after:
		var done = assert.async();
		jQuery.ajaxSetup( {
			dataFilter: function() {
				return "Hello World";
			}
		} );
		jQuery( "<div></div>" ).load( url( "name.html" ), function( responseText ) {
			assert.strictEqual( jQuery( this ).html(), "Hello World", "Test div was filled with filtered data" );
			assert.strictEqual( responseText, "Hello World", "Test callback receives filtered data" );
			done();
		} );
	} );

	QUnit.test( "jQuery.fn.load( String, Object, Function )", function( assert ) {
		assert.expect( 2 );
		var done = assert.async();
		jQuery( "<div></div>" ).load( url( "mock.php?action=echoHtml" ), {
			"bar": "ok"
		}, function() {
			var $node = jQuery( this );
			assert.strictEqual( $node.find( "#method" ).text(), "POST", "Check method" );
			assert.strictEqual( $node.find( "#data" ).text(), "bar=ok", "Check if data is passed correctly" );
			done();
		} );
	} );

	QUnit.test( "jQuery.fn.load( String, String, Function )", function( assert ) {
		assert.expect( 2 );
		var done = assert.async();

		jQuery( "<div></div>" ).load( url( "mock.php?action=echoHtml" ), "foo=3&bar=ok", function() {
			var $node = jQuery( this );
			assert.strictEqual( $node.find( "#method" ).text(), "GET", "Check method" );
			assert.ok( $node.find( "#query" ).text().match( /foo=3&bar=ok/ ), "Check if a string of data is passed correctly" );
			done();
		} );
	} );

	QUnit.test( "jQuery.fn.load() - callbacks get the correct parameters", function( assert ) {
		assert.expect( 8 );
		var completeArgs = {},
			done = assert.async();

		jQuery.ajaxSetup( {
			success: function( _, status, jqXHR ) {
				completeArgs[ this.url ] = [ jqXHR.responseText, status, jqXHR ];
			},
			error: function( jqXHR, status ) {
				completeArgs[ this.url ] = [ jqXHR.responseText, status, jqXHR ];
			}
		} );