Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix removeData for case when cache is a frame's window #470

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
3 participants
@moschel
Copy link

moschel commented Aug 18, 2011

See http://bugs.jquery.com/ticket/10080. The test case in this commit currently breaks in IE8, but works in other browsers. The change in removeData fixes it by ensuring the cache is not any window object.

stop()
var frame = jQuery("#loadediframe");
jQuery(frame[0].contentWindow).bind("unload", function(){
ok(true, "called unload")

This comment has been minimized.

Copy link
@rwaldron

rwaldron Aug 18, 2011

Member

Please add a semi colon to the end of this line

@@ -2209,6 +2210,18 @@ test("custom events with colons (#3533, #8272)", function() {

});

test("unload with iframes", function(){
expect(1)

This comment has been minimized.

Copy link
@rwaldron

rwaldron Aug 18, 2011

Member

Please add a semi colon to the end of this line

@@ -2209,6 +2210,18 @@ test("custom events with colons (#3533, #8272)", function() {

});

test("unload with iframes", function(){
expect(1)
stop()

This comment has been minimized.

Copy link
@rwaldron

rwaldron Aug 18, 2011

Member

Please add a semi colon to the end of this line

@@ -392,6 +392,7 @@ test("bind(), iframes", function() {
}).click().unbind("click");
});


This comment has been minimized.

Copy link
@rwaldron

rwaldron Aug 18, 2011

Member

Please don't add unnec. empty lines

@rwaldron

This comment has been minimized.

Copy link
Member

rwaldron commented Aug 18, 2011

Since this is a change to /src/data.js, the unit test should be in /test/unit/data.js

start();
})
// change the url to trigger unload
frame.attr("src", "data/iframe.html?param=true")

This comment has been minimized.

Copy link
@rwaldron

rwaldron Aug 18, 2011

Member

Please add a semi colon to the end of this line

@@ -2209,6 +2210,18 @@ test("custom events with colons (#3533, #8272)", function() {

});

test("unload with iframes", function(){

This comment has been minimized.

Copy link
@rwaldron

rwaldron Aug 18, 2011

Member

Please add correct whitespace: function(){ => function() {

expect(1)
stop()
var frame = jQuery("#loadediframe");
jQuery(frame[0].contentWindow).bind("unload", function(){

This comment has been minimized.

Copy link
@rwaldron

rwaldron Aug 18, 2011

Member

Please add correct whitespace: function(){ => function() {

jQuery(frame[0].contentWindow).bind("unload", function(){
ok(true, "called unload")
start();
})

This comment has been minimized.

Copy link
@rwaldron

rwaldron Aug 18, 2011

Member

Please add a semi colon to the end of this line

@timmywil

This comment has been minimized.

Copy link
Member

timmywil commented Aug 18, 2011

Thanks for your work! @rwaldron is incorporating some of this into another pull

@timmywil timmywil closed this Aug 18, 2011

@moschel

This comment has been minimized.

Copy link
Author

moschel commented Aug 18, 2011

Re: Since this is a change to /src/data.js, the unit test should be in /test/unit/data.js

I could add a test to data.js but I think keeping the unload test in event.js is useful as well, as an integration test. Its rare in any application you'd see this case by using removeData directly.

Also, I'm confused. Should I resubmit the pull request with the formatting requests @rwaldron made? Or is this already being done.

@rwaldron

This comment has been minimized.

Copy link
Member

rwaldron commented Aug 18, 2011

I've already combined your work with mine (including credit to you) in another branch. The patch reviews were made prior to the pull request being closed.

@moschel

This comment has been minimized.

Copy link
Author

moschel commented Aug 18, 2011

Oh, ok sweet. Thanks man. Thanks for being so quick with your replies, that's rare and awesome.

@rwaldron

This comment has been minimized.

Copy link
Member

rwaldron commented Aug 18, 2011

Thanks for the contributions!

@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.