Closed Bug 859764 Opened 7 years ago Closed 4 years ago

WebSMS: move all MobileMessage interfaces to WebIDL

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(tracking-b2g:backlog, firefox45 fixed)

RESOLVED FIXED
2.6 S1 - 11/20
tracking-b2g backlog
Tracking Status
firefox45 --- fixed

People

(Reporter: vicamo, Assigned: bevis)

References

()

Details

(Whiteboard: [grooming][ft:ril][p=5])

Attachments

(7 files, 27 obsolete files)

60.36 KB, patch
bevis
: review+
Details | Diff | Splinter Review
10.99 KB, patch
bevis
: review+
Details | Diff | Splinter Review
12.85 KB, patch
bevis
: review+
Details | Diff | Splinter Review
45.26 KB, patch
bevis
: review+
Details | Diff | Splinter Review
51.00 KB, patch
bevis
: review+
Details | Diff | Splinter Review
9.61 KB, patch
bevis
: review+
Details | Diff | Splinter Review
3.44 KB, patch
bevis
: review+
Details | Diff | Splinter Review
No description provided.
Component: DOM: CSS Object Model → DOM: Device Interfaces
Amending this bug to convert all DOM types instead.
Summary: WebSMS: rewrite MobileMessageThread with WebIDL → WebSMS: move to WebIDL
Blocks: 859616
No longer blocks: 859616
Blocks: 878533
No longer blocks: 878533
Depends on: 878533
Blocks: 888591
Move only mozMobileMessage first to close bug 888591 as soon as possible.
Summary: WebSMS: move to WebIDL → WebSMS: move to MozMobileMessage WebIDL
Blocks: 859614
Dictionary-typed MmsAttachment can't be an attribute of interface MozMmsAttachmentList when moving to WebIDL, so we must create an interface for MMS attachment instances.  However, we still need a dictionary type representing input parameter of MozMobileMessageManager::sendMMS.  Created a MmsAttachmentDict for that.  And, since that dictionary type is only used in MozMobileMessageManager, move it to MobileMessageManager.webidl as wel.
Assignee: nobody → vyang
Attached patch part 2.b/N: implement : WIP (obsolete) — Splinter Review
No longer depends on: 878533
Duplicate of this bug: 878533
Comment on attachment 816629 [details] [diff] [review]
part 2.a/N: Move all MobileMessage interfaces to WebIDL.  Interface changes. WIP

May I have your early feedback?
Attachment #816629 - Flags: feedback?(Ms2ger)
Comment on attachment 816629 [details] [diff] [review]
part 2.a/N: Move all MobileMessage interfaces to WebIDL.  Interface changes. WIP

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

Don't see anything obviously wrong from a quick skim.
Attachment #816629 - Flags: feedback?(Ms2ger)
Duplicate of this bug: 958782
Depends on: 958782
No longer blocks: 888591
Depends on: 916607
Depends on: 1023148
Whiteboard: [ft:ril][p=5]
Target Milestone: --- → 2.0 S5 (4july)
Vicamo, are you targeting this for 2.0?
bug 958782 already moved MozMobileMessageManager to webidl. We need to move all the rest, too.

In addition, we should have [AvailableIn="CertifiedApps"] and [CheckPermissions="sms"] to better hide these interfaces.
Blocks: 1116665
Component: DOM: Device Interfaces → RIL
Product: Core → Firefox OS
Summary: WebSMS: move to MozMobileMessage WebIDL → WebSMS: move all MobileMessage interfaces to WebIDL
Blocks: 1116654
No longer blocks: 1116665
blocking-b2g: --- → backlog
Whiteboard: [ft:ril][p=5] → [grooming][ft:ril][p=5]
blocking-b2g: backlog → ---
Depends on: 1200564
Assignee: vicamo → btseng
Blocks: 1172794
Attached patch 859764_wip.patch (obsolete) — Splinter Review
Hi Edgar & Olli,

