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.

Duplicate bug 859764?

Oops, yes. Vicamo, are you actively working on that bug?

Reopen here to make some progress.

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

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.

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

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

Fix b2g-desktop mochitest failure.

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

(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?

(In reply to :Ms2ger from comment #10) > Review of attachment 8433037 [details] [diff] [review]: BTW, thanks for your quick feedback. :)

(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.

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

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 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.

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

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 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

review ping.

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 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?

(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; }

(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.

Rebase only.

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

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

Rebase only.

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

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.

(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 .

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.

Thank you, Kyle!

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

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

(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.

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

Update commit summary to include r=khuey only.

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

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

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

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?

(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.

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