Last Comment Bug 761069 - B2G MMS: support email addressing model
: B2G MMS: support email addressing model
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- trivial (vote)
: mozilla17
Assigned To: Vicamo Yang [:vicamo][:vyang]
:
Mentors:
Depends on:
Blocks: b2g-mms
  Show dependency treegraph
 
Reported: 2012-06-03 23:28 PDT by Vicamo Yang [:vicamo][:vyang]
Modified: 2012-07-27 10:45 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
v0 (2.79 KB, patch)
2012-07-23 22:54 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
v1 (2.11 KB, patch)
2012-07-24 00:50 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
v2 (2.22 KB, patch)
2012-07-25 04:18 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review

Description Vicamo Yang [:vicamo][:vyang] 2012-06-03 23:28:35 PDT
The MMS Addressing Model[1] lists four possible categories: email, device-address, alphanum-shortcode, and num-shortcode. Current implementation in bug 749856 uses "unknown" for email addresses.
Comment 1 JP Rosevear [:jpr] 2012-07-10 13:11:25 PDT
MMS is desired to be ready for basecamp, but does not actually block.
Comment 2 Michael:mtreese 2012-07-23 16:04:00 PDT
MMS is requred for v1, but not for Brazil
Comment 3 Vicamo Yang [:vicamo][:vyang] 2012-07-23 22:54:44 PDT
Created attachment 645199 [details] [diff] [review]
v0
Comment 4 Vicamo Yang [:vicamo][:vyang] 2012-07-24 00:50:01 PDT
Created attachment 645220 [details] [diff] [review]
v1

break the long long regex down.
Comment 5 Philipp von Weitershausen [:philikon] 2012-07-24 12:21:24 PDT
Comment on attachment 645220 [details] [diff] [review]
v1

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

::: dom/mms/src/ril/MmsPduHelper.jsm
@@ +97,5 @@
> +      // 3.4 of RFC2822, but excluding the obsolete definitions as indicated by
> +      // the "obs-" prefix. Here we match only `addr-spec`.
> +      let dotatom = "[a-zA-Z\d!#$%&'*+\-/=?^_`{|}~]+";
> +      let addrspec = new RegExp(dotatom + "(?:" + dotatom + ")*@" +
> +                                dotatom + "(?:" + dotatom + ")*");

I'm not sure I'm comfortable with validating the email address with such a specific RegExp. I can think of several invalid email addresses that would pass this test. Conversely I bet there are valid addresses that wouldn't. Maybe we can just check for one and only one "@" character in the string, and leave the validation up to the email protocol layer?

(Also, it's unnecessary to compile the RegExp every time we pass through this code. I would suggest a lazy global, if we were to keep it.)
Comment 6 Vicamo Yang [:vicamo][:vyang] 2012-07-25 04:18:12 PDT
Created attachment 645704 [details] [diff] [review]
v2

Match a '@' char instead and maybe come back for more strict/completed checks if necessary. Thanks Philipp.
Comment 7 Vicamo Yang [:vicamo][:vyang] 2012-07-25 19:09:48 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/fec33ba83a18
Comment 8 Ed Morley [:emorley] 2012-07-26 05:08:11 PDT
https://hg.mozilla.org/mozilla-central/rev/fec33ba83a18

Note You need to log in before you can comment on or make changes to this bug.