In this bug, I am trying to turn |nsIDOMMozMobileMessageThread|, |nsIDOMMozSmsMessage|, |nsIDOMMozMmsMessage| into internal XPCOM interfaces and have new WebIDL implementation as the DOM-level interfaces of these objects.

The Goals of this change are:
1. Prevent data being clone from XPCOM implementation to WebIDL implementation.
2. Allow these objects to be shared among multiple windows.
3. To reuse the data structure defined for IPDL and prevent compiling error from the auto-generated binding.cpp. See bug 1114935 comment 47 for more detail explanation.

To achieve this, instead of having the original MobileMessageThread class to support both XPCOM/WebIDL interfaces, I am going to provide a proxy for each class which provides the implementation of the WebIDL part and each of the proxy instance reuses the data from its corresponding XPCOM object.

In addition, with a separated XXXProxy.h file, we could ensure that there won't be the same compiling problem in obj/dom/XXXBinding.cpp as mentioned in bug 1114935 comment 47.

This attached WIP patch provides an example of how the MobileMessageThread to be refactored. The same approach will be applied to both SmsMessage and MmsMessage.

May I have your earlier feedback of this new design?

Thanks!
Attachment #8671315 - Flags: feedback?(echen)
Attachment #8671315 - Flags: feedback?(bugs)
If MozMobileMessageThread can be used in several not-same-origin-windows, having a proxy per window sounds like the right option.

It might be nice to use MobileMessageThread as the name of the class which .webidl bindings interact with, and the current MobileMessageThread could be renamed to MobileMessageThreadInternal or some such.
That way the change to Bindings.conf wouldn't be needed (assuming MobileMessageThread lives in mozilla::dom namespace)

+MobileMessageThreadProxy::MobileMessageThreadProxy(nsPIDOMWindow* aWindow,
+	                                               MobileMessageThread* aThread)
Nit, align params.


Do we need to somehow cache the new MobileMessageThread proxy objects per window (or per owner) so that we don't create new ones all the time? Or is it ok to create a new object each time FireSuccessWithNextPendingResult is called? How often is that method called?
Attachment #8671315 - Flags: feedback?(bugs) → feedback+
(In reply to Olli Pettay [:smaug] from comment #16)
> If MozMobileMessageThread can be used in several not-same-origin-windows,
> having a proxy per window sounds like the right option.
> 
> It might be nice to use MobileMessageThread as the name of the class which
> .webidl bindings interact with, and the current MobileMessageThread could be
> renamed to MobileMessageThreadInternal or some such.
> That way the change to Bindings.conf wouldn't be needed (assuming
> MobileMessageThread lives in mozilla::dom namespace)
Thanks for the suggestion. I'll try to rename these properly without Bindings.conf being involved.
> 
> +MobileMessageThreadProxy::MobileMessageThreadProxy(nsPIDOMWindow* aWindow,
> +	                                               MobileMessageThread*
> aThread)
> Nit, align params.
Will do.
> 
> 
> Do we need to somehow cache the new MobileMessageThread proxy objects per
> window (or per owner) so that we don't create new ones all the time? Or is
> it ok to create a new object each time FireSuccessWithNextPendingResult is
> called? How often is that method called?
This method will be called when Gaia would like to retrieve the next result of a DB query(Thread List, or Sms/Mms Message of a selected conversation). So the frequency is related when the device user launching the app(thread list) and selecting a thread to display the message list in a conversation.

Hence, I didn't see the reason to cache the proxy objects since gaia will keep the objects to be rendered and re-query it again when the user launches the SMS App and on which conversation the user selecting.
Attachment #8671315 - Flags: feedback?(echen) → feedback+
Target Milestone: 2.0 S5 (4july) → FxOS-S12 (27Nov)
Hi Edgar, Olli,

This patch is to turn the idl object into internal interface by removing the DOMClassInfo from the implementation.

May I have your review for this change?

