changed comments about some still needed in 2.0 oldIE workarounds; refs tickets #5866, #9217, #13553 #1195

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
5 participants
@mgol
Member

mgol commented Mar 4, 2013

No description provided.

@mgol

This comment has been minimized.

Show comment Hide comment
@mgol

This comment has been minimized.

Show comment Hide comment
@mgol

mgol Mar 4, 2013

Member

Ah, crap. I mistook IE10 for IE9; IE9 still needs the scroll hooks. :/ I'll update the pull request with a proper comment and I'll restore the code...

Member

mgol commented Mar 4, 2013

Ah, crap. I mistook IE10 for IE9; IE9 still needs the scroll hooks. :/ I'll update the pull request with a proper comment and I'll restore the code...

@mgol

This comment has been minimized.

Show comment Hide comment
@mgol

mgol Mar 4, 2013

Member

Maybe at least the #5866 backout can be kept. Bug description claims it works even on IE8. This modification saves 34 bytes in the minified code.

Member

mgol commented Mar 4, 2013

Maybe at least the #5866 backout can be kept. Bug description claims it works even on IE8. This modification saves 34 bytes in the minified code.

@jaubourg

View changes

src/ajax.js
// Handle falsy url in the settings object (#10093: consistency with old signature)
// We also use the url parameter if available
- s.url = ( ( url || s.url || ajaxLocation ) + "" ).replace( rhash, "" ).replace( rprotocol, ajaxLocParts[ 1 ] + "//" );
+ s.url = ( ( url || s.url || ajaxLocation ) + "" ).replace( rhash, "" );

This comment has been minimized.

Show comment Hide comment
@jaubourg

jaubourg Mar 4, 2013

Member

Prefilters expect a fully qualified URL in the internal options object. It's a departure in behaviour between 1.x and 2.x.

@jaubourg

jaubourg Mar 4, 2013

Member

Prefilters expect a fully qualified URL in the internal options object. It's a departure in behaviour between 1.x and 2.x.

This comment has been minimized.

Show comment Hide comment
@mgol

mgol Mar 4, 2013

Member

So I guess neither of these changes can be backed out? Any links to API description detailing this behavior change?

Anyway, the comment about IE7 should be changed to a more appropriate one in jQuery 2.0.

@mgol

mgol Mar 4, 2013

Member

So I guess neither of these changes can be backed out? Any links to API description detailing this behavior change?

Anyway, the comment about IE7 should be changed to a more appropriate one in jQuery 2.0.

This comment has been minimized.

Show comment Hide comment
@jaubourg

jaubourg Mar 5, 2013

Member

Well, it was more of a side effect of the fix but since it happened in 1.5rc1 (if memory serves), It's been there since the inception of prefilters. Also, the code controlling for cross-domain assumes the protocol is actually there.

You're right that we should make this clear in the comments.

If you wanna remove something from ajax, it's the logic pertaining to isLocal. It is completly unnecessary now that we listen to XHRs through onsuccess and onerror. I've been meaning to do so for a while but didn't find the time.

@jaubourg

jaubourg Mar 5, 2013

Member

Well, it was more of a side effect of the fix but since it happened in 1.5rc1 (if memory serves), It's been there since the inception of prefilters. Also, the code controlling for cross-domain assumes the protocol is actually there.

You're right that we should make this clear in the comments.

If you wanna remove something from ajax, it's the logic pertaining to isLocal. It is completly unnecessary now that we listen to XHRs through onsuccess and onerror. I've been meaning to do so for a while but didn't find the time.

This comment has been minimized.

Show comment Hide comment
@dmethvin

dmethvin Mar 6, 2013

Member

It seems like a bad idea to change this, since prefilters might expect it.

@dmethvin

dmethvin Mar 6, 2013

Member

It seems like a bad idea to change this, since prefilters might expect it.

@mgol

This comment has been minimized.

Show comment Hide comment
@mgol

mgol Mar 12, 2013

Member

This pull request is now completely changed - I corrected two comments, cleaned up a little code and removed isLocal. @jaubourg, you said sth about a logic around isLocal but I don't see anything actually using it at the moment; is that it?

