774621 - B2G SMS: save error messages before sendSMS()

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect, P1)

Description

[Vicamo Yang [:vicamo][:vyang]]

Messages failed to reach their destinations are now dropped without further processing. To have better error handling, outgoing messages should be saved first and allow later retrieval. Gaia developer, Steve, had implemented an additional data store to meet the requirement. However, in order to have a united database, this function would be better built into Gecko.

This task came from a feature request of Gaia developers. See also https://github.com/mozilla-b2g/gaia/pull/2410

Comment 1

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 1: add sending/pending/failed delivery state in IDL

Comment 2

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 2: implement extra delivery states

Comment 3

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 3: extra delivery states in android backend

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 4: implement sending/pending/failed delivery state for B2G_RIL

There seems to be a lot of things to be done in android build and I'm still not really familiar with our android backend implementation. Could you please give me some hints or suggestions? Thank you.

Comment 5

[Mounir Lamouri (:mounir)]

Comment on attachment 642914 [details] [diff] [review]
Part 1: add sending/pending/failed delivery state in IDL

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

The discussion I had and what I agreed on is that we would add a "delivery-failure" event and that might come with new 'delivery'/'status' values like 'delivered'/'not-delivered'. I'm still strongly opposed to a "draft" support in the API. That should be handled by the app.

Comment 6

[Philipp von Weitershausen [:philikon]]

Comment on attachment 642919 [details] [diff] [review]
Part 4: implement sending/pending/failed delivery state for B2G_RIL

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

Going to clear this given Mounir's concerns.

Comment 7

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Mounir Lamouri (:mounir) from comment #5)
> Comment on attachment 642914 [details] [diff] [review]
> Part 1: add sending/pending/failed delivery state in IDL
> 
> Review of attachment 642914 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The discussion I had and what I agreed on is that we would add a
> "delivery-failure" event and that might come with new 'delivery'/'status'
> values like 'delivered'/'not-delivered'. I'm still strongly opposed to a
> "draft" support in the API. That should be handled by the app.

I'm not aware of the conclusions/reasons of the previous discussion. Could you please give some more detailed thoughts again on it? And, The proposed "sending" delivery state is not a "draft" state. "sending" means the app had invoked send() with the message, while "draft" doesn't.

AFAIK, Android has 6 states: draft/out(sending)/sent/failed/queued(pending)/received in its SMS database API. It somehow gives a hint that a mature SMS application needs these states to be completed. However, in B2G we get only "sent" and "received" and that's why UX developers have to implement their own pending database in Gaia temporarily. Without these new states built into Gecko, you can see there is a lot of duplicated code in gaia/apps/sms/js/pendingDBUtils.js, and there is still more to come. Every app that sends SMS messages out has to implement similar feature.

If we don't want to complete database functions inside Gecko, why did we build it into Gecko in the first place at all? We could remove all SmsDatabaseService code and let application mind their own database business. That way they'll have to manipulate only one database.

Comment 8

[Borja Salguero [:borjasalguero]]

Totally agree with Vicamo... We should have a common and shared way to manage sms, and this issue should be handled by System (even Nokia 3310 does!) as other OS are working. We need a stable and robust way of storing this messages in order to keep a reliable version of SMS DB and a complete API.

We have made a huge effort to include this functionality in App, but I think it's time to go a step forward and include it in  System, letting to future developers to manage SMS as Android does, with a reliable SMS Api which manage different states. Also changes in status (from draft/sending to sent) should be managed by SMS App, as we do in Telephony API (https://wiki.mozilla.org/WebAPI/WebTelephony has states as "dialing", "alerting", "busy", "connecting", "connected", "disconnecting"....). 

Persistence of draft/sending sms is crucial for having a robust SMS Api, and it's one of main action points for increase performance in our SMS App.

Comment 9

[Borja Salguero [:borjasalguero]]

* should be managed by SMS API, not SMS App ;)

Comment 10

