Last Comment Bug 720632 - WebSMS: Expose SmsRequestManager as a scriptable XPCOM service
: WebSMS: Expose SmsRequestManager as a scriptable XPCOM service
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Philipp von Weitershausen [:philikon]
:
Mentors:
Depends on:
Blocks: websms 712809 720643 727319
  Show dependency treegraph
 
Reported: 2012-01-24 01:07 PST by Philipp von Weitershausen [:philikon]
Modified: 2012-02-20 10:21 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 (v1): IDL (3.30 KB, patch)
2012-01-27 10:54 PST, Philipp von Weitershausen [:philikon]
cjones.bugs: review+
gal: review+
Details | Diff | Review
Part 2 (WIP 1): XPCOMtaminate SmsRequestManager (20.24 KB, patch)
2012-01-31 11:38 PST, Philipp von Weitershausen [:philikon]
no flags Details | Diff | Review
Part 2 (v1): XPCOMtaminate SmsRequestManager (29.39 KB, patch)
2012-02-10 16:17 PST, Philipp von Weitershausen [:philikon]
cjones.bugs: review+
Details | Diff | Review
Part 3 (v1): XPCOM turds (5.28 KB, patch)
2012-02-10 17:05 PST, Philipp von Weitershausen [:philikon]
cjones.bugs: review+
Details | Diff | Review
Part 1 (v2): IDL (3.50 KB, patch)
2012-02-14 18:43 PST, Philipp von Weitershausen [:philikon]
no flags Details | Diff | Review
Part 2 (v2): XPCOMtaminate SmsRequestManager (29.65 KB, patch)
2012-02-14 18:44 PST, Philipp von Weitershausen [:philikon]
no flags Details | Diff | Review
Part 4 (v1): Android bits (16.50 KB, patch)
2012-02-17 17:22 PST, Philipp von Weitershausen [:philikon]
cjones.bugs: review+
Details | Diff | Review
Part 2 (v3): XPCOMtaminate SmsRequestManager (30.82 KB, patch)
2012-02-17 17:38 PST, Philipp von Weitershausen [:philikon]
no flags Details | Diff | Review

Description Philipp von Weitershausen [:philikon] 2012-01-24 01:07:55 PST
This is needed for the JS-based implementation of nsISmsDatabaseService (bug 712809).
Comment 1 Philipp von Weitershausen [:philikon] 2012-01-27 10:54:25 PST
Created attachment 592179 [details] [diff] [review]
Part 1 (v1): IDL

I'm taking a shot at this since it blocks some important SMS fixes. Here's the IDL, I'm working through the implementation right now.
Comment 2 Mounir Lamouri (:mounir) 2012-01-30 01:34:23 PST
Comment on attachment 592179 [details] [diff] [review]
Part 1 (v1): IDL

I would prefer to look at the entire patch at the same time if you don't mind.

