Bug 674725 (websms)

WebSMS (or Messaging+)

RESOLVED FIXED in mozilla12

Status

()

RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: cjones, Assigned: mounir)

Tracking

({dev-doc-complete})

Trunk
mozilla12
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
sec-review +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [not-fennec-11], URL)

Attachments

(49 attachments, 29 obsolete attachments)

1.09 KB, text/plain
Details
9.80 KB, patch
smaug
: review+
mounir
: checkin+
Details | Diff | Splinter Review
12.99 KB, patch
smaug
: review+
mounir
: checkin+
Details | Diff | Splinter Review
11.94 KB, patch
smaug
: review+
mounir
: checkin+
Details | Diff | Splinter Review
7.28 KB, patch
smaug
: review+
mounir
: checkin+
Details | Diff | Splinter Review
16.49 KB, patch
smaug
: review+
cjones
: superreview+
mounir
: checkin+
Details | Diff | Splinter Review
3.90 KB, patch
smaug
: review+
mounir
: checkin+
Details | Diff | Splinter Review
16.98 KB, patch
smaug
: review+
cjones
: superreview+
mounir
: checkin+
Details | Diff | Splinter Review
16.77 KB, patch
smaug
: review+
cjones
: superreview+
mounir
: checkin+
Details | Diff | Splinter Review
14.30 KB, patch
smaug
: review+
cjones
: review+
mounir
: checkin+
Details | Diff | Splinter Review
14.02 KB, patch
cjones
: review+
mounir
: checkin+
Details | Diff | Splinter Review
12.97 KB, patch
smaug
: review+
mounir
: checkin+
Details | Diff | Splinter Review
14.43 KB, patch
smaug
: review+
mounir
: checkin+
Details | Diff | Splinter Review
16.02 KB, patch
smaug
: review+
mounir
: checkin+
Details | Diff | Splinter Review
12.68 KB, patch
cjones
: review+
blassey
: review-
mounir
: checkin+
Details | Diff | Splinter Review
15.76 KB, patch
cjones
: review+
mounir
: checkin+
Details | Diff | Splinter Review
25.79 KB, patch
smaug
: review+
cjones
: superreview+
Details | Diff | Splinter Review
22.09 KB, patch
cjones
: review+
Details | Diff | Splinter Review
12.49 KB, patch
smaug
: review+
cjones
: review+
Details | Diff | Splinter Review
10.76 KB, patch
smaug
: review+
Details | Diff | Splinter Review
23.90 KB, patch
cjones
: review+
Details | Diff | Splinter Review
10.61 KB, patch
cjones
: review+
Details | Diff | Splinter Review
17.66 KB, patch
smaug
: review+
mrbkap
: review+
Details | Diff | Splinter Review
9.18 KB, patch
smaug
: review+
Details | Diff | Splinter Review
34.26 KB, patch
smaug
: review+
cjones
: review+
Details | Diff | Splinter Review
20.23 KB, patch
smaug
: review+
cjones
: review+
sicking
: superreview+
Details | Diff | Splinter Review
6.53 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
8.64 KB, patch
smaug
: review+
Details | Diff | Splinter Review
16.52 KB, patch
smaug
: review+
cjones
: review+
sicking
: superreview+
Details | Diff | Splinter Review
8.27 KB, patch
smaug
: review+
Details | Diff | Splinter Review
12.17 KB, patch
smaug
: review+
cjones
: review+
sicking
: superreview+
Details | Diff | Splinter Review
4.57 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
4.57 KB, patch
cjones
: review+
Details | Diff | Splinter Review
26.81 KB, patch
cjones
: review+
Details | Diff | Splinter Review
22.05 KB, patch
cjones
: review+
Details | Diff | Splinter Review
23.56 KB, patch
smaug
: review+
mrbkap
: review+
Details | Diff | Splinter Review
12.64 KB, patch
smaug
: review+
Details | Diff | Splinter Review
14.97 KB, patch
cjones
: review+
Details | Diff | Splinter Review
11.84 KB, patch
smaug
: review+
cjones
: review+
Details | Diff | Splinter Review
13.03 KB, patch
smaug
: review+
cjones
: review+
Details | Diff | Splinter Review
13.69 KB, patch
smaug
: review+
Details | Diff | Splinter Review
21.70 KB, patch
cjones
: review+
Details | Diff | Splinter Review
6.04 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.37 KB, patch
sicking
: review+
Details | Diff | Splinter Review
16.81 KB, patch
smaug
: review+
cjones
: review+
Details | Diff | Splinter Review
13.62 KB, patch
smaug
: review+
cjones
: review+
Details | Diff | Splinter Review
13.08 KB, patch
smaug
: review+
Details | Diff | Splinter Review
16.69 KB, patch
cjones
: review+
Details | Diff | Splinter Review
39.65 KB, patch
Details | Diff | Splinter Review
Goals
 - allow content to send SMS messages
 - allow content to receive SMS messages

This overlaps with sms: URIs and the W3C Messaging work (http://www.w3.org/TR/messaging-api/).  Neither is enough because we need to receive SMS's.

Personally, I don't see much use in generalizing from SMS to Messaging (which is so general to be almost meaningless), but it might be worth reviewing the w3c's rationale for doing so.
(Assignee)

Updated

8 years ago
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Comment 1

8 years ago
(Assignee)

Comment 5

8 years ago
Those patches let a web page send a SMS and get informed when a SMS is received.
We still need to add a few functionalities, like (not exhaustive list):
- return a SmsRequest object when |send| is called. This object would receive a 'success' or 'error' event depending on the SMS being sent or not;
- 'smssent' event;
- 'smsdelivered' event;
- create a SmsMessage object that would be used for events;
- API to read current SMS's.
(Assignee)

Updated

8 years ago
Attachment #553412 - Attachment is obsolete: true
(Assignee)

Comment 8

8 years ago
(Assignee)

Comment 9

8 years ago
So, now, when 'smsreceived' event is sent, |event.message| will contain a SmsMessage object with |.sender| being the number who sent the SMS and |.body| being the text message.

Basically, that means, on Android, with the current API, you can simulate the stock app notification using smsreceived and DesktopNotification :)

What's next:
1. Add two other events: smssent and smsdelivered;
2. SmsRequest object returned when |send| is called (maybe suggest?);
3. API to read current SMS's;
x. Finalizing the API's...
Huzzah!  Awesome work :).
(Assignee)

Comment 11

8 years ago
This event is sent when a SMS is sent. If the SMS is a multi-part SMS (more than 160 characters) an event is sent when all parts are actually sent.

The error handling (SMS sending failing) will be handled later. The idea is that smssent is a 'public' event while sending error will only send an event to the SmsRequest object that is going to be returned by send().
(Assignee)

Updated

8 years ago
Depends on: 680881
(Assignee)

Comment 12

8 years ago
After a fight with Android API's, I've been able to find a way to not have a new PendingIntents erasing the previous one when they should not. I really don't understand why the API behave that way: if you create a PendingIntent with an Intent, it will try to find an intent with a common set of properties (like action) in a "pool".

Also, I tried limiting the intent to GeckoSmsManager but it didn't work. Though, we don't care that much.
Attachment #554123 - Attachment is obsolete: true
(Assignee)

Comment 13

8 years ago
(Assignee)

Comment 14

8 years ago
Now, you can do things like:
var r = navigator.mozSms.send("0000000", "message");
r.onSuccess = function() { alert(r.message + " delivered!"); }
r.onError = function() { alert("error :("); }

This patch doesn't implement different error codes.
(Assignee)

Comment 15

8 years ago
(Assignee)

Comment 16

8 years ago
(Assignee)

Comment 17

8 years ago
Posted patch [WIP] Part 12 - delete method (obsolete) — Splinter Review
delete method can take a 'long' representing the message UUID or a SmsMessage object. Taking a long prevent the applications from storing the message objects to delete them. Taking a message object make sure the obvious signature exist.

The method throws when it is failing. I think it's better than using a SmsRequest because no-one might be really interesting in knowing if the method failed (it should not fail, really).

Updated

8 years ago
Keywords: sec-review-needed
Posted file Example page
Example usage of send/recv.  I'm getting a crash on the send notification, investigating now.
Also: this is working great.  Hot!
(Assignee)

Comment 21

8 years ago
(In reply to Chris Jones [:cjones] [:warhammer] from comment #18)
> Created attachment 559360 [details]
> Example page
> 
> Example usage of send/recv.  I'm getting a crash on the send notification,
> investigating now.

Indeed, there is a crash here as I said during the last WebAPI call. My plan was to investigate it after finishing to implement the get method. Though, I guess you might have fixed it. Thanks a lot :)

