Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Do not call getElements on a script tag. Avoids unnecessary execution… 
…. Fixes #10176.
  • Loading branch information
@timmywil
timmywil committed Oct 12, 2011
1 parent 92b06d3 commit 3ad0ba6
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 6 deletions.
10 changes: 6 additions & 4 deletions src/manipulation.js
View file
Expand Up @@ -530,10 +530,10 @@ jQuery.each({
});

function getAll( elem ) {
if ( "getElementsByTagName" in elem ) {
if ( typeof elem.getElementsByTagName !== "undefined" ) {
return elem.getElementsByTagName( "*" );

} else if ( "querySelectorAll" in elem ) {
} else if ( typeof elem.querySelectorAll !== "undefined" ) {
return elem.querySelectorAll( "*" );

} else {
Expand All @@ -549,9 +549,11 @@ function fixDefaultChecked( elem ) {
}
// Finds all inputs and passes them to fixDefaultChecked
function findInputs( elem ) {
if ( jQuery.nodeName( elem, "input" ) ) {
var nodeName = (elem.nodeName || "").toLowerCase();
if ( nodeName === "input" ) {
fixDefaultChecked( elem );
} else if ( "getElementsByTagName" in elem ) {
// Skip scripts, get other children
} else if ( nodeName !== "script" && typeof elem.getElementsByTagName !== "undefined" ) {
jQuery.grep( elem.getElementsByTagName("input"), fixDefaultChecked );
}
}
Expand Down
10 changes: 8 additions & 2 deletions test/unit/manipulation.js
View file
Expand Up @@ -531,7 +531,7 @@ test("append(xml)", function() {
});

test("appendTo(String|Element|Array<Element>|jQuery)", function() {
expect(16);
expect(17);

var defaultText = "Try them out:"
jQuery("<b>buga</b>").appendTo("#first");
Expand Down Expand Up @@ -579,7 +579,7 @@ test("appendTo(String|Element|Array&lt;Element&gt;|jQuery)", function() {
jQuery("#moretests div:last").click();

QUnit.reset();
var div = jQuery("<div/>").appendTo("#qunit-fixture, #moretests");
div = jQuery("<div/>").appendTo("#qunit-fixture, #moretests");

equals( div.length, 2, "appendTo returns the inserted elements" );

Expand All @@ -603,6 +603,12 @@ test("appendTo(String|Element|Array&lt;Element&gt;|jQuery)", function() {
equals( jQuery("#qunit-fixture div").length, num, "Make sure all the removed divs were inserted." );

QUnit.reset();

stop();
jQuery.getScript('data/test.js', function() {
jQuery('script[src*="data\\/test\\.js"]').remove();
start();
});
});

var testPrepend = function(val) {
Expand Down

8 comments on commit 3ad0ba6

@Krinkle
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps use !== undefined instead of typeof .. !== "undefined" ? Afaik 'undefined' can be safely used in a strict comparison within jQuery context since it's explicitly made undefined in the closure. Using a strict reference comparison gives faster performance (and it's shorter in code/bandwidth) than executing the typeof operator and doing a string comparison on the result.

@timmywil
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing as an argument to typeof avoids the arbitrary execution of the given function in IE and has the added bonus of being noticeably faster than using the in operator. Using typeof vs. solely checking for property existence does not actually differ in performance enough to be a concern, according to what we've seen. http://jsperf.com/in-vs-typeof

@Krinkle
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't suggesting to use the in operator... suggesting to use strict comparison to the undefined reference instead of the string "undefined" returned by the typeof operator. As done in other parts of jQuery core, like here

@mikesherov
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Krinkle, using strict comparison causes the function to be executed in IE... thus causing side effects. The fact that certain other parts of jQuery core may or may not conform to such standards (although the line you pointed do isn't an example of inconsistency in this case, it just looks like one) isn't really a good argument.

The three options are:

  1. use strict comparison, and get unnecessary side effects in IE.
  2. use "in", which is ridiculously slow.
  3. use typeof !== "undefined"

3 is the clear winner here.

@Krinkle
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I didn't think there'd that much of a difference in how IE treats the object member access between if ( foo.bar !== undefined ) and if ( typeof foo.bar !== 'undefined' ). Anyway, I agree option 3 makes perfect sense here. Thanks

@timmywil
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Mike. You said it better.

@anddoutoi
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikesherov this was interesting. I am looking for sources that support this (yes I have trid google it). You know for which versions of IE this is true? You got any method checking this? Is this true only for built in native methods? Cause I couldn´t reproduce it with userland code.

@timmywil
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anddoutoi: http://jsfiddle.net/timmywil/NzUKW/ A situation where the method call throws an error.

Please sign in to comment.