This said, I can really do that this week (I was in the DOM Binding work week last week so I hadn't much time).
Do you still want to work on this?
Comment 3 Philipp von Weitershausen [:philikon] 2012-01-30 01:38:18 PST
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #2)
> I would prefer to look at the entire patch at the same time if you don't
> mind.

Fair enough. Let me see how far I can get it on Monday. If I get stuck somewhere, you're welcome to take it over. :)
Comment 4 Philipp von Weitershausen [:philikon] 2012-01-31 11:31:17 PST
(In reply to Philipp von Weitershausen [:philikon] from comment #3)
> Fair enough. Let me see how far I can get it on Monday. If I get stuck
> somewhere, you're welcome to take it over. :)

I'm putting this on hold until we decide whether WebSMS will adopt DOMRequest (bug 722626) or not. If so, this bug becomes obsolete. If not, then we can pick it up again.
Comment 5 Philipp von Weitershausen [:philikon] 2012-01-31 11:38:21 PST
Created attachment 593170 [details] [diff] [review]
Part 2 (WIP 1): XPCOMtaminate SmsRequestManager

Posting this here for posterity's sake. Grossly incomplete.
Comment 6 Philipp von Weitershausen [:philikon] 2012-02-10 16:17:11 PST
Created attachment 596215 [details] [diff] [review]
Part 2 (v1): XPCOMtaminate SmsRequestManager

I ended up finishing this real quick, seeing that DOMRequest might take a bit longer.
Comment 7 Philipp von Weitershausen [:philikon] 2012-02-10 16:17:37 PST
Comment on attachment 592179 [details] [diff] [review]
Part 1 (v1): IDL

Re-requesting review now that part 2 is up :)
Comment 8 Philipp von Weitershausen [:philikon] 2012-02-10 17:05:55 PST
Created attachment 596233 [details] [diff] [review]
Part 3 (v1): XPCOM turds

XPCOM registration bits.
Comment 9 Philipp von Weitershausen [:philikon] 2012-02-14 11:35:19 PST
Comment on attachment 592179 [details] [diff] [review]
Part 1 (v1): IDL

mounir is swamped and this is blocking the compmletion of WebSMS on Gonk, so over to cjones!
Comment 10 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-14 12:46:40 PST
Comment on attachment 592179 [details] [diff] [review]
Part 1 (v1): IDL

>diff --git a/dom/sms/interfaces/nsISmsRequestManager.idl b/dom/sms/interfaces/nsISmsRequestManager.idl

>+[scriptable, uuid(1638b963-3a45-4937-b6a9-280c1bfb166c)]
>+interface nsISmsRequestManager : nsISupports
>+{
>+
>+  const unsigned short eNoError       = 0;
>+  const unsigned short eNoSignalError = 1;
>+  const unsigned short eNotFoundError = 2;
>+  const unsigned short eUnknownError  = 3;
>+  const unsigned short eInternalError = 4;

I've mostly seen NO_ERROR for enum-alikes in IDL.  Deferring this call
to someone who knows IDL style.

>+  long createRequest(in nsPIDOMWindow aWindow,
>+                     in nsIScriptContext aScriptContext,

I don't know what this is for.  Need to defer this part of the review.

>+  /**
>+   * Track an already existing request object.
>+   *
>+   * @return the request ID.
>+   */
>+  long addRequest(in nsIDOMMozSmsRequest aRequest);
>+

Why are there separate createRequest/addRequest methods?  How can a
request be created outside the purview of SmsRequestManager?

--> gal for IDL style and nsIScriptContext
Comment 11 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-14 12:52:36 PST
Comment on attachment 592179 [details] [diff] [review]
Part 1 (v1): IDL

>diff --git a/dom/sms/interfaces/nsISmsRequestManager.idl b/dom/sms/interfaces/nsISmsRequestManager.idl

>+  const unsigned short eNoError       = 0;
>+  const unsigned short eNoSignalError = 1;
>+  const unsigned short eNotFoundError = 2;
>+  const unsigned short eUnknownError  = 3;
>+  const unsigned short eInternalError = 4;
>+

Also, we need to keep this comment here

  /**
   * All SMS related errors that could apply to SmsRequest objects.
   * Make sure to keep this list in sync with the list in:
   * embedding/android/GeckoSmsManager.java
   */
Comment 12 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-14 12:56:47 PST
Comment on attachment 592179 [details] [diff] [review]
Part 1 (v1): IDL

I understand addRequest now.  I don't like it, but it's not your design.

r=me with comment update above
Comment 13 Andreas Gal :gal 2012-02-14 13:21:14 PST
Comment on attachment 596215 [details] [diff] [review]
Part 2 (v1): XPCOMtaminate SmsRequestManager

Should the request id be long? (for sanity).

There is some commented out dead code in SmsRequestManager.h
Comment 14 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-14 13:21:47 PST
Comment on attachment 592179 [details] [diff] [review]
Part 1 (v1): IDL

>+  void notifySmsSendFailed(in long aRequestId,
>+                           in unsigned short aError);
>+

One more issue here: all these |unsigned short| params should be |long|.
Comment 15 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-14 13:27:11 PST
Comment on attachment 596215 [details] [diff] [review]
Part 2 (v1): XPCOMtaminate SmsRequestManager

Shame on you ;).

>diff --git a/dom/sms/src/SmsManager.cpp b/dom/sms/src/SmsManager.cpp

>+  nsCOMPtr<nsISmsRequestManager> requestManager = do_GetService(SMS_REQUEST_MANAGER_CONTRACTID);
>+  int requestId;

IDL |long| is |PRInt32|.

> NS_IMETHODIMP
> SmsManager::GetMessageMoz(PRInt32 aId, nsIDOMMozSmsRequest** aRequest)

>+  nsCOMPtr<nsISmsRequestManager> requestManager = do_GetService(SMS_REQUEST_MANAGER_CONTRACTID);
>+  int requestId;

And here.

>+  nsresult rv = requestManager->CreateRequest(mOwner, mScriptContext, aRequest,
>+                                              &requestId);
>+  if (NS_FAILED(rv)) {
>+    NS_ERROR("Failed to create the request!");
>+    return rv;
>+  }
> 

If I had reviewed this originally, I would have requested that all
this gunk be factored out into a helper method.  But I won't make you
do that for a mechanical rewrite.

> nsresult
> SmsManager::Delete(PRInt32 aId, nsIDOMMozSmsRequest** aRequest)

>-  NS_ASSERTION(*aRequest, "The request object must have been created!");
>+  nsCOMPtr<nsISmsRequestManager> requestManager = do_GetService(SMS_REQUEST_MANAGER_CONTRACTID);
>+  int requestId;

And here.

>+  nsCOMPtr<nsISmsRequestManager> requestManager = do_GetService(SMS_REQUEST_MANAGER_CONTRACTID);
>+  int requestId;

And here.

>diff --git a/dom/sms/src/SmsRequestManager.cpp b/dom/sms/src/SmsRequestManager.cpp

>+NS_IMETHODIMP
>+SmsRequestManager::AddRequest(nsIDOMMozSmsRequest* aRequest,
>+                              PRInt32* aRequestId)
> {
>   // TODO: merge with CreateRequest
>   PRInt32 size = mRequests.Count();
> 
>   // Look for empty slots.
>   for (PRInt32 i=0; i<size; ++i) {
>     if (mRequests[i]) {
>       continue;
>     }
> 
>     mRequests.ReplaceObjectAt(aRequest, i);
>     return i;

Bzzzrt.

>+NS_IMETHODIMP
> SmsRequestManager::CreateRequest(nsPIDOMWindow* aWindow,
>                                  nsIScriptContext* aScriptContext,
>-                                 nsIDOMMozSmsRequest** aRequest)
>+                                 nsIDOMMozSmsRequest** aRequest,
>+                                 PRInt32* aRequestId)
> {
>   nsCOMPtr<nsIDOMMozSmsRequest> request =
>     new SmsRequest(aWindow, aScriptContext);
> 
>   PRInt32 size = mRequests.Count();
> 
>   // Look for empty slots.
>   for (PRInt32 i=0; i<size; ++i) {
>@@ -114,17 +95,18 @@ SmsRequestManager::CreateRequest(nsPIDOM
> 
>     mRequests.ReplaceObjectAt(request, i);
>     NS_ADDREF(*aRequest = request);
>     return i;

And here.

>+NS_IMETHODIMP
>+SmsRequestManager::NotifySmsSendFailed(PRInt32 aRequestId, PRUint16 aError)

PRInt32 aError and everywhere below.

>diff --git a/dom/sms/src/SmsRequestManager.h b/dom/sms/src/SmsRequestManager.h

>+/*
> class nsIDOMMozSmsRequest;
> class nsPIDOMWindow;
> class nsIScriptContext;
> class nsIDOMMozSmsMessage;
>+*/
> 

Kill this.

r=me with those fixes
Comment 16 Mounir Lamouri (:mounir) 2012-02-14 15:13:10 PST
Comment on attachment 596215 [details] [diff] [review]
Part 2 (v1): XPCOMtaminate SmsRequestManager

Review of attachment 596215 [details] [diff] [review]:
-----------------------------------------------------------------

Removing the ErrorType enum makes me sad :(

Seems good to go though.

::: dom/sms/src/SmsCursor.cpp
@@ +115,5 @@
>    mMessage = nsnull;
>    static_cast<SmsRequest*>(mRequest.get())->Reset();
>  
> +  nsCOMPtr<nsISmsRequestManager> requestManager = do_GetService(SMS_REQUEST_MANAGER_CONTRACTID);
> +  PRInt32 requestId;

nit: leave a blank line between those two lines

::: dom/sms/src/SmsManager.cpp
@@ +146,5 @@
>  
>    nsCOMPtr<nsIDOMMozSmsRequest> request;
>  
> +  nsCOMPtr<nsISmsRequestManager> requestManager = do_GetService(SMS_REQUEST_MANAGER_CONTRACTID);
> +  int requestId;

nit: blank line between those two lines

@@ +222,5 @@
>  NS_IMETHODIMP
>  SmsManager::GetMessageMoz(PRInt32 aId, nsIDOMMozSmsRequest** aRequest)
>  {
> +  nsCOMPtr<nsISmsRequestManager> requestManager = do_GetService(SMS_REQUEST_MANAGER_CONTRACTID);
> +  int requestId;

nit: ditto

@@ +243,5 @@
>  nsresult
>  SmsManager::Delete(PRInt32 aId, nsIDOMMozSmsRequest** aRequest)
>  {
> +  nsCOMPtr<nsISmsRequestManager> requestManager = do_GetService(SMS_REQUEST_MANAGER_CONTRACTID);
> +  int requestId;

nit: ditto

@@ +293,5 @@
>      filter = new SmsFilter();
>    }
>  
> +  nsCOMPtr<nsISmsRequestManager> requestManager = do_GetService(SMS_REQUEST_MANAGER_CONTRACTID);
> +  int requestId;

nit: ditto

::: dom/sms/src/SmsRequest.cpp
@@ +264,2 @@
>        aError.AssignLiteral("InternalError");
>        break;

Please add a |default| statement with a MOZ_ASSERT.

::: dom/sms/src/SmsRequestManager.h
@@ +47,5 @@
>  class nsIDOMMozSmsRequest;
>  class nsPIDOMWindow;
>  class nsIScriptContext;
>  class nsIDOMMozSmsMessage;
> +*/

Please, remove this block if not needed. Don't keep it commented.

::: embedding/android/GeckoSmsManager.java
@@ +328,5 @@
>  
>    /*
> +   * Make sure that the following error codes are in sync with the ones
> +   * defined in dom/sms/interfaces/nsISmsRequestManager.idl. They are owned
> +   * owned by the DOM.

s/DOM/interface/
Comment 17 Philipp von Weitershausen [:philikon] 2012-02-14 18:43:25 PST
Created attachment 597275 [details] [diff] [review]
Part 1 (v2): IDL

Addressed review comments.
Comment 18 Philipp von Weitershausen [:philikon] 2012-02-14 18:44:02 PST
Created attachment 597276 [details] [diff] [review]
Part 2 (v2): XPCOMtaminate SmsRequestManager

Addressed review comments.
Comment 20 Ed Morley [:emorley] 2012-02-16 15:20:28 PST
Push backed out for build failures on Android:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=6c1ded13556d

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2e1b248f40d
Comment 21 Ed Morley [:emorley] 2012-02-17 03:09:45 PST
> Push backed out for build failures on Android:

Also later failed on Windows.
Comment 22 Philipp von Weitershausen [:philikon] 2012-02-17 17:22:57 PST
Created attachment 598450 [details] [diff] [review]
Part 4 (v1): Android bits

Yeah, so, I kinda forgot to change Android's usage of SmsRequestManager. Pretty mechanical, just like the rest. In the old fashion r?cjones with potential fallback to mounir.
Comment 23 Philipp von Weitershausen [:philikon] 2012-02-17 17:38:42 PST
Created attachment 598458 [details] [diff] [review]
Part 2 (v3): XPCOMtaminate SmsRequestManager

I had to change the NO_ERROR constant to SUCCESS_NO_ERROR to avoid breakage on Windows (where apparently there's a #define NO_ERROR that breaks things in unexpected ways.) Stoopid Windoze.
Comment 24 Mounir Lamouri (:mounir) 2012-02-17 19:13:12 PST
Comment on attachment 598450 [details] [diff] [review]
Part 4 (v1): Android bits

Review of attachment 598450 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/android/AndroidJNI.cpp
@@ +344,5 @@
>          obs->NotifyObservers(message, kSmsSentObserverTopic, nsnull);
>  
>          if (mProcessId == 0) { // Parent process.
> +          nsCOMPtr<nsISmsRequestManager> requestManager;
> +            = do_GetService(SMS_REQUEST_MANAGER_CONTRACTID);

You probably want:
nsCOMPtr<nsISmsRequestManager> requestManager =
  do_GetService(SMS_REQUEST_MANAGER_CONTRACTID);

I assume you do.

In addition, |do_GetService| can return |nsnull| so you should check the result.

You are doing this mistake in a lot of places after. I will not point all of them.
Comment 25 Mozilla RelEng Bot 2012-02-17 21:45:21 PST
Try run for eb9207c37e3a is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=eb9207c37e3a
Results (out of 14 total builds):
    success: 11
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/pweitershausen@mozilla.com-eb9207c37e3a
Comment 26 Philipp von Weitershausen [:philikon] 2012-02-17 22:33:47 PST
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #24)
> Comment on attachment 598450 [details] [diff] [review]
> Part 4 (v1): Android bits
> 
> Review of attachment 598450 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/android/AndroidJNI.cpp
> @@ +344,5 @@
> >          obs->NotifyObservers(message, kSmsSentObserverTopic, nsnull);
> >  
> >          if (mProcessId == 0) { // Parent process.
> > +          nsCOMPtr<nsISmsRequestManager> requestManager;
> > +            = do_GetService(SMS_REQUEST_MANAGER_CONTRACTID);
> 
> You probably want:
> nsCOMPtr<nsISmsRequestManager> requestManager =
>   do_GetService(SMS_REQUEST_MANAGER_CONTRACTID);
> 
> I assume you do.

*cough* yes. Thanks for noticing... It was late. :)

> In addition, |do_GetService| can return |nsnull| so you should check the
> result.

Good point. I will do that.
Comment 27 Mozilla RelEng Bot 2012-02-18 02:16:49 PST
Try run for 02a950d519e7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=02a950d519e7
Results (out of 14 total builds):
    success: 11
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/pweitershausen@mozilla.com-02a950d519e7
Comment 28 Mozilla RelEng Bot 2012-02-19 02:00:37 PST
Try run for f8c7bb9ad347 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f8c7bb9ad347
Results (out of 3 total builds):
    success: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/pweitershausen@mozilla.com-f8c7bb9ad347

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