Replace for..in loops with index-based for loops #210
Conversation
|
I plan to run the regression tests and get back to you. |
|
Perfect, thanks |
|
@xoundboy It looks like the changes might have gone a little too far. For example, |
src/main/js/doc.js
Outdated
| @@ -626,7 +626,7 @@ | |||
|
|
|||
| /* resolve desired timing for regions */ | |||
|
|
|||
| for (var region_i in doc.head.layout.regions) { | |||
| for (var region_i = 0; region_i < doc.head.layout.regions.length; region_i++) { | |||
There was a problem hiding this comment.
tt.head.layout.regions is a dictionary, so this should in fact use for..in
There was a problem hiding this comment.
hi @palemieux sorry for the delay. Thanks for the review. I'll address your comments very soon.
src/main/js/doc.js
Outdated
| @@ -575,7 +574,8 @@ | |||
|
|
|||
| var attrs = []; | |||
|
|
|||
| for (var a in node.attributes) { | |||
| for (var a = 0; a < node.attributes.length; a++) { | |||
There was a problem hiding this comment.
node.attributes is a dictionary, so for..in is appropriate
src/main/js/doc.js
Outdated
| @@ -751,7 +751,7 @@ | |||
|
|
|||
| } else { | |||
|
|
|||
| for (var content_i in element.contents) { | |||
| for (var content_i = 0; content_i < element.contents.length; content_i++) { | |||
There was a problem hiding this comment.
We probably need to check if contents exists.
|
@palemieux I reverted my previous commit and started again. I just fixed those problems that were actually preventing subtitles from being parsed on Tizen TVs and dropped all of the other overzealous changes. I also resorted to the hasOwnProperty checks for safety. Thanks |
|
@xoundboy Quick ping. |
|
Replaced by #211 |
I had auto whitespace stripping enabled in Webstorm so there's a lot of whitespace removal too.. I hope this isn't going to be a problem. Thanks.