Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Fix #12370. Explicitly coerce bool/undef to integer.

  • Loading branch information...
commit 0ca936d2c911fe037dda3cb9d6e21f6fe1735a2c 1 parent 3a633b0
Dave Methvin dmethvin authored
Showing with 1 addition and 0 deletions.
  1. +1 −0  src/effects.js
1  src/effects.js
View
@@ -540,6 +540,7 @@ function genFx( type, includeWidth ) {
// if we include width, step value is 1 to do all cssExpand values,
// if we don't include width, step value is 2 to skip over Left and Right
+ includeWidth = includeWidth? 1 : 0;

coughwhitespacecough ;-)

Dave Methvin Owner

My only defense is that I got distracted by the thought of using ~~includeWidth in the for loop. It saves a byte but I was concerned that if Android 2.1 is dumb it might also be dumber. I think we're at 1.8.1 now

Why not just update all calls to genFx to include the includeWith parameter as a Number?

In fact, you could just change includeWidth to stepValue and update all calls to genFx to pass a stepValue (1 or 2). I image there may be a couple of gzipped bytes to be saved here.

Dave Methvin Owner

Could be, but I was primarily looking for a simple and safe fix for 1.8.1. Logically, includeWidth should be boolean; passing in a number doesn't make its purpose as clear.

That's why I suggested changing the parameter name to stepValue. :-)

Dave Methvin Owner

That's exposing too much of the internal workings imo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
for( ; i < 4 ; i += 2 - includeWidth ) {
Richard Gibson Collaborator

If a browser really is failing on ToNumber(boolean) coercion in this expression, we'll have to clean up the other places in which we rely on that behavior. Is there any way for us to get strong confirmation?

Dave Methvin Owner

It may have failed on converting undefined instead, I just saw the boolean call first. There are several genFx() calls with just one arg. Kinda makes me wonder how it ever worked... is this covered by a unit test? /cc @gnarf37

Richard Gibson Collaborator

Yep, that would do it. And undefined casts to NaN, so there's your problem right there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
which = cssExpand[ i ];
attrs[ "margin" + which ] = attrs[ "padding" + which ] = type;
David Murdoch

coughwhitespacecough ;-)

Richard Gibson
Collaborator

If a browser really is failing on ToNumber(boolean) coercion in this expression, we'll have to clean up the other places in which we rely on that behavior. Is there any way for us to get strong confirmation?

Dave Methvin

My only defense is that I got distracted by the thought of using ~~includeWidth in the for loop. It saves a byte but I was concerned that if Android 2.1 is dumb it might also be dumber. I think we're at 1.8.1 now

Dave Methvin

It may have failed on converting undefined instead, I just saw the boolean call first. There are several genFx() calls with just one arg. Kinda makes me wonder how it ever worked... is this covered by a unit test? /cc @gnarf37

Richard Gibson
Collaborator

Yep, that would do it. And undefined casts to NaN, so there's your problem right there.

David Murdoch

Why not just update all calls to genFx to include the includeWith parameter as a Number?

In fact, you could just change includeWidth to stepValue and update all calls to genFx to pass a stepValue (1 or 2). I image there may be a couple of gzipped bytes to be saved here.

Dave Methvin

Could be, but I was primarily looking for a simple and safe fix for 1.8.1. Logically, includeWidth should be boolean; passing in a number doesn't make its purpose as clear.

David Murdoch

That's why I suggested changing the parameter name to stepValue. :-)

Dave Methvin

That's exposing too much of the internal workings imo.

Please sign in to comment.
Something went wrong with that request. Please try again.