Thanks!
Attachment #816423 - Attachment is obsolete: true
Attachment #816424 - Attachment is obsolete: true
Attachment #816425 - Attachment is obsolete: true
Attachment #816629 - Attachment is obsolete: true
Attachment #816630 - Attachment is obsolete: true
Attachment #8671315 - Attachment is obsolete: true
Attachment #8676048 - Flags: review?(echen)
Attachment #8676048 - Flags: review?(bugs)
This patch is to provide the new web IDL definition of the MobileMessage objects.

Hi Edgar, Olli,

May I have your review for this change?

Thanks!
Attachment #8676049 - Flags: review?(echen)
Attachment #8676049 - Flags: review?(bugs)
Implementation corresponding to the WebIDL changes.
Attachment #8676050 - Flags: review?(echen)
Attachment #8676050 - Flags: review?(bugs)
Attachment #8676053 - Flags: review?(echen)
Attached patch (v1) Part 6: Changes in Payment. (obsolete) — Splinter Review
This patch provides the necessary change in PaymentProvider and MozPaymentProvider.webidl by defining Payment's own dictionary of the received SMS to allow the the app developer to use payment API without worrying the permission check of the received sms which is now restricted in patch part 2.

Hi Fernando,

May I have your review for the implementation change and the webidl change in PaymentProvider?