[Steve Chung [:steveck]]

    If the message is a "draft", I agree that app should handle it because this message might be incomplete and it's unnecessary to store draft in SmsDatabase or using any Sms webAPI to access the draft.
    But the purpose of this issue is focused on the ability of handling the sent failed messages. If app need to handle message database by themselves, it's also reasonable to handle the event and store the 'delivered'/'not-delivered' message in message DB. In our sms architecture, we maintain the SmsDatabase in gecko platform, front-end developer might think that sent messages are all accessible in SmsDatabase because no matter the message is delivered or not, message object will callback in request result. Failed message handling should be common for message app and it's a little bit inconvenient for message app developer to implement database for not-delivered messages. Since it already exist in platform, it would be a great help for front-end that we don't have to implement/manage a database and improve the performance(if we need to get messages from different DB, we could not get the messages with required timestamp sequence and update UI directly). If platform has any concern about adding the sent failed message in SmsDatabase, please feel free to raise it. :)

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

Add more DOM peers for more opinions/concerns about extending SMS DOM API. Please give your precious feedback. Thanks in advance!

Comment 12

[Fernando Campo (:fcampo)]

I think that we all agree on the different states (sending/pending/failed) treated by the API, and the 'draft' state treated by the app, so I don't see the reason for the discussion, except probably a missunderstanding at the start point.

The app should take care of whatever states we want to use for the message BEFORE we click the 'send' button. We can create a 'draft' if we want to recover the data in case the app gets closed, or we can do nothing, that's up to UX.

But after that, once the message goes to the RIL, the app have no control over the message, or the data flow, so I really think the API should provide more info on the current state to let the app treat the situation properly.

If anyone is still opposed to this treatment, or think mine is wrong, please specify your reasons so we can understand.

Comment 13

[Henri Sivonen (:hsivonen)]

