Mounir Lamouri (:mounir) Assignee
	Description • 13 years ago

      No description provided.

	Mounir Lamouri (:mounir) Assignee
	Comment 1 • 12 years ago

This should work for the moment. We will move SmsRequest to DOMRequest at some point anyway.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 2 • 12 years ago

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.

	Mounir Lamouri (:mounir) Assignee
	Comment 3 • 12 years ago

Indeed, returning always the same DOMError object makes sense. This patch does that (and updates the UUID).

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 4 • 12 years ago

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?

	Mounir Lamouri (:mounir) 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 :)

	Ryan VanderMeulen [:RyanVM]
	Comment 6 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/745fc1421db4