Closed Bug 774621 Opened 7 years ago Closed 7 years ago

B2G SMS: save error messages before sendSMS()

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
B2G C2 (20nov-10dec)
blocking-basecamp +
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: vicamo, Assigned: vicamo)

References

Details

Attachments

(6 files, 13 obsolete files)

24.43 KB, patch
sicking
: review+
Details | Diff | Splinter Review
3.33 KB, patch
vicamo
: superreview+
Details | Diff | Splinter Review
19.22 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
9.34 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
16.49 KB, patch
sicking
: review+
Details | Diff | Splinter Review
1.94 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
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
Assignee: nobody → vyang
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.
Attachment #642919 - Flags: feedback?(philipp)
Attachment #642919 - Flags: feedback?(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.
Attachment #642914 - Flags: superreview-
Attachment #642919 - Flags: feedback?(mounir)
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.
Attachment #642919 - Flags: feedback?(philipp)
(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.
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.
* should be managed by SMS API, not SMS App ;)
    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. :)
Add more DOM peers for more opinions/concerns about extending SMS DOM API. Please give your precious feedback. Thanks in advance!
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.
(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?
(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.
(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.
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.
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.
blocking-basecamp: --- → ?
My main comment on the API is that we should move it to WebIDL and use enums as needed.  ;)
(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.
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.
(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.
blocking-basecamp: ? → +
blocking-basecamp: + → -
(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
Duplicate of this bug: 809257
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?
blocking-basecamp: - → ?
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.
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?
Flags: needinfo?(jonas)
[: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!
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?
blocking-basecamp: ? → +
Flags: needinfo?(jonas)
Summary: B2G SMS: save outgoing messages before sendSMS() → B2G SMS: save error messages before sendSMS()
(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
[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.
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!
Attachment #642914 - Attachment is obsolete: true
Attachment #642915 - Attachment is obsolete: true
Attachment #642916 - Attachment is obsolete: true
Attachment #642919 - Attachment is obsolete: true
Attachment #681946 - Flags: superreview?(jonas)
Comment on attachment 681946 [details] [diff] [review]
Part 1/4: add queued/failed delivery state

Missing onsending/onsendfailed event targets.
Attachment #681946 - Attachment is obsolete: true
Attachment #681946 - Flags: superreview?(jonas)
Attached patch Part 1: IDL changes : v1 (obsolete) — Splinter Review
1) Add onsending & onfailed event in SmsManager
2) Update possible values for SmsMessage.deliveryStatus in different delivery states.
Attachment #685083 - Flags: superreview?(jonas)
Attached patch Part 2: DOM implementation (obsolete) — Splinter Review
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.
Attachment #685084 - Flags: review?(mounir)
Attachment #685084 - Flags: review?(mounir) → review?(jonas)
Attached patch Part 3: RIL implementation (obsolete) — Splinter Review
Doug, do you want to review this? Hope I won't break your progress in message threading.
Attachment #685089 - Flags: review?(doug.turner)
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?
Attachment #685083 - Flags: superreview?(jonas) → superreview+
(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 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).
Attachment #685084 - Flags: review?(jonas) → review+
re comment #37 - I am not working on message threading.  Is this review for someone else?
Attachment #685089 - Flags: review?(doug.turner) → review?(anygregor)
(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.
Priority: -- → P1
Attached patch Part 1: IDL changes : v2 (obsolete) — Splinter Review
Address review comments in comment #38. Already sr+.
Attachment #685083 - Attachment is obsolete: true
Address review comment #40. Already r+.
Attachment #685084 - Attachment is obsolete: true
(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 :)
Blocks: 817587
Attached patch Part 3: RIL implementation : v2 (obsolete) — Splinter Review
Fix a few typos and verify with test cases.
Attachment #685089 - Attachment is obsolete: true
Attachment #685089 - Flags: review?(anygregor)
Attachment #687767 - Flags: review?(anygregor)
Attached patch Part 5: test cases (obsolete) — Splinter Review
Hi Rob, would you like to review this patch? We'll add a new delivery state in SmsMessage.
Attachment #687774 - Flags: review?(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.
Attachment #687774 - Flags: review?(rwood) → review+
(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.
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.
Target Milestone: --- → B2G C3 (12dec-1jan)
Vicamo: Is the plan to uplift this work to mozilla-beta so that we can use it for v1?
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.
Target Milestone: B2G C3 (12dec-1jan) → B2G C2 (20nov-10dec)
Attachment #687767 - Flags: review?(anygregor) → review+
Comment on attachment 687758 [details] [diff] [review]
Part 1: IDL changes : v2

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

Carrying forward
Attachment #687758 - Flags: superreview+
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 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.
Attachment #687763 - Flags: review+
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?
Updates UUID. Thanks Jonny!
Attachment #687758 - Attachment is obsolete: true
Attachment #690307 - Flags: superreview+
Rebase due to bug 816416.
Attachment #687767 - Attachment is obsolete: true
Attachment #690308 - Flags: review+
Add r=rwood.
Attachment #687774 - Attachment is obsolete: true
Attachment #690309 - Flags: review+
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.
Blocks: 819874
Attached patch Part 4: Android implementation (obsolete) — Splinter Review
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
Attachment #687769 - Attachment is obsolete: true
Attachment #690322 - Flags: review?(blassey.bugs)
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/vyang@mozilla.com-3457730c6fd7
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?
Attachment #690322 - Flags: review?(blassey.bugs) → review-
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
Attachment #690322 - Attachment is obsolete: true
Attachment #690437 - Flags: review?(blassey.bugs)
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/vyang@mozilla.com-181f8011fa49
Attachment #690437 - Flags: review?(blassey.bugs) → review+
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!
Whiteboard: [status-b2g18:fixed]
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?
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.
Thanks for clarifying, bz. Mind rubberstamping this?
Attachment #693700 - Flags: review?(bzbarsky)
Comment on attachment 693700 [details] [diff] [review]
follow-up: rev UUIDs

r=me
Attachment #693700 - Flags: review?(bzbarsky) → review+
Thanks, bz! Marking checkin-needed for the follow-up patch. Please land in m-c, m-a, m-b2g18. Thanks!
Keywords: checkin-needed
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/vyang@mozilla.com-d3d90759978e
(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.
(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.
You need to log in before you can comment on or make changes to this bug.