Thanks!
Attachment #8676055 - Flags: review?(ferjmoreno)
Attachment #8676055 - Flags: review?(bugs)
I'm not sure if I should set a dev-doc-needed flag here: does this bug has an effect for user of this API (like changes in prefix or new methods/properties) ?
Flags: needinfo?(btseng)
(In reply to Jean-Yves Perrier [:teoli] from comment #30)
> I'm not sure if I should set a dev-doc-needed flag here: does this bug has
> an effect for user of this API (like changes in prefix or new
> methods/properties) ?

No, nothing is change from the use of the API.
It's just refactored with different implementation internally. :)
Flags: needinfo?(btseng)
Comment on attachment 8676048 [details] [diff] [review]
(v1) Part 1: Turn IDL implementation into Internal-Only Interface.

>+MmsMessageInternal::GetAttachments(JSContext* aCx, JS::MutableHandle<JS::Value> aAttachments)
> {
>   uint32_t length = mAttachments.Length();
> 
>-  JS::Rooted<JSObject*> attachments(
>-    aCx, JS_NewArrayObject(aCx, length));
>-  NS_ENSURE_TRUE(attachments, NS_ERROR_OUT_OF_MEMORY);
>-
>-  for (uint32_t i = 0; i < length; ++i) {
>-    const Attachment &attachment = mAttachments[i];
>-
>-    JS::Rooted<JSObject*> attachmentObj(aCx, JS_NewPlainObject(aCx));
>-    NS_ENSURE_TRUE(attachmentObj, NS_ERROR_OUT_OF_MEMORY);
>-
>-    JS::Rooted<JSString*> tmpJsStr(aCx);
>-
>-    // Get |attachment.mId|.
>-    tmpJsStr = JS_NewUCStringCopyN(aCx,
>-                                   attachment.id.get(),
>-                                   attachment.id.Length());
>-    NS_ENSURE_TRUE(tmpJsStr, NS_ERROR_OUT_OF_MEMORY);
>-
>-    if (!JS_DefineProperty(aCx, attachmentObj, "id", tmpJsStr, JSPROP_ENUMERATE)) {
>-      return NS_ERROR_FAILURE;
>-    }
>-
>-    // Get |attachment.mLocation|.
>-    tmpJsStr = JS_NewUCStringCopyN(aCx,
>-                                   attachment.location.get(),
>-                                   attachment.location.Length());
>-    NS_ENSURE_TRUE(tmpJsStr, NS_ERROR_OUT_OF_MEMORY);
>-
>-    if (!JS_DefineProperty(aCx, attachmentObj, "location", tmpJsStr, JSPROP_ENUMERATE)) {
>-      return NS_ERROR_FAILURE;
>-    }
>-
>-    // Get |attachment.mContent|.
>-
>-    // Duplicating the File with the correct parent object.
>-    nsIGlobalObject *global = xpc::NativeGlobal(JS::CurrentGlobalOrNull(aCx));
>-    MOZ_ASSERT(global);
>-    RefPtr<Blob> newBlob = Blob::Create(global, attachment.content->Impl());
>-
>-    JS::Rooted<JS::Value> val(aCx);
>-    if (!ToJSValue(aCx, newBlob, &val)) {
>-      return NS_ERROR_FAILURE;
>-    }
>-
>-    if (!JS_DefineProperty(aCx, attachmentObj, "content", val, JSPROP_ENUMERATE)) {
>-      return NS_ERROR_FAILURE;
>-    }
>-
>-    if (!JS_DefineElement(aCx, attachments, i, attachmentObj, JSPROP_ENUMERATE)) {
>-      return NS_ERROR_FAILURE;
>-    }
>+  if (length == 0) {
>+    aAttachments.setNull();
>+    return NS_OK;
>   }
> 
>-  aAttachments.setObject(*attachments);
>+  if (!ToJSValue(aCx, mAttachments, aAttachments)) {
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  }
>+
>   return NS_OK;
> }
Could you explain why we can do this?
http://hg.mozilla.org/mozilla-central/diff/53a14a912960/dom/mobilemessage/MmsMessage.cpp#l1.70 explicitly 
made us create another instance of Blob with right parent object.
Are we perhaps not moving attachments or their blobs across DOM Windows? or only within same-domain Windows?


Other than that, looks good.
Attachment #8676049 - Flags: review?(bugs) → review+
Comment on attachment 8676050 [details] [diff] [review]
(v1) Part 3: The Implementation for WebIDL Change.

So we need MmsMessage and MmsMessageInternal because we may expose the same data on several windows and MmsMessage deals with the right parenting (mWindow).
That means the first patch needs to deal with right parenting for blobs too.

>+MmsMessage::GetAttachments(nsTArray<MmsAttachment>& aRetVal) const
>+{
>+  aRetVal = mMessage->mAttachments;
>+}


and that means we need something a bit different here.
> MobileMessageCallback::NotifySuccess(nsISupports *aMessage, bool aAsync)
> {
>+  nsCOMPtr<nsPIDOMWindow> window = mDOMRequest->GetOwner();
>+  NS_ENSURE_TRUE(window, NS_ERROR_FAILURE);
>+
>+  nsCOMPtr<nsISupports> result;
>+
>+  nsCOMPtr<nsIDOMMozSmsMessage> iSms =
>+    do_QueryInterface(aMessage);
>+  if (iSms) {
>+    SmsMessageInternal* sms = static_cast<SmsMessageInternal*>(iSms.get());
>+    result = new SmsMessage(window, sms);
>+  }
>+
>+  if (!result) {
>+    nsCOMPtr<nsIDOMMozMmsMessage> iMms =
>+      do_QueryInterface(aMessage);
>+    if (iMms) {
>+      MmsMessageInternal* mms = static_cast<MmsMessageInternal*>(iMms.get());
>+      result = new MmsMessage(window, mms);
>+    }
>+  }
perhaps iSms -> internalSms
similar also elsewhere, when dealing with iThread etc.


r- because MmsMessage::GetAttachments probably needs some tweaking (like passing mWindow to the Internal interface so that it can create proper Blobs)
Attachment #8676050 - Flags: review?(bugs) → review-
Comment on attachment 8676048 [details] [diff] [review]
(v1) Part 1: Turn IDL implementation into Internal-Only Interface.

So I think we need different handling for those blobs.
Attachment #8676048 - Flags: review?(bugs) → review-
Attachment #8676055 - Flags: review?(bugs) → review+
Comment on attachment 8676048 [details] [diff] [review]
(v1) Part 1: Turn IDL implementation into Internal-Only Interface.

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

Please see my comments below and I would like to take a look again after comment #33 is addressed.
Thank you.

::: dom/mobilemessage/MmsMessageInternal.cpp
@@ +24,5 @@
>  
>  namespace mozilla {
>  namespace dom {
>  
> +NS_IMPL_ISUPPORTS(MmsMessageInternal, nsIDOMMozMmsMessage)

Should we also rename nsIDOMMozMmsMessage since it is no longer exposed as a DOM interface?

@@ +553,3 @@
>    }
>  
> +  if (!ToJSValue(aCx, mAttachments, aAttachments)) {

Call JS_ClearPendingException() to suppress exception?
See https://bugzilla.mozilla.org/show_bug.cgi?id=1023295
Attachment #8676048 - Flags: review?(echen)
Comment on attachment 8676049 [details] [diff] [review]
(v1) Part 2: Define New WebIDL interfaces for MobileMessage Objects.

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

::: dom/webidl/MmsMessage.webidl
@@ +97,5 @@
> +
> +  /**
> +   * The flag to indicate that a read report is requested by the sender or not.
> +   */
> +  readonly attribute boolean   readReportRequested;

Nit: remove extra space between `boolean` and `readReportReqest`.

::: dom/webidl/MobileMessageThread.webidl
@@ +36,5 @@
> +
> +  /**
> +   * Timestamp of the last message in the thread.
> +   */
> +  readonly attribute DOMTimeStamp       timestamp;

Ditto.

@@ +41,5 @@
> +
> +  /**
> +   * Message type of the last message in the thread.
> +   */
> +  readonly attribute DOMString          lastMessageType;

Ditto.

::: dom/webidl/SmsMessage.webidl
@@ +84,5 @@
> +
> +  /**
> +   * The read status of this message.
> +   */
> +  readonly attribute boolean   read;

Ditto.
Attachment #8676049 - Flags: review?(echen) → review+
(In reply to Olli Pettay [:smaug] from comment #32)
> Could you explain why we can do this?
> http://hg.mozilla.org/mozilla-central/diff/53a14a912960/dom/mobilemessage/
> MmsMessage.cpp#l1.70 explicitly 
> made us create another instance of Blob with right parent object.
> Are we perhaps not moving attachments or their blobs across DOM Windows? or
> only within same-domain Windows?

Thanks for catching this problem.
My original purpose is to get rid of JS API as much as possible.
However, I wasn't aware of the parent object which is required inside the Blob.
I'll rollback the change in this part and have new copy of Blob in MmsMessage::GetAttachments in patch part 3 by using Blob::Create(nsISupports* aParent, BlobImpl* aImpl) instead to provide the parent object accordingly.
Comment on attachment 8676050 [details] [diff] [review]
(v1) Part 3: The Implementation for WebIDL Change.

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

::: dom/mobilemessage/MmsMessage.cpp
@@ +126,5 @@
> +
> +void
> +MmsMessage::GetAttachments(nsTArray<MmsAttachment>& aRetVal) const
> +{
> +  aRetVal = mMessage->mAttachments;

As mentioned in comment #33, We need different handling for the blobs.
Attachment #8676050 - Flags: review?(echen)
Comment on attachment 8676051 [details] [diff] [review]
(v1) Part 4: Implementation Change in Different Backend.

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

::: dom/mobilemessage/gonk/MmsService.js
@@ +1824,5 @@
>                                         DELIVERY_STATUS_ERROR,
>                                         null,
> +                                       (rv, domMessage) => {
> +        let mmsMessage = domMessage ?
> +                       domMessage.QueryInterface(Ci.nsIDOMMozMmsMessage) : null;

QueryInterface throws exception if the interface is not available [1].
Have a instanceof check first or use try-catch, here and elsewhere.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsISupports#QueryInterface%28%29
Attachment #8676051 - Flags: review?(echen)
Comment on attachment 8676053 [details] [diff] [review]
(v1) Part 5: Changes in Test Cases.

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

::: dom/mobilemessage/tests/marionette/test_incoming.js
@@ +11,5 @@
>  const LONG_BODY = new Array(17).join(SHORT_BODY);
>  ok(LONG_BODY.length > 160, "LONG_BODY.length");
>  
>  function checkMessage(aMessage, aBody) {
> +  ok(aMessage instanceof SmsMessage, "Message is instanceof MozSmsMessage");

s/MozSmsMessage/SmsMessage/
Attachment #8676053 - Flags: review?(echen) → review+
Comment on attachment 8676055 [details] [diff] [review]
(v1) Part 6: Changes in Payment.

cancel the review due to the same reason in comment 39.
Attachment #8676055 - Flags: review?(ferjmoreno)
1. Move namespace of these internal interfaces from mozilla::dom to mozilla::dom::mobilemessage.
2. remove the prefix of DOMMoz from these idl definition.
3. Rewrite MmsMessageInternal::GetAttachements() with the dictionary to reduce the use of JS API.
4. Set parent object of the Blob in consistent way according to current JS context even they are accessed internally. (The Web IDL version implementation also use Blob::Create(parent, BlobImpl) to set proper parent window in MmsMessage::GetAttachments())
Attachment #8676048 - Attachment is obsolete: true
Attachment #8677277 - Flags: review?(echen)
Attachment #8677277 - Flags: review?(bugs)
Attachment #8677279 - Flags: review?(echen)
Attach proper window for each Blob in MmsMessage::GetAttachments() before exposing the attachments to the caller.
Attachment #8676050 - Attachment is obsolete: true
Attachment #8677282 - Flags: review?(echen)
Attachment #8677282 - Flags: review?(bugs)
call QueryInteface with try/catch protected.
Attachment #8676051 - Attachment is obsolete: true
Attachment #8677284 - Flags: review?(echen)
Attachment #8676053 - Attachment is obsolete: true
Attachment #8677285 - Flags: review+
Attached patch (v2) Part 6: Changes in Payment. (obsolete) — Splinter Review
Hi Fernando,

The webidl change has been approved by :smaug.
May I have your review for the implementation change in PaymentProvider?

Thanks!
Attachment #8676055 - Attachment is obsolete: true
Attachment #8677288 - Flags: review?(ferjmoreno)
Attachment #8677288 - Flags: review?(ferjmoreno) → review+
Comment on attachment 8677277 [details] [diff] [review]
(v2) Part 1.1: Turn IDL Implementation into Internal-Only Interface.

>+  // Duplicating the Blob with the correct parent object.
>+  nsIGlobalObject *global = xpc::NativeGlobal(JS::CurrentGlobalOrNull(aCx));
Nit, * goes with the type, not with variable name, so
nsIGlobalObject* global


>+  MOZ_ASSERT(global);
>+  nsTArray<MmsAttachment> result;
>+  for (uint32_t i = 0; i < length; i++) {
>+    MmsAttachment attachment;
>+    const MmsAttachment &element = mAttachments[i];
>+    attachment.mId = element.mId;
>+    attachment.mLocation = element.mLocation;
>+    attachment.mContent = Blob::Create(global, element.mContent->Impl());
Don't you need to null check element.mContent.
MmsAttachment.content is nullable (so are the string properties, but ns*String deals with null internally)

with those, r+
Attachment #8677277 - Flags: review?(bugs) → review+
Comment on attachment 8677282 [details] [diff] [review]
(v2) Part 3: The Implementation for WebIDL Change.

>+MmsMessage::GetAttachments(nsTArray<MmsAttachment>& aRetVal) const
>+{
>+  uint32_t length = mMessage->mAttachments.Length();
>+
>+  // Duplicating the Blob with the correct parent object.
>+  for (uint32_t i = 0; i < length; i++) {
>+    MmsAttachment attachment;
>+    const MmsAttachment &element = mMessage->mAttachments[i];
>+    attachment.mId = element.mId;
>+    attachment.mLocation = element.mLocation;
>+    attachment.mContent = Blob::Create(mWindow, element.mContent->Impl());
Assign anything to attachment.mContent only if element.mContent is non-null, so if (element.mContent) { ... }

>+    aRetVal.AppendElement(attachment);
>+  }
>+}
Now I'm a bit lost. Do we really need attachment duplication in two places.
or in other words, I guess we could remove MmsMessageInternal::GetAttachments (and from .idl) now that we have 
MmsMessage::GetAttachments.
Attachment #8677282 - Flags: review?(bugs) → review+
Not necessarily about this bug, but I realized MmsMessageInternal needs to be cycle collected. It keeps some blobs alive and those are cycle collectable.
Another option is that MmsMessageInternal wouldn't keep Blobs alive via 
nsTArray<MmsAttachment> mAttachments; but would have an array of different kinds of attachments where
BlobImpl is kept alive
(In reply to Olli Pettay [:smaug] from comment #50)
> Comment on attachment 8677277 [details] [diff] [review]
> (v2) Part 1.1: Turn IDL Implementation into Internal-Only Interface.
> 
> >+  // Duplicating the Blob with the correct parent object.
> >+  nsIGlobalObject *global = xpc::NativeGlobal(JS::CurrentGlobalOrNull(aCx));
> Nit, * goes with the type, not with variable name, so
> nsIGlobalObject* global
Will do.
> >+  MOZ_ASSERT(global);
> >+  nsTArray<MmsAttachment> result;
> >+  for (uint32_t i = 0; i < length; i++) {
> >+    MmsAttachment attachment;
> >+    const MmsAttachment &element = mAttachments[i];
> >+    attachment.mId = element.mId;
> >+    attachment.mLocation = element.mLocation;
> >+    attachment.mContent = Blob::Create(global, element.mContent->Impl());
> Don't you need to null check element.mContent.
> MmsAttachment.content is nullable (so are the string properties, but
> ns*String deals with null internally)
The mContent shall never be null for all the use case.
I'll add ASSERTION on this instead.
> 
> with those, r+
Thanks!
(In reply to Olli Pettay [:smaug] from comment #51)
> Comment on attachment 8677282 [details] [diff] [review]
> (v2) Part 3: The Implementation for WebIDL Change.
> 
> >+MmsMessage::GetAttachments(nsTArray<MmsAttachment>& aRetVal) const
> >+{
> >+  uint32_t length = mMessage->mAttachments.Length();
> >+
> >+  // Duplicating the Blob with the correct parent object.
> >+  for (uint32_t i = 0; i < length; i++) {
> >+    MmsAttachment attachment;
> >+    const MmsAttachment &element = mMessage->mAttachments[i];
> >+    attachment.mId = element.mId;
> >+    attachment.mLocation = element.mLocation;
> >+    attachment.mContent = Blob::Create(mWindow, element.mContent->Impl());
The mContent shall never be null for all the use case.
I'll add ASSERTION on this instead.
> Assign anything to attachment.mContent only if element.mContent is non-null,
> so if (element.mContent) { ... }
> 
> >+    aRetVal.AppendElement(attachment);
> >+  }
> >+}
> Now I'm a bit lost. Do we really need attachment duplication in two places.
> or in other words, I guess we could remove
> MmsMessageInternal::GetAttachments (and from .idl) now that we have 
> MmsMessage::GetAttachments.
Oh, we still need this because the internal one will still be used internally in MmsService.js.
(In reply to Olli Pettay [:smaug] from comment #52)
> Not necessarily about this bug, but I realized MmsMessageInternal needs to
> be cycle collected. It keeps some blobs alive and those are cycle
> collectable.

Thanks for capturing this problem.
I'll either update the patch or file another bug to address this. :)
fix alignment in nsIMobileMessageService.idl
Attachment #8677279 - Attachment is obsolete: true
Attachment #8677279 - Flags: review?(echen)
Attachment #8677935 - Flags: review?(echen)
Attachment #8677277 - Flags: review?(echen) → review+
Attachment #8677935 - Flags: review?(echen) → review+
Comment on attachment 8677282 [details] [diff] [review]
(v2) Part 3: The Implementation for WebIDL Change.

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

::: dom/mobilemessage/DOMMobileMessageError.cpp
@@ +11,2 @@
>  
> +using namespace mozilla::dom::mobilemessage;

It seems we don't need this.

::: dom/mobilemessage/MmsMessage.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_dom_mobilemessage_MmsMessage_h

s/mozilla_dom_mobilemessage_MmsMessage_h/mozilla_dom_MmsMessage_h/

::: dom/mobilemessage/MobileMessageThread.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_dom_mobilemessage_MobileMessageThread_h

s/mozilla_dom_mobilemessage_MobileMessageThread_h/mozilla_dom_MobileMessageThread_h/

::: dom/mobilemessage/SmsMessage.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_dom_mobilemessage_SmsMessage_h

s/mozilla_dom_mobilemessage_SmsMessage_h/mozilla_dom_SmsMessage_h
Attachment #8677282 - Flags: review?(echen) → review+
Comment on attachment 8677284 [details] [diff] [review]
(v2) Part 4: Implementation Change in Different Backend.

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

Looks good to me, thank you.
Attachment #8677284 - Flags: review?(echen) → review+
Attachment #8677281 - Attachment is obsolete: true
Attachment #8678655 - Flags: review+
address comment 51 and comment 58.
Attachment #8677282 - Attachment is obsolete: true
Attachment #8678657 - Flags: review+
Attachment #8677284 - Attachment is obsolete: true
Attachment #8678658 - Flags: review+
Attachment #8677288 - Attachment description: 859764_part6_v2.patch → (v2) Part 6: Changes in Payment.
Attachment #8677285 - Attachment is obsolete: true
Attachment #8678659 - Flags: review+
Attachment #8677288 - Attachment is obsolete: true
Attachment #8678660 - Flags: review+
See try server result in comment 60.
Keywords: checkin-needed
Hi, could you take a look at part 1.2:


remote: Push rejected because the following IDL interfaces were
remote: modified without changing the UUID:
remote:   - nsIMobileMessageService 

remote: To update the UUID for all of the above interfaces and their
remote: descendants, run:
remote:   ./mach update-uuids nsIMobileMessageService
remote: 
remote: If you intentionally want to keep the current UUID, include
remote: 'IGNORE IDL' in the commit message.
Flags: needinfo?(btseng)
Keywords: checkin-needed
Bevis, BTW, I'd advise to wait for after 2.5 branching to land this big thing... I'm afraid of regressions.
(In reply to Carsten Book [:Tomcat] from comment #69)
> Hi, could you take a look at part 1.2:
> 
> 
> remote: Push rejected because the following IDL interfaces were
> remote: modified without changing the UUID:
> remote:   - nsIMobileMessageService 
> 
> remote: To update the UUID for all of the above interfaces and their
> remote: descendants, run:
> remote:   ./mach update-uuids nsIMobileMessageService
> remote: 
> remote: If you intentionally want to keep the current UUID, include
> remote: 'IGNORE IDL' in the commit message.

My bad. I'll fix this and ask for checkin again after 2.5 is branched due to the concern in comment 70.

(In reply to Julien Wajsberg [:julienw] from comment #70)
> Bevis, BTW, I'd advise to wait for after 2.5 branch is created to land this big
> thing... I'm afraid of regressions.

No problem!
Flags: needinfo?(btseng)
Attachment #8678659 - Attachment is obsolete: true
Attachment #8686409 - Flags: review+
Attachment #8678660 - Attachment is obsolete: true
Attachment #8686410 - Flags: review+
patches are all re-based and comment 69 was addressed in last update.
treehelder result is attached in comment 73.
Keywords: checkin-needed
Hey Isabel, so that you know this bug, as this changes all MobileMessage interfaces (that we use to receive, display and send messages), we can have issues because of this.
Flags: needinfo?(irios)
Thank you Julien, will have it in mind.
Flags: needinfo?(irios)
You need to log in before you can comment on or make changes to this bug.