By the way, these patches might be hard to apply. Did you use my user repo?
If someone else wants to try those patches, my user repo URL is:
https://hg.mozilla.org/users/mlamouri_mozilla.com/webapi-patchqueue/
Here, you will find the updated patches that should apply easily with each other. Though, they might not apply really easily on an updated trunk.
They apply on top of rev 482742e6fff7.  (BTW, when you rebase, it's not a bad idea to note the new base rev in the commit message.)
(Assignee)

Comment 23

8 years ago
(In reply to Chris Jones [:cjones] [:warhammer] from comment #22)
> They apply on top of rev 482742e6fff7.  (BTW, when you rebase, it's not a
> bad idea to note the new base rev in the commit message.)

TBH, I wasn't expecting someone to use my user repo someday :) I will try to do that now.
No worries.  I've found it useful in the past just for myself, even when others weren't using my user repo, when I switch around between different machines or repos.
(Assignee)

Comment 25

8 years ago
This is actually the main reason why I have user repositories for my patches. The fact that anyone can get my work to test it is actually I side effect I'm very happy with :)
Anyway, this is a digression :)
Mounir/Andreas: I'm running into a problem when I run this line

  var r = navigator.mozSms.send(num, msg);

in the "htmlrunner" [1].  I get

  TypeError: navigator.mozSms is undefined

This works in Firefox-on-android in both in- and out-of-process tabs, and for both http:// and file:// URIs.

After fighting with git/hg interaction for a while, I'm pretty sure the rollup patch is applied correctly.  I also tried
 - using a <browser> instead of <iframe> in the htmlrunner XUL
 - using remote=true/false in both (in- and out-of-process)

to no avail.

I set up some logging in nsGlobalWindow.cpp for the navigator and mozSms attrs, and I see

I/GeckoSMS( 2824): nsGlobalWindow::GetNavigator
I/GeckoSMS( 2824): nsGlobalWindow::GetNavigator
I/GeckoSMS( 2824):   createdNavigator
I/GeckoSMS( 2824):   OK!
E/SurfaceFlinger( 2722): GL error 0x0506

that is, we don't even get to GetMozSms() (I guess if that returned an nsnull outparam, that would appear to JS as null?).

I finally got gdb working in (content processes) with b2g (UI process crashes gdb).   I poked around for a bit, but I'm not sure where to be looking.

Any leads?

[1] https://github.com/cgjones/mozilla-central/blob/master/b2g/chrome/content/browser.xul
Fabrice straightened me out wrt comment 26.  Working in b2g now!
Mounir: BTW, I wasn't able to get the SmsRequest callbacks to work with the code in attachment 559360 [details]; they never fired.  I might be doing something wrong, but just a heads up (I copy-n-pasted from comment 14 ;).
(Assignee)

Updated

8 years ago
No longer depends on: 680881

Updated

8 years ago
Keywords: dev-doc-needed
Mounir, what work is needed before we can land these patches on m-c?
(Assignee)

Comment 30

8 years ago
(In reply to Chris Jones [:cjones] [:warhammer] from comment #29)
> Mounir, what work is needed before we can land these patches on m-c?

The patches by themselves should be cleaned, fixed (there are still some crashes) and some new features should be added (like SmsMessage.unread). Nothing major.
However, we need some tests and we need to figure out how we can handle that for the Android code.
need full review
Whiteboard: [secr:curtisk]
One issue I remember coming up is whether Gecko should explicitly save text messages into the system database on android.  I think on android, the right thing to do is let the OS save the messages.  We'll want a followup bug where we implement this for gonk, and there we'll want two parts: a low-level layer to send and receive RIL messages related to SMS, and another piece that stores the messages in an IndexedDB, probably.  (Later to be Sync'd, maybe.)
(Assignee)

Comment 33

8 years ago
This is actually a tricky issue. On Android, there is an intent broadcasted when a message is received. It seems that the stock sms app uses this intent to save the sms in its internal DB. If we use this intent to read the saved values. we are facing a race condition. If we wait for x milliseconds before doing so, we are still facing a race condition. I will try to see if there is a simple way to fix that (like having a callback when the DB is changed). Note that we could alternatively not try to look at the saved message but we would miss some information (or have wrong ones) like id or timestamp.
In another hand, I don't believe we really want to have WebSMS enabled in a shipped browser for the moment because it would require some privileges that might freak out users. We should probably discuss that with the mobile team. With that in mind, we could just not read the DB and have wrong information when receiving a message on Android.
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #33)
> This is actually a tricky issue. On Android, there is an intent broadcasted
> when a message is received. It seems that the stock sms app uses this intent
> to save the sms in its internal DB. If we use this intent to read the saved
> values. we are facing a race condition. If we wait for x milliseconds before
> doing so, we are still facing a race condition. I will try to see if there
> is a simple way to fix that (like having a callback when the DB is changed).
> Note that we could alternatively not try to look at the saved message but we
> would miss some information (or have wrong ones) like id or timestamp.

At which point do sent and received SMS's get assigned IDs?  Is it when they're saved in the OS database, or sometime before?

I wonder if we could temporarily keep around the messages in a "limbo" until we see them come back from a DB query, and automatically add the limbo messages to the DB query results returned to script until we see them from the system DB.  That would require being able to uniquely identify the sent/received messages though.
(Assignee)

Comment 35

8 years ago
(In reply to Chris Jones [:cjones] [:warhammer] from comment #34)
> At which point do sent and received SMS's get assigned IDs?  Is it when
> they're saved in the OS database, or sometime before?

We are using the id in the database.

> I wonder if we could temporarily keep around the messages in a "limbo" until
> we see them come back from a DB query, and automatically add the limbo
> messages to the DB query results returned to script until we see them from
> the system DB.  That would require being able to uniquely identify the
> sent/received messages though.

Unfortunately, we can't do that with a 100% certainty that we will got the correct message because we only have the sender and the body that are sure information and we can't guarantee that someone didn't sent you twice the same message. We can try to put a lower range for the date but again, we can't guarantee that someone didn't sent you twice the same message in a very short interval of time.
Again, given that we might never ship WebSMS for Android, something that works 99% of the time on Android would be fine by me.

Comment 36

8 years ago
What is blocking this from landing? Can we please land this Monday?
(Assignee)

Comment 37

8 years ago
(In reply to Andreas Gal :gal from comment #36)
> What is blocking this from landing?

I've been working on Battery API but most of it has landed now and the last patches are waiting for reviews. I was trying to get Battery API done for Firefox 10. I did what I had to, now WebSMS is my top priority.

> Can we please land this Monday?

I'm now working on WebSMS and trying to use a better (and final) code design. Patches will come soon. Do you want those WIP patches to land ASAP or do you prefer to wait for the new patches? I don't know how long it's gonna take though. I would have say around a week but with MozCamp Europe coming (and a few days off next week), it might take longer.
Also, note that landing the WIP patches is going to require some time: re-basing them to tip and put a big --enable-websms around them. Ideally, I would prefer not to do that given that it would last in the tree for a very short period of time.

Comment 38

8 years ago
Aright let's wait for your new patch but let's really drive that in. We are critically dependent on this.
(Assignee)

Comment 39

8 years ago
Assignee: nobody → mounir
Status: NEW → ASSIGNED
Attachment #572869 - Flags: review?(jonas)
(Assignee)

Comment 40

8 years ago
Attachment #572870 - Flags: review?(jonas)
(Assignee)

Comment 41

8 years ago
Attachment #572871 - Flags: review?(jonas)
(Assignee)

Comment 42

8 years ago
As said in the code comments, the *temporary* security model is going to have two parts:
1. dom.sms.enabled preference that is going to be set to false by default. This is going to stay even when we will have a final security model to simply disable the feature.
2. dom.sms.whitelist preference that is going to be set to the empty string by default. This is a comma separated list of prepath (mostly hostnames) that are allowed to use the WebSMS api.
Attachment #572872 - Flags: review?(jonas)
(Assignee)

Comment 43

8 years ago
Attachment #572873 - Flags: superreview?(jones.chris.g)
(Assignee)

Comment 45

8 years ago
Attachment #572876 - Flags: superreview?(jones.chris.g)
(Assignee)

Comment 46

8 years ago
Jonas, I'm asking a sr here because our draft spec say we should return a unsigned short but I wonder if we should really care about the type and simply return an unsigned long like most specifications seem to do.
Attachment #572881 - Flags: superreview?(jonas)
Attachment #572881 - Flags: review?(jones.chris.g)
Attachment #572873 - Flags: superreview?(jones.chris.g) → superreview+
Comment on attachment 572876 [details] [diff] [review]
Part G - Add PSms IPDL subprotocol

You can combine the *Child/Parent.h headers into a single RemoteSms.h or something, if you don't really need to include them separately.  But I'll leave that to your taste.

Nit: we've been trying to avoid namespaces deeper than 2 elements ("mozilla::foo"), so unless you have a specific requirement for a mozilla::dom::sms namespace, please stick with mozilla::dom.
Attachment #572876 - Flags: superreview?(jones.chris.g) → superreview+
(In reply to Chris Jones [:cjones] [:warhammer] from comment #47)
> Nit: we've been trying to avoid namespaces deeper than 2 elements
> ("mozilla::foo"), so unless you have a specific requirement for a
> mozilla::dom::sms namespace, please stick with mozilla::dom.

workers, indexeddb, and telephony all are subnamespaces of mozilla::dom.
Case in point :).  What did all those extra characters buy you?
Comment on attachment 572881 [details] [diff] [review]
Part I - Implement mozSms.getNumberOfMessagesForText

>diff --git a/dom/sms/src/android/SmsService.cpp b/dom/sms/src/android/SmsService.cpp

>+/* static */ PSmsChild*
>+SmsService::GetSmsChild()
>+{
>+  if (!sSmsChild) {
>+    sSmsChild = ContentChild::GetSingleton()->SendPSmsConstructor();
>+  }
>+  return sSmsChild;
>+}
>+

The IPC implementation shouldn't be in android-specific code.  We're
going to need to share this among all platforms with SMS capability.
Instead, as painful as it may be, I think you'll want to have an "IPC
SMS service" that's created magically in content processes when you
GetService().

> parent:
>+    sync GetNumberOfMessagesForText(nsString aText)
>+        returns (PRUint16 aNumber);

This is a question of the DOM API, but shouldn't this be asynchronous?
I would have thought this required a DB query, so disk IO.
Attachment #572881 - Flags: review?(jones.chris.g)
(In reply to Chris Jones [:cjones] [:warhammer] from comment #50)
> The IPC implementation shouldn't be in android-specific code.  We're
> going to need to share this among all platforms with SMS capability.
> Instead, as painful as it may be, I think you'll want to have an "IPC
> SMS service" that's created magically in content processes when you
> GetService().

Moreover, we want to have a native implementation for gonk, right? So it might make more sense to implement this as an XPCOM service and make navigator.mozSms defer to that. AFAICT from patch B and C, this does not allow for a JS implementation right now.

(There's a lot of patches here... It seems like some of the WIPs can be obsoleted, no?)

> > parent:
> >+    sync GetNumberOfMessagesForText(nsString aText)
> >+        returns (PRUint16 aNumber);
> 
> This is a question of the DOM API, but shouldn't this be asynchronous?
> I would have thought this required a DB query, so disk IO.

Yes, from an implementor's point of view, this should be async. In fact, I can't think of a single method revolving around SMS that shouldn't be async.


On a general note, where's the navigator.sms API being spec'ed out? The request model suggested by cjones's example page is akin to IndexedDB, but both the W3C's and WAC's API drafts seem to be going with directly passing in success and failure callbacks.
(In reply to Philipp von Weitershausen [:philikon] from comment #51)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #50)
> > The IPC implementation shouldn't be in android-specific code.  We're
> > going to need to share this among all platforms with SMS capability.
> > Instead, as painful as it may be, I think you'll want to have an "IPC
> > SMS service" that's created magically in content processes when you
> > GetService().
> 
> Moreover, we want to have a native implementation for gonk, right? So it
> might make more sense to implement this as an XPCOM service and make
> navigator.mozSms defer to that. AFAICT from patch B and C, this does not
> allow for a JS implementation right now.
> 

Yep, that's the idea.  B and C add an nsISmsManager that uses an nsISmsService, of which will have multiple implementations (including the native-JS one).
(Assignee)

Comment 53

8 years ago
(In reply to Chris Jones [:cjones] [:warhammer] from comment #50)
> > parent:
> >+    sync GetNumberOfMessagesForText(nsString aText)
> >+        returns (PRUint16 aNumber);
> 
> This is a question of the DOM API, but shouldn't this be asynchronous?
> I would have thought this required a DB query, so disk IO.

There is no DB query here. The method returns how many messages would be needed to send a specific text. That's likely something like: |upper(aText.length() / 160)| but given that it's unlikely exactly that, I've decided to use the Android implementation. We could directly implement this in SmsManager later (when we will have a b2g-specific backend).

(In reply to Philipp von Weitershausen [:philikon] from comment #51)
> Moreover, we want to have a native implementation for gonk, right? So it
> might make more sense to implement this as an XPCOM service and make
> navigator.mozSms defer to that. AFAICT from patch B and C, this does not
> allow for a JS implementation right now.

The idea is to have the b2g implementation implementing SmsService. SmsManager should be left as is because it will handle common stuff.

> (There's a lot of patches here... It seems like some of the WIPs can be
> obsoleted, no?)

Yes, will do that.

> On a general note, where's the navigator.sms API being spec'ed out? The
> request model suggested by cjones's example page is akin to IndexedDB, but
> both the W3C's and WAC's API drafts seem to be going with directly passing
> in success and failure callbacks.

It's in the bug URL now. We are using an API similar to IndexedDB but getNumberOfMessagesForText doesn't have to be async.
(Assignee)

Comment 54

8 years ago
This should be doing what you were asking. The only disadvantages is that asking for support is now an IPC call. It's the cleanest way to do that without #ifdef I think. I would tend to say it's no big deal but if someone has a simple and clean idea, I'm listening.
Attachment #572881 - Attachment is obsolete: true
Attachment #572881 - Flags: superreview?(jonas)
Attachment #573225 - Flags: superreview?(jones.chris.g)
(Assignee)

Updated

8 years ago
Attachment #573225 - Attachment description: Part I - Add a IPC service to handle IPC calls → Part H - Add a IPC service to handle IPC calls
(Assignee)

Comment 55

8 years ago
Attachment #553408 - Attachment is obsolete: true
Attachment #553409 - Attachment is obsolete: true
Attachment #553410 - Attachment is obsolete: true
Attachment #553538 - Attachment is obsolete: true
Attachment #553539 - Attachment is obsolete: true
Attachment #553540 - Attachment is obsolete: true
Attachment #554871 - Attachment is obsolete: true
Attachment #554902 - Attachment is obsolete: true
Attachment #555549 - Attachment is obsolete: true
Attachment #556028 - Attachment is obsolete: true
Attachment #556842 - Attachment is obsolete: true
Attachment #556887 - Attachment is obsolete: true
Attachment #559389 - Attachment is obsolete: true
Attachment #573232 - Flags: review?(jonas)
(Assignee)

Comment 56

8 years ago
If you are interested in trying the prototype/poc/draft implementation, have a look at this patch queue: https://hg.mozilla.org/users/mlamouri_mozilla.com/webapi-patchqueue/
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #53)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #50)
> > > parent:
> > >+    sync GetNumberOfMessagesForText(nsString aText)
> > >+        returns (PRUint16 aNumber);
> > 
> > This is a question of the DOM API, but shouldn't this be asynchronous?
> > I would have thought this required a DB query, so disk IO.
> 
> There is no DB query here. The method returns how many messages would be
> needed to send a specific text. That's likely something like:
> |upper(aText.length() / 160)| but given that it's unlikely exactly that,
> I've decided to use the Android implementation. We could directly implement
> this in SmsManager later (when we will have a b2g-specific backend).
> 

Yes, that might be better.  It's OK for now.
Comment on attachment 573225 [details] [diff] [review]
Part H - Add a IPC service to handle IPC calls

Looks good.  Same comment applies as before: you can group more of the files needed for the cross-process support into fewer, but if you're more comfortable with multiple separate files, OK.
Attachment #573225 - Flags: superreview?(jones.chris.g) → superreview+
(Assignee)

Comment 59

8 years ago
(In reply to Chris Jones [:cjones] [:warhammer] from comment #58)
> Comment on attachment 573225 [details] [diff] [review] [diff] [details] [review]
> Part H - Add a IPC service to handle IPC calls
> 
> Looks good.  Same comment applies as before: you can group more of the files
> needed for the cross-process support into fewer, but if you're more
> comfortable with multiple separate files, OK.

I don't really like having one header declaring two classes and two cpp files for the implementation. I also prefer to avoid having one header and one cpp file for two classes. And I hate big files :)
(Assignee)

Comment 60

8 years ago
Chris, do you think someone should review the files you superreviewed?
They're simple enough boilerplate that I'm comfortable with just my sr.  But I'm not a DOM peer, so you might want to get a second opinion from one.
(Assignee)

Updated

8 years ago
Attachment #572873 - Flags: review?(jonas)
(Assignee)

Updated

8 years ago
Attachment #572876 - Flags: review?(jonas)
(Assignee)

Updated

8 years ago
Attachment #573225 - Flags: review?(jonas)
(Assignee)

Comment 62

8 years ago
Attachment #573232 - Attachment is obsolete: true
Attachment #573232 - Flags: review?(jonas)
Attachment #573294 - Flags: review?(jonas)
(Assignee)

Comment 63

8 years ago
Attachment #573306 - Flags: review?(jones.chris.g)
Comment on attachment 573306 [details] [diff] [review]
Part J - Basic implementation of mozSms.send()

>diff --git a/embedding/android/GeckoSmsManager.java b/embedding/android/GeckoSmsManager.java

>+  public static void send(String aNumber, String aMessage) {
>+    /*
>+     * TODO:
>+     * This is a basic send method that doesn't handle errors, doesn't listen to
>+     * sent and received messages. It's only calling the sent method.

"send"
Attachment #573306 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 65

8 years ago
I did remove SmsMessage.unread from our proposal because it makes SmsMessage dynamic [1] which is making the implementation way harder. We will see in a second pass if we want to implement that.

[1] if two apps get an SmsMessage with unread = false, one sets it to true, the other one should get an update.
Attachment #573608 - Flags: review?(bugs)
(Assignee)

Comment 66

8 years ago
This is not implementing the js ctor. It's probably not really hard to add that but it will affect all events AFAIUI so let's do that in a follow-up.
Attachment #573609 - Flags: review?(bugs)
(Assignee)

Comment 67

8 years ago
... I hate having so many patches in the same bug :(
Attachment #573610 - Flags: review?(bugs)
(Assignee)

Comment 68

8 years ago
Attachment #573612 - Flags: review?(jones.chris.g)
Comment on attachment 573609 [details] [diff] [review]
Part L - Implement SmsEvent

>@@ -899,11 +899,13 @@ nsEventDispatcher::CreateEvent(nsPresCon
>     return NS_NewDOMCloseEvent(aDOMEvent, aPresContext, nsnull);
>   if (aEventType.LowerCaseEqualsLiteral("touchevent") &&
>       nsDOMTouchEvent::PrefEnabled())
>     return NS_NewDOMTouchEvent(aDOMEvent, aPresContext, nsnull);
>   if (aEventType.LowerCaseEqualsLiteral("hashchangeevent"))
>     return NS_NewDOMHashChangeEvent(aDOMEvent, aPresContext, nsnull);
>   if (aEventType.LowerCaseEqualsLiteral("customevent"))
>     return NS_NewDOMCustomEvent(aDOMEvent, aPresContext, nsnull);
>+  if (aEventType.LowerCaseEqualsLiteral("smsevent"))
>+    return NS_NewDOMSmsEvent(aDOMEvent, aPresContext, nsnull);
mozsmsevent


>+[scriptable, uuid(34dda4c3-4683-4323-9ee3-2a7bfef7df3b)]
>+interface nsIDOMSmsEvent : nsIDOMEvent
nsIDOMMozSmsEvent or something like that


>+class SmsEvent : public nsIDOMSmsEvent
>+               , public nsDOMEvent
, should be in the previous line, IMO.
Attachment #573609 - Flags: review?(bugs) → review+
Comment on attachment 573608 [details] [diff] [review]
Part K - Implement SmsMessage


>+[scriptable, function, uuid(0a0037ba-585e-41f4-b0a5-1d0224353105)]
>+interface nsIDOMSmsMessage : nsISupports
nsIDOMMozSmsMessage or some such


>+#include "jsdate.h" // For js_NewDateObjectMsec
have you asked JS people if this is ok?
Attachment #573608 - Flags: review?(bugs) → review+
Comment on attachment 573612 [details] [diff] [review]
Part M - Receiving SMS, IPC handling

>diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp

> PSmsParent*
> ContentParent::AllocPSms()
> {
>-    return new SmsParent();
>+    SmsParent* smsParent = new SmsParent();
>+    smsParent->Init();

Since the Init() method is infallible, is there any point in having it separate from the ctor?

>diff --git a/dom/sms/src/SmsMessage.cpp b/dom/sms/src/SmsMessage.cpp

>+IPCSmsMessage
>+SmsMessage::ToIPCSmsMessage() const
>+{
>+  return IPCSmsMessage(mId, mDelivery == eDeliveryState_Received, mSender,
>+                       mReceiver, mBody, mTimestamp);
>+}

Instead of duplicating the definition of the IPDL struct for
SmsMessage, it probably makes sense for SmsMessage to be a simple
XPCOM wrapper around the struct defined in IPDL, plus whatever
additional non-data state it needs.  That keeps the definition in one
place.

>diff --git a/dom/sms/src/ipc/PSms.ipdl b/dom/sms/src/ipc/PSms.ipdl
>--- a/dom/sms/src/ipc/PSms.ipdl
>+++ b/dom/sms/src/ipc/PSms.ipdl
>@@ -38,19 +38,31 @@
>  * ***** END LICENSE BLOCK ***** */
> 
> include protocol PContent;
> 
> namespace mozilla {
> namespace dom {
> namespace sms {
> 
>+struct IPCSmsMessage {

I don't like this name but I understand the conflict with SmsMessage.
Maybe SmsMessageData?

>diff --git a/dom/sms/src/ipc/SmsChild.cpp b/dom/sms/src/ipc/SmsChild.cpp

>+bool
>+SmsChild::RecvNotifyReceivedMessage(const IPCSmsMessage& aMessage)
>+{
>+  nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();

|using namespace services;| or |services::GetObserverService()| for
consistency with other uses, plz.

>diff --git a/dom/sms/src/ipc/SmsParent.cpp b/dom/sms/src/ipc/SmsParent.cpp
>--- a/dom/sms/src/ipc/SmsParent.cpp
>+++ b/dom/sms/src/ipc/SmsParent.cpp
>@@ -32,21 +32,69 @@
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "SmsParent.h"
> #include "nsISmsService.h"
>+#include "nsIObserverService.h"
>+#include "mozilla/Services.h"
>+#include "Constants.h"
>+#include "nsIDOMSmsMessage.h"
>+#include "mozilla/unused.h"
>+#include "SmsMessage.h"
> 
> namespace mozilla {
> namespace dom {
> namespace sms {
> 
>+NS_IMPL_THREADSAFE_ISUPPORTS1(SmsParent, nsIObserver)
>+

Why does this need to be thread safe?  That's pretty dangerous.
Actors can only be used on thread they're created on.
Attachment #573612 - Flags: review?(jones.chris.g)
Comment on attachment 573610 [details] [diff] [review]
Part M - Receiving SMS, dom code

>+[scriptable, function, uuid(807d593c-09cb-4aa3-afa5-aa0a671bd0d3)]
>+interface nsIDOMSmsManager : nsIDOMEventTarget
This should be nsIDOMMoz* or something similar

>+void
>+SmsManager::Shutdown()
...
>+  // Those vars come from nsDOMEventTargetHelper.
>+  mOwner = nsnull;
>+  mScriptContext = nsnull;
Why these? After that event handling could behave unexpectedly
Attachment #573610 - Flags: review?(bugs) → review+
(Assignee)

Comment 73

8 years ago
I tried to find a simple way to know the current phone number but the Android SDK method doing that is a SIM card field that isn't always filed by operator (mine being one of those). There are other ways to find that number but they are quite complex so I will just ignore that field for the moment.
I'm also not trying to find the real id of the message. It's not needed for a simple implementation and usage. I will do that in a follow-up I think (for the reasons given in the patch).
Attachment #573682 - Flags: review?(jones.chris.g)
Comment on attachment 573682 [details] [diff] [review]
Part O - Receiving SMS, Android backend

>diff --git a/embedding/android/GeckoApp.java b/embedding/android/GeckoApp.java

>+        IntentFilter smsFilter = new IntentFilter();
>+        smsFilter.addAction("android.provider.Telephony.SMS_RECEIVED");
>+        mSmsReceiver = new GeckoSmsManager();
>+        registerReceiver(mSmsReceiver, smsFilter);
>+

Is there a reason you don't want to register/unregister based on
when there are SMS listeners?  I would prefer gecko not see all
my SMSes until something I explicitly gave permission to
registers to listen for them.

>diff --git a/widget/src/android/AndroidJNI.cpp b/widget/src/android/AndroidJNI.cpp

>+NS_EXPORT void JNICALL
>+Java_org_mozilla_gecko_GeckoAppShell_notifySmsReceived(JNIEnv* jenv, jclass,
>+                                                       jstring aSender,
>+                                                       jstring aBody,
>+                                                       jlong aTimestamp)
>+{
>+    if (!nsAppShell::gAppShell) {
>+        return;
>+    }
>+
>+    nsCOMPtr<nsIDOMSmsMessage> message =
>+      new SmsMessage(0, SmsMessage::eDeliveryState_Received, nsJNIString(aSender, jenv),
>+                     EmptyString(), nsJNIString(aBody, jenv), aTimestamp);
>+
>+    nsAppShell::gAppShell->NotifyObservers(message, kSmsReceivedObserverTopic, nsnull);
>+}

Making nsIDOMSmsMessage thread safe makes me rather
uncomfortable.  AFAICT, you're doing it so that you can use this
NotifyObservers() helper here and save a few lines of code.  I'd
rather not open that can of worms.  I phoned a friend (gal) and
he didn't like it either.  Instead, let's use the SmsMessageData
struct in PSms here: post a task to the gecko thread that
includes the received SmsMessageData, and then NotifyObservers()
from there.  You can remove the NS_IMPL_THREADSAFE on SmsMessage
then.
Attachment #573682 - Flags: review?(jones.chris.g) → review-
(Assignee)

Updated

7 years ago
Attachment #572869 - Flags: review?(jonas) → review?(bugs)
(Assignee)

Updated

7 years ago
Attachment #572870 - Flags: review?(jonas) → review?(bugs)
(Assignee)

Updated

7 years ago
Attachment #572871 - Flags: review?(jonas) → review?(bugs)
(Assignee)

Updated

7 years ago
Attachment #572872 - Flags: review?(jonas) → review?(bugs)
(Assignee)

Updated

7 years ago
Attachment #572873 - Flags: review?(jonas) → review?(bugs)
(Assignee)

Updated

7 years ago
Attachment #572874 - Flags: review?(jonas) → review?(bugs)
(Assignee)

Updated

7 years ago
Attachment #572876 - Flags: review?(jonas) → review?(bugs)
(Assignee)

Updated

7 years ago
Attachment #573225 - Flags: review?(jonas) → review?(bugs)
(Assignee)

Updated

7 years ago
Attachment #573294 - Flags: review?(jonas) → review?(bugs)
(Assignee)

Comment 75

7 years ago
(In reply to Chris Jones [:cjones] [:warhammer] from comment #71)
> > PSmsParent*
> > ContentParent::AllocPSms()
> > {
> >-    return new SmsParent();
> >+    SmsParent* smsParent = new SmsParent();
> >+    smsParent->Init();
> 
> Since the Init() method is infallible, is there any point in having it
> separate from the ctor?

I prefer to have Init/Shutdown couples instead of ctor/Shutdown so I prefer to keep the Init method.

> >diff --git a/dom/sms/src/SmsMessage.cpp b/dom/sms/src/SmsMessage.cpp
> 
> >+IPCSmsMessage
> >+SmsMessage::ToIPCSmsMessage() const
> >+{
> >+  return IPCSmsMessage(mId, mDelivery == eDeliveryState_Received, mSender,
> >+                       mReceiver, mBody, mTimestamp);
> >+}
> 
> Instead of duplicating the definition of the IPDL struct for
> SmsMessage, it probably makes sense for SmsMessage to be a simple
> XPCOM wrapper around the struct defined in IPDL, plus whatever
> additional non-data state it needs.  That keeps the definition in one
> place.

I agree that keeping the definition in one place would be better but I really don't like keeping the definition in PSms.ipdl. The only reason to do that is to prevent writing the IPC struct reading/writing code and have it auto-generated. I agree it's practically a good reason but it seems to be a bad design IMO.
I'm using this IPCSmsMessage as a compromise between writing the code to pass objects between processes and having SmsMessage data owned by PSMs.ipdl.
(Assignee)

Comment 76

7 years ago
(In reply to Chris Jones [:cjones] [:warhammer] from comment #74)
> >diff --git a/embedding/android/GeckoApp.java b/embedding/android/GeckoApp.java
> 
> >+        IntentFilter smsFilter = new IntentFilter();
> >+        smsFilter.addAction("android.provider.Telephony.SMS_RECEIVED");
> >+        mSmsReceiver = new GeckoSmsManager();
> >+        registerReceiver(mSmsReceiver, smsFilter);
> >+
> 
> Is there a reason you don't want to register/unregister based on
> when there are SMS listeners?  I would prefer gecko not see all
> my SMSes until something I explicitly gave permission to
> registers to listen for them.

In my opinion, doing that is going to make the code more complex: we are going to add addObserver and removeObserver methods and we will have to count observers to know when listening to SMS is needed. Perf-wise, doing that is not useful, and, on a B2G device (it's yet not sure this will be enabled in official Firefox Mobile release), there will be no reason to not listen to SMS given that we can assume there will be always one SMS observer.
In addition, permissions are asked at the installation time on Android so users will not see any difference between Gecko always listening or listening when needed.
I think we could consider this as a follow-up if it happens that the pros out-weight the cons but for the moment, that seems to be the opposite.

> Making nsIDOMSmsMessage thread safe makes me rather
> uncomfortable.  AFAICT, you're doing it so that you can use this
> NotifyObservers() helper here and save a few lines of code.  I'd
> rather not open that can of worms.  I phoned a friend (gal) and
> he didn't like it either.  Instead, let's use the SmsMessageData
> struct in PSms here: post a task to the gecko thread that
> includes the received SmsMessageData, and then NotifyObservers()
> from there.  You can remove the NS_IMPL_THREADSAFE on SmsMessage
> then.

I asked Olli and he was comfortable with that. However, I don't really mind so I changed it to non-thread safe.
Attachment #572869 - Flags: review?(bugs) → review+
Comment on attachment 572870 [details] [diff] [review]
Part B - Add .mozSms to navigator


>+//*****************************************************************************
>+//    Navigator::nsIDOMNavigatorSms
>+//*****************************************************************************
>+
>+NS_IMETHODIMP
>+Navigator::GetMozSms(nsIDOMSmsManager** aSmsManager)
>+{

Should be nsIDOMMozSmsManager


>+++ b/dom/base/nsDOMClassInfo.cpp
>@@ -2288,16 +2288,17 @@ nsDOMClassInfo::Init()
>   DOM_CLASSINFO_MAP_BEGIN(Navigator, nsIDOMNavigator)
>     DOM_CLASSINFO_MAP_ENTRY(nsIDOMNavigator)
>     DOM_CLASSINFO_MAP_ENTRY(nsIDOMNavigatorGeolocation)
>     DOM_CLASSINFO_MAP_CONDITIONAL_ENTRY(nsIDOMNavigatorDesktopNotification,
>                                         Navigator::HasDesktopNotificationSupport())
>     DOM_CLASSINFO_MAP_ENTRY(nsIDOMClientInformation)
>     DOM_CLASSINFO_MAP_CONDITIONAL_ENTRY(nsIDOMNavigatorBattery,
>                                         battery::BatteryManager::HasSupport())
>+    DOM_CLASSINFO_MAP_ENTRY(nsIDOMNavigatorSms)
conditional? Or maybe that is added in some followup patch.

>+[scriptable, function, uuid(562eade6-6dee-4c0a-bee9-bfc7f2bcadbe)]
>+interface nsIDOMSmsManager : nsISupports
nsIDOMMozSmsManager


r+ with Moz prefix and assuming conditional is added in some patch.
Attachment #572870 - Flags: review?(bugs) → review+
Comment on attachment 572871 [details] [diff] [review]
Part C - SmsManager stub

You really like splitting patches to tiny parts.
Attachment #572871 - Flags: review?(bugs) → review+
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #75)
> > >diff --git a/dom/sms/src/SmsMessage.cpp b/dom/sms/src/SmsMessage.cpp
> > 
> > >+IPCSmsMessage
> > >+SmsMessage::ToIPCSmsMessage() const
> > >+{
> > >+  return IPCSmsMessage(mId, mDelivery == eDeliveryState_Received, mSender,
> > >+                       mReceiver, mBody, mTimestamp);
> > >+}
> > 
> > Instead of duplicating the definition of the IPDL struct for
> > SmsMessage, it probably makes sense for SmsMessage to be a simple
> > XPCOM wrapper around the struct defined in IPDL, plus whatever
> > additional non-data state it needs.  That keeps the definition in one
> > place.
> 
> I agree that keeping the definition in one place would be better but I
> really don't like keeping the definition in PSms.ipdl. The only reason to do
> that is to prevent writing the IPC struct reading/writing code and have it
> auto-generated. I agree it's practically a good reason but it seems to be a
> bad design IMO.

I don't quite follow this.  Defining the structure in multiple places is a maintenance hell.  I feel strongly about that; please don't do it.  (Imagine what it would take to add a new field ...)  The .ipdl happens to be the best place to do this because you need to send the sms info in IPC messages, and your definition will be auto-serialized.  That's convenient, guarantees optimal performance, and avoids security issues.  IPDL's datatypes are also more powerful than what can sanely be written in C++.

> I'm using this IPCSmsMessage as a compromise between writing the code to
> pass objects between processes and having SmsMessage data owned by PSMs.ipdl.

I don't understand what you're compromising between.  nsSmsMessage is in dom/sms, right, just like PSms?
Comment on attachment 572872 [details] [diff] [review]
Part D - Temporary security model


>+bool
>+Navigator::IsSmsAllowed() const
>+{
>+  static const bool defaultSmsPermission = false;
>+
>+  // First of all, the general pref has to be turned on.
>+  if (!Preferences::GetBool("dom.sms.enabled", defaultSmsPermission)) {
>+    return false;
>+  }
Can you pass const bool to GetBool? That is a bit surprising.
Attachment #572872 - Flags: review?(bugs) → review+
Attachment #572873 - Flags: review?(bugs) → review+
Attachment #572874 - Flags: review?(bugs) → review+
Comment on attachment 572876 [details] [diff] [review]
Part G - Add PSms IPDL subprotocol

Drop NS_OVERRIDE.
Attachment #572876 - Flags: review?(bugs) → review+
Attachment #573225 - Flags: review?(bugs) → review+
Comment on attachment 573294 [details] [diff] [review]
Part I - Implement mozSms.getNumberOfMessagesForText


>+SmsParent::RecvGetNumberOfMessagesForText(const nsString& aText, PRUint16* aResult)
>+{
>+  nsCOMPtr<nsISmsService> smsService = do_GetService(SMSSERVICE_CONTRACTID);
>+  NS_ENSURE_TRUE(smsService, true);
Could you set *aResult = 0; before getting anything from the service.


>+public class GeckoSmsManager
>+{
>+  public static int getNumberOfMessagesForText(String aText) {
>+    return SmsManager.getDefault().divideMessage(aText).size();
>+  }
>+}
I don't know the coding style for our Java code.
In C++ { would be in the next line.


>+AndroidBridge::GetNumberOfMessagesForText(const nsAString& aText)
>+{
>+    ALOG_BRIDGE("AndroidBridge::GetNumberOfMessagesForText");
>+
>+    AutoLocalJNIFrame jniFrame;
>+    jstring jText = GetJNIForThread()->NewString(PromiseFlatString(aText).get(), aText.Length());
>+    return GetJNIForThread()->CallStaticIntMethod(mGeckoAppShellClass, jNumberOfMessages, jText);
>+}
>+
I have no idea about JNI. Someone else should review this.
Attachment #573294 - Flags: review?(bugs) → review+
(Assignee)

Comment 83

7 years ago
Comment on attachment 573294 [details] [diff] [review]
Part I - Implement mozSms.getNumberOfMessagesForText

Chris, could you review the Java/Android parts?
Attachment #573294 - Flags: review?(jones.chris.g)
(Assignee)

Comment 84

7 years ago
(In reply to Olli Pettay [:smaug] from comment #77)
> Comment on attachment 572870 [details] [diff] [review] [diff] [details] [review]
> Part B - Add .mozSms to navigator
> [...]
> r+ with Moz prefix and assuming conditional is added in some patch.

If the pref is off or the site not whitelisted, mozSms returns null. I could use the dom.sms.enabled pref to conditionally add nsIDOMMozNavigatorSms to the Navigator object but it wouldn't be that much helpful in my opinion and would split the security model in different places. If you think we should change that, please open a follow-up.

(In reply to Olli Pettay [:smaug] from comment #80)
> Comment on attachment 572872 [details] [diff] [review] [diff] [details] [review]
> Part D - Temporary security model
> 
> 
> >+bool
> >+Navigator::IsSmsAllowed() const
> >+{
> >+  static const bool defaultSmsPermission = false;
> >+
> >+  // First of all, the general pref has to be turned on.
> >+  if (!Preferences::GetBool("dom.sms.enabled", defaultSmsPermission)) {
> >+    return false;
> >+  }
> Can you pass const bool to GetBool? That is a bit surprising.

The second parameter is the value used as the default value if the pref isn't present. It's not an out-param.

(In reply to Olli Pettay [:smaug] from comment #70)
> Comment on attachment 573608 [details] [diff] [review] [diff] [details] [review]
> Part K - Implement SmsMessage
> [...]
> >+#include "jsdate.h" // For js_NewDateObjectMsec
> have you asked JS people if this is ok?

Why wouldn't that be okay? If that what you asked, I can always ask for a review.
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #84)
> If the pref is off or the site not whitelisted, mozSms returns null.

Ah, right. that is ok.

> 
> The second parameter is the value used as the default value if the pref
> isn't present. It's not an out-param.
indeed. 


> > >+#include "jsdate.h" // For js_NewDateObjectMsec
> > have you asked JS people if this is ok?
> 
> Why wouldn't that be okay? If that what you asked, I can always ask for a
> review.
I thought only jsapi.h should be used when just possible.
(Assignee)

Comment 86

7 years ago
(In reply to Chris Jones [:cjones] [:warhammer] from comment #79)
> > I'm using this IPCSmsMessage as a compromise between writing the code to
> > pass objects between processes and having SmsMessage data owned by PSMs.ipdl.
> 
> I don't understand what you're compromising between.  nsSmsMessage is in
> dom/sms, right, just like PSms?

Yes. But IPDL is related to IPC and SmsMessage is not directly related to that. Making SmsMessage inheriting from something declared in an IPDL file seems weird because of that. IMO, we should be able to remove all IPC related code and still be able to build Gecko.
If you are strongly against any other solution, I will just do as recommended.
I wasn't suggesting inheriting from the message-data struct, only using it internally for data storage.  ("Has-a", not "is-a".)  I'm somewhat confused by the "removing IPC code" point; it's a fundamental technological building block, like xpconnect or spidermonkey or even C++.  If we want to change a building block, code will be affected.

I'm strongly against unnecessarily duplicating these data definitions, and writing unnecessary code. :)
Attachment #573294 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 88

7 years ago
Attachment #573612 - Attachment is obsolete: true
Attachment #576173 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #576173 - Attachment description: Part M - Receiving SMS, IPC handling → Part N - Receiving SMS, IPC handling
(Assignee)

Comment 89

7 years ago
Attachment #573682 - Attachment is obsolete: true
Attachment #576174 - Flags: review?(jones.chris.g)
Comment on attachment 576173 [details] [diff] [review]
Part N - Receiving SMS, IPC handling

>diff --git a/dom/sms/src/ipc/PSms.ipdl b/dom/sms/src/ipc/PSms.ipdl
 
>+struct SmsMessageData {
>+  PRInt32  id;
>+  // Whether the message has been received or sent (using constants in Constants.h).
>+  // TODO: this should be an enum but IPDL doesn't know about enum.
>+  PRInt8   delivery;

Sure it does!

// in .cpp
  enum Foo { /*...*/ };

// in .ipdl
  using Foo;
  //...
  async Message(Foo foo);

You have to write a serializer for it, but that's simple.  See the
serializer for mozilla::PixelFormat in ipc/glue/IPCMessageUtils.h.

This all looks fine except for the removal of enum DeliveryState.
Attachment #576173 - Flags: review?(jones.chris.g)
Attachment #576174 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 91

7 years ago
The current patches can be tested in the Android build you can find here:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mlamouri@mozilla.com-2b78b99255cd/

You can currently send a message with:
navigator.mozSms.send(NUMBER, MESSAGE);

and be informed when a new message comes with:
navigator.mozSms.addEventListener("received", function(e) {
  alert(e.message);
});
(you can also use mozSms.onReceived)
(Assignee)

Comment 92

7 years ago
This is creating a EnumSerializer template class which I'm using for DeliveryState. I'm didn't change PixelFormat yet, I will open a bug for that. There is also a small difference between the template and what PixelFormat does: the highValue represents the highest value, not the value highest not allowed value.
Attachment #576173 - Attachment is obsolete: true
Attachment #576448 - Flags: review?(jones.chris.g)
(Assignee)

Comment 93

7 years ago
Forgot to add one file...
Attachment #576448 - Attachment is obsolete: true
Attachment #576448 - Flags: review?(jones.chris.g)
Attachment #576468 - Flags: review?(jones.chris.g)

Comment 94

7 years ago
Just downloaded the aforementioned try-build and I'm unable to get the sms to send (haven't tried receiving yet).

On loading the following test page, sms should be sent (as I understand it) but presently isn't.  Note that I tried a different number string also dropping the +1 so 2163xxxxxx

<html>
   <head>
     <script type="text/javascript">
       text = "hello sms!";
        navigator.mozSms.send(+12163xxxxxx, text);
     </script> 
	</head>
<body>
	<p>Sends sms on load </p>
	</body>
</html>

page is hosted here:
view-source:http://people.mozilla.com/~jhammink/webapi_test_pages/SMSv4.html
(Assignee)

Comment 95

7 years ago
Sorry, I forgot to say you need to enable WebSMS with this pref:
dom.sms.enabled (have to be set to true)

And you have to whitelist the prepath [1] that are allowed to send SMS with this pref:
dom.sms.whitelist (prepaths are comma-separated)

[1] prepath is equivalent to protocol + user + password + domain. In your case: "http://people.mozilla.com" (I'm not sure if a final '/' is needed off-hand)
Comment on attachment 576468 [details] [diff] [review]
Part N - Receiving SMS, IPC handling

> #endif // mozilla_dom_sms_SmsMessage_h
>diff --git a/dom/sms/src/Types.h b/dom/sms/src/Types.h

>+namespace IPC {
>+
>+using namespace mozilla::dom::sms;
>+

This will import symbols from mozilla::dom::sms (sigh ... ;) into the
IPC namespace for all headers that end up including Types.h, which
isn't what you want to do.  As nasty and verbose as it is, please use
full qualification for the ParamTraits specialization below.

>diff --git a/ipc/glue/IPCMessageUtils.h b/ipc/glue/IPCMessageUtils.h

>+/**
>+ * Generic enum serializer.
>+ * E is the enum type.
>+ * lowValue is the lowest allowed value of the enum.
>+ * highValue is the highest allowed value of the enum.
>+ */
>+template <typename E, E lowValue, E highValue>

Using the highest allowed value as a guard doesn't work, because it
introduces a long invisible dependency from the enum declaration to
where the serializer is declared.  It's going to make it really
annoying to keep the serializer up to date.  So let's only use this
serializer with enums that have a "sentinel" value at the end, like
|eLast|.  (That means you should add a sentinel to the delivery-state
enum.)

r=me with those fixes
Attachment #576468 - Flags: review?(jones.chris.g) → review+
(Assignee)

Updated

7 years ago
Flags: in-testsuite?
(Assignee)

Updated

7 years ago
Attachment #572869 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #572870 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #572871 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #572872 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #572873 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #572874 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #572876 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #573225 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #573294 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #573306 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #573608 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #573609 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #573610 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #576174 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #576468 - Flags: checkin+
(Assignee)

Updated

7 years ago
Depends on: 705245
(Assignee)

Comment 98

7 years ago
Attachment #576912 - Flags: review?(jones.chris.g)
(Assignee)

Comment 99

7 years ago
Attachment #576914 - Flags: review?(jones.chris.g)
(Assignee)

Comment 100

7 years ago
Attachment #576915 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #576914 - Flags: superreview?(jones.chris.g)
Attachment #576914 - Flags: review?(jones.chris.g)
Attachment #576914 - Flags: review?(bugs)
(Assignee)

Comment 101

7 years ago
Attachment #576916 - Flags: review?(bugs)
(Assignee)

Updated

7 years ago
Attachment #576916 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Depends on: 705386
(Assignee)

Comment 103

7 years ago
Attachment #577019 - Flags: review?(jones.chris.g)
Comment on attachment 576912 [details] [diff] [review]
Part P - System to listen for sent messages in the Android backend

Why does the SMS code need to be thread safe?  In what circumstances will it not run on the java main thread?
Attachment #576914 - Flags: superreview?(jones.chris.g) → superreview+
Comment on attachment 576915 [details] [diff] [review]
Part R - Save sent messages in the Android DB

>+[scriptable, function, uuid(ab23f736-f545-4fcd-a298-3d6e2b380042)]
> interface nsISmsDatabaseService : nsISupports
> {
>+  // Takes some information required to write the message and returns its id.
>+  long writeSentMessage(in DOMString aReceiver, in DOMString aBody, in unsigned long long aDate);

I prefer the name "saveSentMessage()".  Feel free to call
bikeshedding if you want.

>diff --git a/dom/sms/src/android/SmsDatabaseService.cpp b/dom/sms/src/android/SmsDatabaseService.cpp
>+NS_IMETHODIMP
>+SmsDatabaseService::WriteSentMessage(const nsAString& aReceiver,
>+                                     const nsAString& aBody,
>+                                     PRUint64 aDate, PRInt32* aId)
>+{
>+  if (!AndroidBridge::Bridge()) {
>+    return NS_OK;
>+  }
>+
>+  *aId = AndroidBridge::Bridge()->WriteSentMessage(aReceiver, aBody, aDate);
>+  return NS_OK;

Nit:

  if (AndroidBridge* bridge = AndroidBridge::Bridge()) {
    *aId = bridge->WriteSentMessage(aReceiver, aBody, aDate);
  }
  return NS_OK;

Except ... isn't the XPCOM contract that the outparam has to be
written if you return NS_OK?  I think you probably want to set *aId to
-1 or 0 or something when we can't talk to android.  So maybe add an
|else| case to do that above.

>diff --git a/dom/sms/src/ipc/PSms.ipdl b/dom/sms/src/ipc/PSms.ipdl

>+    sync WriteSentMessage(nsString aReceiver, nsString aBody, PRUint64 aDate)

I would be tempted to re-use SmsMessageData here, but I'll leave that
up to you.
Attachment #576915 - Flags: review?(jones.chris.g) → review+
Attachment #576916 - Flags: review?(jones.chris.g) → review+
Comment on attachment 577019 [details] [diff] [review]
Part U - Delivered event: Android backend

(Holding off on reviewing this pending "Part P".)
(Assignee)

Comment 107

7 years ago
(In reply to Chris Jones [:cjones] [:warhammer] from comment #104)
> Comment on attachment 576912 [details] [diff] [review] [diff] [details] [review]
> Part P - System to listen for sent messages in the Android backend
> 
> Why does the SMS code need to be thread safe?  In what circumstances will it
> not run on the java main thread?

GeckoSmsManager.send() is always run in the Java main thread but GeckoSmsManager.onReceive() isn't. According to the doc, this method is called from another Dalvik process. I believe a Dalvik process is equivalent to a C-thread in the sense that the memory is shared but I have to make sure we don't have send() being called at the same time or even two onReceive() being called simultaneously.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #105)
> Nit:
> 
>   if (AndroidBridge* bridge = AndroidBridge::Bridge()) {
>     *aId = bridge->WriteSentMessage(aReceiver, aBody, aDate);
>   }
>   return NS_OK;
> 
> Except ... isn't the XPCOM contract that the outparam has to be
> written if you return NS_OK?  I think you probably want to set *aId to
> -1 or 0 or something when we can't talk to android.  So maybe add an
> |else| case to do that above.

I did that for SmsParent. I forgot to do it here.

> >diff --git a/dom/sms/src/ipc/PSms.ipdl b/dom/sms/src/ipc/PSms.ipdl
> 
> >+    sync WriteSentMessage(nsString aReceiver, nsString aBody, PRUint64 aDate)
> 
> I would be tempted to re-use SmsMessageData here, but I'll leave that
> up to you.

It would increase the amount of code to write if we add a parameter in this list. In addition, I don't really like using half-complete objects (that's one of the reason the service isn't using nsIDOMSmsMessage).
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #107)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #104)
> > Comment on attachment 576912 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > Part P - System to listen for sent messages in the Android backend
> > 
> > Why does the SMS code need to be thread safe?  In what circumstances will it
> > not run on the java main thread?
> 
> GeckoSmsManager.send() is always run in the Java main thread but
> GeckoSmsManager.onReceive() isn't. According to the doc, this method is
> called from another Dalvik process. I believe a Dalvik process is equivalent
> to a C-thread in the sense that the memory is shared but I have to make sure
> we don't have send() being called at the same time or even two onReceive()
> being called simultaneously.
> 

http://stackoverflow.com/questions/5674518/does-broadcastreceiver-onreceive-always-run-in-the-ui-thread thinks otherwise.  Since the google docs aren't clear on the point, I think it's fair to assume it's all UI-thread only, or else a mass of android apps would break.
Attachment #576912 - Flags: review?(jones.chris.g)
(Assignee)

Comment 109

7 years ago
Same patch without the semaphore.
Attachment #576912 - Attachment is obsolete: true
Attachment #577638 - Flags: review?(jones.chris.g)
Attachment #577018 - Flags: review?(bugs) → review+
Attachment #576916 - Flags: review?(bugs) → review+
Comment on attachment 576914 [details] [diff] [review]
Part Q - SmsDatabaseService

>+%{C++
>+#define SMS_DATABASE_SERVICE_CID { 0xb5bd6b55, 0x2e56, 0x4f1d, { 0xa4, 0x38, 0x1d, 0x10, 0xd6, 0xf3, 0x4d, 0xe5 } }
Could you please split this to few lines. Or does .idl not allow that?
Attachment #576914 - Flags: review?(bugs) → review+
(Assignee)

Comment 111

7 years ago
Same patch rebased.
Attachment #577019 - Attachment is obsolete: true
Attachment #577019 - Flags: review?(jones.chris.g)
Attachment #577650 - Flags: review?(jones.chris.g)
Just a heads up. Part O was causing Firefox to crash on Android. Bug 706449 shows the fix. We'd appreciate getting one of the Android devs to do feedback reviews when landing into those files.
Attachment #577638 - Flags: review?(jones.chris.g) → review+
Comment on attachment 577638 [details] [diff] [review]
Part P - System to listen for sent messages in the Android backend

This is quite different than the original part P ... is this the right patch?
(Assignee)

Comment 114

7 years ago
You did r+ that patch but it was the wrong one as you said in the comment. Did you meant to r+ "Part U" instead?
Anyhow, that patch should be the good one.
Attachment #577638 - Attachment is obsolete: true
Attachment #578027 - Flags: review?(jones.chris.g)
If you all are at a stage where this is nearing completion or a design has been chosen to fix the problem we really should schedule a security review.  Please take a look at the calendar (https://mail.mozilla.com/home/ckoenig@mozilla.com/Security%20Review.html) and let me know the date/time slot you would like.
dom/sms/Makefile.in, dom/sms/interfaces/Makefile.in and dom/sms/src/Makefile.in were created in the tree by part A of this bug, but were not listed in toolkit/toolkit-makefiles.sh, so I've included them here:
https://hg.mozilla.org/integration/mozilla-inbound/rev/17e8e969fa09
Comment on attachment 578027 [details] [diff] [review]
Part P - System to listen for sent messages in the Android backend

>diff --git a/embedding/android/GeckoSmsManager.java b/embedding/android/GeckoSmsManager.java

>+/**
>+ * This class is returning unique ids for PendingIntent requestCode attribute.
>+ * The ids are not really unique in the sense of if you are able to generate a
>+ * number of PendingIntent of the same type equals to the number of values an
>+ * Integer can have, you will have values conflicts.
>+ * However, we can safely consider this issue as unlikely.

  Return a "unique" ID.  There are only |Integer.MAX_VALUE -
  Integer.MIN_VALUE| unique IDs available, and they wrap around.

>+ */
>+class PendingIntentUID
>+{
>+  static private int sUID = Integer.MAX_VALUE;
>+
>+  static public int generate() {
>+    if (sUID == Integer.MAX_VALUE) {
>+      sUID = Integer.MIN_VALUE;
>+      return sUID;
>+    }
>+
>+    // Incrementing sUID *before* returning it.
>+    return ++sUID;
>+  }

  static private int sUID = Integer.MIN_VALUE;
  static public int generate() { return sUID++; }

>+/**
>+ * The envelope class contains all information that are needed to keep track of
>+ * a sent SMS.
>+ */
>+class Envelope
>+{
>+  protected int     mId;
>+  protected int     mRemainingParts;
>+  protected boolean mFailing;
>+

I tried very hard but failed to understand what benefit we gain from
having a module-private Envelope "base class" along with another
module-private EnvelopePrivate "subclass".  Unless you have a really
good reason for the current setup, please make
mId+mRemainingParts+mFailing public, get rid of the accessor methods
below, and move the EnvelopePrivate ctor into Envelope.

Don't give in to the temptation of over-Java-ification!  Gosling
begone!

> public class GeckoSmsManager
>   extends BroadcastReceiver
> {
>   public static void send(String aNumber, String aMessage) {

>       if (aMessage.length() <= kMaxMessageSize) {

Please remove this special case.  AFAICT it's added maintenance burden
for no gain.

Looks OK, would like to see the next version though.
Attachment #578027 - Flags: review?(jones.chris.g)
Comment on attachment 577650 [details] [diff] [review]
Part U - Delivered event: Android backend

Looking through here, I'm getting a bit confused about why we don't split the message parts up and then treat them as completely separate messages.  That seems to be how they're exposed to gecko.  If we did that, we could eliminate a lot of the state-tracking code in this patch, and in Part P.  There are some other things I'd like to see changed in this patch, but let's discuss the above first.

I'll be back on IRC around 1300-1400 Taiwan time, but we can discuss here too.
Attachment #577650 - Flags: review?(jones.chris.g)
(Assignee)

Comment 119

7 years ago
(In reply to Chris Jones [:cjones] [:warhammer] from comment #117)
> Comment on attachment 578027 [details] [diff] [review] [diff] [details] [review]
> Part P - System to listen for sent messages in the Android backend
> 
> >diff --git a/embedding/android/GeckoSmsManager.java b/embedding/android/GeckoSmsManager.java
> >+ */
> >+class PendingIntentUID
> >+{
> >+  static private int sUID = Integer.MAX_VALUE;
> >+
> >+  static public int generate() {
> >+    if (sUID == Integer.MAX_VALUE) {
> >+      sUID = Integer.MIN_VALUE;
> >+      return sUID;
> >+    }
> >+
> >+    // Incrementing sUID *before* returning it.
> >+    return ++sUID;
> >+  }
> 
>   static private int sUID = Integer.MIN_VALUE;
>   static public int generate() { return sUID++; }

Do you know for sure that Java would behave like C here?
I'm not, that's why my code is doing the checks.

> > public class GeckoSmsManager
> >   extends BroadcastReceiver
> > {
> >   public static void send(String aNumber, String aMessage) {
> 
> >       if (aMessage.length() <= kMaxMessageSize) {
> 
> Please remove this special case.  AFAICT it's added maintenance burden
> for no gain.

The reason why there are two paths is that I don't know what is the difference between sendTextMessage() and sendMultipartTextMessage() for a message that should take only one part. IOW, I don't know if calling sendMultipartTextMessage() puts some overhead that might make a 160 length message takes two parts.
I would prefer to keep that as is and open a follow-up to investigate if it is safe to remove the special casing unless you already know how it is behaving.
(Assignee)

Comment 120

7 years ago
(In reply to Chris Jones [:cjones] [:warhammer] from comment #118)
> Comment on attachment 577650 [details] [diff] [review] [diff] [details] [review]
> Part U - Delivered event: Android backend
> 
> Looking through here, I'm getting a bit confused about why we don't split
> the message parts up and then treat them as completely separate messages. 
> That seems to be how they're exposed to gecko.  If we did that, we could
> eliminate a lot of the state-tracking code in this patch, and in Part P. 
> There are some other things I'd like to see changed in this patch, but let's
> discuss the above first.

I disagree. Android has a very bad API here. The developer has to split the SMS message into multiple messages and then get a callback for each parts being sent or delivered but what a developer really cares about when using the API is whether the message has been sent or delivered the fact that the message has 1, 2 or X parts should be a problem for the backend.
Basically what I'm trying to do here is to make sure WebSMS consumers don't have to suffer from this exact same problem.
Basically, the intent of this API is to let consumers do that regardless of the message:
var request = navigator.mozSms.send(myNumber, myMessage);
request.onsuccess = function() { alert("Message sent!"); };

Does that make sense?
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #119)
> Do you know for sure that Java would behave like C here?
> I'm not, that's why my code is doing the checks.
> 

Yes.

> > > public class GeckoSmsManager
> > >   extends BroadcastReceiver
> > > {
> > >   public static void send(String aNumber, String aMessage) {
> > 
> > >       if (aMessage.length() <= kMaxMessageSize) {
> > 
> > Please remove this special case.  AFAICT it's added maintenance burden
> > for no gain.
> 
> The reason why there are two paths is that I don't know what is the
> difference between sendTextMessage() and sendMultipartTextMessage() for a
> message that should take only one part. IOW, I don't know if calling
> sendMultipartTextMessage() puts some overhead that might make a 160 length
> message takes two parts.

Read the code.  I would bet money that sendTextMessage() just wraps sendMultipartTextMessage().

In fact, have you checked that sendTextMessage() doesn't automatically split for you?  The docs really suck ...
(Assignee)

Updated

7 years ago
Depends on: 707867
(Assignee)

Comment 122

7 years ago
(In reply to Chris Jones [:cjones] [:warhammer] from comment #117)
> >+/**
> >+ * The envelope class contains all information that are needed to keep track of
> >+ * a sent SMS.
> >+ */
> >+class Envelope
> >+{
> >+  protected int     mId;
> >+  protected int     mRemainingParts;
> >+  protected boolean mFailing;
> >+
> 
> I tried very hard but failed to understand what benefit we gain from
> having a module-private Envelope "base class" along with another
> module-private EnvelopePrivate "subclass".  Unless you have a really
> good reason for the current setup, please make
> mId+mRemainingParts+mFailing public, get rid of the accessor methods
> below, and move the EnvelopePrivate ctor into Envelope.

I removed EnvelopePrivate but kept the getter/setter because no attributes should be fully public (getters/setters don't simply set/return).

> > public class GeckoSmsManager
> >   extends BroadcastReceiver
> > {
> >   public static void send(String aNumber, String aMessage) {
> 
> >       if (aMessage.length() <= kMaxMessageSize) {
> 
> Please remove this special case.  AFAICT it's added maintenance burden
> for no gain.

We can indeed remove the special casing. I will do that in a follow-up: bug 707867.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #121)
> Read the code.  I would bet money that sendTextMessage() just wraps
> sendMultipartTextMessage().

It's actually the other way around: sendMultipartTextMessage() calls sendTextMessage() if there is only one part ;)
(Assignee)

Comment 123

7 years ago
Attachment #579202 - Flags: review?(bugs)
(Assignee)

Updated

7 years ago
Attachment #579205 - Flags: review?(jones.chris.g)
(Assignee)

Comment 126

7 years ago
Attachment #579207 - Flags: superreview?(jonas)
Attachment #579207 - Flags: review?(bugs)
(Assignee)

Updated

7 years ago
Attachment #579207 - Flags: review?(jones.chris.g)
(Assignee)

Comment 129

7 years ago
Attachment #579211 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Depends on: 707870
(Assignee)

Comment 130

7 years ago
Attachment #579213 - Flags: superreview?(jonas)
Attachment #579213 - Flags: review?(bugs)
(Assignee)

Updated

7 years ago
Attachment #579213 - Flags: review?(jones.chris.g)
(Assignee)

Comment 132

7 years ago
Attachment #579215 - Flags: review?(jones.chris.g)
(Assignee)

Comment 133

7 years ago
Attachment #579216 - Flags: superreview?(jonas)
Attachment #579216 - Flags: review?(bugs)
(Assignee)

Updated

7 years ago
Attachment #579216 - Flags: review?(jones.chris.g)
Comment on attachment 579209 [details] [diff] [review]
Part Z - Allow to send to multiple recipients with send()

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

A few small comments below. r=mrbkap with them addressed.

::: dom/sms/interfaces/nsIDOMSmsManager.idl
@@ +42,4 @@
>  interface nsIDOMMozSmsManager : nsIDOMEventTarget
>  {
>    unsigned short      getNumberOfMessagesForText(in DOMString text);
> +  // numbers can be whether one number (DOMString) or an array of numbers.

This line of the comment doesn't seem to make sense.

::: dom/sms/src/SmsManager.cpp
@@ +152,2 @@
>  
> +  smsService->Send(nsAutoString(number), aMessage, requestId, 0);

Why is the temporary nsAutoString needed?

@@ +153,5 @@
> +  smsService->Send(nsAutoString(number), aMessage, requestId, 0);
> +
> +  nsresult rv = nsContentUtils::WrapNative(aCx, aGlobal, request, aRequest);
> +  if (NS_FAILED(rv)) {
> +    *aRequest = JSVAL_NULL;

This isn't really needed.

@@ +168,5 @@
> +  JSContext* cx = mScriptContext->GetNativeContext();
> +  NS_ASSERTION(cx, "Failed to get a context!");
> +
> +  if (!aNumber.isString() &&
> +      !(aNumber.toObjectOrNull() && JS_IsArrayObject(cx, aNumber.toObjectOrNull()))) {

This second line should probably be written as:

aNumber.isObject() && JS_IsArrayObject(cx, &aNumber.toObject())

the JS engine prefers this style internally as it shows more clearly that we're not going to pass null to JS_IsArrayObject.

@@ +187,5 @@
> +    return Send(cx, global, aNumber.toString(), aMessage, aReturn);
> +  }
> +
> +  // Must be an array then.
> +  JSObject* numbers = aNumber.toObjectOrNull();

... = &aNumber.toObject();

@@ +188,5 @@
> +  }
> +
> +  // Must be an array then.
> +  JSObject* numbers = aNumber.toObjectOrNull();
> +  NS_ASSERTION(numbers, "The argument must be an array at that point!");

Then, this assertion becomes obsolete.

@@ +191,5 @@
> +  JSObject* numbers = aNumber.toObjectOrNull();
> +  NS_ASSERTION(numbers, "The argument must be an array at that point!");
> +
> +  jsuint size;
> +  JS_GetArrayLength(cx, numbers, &size);

This call and the JS_GetElement call below need to check for error.

@@ +203,5 @@
> +    nsresult rv = Send(cx, global, number.toString(), aMessage, &requests[i]);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  }
> +
> +  *aReturn = OBJECT_TO_JSVAL(JS_NewArrayObject(cx, size, requests));

Need to deal with failure here, too.
Attachment #579209 - Flags: review?(mrbkap) → review+
Comment on attachment 579217 [details] [diff] [review]
Part AH - Allow SmsMessage or id as parameter to delete()

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

r=mrbkap with some small things addressed.

::: dom/sms/interfaces/nsIDOMSmsManager.idl
@@ +47,5 @@
>    // The method returns a SmsRequest object if one number has been passed.
>    // An array of SmsRequest objects otherwise.
>    jsval send(in jsval number, in DOMString message);
>    nsIDOMMozSmsRequest getMessage(in long id);
> +  // param can be whether an id (long) or a message (nsIDOMSmsMessage).

s/whether/either/

::: dom/sms/src/SmsManager.cpp
@@ +249,5 @@
> +  if (aParam.isInt32()) {
> +    return Delete(aParam.toInt32(), aRequest);
> +  }
> +
> +  if (!aParam.isObjectOrNull()) {

if (!aParam.isObject()) is what you want. This will allow JSVAL_NULL through.

@@ +253,5 @@
> +  if (!aParam.isObjectOrNull()) {
> +    return NS_ERROR_INVALID_ARG;
> +  }
> +
> +  JSObject* obj = aParam.toObjectOrNull();

Prefer ... = &aParam.toObject();

@@ +260,5 @@
> +  NS_ASSERTION(xpc, "This should never be null!");
> +
> +  nsCOMPtr<nsIXPConnectWrappedNative> wrapper;
> +  if (NS_FAILED(xpc->GetWrappedNativeOfJSObject(mScriptContext->GetNativeContext(), obj,
> +                                                getter_AddRefs(wrapper))) ||

Use GetNativeOfWrapper instead of GetWrappedNativeOfJSObject. It's cleaner and avoids some refcounting.
Attachment #579217 - Flags: review?(mrbkap) → review+
Comment on attachment 579213 [details] [diff] [review]
Part AD - Notify when getMessage() fails

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

Looks good apart from that.

::: dom/sms/src/SmsRequest.cpp
@@ +202,5 @@
>        aError.AssignLiteral("UnknownError");
>        break;
>      case eInternalError:
>        aError.AssignLiteral("InternalError");
>        break;

Unrelated to this patch, but I would merge InternalError and UnknownError. They both seem to make up the "other" category.

Also, this function should return a DOMError where .name is set to the above strings.

Feel free to do both these things in a separate bug.
Attachment #579213 - Flags: superreview?(jonas) → superreview+
Comment on attachment 579216 [details] [diff] [review]
Part AG - Notify when delete() fails

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

Was there anything in particular you wanted me to look at in this patch?
Attachment #579216 - Flags: superreview?(jonas) → superreview+
Attachment #579207 - Flags: superreview?(jonas) → superreview+
(Assignee)

Comment 139

7 years ago
(In reply to Jonas Sicking (:sicking) from comment #138)
> Was there anything in particular you wanted me to look at in this patch?

To make sure you agree with the general idea: SmsRequest.result returns true if the sms has been deleted and false otherwise. Errors only happen if we were not able to reach the DB or something weird like that.
Comment on attachment 579202 [details] [diff] [review]
Part V - Implement SmsRequest


>+[scriptable, function, uuid(1b24469d-cfb7-4667-aaf0-c1d17289ae7c)]
>+interface nsIDOMMozSmsRequest : nsIDOMEventTarget
>+{
>+  // Returns whether "processing" or "done".
>+  readonly attribute DOMString           readyState;
>+  // Can be null.
>+  readonly attribute DOMString           error;
>+  // Can be bool, nsIDOMSmsMessage, nsIDOMSmsIterator or null.
>+  readonly attribute jsval               result;
Why jsval and why not nsIVariant?
I would assume nsIVariant would be easier in this case since there wouldn't be need
for JS rooting.
Or actually, would nsISupports work here?
Attachment #579202 - Flags: review?(bugs) → review-
Comment on attachment 579203 [details] [diff] [review]
Part W - Implement SmsRequestManager


>+SmsRequestManager::CreateRequest(nsPIDOMWindow* aWindow,
>+                                 nsIScriptContext* aScriptContext,
>+                                 nsIDOMMozSmsRequest** aRequest)
>+{
>+  nsCOMPtr<nsIDOMMozSmsRequest> request =
>+    new SmsRequest(aWindow, aScriptContext);
>+
>+  PRInt32 size = mRequests.Count();
>+
>+  // Look for empty slots.
>+  for (PRInt32 i=0; i<size; ++i) {
i = 0; i < size;


>+    if (mRequests[i]) {
>+      continue;
>+    }
>+
>+    mRequests.ReplaceObjectAt(request, i);
>+    NS_ADDREF(*aRequest = request);
>+    return i;
>+  }
>+
>+
>+  mRequests.AppendObject(request);
>+  NS_ADDREF(*aRequest = request);
>+  return size;
>+}
How can there be empty slots?
I some other patch explains that.
Attachment #579203 - Flags: review?(bugs) → review+
Comment on attachment 579205 [details] [diff] [review]
Patch X - Use SmsRequest for send() method

>+SmsManager::Send(const nsAString& aNumber, const nsAString& aMessage, nsIDOMMozSmsRequest** aRequest)
> {
>   nsCOMPtr<nsISmsService> smsService = do_GetService(SMS_SERVICE_CONTRACTID);
>   NS_ENSURE_TRUE(smsService, NS_OK);
> 
>-  smsService->Send(aNumber, aMessage);
>+  int requestId =
>+    SmsRequestManager::GetInstance()->CreateRequest(mOwner, mScriptContext, aRequest);
>+  NS_ASSERTION(*aRequest, "The request object must have been created!");
Useless assertion. You crash immediately after this if *aRequest is null.


I know nothing about the android part.
Attachment #579205 - Flags: review?(bugs) → review+
Comment on attachment 579210 [details] [diff] [review]
Part AA - getMessage() DOM and IPC boilerplate

>+SmsManager::GetMessage(PRInt32 aId, nsIDOMMozSmsRequest** aRequest)
>+{
>+  int requestId =
>+    SmsRequestManager::GetInstance()->CreateRequest(mOwner, mScriptContext, aRequest);
>+  NS_ASSERTION(*aRequest, "The request object must have been created!");
>+
>+  nsCOMPtr<nsISmsDatabaseService> smsDBService =
>+    do_GetService(SMS_DATABASE_SERVICE_CONTRACTID);
>+  NS_ENSURE_TRUE(smsDBService, NS_ERROR_FAILURE);
>+
>+  smsDBService->GetMessage(aId, requestId, 0);
>+
>+  return NS_OK;
>+}
Somewhat strange, but I guess that is the way the API works.
Attachment #579210 - Flags: review?(bugs) → review+
Attachment #579213 - Flags: review?(bugs) → review+
Comment on attachment 579214 [details] [diff] [review]
Part AE - delete() DOM and IPC boilerplate

>-[scriptable, function, uuid(8bebc119-845c-4ae1-96e7-7b7ee970485b)]
>+[scriptable, function, uuid(4e628d96-abc9-45e1-b158-8970885a2552)]
> interface nsIDOMMozSmsManager : nsIDOMEventTarget
> {
>   unsigned short      getNumberOfMessagesForText(in DOMString text);
>   // numbers can be whether one number (DOMString) or an array of numbers.
>   // The method returns a SmsRequest object if one number has been passed.
>   // An array of SmsRequest objects otherwise.
>   jsval send(in jsval number, in DOMString message);
>   nsIDOMMozSmsRequest getMessage(in long id);
>+  nsIDOMMozSmsRequest delete(in long id);

Why do you need nsIDOMMozSmsRequest as return value?

What happens if one calls first delete(foo); and then getMessage(foo) ?
Attachment #579214 - Flags: review?(bugs) → review-
Comment on attachment 579216 [details] [diff] [review]
Part AG - Notify when delete() fails


>+NS_EXPORT void JNICALL
>+Java_org_mozilla_gecko_GeckoAppShell_notifySmsDeleteFailed(JNIEnv* jenv, jclass,
>+                                                           jint aError,
>+                                                           jint aRequestId,
>+                                                           jlong aProcessId)
>+{
>+    class NotifySmsDeleteFailedRunnable : public nsRunnable {
{ should be in the next line


But what is the use case to know that delete() failed? or why can it fail?
Comment on attachment 579216 [details] [diff] [review]
Part AG - Notify when delete() fails

r- until I understand the reason for this functionality.
Attachment #579216 - Flags: review?(bugs) → review-
Attachment #579207 - Flags: review?(bugs) → review+
(Assignee)

Comment 148

7 years ago
Comment on attachment 579214 [details] [diff] [review]
Part AE - delete() DOM and IPC boilerplate

Re-asking review per IRC conversation. Basically, delete() is async and like all async method in this API is using SmsRequest. Errors should not happen for regular situations right now. It might happen if DB permissions are not granted for examples. Depending on how the permissions management evolves it might be very useful though.
Attachment #579214 - Flags: review- → review?(bugs)
(Assignee)

Comment 149

7 years ago
Comment on attachment 579216 [details] [diff] [review]
Part AG - Notify when delete() fails

Re-asking reviews for reasons mentioned in the previous comment.
Attachment #579216 - Flags: review- → review?(bugs)
Comment on attachment 579214 [details] [diff] [review]
Part AE - delete() DOM and IPC boilerplate

OK.

But in general, I think the request based API makes it too complicated.
Attachment #579214 - Flags: review?(bugs) → review+
Comment on attachment 579202 [details] [diff] [review]
Part V - Implement SmsRequest

after IRC discussion, jsval is ok, but someone else needs to review
JSAPI usage.
Attachment #579202 - Flags: review- → review+
(Assignee)

Comment 152

7 years ago
Comment on attachment 579202 [details] [diff] [review]
Part V - Implement SmsRequest

Blake, could you review the JSAPI usage here? Should be fast given that you review the following patch that refactors everything.
Attachment #579202 - Flags: review?(mrbkap)
Comment on attachment 579216 [details] [diff] [review]
Part AG - Notify when delete() fails


>+bool
>+SmsChild::RecvNotifyRequestSmsDeleteFailed(const PRInt32& aError,
>+                                           const PRInt32& aRequestId,
>+                                           const PRUint64& aProcessId)
>+{
>+  if (ContentChild::GetSingleton()->GetID() != aProcessId) {
>+    return true;
>+  }
This needs some comment. Why can a wrong process get deletefailed message.
And there needs to be a followup bug to fix this case. Wrong process shouldn't get the message.

So, please, a follow up to fix process ID handling.
Attachment #579216 - Flags: review?(bugs) → review+
Comment on attachment 579205 [details] [diff] [review]
Patch X - Use SmsRequest for send() method

This patch makes me very sad.  I've grown to dislike it a lot more
than since we chatted yesterday.  Since we're in rather a time crunch
and won't be using SMS in content processes immediately after this
lands, I'll begrudgingly let this impl land.  But only if you file a
followup bug, and promise to take it seriously ;).

I think all we needed to do was instead of passing around requestIds,
pass around nsISmsRequest references (or something like that).  So the
workflow across processes might have looked something like,

  content process:
    var req = navigator.mozSms.send(...)
      create nsIDOMSmsRequest for |req|.  nsIDOMSmsRequest implements
      nsISmsRequest

      nsISmsService.send(..., req)
         nsRemoteSmsService: create PSmsRequestChild actor under PSms
         that wraps |req|.

  chrome process:
      recv ctor for PSmsRequest.  Create nsISmsRequest that wraps
      PSmsRequestParent.  PSmsRequestParent implements nsISmsRequest.

      nsISmsService.send(..., req)
        ...

Do you see any issues with this implementation?  The SmsManager only
needs to track nsIDOMSmsRequest instances; the rest are managed by
IPDL automatically.

So yeah, followup bug plz.

>diff --git a/dom/sms/interfaces/nsISmsService.idl b/dom/sms/interfaces/nsISmsService.idl

>+            void send(in DOMString number, in DOMString message,
>+                      in long requestId, [optional] in unsigned long long proccessId);

"processId"

>diff --git a/dom/sms/src/ipc/PSms.ipdl b/dom/sms/src/ipc/PSms.ipdl

>+    NotifyRequestSmsSent(SmsMessageData aMessageData, PRInt32 aRequestId,
>+                         PRUint64 aProcessId);

Let's get rid of processId before landing this.  The change I proposed
above to remove processId entirely will supersede this, but let's get
this fix made before landing.  I was promised a followup patch :).

> parent:

>+    SendMessage(nsString aNumber, nsString aMessage, PRInt32 aRequestId,
>+                PRUint64 aProcessId);

(aProcessId needs to go away from here.)

>diff --git a/dom/sms/src/ipc/SmsChild.cpp b/dom/sms/src/ipc/SmsChild.cpp

>+bool
>+SmsChild::RecvNotifyRequestSmsSent(const SmsMessageData& aMessage,
>+                                   const PRInt32& aRequestId,
>+                                   const PRUint64& aProcessId)
>+{
>+  if (ContentChild::GetSingleton()->GetID() != aProcessId) {
>+    return true;
>+  }
>+

(This check should go away.)

>diff --git a/dom/sms/src/ipc/SmsParent.cpp b/dom/sms/src/ipc/SmsParent.cpp

>+nsTArray<SmsParent*>* SmsParent::gSmsParents = nsnull;
>+

You don't need to track this manually.  Remove it.

>+/* static */ void
>+SmsParent::GetAll(nsTArray<SmsParent*>& aArray)

When ContentParent knows about processId, we won't need this helper
anymore.  But for now, rewrite it using ContentParent::GetAll() and
then adding contentParent->ManagedPSms().

>diff --git a/widget/src/android/AndroidJNI.cpp b/widget/src/android/AndroidJNI.cpp

>+          nsTArray<SmsParent*> spList;
>+          SmsParent::GetAll(spList);
>+
>+          for (PRUint32 i=0; i<spList.Length(); ++i) {
>+            unused << spList[i]->SendNotifyRequestSmsSent(mMessageData,
>+                                                          mRequestId,
>+                                                          mProcessId);
>+          }

This needs to be fixed in the followup patch.
Attachment #579205 - Flags: review?(jones.chris.g) → review+
Attachment #579202 - Flags: review?(mrbkap) → review+
Comment on attachment 579207 [details] [diff] [review]
Patch Y - Notify when send() fails.

>diff --git a/embedding/android/GeckoSmsManager.java b/embedding/android/GeckoSmsManager.java

>+  /**
>+   * Error type (only for sent).
>+   */
>+  protected int       mError;
>+

Make this public and drop the getter/setter.

This patch needs some similar fix-ups to the previous.
Attachment #579207 - Flags: review?(jones.chris.g) → review+
Comment on attachment 579211 [details] [diff] [review]
Part AB - getMessage() Android implementation

>diff --git a/embedding/android/GeckoSmsManager.java b/embedding/android/GeckoSmsManager.java

>+  public static void getMessage(int aMessageId, int aRequestId, long aProcessId) {

This is probably going to involve disk IO, so we can't do it on the
main thread.  Forwarding this to a sms-db-query thread should be a
relatively small change.
Attachment #579211 - Flags: review?(jones.chris.g) → review-
(Assignee)

Updated

7 years ago
Depends on: 708546
(Assignee)

Updated

7 years ago
Depends on: 708547
(Assignee)

Updated

7 years ago
Depends on: 708552
(Assignee)

Updated

7 years ago
Depends on: 708556
(Assignee)

Comment 157

7 years ago
(In reply to Chris Jones [:cjones] [:warhammer] from comment #155)
> >+  /**
> >+   * Error type (only for sent).
> >+   */
> >+  protected int       mError;
> >+
> 
> Make this public and drop the getter/setter.
> 
> This patch needs some similar fix-ups to the previous.

Given that I don't want to expose mRemainingParts and mFailing publicly, I thought it would be better to also put getters and setters for the other attributes for consistency.
Comment on attachment 579213 [details] [diff] [review]
Part AD - Notify when getMessage() fails

>diff --git a/dom/sms/src/ipc/PSms.ipdl b/dom/sms/src/ipc/PSms.ipdl

>+    NotifyRequestGetSmsFailed(PRInt32 aError, PRInt32 aRequestId,

I noticed this in a previous patch, but forgot to mention it, sorry:
you'll save some code and be a bit more resilient to errors by writing
a EnumSerializer for SmsError and then using it here instead of
PRInt32.  Followup is fine.

>diff --git a/dom/sms/src/ipc/SmsChild.cpp b/dom/sms/src/ipc/SmsChild.cpp

>+  SmsRequestManager::GetInstance()->NotifyGetSmsFailed(aRequestId,
>+                                                        SmsRequest::ErrorType(aError));

Nit: indent is weird here.

>diff --git a/embedding/android/GeckoSmsManager.java b/embedding/android/GeckoSmsManager.java

>       Log.e("GeckoSmsManager", "Requested message id (" + aMessageId +
>                                ") is different from the one we got.");
>+      GeckoAppShell.notifyGetSmsFailed(kUnknownError, aRequestId, aProcessId);

Should this be kInternalError?  Doesn't matter that much.

>diff --git a/widget/src/android/AndroidJNI.cpp b/widget/src/android/AndroidJNI.cpp

>+{
>+    class NotifyGetSmsFailedRunnable : public nsRunnable {

>+    nsCOMPtr<nsIRunnable> runnable =
>+      new NotifyGetSmsFailedRunnable(SmsRequest::ErrorType(aError), aRequestId, aProcessId);
>+    NS_DispatchToMainThread(runnable);
>+}

It's looking we should be able to share more of the code implementing
these runnables.  Can go in a followup.
Attachment #579213 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 159

7 years ago
(In reply to Chris Jones [:cjones] [:warhammer] from comment #158)
> >       Log.e("GeckoSmsManager", "Requested message id (" + aMessageId +
> >                                ") is different from the one we got.");
> >+      GeckoAppShell.notifyGetSmsFailed(kUnknownError, aRequestId, aProcessId);
> 
> Should this be kInternalError?  Doesn't matter that much.

I already have a follow-up opened to merge those two kind of errors.

> >+{
> >+    class NotifyGetSmsFailedRunnable : public nsRunnable {
> 
> >+    nsCOMPtr<nsIRunnable> runnable =
> >+      new NotifyGetSmsFailedRunnable(SmsRequest::ErrorType(aError), aRequestId, aProcessId);
> >+    NS_DispatchToMainThread(runnable);
> >+}
> 
> It's looking we should be able to share more of the code implementing
> these runnables.  Can go in a followup.

This is in my notes and will be fixed in a follow-up.
Comment on attachment 579215 [details] [diff] [review]
Part AE - delete() implementation for Android

>diff --git a/embedding/android/GeckoSmsManager.java b/embedding/android/GeckoSmsManager.java

>+  public static void deleteMessage(int aMessageId, int aRequestId, long aProcessId) {

More synchronous disk IO.  Post it to the disk thread that's to-be-created.

The rest looks OK.
Attachment #579215 - Flags: review?(jones.chris.g) → review-
Comment on attachment 579216 [details] [diff] [review]
Part AG - Notify when delete() fails

Same comments here as comment 158.
Attachment #579216 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 162

7 years ago
Attachment #580272 - Flags: review?(jones.chris.g)
(Assignee)

Comment 163

7 years ago
With IO done outside of the UI thread.
Attachment #579211 - Attachment is obsolete: true
Attachment #580273 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #579213 - Attachment description: Part AC - Notify when getMessage() fails → Part AD - Notify when getMessage() fails
(Assignee)

Updated

7 years ago
Attachment #579214 - Attachment description: Part AD - delete() DOM and IPC boilerplate → Part AE - delete() DOM and IPC boilerplate
(Assignee)

Comment 164

7 years ago
Attachment #579215 - Attachment is obsolete: true
Attachment #580278 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #579216 - Attachment description: Part AF - Notify when delete() fails → Part AG - Notify when delete() fails
(Assignee)

Updated

7 years ago
Attachment #579217 - Attachment description: Part AG - Allow SmsMessage or id as parameter to delete() → Part AH - Allow SmsMessage or id as parameter to delete()
(Assignee)

Comment 165

7 years ago
Attachment #580285 - Flags: review?(bugs)
(Assignee)

Updated

7 years ago
Attachment #580285 - Flags: review?(mrbkap)
(Assignee)

Comment 167

7 years ago
Attachment #580289 - Flags: review?(jones.chris.g)
(Assignee)

Comment 169

7 years ago
Attachment #580291 - Flags: review?(bugs)
(Assignee)

Updated

7 years ago
Attachment #580292 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #580293 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #580292 - Attachment description: Part AN - Handling no messages in created message list → Part AN - getMessages(): Handling no messages in created message list
(Assignee)

Updated

7 years ago
Attachment #580293 - Attachment description: Part AO - Get first message in created list → Part AO - getMessages(): Get first message in created list
(Assignee)

Comment 175

7 years ago
Attachment #580297 - Flags: review?(jonas)
(Assignee)

Updated

7 years ago
Attachment #580298 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #580299 - Flags: review?(jones.chris.g)
Comment on attachment 580285 [details] [diff] [review]
Part AI - Implement SmsFilter

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

::: dom/sms/src/SmsFilter.cpp
@@ +85,5 @@
> +    *aStartDate = JSVAL_NULL;
> +    return NS_OK;
> +  }
> +
> +  aStartDate->setObjectOrNull(JS_NewDateObjectMsec(aCx, mData.startDate()));

JS_NewDateObjectMsec can fail due to OOM. You need to null check the value and return a failure if it does. There's a few more instances below, I won't comment on each one.

Also, it's preferred to use aStartDate->setObject(*obj);

@@ +118,5 @@
> +    *aEndDate = JSVAL_NULL;
> +    return NS_OK;
> +  }
> +
> +  aEndDate->setObjectOrNull(JS_NewDateObjectMsec(aCx, mData.endDate()));

Ditto.
Attachment #580285 - Flags: review?(mrbkap) → review+
Attachment #580272 - Flags: review?(jones.chris.g) → review+
Comment on attachment 580273 [details] [diff] [review]
Part AC - getMessage() Android implementation

>diff --git a/embedding/android/GeckoSmsManager.java b/embedding/android/GeckoSmsManager.java

>+        } catch (NotFoundException e) {
>+          // TODO: send failure notification
>+          Log.i("GeckoSmsManager", "Message id " + mMessageId + " not found");
>+        } catch (UnmatchingIdException e) {
>+          // TODO: send failure notification
>+          Log.e("GeckoSmsManager", "Requested message id (" + mMessageId +
>+                                   ") is different from the one we got.");
>+        } catch (TooManyResultsException e) {
>+          // TODO: send failure notification
>+          Log.e("GeckoSmsManager", "Get too many results for id " + mMessageId);
>+        } catch (InvalidTypeException e) {
>+          // TODO: send failure notification
>+          Log.i("GeckoSmsManager", "Message has an invalid type, we ignore it.");
>+        } catch (Exception e) {
>+          // TODO: send failure notification
>+          Log.e("GeckoSmsManager", "Error while trying to get message: " + e);

This is more expensive and more verbose than just keeping these cases
in the consequents of the if-stmts above; I think we only lose here,
not gain.  Plz move them back to the consequents (keeping the
"finally" block, of course).

The rest looks ok, r=me with that fix.
Attachment #580273 - Flags: review?(jones.chris.g) → review+
Comment on attachment 580278 [details] [diff] [review]
Patch AF - delete() implementation for Android

>+        } catch (TooManyResultsException e) {
>+          // TODO: Notify failure
>+          Log.e("GeckoSmsManager", "Delete more than one message? " + e);

Same comment here: let's just move this code into the if-stmt consequent.

r=me with that
Attachment #580278 - Flags: review?(jones.chris.g) → review+
Attachment #580285 - Flags: review?(bugs) → review+
Attachment #580287 - Flags: review?(bugs) → review+
Comment on attachment 580291 [details] [diff] [review]
Part AM - Implement SmsIterator

>+SmsIterator::Next()
>+{
>+  // No message means we are waiting for a message or we got the last one.
>+  if (!mMessage) {
>+    return NS_ERROR_DOM_INVALID_STATE_ERR;
>+  }
Quite surprising to throw here, I think.
Could next() return boolean. false would mean iteration certainly couldn't proceed.
Attachment #580291 - Flags: review?(bugs) → review-
Comment on attachment 580285 [details] [diff] [review]
Part AI - Implement SmsFilter


>+[scriptable, function, uuid(7da08e45-ee81-4293-912b-2f2fea5b6935)]
>+interface nsIDOMMozSmsFilter : nsISupports
Please mark the interface builtin
Attachment #580292 - Flags: review?(bugs) → review+
Attachment #580293 - Flags: review?(bugs) → review+
Comment on attachment 580294 [details] [diff] [review]
Part AP - getMessages(): iterator.next() implementation (DOM)

># HG changeset patch
># Parent 6080db4734fbb75b11b18127dfa9056cdad5ce24
># User Mounir Lamouri <mounir.lamouri@gmail.com>
># Date 1323320292 -28800
>
>diff --git a/dom/sms/interfaces/nsISmsDatabaseService.idl b/dom/sms/interfaces/nsISmsDatabaseService.idl
>--- a/dom/sms/interfaces/nsISmsDatabaseService.idl
>+++ b/dom/sms/interfaces/nsISmsDatabaseService.idl
>@@ -40,19 +40,20 @@
> #define SMS_DATABASE_SERVICE_CID \
> { 0xcb971459, 0xe85a, 0x49b3,    \
> { 0xbc, 0x1c, 0x10, 0x40, 0x27, 0x1e, 0x04, 0x6c } }
> #define SMS_DATABASE_SERVICE_CONTRACTID "@mozilla.org/sms/smsdatabaseservice;1"
> %}
> 
> interface nsIDOMMozSmsFilter;
> 
>-[scriptable, function, uuid(33358749-d1b3-4bd6-835c-ef3869f0e966)]
>+[scriptable, function, uuid(a253ec42-142e-490b-a479-1e9c605c0a58)]
> interface nsISmsDatabaseService : nsISupports
> {
>   // Takes some information required to save the message and returns its id.
>   long saveSentMessage(in DOMString aReceiver, in DOMString aBody, in unsigned long long aDate);
> 
>   void getMessage(in long messageId, in long requestId, [optional] in unsigned long long processId);
>   void deleteMessage(in long messadeId, in long requestId, [optional] in unsigned long long processId);
> 
>   void createMessageList(in nsIDOMMozSmsFilter filter, in boolean reverse, in long requestId, [optional] in unsigned long long processId);
>+  void getNextMessageInList(in long listId, in long requestId, in unsigned long long processId);
> };
>diff --git a/dom/sms/src/SmsIterator.cpp b/dom/sms/src/SmsIterator.cpp
>--- a/dom/sms/src/SmsIterator.cpp
>+++ b/dom/sms/src/SmsIterator.cpp
>@@ -36,16 +36,19 @@
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "SmsIterator.h"
> #include "nsIDOMClassInfo.h"
> #include "nsDOMError.h"
> #include "nsIDOMSmsFilter.h"
> #include "nsIDOMSmsMessage.h"
> #include "nsIDOMSmsRequest.h"
>+#include "SmsRequest.h"
>+#include "SmsRequestManager.h"
>+#include "nsISmsDatabaseService.h"
> 
> DOMCI_DATA(MozSmsIterator, mozilla::dom::sms::SmsIterator)
> 
> namespace mozilla {
> namespace dom {
> namespace sms {
> 
> NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(SmsIterator)
>@@ -55,22 +58,24 @@ NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(
> NS_INTERFACE_MAP_END
> 
> NS_IMPL_CYCLE_COLLECTION_3(SmsIterator, mFilter, mRequest, mMessage)
> 
> NS_IMPL_CYCLE_COLLECTING_ADDREF(SmsIterator)
> NS_IMPL_CYCLE_COLLECTING_RELEASE(SmsIterator)
> 
> SmsIterator::SmsIterator(nsIDOMMozSmsFilter* aFilter)
>-  : mFilter(aFilter)
>+  : mListId(-1)
>+  , mFilter(aFilter)
> {
> }
> 
>-SmsIterator::SmsIterator(nsIDOMMozSmsFilter* aFilter, nsIDOMMozSmsRequest* aRequest)
>-  : mFilter(aFilter)
>+SmsIterator::SmsIterator(PRInt32 aListId, nsIDOMMozSmsFilter* aFilter, nsIDOMMozSmsRequest* aRequest)
>+  : mListId(aListId)
>+  , mFilter(aFilter)
>   , mRequest(aRequest)
> {
> }
> 
> NS_IMETHODIMP
> SmsIterator::GetFilter(nsIDOMMozSmsFilter** aFilter)
> {
>   NS_ADDREF(*aFilter = mFilter);
>@@ -87,19 +92,26 @@ SmsIterator::GetMessage(nsIDOMMozSmsMess
> NS_IMETHODIMP
> SmsIterator::Next()
> {
>   // No message means we are waiting for a message or we got the last one.
>   if (!mMessage) {
>     return NS_ERROR_DOM_INVALID_STATE_ERR;
>   }
> 
>-  // TODO: ask for the next message and reset the request
>-  // TODO: add the associated request to the request manager
>-  //       and send the id to the backend
>+  mMessage = nsnull;
>+  static_cast<SmsRequest*>(mRequest.get())->Reset();
>+
>+  PRInt32 requestId = SmsRequestManager::GetInstance()->AddRequest(mRequest);
>+
>+  nsCOMPtr<nsISmsDatabaseService> smsDBService =
>+    do_GetService(SMS_DATABASE_SERVICE_CONTRACTID);
>+  NS_ENSURE_TRUE(smsDBService, NS_ERROR_FAILURE);
>+
>+  smsDBService->GetNextMessageInList(mListId, requestId, 0);
> 
>   return NS_OK;
> }
> 
> } // namespace sms
> } // namespace dom
> } // namespace mozilla
> 
>diff --git a/dom/sms/src/SmsIterator.h b/dom/sms/src/SmsIterator.h
>--- a/dom/sms/src/SmsIterator.h
>+++ b/dom/sms/src/SmsIterator.h
>@@ -54,21 +54,22 @@ class SmsIterator : public nsIDOMMozSmsI
> {
> public:
>   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>   NS_DECL_NSIDOMMOZSMSITERATOR
> 
>   NS_DECL_CYCLE_COLLECTION_CLASS(SmsIterator)
> 
>   SmsIterator(nsIDOMMozSmsFilter* aFilter);
>-  SmsIterator(nsIDOMMozSmsFilter* aFilter, nsIDOMMozSmsRequest* aRequest);
>+  SmsIterator(PRInt32 aListId, nsIDOMMozSmsFilter* aFilter, nsIDOMMozSmsRequest* aRequest);
> 
>   void SetMessage(nsIDOMMozSmsMessage* aMessage);
> 
> private:
>+  PRInt32                       mListId;
>   nsCOMPtr<nsIDOMMozSmsFilter>  mFilter;
>   nsCOMPtr<nsIDOMMozSmsRequest> mRequest;
>   nsCOMPtr<nsIDOMMozSmsMessage> mMessage;
> };
> 
> inline void
> SmsIterator::SetMessage(nsIDOMMozSmsMessage* aMessage)
> {
>diff --git a/dom/sms/src/SmsRequest.cpp b/dom/sms/src/SmsRequest.cpp
>--- a/dom/sms/src/SmsRequest.cpp
>+++ b/dom/sms/src/SmsRequest.cpp
>@@ -101,16 +101,31 @@ SmsRequest::SmsRequest(nsPIDOMWindow* aW
> SmsRequest::~SmsRequest()
> {
>   if (mResultRooted) {
>     UnrootResult();
>   }
> }
> 
> void
>+SmsRequest::Reset()
>+{
>+  NS_ASSERTION(mDone, "mDone should be true if we try to reset!");
>+  NS_ASSERTION(mResult != JSVAL_VOID, "mResult should be set if we try to reset!");
>+  NS_ASSERTION(mError == eNoError, "There should be no error if we try to reset!");
>+
>+  if (mResultRooted) {
>+    UnrootResult();
>+  }
>+
>+  mResult = JSVAL_VOID;
>+  mDone = false;
>+}
>+
>+void
> SmsRequest::RootResult()
> {
>   NS_ASSERTION(!mResultRooted, "Don't call RootResult() if already rooted!");
>   NS_HOLD_JS_OBJECTS(this, SmsRequest);
>   mResultRooted = true;
> }
> 
> void
>diff --git a/dom/sms/src/SmsRequest.h b/dom/sms/src/SmsRequest.h
>--- a/dom/sms/src/SmsRequest.h
>+++ b/dom/sms/src/SmsRequest.h
>@@ -70,16 +70,18 @@ public:
>   NS_DECL_ISUPPORTS
>   NS_DECL_NSIDOMMOZSMSREQUEST
> 
>   NS_FORWARD_NSIDOMEVENTTARGET(nsDOMEventTargetWrapperCache::)
> 
>   NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(SmsRequest,
>                                                          nsDOMEventTargetWrapperCache)
> 
>+  void Reset();
>+
> private:
>   SmsRequest() MOZ_DELETE;
> 
>   SmsRequest(nsPIDOMWindow* aWindow, nsIScriptContext* aScriptContext);
>   ~SmsRequest();
> 
>   /**
>    * Root mResult (jsval) to prevent garbage collection.
>diff --git a/dom/sms/src/SmsRequestManager.cpp b/dom/sms/src/SmsRequestManager.cpp
>--- a/dom/sms/src/SmsRequestManager.cpp
>+++ b/dom/sms/src/SmsRequestManager.cpp
>@@ -70,16 +70,36 @@ SmsRequestManager::Shutdown()
> 
> /* static */ SmsRequestManager*
> SmsRequestManager::GetInstance()
> {
>   return sInstance;
> }
> 
> PRInt32
>+SmsRequestManager::AddRequest(nsIDOMMozSmsRequest* aRequest)
>+{
>+  // TODO: merge with CreateRequest
>+  PRInt32 size = mRequests.Count();
>+
>+  // Look for empty slots.
>+  for (PRInt32 i=0; i<size; ++i) {
>+    if (mRequests[i]) {
>+      continue;
>+    }
>+
>+    mRequests.ReplaceObjectAt(aRequest, i);
>+    return i;
>+  }
>+
>+  mRequests.AppendObject(aRequest);
>+  return size;
>+}
>+
>+PRInt32
> SmsRequestManager::CreateRequest(nsPIDOMWindow* aWindow,
>                                  nsIScriptContext* aScriptContext,
>                                  nsIDOMMozSmsRequest** aRequest)
> {
>   nsCOMPtr<nsIDOMMozSmsRequest> request =
>     new SmsRequest(aWindow, aScriptContext);
> 
>   PRInt32 size = mRequests.Count();
>@@ -197,17 +217,17 @@ SmsRequestManager::NotifyNoMessageInList
> 
> void
> SmsRequestManager::NotifyCreateMessageList(PRInt32 aRequestId, PRInt32 aListId,
>                                            nsIDOMMozSmsMessage* aMessage)
> {
>   // TODO: use Filter!
>   SmsRequest* request = GetRequest(aRequestId);
> 
>-  nsCOMPtr<SmsIterator> iterator = new SmsIterator(nsnull, request);
>+  nsCOMPtr<SmsIterator> iterator = new SmsIterator(aListId, nsnull, request);
>   iterator->SetMessage(aMessage);
> 
>   NotifySuccess<nsIDOMMozSmsIterator*>(aRequestId, iterator);
> }
> 
> } // namespace sms
> } // namespace dom
> } // namespace mozilla
>diff --git a/dom/sms/src/SmsRequestManager.h b/dom/sms/src/SmsRequestManager.h
>--- a/dom/sms/src/SmsRequestManager.h
>+++ b/dom/sms/src/SmsRequestManager.h
>@@ -57,16 +57,18 @@ public:
>   static void Init();
>   static void Shutdown();
>   static SmsRequestManager* GetInstance();
> 
>   PRInt32 CreateRequest(nsPIDOMWindow* aWindow,
>                         nsIScriptContext* aScriptContext,
>                         nsIDOMMozSmsRequest** aRequest);
> 
>+  PRInt32 AddRequest(nsIDOMMozSmsRequest* aRequest);
>+
>   void NotifySmsSent(PRInt32 aRequestId, nsIDOMMozSmsMessage* aMessage);
>   void NotifySmsSendFailed(PRInt32 aRequestId, SmsRequest::ErrorType aError);
>   void NotifyGotSms(PRInt32 aRequestId, nsIDOMMozSmsMessage* aMessage);
>   void NotifyGetSmsFailed(PRInt32 aRequestId, SmsRequest::ErrorType aError);
>   void NotifySmsDeleted(PRInt32 aRequestId, bool aDeleted);
>   void NotifySmsDeleteFailed(PRInt32 aRequestId, SmsRequest::ErrorType aError);
>   void NotifyNoMessageInList(PRInt32 aRequestId);
>   void NotifyCreateMessageList(PRInt32 aRequestId, PRInt32 aListId, nsIDOMMozSmsMessage* aMessage);
>diff --git a/dom/sms/src/android/SmsDatabaseService.cpp b/dom/sms/src/android/SmsDatabaseService.cpp
>--- a/dom/sms/src/android/SmsDatabaseService.cpp
>+++ b/dom/sms/src/android/SmsDatabaseService.cpp
>@@ -94,11 +94,19 @@ SmsDatabaseService::CreateMessageList(ns
>   }
> 
>   AndroidBridge::Bridge()->CreateMessageList(
>     static_cast<SmsFilter*>(aFilter)->GetData(), aReverse, aRequestId, aProcessId
>   );
>   return NS_OK;
> }
> 
>+NS_IMETHODIMP
>+SmsDatabaseService::GetNextMessageInList(PRInt32 aListId, PRInt32 aRequestId,
>+                                         PRUint64 aProcessId)
>+{
>+  // TODO: implement
>+  return NS_OK;
>+}
>+
> } // namespace sms
> } // namespace dom
> } // namespace mozilla
>diff --git a/dom/sms/src/fallback/SmsDatabaseService.cpp b/dom/sms/src/fallback/SmsDatabaseService.cpp
>--- a/dom/sms/src/fallback/SmsDatabaseService.cpp
>+++ b/dom/sms/src/fallback/SmsDatabaseService.cpp
>@@ -73,11 +73,19 @@ NS_IMETHODIMP
> SmsDatabaseService::CreateMessageList(nsIDOMMozSmsFilter* aFilter,
>                                       bool aReverse, PRInt32 aRequestId,
>                                       PRUint64 aProcessId)
> {
>   NS_ERROR("We should not be here!");
>   return NS_OK;
> }
> 
>+NS_IMETHODIMP
>+SmsDatabaseService::GetNextMessageInList(PRInt32 aListId, PRInt32 aRequestId,
>+                                         PRUint64 aProcessId)
>+{
>+  NS_ERROR("We should not be here!");
>+  return NS_OK;
>+}
>+
> } // namespace sms
> } // namespace dom
> } // namespace mozilla
>diff --git a/dom/sms/src/ipc/PSms.ipdl b/dom/sms/src/ipc/PSms.ipdl
>--- a/dom/sms/src/ipc/PSms.ipdl
>+++ b/dom/sms/src/ipc/PSms.ipdl
>@@ -108,14 +108,16 @@ parent:
>         returns (PRInt32 aId);
> 
>     GetMessage(PRInt32 aMessageId, PRInt32 aRequestId, PRUint64 aProcessId);
> 
>     DeleteMessage(PRInt32 aMessageId, PRInt32 aRequestId, PRUint64 aProcessId);
> 
>     CreateMessageList(SmsFilterData aFilter, bool aReverse, PRInt32 aRequestId, PRUint64 aProcessId);
> 
>+    GetNextMessageInList(PRInt32 aListId, PRInt32 aRequestId, PRUint64 aProcessId);
>+
>     __delete__();
> };
> 
> } // namespace sms
> } // namespace dom
> } // namespace mozilla
>diff --git a/dom/sms/src/ipc/SmsIPCService.cpp b/dom/sms/src/ipc/SmsIPCService.cpp
>--- a/dom/sms/src/ipc/SmsIPCService.cpp
>+++ b/dom/sms/src/ipc/SmsIPCService.cpp
>@@ -126,11 +126,20 @@ SmsIPCService::CreateMessageList(nsIDOMM
> {
>   SmsFilter* filter = static_cast<SmsFilter*>(aFilter);
>   GetSmsChild()->SendCreateMessageList(filter->GetData(), aReverse, aRequestId,
>                                        ContentChild::GetSingleton()->GetID());
> 
>   return NS_OK;
> }
> 
>+NS_IMETHODIMP
>+SmsIPCService::GetNextMessageInList(PRInt32 aListId, PRInt32 aRequestId,
>+                                    PRUint64 aProcessId)
>+{
>+  GetSmsChild()->SendGetNextMessageInList(aListId, aRequestId,
>+                                          ContentChild::GetSingleton()->GetID());
>+  return NS_OK;
>+}
>+
> } // namespace sms
> } // namespace dom
> } // namespace mozilla
>diff --git a/dom/sms/src/ipc/SmsParent.cpp b/dom/sms/src/ipc/SmsParent.cpp
>--- a/dom/sms/src/ipc/SmsParent.cpp
>+++ b/dom/sms/src/ipc/SmsParent.cpp
>@@ -228,11 +228,25 @@ SmsParent::RecvCreateMessageList(const S
>   NS_ENSURE_TRUE(smsDBService, true);
> 
>   nsCOMPtr<nsIDOMMozSmsFilter> filter = new SmsFilter(aFilter);
>   smsDBService->CreateMessageList(filter, aReverse, aRequestId, aProcessId);
> 
>   return true;
> }
> 
>+bool
>+SmsParent::RecvGetNextMessageInList(const PRInt32& aListId,
>+                                    const PRInt32& aRequestId,
>+                                    const PRUint64& aProcessId)
>+{
>+  nsCOMPtr<nsISmsDatabaseService> smsDBService =
>+    do_GetService(SMS_DATABASE_SERVICE_CONTRACTID);
>+  NS_ENSURE_TRUE(smsDBService, true);
>+
>+  smsDBService->GetNextMessageInList(aListId, aRequestId, aProcessId);
>+
>+  return true;
>+}
>+
> } // namespace sms
> } // namespace dom
> } // namespace mozilla
>diff --git a/dom/sms/src/ipc/SmsParent.h b/dom/sms/src/ipc/SmsParent.h
>--- a/dom/sms/src/ipc/SmsParent.h
>+++ b/dom/sms/src/ipc/SmsParent.h
>@@ -58,16 +58,17 @@ public:
> 
>   NS_OVERRIDE virtual bool RecvHasSupport(bool* aHasSupport);
>   NS_OVERRIDE virtual bool RecvGetNumberOfMessagesForText(const nsString& aText, PRUint16* aResult);
>   NS_OVERRIDE virtual bool RecvSendMessage(const nsString& aNumber, const nsString& aMessage, const PRInt32& aRequestId, const PRUint64& aProcessId);
>   NS_OVERRIDE virtual bool RecvSaveSentMessage(const nsString& aRecipient, const nsString& aBody, const PRUint64& aDate, PRInt32* aId);
>   NS_OVERRIDE virtual bool RecvGetMessage(const PRInt32& aMessageId, const PRInt32& aRequestId, const PRUint64& aProcessId);
>   NS_OVERRIDE virtual bool RecvDeleteMessage(const PRInt32& aMessageId, const PRInt32& aRequestId, const PRUint64& aProcessId);
>   NS_OVERRIDE virtual bool RecvCreateMessageList(const SmsFilterData& aFilter, const bool& aReverse, const PRInt32& aRequestId, const PRUint64& aProcessId);
>+  NS_OVERRIDE virtual bool RecvGetNextMessageInList(const PRInt32& aListId, const PRInt32& aRequestId, const PRUint64& aProcessId);
> 
> protected:
>   virtual void ActorDestroy(ActorDestroyReason why);
> 
> private:
>   static nsTArray<SmsParent*>* gSmsParents;
> };
>
Attachment #580294 - Flags: review?(bugs) → review+
Attachment #580296 - Flags: review?(bugs) → review+
Attachment #580298 - Flags: review?(bugs) → review+
Attachment #580299 - Flags: review?(bugs) → review+
It is really hard to see how this all works since the patch is split to so many pieces.
(Assignee)

Comment 185

7 years ago
(In reply to Olli Pettay [:smaug] from comment #181)
> Comment on attachment 580291 [details] [diff] [review]
> Part AM - Implement SmsIterator
> 
> >+SmsIterator::Next()
> >+{
> >+  // No message means we are waiting for a message or we got the last one.
> >+  if (!mMessage) {
> >+    return NS_ERROR_DOM_INVALID_STATE_ERR;
> >+  }
> Quite surprising to throw here, I think.
> Could next() return boolean. false would mean iteration certainly couldn't
> proceed.

According to the spec if iterator.message is null, we are past the last message and we shouldn't try to call next() again. AFAIK, it's the same behavior than IndexedDB.
(Assignee)

Comment 186

7 years ago
After discussing with Olli on IRC, we are going to rename SmsIterator to SmsCursor and next() method to continue(). That way, things should be more understandable: there was a confusing with SmsIterator.next() being sync.
(Assignee)

Comment 187

7 years ago
With the following changes:
SmsIterator -> SmsCursor
next() -> continue()

All following patches in the queue have been updated locally to match the changes.
Attachment #580291 - Attachment is obsolete: true
Attachment #581646 - Flags: review?(bugs)
Comment on attachment 580297 [details] [diff] [review]
Part AS - Remove iterator.filter

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

r=me
Attachment #580297 - Flags: review?(jonas) → review+
Comment on attachment 581646 [details] [diff] [review]
Part AM - Implement SmsIterator


>+[scriptable, function, uuid(5000ce1d-2ed3-4be5-b34c-439907489995)]
>+interface nsIDOMMozSmsCursor : nsISupports
>+{
>+  readonly attribute nsIDOMMozSmsFilter  filter;
>+  // Can be null if there is no more results.
>+  readonly attribute nsIDOMMozSmsMessage message;
>+                                    void continue();

Could you document continue() a bit. At least mention in
which cases it may throw.
Attachment #581646 - Flags: review?(bugs) → review+
Comment on attachment 580289 [details] [diff] [review]
Part AK - getMessages(): create the message list (Android)

>diff --git a/dom/sms/src/Types.h b/dom/sms/src/Types.h

> // For SmsMessageDate.delivery.  +// Please keep this enum in sync
>with the constants in: +// embedding/android/GeckoSmsManager.cpp

GeckoSmsManager.java?  And shouldn't this be the other way, that
GeckoSmsManager.java needs to stay in sync with the values here?  I
think DOM should own these.

> enum DeliveryState { - eDeliveryState_Sent, + eDeliveryState_Sent =
>0,

Fyi, this isn't necessary, but being explicit here is fine.

>diff --git a/dom/sms/src/android/SmsDatabaseService.cpp
 b/dom/sms/src/android/SmsDatabaseService.cpp

>+ AndroidBridge::Bridge()->CreateMessageList( +
>static_cast<SmsFilter*>(aFilter)->GetData(), aReverse, aRequestId,
>aProcessId

This static cast is a bit scary.  Is nsIDOMMozSmsFilter declared so
that script can't override it?

I don't know what |aReverse| means here.  An enum would be clearer.

>diff --git a/embedding/android/GeckoSmsManager.java
 b/embedding/android/GeckoSmsManager.java

>+ restrictions.add("type = " + kSmsTypeInbox); + } else { + throw new
>UnexpectedDeliveryStateException();

Would like error handling moved from the catch block to this alternate
case.

>diff --git a/widget/src/android/AndroidBridge.cpp
 b/widget/src/android/AndroidBridge.cpp

>+void +AndroidBridge::CreateMessageList(const
>dom::sms::SmsFilterData& aFilter, bool aReverse, + PRInt32
>aRequestId, PRUint64 aProcessId) +{ +
>ALOG_BRIDGE("AndroidBridge::CreateMessageList"); +

Missing

  AutoLocalJNIFrame jniFrame;

>+ for (PRUint32 i=0; i<aFilter.numbers().Length(); ++i) {

Nit: |i = 0|, |i < |.

r- for missing AutoLocalJNIFrame.  Otherwise OK with other fixes.
Attachment #580289 - Flags: review?(jones.chris.g) → review-
Comment on attachment 580290 [details] [diff] [review]
Part AL - getMessages(): mechanism to store created cursor in the Android backend

>diff --git a/embedding/android/GeckoSmsManager.java b/embedding/android/GeckoSmsManager.java

>+  public void clear() {
>+    for (int i=0; i<mCursors.size(); ++i) {
>+      Cursor c = mCursors.get(i);
>+      if (c == null) {
>+        continue;
>+      }
>+
>+      c.close();

Nit:
     Cursor c = mCursors.get(i);
     if (c != null) {
         c.close();
     }

It's time to start thinking about factoring out this pattern into a
java-generic-ized (~templated) helper class.

>-        Cursor c = null;
>+        Cursor cursor = null;
>+        boolean dontCloseCursor = false;
> 

|closeCursor = true|.  Double negatives generally don't make code not
less harder to read.

>+          } else {
>+            throw new UnexpectedDeliveryStateException();

This is the first exception thrown multiple times, but even here,
inlining the error handling allows you to write a better error
message.  So please do that.

>+          dontCloseCursor = true;
>+          int listId = MessagesListManager.getInstance().add(cursor);

Reverse these statements.  .add() can throw RuntimeExceptions.

r=me with those fixes.
Attachment #580290 - Flags: review?(jones.chris.g) → review+
Comment on attachment 580292 [details] [diff] [review]
Part AN - getMessages(): Handling no messages in created message list

>diff --git a/dom/sms/src/ipc/SmsChild.cpp b/dom/sms/src/ipc/SmsChild.cpp

>+bool
>+SmsChild::RecvNotifyRequestNoMessageInList(const PRInt32& aRequestId,
>+                                           const PRUint64& aProcessId)
>+{
>+  if (ContentChild::GetSingleton()->GetID() != aProcessId) {
>+    return true;
>+  }
>+
>+  SmsRequestManager::GetInstance()->NotifyNoMessageInList(aRequestId);
>+  return true;
>+}

I prefer 

  if (ContentChild::GetSingleton()->GetID() == aProcessId) {
    SmsRequestManager::GetInstance()->NotifyNoMessageInList(aRequestId);
  }
  return true;

but not particularly strongly.
Attachment #580292 - Flags: review?(jones.chris.g) → review+
Comment on attachment 580293 [details] [diff] [review]
Part AO - getMessages(): Get first message in created list

>diff --git a/dom/sms/src/SmsIterator.cpp b/dom/sms/src/SmsIterator.cpp

Didn't review this.
Attachment #580293 - Flags: review?(jones.chris.g) → review+
Comment on attachment 580295 [details] [diff] [review]
Part AQ - getMessages(): read the next message in the Android backend

>diff --git a/dom/sms/src/SmsRequestManager.cpp b/dom/sms/src/SmsRequestManager.cpp
 
>+void
>+SmsRequestManager::NotifyGotNextMessage(PRInt32 aRequestId, nsIDOMMozSmsMessage* aMessage)
>+{
>+  // TODO: implement
>+  printf_stderr("\nHERE\n\n");

Eh?

>diff --git a/embedding/android/GeckoAppShell.java b/embedding/android/GeckoAppShell.java

>+
>+    public static void getNextMessageInList(int aListId, int aRequestId, long aProcessId) {
>+        GeckoSmsManager.getNextMessageInList(aListId, aRequestId, aProcessId);
>+    }
> }

>diff --git a/embedding/android/GeckoSmsManager.java b/embedding/android/GeckoSmsManager.java

>+      public void run() {

>+          int type = cursor.getInt(cursor.getColumnIndex("type"));
>+          String sender = "";
>+          String receiver = "";
>+
>+          if (type == kSmsTypeInbox) {
>+            sender = cursor.getString(cursor.getColumnIndex("address"));
>+          } else if (type == kSmsTypeSentbox) {
>+            receiver = cursor.getString(cursor.getColumnIndex("address"));
>+          } else {
>+            throw new UnexpectedDeliveryStateException();

Would like error handling inlined.

>+          }

Can we refactor some of this code?
Attachment #580295 - Flags: review?(jones.chris.g) → review+
Comment on attachment 580298 [details] [diff] [review]
Part AT - getMessages(): handle failures

>diff --git a/dom/sms/src/SmsRequestManager.cpp b/dom/sms/src/SmsRequestManager.cpp

>+  nsCOMPtr<nsIDOMMozSmsIterator> iterator = request->GetIterator();
>+  if (iterator) {
>+    static_cast<SmsIterator*>(iterator.get())->Disconnect();

nsIDOMMozSmsIterator is adequately non-script etc?

>diff --git a/dom/sms/src/ipc/PSms.ipdl b/dom/sms/src/ipc/PSms.ipdl

>+    NotifyRequestReadListFailed(PRInt32 aError, PRInt32 aRequestId,
>+                                PRUint64 aProcessId);

The error should be an enum.  Fix in a followup.
Attachment #580298 - Flags: review?(jones.chris.g) → review+
Attachment #580299 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 196

7 years ago
Sadly this r- is going to prevent this code to go in Firefox 11. No big deal but that would have been nice I guess.
Attachment #580289 - Attachment is obsolete: true
Attachment #582768 - Flags: review?(jones.chris.g)
Sorry, c'est la guerre :/.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #190)
> Comment on attachment 580289 [details] [diff] [review]
> Part AK - getMessages(): create the message list (Android)
> 
> >diff --git a/dom/sms/src/Types.h b/dom/sms/src/Types.h
> 
> > // For SmsMessageDate.delivery.  +// Please keep this enum in sync
> >with the constants in: +// embedding/android/GeckoSmsManager.cpp
> 
> GeckoSmsManager.java?  And shouldn't this be the other way, that
> GeckoSmsManager.java needs to stay in sync with the values here?  I
> think DOM should own these.
> 

You didn't address this.

> >diff --git a/dom/sms/src/android/SmsDatabaseService.cpp
>  b/dom/sms/src/android/SmsDatabaseService.cpp
> 
> >+ AndroidBridge::Bridge()->CreateMessageList( +
> >static_cast<SmsFilter*>(aFilter)->GetData(), aReverse, aRequestId,
> >aProcessId
> 
> This static cast is a bit scary.  Is nsIDOMMozSmsFilter declared so
> that script can't override it?
> 
> I don't know what |aReverse| means here.  An enum would be clearer.
> 

Didn't address this.

JNIFrame fix looks ok.  Are you going to fix the above in this patch or a followup?
Attachment #582768 - Flags: review?(jones.chris.g)
(Assignee)

Comment 198

7 years ago
The comment is a bit changed. For aReverse, I will address this in a follow-up.
Attachment #582768 - Attachment is obsolete: true
Attachment #583088 - Flags: review?(jones.chris.g)
Comment on attachment 583088 [details] [diff] [review]
Part AK - getMessages(): create the message list (Android)

Typo and slightly misleading comment fixed locally, followups for the rest.
Attachment #583088 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 200

7 years ago
I have to do that because AndroidBridge is declaring methods that are not available in the native version of our Android browser.

This is basically a copy-paste of the code that has been added in embedding/android/. This code has already been reviewed by cjones.

I think we should whether find a way to not duplicate code between those two directories or deprecate WebSMS in embedding/android. For the moment the cost is low but I'm afraid it will grow high soon.
Attachment #583155 - Flags: review?(mark.finkle)
Attachment #583155 - Flags: review?(mark.finkle) → review?(blassey.bugs)
dougt was looking at hooking some of the old embedding/android impls into the new frontend.  He might have some ideas on sharing the code.
Comment on attachment 576174 [details] [diff] [review]
Part O - Receiving SMS, Android backend

where did you check this in? it needs review from an android widget peer before it lands on mozilla-central
Attachment #576174 - Flags: review-
Comment on attachment 583155 [details] [diff] [review]
Part AV - Port WebSMS to Android Native

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

::: mobile/android/base/AndroidManifest.xml.in
@@ +24,5 @@
> +    <!-- WebSMS -->
> +    <uses-permission android:name="android.permission.SEND_SMS"/>
> +    <uses-permission android:name="android.permission.RECEIVE_SMS"/>
> +    <uses-permission android:name="android.permission.WRITE_SMS"/>
> +    <uses-permission android:name="android.permission.READ_SMS"/>

this needs to be wrapped in an #ifdef, we don't want to ship with these permission requests. or at least need the option not to

::: mobile/android/base/GeckoAppShell.java
@@ +146,5 @@
> +    public static native void notifySmsDeleteFailed(int aError, int aRequestId, long aProcessId);
> +    public static native void notifyNoMessageInList(int aRequestId, long aProcessId);
> +    public static native void notifyListCreated(int aListId, int aMessageId, String aReceiver, String aSender, String aBody, long aTimestamp, int aRequestId, long aProcessId);
> +    public static native void notifyGotNextMessage(int aMessageId, String aReceiver, String aSender, String aBody, long aTimestamp, int aRequestId, long aProcessId);
> +    public static native void notifyReadingMessageListFailed(int aError, int aRequestId, long aProcessId);

all of this should go through the generic message passing API

@@ +1574,5 @@
> +        GeckoSmsManager.getNextMessageInList(aListId, aRequestId, aProcessId);
> +    }
> +
> +    public static void clearMessageList(int aListId) {
> +        GeckoSmsManager.clearMessageList(aListId);

again, message passing API
Attachment #583155 - Flags: review?(blassey.bugs) → review-
(Assignee)

Comment 204

7 years ago
(In reply to Brad Lassey [:blassey] from comment #203)
> Comment on attachment 583155 [details] [diff] [review]
> Part AV - Port WebSMS to Android Native
> 
> Review of attachment 583155 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/AndroidManifest.xml.in
> @@ +24,5 @@
> > +    <!-- WebSMS -->
> > +    <uses-permission android:name="android.permission.SEND_SMS"/>
> > +    <uses-permission android:name="android.permission.RECEIVE_SMS"/>
> > +    <uses-permission android:name="android.permission.WRITE_SMS"/>
> > +    <uses-permission android:name="android.permission.READ_SMS"/>
> 
> this needs to be wrapped in an #ifdef, we don't want to ship with these
> permission requests. or at least need the option not to

Yes, we have to figure out a plan. For the moment, it's far from being in a version in the market. The easiest idea might be to have #if that makes it available with Aurora and Nightly versions (or maybe only Nightly?). My idea was to handle that in a follow-up.

> ::: mobile/android/base/GeckoAppShell.java
> @@ +146,5 @@
> > +    public static native void notifySmsDeleteFailed(int aError, int aRequestId, long aProcessId);
> > +    public static native void notifyNoMessageInList(int aRequestId, long aProcessId);
> > +    public static native void notifyListCreated(int aListId, int aMessageId, String aReceiver, String aSender, String aBody, long aTimestamp, int aRequestId, long aProcessId);
> > +    public static native void notifyGotNextMessage(int aMessageId, String aReceiver, String aSender, String aBody, long aTimestamp, int aRequestId, long aProcessId);
> > +    public static native void notifyReadingMessageListFailed(int aError, int aRequestId, long aProcessId);
> 
> all of this should go through the generic message passing API
> 
> @@ +1574,5 @@
> > +        GeckoSmsManager.getNextMessageInList(aListId, aRequestId, aProcessId);
> > +    }
> > +
> > +    public static void clearMessageList(int aListId) {
> > +        GeckoSmsManager.clearMessageList(aListId);
> 
> again, message passing API

What is the message passing API?!
(In reply to Brad Lassey [:blassey] from comment #202)
> Comment on attachment 576174 [details] [diff] [review]
> Part O - Receiving SMS, Android backend
> 
> where did you check this in? it needs review from an android widget peer
> before it lands on mozilla-central

This isn't android widget code.  If that's the only reason you r-'d, please un-r-.  Otherwise, list your specific concerns.
(Assignee)

Comment 206

7 years ago
Comment on attachment 583155 [details] [diff] [review]
Part AV - Port WebSMS to Android Native

The two raised concerned with this patch were:
1. Permissions should not go to the market;
2. Use "message API".

For 1, as I said, we will have to figure out a plan but my idea was to do that in a follow-up. This code is far away from the market. The most possible plan would be to use a #if statement that would make it Nightly only.

For the second issue, I don't understand how this "message API" is better than classic JNI code. For what I understand, those messages are sent to browser.js with JSON objects. To me, it's one kind of boilerplate vs another: both seems to be used and I would prefer to use the one that is already written. If the plan of the mobile team is to remove all JNI calls in favor of that API, we can discuss this in a follow-up.
As a side note, I don't really like the idea of a Gecko feature that has to be implemented in browser.js (which isn't part of Gecko).
Attachment #583155 - Flags: review- → review?(blassey.bugs)
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #206)
> As a side note, I don't really like the idea of a Gecko feature that has to
> be implemented in browser.js (which isn't part of Gecko).

Big +1 to that.  Not doing that has already bitten us.  (Not blaming past work that was driven by need, but let's not move further away from the ideal.)
(Assignee)

Updated

7 years ago
Depends on: 712683
(In reply to Chris Jones [:cjones] [:warhammer] from comment #205)
> (In reply to Brad Lassey [:blassey] from comment #202)
> > Comment on attachment 576174 [details] [diff] [review]
> > Part O - Receiving SMS, Android backend
> > 
> > where did you check this in? it needs review from an android widget peer
> > before it lands on mozilla-central
> 
> This isn't android widget code.  If that's the only reason you r-'d, please
> un-r-.  
embedding/android and widget/src/android are android widget code
(In reply to Chris Jones [:cjones] [:warhammer] from comment #207)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #206)
> > As a side note, I don't really like the idea of a Gecko feature that has to
> > be implemented in browser.js (which isn't part of Gecko).
> 
> Big +1 to that.  Not doing that has already bitten us.  (Not blaming past
> work that was driven by need, but let's not move further away from the
> ideal.)

no one is talking about putting anything in browser.js
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #206)
> Comment on attachment 583155 [details] [diff] [review]
> Part AV - Port WebSMS to Android Native
> 
> As a side note, I don't really like the idea of a Gecko feature that has to
> be implemented in browser.js (which isn't part of Gecko).

Messages are passed via. the observer service, and sent via an nsIAndroidBridge component. There's no reason that browser.js needs to be involved.
(Assignee)

Comment 211

7 years ago
I've been told in #mobile stuff had to be hooked-up in browser.js. I guess I had wrong information or I misunderstood them. Though, my points still stay: why should we change working boilerplate to another kind of boilerplate while some code is already written and reviewed?
For the moment, the only reason I had (on #mobile again) was that I'm polluting GeckoAppShell. That doesn't seem a strong point. In addition, when I began working on WebSMS, I pointed that I will be polluting GeckoAppShell and I've been told that was the point of it (Native UI project didn't exist at the time IIRC).
Woah, long bug is long :)

While I don't understand the technical details, I'm sure everybody's concerns are valid and that we can come to an agreement pretty quickly. In the meantime, would it be acceptable to do that and the remaining unreviewed patch (part AV) in follow-up bugs? This would allow us to land the enormous patch queue from this bug now, which would be very helpful as it would unblock other people. -- We all want the same thing: move fast and do things right. :)
(In reply to Brad Lassey [:blassey] from comment #208)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #205)
> > (In reply to Brad Lassey [:blassey] from comment #202)
> > > Comment on attachment 576174 [details] [diff] [review]
> > > Part O - Receiving SMS, Android backend
> > > 
> > > where did you check this in? it needs review from an android widget peer
> > > before it lands on mozilla-central
> > 
> > This isn't android widget code.  If that's the only reason you r-'d, please
> > un-r-.  
> embedding/android and widget/src/android are android widget code

No, the code these patches touch are JNI and gecko things that happen to be implemented in Java.  That some bits of code happen to live in widget/src/android is incidental.

Claiming all java/android things are android widgetry is like saying that win32 networking code is win32 widgetry, and needs to be reviewed by a win32 widget peer, because it uses the win32 API.

We're still waiting to hear specific concerns you have.  This r- is partly blocking WebSMS right now.
(In reply to Wesley Johnston (:wesj) from comment #210)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #206)
> > Comment on attachment 583155 [details] [diff] [review]
> > Part AV - Port WebSMS to Android Native
> > 
> > As a side note, I don't really like the idea of a Gecko feature that has to
> > be implemented in browser.js (which isn't part of Gecko).
> 
> Messages are passed via. the observer service, and sent via an
> nsIAndroidBridge component. There's no reason that browser.js needs to be
> involved.

That's a poor architectural choice, because extensions and other random code can register to listen to those observer service broadcasts.  That means the code needs to be hardened against arbitrary re-entry and interactions with other parts of the system.

What's the concrete advantage of using the observer service instead of JNI?  JNI gives static typing and statically-understood interactions.
Comment on attachment 583155 [details] [diff] [review]
Part AV - Port WebSMS to Android Native

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

still r-, I don't want these permissions. As a consequence we also shouldn't build GeckoSmsManager. I also greatly prefer using the message passing API.
Attachment #583155 - Flags: review?(blassey.bugs) → review-
(Assignee)

Comment 216

7 years ago
(In reply to Brad Lassey [:blassey] from comment #215)
> Comment on attachment 583155 [details] [diff] [review]
> Part AV - Port WebSMS to Android Native
> 
> Review of attachment 583155 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> still r-, I don't want these permissions.

They are not going to hit the market. That should be enough, isn't it?
We need them in Nightly at least (maybe Aurora).

> As a consequence we also shouldn't build GeckoSmsManager.

I do not understand the point of that.

> I also greatly prefer using the message passing API.

Why? What is the advantages of this API or the disadvantages or using JNI calls?
Also, the reason why I had to ask that patch to be reviewed is that AndroidBridge is built for both XUL and Native UI which make Native UI fails to run (AndroidBridge init fails). If we don't use the calls from AndroidBridge, it will not solve that problem...
(In reply to Chris Jones [:cjones] [:warhammer] from comment #214)
> (In reply to Wesley Johnston (:wesj) from comment #210)
> > (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #206)
> > > Comment on attachment 583155 [details] [diff] [review]
> > > Part AV - Port WebSMS to Android Native
> > > 
> > > As a side note, I don't really like the idea of a Gecko feature that has to
> > > be implemented in browser.js (which isn't part of Gecko).
> > 
> > Messages are passed via. the observer service, and sent via an
> > nsIAndroidBridge component. There's no reason that browser.js needs to be
> > involved.
> 
> That's a poor architectural choice, because extensions and other random code
> can register to listen to those observer service broadcasts.  That means the
> code needs to be hardened against arbitrary re-entry and interactions with
> other parts of the system.
> 
> What's the concrete advantage of using the observer service instead of JNI? 
> JNI gives static typing and statically-understood interactions.
Sending a message to Java-land has nothing to do with the observer system, you call nsIAndroidBridge->handleGeckoMessage(). 

For sending a message from Java-land to C++ there are two options. One is the observer system, but I don't think that's a particularly good choice here. The better choice would be to add an new event type to GeckoEvent and send that through GeckoAppShell.sendEventToGecko().

The major advantage of using the message passing system is to make this code easier to maintain. Prior to the introduction of the message passing system GeckoAppShell and AndroidBridge was growing uncontrollably. Using the message passing system allows things to be more modular, with less boilerplate code.