Closed Bug 790192 Opened 7 years ago Closed 7 years ago

B2G SMS: support automatically replacing Latin characters with ASCII ones

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18
blocking-basecamp +

People

(Reporter: vicamo, Assigned: vicamo)

Details

(Keywords: feature, Whiteboard: [LOE:S])

Attachments

(2 files, 6 obsolete files)

See Gaia issue https://github.com/mozilla-b2g/gaia/issues/3298.

For some countries, they want to use GSM 7-bit default alphabet encoding for characters that can be replaced with similar-in-shape ASCII ones. The sent message stored should be still consisted of Latin characters while the received message is partially or fully in ASCII.
Characters to be replaced:
Á(\u00C1) => A(\u0041),
á(\u00E1) => a(\u0061),
Í(\u00CD) => I(\u0049),
í(\u00ED) => i(\u0069),
Ó(\u00D3) => O(\u004F),
ó(\u00F3) => o(\u006F),
Ú(\u00DA) => U(\u0055),
ú(\u00FA) => u(\u0075),
ç(\u00E7) => Ç(\u00C7)
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #1)
> Characters to be replaced:
[...]
> ç(\u00E7) => Ç(\u00C7)

You probably meant:
ç(\u00E7) => c(???)

Right?
(In reply to Mounir Lamouri (:mounir) from comment #2)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #1)
> > Characters to be replaced:
> [...]
> > ç(\u00E7) => Ç(\u00C7)
> 
> You probably meant:
> ç(\u00E7) => c(???)
> 
> Right?

Actually, Ç(capital cedilla) is in GSM 7-bit default but ç(lower case cedilla) is not. Beatriz and I will confirm this again.
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #3)
> (In reply to Mounir Lamouri (:mounir) from comment #2)
> > (In reply to Vicamo Yang [:vicamo][:vyang] from comment #1)
> > > Characters to be replaced:
> > [...]
> > > ç(\u00E7) => Ç(\u00C7)
> > 
> > You probably meant:
> > ç(\u00E7) => c(???)
> > 
> > Right?
> 
> Actually, Ç(capital cedilla) is in GSM 7-bit default but ç(lower case
> cedilla) is not. Beatriz and I will confirm this again.

Checked today with the help of vicamo, the suitable change is:
> > > ç(\u00E7) => Ç(\u00C7)
ç(lower case cedilla) is not in GSM 7-bit default Code Scheme.
That sounds really weird. In French, I would prefer to read "garcon" instead of "garÇon"...
Portuguese and catalan people are used to see Ç, they prefer it because it is not a gramatical mistake only a minor mistake.
In Portuguese and Spanish languages, the default code scheme for SMS should be 7 bit. The reason is the number of characters included in a SMS is 160. The latin characters that are not covered with the 7 bit default scheme are replaced as described in comment #1.

Please mark this issue as blocking because it is important for platform customers.
blocking-basecamp: --- → ?
Do we must have a Settings UI for this? Or it's fine to be a product specific, read-only flag? Or a mutable preference setting like "dom.sms.strict7bitEncoding"?
Keywords: feature
Is this going to make feature freeze?  Otherwise I think we'll have to - this.
blocking-basecamp: ? → +
Are you going to take this, Vicamo?
Assignee: nobody → vyang
Whiteboard: [LOE:S]
Attached patch Part 1: WIP (obsolete) — Splinter Review
Attached patch Part 2: test cases WIP (obsolete) — Splinter Review
Attached patch Part 1 (obsolete) — Splinter Review
Attachment #666177 - Attachment is obsolete: true
Attachment #666416 - Flags: review?(marshall)
Attached patch Part 2: test cases (obsolete) — Splinter Review
Attachment #666178 - Attachment is obsolete: true
Attachment #666417 - Flags: review?(marshall)
Attached patch Part1: v2 (obsolete) — Splinter Review
let the map be string to string.
Attachment #666416 - Attachment is obsolete: true
Attachment #666416 - Flags: review?(marshall)
Attachment #666484 - Flags: review?(marshall)
Attached patch Part 2: test cases : v2 (obsolete) — Splinter Review
add test cases to verify the map
Attachment #666417 - Attachment is obsolete: true
Attachment #666417 - Flags: review?(marshall)
Attachment #666485 - Flags: review?(marshall)
Comment on attachment 666484 [details] [diff] [review]
Part1: v2

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

Looks good, r=me with comments addressed.

::: b2g/app/b2g.js
@@ +385,5 @@
>  pref("dom.ipc.browser_frames.oop_by_default", false);
>  
>  // Temporary permission hack for WebSMS
>  pref("dom.sms.enabled", true);
> +//pref("dom.sms.strict7BitEncoding", true); // Disabled by default.

nit: should we uncomment this and set it to false?

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1952,5 @@
>      return options;
>    },
>  
>    getNumberOfMessagesForText: function getNumberOfMessagesForText(text) {
> +    let strict7BitEncoding = Services.prefs.getBoolPref("dom.sms.strict7BitEncoding");

IIRC this will throw an exception if the pref doesn't exist. We should handle that gracefully here (and below in sendSMS)

::: dom/system/gonk/ril_consts.js
@@ +1621,5 @@
> +  map["\u00FA"] = "\u0075"; // ú(\u00FA) => u(\u0075)
> +  map["\u00E7"] = "\u00C7"; // ç(\u00E7) => Ç(\u00C7)
> +
> +  return map;
> +})();

Any reason this can't be a literal object instead of creating and discarding a function scope?
Attachment #666484 - Flags: review?(marshall) → review+
Attachment #666485 - Flags: review?(marshall) → review+
Attached patch Part 1: v3Splinter Review
address review comment #17:
1. Uncomment pref() call and set it to false. This does make it more clear.
2. Wrap call to getBoolPref with try..catch
3. Use literal object instead.

Thank you :)
Attachment #666484 - Attachment is obsolete: true
re-formated in hg
Attachment #666485 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/6f1ea4ac1aa4
https://hg.mozilla.org/mozilla-central/rev/97d7f66e2b30
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.