GNOME Bugzilla – Bug 781219
tweener: Explicitly check for undefined properties
Last modified: 2017-05-21 20:42:29 UTC
With SpiderMonkey 38 and the gjs-1.48.x, I see these in gnome-documents: JS WARNING: [resource:///org/gnome/gjs/modules/tweener/tweener.js 514]: reference to undefined property obj.delay JS WARNING: [resource:///org/gnome/gjs/modules/tweener/tweener.js 538]: reference to undefined property properties[istr].arrayIndex The first one is easily fixed; but the second is more involved - that code has a real world example of the difference between 'let' and var'. :)
Created attachment 349734 [details] [review] tweener: Explicitly check if obj.time and obj.delay are defined
Review of attachment 349734 [details] [review]: Since undefined, NaN and 0 are all falsy values, you could do "var time = obj.time || 0;" which would be more readable. On the other hand, that would also silently accept obj.time === "" where it might have thrown an exception before. Either way is +1 from me.
Thanks for the patch. Can you help me understand the problem you see with the second instance? I see "properties" is defined in an awkward place.
(In reply to Philip Chimento from comment #2) > Review of attachment 349734 [details] [review] [review]: > > Since undefined, NaN and 0 are all falsy values, you could do "var time = > obj.time || 0;" which would be more readable. Ok, I will change it to 'obj.time || 0'.
(In reply to Philip Chimento from comment #3) > Can you help me understand the problem you see with > the second instance? I see "properties" is defined in an awkward place. I should mention that I only got the second WARNING once I fixed the first. In the second instance it is complaining about properties[istr].arrayIndex. I haven't tracked it down yet, so I don't know why. Even though 'properties' is awkwardly defined, it is correctly conditioned on 'isCaller', so that's not the issue. Although it might be nicer to use 'let' and define 'properties' in the outer block. As I said, it is a bit more involved than the first WARNING. :)
Comment on attachment 349734 [details] [review] tweener: Explicitly check if obj.time and obj.delay are defined I committed the patch with the modification. Attachment 349734 [details] pushed as 3e63b34 - tweener: Explicitly check if obj.time and obj.delay are defined
Can the second warning be solved with just a well-placed properties[istr].arrayIndex || someDefaultValue?
Pushed the first patch to gnome-3-24.
I'll add a simple `|| 0` to the offending line of code. I don't think it matters, actually as far as I can tell arrayIndex is not even used by the Tweener code.