(In reply to Vicamo Yang [:vicamo] from comment #11)
> Add more DOM peers for more opinions/concerns about extending SMS DOM API.
> Please give your precious feedback. Thanks in advance!

I don't think it's safe to assume that DOM peers are familiar with the design rationale of the SMS API. At least I am not.

Logically, it seems that the minimal added API surface to the Web platform would be a method for sending messages, an event handler for receiving messages and an API for managing messages stored on a SIM.

On the face of things, I find it weird to add special-purpose databases to the Web platform, though I can see why having a special-purpose database for received messages could help reduce the amount of JS that needs to be resident at all times. So I find having platform-level databases for draft messages and sent messages roughly equally weird. (For sure, it would be very weird to bake an API for local email storage into the Web platform.) That said, I can see why it might be desirable to have shared storage among potential alternative SMS apps on B2G. It's hard to comment out of the blue without knowing the design motivations of the API.

What's Tizen doing about SMS storage?

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Henri Sivonen (:hsivonen) from comment #13)
> (In reply to Vicamo Yang [:vicamo] from comment #11)
> > Add more DOM peers for more opinions/concerns about extending SMS DOM API.
> > Please give your precious feedback. Thanks in advance!
> 
> I don't think it's safe to assume that DOM peers are familiar with the
> design rationale of the SMS API. At least I am not.

Thanks for your opinions.

I know. In the contrary, people who read SMS specification a lot won't necessary know what's the key points or judgement rules of a good SMS API proposal. So we have to communicate more, to exchange knowledge only known by others and finally get things done in an acceptable way.

> Logically, it seems that the minimal added API surface to the Web platform
> would be a method for sending messages, an event handler for receiving
> messages and an API for managing messages stored on a SIM.

For Android, the core framework supports only SMS sending/notification interfaces. Although database column definitions are built into core framework, the owner of SMS database is actually TelephonyProvider. The SMS application(Mms) manipulates SMS/MMS messages directly without interferences from core framework.

> On the face of things, I find it weird to add special-purpose databases to
> the Web platform, though I can see why having a special-purpose database for
> received messages could help reduce the amount of JS that needs to be
> resident at all times. So I find having platform-level databases for draft
> messages and sent messages roughly equally weird. (For sure, it would be
> very weird to bake an API for local email storage into the Web platform.)
> That said, I can see why it might be desirable to have shared storage among
> potential alternative SMS apps on B2G. It's hard to comment out of the blue
> without knowing the design motivations of the API.
> 
> What's Tizen doing about SMS storage?

http://meego.gitorious.org/meego-handset-ux/meego-handset-sms/blobs/master/src/smsdata.h
I didn't spend much time tracing Tizen/Meego. Correct me if I'm wrong.

Tizen/Meego has SmsService and SmsApplication communicating to each other by dbus. Its backend is actually integrated into some kind of mail message store. The store is not directly owned by the SmsApplication, but do allow modifications from permitted clients. Each message in that store has a field named `status`, which is a bitmask for incoming/outgoing/draft. There is still another customized field, also named as `status`, for delivery states pending/sent/failed/delivered/deliver_failed. This design is very similar to Android's for me.

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Fernando Campo from comment #12)
> The app should take care of whatever states we want to use for the message
> BEFORE we click the 'send' button. We can create a 'draft' if we want to
> recover the data in case the app gets closed, or we can do nothing, that's
> up to UX.

Both Android and Meego have "draft" state in their message database. I'm not quite sure why we have to explicitly remove this state from API, but I'm not strongly opposed to this.

Comment 16

[Mounir Lamouri (:mounir)]

Do we want to make this bug a blocker for v1? If not, we should probably have that discussion later when everybody will have more time to dedicate.
Definitely, 'draft' status shouldn't be blocker for v1 because the app can handle that itself. Regarding the 'delivery-sent' status, I think we should indeed add that to the API but is that something that should block v1?

Regarding the comparison with Android/Meego/whatever, you should be careful: there is a big difference between an API and internal states. Android doesn't provide an API to store SMS. The public API only allows you to be notified when an SMS is received and allows you to send SMS. We have added storage so a user can switch from an application to another easily. Drafts are by nature temporary and could be considered as part of the application. We could discuss this but I would prefer if we could post-pone that to after v1.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So a few comments.

First of all, is handling delivery-failures a v1 blocker? I have no idea and defer the discussion to the PMs. But if it's not a v1 blocker, we should make sure that anyone that can help out with blockers do that before spending time on this bug. Nominating for blocker so that we can get some decision.

Second, I agree it makes sense that as soon as we've fired a "success" event after someone calls .send() the API is effectively "taking ownership" of the message. I.e. we should from that point out keep it in the APIs database. However if "error" is fired on the request to send() the message, then we should *not* add it to the database.

Third, I think the way to do this is to have a .status or .deliveryStatus property on messages which is one of "received", "sent", "delivered" or "deliveryfailure". Or something like that. Probably also with a property which indicates when the message was delivered or failed to be delivered.

Comment 18

[Boris Zbarsky [:bzbarsky]]

My main comment on the API is that we should move it to WebIDL and use enums as needed.  ;)

Comment 19

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Mounir Lamouri (:mounir) from comment #16)
> Regarding the comparison with Android/Meego/whatever, you should be careful:
> there is a big difference between an API and internal states. Android
> doesn't provide an API to store SMS. The public API only allows you to be
> notified when an SMS is received and allows you to send SMS.

No, that's not true. Android built SMS database column definitions into its framework API, and that's why the SMS application imports `android.provider.Telephony.java`. Android provides generic database access Content API, together with public database column definitions, I don't think the data abstraction of SMS database is simply some internal state. Every field in it is pre-defined, its possible values are pre-defined and any application has the SMS_READ/WRITE permission can access SMS database in the same way.

For Meego, they use a unified message store to save all SMS messages. The message store has to support customized fields to complete features needed by SMS app. It's true that there is no strong definition for these customized fields, they are dictionary entries, key and value. But I'd rather say that's a natural limit of its QMailMessage class. An object created from a generic store always has some customized dictionary structure, or it won't be generic.

Comment 20

[Henri Sivonen (:hsivonen)]

My question about Tizen was meant to be about JS APIs in Tizen (to the extent Tizen is becoming a "Boot2WebKit" platform). I didn't mean to suggest that comparisons with Meego C/C++ APIs would be useful. After all, if we want to standardize this stuff, we'll probably have to take into account what Tizen is doing if they want to standardize, too.

Comment 21

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Henri Sivonen (:hsivonen) from comment #20)
> My question about Tizen was meant to be about JS APIs in Tizen (to the
> extent Tizen is becoming a "Boot2WebKit" platform). I didn't mean to suggest
> that comparisons with Meego C/C++ APIs would be useful. After all, if we
> want to standardize this stuff, we'll probably have to take into account
> what Tizen is doing if they want to standardize, too.

For WebKit-based platforms, there doesn't seem to be any initial work for WebSMS, at least I can't find any related key word in its trunk tree. However, WebKit does support sms:// protocol, but it has nothing to do with the DOM API.

http://dev.opera.com/articles/view/opera-mini-web-content-authoring-guidelines/#operamini-sms
Neither does OperaMini.

Comment 22

[Henri Sivonen (:hsivonen)]

(In reply to Vicamo Yang [:vicamo] from comment #21)
> For WebKit-based platforms, there doesn't seem to be any initial work for
> WebSMS, at least I can't find any related key word in its trunk tree.

Tizen's additional APIs are not on WebKit trunk. It seems they are at:
https://review.tizen.org/git/?p=pkgs/w/wrt-plugins-tizen.git;a=tree;f=src/standards/Tizen;h=d207d3b07fd4465537617922f035b936d9d61078;hb=eccb4b05019c419d66978179f44297c7b0e79710

Comment 23

[Henri Sivonen (:hsivonen)]

And the API docs are at
https://developer.tizen.org/help/index.jsp?topic=%2Forg.tizen.help.web.devguide%2FDevice+API+Guide.html

Comment 25

[Borja Salguero [:borjasalguero]]

We were talking about some issues about performance and one of the key point was having a common way of storing 'failed' messages. Is not a 'draft', it's the concept of messages that are not sent due to any error in RIL or whatever, so we need to have the capability or resending it (updating the status). Now it's inside SMS App, but having it in Gecko will optimize the process of merging the info from API (now it's gonna be optimized) and the local indexedDB. This is a great increase in performance, and the code it's here! Why dont we review & merge it?

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I agree that this is something that we should fix. But let's look at what performance we get with the other proposed changes to the SMS API before we mark this as blocking.

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Is this about outgoing or error messages? I'm definitely convinced that it makes sense to add error messages to the database.

I'm not yet sure that it makes sense to add outgoing messages without also adding callbacks to handle things the transition from outgoing to error so that the app can notify the user. And such a callback probably needs to be system-message based.

In other words, what exactly are we planning on doing for outgoing messages in v1?

Comment 28

[Borja Salguero [:borjasalguero]]

[:sicking] This is related with error messages. While a sms is being sent (during the seconds while the RIL is sending the SMS), how this SMS is stored in the DB of Gecko? If I close the App while is 'sending', How could I retrieve this 'message' (imagine that it's 'sending' yet) to the thread list?  Thanks!

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I agree that putting error messages in the database is a blocker.

I'm fine with putting outgoing messages in the database as well, but that doesn't seem as high-priority to me. The effect of not putting an outgoing message in the database is that if the user manually kills the SMS app and then restarts it the message wouldn't show up for a few seconds. That doesn't seem like a big deal.

If we are putting outgoing messages into the database we need to also add events for when the message transitions from "outgoing" state to "error" or "sent" state. Do the patches here do that?

Comment 30

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Jonas Sicking (:sicking) from comment #29)
> I agree that putting error messages in the database is a blocker.
> 
> I'm fine with putting outgoing messages in the database as well, but that
> doesn't seem as high-priority to me. The effect of not putting an outgoing
> message in the database is that if the user manually kills the SMS app and
> then restarts it the message wouldn't show up for a few seconds. That
> doesn't seem like a big deal.
> 
> If we are putting outgoing messages into the database we need to also add
> events for when the message transitions from "outgoing" state to "error" or
> "sent" state. Do the patches here do that?

No, the patches here are out-dated especially after having deliveryStatus. The feature requests I got for MMS, not SMS, have following delivery states:

0) draft
1) received
2) sending - the MMS dispatcher is now sending the message
3) pending - the MMS dispatcher had tried to send to message but encountered some network errors and will try again later
4) re-sending - the MMS dispatcher is now re-sending a pended message.
5) sent
6) error

