Closed
Bug 708546
Opened 13 years ago
Closed 12 years ago
Use DOMError in WebSMS
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: mounir, Assigned: mounir)
References
Details
Attachments
(1 file, 1 obsolete file)
8.79 KB,
patch
|
bent.mozilla
:
review+
mounir
:
checkin+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•12 years ago
|
||
This should work for the moment. We will move SmsRequest to DOMRequest at some point anyway.
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•12 years ago
|
||
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•12 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•12 years ago
|
Target Milestone: --- → mozilla16
Assignee | ||
Updated•12 years ago
|
Attachment #630116 -
Flags: checkin+
Comment 6•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/745fc1421db4
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•