Member

mgol commented Mar 12, 2013

This pull request is now completely changed - I corrected two comments, cleaned up a little code and removed isLocal. @jaubourg, you said sth about a logic around isLocal but I don't see anything actually using it at the moment; is that it?

@timmywil timmywil closed this in 79992d7 Mar 13, 2013

@jaubourg

This comment has been minimized.

Show comment Hide comment
@jaubourg

jaubourg Mar 13, 2013

Member

Only remaining logic was in the main ajax file, hence the need for removal, so you did it right (tm) :)

Member

jaubourg commented Mar 13, 2013

Only remaining logic was in the main ajax file, hence the need for removal, so you did it right (tm) :)

@mgol

This comment has been minimized.

Show comment Hide comment
@mgol

mgol Mar 13, 2013

Member

Thanks for adding me to AUTHORS! However, I'd ask for my mail to be changed to m.goleb@gmail.com instead of the current one (the current one is my work e-mail and I did all the work in my free time).

Member

mgol commented Mar 13, 2013

Thanks for adding me to AUTHORS! However, I'd ask for my mail to be changed to m.goleb@gmail.com instead of the current one (the current one is my work e-mail and I did all the work in my free time).

@scottgonzalez

This comment has been minimized.

Show comment Hide comment
@scottgonzalez

scottgonzalez Mar 13, 2013

Member

This is a mistake on two people's part. @mzgol You should change your git settings to use the email address that matches the one you used to sign the CLA. Let us know if you need any help with that. @timmywil You should always make sure that the name and email address match the CLA before committing. The best course of action at this point is to add an entry in the mailmap. Ping me if you need info about how to use the mailmap.

Member

scottgonzalez commented Mar 13, 2013

This is a mistake on two people's part. @mzgol You should change your git settings to use the email address that matches the one you used to sign the CLA. Let us know if you need any help with that. @timmywil You should always make sure that the name and email address match the CLA before committing. The best course of action at this point is to add an entry in the mailmap. Ping me if you need info about how to use the mailmap.

@mgol

This comment has been minimized.

Show comment Hide comment
@mgol

mgol Mar 13, 2013

Member

Ah, now I see it, sorry for the mess. I guess these commits can't now be changed retrospectively (commit ids would break etc.). I'll obviously change the e-mail data in gitconfig; is there anything else I can do to help correct this mistake?

Member

mgol commented Mar 13, 2013

Ah, now I see it, sorry for the mess. I guess these commits can't now be changed retrospectively (commit ids would break etc.). I'll obviously change the e-mail data in gitconfig; is there anything else I can do to help correct this mistake?

@scottgonzalez

This comment has been minimized.

Show comment Hide comment
@scottgonzalez

scottgonzalez Mar 13, 2013

Member

You could send a PR that updates the mailmap and AUTHORS.txt if you want, or just wait for it to get fixed.

Member

scottgonzalez commented Mar 13, 2013

You could send a PR that updates the mailmap and AUTHORS.txt if you want, or just wait for it to get fixed.

@scottgonzalez

This comment has been minimized.

Show comment Hide comment
@scottgonzalez

scottgonzalez Mar 13, 2013

Member

Hmm... @dmethvin looks like you never committed the mailmap. Do you want me to make one?

Member

scottgonzalez commented Mar 13, 2013

Hmm... @dmethvin looks like you never committed the mailmap. Do you want me to make one?

@mgol mgol deleted the mgol:oldie branch Mar 13, 2013

@mgol

This comment has been minimized.

Show comment Hide comment
@mgol

mgol Mar 13, 2013

Member

@scottgonzalez I submitted a pull request:
#1201

Member

mgol commented Mar 13, 2013

@scottgonzalez I submitted a pull request:
#1201

@mgol

This comment has been minimized.

Show comment Hide comment
@mgol

mgol Mar 13, 2013

Member

(I've also amended all my other current pull requests so that their author entries for all commits are OK. Too bad this one has been already merged.)

Member

mgol commented Mar 13, 2013

(I've also amended all my other current pull requests so that their author entries for all commits are OK. Too bad this one has been already merged.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment