958782 - Convert nsIDOMMozMobileMessageManager to webidl

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

nsIDOMMobileMessageManager is current written in XPIDL and because of that it has to do a bunch of manual JSAPI stuff that we'd rather not have.

We should convert it to WebIDL, which will should get rid of the manual JSAPI stuff and hopefully make the API better defined and cleaner.

Comment 1

[Vicamo Yang [:vicamo][:vyang]]

Duplicate bug 859764?

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Oops, yes.

Vicamo, are you actively working on that bug?

Comment 3

[Vicamo Yang [:vicamo][:vyang]]

Reopen here to make some progress.

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/3: webidl/xpidl changes

1) Rename dom/webidl/MobileMessageManager.webidl to dom/webidl/MozMobileMessageManager.idl
2) Add MozMobileMessageManager WebIDL definition.

Comment 5

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2.a/3: refactorings, alignments, renames

This patch cleans up some bits so that we can have a much more trivial part 2.b patch that really implements WebIDL stuff.  This patch has no functional change.

Comment 6

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2.b/3: WebIDL implementation

Comment 7

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3/3: fix test cases.

WebIDL converts null to string "null" for MozMobileMessageManager::GetSegmentInfoForText(), so one test case in test_getsegmentinfofortext.js no longer throws and fails the run.

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

Full try in progress: https://tbpl.mozilla.org/?tree=Try&rev=e969d7ab1e4c

Comment 9

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3/3: fix test cases. : v2

Fix b2g-desktop mochitest failure.

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8433037 [details] [diff] [review]
part 2.b/3: WebIDL implementation

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

A lot of this code could be simplified by using WebIDL sequences etc.; I'll leave it to khuey to decide whether to do that in this bug.

::: dom/mobilemessage/src/MobileMessageManager.cpp
@@ +125,2 @@
>  
> +  return request.forget().downcast<DOMRequest>();

Should not need the downcast

@@ +333,2 @@
>  
> +  return request.forget().downcast<DOMRequest>();

Ditto

::: dom/mobilemessage/src/MobileMessageManager.h
@@ +30,4 @@
>  
>    NS_REALLY_FORWARD_NSIDOMEVENTTARGET(DOMEventTargetHelper)
>  
> +  MobileMessageManager(nsPIDOMWindow *aWindow);

* to the left

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

(In reply to :Ms2ger from comment #10)
> Comment on attachment 8433037 [details] [diff] [review]
> -----------------------------------------------------------------
> A lot of this code could be simplified by using WebIDL sequences etc.; I'll
> leave it to khuey to decide whether to do that in this bug.

Two problems: we don't have array support and union can't have sequence<T> members.  Both Send() and Delete() take an union parameter.  Any other solutions?

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

(In reply to :Ms2ger from comment #10)
> Review of attachment 8433037 [details] [diff] [review]:

BTW, thanks for your quick feedback. :)

Comment 13

[Manish Goregaokar [:manishearth]]

(Channeling Ms2ger who has internet problems right now)

Perhaps
> +  [Throws]
> +  DOMRequest send(DOMString number, DOMString message,
> +                  optional SmsSendParameters sendParameters);
> +  [Throws]
> +  sequence<DOMRequest> send(sequence<DOMString> numbers, DOMString message,
> +                            optional SmsSendParameters sendParameters);
and
> +  DOMRequest delete(long id);
> +  DOMRequest delete(MozSmsMessage message);
> +  DOMRequest delete(MozMmsMessage message);
> +  DOMRequest delete(sequence<(long or MozSmsMessage or MozMmsMessage)> params);
would work.

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/3: webidl/xpidl changes : v2

1) Use WebIDL suggested in comment 13,
2) add extended attribute [Pref="dom.sms.enabled"] to MozMobileMessageManager,
3) accept MozMmsMessage in |retrieveMMS| as well.

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2.b/3: WebIDL implementation : v2

1) address comment 10,
2) accommodate to suggested WebIDL rules in comment 13,
3) s/uint32_t/int32_t/ for message id field of several methods to strictly conform to what XPIDL and WebIDL have defined.

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8435093 [details] [diff] [review]
part 1/3: webidl/xpidl changes : v2

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

This seems fine, but it should be merged with the other patches.

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2.c/3: remove unused headers and sort

Suggestions in comment 13 also eliminates most JS API usage, so several headers can be removed now.

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8433036 [details] [diff] [review]
part 2.a/3: refactorings, alignments, renames

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

::: dom/mobilemessage/src/MobileMessageManager.cpp
@@ +198,5 @@
>      if (!sendParams.Init(aCx, param)) {
>        return NS_ERROR_TYPE_ERR;
>      }
> +  }
> +  if (sendParams.mServiceId.WasPassed()) {

nit: \n above if

@@ +270,5 @@
>      if (!sendParams.Init(aCx, param)) {
>        return NS_ERROR_TYPE_ERR;
>      }
> +  }
> +  if (sendParams.mServiceId.WasPassed()) {

here too

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8435096 [details] [diff] [review]
part 2.b/3: WebIDL implementation : v2

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

::: dom/mobilemessage/src/MobileMessageManager.cpp
@@ +339,5 @@
> +MobileMessageManager::Delete(const Sequence<OwningLongOrMozSmsMessageOrMozMmsMessage>& aParams,
> +                             ErrorResult& aRv)
> +{
> +  const uint32_t size = aParams.Length();
> +  nsAutoArrayPtr<int32_t> idAutoArray(new int32_t[size]);

Very nit-picky, but please use the fallible new and null check

Comment 21

[Vicamo Yang [:vicamo][:vyang]]

review ping.

Comment 22

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8435096 [details] [diff] [review]
part 2.b/3: WebIDL implementation : v2

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

::: dom/mobilemessage/src/MobileMessageManager.cpp
@@ +241,4 @@
>  
> +  AutoSafeJSContext cx;
> +  JS::Rooted<JS::Value> val(cx);
> +  if (!aParams.ToObject(cx, &val)) {

bz, what's the status of ToObject?

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8435096 [details] [diff] [review]
part 2.b/3: WebIDL implementation : v2

The status is that it's gone.  You should be using ToJSValue.

Also, you should probably be entering some compartment in SendMMS, not just doing AutoSafeJSContext, right Bobby?

Comment 24

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #23)
> Comment on attachment 8435096 [details] [diff] [review]
> part 2.b/3: WebIDL implementation : v2
> 
> The status is that it's gone.  You should be using ToJSValue.
> 
> Also, you should probably be entering some compartment in SendMMS, not just
> doing AutoSafeJSContext, right Bobby?

Yes.

Before bug 1025476 lands, you should do:

if (!NS_WARN_IF(GetOwner()->GetGlobalJSObject())) {
  return nullptr;
}
AutoJSAPI jsapi;
JSContext *cx = jsapi.cx();
JSAutoCompartment ac(cx, GetOwner()->GetGlobalJSObject());

After bug 1025476 lands, you should do:

AutoJSAPI jsapi;
if (!NS_WARN_IF(jsapi.init(GetOwner())) {
  return nullptr;
}

Comment 25

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Bobby Holley (:bholley) from comment #24)
> (In reply to Boris Zbarsky [:bz] from comment #23)
> > Comment on attachment 8435096 [details] [diff] [review]
> > part 2.b/3: WebIDL implementation : v2
> > 
> > The status is that it's gone.  You should be using ToJSValue.

Thank you.  I've already include this in my local working branch.

> > Also, you should probably be entering some compartment in SendMMS, not just
> > doing AutoSafeJSContext, right Bobby?
> 
> Yes.
> 
> Before bug 1025476 lands, you should do:
> 
> if (!NS_WARN_IF(GetOwner()->GetGlobalJSObject())) {
>   return nullptr;
> }
> AutoJSAPI jsapi;
> JSContext *cx = jsapi.cx();
> JSAutoCompartment ac(cx, GetOwner()->GetGlobalJSObject());
> 
> After bug 1025476 lands, you should do:
> 
> AutoJSAPI jsapi;
> if (!NS_WARN_IF(jsapi.init(GetOwner())) {
>   return nullptr;
> }

Thank you.

Comment 26

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/3: webidl/xpidl changes : v3

Rebase only.

Comment 27

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2.a/3: refactorings, alignments, renames : v2

1) Address review comment 19.
2) Pass nsISmsService instance to Send(...) so that we don't bother calling do_GetService() again and again.

Comment 28

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2.b/3: WebIDL implementation : v3

1) Address review comment 20 -- using FallibleArray instead
2) Address review comment 24 -- use AutoJSAPI::InitUsingWin.  Also fixes MobileConnection.

Comment 29

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2.c/3: remove unused headers and sort : v2

Rebase only.

Comment 30

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2.b/3: WebIDL implementation : v4

Fix -Werror,-Wunused-variable only. Full try in https://tbpl.mozilla.org/?tree=Try&rev=92a98cbac4b2 .

Comment 31

[Bobby Holley (:bholley)]

Comment on attachment 8442787 [details] [diff] [review]
part 2.b/3: WebIDL implementation : v4

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

AutoJSAPI stuff looks good - thanks for doing it.

Comment 32

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #30)
> Full try in https://tbpl.mozilla.org/?tree=Try&rev=92a98cbac4b2 .

Fails dom/tests/mochitest/general/test_interfaces.html because MozMobileMessageManager is now guarded by preference "dom.sms.enabled" in part 1/3 as other RIL interfaces do.  Had another revision for part 3/3 and is trying mochitest again in https://tbpl.mozilla.org/?tree=Try&rev=2dec47a33c60 .

Comment 33

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3/3: fix test cases. : v3

Fix failure in "dom/tests/mochitest/general/test_interfaces.html", therefore need additional reviews from DOM peers. Now MozMobileMessageManager has same conditions with other RIL components.

Comment 34

[Vicamo Yang [:vicamo][:vyang]]

Thank you, Kyle!

Comment 35

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/033708b53979
https://hg.mozilla.org/integration/b2g-inbound/rev/15a281081b97
https://hg.mozilla.org/integration/b2g-inbound/rev/731f37afb649
https://hg.mozilla.org/integration/b2g-inbound/rev/f06d2ba57fb3
https://hg.mozilla.org/integration/b2g-inbound/rev/29a1c6a34550

Comment 36

[Carsten Book [:Tomcat]]

sorry had to back out this changes for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=42263193&tree=B2g-Inbound

Comment 37

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Carsten Book [:Tomcat] from comment #36)
> sorry had to back out this changes for test failures like
> https://tbpl.mozilla.org/php/getParsedLog.php?id=42263193&tree=B2g-Inbound

I'm the one to say sorry. :(
Besides the unexpected -Werror,-Wmismatched-tags error, I'm supposed to land this bug *after* bug 1025476.

Comment 38

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2.b/3: WebIDL implementation : v5

Fix -Werror,-Wmismatched-tags error on Mac OS X, Windows. MmsParameters/MmsSendParameters/SmsSendParameters was previously declared as struct, not class.

Comment 39

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3/3: fix test cases. : v4

Update commit summary to include r=khuey only.

Comment 40

[Vicamo Yang [:vicamo][:vyang]]

Full try again in https://tbpl.mozilla.org/?tree=Try&rev=288c95845afb

Comment 41

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/61d0e4bb1ebb
https://hg.mozilla.org/integration/b2g-inbound/rev/829c05c420e0
https://hg.mozilla.org/integration/b2g-inbound/rev/61d118d19b05
https://hg.mozilla.org/integration/b2g-inbound/rev/c2fb5b4eeabe
https://hg.mozilla.org/integration/b2g-inbound/rev/168ea7238c94

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/61d0e4bb1ebb
https://hg.mozilla.org/mozilla-central/rev/829c05c420e0
https://hg.mozilla.org/mozilla-central/rev/61d118d19b05
https://hg.mozilla.org/mozilla-central/rev/c2fb5b4eeabe
https://hg.mozilla.org/mozilla-central/rev/168ea7238c94

Comment 43

[Ed Morley [:emorley]]

We're seeing this on inbound, after this merged from mozilla-central on both clobber and non-clobber builds:
https://tbpl.mozilla.org/php/getParsedLog.php?id=42445354&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=42444802&tree=Mozilla-Inbound

I'm guessing yet-to-be-merged-from-inbound landings added more uses that need converting - would you mind taking a look?

Comment 44

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Ed Morley [:edmorley UTC+0] from comment #43)
> We're seeing this on inbound, after this merged from mozilla-central on both
> clobber and non-clobber builds:
> 
> I'm guessing yet-to-be-merged-from-inbound landings added more uses that
> need converting - would you mind taking a look?

I'm not sure I know what happens to m-i, but, yes, I'm looking at it.

Comment 45

[Vicamo Yang [:vicamo][:vyang]]

Bug 1029866 renames InitUsingWin() to Init() again.

Comment 46

[Vicamo Yang [:vicamo][:vyang]]

Attachment: fix-inbound-on-closed-tree.patch

Comment 47

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8e1d53f295a

Comment 48

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e8e1d53f295a