Last Comment Bug 875243 - B2G RIL: Fix some coding style
: B2G RIL: Fix some coding style
Status: RESOLVED FIXED
RN6/7
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: All Gonk (Firefox OS)
: -- normal (vote)
: mozilla24
Assigned To: Szu-Yu Chen [:aknow] (inactive)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-23 02:21 PDT by Szu-Yu Chen [:aknow] (inactive)
Modified: 2013-06-26 23:42 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix some coding style (38.34 KB, patch)
2013-05-23 02:23 PDT, Szu-Yu Chen [:aknow] (inactive)
no flags Details | Diff | Review
Fix some coding style #2 (39.00 KB, patch)
2013-05-23 03:53 PDT, Szu-Yu Chen [:aknow] (inactive)
no flags Details | Diff | Review
Fix some coding style #3 (40.33 KB, patch)
2013-05-28 03:31 PDT, Szu-Yu Chen [:aknow] (inactive)
htsai: review+
Details | Diff | Review
(wrong) Fix some coding style #4 (57.15 KB, patch)
2013-06-07 00:39 PDT, Szu-Yu Chen [:aknow] (inactive)
no flags Details | Diff | Review
Fix some coding style #5 (46.65 KB, patch)
2013-06-07 00:52 PDT, Szu-Yu Chen [:aknow] (inactive)
no flags Details | Diff | Review
(reference) add test for coding style check (316.87 KB, patch)
2013-06-07 00:55 PDT, Szu-Yu Chen [:aknow] (inactive)
no flags Details | Diff | Review
Fix some coding style #6 (46.57 KB, patch)
2013-06-07 01:28 PDT, Szu-Yu Chen [:aknow] (inactive)
htsai: review+
Details | Diff | Review
[final] Fix some coding style (43.21 KB, patch)
2013-06-07 03:11 PDT, Szu-Yu Chen [:aknow] (inactive)
no flags Details | Diff | Review

Description Szu-Yu Chen [:aknow] (inactive) 2013-05-23 02:21:06 PDT
Fix some minor coding style
(1) missing ';'
(2) unnecessary ';'
(3) alignment
(4) replace "for each ... in" by "for ... of" if possible (for each is Deprecated)
(5) when comparing to 0, use '===' or '!==' to ensure the type checking
(6) remove defined but unused variable
(7) use /* fall through */ annotation in switch so that it could pass jslint or jshint checking
(8) others ...
Comment 1 Szu-Yu Chen [:aknow] (inactive) 2013-05-23 02:23:07 PDT
Created attachment 753183 [details] [diff] [review]
Fix some coding style
Comment 2 Szu-Yu Chen [:aknow] (inactive) 2013-05-23 02:23:58 PDT
Tested basic power-on and phone call

https://tbpl.mozilla.org/?tree=Try&rev=6667abdd8976
Comment 3 Hsin-Yi Tsai [:hsinyi] 2013-05-23 02:33:31 PDT
CC vicamo and yoshi since some changes don't follow the convention and the mozilla coding style as I know.

Also, if you think some code are not used, then the better way seems be simply removing them or correcting them, not commenting them.
Comment 4 Yoshi Huang[:allstars.chh] 2013-05-23 02:44:24 PDT
(In reply to Szu-Yu Chen [:aknow] from comment #0)
> (7) use /* fall through */ annotation in switch so that it could pass jslint
> or jshint checking

jslint doesn't accept '// Fall through' ?
Comment 5 Hsin-Yi Tsai [:hsinyi] 2013-05-23 02:45:27 PDT
Loop Gene as well :)
Comment 6 Yoshi Huang[:allstars.chh] 2013-05-23 03:00:51 PDT
Comment on attachment 753183 [details] [diff] [review]
Fix some coding style

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

::: dom/system/gonk/RILContentHelper.js
@@ +1576,5 @@
> +        case Ci.nsIDOMMozMobileCFInfo.CALL_FORWARD_REASON_ALL_CONDITIONAL_CALL_FORWARDING:
> +        return true;
> +      default:
> +        return false;
> +    }

From line 1572 indent is wrong.

