859200 - B2G RIL: Properties for RIL do not work as expected

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: Proposed fix

Because some values are handled as boolean but libcutils.property_get() returns string.

Comment 1

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

Comment on attachment 734465 [details] [diff] [review]
Proposed fix

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

Hi, Hiroyuki-san

Nice catch.
Next time your could use 'git blame' or 'hg blame' to find the reviewer and send r? to him/her.
Otherwise nobody will notice this.

I got some questions for your patch.
See below.

::: dom/system/gonk/ril_worker.js
@@ +74,1 @@
>  let RILQUIRKS_REQUEST_USE_DIAL_EMERGENCY_CALL = libcutils.property_get("ro.moz.ril.dial_emergency_call");

Why didn't you modify this?

@@ +74,2 @@
>  let RILQUIRKS_REQUEST_USE_DIAL_EMERGENCY_CALL = libcutils.property_get("ro.moz.ril.dial_emergency_call");
> +let RILQUIRKS_MODEM_DEFAULTS_TO_EMERGENCY_MODE = libcutils.property_get("ro.moz.ril.emergency_by_default", "true") === "true";

Why use 'true' as default value?

@@ +74,3 @@
>  let RILQUIRKS_REQUEST_USE_DIAL_EMERGENCY_CALL = libcutils.property_get("ro.moz.ril.dial_emergency_call");
> +let RILQUIRKS_MODEM_DEFAULTS_TO_EMERGENCY_MODE = libcutils.property_get("ro.moz.ril.emergency_by_default", "true") === "true";
> +let RILQUIRKS_SIM_APP_STATE_EXTRA_FIELDS = libcutils.property_get("ro.moz.ril.simstate_extra_field", "true") === "true";

ditto.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Fix v2

Set default to the same as in https://hg.mozilla.org/mozilla-central/rev/ee73e7a89076

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Yoshi, thank you for the comment.

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #1)
> ::: dom/system/gonk/ril_worker.js
> @@ +74,1 @@
> >  let RILQUIRKS_REQUEST_USE_DIAL_EMERGENCY_CALL = libcutils.property_get("ro.moz.ril.dial_emergency_call");
> 
> Why didn't you modify this?

I misunderstood the value is the emergency call numbers. I mean I thought the value is "911" or something similar.

> @@ +74,2 @@
> >  let RILQUIRKS_REQUEST_USE_DIAL_EMERGENCY_CALL = libcutils.property_get("ro.moz.ril.dial_emergency_call");
> > +let RILQUIRKS_MODEM_DEFAULTS_TO_EMERGENCY_MODE = libcutils.property_get("ro.moz.ril.emergency_by_default", "true") === "true";
> 
> Why use 'true' as default value?

Actually I did not know about which values should be appropriate by default, but thanks Yoshi, cloud find the previous commit <https://hg.mozilla.org/mozilla-central/rev/ee73e7a89076>. 

The new patch in comment#2 has the same values as default in the commit.

Comment 4

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

Comment on attachment 734507 [details] [diff] [review]
Fix v2

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

Hi Hiroyuki-san
In my previous comment I mean you can find the reviewer, not find the author.
I am also one of the reviewers here so I just took the review.

The patch looks good to me,
please add r=me.
However please update your bug title.
It's a rule defined in Mozilla.

"[PATCH] Get properties for RIL as boolean." -> "Bug 859200 - ... r=allstars.chh"

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Just modified bug title

Carrying over r+.
Yoshi, thank you for your kindness!

Comment 6

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

Can you do it again?

Make it single line and remove the [PATCH] prefix.

"[PATCH] Bug 859200 - Get properties for RIL as boolean.
 r=allstars.chh" -->

"Bug 859200 - Get properties for RIL as boolean. r=allstars.chh"

Comment 7

[Vincent Liu[:vliu]]

As my point of view, these ril quirks should be platform dependent and we won't assume any default value in ril_worker.js.

How about we just use string when we really use these quriks in ril_worker.js. Take an example.

if (RILQUIRKS_SIM_APP_STATE_EXTRA_FIELDS === "true") {
   .....
}

How do you think?

Comment 8

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