In summary (combined with SMS):

  delivery state  | delivery status
  ----------------+-----------------
  received(inbox) | success, pending(deferred)
  ----------------+-----------------
  ???(outbox)     | sending, pending, re-sending
  ----------------+-----------------
  sent(sentbox)   | pending(deferred), success(retrieved), error(expired, rejected,
                  | unreachable), not-applicable(unrecognised, indeterminate)
  ----------------+-----------------
  sent failed(?)  | not-applicable

Comment 31

[Vicamo Yang [:vicamo][:vyang]]

[received, pending], [out, sending], [out, pending], [out, re-sending], [failed, not-applicable]: unimplemented yet

[received, pending] is actaully MMS-specific
[out, sending], [failed, not-applicable] are easy targets
[out, pending], [out, re-sending] depends on more complex retry-scheme like allowing an user to willingly re-send a currently pending message.

Comment 32

[Borja Salguero [:borjasalguero]]

For SMS App we need at least the following states:
- received (Implemented)
- sent (Implemented)
- sending (Not implemented-easy target)
- error/failed (Not implemented-easy target)

We could implement the 'retry-scheme' in Gaia as we are doing now until having it in Gecko, but for V1 we need only the status mentioned before. Thanks!

Comment 33

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 1/4: add queued/failed delivery state