@@ +1585,5 @@
>     */
> +  _isValidCFAction: function _isValidCFAction(action) {
> +    switch (action) {
> +      case Ci.nsIDOMMozMobileCFInfo.CALL_FORWARD_ACTION_DISABLE:
> +        case Ci.nsIDOMMozMobileCFInfo.CALL_FORWARD_ACTION_ENABLE:

ditto

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2857,2 @@
>      }
> +    return false;

put it in default?

::: dom/system/gonk/ril_worker.js
@@ +1536,3 @@
>      var path = options.apdu.path;
>      var data = options.apdu.data;
>      var data2 = options.apdu.data2;

let path = options.apdu.path || "";
... etc

Please note that I use 'let' instead of 'var'

@@ +4218,5 @@
>          throw "Invalid format";
>        }
>  
> +      from = parseInt(result[1], 10);
> +      to = (result[2] != null) ? parseInt(result[2], 10) + 1 : from + 1;

is != null neccesary?

@@ +6924,4 @@
>              // short message waiting.` ~ 3GPP TS 31.111 7.1.1.1
>              return [null, PDU_FCS_RESERVED];
>            }
> +          

trailing ws

@@ +6928,4 @@
>            // If the service "data download via SMS-PP" is not available in the
>            // (U)SIM Service Table, ..., then the ME shall store the message in
>            // EFsms in accordance with TS 31.102` ~ 3GPP TS 31.111 7.1.1.1
> +          

trailing ws.

@@ +10615,4 @@
>    readANR: function readANR(fileId, fileType, recordNumber, onsuccess, onerror) {
>      function callback(options) {
>        let strLen = Buf.readUint32();
> +      // let octetLen = strLen / 2;

rm it.
Comment 7 Szu-Yu Chen [:aknow] (inactive) 2013-05-23 03:15:11 PDT
Comment on attachment 753183 [details] [diff] [review]
Fix some coding style

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

::: dom/system/gonk/RILContentHelper.js
@@ +1577,5 @@
> +        return true;
> +      default:
> +        return false;
> +    }
> +  },

sorry for this part. It's my fault and will be corrected.

::: dom/system/gonk/ril_worker.js
@@ +3158,1 @@
>      }

The code indicates how to interpret the data.
So it's better to keep them there.
Removing them might lose some information

@@ +4218,5 @@
>          throw "Invalid format";
>        }
>  
> +      from = parseInt(result[1], 10);
> +      to = (result[2] != null) ? parseInt(result[2], 10) + 1 : from + 1;

Not sure. I will check this part
the difference is when => result[2] is "" (empty string)
Comment 8 Hsin-Yi Tsai [:hsinyi] 2013-05-23 03:19:16 PDT
Comment on attachment 753183 [details] [diff] [review]
Fix some coding style

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

::: dom/system/gonk/ril_worker.js
@@ +3142,4 @@
>      }
>  
>      if (this._isCdma) {
> +      // let baseStationId = RIL.parseInt(state[4]);

I will like to keep the current code here and the followings.

@@ +6924,4 @@
>              // short message waiting.` ~ 3GPP TS 31.111 7.1.1.1
>              return [null, PDU_FCS_RESERVED];
>            }
> +          

nit: trailing ws

@@ +6928,4 @@
>            // If the service "data download via SMS-PP" is not available in the
>            // (U)SIM Service Table, ..., then the ME shall store the message in
>            // EFsms in accordance with TS 31.102` ~ 3GPP TS 31.111 7.1.1.1
> +          

ditto.
Comment 9 Hsin-Yi Tsai [:hsinyi] 2013-05-23 03:25:12 PDT
Comment on attachment 753183 [details] [diff] [review]
Fix some coding style

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

::: dom/system/gonk/ril_worker.js
@@ +3926,2 @@
>            // Fallback to default error handling if it meets max retry count.
> +          /* fall through */

I don't quite understand why we can't use the same comment style "//" here and other places.

@@ +9698,4 @@
>        RIL.sendStkTerminalResponse({
>          resultCode: STK_RESULT_CMD_DATA_NOT_UNDERSTOOD});
>        throw new Error("BerTlvHelper value length too long!!");
> +      // return;

