859764 - WebSMS: move all MobileMessage interfaces to WebIDL

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Comment 1

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

Amending this bug to convert all DOM types instead.

Comment 2

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

Move only mozMobileMessage first to close bug 888591 as soon as possible.

Comment 3

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

Attachment: part 1.a/N - Add nsIDOMMozMmsAttachment

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.

Comment 4

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

Attachment: part 1.b/N: don't align parameter names

Comment 5

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

Attachment: part 1.c/N: nsIDOMMozMmsAttachment DOM implementation

Comment 6

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

Attachment: part 2.a/N: Move all MobileMessage interfaces to WebIDL. Interface changes. WIP

Comment 7

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

Attachment: part 2.b/N: implement : WIP

Comment 9

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

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?

Comment 10

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

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.

Comment 12

[Anshul]

Vicamo, are you targeting this for 2.0?

Comment 13

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

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.

Comment 14

[Bevis Tseng[:bevis][:btseng](Exited)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b017394166db

Comment 15

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: 859764_wip.patch

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!

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 17

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 18

[Bevis Tseng[:bevis][:btseng](Exited)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c56322322c0

Comment 19

[Bevis Tseng[:bevis][:btseng](Exited)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7128adb93e6e

Comment 20

[Bevis Tseng[:bevis][:btseng](Exited)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2136cdf822c0

Comment 21

[Bevis Tseng[:bevis][:btseng](Exited)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8df2933089c4

Comment 22

[Bevis Tseng[:bevis][:btseng](Exited)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b6c497e6c7a

Comment 23

[Bevis Tseng[:bevis][:btseng](Exited)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1125b237cae

Comment 24

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v1) Part 1: Turn IDL implementation into Internal-Only Interface.

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!

Comment 25

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v1) Part 2: Define New WebIDL interfaces for MobileMessage Objects.

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!

Comment 26

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v1) Part 3: The Implementation for WebIDL Change.

Implementation corresponding to the WebIDL changes.

Comment 27

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v1) Part 4: Implementation Change in Different Backend.

Comment 28

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v1) Part 5: Changes in Test Cases.

Comment 29

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v1) Part 6: Changes in Payment.

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!

Comment 30

[Jean-Yves Perrier [:teoli]]

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

Comment 31

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

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)

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 35

[Edgar Chen [:edgar]]

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

Comment 36

[Edgar Chen [:edgar]]

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.

Comment 37

[Bevis Tseng[:bevis][:btseng](Exited)]

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

[Edgar Chen [:edgar]]

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.

Comment 39

[Edgar Chen [:edgar]]

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

Comment 40

[Edgar Chen [:edgar]]

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/

Comment 41

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8676055 [details] [diff] [review]
(v1) Part 6: Changes in Payment.

cancel the review due to the same reason in comment 39.

Comment 42

[Bevis Tseng[:bevis][:btseng](Exited)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3dc32030d60

Comment 43

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v2) Part 1.1: Turn IDL Implementation into Internal-Only Interface.

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

Comment 44

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v2) Part 1.2: Clearn Up Naming in IDL.

Comment 45

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v2) Part 2: Define New WebIDL interfaces for MobileMessage Objects. r=echen,bugs

Comment 46

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v2) Part 3: The Implementation for WebIDL Change.

Attach proper window for each Blob in MmsMessage::GetAttachments() before exposing the attachments to the caller.

Comment 47

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v2) Part 4: Implementation Change in Different Backend.

call QueryInteface with try/catch protected.

Comment 48

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v2) Part 5: Changes in Test Cases.

Comment 49

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v2) Part 6: Changes in Payment.

Hi Fernando,

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

Thanks!

Comment 50

[Olli Pettay [:smaug][[e-mail]]]

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+

Comment 51

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 52

[Olli Pettay [:smaug][[e-mail]]]

Not necessarily about this bug, but I realized MmsMessageInternal needs to be cycle collected. It keeps some blobs alive and those are cycle collectable.

Comment 53

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 54

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 55

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 56

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 57

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v2) Part 1.2: Clearn Up Naming in IDL.

fix alignment in nsIMobileMessageService.idl

Comment 58

[Edgar Chen [:edgar]]

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

Comment 59

[Edgar Chen [:edgar]]

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.

Comment 60

[Bevis Tseng[:bevis][:btseng](Exited)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7165fc43fc7

Comment 61

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1.1: Turn IDL Implementation into Internal-Only Interface. r=echen,bugs

Comment 62

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1.2: Clearn Up Naming in IDL. r=echen

Comment 63

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 2: Define New WebIDL interfaces for MobileMessage Objects. r=echen,bugs

Comment 64

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 3: The Implementation for WebIDL Change. r=echen,bugs

address comment 51 and comment 58.

Comment 65

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 4: Implementation Change in Different Backend. r=echen

Comment 66

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 5: Changes in Test Cases. r=echen

Comment 67

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6: Changes in Payment. r=ferjm,bugs

Comment 68

[Bevis Tseng[:bevis][:btseng](Exited)]

See try server result in comment 60.

Comment 69

[Carsten Book [:Tomcat]]

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.

Comment 70

[Julien Wajsberg [:julienw]]

Bevis, BTW, I'd advise to wait for after 2.5 branching to land this big thing... I'm afraid of regressions.

Comment 71

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 72

[Bevis Tseng[:bevis][:btseng](Exited)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0713157b854a

Comment 73

[Bevis Tseng[:bevis][:btseng](Exited)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=06b13ba49010

Comment 74

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1.1: Turn IDL Implementation into Internal-Only Interface. r=echen,bugs

Comment 75

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1.2: Clearn Up Naming in IDL. r=echen

Comment 76

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 2: Define New WebIDL interfaces for MobileMessage Objects. r=echen,bugs

Comment 77

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 3: The Implementation for WebIDL Change. r=echen,bugs

Comment 78

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 4: Implementation Change in Different Backend. r=echen

Comment 79

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 5: Changes in Test Cases. r=echen

Comment 80

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 6: Changes in Payment. r=ferjm,bugs

Comment 81

[Bevis Tseng[:bevis][:btseng](Exited)]

patches are all re-based and comment 69 was addressed in last update.
treehelder result is attached in comment 73.

Comment 82

[Julien Wajsberg [:julienw]]

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.

Comment 83

[Isabel Rios[:isabel_rios]]

Thank you Julien, will have it in mind.

Comment 84

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/7645ba2fe424
https://hg.mozilla.org/integration/b2g-inbound/rev/3435cac95ebf
https://hg.mozilla.org/integration/b2g-inbound/rev/e55be126889b
https://hg.mozilla.org/integration/b2g-inbound/rev/ad9c6df171c6
https://hg.mozilla.org/integration/b2g-inbound/rev/44e4de554dfc
https://hg.mozilla.org/integration/b2g-inbound/rev/f2fd07212711
https://hg.mozilla.org/integration/b2g-inbound/rev/5f4725e5f83b

Comment 85

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

https://hg.mozilla.org/mozilla-central/rev/7645ba2fe424
https://hg.mozilla.org/mozilla-central/rev/3435cac95ebf
https://hg.mozilla.org/mozilla-central/rev/e55be126889b
https://hg.mozilla.org/mozilla-central/rev/ad9c6df171c6
https://hg.mozilla.org/mozilla-central/rev/44e4de554dfc
https://hg.mozilla.org/mozilla-central/rev/f2fd07212711
https://hg.mozilla.org/mozilla-central/rev/5f4725e5f83b