708546 - Use DOMError in WebSMS

Status: RESOLVED FIXED

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

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

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

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

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

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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?

Comment 5

[Mounir Lamouri (:mounir)]

(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 :)

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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