rm
Comment 10 Szu-Yu Chen [:aknow] (inactive) 2013-05-23 03:31:45 PDT
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #4)
> (In reply to Szu-Yu Chen [:aknow] from comment #0)
> > (7) use /* fall through */ annotation in switch so that it could pass jslint
> > or jshint checking
> 
> jslint doesn't accept '// Fall through' ?

So "// Fall through" is mozilla's convention?
I could just modify the jshint to match the string. (it just works)
Will change back to the original format.
Comment 11 Yoshi Huang[:allstars.chh] 2013-05-23 03:48:51 PDT
(In reply to Szu-Yu Chen [:aknow] from comment #10)
> So "// Fall through" is mozilla's convention?
> I could just modify the jshint to match the string. (it just works)
> Will change back to the original format.

I don't think Mozilla has defined this rule.
But we should use consistent comment style, if we already use '//' in other places,
we should stick to it.
Comment 12 Szu-Yu Chen [:aknow] (inactive) 2013-05-23 03:53:21 PDT
Created attachment 753210 [details] [diff] [review]
Fix some coding style #2

thanks for everyone, let me upload the 2nd version
Comment 13 Gene Lian [:gene] (I already quit Mozilla) 2013-05-23 05:16:46 PDT
We often use:

//{one_space}{upper-case-letter}...{period}, like:

// Vicamo is handsome.
Comment 14 Hsin-Yi Tsai [:hsinyi] 2013-05-23 21:06:03 PDT
Comment on attachment 753210 [details] [diff] [review]
Fix some coding style #2

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

Looks great. Thank you for the careful catch!

Please help to make sure the patch won't affect related marionette, xpcshell tests.

::: dom/system/gonk/ril_worker.js
@@ +3134,4 @@
>      }
>  
>      if (this._isCdma) {
> +      // Some variables below are not used. Comment them instead of removed to

grammar nit: s/removed/removing them
Comment 15 Hsin-Yi Tsai [:hsinyi] 2013-05-23 21:07:55 PDT
(In reply to Szu-Yu Chen [:aknow] from comment #0)
> Fix some minor coding style
> (1) missing ';'
> (2) unnecessary ';'
> (3) alignment
> (4) replace "for each ... in" by "for ... of" if possible (for each is
> Deprecated)
> (5) when comparing to 0, use '===' or '!==' to ensure the type checking
> (6) remove defined but unused variable
> (7) use /* fall through */ annotation in switch so that it could pass jslint
> or jshint checking
> (8) others ...

Dear RIL reviewers,

So how about we try our best to follow the above coding style for further review, except (7)? :)
Comment 16 Szu-Yu Chen [:aknow] (inactive) 2013-05-24 03:10:38 PDT
The above errors are detected by jshint

Ril code are written in js. Thus syntax error could not be catched during the compiling time. For me, I encounter many many times fail when power-on device just because some trivial syntax error in js. So I am wondering that maybe I could use some static code analysis tool before flashing the code to device.

I found jshint and this is how I use it on our ril code.
https://taiwan.etherpad.mozilla.org/216

I try to make it work well on ril code, but there is still a lot of room for the improvement.
Need everyone's help and share your experience for developing ril.
Comment 17 Szu-Yu Chen [:aknow] (inactive) 2013-05-28 03:31:25 PDT
Created attachment 754742 [details] [diff] [review]
Fix some coding style #3
Comment 18 Szu-Yu Chen [:aknow] (inactive) 2013-05-28 03:35:17 PDT
Comment on attachment 754742 [details] [diff] [review]
Fix some coding style #3

Move debug part to the beginning.
It's javascript scope issue. For the original code, it might refer to |debug| before it defined.
Comment 19 Szu-Yu Chen [:aknow] (inactive) 2013-05-28 19:24:41 PDT
xpcshell test pass for dom/system/gonk

https://tbpl.mozilla.org/?tree=Try&rev=027eb1b82224
Comment 20 Hsin-Yi Tsai [:hsinyi] 2013-05-29 22:52:54 PDT
Comment on attachment 754742 [details] [diff] [review]
Fix some coding style #3

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

Thanks, r= me with comments addressed.

Hope we figure out a better way to help developers avoid these coding style nits.

B.t.w, please kindly mark the invalid files obsolete.

::: dom/system/gonk/ril_worker.js
@@ +7937,4 @@
>                if (msgDigit == -1) {
>                  throw new Error("'" + msgDigitChar + "' is not in 7 bit alphabet "
>                                  + langIndex + ":" + langShiftIndex + "!");
> +                // break;

rm it.

@@ +9394,4 @@
>          RIL.sendStkTerminalResponse({
>            resultCode: STK_RESULT_CMD_DATA_NOT_UNDERSTOOD});
>          throw new Error("Invalid octet when parsing Comprehension TLV :" + temp);
> +        // break;

rm it.
Comment 21 Hsin-Yi Tsai [:hsinyi] 2013-05-29 23:05:23 PDT
(In reply to Szu-Yu Chen [:aknow] from comment #19)
> xpcshell test pass for dom/system/gonk
> 
> https://tbpl.mozilla.org/?tree=Try&rev=027eb1b82224

I don't see xpcshell log in the try link @@
Just make sure xpcshell tests pass before we land the patch. :)
Comment 22 Szu-Yu Chen [:aknow] (inactive) 2013-05-30 00:44:48 PDT
I run the xpcshell test locally.

Btw, I'm now trying to add the coding style check into a marionette test case.
It seems possible but require some complicated work...
Comment 23 Szu-Yu Chen [:aknow] (inactive) 2013-06-07 00:39:19 PDT
Created attachment 759644 [details] [diff] [review]
(wrong) Fix some coding style #4
Comment 24 Szu-Yu Chen [:aknow] (inactive) 2013-06-07 00:52:24 PDT
Created attachment 759649 [details] [diff] [review]
Fix some coding style #5
Comment 25 Szu-Yu Chen [:aknow] (inactive) 2013-06-07 00:55:27 PDT
Created attachment 759650 [details] [diff] [review]
(reference) add test for coding style check
Comment 26 Szu-Yu Chen [:aknow] (inactive) 2013-06-07 01:15:18 PDT
Give up for making a test case on try server.
I encounter a problem that cannot resolved currently.

For reference, I also attached the test case to keep the current progress. It works well when testing on local pc through `./test.sh marionette` . However, on try server, I cannot locate the source javascript file (ex: ril_worker.js). I even try to add a mechanism in test case to search the file on try server. But it still cannot got result in 20mins; thus cause the timeout fail for this test case.

1. For checking locally by jshint. You can refer to https://taiwan.etherpad.mozilla.org/216
2. or it is also possible to apply this patch (test case), and then run the marioneete test locally
3. If we know how to locate the source file on try server. We could complete this test case on try.
Comment 27 Szu-Yu Chen [:aknow] (inactive) 2013-06-07 01:28:40 PDT
Created attachment 759659 [details] [diff] [review]
Fix some coding style #6

same as rev#5, w/ rebase current master
Comment 28 Hsin-Yi Tsai [:hsinyi] 2013-06-07 01:59:18 PDT
Comment on attachment 759659 [details] [diff] [review]
Fix some coding style #6

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

Thanks for the catch again. r=me with comments addressed.

::: dom/system/gonk/ril_worker.js
@@ +3051,4 @@
>      // Pending network info is ready to be sent when no more messages
>      // are waiting for responses, but the combined payload hasn't been sent.
>      for (let i = 0; i < NETWORK_INFO_MESSAGE_TYPES.length; i++) {
> +      let ttype = NETWORK_INFO_MESSAGE_TYPES[i];

s/ttype/msgType

@@ +3051,5 @@
>      // Pending network info is ready to be sent when no more messages
>      // are waiting for responses, but the combined payload hasn't been sent.
>      for (let i = 0; i < NETWORK_INFO_MESSAGE_TYPES.length; i++) {
> +      let ttype = NETWORK_INFO_MESSAGE_TYPES[i];
> +      if (!(ttype in pending)) {

ditto.

@@ +6101,4 @@
>              header.segmentMaxSeq = max;
>              header.segmentSeq = seq;
>            }
> +        } break;

Coding style we use for switch-case is:
switch (variable) {
  case A:
    // code...
    break;
  case B:
    // code ...
    break;
} 

Let's remove the curly brackets for each 'case', here and followings.
Comment 29 Hsin-Yi Tsai [:hsinyi] 2013-06-07 02:06:24 PDT
(In reply to Szu-Yu Chen [:aknow] from comment #26)
> Give up for making a test case on try server.
> I encounter a problem that cannot resolved currently.
> 
> For reference, I also attached the test case to keep the current progress.
> It works well when testing on local pc through `./test.sh marionette` .
> However, on try server, I cannot locate the source javascript file (ex:
> ril_worker.js). I even try to add a mechanism in test case to search the
> file on try server. But it still cannot got result in 20mins; thus cause the
> timeout fail for this test case.
> 
> 1. For checking locally by jshint. You can refer to
> https://taiwan.etherpad.mozilla.org/216
> 2. or it is also possible to apply this patch (test case), and then run the
> marioneete test locally
> 3. If we know how to locate the source file on try server. We could complete
> this test case on try.

I think it is a good idea that we find out a way to do automatic check in try. As it might take more time before coming out a solution, would you please help file a bug for adding a try test for (ril) javascript code in gecko, and summarize your investigation so far? Then we can move there for further discussion. :)
Comment 30 Szu-Yu Chen [:aknow] (inactive) 2013-06-07 02:11:25 PDT
Comment on attachment 759659 [details] [diff] [review]
Fix some coding style #6

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

::: dom/system/gonk/ril_worker.js
@@ -6097,5 @@
>              header.segmentMaxSeq = max;
>              header.segmentSeq = seq;
>            }
> -          break;
> -        }

It's because of the scope of 'let'.

We have defined the same variable in multiple cases. ex: dstp, orip.
Without {} for each 'case', all of the defined variable will fall into the same scope of 'switch {}'. => variable redifined
Comment 31 Hsin-Yi Tsai [:hsinyi] 2013-06-07 02:34:36 PDT
(In reply to Szu-Yu Chen [:aknow] from comment #30)
> Comment on attachment 759659 [details] [diff] [review]
> Fix some coding style #6
> 
> Review of attachment 759659 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ -6097,5 @@
> >              header.segmentMaxSeq = max;
> >              header.segmentSeq = seq;
> >            }
> > -          break;
> > -        }
> 
> It's because of the scope of 'let'.
> 
> We have defined the same variable in multiple cases. ex: dstp, orip.
> Without {} for each 'case', all of the defined variable will fall into the
> same scope of 'switch {}'. => variable redifined

Ah, I see. Thanks for explanation.
Comment 32 Szu-Yu Chen [:aknow] (inactive) 2013-06-07 03:11:18 PDT
Created attachment 759677 [details] [diff] [review]
[final] Fix some coding style
Comment 33 Szu-Yu Chen [:aknow] (inactive) 2013-06-07 03:56:34 PDT
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #29)
> (In reply to Szu-Yu Chen [:aknow] from comment #26)
> > Give up for making a test case on try server.
> > I encounter a problem that cannot resolved currently.
> > 
> > For reference, I also attached the test case to keep the current progress.
> > It works well when testing on local pc through `./test.sh marionette` .
> > However, on try server, I cannot locate the source javascript file (ex:
> > ril_worker.js). I even try to add a mechanism in test case to search the
> > file on try server. But it still cannot got result in 20mins; thus cause the
> > timeout fail for this test case.
> > 
> > 1. For checking locally by jshint. You can refer to
> > https://taiwan.etherpad.mozilla.org/216
> > 2. or it is also possible to apply this patch (test case), and then run the
> > marioneete test locally
> > 3. If we know how to locate the source file on try server. We could complete
> > this test case on try.
> 
> I think it is a good idea that we find out a way to do automatic check in
> try. As it might take more time before coming out a solution, would you
> please help file a bug for adding a try test for (ril) javascript code in
> gecko, and summarize your investigation so far? Then we can move there for
> further discussion. :)

Creat a bug here
https://bugzilla.mozilla.org/show_bug.cgi?id=880643
Comment 34 Szu-Yu Chen [:aknow] (inactive) 2013-06-09 19:25:47 PDT
Attachment '[final] Fix some coding style' => ready for check-in

https://tbpl.mozilla.org/?tree=Try&rev=ee09ff5ccf08
Comment 35 Hsin-Yi Tsai [:hsinyi] 2013-06-09 19:43:19 PDT
Nice thanks! Please remember to add r=XXX in the commit comment. :)