Comment 34

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 681946 [details] [diff] [review]
Part 1/4: add queued/failed delivery state

Missing onsending/onsendfailed event targets.

Comment 35

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 1: IDL changes : v1

1) Add onsending & onfailed event in SmsManager
2) Update possible values for SmsMessage.deliveryStatus in different delivery states.

Comment 36

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 2: DOM implementation

1) Remove backend dependent methods in nsISmsDatabaseService. Removed are saveReceivedMessage, saveSentMessage, and setMessageDeliveryStatus. I removed them because:
1.1) saveReceivedMessage: it's not used in Android's backend
1.2) saveSentMessage: we need saveSendingMessage instead in B2G
1.3) setMessageDeliveryStatus: not necessary for Android's backend. Android can set deliveryStatus directly through Content API.

2) Complete DOM parts for onsending/onfailed event addition.

Comment 37

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 3: RIL implementation

Doug, do you want to review this? Hope I won't break your progress in message threading.

Comment 38

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 685083 [details] [diff] [review]
Part 1: IDL changes : v1

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

r=me with that fixed.

::: dom/sms/interfaces/nsIDOMSmsMessage.idl
@@ +10,4 @@
>    readonly attribute long      id;
> +
> +  /**
> +   * Should be "received", "sending", "sent" or "failed".

We generally use "error" to describe similar states elsewhere. Indeed even deliveryStatus uses "error". I think it'd make sense to use it here too?

Comment 39

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Jonas Sicking (:sicking) from comment #38)
> Comment on attachment 685083 [details] [diff] [review]
> ::: dom/sms/interfaces/nsIDOMSmsMessage.idl
> @@ +10,4 @@
> >    readonly attribute long      id;
> > +
> > +  /**
> > +   * Should be "received", "sending", "sent" or "failed".
> 
> We generally use "error" to describe similar states elsewhere. Indeed even
> deliveryStatus uses "error". I think it'd make sense to use it here too?

That's why I use "failed" here, don't want to get confused with deliveryStatus "error". But I just found we have "error" in bug 760065, so I'll follow it.

Comment 40

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 685084 [details] [diff] [review]
Part 2: DOM implementation

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

There's a bunch of places where "failed" should be changed to "error".

::: dom/sms/src/SmsManager.cpp
@@ +353,5 @@
>  
> +  if (!strcmp(aTopic, kSmsSendingObserverTopic)) {
> +    nsCOMPtr<nsIDOMMozSmsMessage> message = do_QueryInterface(aSubject);
> +    if (!message) {
> +      NS_ERROR("Got a 'sms-sending' topic without a valid message!");

This is really more code than needed. You can simply do

nsCOMPtr<nsIDOMMozSmsMessage> message = ...
MOZ_ASSERT(message, "sms-sending topic without valid message!");

DispatchTrustedSmsEventToSelf(...);

Since you are just copying the pattern from elsewhere in this file, no need to fix it in this patch if you don't want to. If so, please do file a followup though. (Fixing that followup is obviously not a blocker though).

Comment 41

[Doug Turner (:dougt)]

re comment #37 - I am not working on message threading.  Is this review for someone else?

Comment 42

[Gregor Wagner [:gwagner]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #37)
> 
> Doug, do you want to review this? Hope I won't break your progress in
> message threading.

The wrong Turner :) vicamo you were looking for bent.
I can take a look tomorrow.

Comment 43

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 1: IDL changes : v2

Address review comments in comment #38. Already sr+.

Comment 44

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 2: DOM implementation : v2

Address review comment #40. Already r+.

Comment 45

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Jonas Sicking (:sicking) from comment #40)
> Comment on attachment 685084 [details] [diff] [review]
> Since you are just copying the pattern from elsewhere in this file, no need
> to fix it in this patch if you don't want to. If so, please do file a
> followup though. (Fixing that followup is obviously not a blocker though).

Bug 817587 filed :)

Comment 46

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 3: RIL implementation : v2

Fix a few typos and verify with test cases.

Comment 47

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 4: Android implementation : WIP

Comment 48

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 5: test cases

Hi Rob, would you like to review this patch? We'll add a new delivery state in SmsMessage.

Comment 49

[Robert Wood [:rwood]]

Comment on attachment 687774 [details] [diff] [review]
Part 5: test cases

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

The test update looks good to me, thanks Vicamo.
The other existing SMS tests should still run fine after this change since they use the existing 'sent' event.

Comment 50

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Rob Wood [:rwood] from comment #49)
> Comment on attachment 687774 [details] [diff] [review]
> -----------------------------------------------------------------
> The test update looks good to me, thanks Vicamo.
> The other existing SMS tests should still run fine after this change since
> they use the existing 'sent' event.

Yes, actually no test cases are affected, but I still want to verify onsending/sent events. test_outgoing seems to be a nice target for me.

Comment 51

[Alex Keybl [:akeybl]]

Mass Modify: All un-milestoned, unresolved blocking-basecamp+ bugs are being moved into the C3 milestone. Note that the target milestone does not mean that these bugs can't be resolved prior to 12/10, rather C2 bugs should be prioritized ahead of C3 bugs.

Comment 52

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Vicamo: Is the plan to uplift this work to mozilla-beta so that we can use it for v1?

Comment 53

[Andrew Overholt [:overholt]]

Since this blocks deleting PhoneNumberJS, I'm moving it to the C2 milestone and moving deleting of PhoneNumberJS (bug 811539) to the C3 milestone.

Can this land by Monday, Vicamo?  If not, I can try to find someone less over-loaded.

Comment 54

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 687758 [details] [diff] [review]
Part 1: IDL changes : v2

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

Carrying forward

Comment 55

[Johnny Stenback (:jst)]

Comment on attachment 687758 [details] [diff] [review]
Part 1: IDL changes : v2

Before landing this, please bump the UUID for the changed IDL interfaces.

Comment 56

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 687763 [details] [diff] [review]
Part 2: DOM implementation : v2

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

Carrying forward with the assumption that this just addressed review comments.

Comment 57

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Vicamo, is this ready to land? I don't think we should spend time on making the Android implementation work. And we definitely shouldn't hold landing this until the Android implementation work.

Do we need to do gaia work before we can land this?

Comment 58

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 1: IDL changes : v3

Updates UUID. Thanks Jonny!

Comment 59

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 3: RIL implementation : v3

Rebase due to bug 816416.

Comment 60

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 5: test cases : v2

Add r=rwood.

Comment 61

[Shian-Yow Wu [:swu]]

Do we really need Android part now?
If this is a C2 bug, it's more reasonable to file a follow up bug for Android part, so we can land this bug for B2G.

Comment 62

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 4: Android implementation

This patch keeps the original arch of WebSMS Android backend and does not implement onsending/onfailed events. The task is left for bug 819874.

try: https://tbpl.mozilla.org/?tree=Try&rev=3457730c6fd7

Comment 63

[Mozilla RelEng Bot]

Try run for 3457730c6fd7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=3457730c6fd7
Results (out of 46 total builds):
    success: 44
    warnings: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-3457730c6fd7

Comment 64

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 690322 [details] [diff] [review]
Part 4: Android implementation

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

I don't understand the need for  nsIAndroidSmsDatabaseService.idl. What javascript needs to call nsIAndroidSmsDatabaseService?

Comment 65

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 4: Android implementation : v2

Blassey, you're right, I don't have to create another interface for Android SMS database service. The original Android onsent event call stack looks like:

1) GeckoSmsManager.onReceived()
2) GeckoAppShell.saveMessageInSentbox(), which is a native function and is implemented in Java_org_mozilla_gecko_GeckoAppShell_saveMessageInSentbox()
3) nsISmsDatabaseService.SaveSentMessage(), which is a COMPtr interface method and is implemented in SmsDatabaseService::SaveSentMessage()
4) AndroidBridge::SaveSentMessage()
5) GeckoAppShell.saveSentMessage()
6) GeckoSmsManager.saveSentMessage()

Since we've removed backend specific methods from nsISmsDatabaseService in Part 2 and we don't really need another IDL interface in order to keep such a round trip, this patch removes all unnecessary code and nsIAndroidSmsDatabaseService introduced in previous patch as well.

Note that in the three removed nsISmsDatabaseService methods,
1) saveReceivedMessage() is never required for Android because its official SMS application is responsible for saving incoming messages.
2) saveSentMessage() is required, but we don't have to expose it as an interface method.
3) setMessageDeliveryStatus() is not implemented in Android backend by now. And I think we might never need it at all for the same reason of saveSentMessage(). Discuss it later in bug 803828.

try: https://tbpl.mozilla.org/?tree=Try&rev=181f8011fa49

Comment 66

[Mozilla RelEng Bot]

Try run for 181f8011fa49 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=181f8011fa49
Results (out of 46 total builds):
    success: 45
    warnings: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-181f8011fa49

Comment 67

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a845afe5f4c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd49f34665d9
https://hg.mozilla.org/integration/mozilla-inbound/rev/8216f817af45
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a54cd08b141
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5c9297c922a

Comment 68

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a845afe5f4c6
https://hg.mozilla.org/mozilla-central/rev/bd49f34665d9
https://hg.mozilla.org/mozilla-central/rev/8216f817af45
https://hg.mozilla.org/mozilla-central/rev/3a54cd08b141
https://hg.mozilla.org/mozilla-central/rev/f5c9297c922a

Comment 69

[Vicamo Yang [:vicamo][:vyang]]

hi Blad, we really have to land it asap for C2. But should you have any comments on these patches, please let me know. I'll file a follow-up bug instantly, address your comments, and request for your review again. Thank you!

Comment 70

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f8f769859c19
https://hg.mozilla.org/releases/mozilla-aurora/rev/ec2e5d5169e6
https://hg.mozilla.org/releases/mozilla-aurora/rev/148b38305445
https://hg.mozilla.org/releases/mozilla-aurora/rev/6716f2e5e2c8
https://hg.mozilla.org/releases/mozilla-aurora/rev/4db8f6cdd896

https://hg.mozilla.org/releases/mozilla-b2g18/rev/3c817bfbe2b1
https://hg.mozilla.org/releases/mozilla-b2g18/rev/badeb606ee1e
https://hg.mozilla.org/releases/mozilla-b2g18/rev/ed670b438f1c
https://hg.mozilla.org/releases/mozilla-b2g18/rev/5ae76a8b822f
https://hg.mozilla.org/releases/mozilla-b2g18/rev/ee88b3a095cc

Comment 71

[Philipp von Weitershausen [:philikon]]

A bunch of these patches did not update the UUIDs of interfaces that were changed. I'm not 100% certain on the mechanics of IDL revving... do we need to address this in a follow-up?

Comment 72

[Boris Zbarsky [:bzbarsky]]

Yes!  And please do it before those patches make it from Aurora to beta, because once they do you won't be able to change the UUID.

Comment 73

[Philipp von Weitershausen [:philikon]]

Attachment: follow-up: rev UUIDs

Thanks for clarifying, bz. Mind rubberstamping this?

Comment 74

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 693700 [details] [diff] [review]
follow-up: rev UUIDs

r=me

Comment 75

[Philipp von Weitershausen [:philikon]]

Thanks, bz! Marking checkin-needed for the follow-up patch. Please land in m-c, m-a, m-b2g18. Thanks!

Comment 76

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0fca3f2c263b

Comment 77

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ba92400c2bd6
https://hg.mozilla.org/releases/mozilla-b2g18/rev/81b03770cbbc

Comment 78

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0fca3f2c263b

Comment 79

[Mozilla RelEng Bot]

Try run for d3d90759978e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d3d90759978e
Results (out of 98 total builds):
    success: 82
    warnings: 15
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d3d90759978e

Comment 80

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Philipp von Weitershausen [:philikon] from comment #71)
> A bunch of these patches did not update the UUIDs of
> interfaces that were changed.

Why? I've only modified comments of the two interfaces.

Comment 81

[Philipp von Weitershausen [:philikon]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #80)
> (In reply to Philipp von Weitershausen [:philikon] from comment #71)
> > A bunch of these patches did not update the UUIDs of
> > interfaces that were changed.
> 
> Why? I've only modified comments of the two interfaces.

nsISmsDatabaseService definitely had some methods removed.