It would be useful to access the message object when sending a SMS failed. Indeed there is a new message object in the Database, and we'd need to know its ID to, for example, be able to delete it. Without this, we need to rely on the timestamp to find the message that was persisted in the database service, and it is not very reliable. Asking for blocking basecamp even if I'm not sure it is.

blocking- assuming this is only a nice-to-have. If this prevents us from fixing a blocking issue, then please renominate.

This is a nice to have but I'd be happy if someone could think of this now, because I understand this must be consistent across platforms.

Hi Jonas and Mounir, We're facing a dilemma due to the the natural limitation of DOMRequest.onerror(...). Currently, The .onerror(...) only allows to set the |.error| attribute which simply carries an error string. However, we're hoping to expose more info (like an object) to the content end when the .onerror(...) is happening. We attempted to set the |.result|, but the current codes stop us doing so [1]. NS_ASSERTION(mResult == JSVAL_VOID, "mResult shouldn't have been set!"); Can we remove this assertion? Or it's actually a formal DOMRequest API definition? That is, we're not allowed to set |.result| for .onerror(...) in any way. [1] http://mxr.mozilla.org/mozilla-central/source/dom/base/DOMRequest.cpp#152

I don't think we should set the .result here. Instead we should expose additional information through .error. Right now .error is always set to a plain DOMError which only contains a .name string. I think the simplest solution would be to add a DOMRequestService.fireDetailedError(in nsIDOMDOMRequest request, in nsISupports error) function. This function would fire an "error" event like normal, but would set .error to the passed in object. That way we could let the SMS code do something like: DOMRequestService.fireDetailedError(req, { name: errorMsg, id: messageId });

As Jonas said, DOMError can be extended. Actually, it was intended to be. So, we could have: interface SMSError : DOMError { DOMString id; }; And then, you can provide the error .name and the message .id.

Thanks Jonas and Mounir for the info. Anyway, this bug is just nice to have. We'll revisit this when all the blocking issues are solved. Temporally taking this by myself to easily keep track of the pending issues. If anyone wants to enhance this, please feel free to take this.

So, I filed this bug a long time ago, and I would like to use it now, sadly it's not fixed :( In Bug 974733, we need to know the new threadId when a message is sent, however we don't have it when there is an error. Until now we were relying on the "sending" event but we can't now because of how inline activities work.