http://hg.mozilla.org/projects/birch/rev/86a801e6e763
Comment 36 Ed Morley [:emorley] 2013-06-10 02:25:55 PDT
https://hg.mozilla.org/mozilla-central/rev/86a801e6e763
Comment 37 Yoshi Huang[:allstars.chh] 2013-06-26 21:22:45 PDT
Comment on attachment 759677 [details] [diff] [review]
[final] Fix some coding style

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

::: dom/system/gonk/ril_worker.js
@@ +11426,4 @@
>  
>      return (serviceTable &&
>             (index < serviceTable.length) &&
> +           (serviceTable[index] & bitmask)) !== 0;

xpcshell fail.

TEST-UNEXPECTED-FAIL | /home/allstars/src/mozilla/mozilla-central/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/dom/system/gonk/tests/test_ril_worker_icc.js | true == false - See following stack:
JS frame :: /home/allstars/src/mozilla/mozilla-central/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/dom/system/gonk/tests/test_ril_worker_icc.js :: test_table :: line 461
JS frame :: /home/allstars/src/mozilla/mozilla-central/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/dom/system/gonk/tests/test_ril_worker_icc.js :: test_is_icc_service_available :: line 468
JS frame :: /home/allstars/src/mozilla/mozilla-central/testing/xpcshell/head.js :: _run_next_test :: line 1129
Comment 38 Szu-Yu Chen [:aknow] (inactive) 2013-06-26 22:19:04 PDT
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #37)
> Comment on attachment 759677 [details] [diff] [review]
> [final] Fix some coding style
> 
> Review of attachment 759677 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +11426,4 @@
> >  
> >      return (serviceTable &&
> >             (index < serviceTable.length) &&
> > +           (serviceTable[index] & bitmask)) !== 0;
> 
> xpcshell fail.
> 
> TEST-UNEXPECTED-FAIL |
> /home/allstars/src/mozilla/mozilla-central/obj-x86_64-unknown-linux-gnu/
> _tests/xpcshell/dom/system/gonk/tests/test_ril_worker_icc.js | true == false
> - See following stack:
> JS frame ::
> /home/allstars/src/mozilla/mozilla-central/obj-x86_64-unknown-linux-gnu/
> _tests/xpcshell/dom/system/gonk/tests/test_ril_worker_icc.js :: test_table
> :: line 461
> JS frame ::
> /home/allstars/src/mozilla/mozilla-central/obj-x86_64-unknown-linux-gnu/
> _tests/xpcshell/dom/system/gonk/tests/test_ril_worker_icc.js ::
> test_is_icc_service_available :: line 468
> JS frame ::
> /home/allstars/src/mozilla/mozilla-central/testing/xpcshell/head.js ::
> _run_next_test :: line 1129

Sorry for the wrong modification.
I'll file another bug and patch later.

BTW, any suggestion for the fix.
The condition is complicated, the result might be |null|, |false|, |0|, and others.

I don't want to keep the original usage |xxx != 0|
How about convert it to Boolean => return Boolean(serviceTable && ... (... & bitmask));
Comment 39 Yoshi Huang[:allstars.chh] 2013-06-26 23:42:11 PDT
return (serviceTable !== null) &&
       (index < serviceTable.length) &&
       ((serviceTable[index] & bitmask) !== 0);

Note You need to log in before you can comment on or make changes to this bug.