Closed Bug 875243 Opened 11 years ago Closed 11 years ago

B2G RIL: Fix some coding style

Categories

(Core :: DOM: Device Interfaces, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: aknow, Assigned: aknow)

Details

(Whiteboard: RN6/7)

Attachments

(3 files, 5 obsolete files)

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 ...
Assignee: nobody → szchen
Attached patch Fix some coding style (obsolete) — Splinter Review
Attachment #753183 - Flags: review?(htsai)
Tested basic power-on and phone call

https://tbpl.mozilla.org/?tree=Try&rev=6667abdd8976
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.
(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' ?
Loop Gene as well :)
Attachment #753183 - Flags: review?(htsai)
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 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 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 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
(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.
(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.
Attached patch Fix some coding style #2 (obsolete) — Splinter Review
thanks for everyone, let me upload the 2nd version
We often use:

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

// Vicamo is handsome.
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
(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)? :)
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.
Attached patch Fix some coding style #3 (obsolete) — Splinter Review
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.
Attachment #754742 - Flags: review?(htsai)
xpcshell test pass for dom/system/gonk

https://tbpl.mozilla.org/?tree=Try&rev=027eb1b82224
Whiteboard: RN5/29
Whiteboard: RN5/29 → RN6/7
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.
Attachment #754742 - Flags: review?(htsai) → review+
(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. :)
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...
Attached patch (wrong) Fix some coding style #4 (obsolete) — Splinter Review
Attachment #753183 - Attachment is obsolete: true
Attachment #753210 - Attachment is obsolete: true
Attachment #754742 - Attachment is obsolete: true
Attached patch Fix some coding style #5 (obsolete) — Splinter Review
Attachment #759644 - Attachment is obsolete: true
Attachment #759644 - Attachment description: Fix some coding style #4 → (wrong) Fix some coding style #4
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.
Attachment #759649 - Flags: review?(htsai)
same as rev#5, w/ rebase current master
Attachment #759649 - Attachment is obsolete: true
Attachment #759649 - Flags: review?(htsai)
Attachment #759659 - Flags: review?(htsai)
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.
Attachment #759659 - Flags: review?(htsai) → review+
(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 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
(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.
(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
Attachment '[final] Fix some coding style' => ready for check-in

https://tbpl.mozilla.org/?tree=Try&rev=ee09ff5ccf08
Keywords: checkin-needed
Nice thanks! Please remember to add r=XXX in the commit comment. :)

http://hg.mozilla.org/projects/birch/rev/86a801e6e763
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/86a801e6e763
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
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
(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));
return (serviceTable !== null) &&
       (index < serviceTable.length) &&
       ((serviceTable[index] & bitmask) !== 0);
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: