Closed
Bug 850529
Opened 12 years ago
Closed 11 years ago
B2G MMS: MmsMessage::GetAttachments() should return an empty array when it has no attachments
Categories
(Core :: DOM: Device Interfaces, enhancement, P5)
Tracking
()
RESOLVED
FIXED
mozilla25
People
(Reporter: airpingu, Assigned: vicamo)
References
Details
(Whiteboard: [fixed-in-birch])
Attachments
(1 file)
(deleted),
patch
|
airpingu
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•12 years ago
|
||
(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
Reporter | ||
Comment 2•12 years ago
|
||
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|?
Flags: needinfo?(mounir)
Flags: needinfo?(jonas)
Reporter | ||
Updated•12 years ago
|
Comment 3•12 years ago
|
||
I would prefer an empty array. However, this is not very urgent. You guys could also fix that later when there is less pressure.
Flags: needinfo?(mounir)
Reporter | ||
Updated•12 years ago
|
Priority: -- → P5
Reporter | ||
Updated•12 years ago
|
Severity: normal → enhancement
Assignee | ||
Comment 5•11 years ago
|
||
Assignee: nobody → vyang
Attachment #771567 -
Flags: review?(gene.lian)
Reporter | ||
Comment 6•11 years ago
|
||
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. :)
Attachment #771567 -
Flags: review?(gene.lian) → review+
Reporter | ||
Updated•11 years ago
|
Flags: in-testsuite+
Assignee | ||
Comment 7•11 years ago
|
||
(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.
Reporter | ||
Comment 8•11 years ago
|
||
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?
Assignee | ||
Comment 9•11 years ago
|
||
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.
Assignee | ||
Comment 10•11 years ago
|
||
https://hg.mozilla.org/projects/birch/rev/8106727efe7a
Whiteboard: [fixed-in-birch]
Comment 11•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8106727efe7a
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in
before you can comment on or make changes to this bug.
Description
•