850529 - B2G MMS: MmsMessage::GetAttachments() should return an empty array when it has no attachments

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, enhancement, P5)

Description

[Gene Lian (I already quit Mozilla)]

Please see https://bugzilla.mozilla.org/show_bug.cgi?id=844431#c97 for Mounir's comment as below:

I think it could be better to return an empty array instead but I am not sure... Could you file a follow-up and CC jonas and I.

Comment 1

[Gene Lian (I already quit Mozilla)]

(In reply to Gene Lian [:gene] from comment #0)
> Please see https://bugzilla.mozilla.org/show_bug.cgi?id=844431#c97 for

Sorry. Should be https://bugzilla.mozilla.org/show_bug.cgi?id=844431#c96

Comment 2

[Gene Lian (I already quit Mozilla)]

Hi Jonas and Mounir,

This is an issue raised by Mounir when implementing MMS DOM structure at bug 844431. I know this is not an extremely major issue but we still need your final decision to resolve this bug. Let's review the current MMS structure as below:

  dictionary MmsAttachment
  {
    DOMString? id;
    DOMString? location;
    nsIDOMBlob content;
  };

  interface nsIDOMMozMmsMessage : nsISupports
  {
    // skip...

    [implicit_jscontext]
    readonly attribute jsval attachments; // MmsAttachment[]
  };


The question is: when an MMS has no attachments, we should return a null or an empty array for the |.attachments|?

Comment 3

[Mounir Lamouri (:mounir)]

I would prefer an empty array. However, this is not very urgent. You guys could also fix that later when there is less pressure.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I agree with comment 3.

Comment 5

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

Attachment: patch

Comment 6

[Gene Lian (I already quit Mozilla)]

Comment on attachment 771567 [details] [diff] [review]
patch

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

Maybe add an .onsent event handler to ensure we shouldn't succeed to send anyway? The current patch looks fine to me. It's up to you. :)

Comment 7

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

(In reply to Gene Lian [:gene] from comment #6)
> Maybe add an .onsent event handler to ensure we shouldn't succeed to send
> anyway? The current patch looks fine to me. It's up to you. :)

We only have either a onsent or a onfailed event.  Never have both.  |tasks.next()| is only called in onfailed event handler, so if somehow we get a onsent event instead, the test case will fail with a timeout error.

Comment 8

[Gene Lian (I already quit Mozilla)]

Yes, I know. My point is we don't need to rely on the timeout. We can use .onsent to explicitly return error earlier.

Btw, it comes to my head that maybe using .onsending is a better alternative. In this way, we don't need to play the .onfailed trick. ;)

What do you think?

Comment 9

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

I've tried onsending before submitting for review, but onfailed is better for me, so that we don't have further impact on succeeding test cases.

Comment 10

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

https://hg.mozilla.org/projects/birch/rev/8106727efe7a

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8106727efe7a