(In reply to Julien Wajsberg [:julienw] from comment #7) > So, I filed this bug a long time ago, and I would like to use it now, sadly > it's not fixed :( > > In Bug 974733, we need to know the new threadId when a message is sent, > however we don't have it when there is an error. Until now we were relying > on the "sending" event but we can't now because of how inline activities > work. Not happening. This has not been a blocker since 1.01 and still stays that way. Please find a way to fix the regressing cause in bug 974733 without relying on this.

This has not been a blocker since 1.01 because we didn't need it. Now we need it.

Drivers disagree with the approach here. I want evidence of what regressed in gecko here before we justify what actual fix we need here. bug 824717 isn't a regression, so drivers fail to see the need to include that to fix this bug. Please bisect to find the regressing cause in gecko & justify with data on what a safe fix is needed here.

It is not a question of regression, it's a question of doing another approach to redirect to the correct thread in Bug 974733. That another approach is necessary because of how inline activities work. That another approach needs this bug to work properly when a message can't be sent for some reason. For me, the only answer I'll accept to not block is from Bevis telling me he's too overbusy. In the mean time I'll try to fix this myself. Now, please, I'd like to do my job instead of arguing for hours.

(In reply to Julien Wajsberg [:julienw] from comment #11) > It is not a question of regression, it's a question of doing another > approach to redirect to the correct thread in Bug 974733. That another > approach is necessary because of how inline activities work. That another > approach needs this bug to work properly when a message can't be sent for > some reason. > > For me, the only answer I'll accept to not block is from Bevis telling me > he's too overbusy. In the mean time I'll try to fix this myself. > > Now, please, I'd like to do my job instead of arguing for hours. See https://bugzilla.mozilla.org/show_bug.cgi?id=974733#c24. We aren't taking this bug. Please identify the regressing changeset & use that justify the root cause of the problem.

WIP patch, I copied most of it from Bug 883178. Obviously not finished yet, we need to change MobileMessageCallback, create the error in sendSMS and sendMMS, and send it in all useful places. This part of the code is not very easy to change because this is not very well factorized IMO. I stop working on this now because this is not blocking.

Now with all the files...

(In reply to Jason Smith [:jsmith] from comment #10) > Drivers disagree with the approach here. I want evidence of what regressed > in gecko here before we justify what actual fix we need here. bug 824717 > isn't a regression, so drivers fail to see the need to include that to fix > this bug. We shouldn't not fix bugs for the sake of not fixing bugs. Whenever we have a blocker late in the release cycle we should do the most low-risk reasonable patch to fix the blocker. It can very well be that the most low-risk way to fix a bug means fixing another bug as well. Which approach to fix a given bug is up to the engineer who is fixing it, not up to drivers. If the final patch is too risky then indeed it's drivers jobs to ask for lower risk patch. But any patch that includes fixing other non-blocking bugs doesn't automatically disqualify a patch.

(In reply to Jonas Sicking (:sicking) from comment #15) > (In reply to Jason Smith [:jsmith] from comment #10) > > Drivers disagree with the approach here. I want evidence of what regressed > > in gecko here before we justify what actual fix we need here. bug 824717 > > isn't a regression, so drivers fail to see the need to include that to fix > > this bug. > > We shouldn't not fix bugs for the sake of not fixing bugs. Whenever we have > a blocker late in the release cycle we should do the most low-risk > reasonable patch to fix the blocker. It can very well be that the most > low-risk way to fix a bug means fixing another bug as well. Potentially yes, but that's something that needs to include justification behind. We've already had multiple instances where people have abused the "blocks a blocker" argument to include a bunch of enhancements that were not necessary to fix the blocking bug. With these enhancements sometimes came with blocking regressions, which ended up hurting in the long run, not helping us. In this bug's case, everyone in triage questioned this bug because it's been around for a long time, so everyone questioned why this was required to fix the blocking bug. There was a lack of data on the bug why a regression would require an enhancement request on the API side, which lead the bug to be minused for blocking. > > Which approach to fix a given bug is up to the engineer who is fixing it, > not up to drivers. As soon as that blocking flag is flipped, drivers are going to get involved in the process. And people will check that the blocks a blocker argument is sound, as we've had too many instances of people trying to sneak in features while trying to help solve the actual blocking bug. This recently happened with the Network Stats API, where we went in a regression spiral because someone was trying to slip in enhancements while trying to fix a couple of blocker bugs. > > If the final patch is too risky then indeed it's drivers jobs to ask for > lower risk patch. But any patch that includes fixing other non-blocking bugs > doesn't automatically disqualify a patch. Then we shouldn't nominate the bug up front then until either the patch is included proving that it's the lowest risk solution possible or we need proof why the blocks a blocker argument applies here. The process of blinding of applying the blocks a blocker argument has caused numerous regressions historically, so that's why we've been cracking down on this in the triage & approval process.

Sure, I agree that in the discussed scenario we shouldn't mark the blocking bug as a blocker. The discussion and back and forth with the blocking flag doesn't seem constructive here. I would have rather seen the discussion be about how to fix the other bug most easily. It's also quite frustrating to see bugs like this one never raise to the level of getting on a backlog and getting fixed. It's always bad when we end up doing more work in the frontend to work around something that would be easy to fix in backend.

(In reply to Jonas Sicking (:sicking) from comment #17) > The discussion and back and forth with the blocking flag doesn't seem > constructive here. I would have rather seen the discussion be about how to > fix the other bug most easily. Just want to mention I did give more information on the other Bug 974733 about other potential solutions.

I did some more work on it today, doesn't even compile because it's not finished

To be fair, seeing how the sendSMS method is implemented, I don't think we can do a safe enough patch for v1.3 here. (and I didn't even looked at sendMMS at all).

An heads up for Bevis here: this would still be nice to have but we get the request's error/success events too late to be really useful right now. We'll eventually need to sit and discuss to make these events useful. So Bevis, don't rush to fix this if you have more important work to do :)

Thanks for reminding it. My original purpose is actually reassign Gene's bug to myself for further study in the future. ;) (In reply to Julien Wajsberg [:julienw] from comment #21) > An heads up for Bevis here: this would still be nice to have but we get the > request's error/success events too late to be really useful right now. We'll > eventually need to sit and discuss to make these events useful. > > So Bevis, don't rush to fix this if you have more important work to do :)

Another location where this could be useful: https://github.com/mozilla-b2g/gaia/pull/18072/files#r11384941

This one is quite important to refine Gaia's implementation for the error handling of sending SMS/MMS. NI Wesley to add this into backlog as well to do this in 1.5 timeframe in priority.

Hi Hsinyi & Olli, This patch is to define a new DOMError called DOMMobileMessageError with a hook to append any nsISupports object when return a DOMError. The scenario in this bug is to append either a nsIDOMMozSmsMessage or a nsIDOMMozMmsMessage when replying the error. However, I am not pretty sure if I should give it a more generic name like |DOMDetailedError| and rename the new attribute |data| to |detail|. If you think this is better, please r- me. I'll refine it again. :)

Comment on attachment 8415094 [details] [diff] [review] Patch Part 1 v1: Define DOMMobileMessageError:DOMError to hook an additional nsISupports object like nsIDOMMozSmsMessage/nsIDOMMozMmsMessage when error occurs. r=hsinyi,smaug Review of attachment 8415094 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/DOMMobileMessageError.h @@ +4,5 @@ > +* License, v. 2.0. If a copy of the MPL was not distributed with this file, > +* You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#ifndef mozilla_dom_MmiError_h > +#define mozilla_dom_MmiError_h Shouldn't be _MmiError @@ +36,5 @@ > + > +} // namespace dom > +} // namespace mozilla > + > +#endif // mozilla_dom_MmiError_h \ No newline at end of file ditto. ::: dom/tests/mochitest/general/test_interfaces.html @@ +305,5 @@ > "DOMImplementation", > // IMPORTANT: Do not change this list without review from a DOM peer! > "DOMMMIError", > // IMPORTANT: Do not change this list without review from a DOM peer! > + "DOMMobileMessageError", Need to have "dom.sms.enabled" ::: dom/webidl/DOMMobileMessageError.webidl @@ +3,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. > + */ > + > +interface DOMMobileMessageError : DOMError { Please have this guarded from the pref "dom.sms.enabled." @@ +4,5 @@ > + * You can obtain one at http://mozilla.org/MPL/2.0/. > + */ > + > +interface DOMMobileMessageError : DOMError { > + readonly attribute nsISupports data; 1) Please drop a comment saying the data could be nsIDOMMozSmsMessage or nsIDOMMozMmsMessage. Thanks! 2) Is it possible that data is going to be null?

(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) from comment #25) > Created attachment 8415094 [details] [diff] [review] > Patch Part 1 v1: Define DOMMobileMessageError:DOMError to hook an additional > nsISupports object like nsIDOMMozSmsMessage/nsIDOMMozMmsMessage when error > occurs. r=hsinyi,smaug > > Hi Hsinyi & Olli, > > This patch is to define a new DOMError called DOMMobileMessageError with a > hook to append any nsISupports object when return a DOMError. > The scenario in this bug is to append either a nsIDOMMozSmsMessage or a > nsIDOMMozMmsMessage when replying the error. > > However, I am not pretty sure if I should give it a more generic name like > |DOMDetailedError| and rename the new attribute |data| to |detail|. > > If you think this is better, please r- me. > I'll refine it again. :) Hi Bevis, I think it's fine to have this new Error interface specific to MobileMessage, because it's always nice to have a clear use case before designing an interface. And now I don't see a clear picture of how a generic detailed error is going to be requested or be used. :)

Comment on attachment 8415094 [details] [diff] [review] Patch Part 1 v1: Define DOMMobileMessageError:DOMError to hook an additional nsISupports object like nsIDOMMozSmsMessage/nsIDOMMozMmsMessage when error occurs. r=hsinyi,smaug Review of attachment 8415094 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/DOMMobileMessageError.h @@ +4,5 @@ > +* License, v. 2.0. If a copy of the MPL was not distributed with this file, > +* You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#ifndef mozilla_dom_MmiError_h > +#define mozilla_dom_MmiError_h Sorry, my bad. @@ +36,5 @@ > + > +} // namespace dom > +} // namespace mozilla > + > +#endif // mozilla_dom_MmiError_h \ No newline at end of file Will do. ::: dom/tests/mochitest/general/test_interfaces.html @@ +305,5 @@ > "DOMImplementation", > // IMPORTANT: Do not change this list without review from a DOM peer! > "DOMMMIError", > // IMPORTANT: Do not change this list without review from a DOM peer! > + "DOMMobileMessageError", Will do. ::: dom/webidl/DOMMobileMessageError.webidl @@ +3,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. > + */ > + > +interface DOMMobileMessageError : DOMError { Will do. @@ +4,5 @@ > + * You can obtain one at http://mozilla.org/MPL/2.0/. > + */ > + > +interface DOMMobileMessageError : DOMError { > + readonly attribute nsISupports data; Will add the comment in detail. Besides, here is the answer of 2), in current implementation, DOMMobileMessageError will be fired only if data is available. If not, DOMError will be fired instead.

Hi Julien, I'd like to NI you for this question: May I know if gaia always expect to have the message requested to send when we report a error to gaia due to send/sendMMS failed? The reason I ask is that in current implementation, except silent SMS, the creation of nsIDOMMozSmsMessage/nsIDOMMozMmsMessage relies on whether the message is stored into MobileMessageDB. Hence, for the error caused internally by indexedDB or by storage full, there is no DomMessage available when reporting the error. That is, if DomMessage is available, DomMobileMessageError will be fired. Otherwise, DOMError will be fired. If it's a MUST to have for gaia, I''ll have to break this dependency. Thanks, Bevis

No, I think what we usually want is the message id; so if it's not stored in the db, the id won't be meaningful anyway, right?

(In reply to Julien Wajsberg [:julienw] (away until 5th May) from comment #36) > No, I think what we usually want is the message id; so if it's not stored in > the db, the id won't be meaningful anyway, right? Yes, I have the same understanding. Thanks for your clarification. :)

update patch to address issues in comment 32.

Comment on attachment 8417788 [details] [diff] [review] Patch Part 1 v2: Define DOMMobileMessageError:DOMError to hook an additional nsISupports object like nsIDOMMozSmsMessage/nsIDOMMozMmsMessage when error occurs. r=hsinyi,smaug Review of attachment 8417788 [details] [diff] [review]: ----------------------------------------------------------------- Changes on .webidl look good to me, r=me for that. I provided comments for other files though they in fact need DOM Peer's official review. Please take care of those and see if Olli has different opinions, thanks. ::: dom/tests/mochitest/general/test_interfaces.html @@ +305,5 @@ > "DOMImplementation", > // IMPORTANT: Do not change this list without review from a DOM peer! > "DOMMMIError", > // IMPORTANT: Do not change this list without review from a DOM peer! > + {name: "DOMMobileMessageError", pref: "dom.sms.enabled"}, I think we will need "b2g: true" as well. Sorry for not pointing this out last time. ::: dom/webidl/DOMMobileMessageError.webidl @@ +6,5 @@ > + > +[Pref="dom.sms.enabled"] > +interface DOMMobileMessageError : DOMError { > + // Either a nsIDOMMozSmsMessage or a nsIDOMMozMmsMessage will be attached. > + readonly attribute nsISupports data; this .webidl looks good to me.

Comment on attachment 8417788 [details] [diff] [review] Patch Part 1 v2: Define DOMMobileMessageError:DOMError to hook an additional nsISupports object like nsIDOMMozSmsMessage/nsIDOMMozMmsMessage when error occurs. r=hsinyi,smaug Actually, why .data. Why not .message?

And once MozSmsMessage and MozMmsMessage are webidl-fied, its type could be readonly attribute (MozSmsMessage or MozMmsMessage)

(In reply to Olli Pettay [:smaug] from comment #40) > Comment on attachment 8417788 [details] [diff] [review] > Patch Part 1 v2: Define DOMMobileMessageError:DOMError to hook an additional > nsISupports object like nsIDOMMozSmsMessage/nsIDOMMozMmsMessage when error > occurs. r=hsinyi,smaug > > Actually, why .data. Why not .message? Hi Olli, The reason is that there is already a |message| attribute defined in DOMError and DOMMobileMessageError is inherited from DOMError. Or is it more suitable if I named it as |mobileMessage| instead? (In reply to Olli Pettay [:smaug] from comment #41) >And once MozSmsMessage and MozMmsMessage are webidl-fied, its type could be >readonly attribute (MozSmsMessage or MozMmsMessage) Thanks for reminding. We'll revisit this again when we webidl-fied MozSmsMessage and MozMmsMessage.

Comment on attachment 8417788 [details] [diff] [review] Patch Part 1 v2: Define DOMMobileMessageError:DOMError to hook an additional nsISupports object like nsIDOMMozSmsMessage/nsIDOMMozMmsMessage when error occurs. r=hsinyi,smaug Oh, right, .message in DOMError. I guess .data is good enough then. >+DOMMobileMessageError::DOMMobileMessageError(nsPIDOMWindow* aWindow, const nsAString& aName, >+ nsISupports* aData) >+ : DOMError(aWindow, aName) >+ , mData(aData) >+{ MOZ_ASSERT that aData is non-null >+interface DOMMobileMessageError : DOMError { >+ // Either a nsIDOMMozSmsMessage or a nsIDOMMozMmsMessage will be attached. perhaps // data is either an nsIDOMMozSmsMessage or an nsIDOMMozMmsMessage object. I'm not sure if webidl codegen could deal with idl interfaces here so that the type could be readonly attribute (MozSmsMessage or MozMmsMessage) data; Could you please try if that works. If not, nsISupports is fine. Also, if it work, no need to add the comment about what the .data is.

(In reply to Olli Pettay [:smaug] from comment #43) > >+interface DOMMobileMessageError : DOMError { > >+ // Either a nsIDOMMozSmsMessage or a nsIDOMMozMmsMessage will be attached. > perhaps > // data is either an nsIDOMMozSmsMessage or an nsIDOMMozMmsMessage object. > > I'm not sure if webidl codegen could deal with idl interfaces here so that > the type could be > readonly attribute (MozSmsMessage or MozMmsMessage) data; > Could you please try if that works. If not, nsISupports is fine. > Also, if it work, no need to add the comment about what the .data is. Sure, I give it a try. :-)

Comment on attachment 8415096 [details] [diff] [review] Patch Part 2 v1: Refactor SmsParent::GetMobileMessageDataFromMessage() as a static method to be used in both SmsParent/SmsRequestParent. r=vyang Review of attachment 8415096 [details] [diff] [review]: ----------------------------------------------------------------- Mostly done, but I think I'd still like to check again the next revision. Thank you :) ::: dom/mobilemessage/src/ipc/SmsParent.cpp @@ +227,1 @@ > NS_ERROR("Got a 'sms-failed' topic without a valid message!"); It's not so correct for a sms-failed event here because we may have such error because of the failure to save sending message into database, therefore we don't have a valid DOM message here. Need to file a follow-up bug for this, and it will depends on bug 926277 for test cases. @@ +289,5 @@ > > if (!strcmp(aTopic, kSmsReadErrorObserverTopic)) { > MobileMessageData msgData; > + if (!GetMobileMessageDataFromMessage(static_cast<ContentParent*>(Manager()), > + aSubject, msgData)) { Just found a minor error in MmsService.js. In line 1944 we have: gMobileMessageDatabaseService .setMessageReadStatusByEnvelopeId(envelopeId, address, readStatus, (function(aRv, aDomMessage) { if (!Components.isSuccessCode(aRv)) { // Notifying observers the read status is error. Services.obs.notifyObservers(aDomMessage, kSmsReadSuccessObserverTopic, null); return; } ... This results in same nulled message problem. However, the error notification here uses a wrong topic and actually should not be ever dispatched at all. Failure to mark message read here doesn't follow the message read state should become either true or false. Please help remove that notifyObservers() line. [1]: http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/gonk/MmsService.js#1944 @@ +639,5 @@ > > + MobileMessageData data; > + if (SmsParent::GetMobileMessageDataFromMessage( > + static_cast<ContentParent*>(Manager()->Manager()), > + aMessage, data)) { nit: ContentParent *parent = static_cast<ContentParent*>(Manager()->Manager()); MobileMessageData data; if (GetMobileMessageDataFromMessage(parent, aMessage, data)) { ... } @@ +660,5 @@ > > + MobileMessageData data; > + if (SmsParent::GetMobileMessageDataFromMessage( > + static_cast<ContentParent*>(Manager()->Manager()), > + aMessage, data)) { ditto. ::: dom/mobilemessage/src/ipc/SmsParent.h @@ +32,5 @@ > > + static bool > + GetMobileMessageDataFromMessage(ContentParent* aParent, > + nsISupports* aMsg, > + MobileMessageData& aData); nit: please remove it from this class and make it a independent utility function inside SmsParent.cpp. It has no longer any direct relation to SmsParent.

Comment on attachment 8415099 [details] [diff] [review] Patch Part 4 v1: Add a optional MobileMessageData data member in ReplyMessageSendFail. r=vyang Review of attachment 8415099 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/ipc/PSmsRequest.ipdl @@ +30,5 @@ > > struct ReplyMessageSendFail > { > int32_t error; > + MobileMessageData[] messageData; // 0 or 1 element. union OptionalMobileMessageData { void_t; MobileMessageData; }; struct ReplyMessageSendFail { int32_t error; OptionalMobileMessageData messageData; }; ::: dom/mobilemessage/src/ipc/SmsChild.cpp @@ +197,5 @@ > + nsCOMPtr<nsISupports> msgData = (msgDataArray.Length() > 0) ? > + CreateMessageFromMessageData(msgDataArray.ElementAt(0)) : nullptr; > + mReplyRequest->NotifySendMessageFailed( > + aReply.get_ReplyMessageSendFail().error(), msgData); > + } case MessageReply::TReplyMessageSendFail: { ReplyMessageSendFail &replyFail = aReply.get_ReplyMessageSendFail(); nsCOMPtr<nsISupports> msg; if (replyFail.messageData().type() == OptionalMobileMessageData::TMobileMessageData) { msg = CreateMessageFromMessageData(replyFail.messageData().get_MobileMessageData()); } mReplyRequest->NotifySendMessageFailed(replyFail.error(), msg); } ::: dom/mobilemessage/src/ipc/SmsParent.cpp @@ +661,5 @@ > + aMessage, data)) { > + replyData.messageData().AppendElement(data); > + } > + > + return SendReply(replyData);; NS_ENSURE_TRUE(!mActorDestroyed, NS_ERROR_FAILURE); MobileMessageData data; ContentParent *parent = static_cast<ContentParent*>(Manager()->Manager()) if (!GetMobileMessageDataFromMessage(parent, aMessage, data)) { return SendReply(ReplyMessageSendFail(aError, OptionalMobileMessageData())); } return SendReply(ReplyMessageSendFail(aError, OptionalMobileMessageData(data)));

Comment on attachment 8415100 [details] [diff] [review] Patch Part 5 v1: Fire DOMMobileMessageError when NotifySendMessageFailed(). r=vyang Review of attachment 8415100 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/MobileMessageCallback.h @@ +30,5 @@ > nsRefPtr<DOMRequest> mDOMRequest; > > nsresult NotifySuccess(JS::Handle<JS::Value> aResult, bool aAsync = false); > nsresult NotifySuccess(nsISupports *aMessage, bool aAsync = false); > + nsresult NotifyError(int32_t aError, nsISupports* aData = nullptr, bool aAsync = false); nit: nsISupports *aData

Comment on attachment 8415102 [details] [diff] [review] Patch Part 6 v1: Notify SendMesageFailed with saved DOM Message if available. r=vyang. Review of attachment 8415102 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/RadioInterfaceLayer.js @@ +4149,5 @@ > break; > } > > if (context.silent) { > + context.request.notifySendMessageFailed(error, context.sms); Sorry, you can't pass |context.sms| directly here because its |delivery| and |deliveryStatus| attribute is still 'sending'/'pending'. You'll have to re-create a new SMS message using |gMobileMessageService.createSmsMessage|.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #48) > > if (context.silent) { > > + context.request.notifySendMessageFailed(error, context.sms); > > Sorry, you can't pass |context.sms| directly here because its |delivery| and > |deliveryStatus| attribute is still 'sending'/'pending'. You'll have to > re-create a new SMS message using |gMobileMessageService.createSmsMessage|. Thanks for catching up this issue. I'll recreate a new one with correct |delivery| and |deliveryStatus|.

Comment on attachment 8415104 [details] [diff] [review] Patch Part 7 v1: Modify and Add test cases to check the message id and message content when failed to send SMS/MMS message. r=vyang Review of attachment 8415104 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/tests/marionette/head.js @@ +137,5 @@ > + } > + > + manager.addEventListener("failed", function onfailed(event) { > + manager.removeEventListener("failed", onfailed); > + got("message", event.message); use waitForManagerEvent('failed') instead. @@ +145,5 @@ > + request.onsuccess = function(event) { > + deferred.reject(); > + }; > + request.onerror = function(event) { > + got("error", event.target.error); Can you help copy wrapDomRequestAsPromise() from http://mxr.mozilla.org/mozilla-central/source/dom/mobileconnection/tests/marionette/head.js#50 ? Then we do: let promises = []; promises.push(waitForManagerEvent('failed')); let request = manager.send(aReceiver, aText); let p = wrapDomRequestAsPromise(request) .then((x) => { throw x }, (y) => { return y; }); promises.push(p); return Promise.all() .then(function(aResults) { // check aResults[0].message && aResults[1].target.error.data }); @@ +169,5 @@ > * @param aSendParameters a MmsSendParameters instance. > * > * @return A deferred promise. > */ > function sendMmsWithFailure(aMmsParameters, aSendParameters) { ditto.

Apply DOM peer's suggestion in comment 43 to add a union type of MozSmsMessage and MozMmsMessage instead of nsISupports.

1. Move GetParamsFromSendMmsMessageRequest() to a static utility in SmsParent.cpp. 2. Follow up bug is fired in Bug 1010718. 3. The issue we found of the MMS read report will be addressed in patch part6.

rebase due to the collision in patch part2v2.

Apply the suggestion in comment 46 to define a new union type with void_t for optional MobileMessageData.

1. Fix nit. 2. Precisely create DOMMobileMessageError with either MozSmsMessage or MozMmsMessage due to the design change in patch part_1_v3.

1. Recreate new DOMMessage for silent SMS with correct delivery status when notifying failure of sending sms. 2. Fix the problem of notifying wrong read status when read report is not able to saved.

refactor SendSmsWithFailure() and SendMmsWithFailure() with waitForManagerEvent('failed') and wrapDomRequestAsPromise().

Comment on attachment 8422987 [details] [diff] [review] Patch Part 7 v2: Modify and Add test cases to check the message id and message content when failed to send SMS/MMS message. r=vyang Review of attachment 8422987 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/tests/marionette/head.js @@ +123,5 @@ > + > + let request = manager.send(aReceiver, aText); > + promises.push(wrapDomRequestAsPromise(request) > + .then(() => { ok(false, "We expect to send SMS with failure!"); }, > + (aEvent) => { return aEvent.target.error; })); .then((aEvent) => { throw aEvent; }, (aEvent) => { return aEvent.target.error; }); Because |ok(false, ...)| doesn't turn a resolved value into a rejected one, so we'll get a resolved states in both situations. @@ +138,5 @@ > + "Checking text body."); > + } else { > + // DomMessage might not be available when failed to stored into DB. > + ok(!result.message, "domMessage is not available in event.message."); > + ok(!result.error.data, "domMessage is not available in error.data."); This is a convenient API for SMS testing, so I'd like to avoid assertions in these APIs. You can find the only |ok(false, ...)| is in clean-up stage. So, please move these tests into test script body and leave only: return Promise.all(promises) .then((aResults) => { return { message: aResults[0], error: aResults[1] }; }); @@ +171,5 @@ > > let request = manager.sendMMS(aMmsParameters, aSendParameters); > + promises.push(wrapDomRequestAsPromise(request) > + .then(() => { ok(false, "We expect to send SMS with failure!"); }, > + (aEvent) => { return aEvent.target.error; })); .then((aEvent) => { throw aEvent; }, (aEvent) => { return aEvent.target.error; }); @@ +187,5 @@ > + ok(!result.error.data, "domMessage is not available in error.data."); > + } > + > + return result; > + }); ditto.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #58) > Comment on attachment 8422987 [details] [diff] [review] > Patch Part 7 v2: Modify and Add test cases to check the message id and > message content when failed to send SMS/MMS message. r=vyang > > Review of attachment 8422987 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/mobilemessage/tests/marionette/head.js > @@ +123,5 @@ > > + > > + let request = manager.send(aReceiver, aText); > > + promises.push(wrapDomRequestAsPromise(request) > > + .then(() => { ok(false, "We expect to send SMS with failure!"); }, > > + (aEvent) => { return aEvent.target.error; })); > > .then((aEvent) => { throw aEvent; }, > (aEvent) => { return aEvent.target.error; }); > > Because |ok(false, ...)| doesn't turn a resolved value into a rejected one, > so we'll get a resolved states in both situations. > Thanks for more detailed explanation of the purpose to throw the event. Will follow this design instead. > @@ +138,5 @@ > > + "Checking text body."); > > + } else { > > + // DomMessage might not be available when failed to stored into DB. > > + ok(!result.message, "domMessage is not available in event.message."); > > + ok(!result.error.data, "domMessage is not available in error.data."); > > This is a convenient API for SMS testing, so I'd like to avoid assertions in > these APIs. You can find the only |ok(false, ...)| is in clean-up stage. > So, please move these tests into test script body and leave only: > > return Promise.all(promises) > .then((aResults) => { return { message: aResults[0], > error: aResults[1] }; }); > Will do. > @@ +171,5 @@ > > > > let request = manager.sendMMS(aMmsParameters, aSendParameters); > > + promises.push(wrapDomRequestAsPromise(request) > > + .then(() => { ok(false, "We expect to send SMS with failure!"); }, > > + (aEvent) => { return aEvent.target.error; })); > > .then((aEvent) => { throw aEvent; }, > (aEvent) => { return aEvent.target.error; }); Will do. > > @@ +187,5 @@ > > + ok(!result.error.data, "domMessage is not available in error.data."); > > + } > > + > > + return result; > > + }); > > ditto. Will do.

1. Throw Event instead of |ok(false)| assertion in reject callback to prevent ambiguity. 2. Move assertion logic out from utilities to the test cases. 3. Refactor DOMRequests used in head.js with wrapDomRequestAsPromise().

Comment on attachment 8423628 [details] [diff] [review] Patch Part 7 v3: Modify and Add test cases to check the message id and message content when failed to send SMS/MMS message. r=vyang Review of attachment 8423628 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/tests/marionette/head.js @@ +115,5 @@ > function sendSmsWithSuccess(aReceiver, aText) { > + let request = manager.send(aReceiver, aText); > + return wrapDomRequestAsPromise(request) > + .then((aEvent) => { return aEvent.target.result; }, > + (aEvent) => { return aEvent.target.error; }); throw aEvent.target.error; @@ +315,4 @@ > let request = manager.delete(aMessageIds); > + return wrapDomRequestAsPromise(request) > + .then((aEvent) => { return aEvent.target.result; }, > + (aEvent) => { return aEvent; }); don't need a reject handler here.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #61) > Comment on attachment 8423628 [details] [diff] [review] > Patch Part 7 v3: Modify and Add test cases to check the message id and > message content when failed to send SMS/MMS message. r=vyang > > Review of attachment 8423628 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/mobilemessage/tests/marionette/head.js > @@ +115,5 @@ > > function sendSmsWithSuccess(aReceiver, aText) { > > + let request = manager.send(aReceiver, aText); > > + return wrapDomRequestAsPromise(request) > > + .then((aEvent) => { return aEvent.target.result; }, > > + (aEvent) => { return aEvent.target.error; }); > > throw aEvent.target.error; Oops. Thanks for reminding this again. > > @@ +315,4 @@ > > let request = manager.delete(aMessageIds); > > + return wrapDomRequestAsPromise(request) > > + .then((aEvent) => { return aEvent.target.result; }, > > + (aEvent) => { return aEvent; }); > > don't need a reject handler here. You are right! Since, the input and output are the same.

address the problems in comment 61.

update try server result: https://tbpl.mozilla.org/?tree=Try&rev=2a48ad8bdec8

https://hg.mozilla.org/integration/b2g-inbound/rev/62e12031b8bc https://hg.mozilla.org/integration/b2g-inbound/rev/c80dbc9a693a https://hg.mozilla.org/integration/b2g-inbound/rev/5eaab20bb1aa https://hg.mozilla.org/integration/b2g-inbound/rev/d66a08988272 https://hg.mozilla.org/integration/b2g-inbound/rev/fd9e72e50518 https://hg.mozilla.org/integration/b2g-inbound/rev/4c4b8b15da37 https://hg.mozilla.org/integration/b2g-inbound/rev/bd7c604de980

Backed out for bustage on all platforms. https://hg.mozilla.org/integration/b2g-inbound/rev/f99dce77b837 https://tbpl.mozilla.org/php/getParsedLog.php?id=40095137&tree=B2g-Inbound

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #66) > Backed out for bustage on all platforms. > https://hg.mozilla.org/integration/b2g-inbound/rev/f99dce77b837 > > https://tbpl.mozilla.org/php/getParsedLog.php?id=40095137&tree=B2g-Inbound Too bad, the signature of DOMRequest::FireDetailedError was changed due to bug 1011815.[1]: -DOMRequest::FireDetailedError(nsISupports* aError) +DOMRequest::FireDetailedError(DOMError* aError) Need to modify the patch according to this change. [1] http://hg.mozilla.org/mozilla-central/rev/f654c8eba42e#l3.31

Fix compiling error due to the signature of DOMRequest::FireDetailedError() is changed in bug 1011815.

Rebase to prevent merge conflict.

Address the build break in DEBUG build if we use nsCOMPtr to hold a concrete XPCOM object. We should use nsRefPtr instead according to https://developer.mozilla.org/en-US/docs/nsRefPtr

update tryserver result: https://tbpl.mozilla.org/?tree=Try&rev=137be02546f8

https://hg.mozilla.org/integration/b2g-inbound/rev/98cb5673fc0f https://hg.mozilla.org/integration/b2g-inbound/rev/05779c9e973d https://hg.mozilla.org/integration/b2g-inbound/rev/07553a3f8f97 https://hg.mozilla.org/integration/b2g-inbound/rev/f9d55157f75c https://hg.mozilla.org/integration/b2g-inbound/rev/b7fce906b65d https://hg.mozilla.org/integration/b2g-inbound/rev/b46bd7827ccf https://hg.mozilla.org/integration/b2g-inbound/rev/22b5eefc2754

https://hg.mozilla.org/mozilla-central/rev/98cb5673fc0f https://hg.mozilla.org/mozilla-central/rev/05779c9e973d https://hg.mozilla.org/mozilla-central/rev/07553a3f8f97 https://hg.mozilla.org/mozilla-central/rev/f9d55157f75c https://hg.mozilla.org/mozilla-central/rev/b7fce906b65d https://hg.mozilla.org/mozilla-central/rev/b46bd7827ccf https://hg.mozilla.org/mozilla-central/rev/22b5eefc2754