Closed Bug 708546 Opened 13 years ago Closed 12 years ago

Use DOMError in WebSMS

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: mounir, Assigned: mounir)

References

Details

Attachments

(1 file, 1 obsolete file)

      No description provided.
Attached patch Patch v1 (obsolete) — Splinter Review
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)
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.
Attached patch Patch v2Splinter Review
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+
(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 :)
Target Milestone: --- → mozilla16
Attachment #630116 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/745fc1421db4
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: