907018 - B2G RIL: Refactor message sending in RILContentHelper.js

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: RIL, defect)

Comment 1

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

Fix style nits according to Bug 818393 Comment 43.

Comment 2

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

Attachment: Refactor message sending in RILContentHelper.js

Comment 3

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

Attachment: Refactor message sending in RILContentHelper.js

Comment 4

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

Comment on attachment 792751 [details] [diff] [review]
Refactor message sending in RILContentHelper.js

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

Can you split this patch to different parts?

Part 1: Those functions you add
Part 2: Callers.
Part 3: nits 

Also should those WebAPI also rename to Options?

Comment 5

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

Attachment: (x) Part 1: Fix nits (idl, dom)

According to Bug 818393 Comment 43, we would like to change 'option' to 'options'

Comment 6

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

Attachment: (x) Part 2: Fix nits (ril)

1. Change 'option' to 'options'.
2. 'requestId' in enumeratecalls seems not needed? So I remove it.

Comment 7

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

Attachment: (x) Part 3: Add helper functions for message sending

Comment 8

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

Attachment: (x) Part 4: Modify callers to use helper functions

Comment 9

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

Comment on attachment 793837 [details] [diff] [review]
(x) Part 2: Fix nits (ril)

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

r=me with the comment addressed. Thank you.

