885688 - B2G RIL: Fix javascript warning in ril_worker.js

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Szu-Yu Chen [:aknow] (inactive)]

Fix following bugs together:
Bug 883753
Bug 883757
Bug 883758

Comment 4

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Fix javascript warning in ril_worker.js

Comment 5

[Szu-Yu Chen [:aknow] (inactive)]

Hi Yoshi,

I got two ways to handle "reference to undefined property foo.bar"

1. if (!('bar' in foo) || foo.bar !== something)
2. initialize foo.bar to undefined

Just choose method 2 in this patch because it may looks more elegant.

Note:
1. If we haven't set a property. It will be undefined and get 'false' when test by 'property' in object.
2. However, if we set a property to 'undefined', we will get 'true' when test by 'property' in object.

> var foo = {};
undefined
> foo.ss
undefined
> 'ss' in foo
false
> foo.ss = undefined
undefined
> 'ss' in foo
true
> foo.ss === foo.tt
true
> 'tt' in foo
false

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 765874 [details] [diff] [review]
Fix javascript warning in ril_worker.js

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

Initialize a property to undefined doesn't make any sense to me.

Common pattern is to use || for a default value, as you did for mcc below.
Or using 
if (obj.prop === undefined)

In those duplicated bugs, I think what we really care is the value of the property, not the property exists or not.

Comment 7

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #6)
> Comment on attachment 765874 [details] [diff] [review]
> Fix javascript warning in ril_worker.js
> 
> Review of attachment 765874 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Initialize a property to undefined doesn't make any sense to me.
> 
> Common pattern is to use || for a default value, as you did for mcc below.
> Or using 
> if (obj.prop === undefined)
> 
> In those duplicated bugs, I think what we really care is the value of the
> property, not the property exists or not.

In our code, we have:

    if (curState.regState != regState) {

The property `regState` is not exists and raise a javascript warning

Bug 883757 - [JavaScript Warning: "reference to undefined property curState.regState" {file: "resource://gre/modules/ril_worker.js" line: 3116}

"||" for default value is not useful here.

In order to resolve this warning, we have to chage the line to

   if (!('regState' in curState) || curState.regState != regState) {

and I guess `if (obj.prop === undefined)` will also cause the same problem because you indeed reference to obj.prop when `prop` is not existed

Instead of rewriting all the `if` statement, I suggest to create the `property` in the beginning. Assign them to `undefined` to make sure all the behavior are not changed because when property is not existed (the case before modification) is evaluated to `undefined`

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Sorry for not being clear.
I mean

let oldState = curState.regState || null;
if (oldState != regState) {
...
}

Please also read again what I commented in Comment 6.
"I think what we really care is the value of the property, not the property exists or not."

Checking the property exists nor initializing the property to undefined don't make any sense.

Comment 9

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: #2 Fix javascript warning in ril_worker.js

Comment 10

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 769607 [details] [diff] [review]
#2 Fix javascript warning in ril_worker.js

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

::: dom/system/gonk/ril_worker.js
@@ +3207,5 @@
>        curState.cell.gsmCellId = cid;
>        changed = true;
>      }
>  
> +    let radioTech = (newState[3] === undefined

Sometimes newState[3] is not existed. Passing it into RIL.parseInt will get javascript warning.

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 769607 [details] [diff] [review]
#2 Fix javascript warning in ril_worker.js

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

::: dom/system/gonk/ril_worker.js
@@ +3209,5 @@
>      }
>  
> +    let radioTech = (newState[3] === undefined
> +                     ? NETWORK_CREG_TECH_UNKNOWN
> +                     : RIL.parseInt(newState[3], NETWORK_CREG_TECH_UNKNOWN));

nit, move operators to the end of line.
expr1 ?
expr2 :
expr ;

So I can know immediately without guessing is it
expr1
,expr2

or 

expr1
&& expr2

Comment 12

[Szu-Yu Chen [:aknow] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=a3487c69779f

Comment 13

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Fix javascript warning in ril_worker.js. r=yoshi.

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

http://hg.mozilla.org/projects/birch/rev/1bb80977f287

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1bb80977f287