(In reply to Vincent Liu[:vliu] from comment #7)
> As my point of view, these ril quirks should be platform dependent and we
> won't assume any default value in ril_worker.js.
> 
> How about we just use string when we really use these quriks in
> ril_worker.js. Take an example.
> 
> if (RILQUIRKS_SIM_APP_STATE_EXTRA_FIELDS === "true") {
>    .....
> }
> 
> How do you think?

For these quirks, a boolean will suffice.
We don't need to make it become 'string' to make it more complicated.
And to make it type of string, those values 'true' or 'false' don't make sense either.

Also these quirks are some work-around/hacks, they are not defined in Android document, we should give them a default value 'false'.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

(In reply to Vincent Liu[:vliu] from comment #7)
> As my point of view, these ril quirks should be platform dependent and we
> won't assume any default value in ril_worker.js.
> 
> How about we just use string when we really use these quriks in
> ril_worker.js. Take an example.
> 
> if (RILQUIRKS_SIM_APP_STATE_EXTRA_FIELDS === "true") {
>    .....
> }

Without default values and if vendor does not specific those values in prop, the following codes will be totally broken

if (RILQUIRKS_V5_LEGACY === "true") {
  ....
}
....

if (RILQUIRKS_V5_LEGACY === "false) {
  ....
}

Comment 10

[Vincent Liu[:vliu]]

I agreed my idea will make it more complicated. Please forget it.

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #8)

> Also these quirks are some work-around/hacks, they are not defined in
> Android document, we should give them a default value 'false'.

I think it is good to give them a default value 'false'. The correct value should set in /device/<partner>/full_<partner>.mk.


(In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> (In reply to Vincent Liu[:vliu] from comment #7)
> > As my point of view, these ril quirks should be platform dependent and we
> > won't assume any default value in ril_worker.js.
> > 
> > How about we just use string when we really use these quriks in
> > ril_worker.js. Take an example.
> > 
> > if (RILQUIRKS_SIM_APP_STATE_EXTRA_FIELDS === "true") {
> >    .....
> > }
> 
> Without default values and if vendor does not specific those values in prop,
> the following codes will be totally broken
> 
> if (RILQUIRKS_V5_LEGACY === "true") {
>   ....
> }
> ....
> 
> if (RILQUIRKS_V5_LEGACY === "false) {
>   ....
> }

I think partner should define the correct quirk value in /device/<partner>/full_<partner>.mk no matter which way to go.

Comment 11

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

(In reply to Vincent Liu[:vliu] from comment #10)
> I think it is good to give them a default value 'false'. The correct value
> should set in /device/<partner>/full_<partner>.mk.
> 
You could still add default values in Makefiles.
But adding a default value here (in JS) can make the code more robustive.

Also just adding default value in Makefiles could make it harder.
Image now we want to add a quirk called MOZ_FOO on some device 'BAR',
now you need to add 'MOZ_FOO := false' to all Makefiles to non-BAR devices.

Comment 12

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

Attachment: Patch

update title.

Comment 13

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

I'll check if the patch works before pushing it.

Comment 14

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

Seems okay to me.
Thanks, Hiroyuki san.

Comment 15

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/f55d9235d5a0

Comment 16

[Phil Ringnalda (:philor)]

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/230b1e362be4 for the test_ril_worker_icc.js failure in https://tbpl.mozilla.org/php/getParsedLog.php?id=21578355&tree=Mozilla-Inbound

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Is there any test stub of libcutils.property_get in xpcshell? We should use the stub in test codes or just modify the magic value '4' to '3'?

Comment 18

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

I am working on refining the test case now.

in dom/system/gonk/tests/test_ril_worker_icc.js,
some tests didn't consider the RIL_V5_LEGACY.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Thank you so much!

Comment 20

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

Attachment: Part 2: update xpcshell tests.

Comment 21

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

Try result : https://tbpl.mozilla.org/?tree=Try&rev=79ff68426df6

Comment 22

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

Attachment: Part 1: Patch v2.

Add 'Part 1' in patch title.

Comment 23

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/47ca2082e784
https://hg.mozilla.org/integration/mozilla-inbound/rev/9aa0ec79f823

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/47ca2082e784
https://hg.mozilla.org/mozilla-central/rev/9aa0ec79f823