1053110 - [Telephony] Move temporary clir logic from ril_worker to telephonyService

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Comment 1

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

Attachment: Part 2: Refactoring MMI regex

Try to make it more readable.

Comment 2

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

Attachment: Part 3: Move clir logic to TelephonyService

1. Move the clir logic from ril_worker to telephonyService.
2. At this time, let promise rejected when passing an mmi code to telephony.dial

Comment 3

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

Attachment: Part 1: Add mmi code test

Comment 4

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

Comment on attachment 8472201 [details] [diff] [review]
Part 3: Move clir logic to TelephonyService

Got new modification. Cancel the review.

Comment 5

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8472202 [details] [diff] [review]
Part 1: Add mmi code test

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

This looks good though it seems not directly related to this bug.

Comment 6

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

Attachment: Part 3: Create a helper: sendToRilWorker

Comment 7

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

Attachment: Part 4: Move clir logic to TelephonyService

Comment 8

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8474416 [details] [diff] [review]
Part 3: Create a helper: sendToRilWorker

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

::: dom/telephony/gonk/TelephonyService.js
@@ -331,5 @@
>  
>          this._currentCalls[aClientId][call.callIndex] = call;
>        }
> -
> -      return false;

We can't remove this. The 'false' value does matter. Please see http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js?from=RadioInterfaceLayer.js&case=true#1830

Comment 9

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #8)
> Comment on attachment 8474416 [details] [diff] [review]
> Part 3: Create a helper: sendToRilWorker
> 
> Review of attachment 8474416 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/telephony/gonk/TelephonyService.js
> @@ -331,5 @@
> >  
> >          this._currentCalls[aClientId][call.callIndex] = call;
> >        }
> > -
> > -      return false;
> 
> We can't remove this. The 'false' value does matter. Please see
> http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/
> RadioInterfaceLayer.js?from=RadioInterfaceLayer.js&case=true#1830

Returning nothing means 'undefined' and 'undefined' is treated as false.

Comment 10

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8474416 [details] [diff] [review]
Part 3: Create a helper: sendToRilWorker

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

It's okay though considering the workermessenger protocol I am still inclined to returning 'false' explicitly.

Comment 11

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #10)
> Comment on attachment 8474416 [details] [diff] [review]
> Part 3: Create a helper: sendToRilWorker
> 
> Review of attachment 8474416 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It's okay though considering the workermessenger protocol I am still
> inclined to returning 'false' explicitly.

I have a different opinion.

The purpose of the returning value is documented in workermessenger.send() but not sendWorkerMessage(). So, when writing a callback, it's really hard to understand why the function needs a return value. Also, what's the meaning of true or false? I just think that showing a "return" statement there is not so nature. It's better to hide the special ability of the "return value" and not to encourage the usage of it.

Comment 12

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Szu-Yu Chen [:aknow] from comment #11)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #10)
> > Comment on attachment 8474416 [details] [diff] [review]
> > Part 3: Create a helper: sendToRilWorker
> > 
> > Review of attachment 8474416 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > It's okay though considering the workermessenger protocol I am still
> > inclined to returning 'false' explicitly.
> 
> I have a different opinion.
> 
> The purpose of the returning value is documented in workermessenger.send()
> but not sendWorkerMessage(). So, when writing a callback, it's really hard
> to understand why the function needs a return value. 

You are right. I agree with you on this. :\

> Also, what's the
> meaning of true or false? I just think that showing a "return" statement
> there is not so nature. It's better to hide the special ability of the
> "return value" and not to encourage the usage of it.

The file RadioInterfaceLayer.js defines "true" and "false" clearly, but I am convinced by you that for a callback writer, there seems space to improving the design.

Comment 13

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8474417 [details] [diff] [review]
Part 4: Move clir logic to TelephonyService

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

Looks good to me!

::: dom/system/gonk/ril_worker.js
@@ +2381,5 @@
> +   *
> +   * @param number
> +   *        Phone number to be parsed
> +   */
> +  parseMMIFromDialNumber: function(options) {

I believe it's a transient state that we will move the whole parsing work to TelephonyService.js in the bug of cleaning API backward compatibility. (Let me file a bug for tracking.) So it looks fine for now.

Comment 14

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

Attachment: [final] Part 1: Add mmi code test. r=hsinyi

Comment 15

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

Attachment: [final] Part 2: Refactoring MMI regex. r=hsinyi

Comment 16

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

Attachment: [final] Part 3: Create a helper: sendToRilWorker. r=hsinyi

Comment 17

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

Attachment: [final] Part 4: Move clir logic to TelephonyService. r=hsinyi

Comment 18

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

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/c1b4aead588e
https://hg.mozilla.org/integration/b2g-inbound/rev/a2cf291453d9
https://hg.mozilla.org/integration/b2g-inbound/rev/7ab3375eef2b
https://hg.mozilla.org/integration/b2g-inbound/rev/ef36a3938db7

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c1b4aead588e
https://hg.mozilla.org/mozilla-central/rev/a2cf291453d9
https://hg.mozilla.org/mozilla-central/rev/7ab3375eef2b
https://hg.mozilla.org/mozilla-central/rev/ef36a3938db7