Closed
Bug 824717
Opened 12 years ago
Closed 11 years ago
WebSMS: When sending a SMS fails, it would be handy to have the message in the error event object
Categories
(Firefox OS Graveyard :: RIL, defect)
Tracking
(blocking-basecamp:-, tracking-b2g:backlog)
RESOLVED
FIXED
2.0 S3 (6june)
People
(Reporter: julienw, Assigned: bevis)
References
Details
(Whiteboard: [priority][ft:ril][p=8])
Attachments
(7 files, 16 obsolete files)
7.62 KB,
patch
|
bevis
:
review+
|
Details | Diff | Splinter Review |
10.02 KB,
patch
|
vicamo
:
review+
|
Details | Diff | Splinter Review |
4.44 KB,
patch
|
bevis
:
review+
|
Details | Diff | Splinter Review |
3.88 KB,
patch
|
vicamo
:
review+
|
Details | Diff | Splinter Review |
9.08 KB,
patch
|
vicamo
:
review+
|
Details | Diff | Splinter Review |
11.21 KB,
patch
|
bevis
:
review+
|
Details | Diff | Splinter Review |
7.69 KB,
patch
|
Details | Diff | Splinter Review |
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.
Comment 1•12 years ago
|
||
blocking- assuming this is only a nice-to-have. If this prevents us from fixing a blocking issue, then please renominate.
blocking-basecamp: ? → -
Reporter | ||
Comment 2•12 years ago
|
||
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.
Updated•12 years ago
|
Summary: When sending a SMS fails, it would be handy to have the message in the error event object → WebSMS: When sending a SMS fails, it would be handy to have the message in the error event object
Updated•12 years ago
|
Comment 3•12 years ago
|
||
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
Flags: needinfo?(mounir)
Flags: needinfo?(jonas)
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 });
Flags: needinfo?(jonas)
Comment 5•12 years ago
|
||
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.
Flags: needinfo?(mounir)
Comment 6•12 years ago
|
||
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.
Assignee: nobody → gene.lian
Blocks: b2g-mms
Reporter | ||
Comment 7•11 years ago
|
||
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.
blocking-b2g: --- → 1.3?
Flags: needinfo?(btseng)
Comment 8•11 years ago
|
||
(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.
No longer blocks: 974733
blocking-b2g: 1.3? → -
Reporter | ||
Comment 9•11 years ago
|
||
This has not been a blocker since 1.01 because we didn't need it.
Now we need it.
blocking-b2g: - → 1.3?
Comment 10•11 years ago
|
||
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.
blocking-b2g: 1.3? → -
Reporter | ||
Comment 11•11 years ago
|
||
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.
Blocks: 974733
blocking-b2g: - → 1.3?
Comment 12•11 years ago
|
||
(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.
blocking-b2g: 1.3? → -
Reporter | ||
Comment 13•11 years ago
|
||
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.
Reporter | ||
Comment 14•11 years ago
|
||
Now with all the files...
Attachment #8383179 -
Attachment is obsolete: true
(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.
Comment 16•11 years ago
|
||
(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.
Reporter | ||
Comment 18•11 years ago
|
||
(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.
Reporter | ||
Comment 19•11 years ago
|
||
I did some more work on it today, doesn't even compile because it's not finished
Reporter | ||
Updated•11 years ago
|
Attachment #8383183 -
Attachment is obsolete: true
Reporter | ||
Updated•11 years ago
|
Attachment #8383183 -
Attachment is patch: true
Reporter | ||
Comment 20•11 years ago
|
||
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).
Assignee | ||
Updated•11 years ago
|
Assignee: gene.lian → btseng
Flags: needinfo?(btseng)
Reporter | ||
Comment 21•11 years ago
|
||
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 :)
Assignee | ||
Comment 22•11 years ago
|
||
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 :)
Reporter | ||
Comment 23•11 years ago
|
||
Another location where this could be useful: https://github.com/mozilla-b2g/gaia/pull/18072/files#r11384941
Assignee | ||
Comment 24•11 years ago
|
||
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.
Flags: needinfo?(whuang)
Updated•11 years ago
|
blocking-b2g: - → backlog
Flags: needinfo?(whuang)
Whiteboard: [priority][ft:ril]
Assignee | ||
Updated•11 years ago
|
Whiteboard: [priority][ft:ril] → [priority][ft:ril][p=5]
Target Milestone: --- → 2.0 S1 (9may)
Assignee | ||
Comment 25•11 years ago
|
||
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. :)
Attachment #8383751 -
Attachment is obsolete: true
Attachment #8415094 -
Flags: review?(htsai)
Attachment #8415094 -
Flags: review?(bugs)
Assignee | ||
Comment 26•11 years ago
|
||
Attachment #8415096 -
Flags: review?(vyang)
Assignee | ||
Comment 27•11 years ago
|
||
Attachment #8415097 -
Flags: review?(vyang)
Assignee | ||
Comment 28•11 years ago
|
||
Attachment #8415099 -
Flags: review?(vyang)
Assignee | ||
Comment 29•11 years ago
|
||
Attachment #8415100 -
Flags: review?(vyang)
Assignee | ||
Comment 30•11 years ago
|
||
Attachment #8415102 -
Flags: review?(vyang)
Assignee | ||
Comment 31•11 years ago
|
||
Attachment #8415104 -
Flags: review?(vyang)
Comment 32•11 years ago
|
||
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?
Attachment #8415094 -
Flags: review?(htsai)
Comment 33•11 years ago
|
||
(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. :)
Assignee | ||
Comment 34•11 years ago
|
||
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.
Attachment #8415094 -
Flags: review?(bugs)
Assignee | ||
Comment 35•11 years ago
|
||
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
Flags: needinfo?(felash)
Reporter | ||
Comment 36•11 years ago
|
||
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?
Flags: needinfo?(felash)
Assignee | ||
Comment 37•11 years ago
|
||
(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. :)
Assignee | ||
Comment 38•11 years ago
|
||
update patch to address issues in comment 32.
Attachment #8415094 -
Attachment is obsolete: true
Attachment #8417788 -
Flags: review?(htsai)
Attachment #8417788 -
Flags: review?(bugs)
Assignee | ||
Updated•11 years ago
|
Blocks: b2g-ril-interface
Comment 39•11 years ago
|
||
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.
Attachment #8417788 -
Flags: review?(htsai) → review+
Updated•11 years ago
|
Attachment #8417788 -
Flags: review?(bugs) → review+
Comment 40•11 years ago
|
||
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?
Attachment #8417788 -
Flags: review+ → review-
Comment 41•11 years ago
|
||
And once MozSmsMessage and MozMmsMessage are webidl-fied, its type could be
readonly attribute (MozSmsMessage or MozMmsMessage)
Assignee | ||
Comment 42•11 years ago
|
||
(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.
Flags: needinfo?(bugs)
Comment 43•11 years ago
|
||
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.
Attachment #8417788 -
Flags: review- → review+
Assignee | ||
Comment 44•11 years ago
|
||
(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. :-)
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(bugs)
Comment 45•11 years ago
|
||
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.
Attachment #8415096 -
Flags: review?(vyang)
Updated•11 years ago
|
Attachment #8415097 -
Flags: review?(vyang) → review+
Comment 46•11 years ago
|
||
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)));
Attachment #8415099 -
Flags: review?(vyang)
Comment 47•11 years ago
|
||
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
Attachment #8415100 -
Flags: review?(vyang) → review+
Comment 48•11 years ago
|
||
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|.
Attachment #8415102 -
Flags: review?(vyang)
Assignee | ||
Comment 49•11 years ago
|
||
(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 50•11 years ago
|
||
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.
Attachment #8415104 -
Flags: review?(vyang)
Assignee | ||
Updated•11 years ago
|
Whiteboard: [priority][ft:ril][p=5] → [priority][ft:ril][p=8]
Target Milestone: 2.0 S1 (9may) → 2.0 S2 (23may)
Assignee | ||
Comment 51•11 years ago
|
||
Apply DOM peer's suggestion in comment 43 to add a union type of MozSmsMessage and MozMmsMessage instead of nsISupports.
Attachment #8417788 -
Attachment is obsolete: true
Attachment #8422958 -
Flags: review+
Assignee | ||
Comment 52•11 years ago
|
||
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.
Attachment #8415096 -
Attachment is obsolete: true
Attachment #8422970 -
Flags: review?(vyang)
Assignee | ||
Updated•11 years ago
|
Attachment #8422970 -
Attachment description: Part 2 v2: Refactor SmsParent::GetMobileMessageDataFromMessage() as a static method to be used in both SmsParent/SmsRequestParent. r=vyang → Patch Part 2 v2: Refactor SmsParent::GetMobileMessageDataFromMessage() as a static method to be used in both SmsParent/SmsRequestParent. r=vyang
Assignee | ||
Comment 53•11 years ago
|
||
rebase due to the collision in patch part2v2.
Attachment #8415097 -
Attachment is obsolete: true
Attachment #8422977 -
Flags: review+
Assignee | ||
Comment 54•11 years ago
|
||
Apply the suggestion in comment 46 to define a new union type with void_t for optional MobileMessageData.
Attachment #8415099 -
Attachment is obsolete: true
Attachment #8422979 -
Flags: review?(vyang)
Assignee | ||
Comment 55•11 years ago
|
||
1. Fix nit.
2. Precisely create DOMMobileMessageError with either MozSmsMessage or MozMmsMessage due to the design change in patch part_1_v3.
Attachment #8422982 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #8415100 -
Attachment is obsolete: true
Assignee | ||
Comment 56•11 years ago
|
||
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.
Attachment #8415102 -
Attachment is obsolete: true
Attachment #8422986 -
Flags: review?(vyang)
Assignee | ||
Comment 57•11 years ago
|
||
refactor SendSmsWithFailure() and SendMmsWithFailure() with waitForManagerEvent('failed') and wrapDomRequestAsPromise().
Attachment #8415104 -
Attachment is obsolete: true
Attachment #8422987 -
Flags: review?(vyang)
Updated•11 years ago
|
Attachment #8422970 -
Flags: review?(vyang) → review+
Updated•11 years ago
|
Attachment #8422979 -
Flags: review?(vyang) → review+
Updated•11 years ago
|
Attachment #8422986 -
Flags: review?(vyang) → review+
Comment 58•11 years ago
|
||
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.
Attachment #8422987 -
Flags: review?(vyang)
Assignee | ||
Comment 59•11 years ago
|
||
(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.
Assignee | ||
Comment 60•11 years ago
|
||
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().
Attachment #8422987 -
Attachment is obsolete: true
Attachment #8423628 -
Flags: review?(vyang)
Comment 61•11 years ago
|
||
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.
Attachment #8423628 -
Flags: review?(vyang) → review+
Assignee | ||
Comment 62•11 years ago
|
||
(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.
Assignee | ||
Comment 63•11 years ago
|
||
address the problems in comment 61.
Attachment #8423628 -
Attachment is obsolete: true
Attachment #8423704 -
Flags: review+
Assignee | ||
Comment 64•11 years ago
|
||
update try server result:
https://tbpl.mozilla.org/?tree=Try&rev=2a48ad8bdec8
Keywords: checkin-needed
Comment 65•11 years ago
|
||
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
Flags: in-testsuite+
Keywords: checkin-needed
Comment 66•11 years ago
|
||
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
Assignee | ||
Comment 67•11 years ago
|
||
(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
Assignee | ||
Comment 68•11 years ago
|
||
Fix compiling error due to the signature of DOMRequest::FireDetailedError() is changed in bug 1011815.
Attachment #8426827 -
Flags: review+
Assignee | ||
Comment 69•11 years ago
|
||
Rebase to prevent merge conflict.
Attachment #8423704 -
Attachment is obsolete: true
Attachment #8426828 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #8422982 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Component: DOM: Device Interfaces → RIL
Product: Core → Firefox OS
Target Milestone: 2.0 S2 (23may) → 2.0 S3 (6june)
Version: Trunk → unspecified
Assignee | ||
Comment 70•11 years ago
|
||
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
Attachment #8426827 -
Attachment is obsolete: true
Assignee | ||
Comment 71•11 years ago
|
||
update tryserver result:
https://tbpl.mozilla.org/?tree=Try&rev=137be02546f8
Keywords: checkin-needed
Comment 72•11 years ago
|
||
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
Keywords: checkin-needed
Comment 73•11 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
You need to log in
before you can comment on or make changes to this bug.
Description
•