Last Comment Bug 708546 - Use DOMError in WebSMS
: Use DOMError in WebSMS
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Mounir Lamouri (:mounir)
:
:
Mentors:
Depends on: 705640
Blocks: websms 760011
  Show dependency treegraph
 
Reported: 2011-12-08 01:21 PST by Mounir Lamouri (:mounir)
Modified: 2012-06-09 19:35 PDT (History)
3 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (3.40 KB, patch)
2012-05-31 01:35 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v2 (8.79 KB, patch)
2012-06-05 02:45 PDT, Mounir Lamouri (:mounir)
bent.mozilla: review+
mounir: checkin+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2011-12-08 01:21:28 PST

    
Comment 1 Mounir Lamouri (:mounir) 2012-05-31 01:35:47 PDT
Created attachment 628623 [details] [diff] [review]
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!) 2012-06-04 14:40:29 PDT
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) 2012-06-05 02:45:03 PDT
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).
Comment 4 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-06-08 15:48:27 PDT
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) 2012-06-09 03:38:52 PDT
(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] 2012-06-09 19:35:08 PDT
https://hg.mozilla.org/mozilla-central/rev/745fc1421db4

Note You need to log in before you can comment on or make changes to this bug.