912469 - Always check hasOwnProperty when iterating over an object's key

Status: RESOLVED FIXED

(L20n :: JS Library, defect)

Description

[Staś Małolepszy :stas]

Third-party code might have extended Object's prototype.

Comment 1

[Staś Małolepszy :stas]

Attachment: Patch

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 805021 [details] [diff] [review]
Patch

Review of attachment 805021 [details] [diff] [review]:
-----------------------------------------------------------------

::: bindings/l20n/html.js
@@ +206,3 @@
>        return;
>      }
>      for (var key in entity.attributes) {

maybe just getOwnPropertyNames?

::: lib/l20n/context.js
@@ +254,4 @@
>  
>      function extend(dst, src) {
>        for (var i in src) {
> +        if (!src.hasOwnProperty(i)) {

maybe just use getOwnPropertyNames ?

Comment 3

[Masatoshi Kimura [:emk]]

(In reply to Zbigniew Braniecki [:gandalf] from comment #2)
> ::: bindings/l20n/html.js
> @@ +206,3 @@
> >        return;
> >      }
> >      for (var key in entity.attributes) {
> 
> maybe just getOwnPropertyNames?

getOwnPropertyNames will enumerate non-enumerable properties. Object.keys() would be appropriate here.

Comment 4

[Staś Małolepszy :stas]

Attachment: Use Object.keys

I followed Masatoshi's advice and used Object.keys were possible.

I also moved the test into context/ctxdata.js because it wasn't really relevant 
in retranslation.js.  I still kept the relevant tests that I wrote in 
retranslation.js

Comment 5

[Staś Małolepszy :stas]

Comment on attachment 806651 [details] [diff] [review]
Use Object.keys


>+  it('when absent, all testing entities don\'t work', function() {
>+  it('when absent, all testing entities don\'t work', function() {
>+  it('when absent, all testing entities don\'t work', function() {

Oops, too much copy&paste :)  I'll fix this when I land.

Comment 6

[Staś Małolepszy :stas]

https://github.com/l20n/l20n.js/commit/e88b21aa1bf5ae196f406beca6416f08616bc8a0

In one place (Context::getMany), I used hasOwnProperty instead of Object.keys to avoid creating functions inside of a loop.