> On 2012-02-13 23:12:07, Henry Saputra wrote:
> > trunk/features/src/main/javascript/features/actions/actions_container.js,
> > line 291
> > <https://reviews.apache.org/r/3873/diff/2/?file=74485#file74485line291>
> >
> > Nothing in particular, just making it easier to read for me while
> > fixing the array errors.
>
> Dan Dumont wrote:
> I ask because the code you introduced is often larger (even when closure
> complied), with the exception of perhaps the var statements.
>
> When traversing an array with an index, doing
> for(var i=0,b;b=a[i];i++){}
>
> is smaller than:
> for(var i=0;i<a.length;i++){var b=a[i];}
>
> by about 13 characters
>
> Yeah sure, just 13 characters :) but that's for each for loop. This
> is something that closure doesn't optimize for you because of the trickiness
> of the truthy assignment. You as a programmer can know if the value assigned
> might possibly be falsy, and should not use this technique in this case (so
> closure can't really know that, unless there is an annotation I'm unaware of)
> like if a value in an array might legitimately be null, 0, undefined or ''.
The main goal of this patch is to fix error/ wrong associative array accesses
that caused browsers to throw exception.
You can definitely submit patch improvement to make it smaller yo load.
Probably also add JSUnit test to make sure the intended behavior are done
through the code, sometimes short code could fall with corner cases if not
carefully coded.
To make sure the condition portion of the loop check for proper boolean value,
I think we could write it like:
for(var i=0, b; !!(b=a[i]); i++) {
....
}
- Henry
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3873/#review5062
-----------------------------------------------------------
On 2012-02-12 01:46:58, Henry Saputra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3873/
> -----------------------------------------------------------
>
> (Updated 2012-02-12 01:46:58)
>
>
> Review request for shindig.
>
>
> Summary
> -------
>
> Some of the iterations in the actions_container.js file are using "in"
> operator without calling hasOwnProperty to make sure its not going up to
> prototype chain (http://yuiblog.com/blog/2006/09/26/for-in-intrigue/). It
> causes trouble when executing in some browsers like FF5.
>
> Small cleanup for actual array to just simply used for-loop with index.
>
>
> Diffs
> -----
>
> trunk/features/src/main/javascript/features/actions/actions_container.js
> 1243090
>
> Diff: https://reviews.apache.org/r/3873/diff
>
>
> Testing
> -------
>
> Pass JS unit tests and manual test with common container and action container.
>
>
> Thanks,
>
> Henry
>
>