::: dom/system/gonk/RILContentHelper.js
@@ +1316,3 @@
>      cpmm.sendAsyncMessage("RIL:EnumerateCalls", {
>        clientId: 0,
>        data: {

Remove the whole 'data' please.

Comment 10

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

Comment on attachment 793837 [details] [diff] [review]
(x) Part 2: Fix nits (ril)

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

::: dom/system/gonk/RILContentHelper.js
@@ +1048,5 @@
>  
>      return request;
>    },
>  
> +  getCallBarringOption: function getCallBarringOption(window, options) {

getCallBarringOptions?

@@ +1068,5 @@
>        data: {
>          requestId: requestId,
> +        program: options.program,
> +        password: options.password,
> +        serviceClass: options.serviceClass

Can't we reuse options?

@@ +1074,5 @@
>      });
>      return request;
>    },
>  
> +  setCallBarringOption: function setCallBarringOption(window, options) {

setCallBarringOptions?

@@ +1095,5 @@
>          requestId: requestId,
> +        program: options.program,
> +        enabled: options.enabled,
> +        password: options.password,
> +        serviceClass: options.serviceClass

reuse?

@@ +2031,3 @@
>     */
> +  _isValidCallBarringOption: function _isValidCallBarringOption(options) {
> +    return (options &&

options != null

if options is null or undefined, your function will return that, instead of true or false

Comment 11

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

Comment on attachment 793837 [details] [diff] [review]
(x) Part 2: Fix nits (ril)

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

Object reusing is handled in next patch.

::: dom/system/gonk/RILContentHelper.js
@@ +1048,5 @@
>  
>      return request;
>    },
>  
> +  getCallBarringOption: function getCallBarringOption(window, options) {

No. We use 'Option' for others like call waiting, call forwarding.

@@ +2031,3 @@
>     */
> +  _isValidCallBarringOption: function _isValidCallBarringOption(options) {
> +    return (options &&

Yes. I will fix it.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 793836 [details] [diff] [review]
(x) Part 1: Fix nits (idl, dom)

You're not changing the interface (the binary representation of it) so
updating uuid isn't technically needed, but doesn't cause much harm either.

But why the methods *Option() but then they take option_s_
So while you're fixing this stuff, perhaps fix also the method names?

Comment 13

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

Comment on attachment 793838 [details] [diff] [review]
(x) Part 3: Add helper functions for message sending

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

::: dom/system/gonk/RILContentHelper.js
@@ +487,5 @@
> +    return this._createSuccessOrErrorRequest(window, false, options);
> +  },
> +
> +  _createSuccessOrErrorRequest:
> +      function _createSuccessOrErrorRequest(window, success, options) {

Cannot we call it createRequest?
And do we really need createSuccess and createError?

Seems createRequest is enough.

@@ +494,5 @@
> +    let requestId = this.getRequestId(request);
> +
> +    if (options === undefined) {
> +      options = null;
> +    }

Can you explain here?

@@ +505,5 @@
> +    return request;
> +  },
> +
> +  _createAsyncMessage:
> +      function _createAsyncMessage(clientId, message, options) {

I think what you need is a wrapper to wrap those arguments into an object.

And it didn't create a message, so the function needs to be renamed.

@@ +508,5 @@
> +  _createAsyncMessage:
> +      function _createAsyncMessage(clientId, message, options) {
> +    if (options === undefined) {
> +      options = {};
> +    }

ditto.

@@ +519,5 @@
> +                                          recordWindow) {
> +    this._checkWindow(window);
> +    let request = Services.DOMRequest.createRequest(window);
> +    let requestId = this.getRequestId(request);
> +

The code here looks like createRequest.
Can't we reuse the existing code ?

@@ +526,5 @@
> +    }
> +
> +    if (options == null) {
> +      options = {};
> +    }

ditto

@@ +532,5 @@
> +    this._createAsyncMessage(clientId, message, options);
> +    return request;
> +  },
> +
> +  _getSyncMessageResponse: function _getSyncMessageResponse(clientId, message) {

You didn't like the original sendSyncMessage(..)[0] style?

I think you could rename your function to sendSycMessage(..), because it did send a message.

Comment 14

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

(In reply to Olli Pettay [:smaug] from comment #12)
> Comment on attachment 793836 [details] [diff] [review]
> Part 1: Fix nits (idl, dom)
> 
> You're not changing the interface (the binary representation of it) so
> updating uuid isn't technically needed, but doesn't cause much harm either.
> 
> But why the methods *Option() but then they take option_s_
> So while you're fixing this stuff, perhaps fix also the method names?

Actually, I like to use the singular form. This is also the origiinal format.
However, it looks like there are some conflict.

For function name and message name, we use 'Option'.
For parameters, we always use 'options'.

Yoshi, do you have any suggestion?

Comment 15

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

I have the same question with smaug, see Comment 4.

Comment 16

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

Comment on attachment 793838 [details] [diff] [review]
(x) Part 3: Add helper functions for message sending

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

::: dom/system/gonk/RILContentHelper.js
@@ +487,5 @@
> +    return this._createSuccessOrErrorRequest(window, false, options);
> +  },
> +
> +  _createSuccessOrErrorRequest:
> +      function _createSuccessOrErrorRequest(window, success, options) {

Use 'Success'/'Error' in function name might be easier to use rather than give a true/false parameter when calling.

@@ +494,5 @@
> +    let requestId = this.getRequestId(request);
> +
> +    if (options === undefined) {
> +      options = null;
> +    }

The function could be use by given only two arguments.
In this case. 'options' is default to null.

@@ +505,5 @@
> +    return request;
> +  },
> +
> +  _createAsyncMessage:
> +      function _createAsyncMessage(clientId, message, options) {

Maybe rename it to _sendAsycMessage?

I think that use multiple parameters form is easier for caller to use.
Otherwise, we have to write a lot of characters for an object literal.

@@ +519,5 @@
> +                                          recordWindow) {
> +    this._checkWindow(window);
> +    let request = Services.DOMRequest.createRequest(window);
> +    let requestId = this.getRequestId(request);
> +

createRequest will fire a success/error event.
What I want to do here is to send an async message and return the request object.

@@ +532,5 @@
> +    this._createAsyncMessage(clientId, message, options);
> +    return request;
> +  },
> +
> +  _getSyncMessageResponse: function _getSyncMessageResponse(clientId, message) {

It's OK for original usage.
Since I wrap the cpmm.sendAsyncMessage in another helper function.
So I think maybe we should have a similar form for syncMessage.

Comment 17

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

Comment on attachment 793838 [details] [diff] [review]
(x) Part 3: Add helper functions for message sending

Hsinyi, what's your opinion for this patch?

Comment 18

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

Comment on attachment 793839 [details] [diff] [review]
(x) Part 4: Modify callers to use helper functions

Hsinyi, what's your opinion for this patch?

Comment 19

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

I glanced over the whole patches. The scope of this bug looks ambiguous. The patches want to solve two things: part1 & part2 for Bug 818393 Comment 43 & Bug 818393 Comment 44, while part2 & part3 & part4 for the *real* refactoring message sending in RILContentHelper.js. I am personally inclined to have every bug scope clear, even we might have several smaller pieces. Can we split them and put them in the appropriate bugs?

(In reply to Szu-Yu Chen [:aknow] from comment #14)
> (In reply to Olli Pettay [:smaug] from comment #12)
> > Comment on attachment 793836 [details] [diff] [review]
> > Part 1: Fix nits (idl, dom)
> > 
> > You're not changing the interface (the binary representation of it) so
> > updating uuid isn't technically needed, but doesn't cause much harm either.
> > 
> > But why the methods *Option() but then they take option_s_
> > So while you're fixing this stuff, perhaps fix also the method names?
> 
> Actually, I like to use the singular form. This is also the origiinal format.
> However, it looks like there are some conflict.
> 
> For function name and message name, we use 'Option'.
> For parameters, we always use 'options'.
> 
> Yoshi, do you have any suggestion?

For Bug 818393 Comment 44, first as my comment above, part1 is out of scope of this *refactoring* bug, I don't think we should address it here. Secondly, we use 'option' to be consistent with other existing functions, but I agree that we could make the naming better (taking smaug and yoshi's suggestions). I see some options we could have:
1) File another bug to address the similar naming problems all together.
2) We are moving MobileConnection API to webidl very soon, and let's rename all together there.
3) I am thinking of proposing moving call setting related functions from MobileConnection API to Telephony API, and let's rename all together there.

Make sense? Thank you.

Comment 20

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #19)
> I glanced over the whole patches. The scope of this bug looks ambiguous. The
> patches want to solve two things: part1 & part2 for Bug 818393 Comment 43 &
> Bug 818393 Comment 44, while part2 & part3 & part4 for the *real*
> refactoring message sending in RILContentHelper.js. I am personally inclined
> to have every bug scope clear, even we might have several smaller pieces.
> Can we split them and put them in the appropriate bugs?
> 
> (In reply to Szu-Yu Chen [:aknow] from comment #14)
> > (In reply to Olli Pettay [:smaug] from comment #12)
> > > Comment on attachment 793836 [details] [diff] [review]
> > > Part 1: Fix nits (idl, dom)
> > > 
> > > You're not changing the interface (the binary representation of it) so
> > > updating uuid isn't technically needed, but doesn't cause much harm either.
> > > 
> > > But why the methods *Option() but then they take option_s_
> > > So while you're fixing this stuff, perhaps fix also the method names?
> > 
> > Actually, I like to use the singular form. This is also the origiinal format.
> > However, it looks like there are some conflict.
> > 
> > For function name and message name, we use 'Option'.
> > For parameters, we always use 'options'.
> > 
> > Yoshi, do you have any suggestion?
> 
> For Bug 818393 Comment 44, first as my comment above, part1 is out of scope
> of this *refactoring* bug, I don't think we should address it here.
> Secondly, we use 'option' to be consistent with other existing functions,
> but I agree that we could make the naming better (taking smaug and yoshi's
> suggestions). I see some options we could have:
> 1) File another bug to address the similar naming problems all together.
> 2) We are moving MobileConnection API to webidl very soon, and let's rename
> all together there.
> 3) I am thinking of proposing moving call setting related functions from
> MobileConnection API to Telephony API, and let's rename all together there.
> 
> Make sense? Thank you.

Yes. I also found that the scope of this bug is ambiguous.
I would like to work on refactoring on this bug. Then file another bug for the naming issue and also addressing Bug 818393 Comment 43 & Bug 818393 Comment 44.

Comment 21

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

Attachment: Part 1: Add helper functions for message sending

Comment 22

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

Attachment: Part 2: Modify callers to use helper functions

Comment 23

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

Attachment: Part 1#2: Add helper functions for message sending

Comment 24

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

Comment on attachment 794491 [details] [diff] [review]
Part 1#2: Add helper functions for message sending

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

::: dom/system/gonk/RILContentHelper.js
@@ +486,5 @@
> +  /**
> +   * Create request and fire request success. Default |options| is null if not
> +   * provided.
> +   */
> +  _createSuccessRequest: function _createSuccessRequest(window, options) {

The name seems not correct. We cannot and don't really create a 'success' request. Sounds a little bit weird to me...

From the caller side, creating a request is one thing, and how we are going to handle the request, such as dispatching success/error, firing success/error or sending Asyn message, is another. Wouldn't it clearer that we indicate the two operations in the caller function?

Same feeling for _createErrorRequest and _createAsyncMessageRequest.

@@ +497,5 @@
> +  /**
> +   * Create request and fire request error. Default |options| is null if not
> +   * provided.
> +   */
> +  _createErrorRequest: function _createErrorRequest(window, options) {

ditto.

@@ +520,5 @@
> +  /**
> +   * Create request and send async message. Default |options| is an empty object
> +   * if not provided.
> +   */
> +  _createAsyncMessageRequest:

ditto.

@@ +536,5 @@
> +    this._sendAsyncMessage(clientId, message, options);
> +    return request;
> +  },
> +
> +  _getSyncMessageResponse: function _getSyncMessageResponse(clientId, message) {

Sorry I am not that in favor of this helper function. 
cpmm.sendSyncMessage broadcasts the message to all the listeners, and returns an array containing return values from each listener invoked.

In RIL code, yes, currently there's only one listener so it's clear that we get the value from array[0]. But if we want to make it a helper function, I'm afraid it's not complete and general enough.

More, I don't see much benefit from _getSyncMessageResponse, the code size and naming explicitness, especially we will be very soon getting rid of the line 'clientId = clientId || 0.' So I am inclined to simply using what we are using now.

Comment 25

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

Comment on attachment 794491 [details] [diff] [review]
Part 1#2: Add helper functions for message sending

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

::: dom/system/gonk/RILContentHelper.js
@@ +486,5 @@
> +  /**
> +   * Create request and fire request success. Default |options| is null if not
> +   * provided.
> +   */
> +  _createSuccessRequest: function _createSuccessRequest(window, options) {

Naming issue is easier to fix.
For example, =>
  createRequestAndFireSuccess()
  createRequestAndFireError()
  createRequestAndSendAsyncMessage()
I just thought the name like these is lengthy.

Moreover, one of the reason I name the function to createSuccessRequest() is that I would like to show that the function will return a request object.

There are three types of actions we may do after create request. 1) fire success, 2) fire error, 3) send async message.

Every time, we only do one and exactly one of above action. So I suggest to simply combine creating request and the action above into one function for caller to use.

@@ +536,5 @@
> +    this._sendAsyncMessage(clientId, message, options);
> +    return request;
> +  },
> +
> +  _getSyncMessageResponse: function _getSyncMessageResponse(clientId, message) {

I am not going to shorten the original code. I would like to create a helper function for sync message that is similar to what we do for async message.

Therefore, they could be called with similar parameters (clientId, message, and maybe options). Moreover, I don't like to see {clienId: clientId} object literal everywhere we send the message. If the parameter is necessary, we should make it a positional parameter not a keyword parameter. So every time, we call the function, we know we should supply a clientId argument. Using original keyword argument form is lacking of this information.

The helper function is to help the code in RIL. I think that taking care of the unexisted usage might be overdesign. Yes, it might be not complete. However, why it should be complete and general in this time. Code evolves with time. Helper function could be change in the future if the requirement changes.

Comment 26

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #24)
> @@ +536,5 @@
> > +    this._sendAsyncMessage(clientId, message, options);
> > +    return request;
> > +  },
> > +
> > +  _getSyncMessageResponse: function _getSyncMessageResponse(clientId, message) {
> 
> Sorry I am not that in favor of this helper function. 
> cpmm.sendSyncMessage broadcasts the message to all the listeners, and
> returns an array containing return values from each listener invoked.
> 
> In RIL code, yes, currently there's only one listener so it's clear that we
> get the value from array[0]. But if we want to make it a helper function,
> I'm afraid it's not complete and general enough.
> 
> More, I don't see much benefit from _getSyncMessageResponse, the code size
> and naming explicitness, especially we will be very soon getting rid of the
> line 'clientId = clientId || 0.' So I am inclined to simply using what we
> are using now.

Btw, the issue here is also easier to fix.
Just rename the function to getSyncMessageResponse's' and return a array object cpmm.sendSyncMessage(message, {clientId: clientId});

Then, getting responses[0] is controlled in caller.

Comment 27

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

(In reply to Szu-Yu Chen [:aknow] from comment #25)
> Comment on attachment 794491 [details] [diff] [review]
> Part 1#2: Add helper functions for message sending
> 
> Review of attachment 794491 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RILContentHelper.js
> @@ +486,5 @@
> > +  /**
> > +   * Create request and fire request success. Default |options| is null if not
> > +   * provided.
> > +   */
> > +  _createSuccessRequest: function _createSuccessRequest(window, options) {
> 
> Naming issue is easier to fix.
> For example, =>
>   createRequestAndFireSuccess()
>   createRequestAndFireError()
>   createRequestAndSendAsyncMessage()
> I just thought the name like these is lengthy.
> 
> Moreover, one of the reason I name the function to createSuccessRequest() is
> that I would like to show that the function will return a request object.
> 
> There are three types of actions we may do after create request. 1) fire
> success, 2) fire error, 3) send async message.

Not exactly.

Please be careful that we have dispatching and firing the two mean differently.

> Every time, we only do one and exactly one of above action. So I suggest to
> simply combine creating request and the action above into one function for
> caller to use.
> 
> @@ +536,5 @@
> > +    this._sendAsyncMessage(clientId, message, options);
> > +    return request;
> > +  },
> > +
> > +  _getSyncMessageResponse: function _getSyncMessageResponse(clientId, message) {
> 
> I am not going to shorten the original code. I would like to create a helper
> function for sync message that is similar to what we do for async message.
> 
> Therefore, they could be called with similar parameters (clientId, message,
> and maybe options). Moreover, I don't like to see {clienId: clientId} object
> literal everywhere we send the message. If the parameter is necessary, we
> should make it a positional parameter not a keyword parameter. So every
> time, we call the function, we know we should supply a clientId argument.
> Using original keyword argument form is lacking of this information.
> 
> The helper function is to help the code in RIL. I think that taking care of
> the unexisted usage might be overdesign. Yes, it might be not complete.
> However, why it should be complete and general in this time. Code evolves
> with time. Helper function could be change in the future if the requirement
> changes.

Comment 28

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #27)
> (In reply to Szu-Yu Chen [:aknow] from comment #25)
> > Comment on attachment 794491 [details] [diff] [review]
> > Part 1#2: Add helper functions for message sending
> > 
> > Review of attachment 794491 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/system/gonk/RILContentHelper.js
> > @@ +486,5 @@
> > > +  /**
> > > +   * Create request and fire request success. Default |options| is null if not
> > > +   * provided.
> > > +   */
> > > +  _createSuccessRequest: function _createSuccessRequest(window, options) {
> > 
> > Naming issue is easier to fix.
> > For example, =>
> >   createRequestAndFireSuccess()
> >   createRequestAndFireError()
> >   createRequestAndSendAsyncMessage()
> > I just thought the name like these is lengthy.
> > 
> > Moreover, one of the reason I name the function to createSuccessRequest() is
> > that I would like to show that the function will return a request object.
> > 
> > There are three types of actions we may do after create request. 1) fire
> > success, 2) fire error, 3) send async message.
> 
> Not exactly.
> 
> Please be careful that we have dispatching and firing the two mean
> differently.

When calling from DOM->RILContentHelper,
After creating request, we only use dispatchFireRequestXXXXX.
I don't see the counter example.

firingRequestXXX is used when we received the response from RadioInterfaceLayer

Comment 29

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

(In reply to Szu-Yu Chen [:aknow] from comment #26)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #24)
> > @@ +536,5 @@
> > > +    this._sendAsyncMessage(clientId, message, options);
> > > +    return request;
> > > +  },
> > > +
> > > +  _getSyncMessageResponse: function _getSyncMessageResponse(clientId, message) {
> > 
> > Sorry I am not that in favor of this helper function. 
> > cpmm.sendSyncMessage broadcasts the message to all the listeners, and
> > returns an array containing return values from each listener invoked.
> > 
> > In RIL code, yes, currently there's only one listener so it's clear that we
> > get the value from array[0]. But if we want to make it a helper function,
> > I'm afraid it's not complete and general enough.
> > 
> > More, I don't see much benefit from _getSyncMessageResponse, the code size
> > and naming explicitness, especially we will be very soon getting rid of the
> > line 'clientId = clientId || 0.' So I am inclined to simply using what we
> > are using now.
> 
> Btw, the issue here is also easier to fix.
> Just rename the function to getSyncMessageResponse's' and return a array
> object cpmm.sendSyncMessage(message, {clientId: clientId});
> 
> Then, getting responses[0] is controlled in caller.
Being controlled in calles seems the right way at least. We should do so, and please do.

Also, any reason that you didn't name the function _sendSyncMessage, since you have _sendAsyncMessage? Vote for _sendSyncMessage.

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #27)
> (In reply to Szu-Yu Chen [:aknow] from comment #25)
> > Comment on attachment 794491 [details] [diff] [review]
> > Part 1#2: Add helper functions for message sending
> > 
> > Review of attachment 794491 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/system/gonk/RILContentHelper.js
> > @@ +486,5 @@
> > > +  /**
> > > +   * Create request and fire request success. Default |options| is null if not
> > > +   * provided.
> > > +   */
> > > +  _createSuccessRequest: function _createSuccessRequest(window, options) {
> > 
> > Naming issue is easier to fix.
> > For example, =>
> >   createRequestAndFireSuccess()
> >   createRequestAndFireError()
> >   createRequestAndSendAsyncMessage()
> > I just thought the name like these is lengthy.
> > 

I am not that excited to combine those two distinct but sequential options into one helper function. However, I am okay if we can have clearer and friendly names. The above recommendations look better than the ones in the patch to me. I am hoping to see Yoshi's comment here. But please use 'DispatchSuccess' and 'DispatchError' since the results are going to be fired on the next tick, not right here. 'Dispatch' implies well than 'Fire.' Also, add a comment emphasizing 'fire success/error *on the next tick*' thank you!

> > Moreover, one of the reason I name the function to createSuccessRequest() is
> > that I would like to show that the function will return a request object.
Yup, your attempt is obvious, and I don't doubt the 'create' usage. :)

Comment 30

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #29)
> (In reply to Szu-Yu Chen [:aknow] from comment #26)
> > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #24)
> > > @@ +536,5 @@
> > > > +    this._sendAsyncMessage(clientId, message, options);
> > > > +    return request;
> > > > +  },
> > > > +
> > > > +  _getSyncMessageResponse: function _getSyncMessageResponse(clientId, message) {
> > > 
> > > Sorry I am not that in favor of this helper function. 
> > > cpmm.sendSyncMessage broadcasts the message to all the listeners, and
> > > returns an array containing return values from each listener invoked.
> > > 
> > > In RIL code, yes, currently there's only one listener so it's clear that we
> > > get the value from array[0]. But if we want to make it a helper function,
> > > I'm afraid it's not complete and general enough.
> > > 
> > > More, I don't see much benefit from _getSyncMessageResponse, the code size
> > > and naming explicitness, especially we will be very soon getting rid of the
> > > line 'clientId = clientId || 0.' So I am inclined to simply using what we
> > > are using now.
> > 
> > Btw, the issue here is also easier to fix.
> > Just rename the function to getSyncMessageResponse's' and return a array
> > object cpmm.sendSyncMessage(message, {clientId: clientId});
> > 
> > Then, getting responses[0] is controlled in caller.
> Being controlled in calles seems the right way at least. We should do so,
> and please do.
> 
> Also, any reason that you didn't name the function _sendSyncMessage, since
> you have _sendAsyncMessage? Vote for _sendSyncMessage.

I think that sendSyncMessage has less information about the response. Considering
1) let response = this._sendSyncMessage(0, "RIL:XXX")[0];
2) let response = this._getSyncMessageResponses(0, "RIL:XXX")[0];

_sendSyncMessage is indeed a very good choice. I am also ok for this.

> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #27)
> > (In reply to Szu-Yu Chen [:aknow] from comment #25)
> > > Comment on attachment 794491 [details] [diff] [review]
> > > Part 1#2: Add helper functions for message sending
> > > 
> > > Review of attachment 794491 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: dom/system/gonk/RILContentHelper.js
> > > @@ +486,5 @@
> > > > +  /**
> > > > +   * Create request and fire request success. Default |options| is null if not
> > > > +   * provided.
> > > > +   */
> > > > +  _createSuccessRequest: function _createSuccessRequest(window, options) {
> > > 
> > > Naming issue is easier to fix.
> > > For example, =>
> > >   createRequestAndFireSuccess()
> > >   createRequestAndFireError()
> > >   createRequestAndSendAsyncMessage()
> > > I just thought the name like these is lengthy.
> > > 
> 
> I am not that excited to combine those two distinct but sequential options
> into one helper function. However, I am okay if we can have clearer and
> friendly names. The above recommendations look better than the ones in the
> patch to me. I am hoping to see Yoshi's comment here. But please use
> 'DispatchSuccess' and 'DispatchError' since the results are going to be
> fired on the next tick, not right here. 'Dispatch' implies well than 'Fire.'
> Also, add a comment emphasizing 'fire success/error *on the next tick*'
> thank you!
> 
> > > Moreover, one of the reason I name the function to createSuccessRequest() is
> > > that I would like to show that the function will return a request object.
> Yup, your attempt is obvious, and I don't doubt the 'create' usage. :)

Another question?

_sendAsyncMessage: function _sendAsyncMessage(clientId, message, options) {
  clientId = clientId || 0;                                                
  cpmm.sendAsyncMessage(message, {               
    clientId: clientId,                          
    data: (options === undefined) ? {} : options 
  });                                            
}

Currently, the line of setting default clientId is not useful. We always provide '0' when calling this function.

Also, I am now plan to re-order the parameters => (message, clientId, options)
So it could align with the function in RadioInterfaceLayer, ex: sendTelephonyMessage(message, clientId, data)

After re-order, if the function is called w/o clientId, the property 'clientId' will be ignored in message instead of default to 0.

Comment 31

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

(In reply to Szu-Yu Chen [:aknow] from comment #30)
> 
> Another question?
> 
> _sendAsyncMessage: function _sendAsyncMessage(clientId, message, options) {
>   clientId = clientId || 0;                                                
>   cpmm.sendAsyncMessage(message, {               
>     clientId: clientId,                          
>     data: (options === undefined) ? {} : options 
>   });                                            
> }
> 
> Currently, the line of setting default clientId is not useful. We always
> provide '0' when calling this function.
> 

That's because the multisim API isn't landed yet. So only the default , id == 0, is called so far.

> Also, I am now plan to re-order the parameters => (message, clientId,
> options)
> So it could align with the function in RadioInterfaceLayer, ex:
> sendTelephonyMessage(message, clientId, data)
> 
> After re-order, if the function is called w/o clientId, the property
> 'clientId' will be ignored in message instead of default to 0.

Re-ordering looks good! Just kindly keep the multisim structure in mind. Thank you~~

Comment 32

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #31)
> (In reply to Szu-Yu Chen [:aknow] from comment #30)
> > 
> > Another question?
> > 
> > _sendAsyncMessage: function _sendAsyncMessage(clientId, message, options) {
> >   clientId = clientId || 0;                                                
> >   cpmm.sendAsyncMessage(message, {               
> >     clientId: clientId,                          
> >     data: (options === undefined) ? {} : options 
> >   });                                            
> > }
> > 
> > Currently, the line of setting default clientId is not useful. We always
> > provide '0' when calling this function.
> > 
> 
> That's because the multisim API isn't landed yet. So only the default , id
> == 0, is called so far.

What I mean is that this is one of the supporting reason for re-ordering the param list. If we want to apply the default value for clientId, 'clientId' should be an optional parameter and not place at the beginning position (my original proposal). Otherwise, we should provide the value when invoke the function.

_sendAsyncMessage(0 /* impossible to ignore this field */, "RIL:XXX")


After re-ordering
It is available to invoke the function by following ways:
_sendAsyncMessage("RIL:XXX", 0)
_sendAsyncMessage("RIL:xxx") <= in this case, we could apply default clinetId.

> > Also, I am now plan to re-order the parameters => (message, clientId,
> > options)
> > So it could align with the function in RadioInterfaceLayer, ex:
> > sendTelephonyMessage(message, clientId, data)
> > 
> > After re-order, if the function is called w/o clientId, the property
> > 'clientId' will be ignored in message instead of default to 0.
> 
> Re-ordering looks good! Just kindly keep the multisim structure in mind.
> Thank you~~

Comment 33

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

Attachment: Part 1#3: Add helper functions for message sending

Comment 34

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

Attachment: Part 2#2: Modify callers to use helper functions

Comment 35

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

Where is the nit fixed I asked in the first place?

Comment 36

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

Comment 20, will file a new bug for naming issue

Comment 37

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

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #10)
> Comment on attachment 793837 [details] [diff] [review]
> (x) Part 2: Fix nits (ril)
> 
> @@ +1068,5 @@
> >        data: {
> >          requestId: requestId,
> > +        program: options.program,
> > +        password: options.password,
> > +        serviceClass: options.serviceClass
> 
> Can't we reuse options?
> 
> 
> @@ +1095,5 @@
> >          requestId: requestId,
> > +        program: options.program,
> > +        enabled: options.enabled,
> > +        password: options.password,
> > +        serviceClass: options.serviceClass
> 
> reuse?
> 
> @@ +2031,3 @@
> >     */
> > +  _isValidCallBarringOption: function _isValidCallBarringOption(options) {
> > +    return (options &&
> 
> options != null
> 
> if options is null or undefined, your function will return that, instead of
> true or false


Where is the patch now?

Comment 38

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

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #37)
> (In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #10)
> > Comment on attachment 793837 [details] [diff] [review]
> > (x) Part 2: Fix nits (ril)
> > 
> > @@ +1068,5 @@
> > >        data: {
> > >          requestId: requestId,
> > > +        program: options.program,
> > > +        password: options.password,
> > > +        serviceClass: options.serviceClass
> > 
> > Can't we reuse options?
> > 
> > 
> > @@ +1095,5 @@
> > >          requestId: requestId,
> > > +        program: options.program,
> > > +        enabled: options.enabled,
> > > +        password: options.password,
> > > +        serviceClass: options.serviceClass
> > 
> > reuse?
> > 
> > @@ +2031,3 @@
> > >     */
> > > +  _isValidCallBarringOption: function _isValidCallBarringOption(options) {
> > > +    return (options &&
> > 
> > options != null
> > 
> > if options is null or undefined, your function will return that, instead of
> > true or false
> 
> 
> Where is the patch now?

(1) option reusing is handled in part2 patch
(2) options != null problem could also be resolved in Bug 909684

Comment 39

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

As I have explained,
please put nits in one part of patch.

Please don't waste others' time on reviewing your patch again and again.

Comment 40

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

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #39)
> As I have explained,
> please put nits in one part of patch.
> 
> Please don't waste others' time on reviewing your patch again and again.

Your original comment for nits contains two parts.
(1) option => options
(2) reuse 'option' object

As comment 19, and comment 20
We decide to put the naming fix (1) into another bug since it might include more api naming change than modifying callbarring itself.

So, in this bug, we only focus on refactoring of message sending. For second parts of nits, 'option' reused. It just handled in part 2 patch, when I modify the caller to use the new helper function. I check all the usage and modify them, not just call barring case.

Actually, this bug do not contain any nits change. As title says, it works on message sending related refactoring.

Comment 41

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

Comment on attachment 795355 [details] [diff] [review]
Part 2#2: Modify callers to use helper functions

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

This part looks good. I'll do a formal review with a new patch rebased on the latest m-c. Thanks.
I pointed some conflicts below but I believe there might be more.

::: dom/system/gonk/RILContentHelper.js
@@ -117,5 @@
>                                     "nsISyncMessageSender");
>  
> -XPCOMUtils.defineLazyServiceGetter(this, "gUUIDGenerator",
> -                                   "@mozilla.org/uuid-generator;1",
> -                                   "nsIUUIDGenerator");

This has been removed in bug 864485.

@@ +841,4 @@
>      if (DEBUG) debug("getCallBarringOption: " + JSON.stringify(option));
>      if (!this._isValidCallBarringOption(option)) {
> +      return this._createRequestAndDispatchError(window,
> +                                                 "InvalidCallBarringOption");

Please rebase it!

@@ -2042,5 @@
>      }
>    },
>  
> -  _getRandomId: function _getRandomId() {
> -    return gUUIDGenerator.generateUUID().toString();

This has been removed in bug 864485.

Comment 42

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

Close the bug. No longer needed.