Permalink
13 comments
on commit
sign in to comment.
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Showing
1 changed file
with
19 additions
and
3 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eda283d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should add that amount of code without having discussion about it first, until then, i think we should revert this
eda283d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I discussed this with @dmethvin & @gibson042.
EDIT: Anyway, this commit is already in, no point in reverting it back in forth, we can discuss it here if you have doubts and optionally revert later.
One way to reduce the code size here would be to remove the check completely and just always use the second logic. I'm not sure if that would introduce any performance penalty.
BTW, this is the only issue that was making Android 4.0 consistently fail.
eda283d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But not with the rest of the team, that's why we use PRs, to have full discussions and commit only if we all on the same page.
That's not an argument, for example – if i would revert it and then said exactly the same thing, would that really matter for this issue?
My arguments are the same as in #1514
eda283d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another example, if it would start with PR, i, or someone else could point that this is not our style guide, now in order to fix it, you should do another commit.
eda283d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true, I just didn't think this change will be controversial as this is the only issue making Android 4.0 fail. There are a lot of things commited without PRs by most of team members.
That is an argument (you just disagree with it), please try to be considerate. Anyway, yes, this would apply in the same way if you did the same, it's just now we know we disagree on the issue and if we revert it now it'll generate 1-2 additional commits depending on the team decision whereas if we wait for the next meeting and discuss it there, we'll have 0-1 additional commits depending on the resolution.
eda283d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These kinds of things should be handled by our linters, otherwise mistakes will always keep happening.
eda283d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry if i wasn't
i'm not saying we should do the revert now, i was just expression my position on this.
I probably went a little over the board, but still, review for this amount of code should have happened, if we would decided to land it, we could have discuss the code, maybe it's presented with a new bug or it could be simplified, now we can do only damage control.
From the old days, reviews was a solution for it, i don't see why it couldn't be now
eda283d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was premature on my part, I agree for +59 bytes I should have created a PR and waited for feedback a little longer.
eda283d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussing it last night with @mzgol I wondered whether we should just go back to our own string replace for all browsers. It seems like the only place where we might process a big string would be globalEval.
eda283d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And even there, explicit regex anchoring means we'll stay pretty darn efficient. I'm sold on abandoning
String.trim
for Android<4.1, but doing it ourselves will still be costly even without the test.eda283d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why we do this? See https://github.com/jquery/jquery/pull/1449/files
eda283d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wouldn't seem that trimming is even needed there, I agree.
eda283d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#1449 aside, the performance hit of non-native
jQuery.trim
should be negligible in both places where we might process big strings:jQuery.globalEval
(where it's followed by interpretation and execution of the string)jQuery.parseJSON
(where it's followed by.replace( <global JSON token regex>, function )
:jquery/src/ajax/parseJSON.js
Line 17 in a96ff1c
If there is a performance penalty, we're actually more likely to see it when processing shorter strings in a loop:
jquery/src/attributes/classes.js
Line 43 in a96ff1c
jquery/src/attributes/classes.js
Line 86 in a96ff1c
jquery/src/css.js
Line 402 in a96ff1c