Use DOMError in WebSMS

RESOLVED FIXED in mozilla16

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

Trunk
mozilla16
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

8.79 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
mounir
: checkin+
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 628623 [details] [diff] [review]
Patch v1

This should work for the moment. We will move SmsRequest to DOMRequest at some point anyway.
Assignee: nobody → mounir
Status: NEW → ASSIGNED
Attachment #628623 - Flags: review?(bent.mozilla)
(Assignee)

Updated

5 years ago
Blocks: 760011
Comment on attachment 628623 [details] [diff] [review]
Patch v1

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

::: dom/sms/interfaces/nsIDOMSmsRequest.idl
@@ +9,3 @@
>  
>  [scriptable, uuid(1b24469d-cfb7-4667-aaf0-c1d17289ae7c)]
>  interface nsIDOMMozSmsRequest : nsIDOMEventTarget

Need to bump uuid

::: dom/sms/src/SmsRequest.cpp
@@ +228,3 @@
>        break;
>      case nsISmsRequestManager::NO_SIGNAL_ERROR:
> +      *aError = DOMError::CreateWithName(NS_LITERAL_STRING("NoSignalError")).get();

Talked with jonas and we both think that you shouldn't return a new error object each time, but rather hang on to one and hand it out each time.
(Assignee)

Comment 3

5 years ago
Created attachment 630116 [details] [diff] [review]
Patch v2

Indeed, returning always the same DOMError object makes sense. This patch does that (and updates the UUID).
Attachment #628623 - Attachment is obsolete: true
Attachment #628623 - Flags: review?(bent.mozilla)
Attachment #630116 - Flags: review?(bent.mozilla)
Comment on attachment 630116 [details] [diff] [review]
Patch v2

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

::: dom/sms/src/SmsRequest.cpp
@@ +188,3 @@
>    NS_PRECONDITION(mResult == JSVAL_VOID, "mResult shouldn't have been set!");
> +  MOZ_ASSERT(aError != nsISmsRequestManager::SUCCESS_NO_ERROR,
> +             "Can't call SetError() with SUCCESS_NO_ERROR!");

I really hate mixing these macros. And the MOZ_ASSERT doesn't generate stack traces on tinderbox :( Would you mind using NS_PRECONDITION here?
Attachment #630116 - Flags: review?(bent.mozilla) → review+
(Assignee)

Comment 5

5 years ago
(In reply to ben turner [:bent] from comment #4)
> Comment on attachment 630116 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 630116 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/sms/src/SmsRequest.cpp
> @@ +188,3 @@
> >    NS_PRECONDITION(mResult == JSVAL_VOID, "mResult shouldn't have been set!");
> > +  MOZ_ASSERT(aError != nsISmsRequestManager::SUCCESS_NO_ERROR,
> > +             "Can't call SetError() with SUCCESS_NO_ERROR!");
> 
> I really hate mixing these macros. And the MOZ_ASSERT doesn't generate stack
> traces on tinderbox :( Would you mind using NS_PRECONDITION here?

Not at all. And thanks for the review :)
(Assignee)

Updated

5 years ago
Target Milestone: --- → mozilla16
(Assignee)

Updated

5 years ago
Attachment #630116 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/745fc1421db4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.