Closed Bug 958782 Opened 6 years ago Closed 5 years ago

Convert nsIDOMMozMobileMessageManager to webidl

Categories

(Core :: DOM: Device Interfaces, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
2.0 S5 (4july)
tracking-b2g backlog

People

(Reporter: mrbkap, Assigned: vicamo)

References

Details

(Whiteboard: [ft:ril][p=3])

Attachments

(6 files, 11 obsolete files)

12.85 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
13.52 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
2.60 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
39.54 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
3.38 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
3.02 KB, patch
Details | Diff | Splinter Review
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.
Assignee: nobody → tclancy
Summary: Convert nsIDOMMobileMessageManager to webidl → Convert nsIDOMMozMobileMessageManager to webidl
Duplicate bug 859764?
Oops, yes.

Vicamo, are you actively working on that bug?
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 859764
Reopen here to make some progress.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Blocks: 859614, 888591, 859764
Attached patch part 1/3: webidl/xpidl changes (obsolete) — Splinter Review
1) Rename dom/webidl/MobileMessageManager.webidl to dom/webidl/MozMobileMessageManager.idl
2) Add MozMobileMessageManager WebIDL definition.
Assignee: tclancy → vyang
Attachment #8433034 - Flags: review?(khuey)
Attachment #8433034 - Flags: review?(Ms2ger)
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.
Attachment #8433036 - Flags: review?(Ms2ger)
Attachment #8433037 - Flags: review?(Ms2ger)
Attached patch part 3/3: fix test cases. (obsolete) — Splinter Review
WebIDL converts null to string "null" for MozMobileMessageManager::GetSegmentInfoForText(), so one test case in test_getsegmentinfofortext.js no longer throws and fails the run.
Attachment #8433043 - Flags: review?(gene.lian)
Attached patch part 3/3: fix test cases. : v2 (obsolete) — Splinter Review
Fix b2g-desktop mochitest failure.
Attachment #8433043 - Attachment is obsolete: true
Attachment #8433043 - Flags: review?(gene.lian)
Attachment #8433064 - Flags: review?(gene.lian)
Attachment #8433034 - Flags: review?(Ms2ger) → feedback+
Attachment #8433036 - Flags: review?(khuey)
Attachment #8433036 - Flags: review?(Ms2ger)
Attachment #8433036 - Flags: feedback+
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
Attachment #8433037 - Flags: review?(khuey)
Attachment #8433037 - Flags: review?(Ms2ger)
Attachment #8433037 - Flags: feedback+
(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?
Flags: needinfo?(Ms2ger)
(In reply to :Ms2ger from comment #10)
> Review of attachment 8433037 [details] [diff] [review]:

BTW, thanks for your quick feedback. :)
Whiteboard: [p=3]
(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.
Flags: needinfo?(Ms2ger)
Attachment #8433064 - Flags: review?(gene.lian) → review+
1) Use WebIDL suggested in comment 13,
2) add extended attribute [Pref="dom.sms.enabled"] to MozMobileMessageManager,
3) accept MozMmsMessage in |retrieveMMS| as well.
Attachment #8433034 - Attachment is obsolete: true
Attachment #8433034 - Flags: review?(khuey)
Attachment #8435093 - Flags: review?(khuey)
Attachment #8435093 - Flags: feedback?(Ms2ger)
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.
Attachment #8433037 - Attachment is obsolete: true
Attachment #8433037 - Flags: review?(khuey)
Attachment #8435096 - Flags: review?(khuey)
Attachment #8435096 - Flags: feedback?(Ms2ger)
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.
Attachment #8435093 - Flags: feedback?(Ms2ger) → feedback+
Suggestions in comment 13 also eliminates most JS API usage, so several headers can be removed now.
Attachment #8435109 - Flags: review?(khuey)
Duplicate of this bug: 1012707
Whiteboard: [p=3] → [ft:ril][p=3]
Target Milestone: --- → 2.0 S4 (20june)
Blocks: 916607
Blocks: 878533
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
Attachment #8433036 - Flags: review?(khuey) → review+
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
Attachment #8435096 - Flags: review?(khuey) → review+
blocking-b2g: --- → backlog
review ping.
Flags: needinfo?(Ms2ger)
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?
Attachment #8435096 - Flags: feedback?(Ms2ger) → feedback?(bzbarsky)
Flags: needinfo?(Ms2ger)
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?
Attachment #8435096 - Flags: feedback?(bzbarsky) → feedback+
Flags: needinfo?(bobbyholley)
(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;
}
Flags: needinfo?(bobbyholley)
(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.
Depends on: 1025476
Rebase only.
Attachment #8435093 - Attachment is obsolete: true
Attachment #8442673 - Flags: review+
1) Address review comment 19.
2) Pass nsISmsService instance to Send(...) so that we don't bother calling do_GetService() again and again.
Attachment #8433036 - Attachment is obsolete: true
Attachment #8442674 - Flags: review+
1) Address review comment 20 -- using FallibleArray instead
2) Address review comment 24 -- use AutoJSAPI::InitUsingWin.  Also fixes MobileConnection.
Attachment #8435096 - Attachment is obsolete: true
Attachment #8442689 - Flags: feedback?(bobbyholley)
Rebase only.
Attachment #8435109 - Attachment is obsolete: true
Attachment #8442690 - Flags: review+
Fix -Werror,-Wunused-variable only. Full try in https://tbpl.mozilla.org/?tree=Try&rev=92a98cbac4b2 .
Attachment #8442689 - Attachment is obsolete: true
Attachment #8442689 - Flags: feedback?(bobbyholley)
Attachment #8442787 - Flags: feedback?(bobbyholley)
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.
Attachment #8442787 - Flags: feedback?(bobbyholley) → feedback+
(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 .
Attached patch part 3/3: fix test cases. : v3 (obsolete) — Splinter Review
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.
Attachment #8433064 - Attachment is obsolete: true
Attachment #8444312 - Flags: review?(bobbyholley)
Thank you, Kyle!
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.
Attachment #8442787 - Attachment is obsolete: true
Attachment #8444988 - Flags: review+
Update commit summary to include r=khuey only.
Attachment #8444312 - Attachment is obsolete: true
Attachment #8444990 - Flags: review+
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?
Status: RESOLVED → REOPENED
Flags: needinfo?(vyang)
Resolution: FIXED → ---
(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.
Flags: needinfo?(vyang)
Bug 1029866 renames InitUsingWin() to Init() again.
https://hg.mozilla.org/mozilla-central/rev/e8e1d53f295a
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: 2.0 S4 (20june) → 2.0 S5 (4july)
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.