Closed Bug 859200 Opened 8 years ago Closed 8 years ago

B2G RIL: Properties for RIL do not work as expected

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: hiro, Assigned: hiro)

Details

Attachments

(2 files, 4 obsolete files)

Attached patch Proposed fix (obsolete) — Splinter Review
Because some values are handled as boolean but libcutils.property_get() returns string.
Attachment #734465 - Flags: review?
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.
Attachment #734465 - Flags: review?
Summary: Properties for RIL do not work as expected → B2G RIL: Properties for RIL do not work as expected
Version: unspecified → Trunk
Attached patch Fix v2 (obsolete) — Splinter Review
Set default to the same as in https://hg.mozilla.org/mozilla-central/rev/ee73e7a89076
Assignee: nobody → hiikezoe
Attachment #734465 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #734507 - Flags: review?(vliu)
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 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"
Attachment #734507 - Flags: review?(vliu) → review+
Attached patch Just modified bug title (obsolete) — Splinter Review
Carrying over r+.
Yoshi, thank you for your kindness!
Attachment #734507 - Attachment is obsolete: true
Attachment #734514 - Flags: review+
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"
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?
(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'.
(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 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.
(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.
Attached patch Patch (obsolete) — Splinter Review
update title.
Attachment #734514 - Attachment is obsolete: true
Attachment #734560 - Flags: review+
I'll check if the patch works before pushing it.
Seems okay to me.
Thanks, Hiroyuki san.
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'?
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.
Thank you so much!
Attachment #735043 - Flags: review?(vyang) → review+
Add 'Part 1' in patch title.
Attachment #734560 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/47ca2082e784
https://hg.mozilla.org/mozilla-central/rev/9aa0ec79f823
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.