867227 - B2G MMS: Add expiry date into nsIDOMMozMmsMessage.

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Comment 1

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Part 1/2: Interface v1.0

Comment 2

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Part 2/2: Patch v1.0

Comment 3

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Part 1/2: Interface v1.1

Comment 4

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

Comment on attachment 743914 [details] [diff] [review]
Part 2/2: Patch v1.0

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

::: dom/mobilemessage/src/MmsMessage.cpp
@@ +523,5 @@
> +  NS_ENSURE_TRUE(obj, NS_ERROR_FAILURE);
> +
> +  *aDate = OBJECT_TO_JSVAL(obj);
> +  return NS_OK;
> +}

nit: add an empty line after.

Comment 5

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Part 2/2: Patch v1.1

Add new line.

Comment 6

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Part 2/2: Patch v1.2

Comment 7

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

This one is correct one. Part 2/2: Patch v1.1 is wrong.
(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #6)
> Created attachment 744158 [details] [diff] [review]
> Part 2/2: Patch v1.2

Comment 8

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Nominating for leo+ because of blocking 862262 which is nominated for leo+.

Comment 9

[Alhad Purnapatre]

Removed NO_UPLIFT, as no change required in commercial RIL. Thanks.

Comment 10

[Mounir Lamouri (:mounir)]

Why did we decide to implement this? I can't understand the reasons by reading bug 862262.

Comment 11

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Because the notification message shows the expiry date when the retrieval mode is manual in UX wireframe designed by Ayman(Telfonica guy responsible for Firefox OS MMS UX). We need this patch to meet this requirement.

Comment 12

[Dietrich Ayala (:dietrich)]

(In reply to Mounir Lamouri (:mounir) from comment #10)
> Why did we decide to implement this? I can't understand the reasons by
> reading bug 862262.

Messages on the server have an expiration date. For cost-conscious markets and customers, it appears we ship with auto-retrieve off, which means we need to indicate to the user how long they have to be able to download a given MMS message.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/f2cc1aab0edf
https://hg.mozilla.org/projects/birch/rev/3e0f1e2b853b

Should this have a test?

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f2cc1aab0edf
https://hg.mozilla.org/mozilla-central/rev/3e0f1e2b853b

Comment 15

[Alex Keybl [:akeybl]]

We didn't see in bug 862262 whether or not this was a blocker. Peter, can you help us to understand?

Don't let the lack of blocking status prevent you from requesting approval-b2g18 on the patch though (this will also get the fix into v1.1).

Comment 16

[Dietrich Ayala (:dietrich)]

bug 862262 has the comments from maria about why this is blocking+ (this blocks the UI impl).

Comment 17

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

WIP b2g18 uplift branch - https://github.com/vicamo/b2g_mozilla-central/tree/bugzilla/867227/mms-message-expiryDate

Comment 18

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

https://hg.mozilla.org/releases/mozilla-b2g18/rev/19435cc447c9
https://hg.mozilla.org/releases/mozilla-b2g18/rev/57c0cda90f17

Comment 19

[Gene Lian (I already quit Mozilla)]

Comment on attachment 744158 [details] [diff] [review]
Part 2/2: Patch v1.2

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

::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
@@ +669,5 @@
>              "content": partContent
>            });
>          }
>        }
> +      let expiryDate = aMessageRecord.timestamp + headers["x-mms-expiry"] * 1000;

Please see below.

@@ +681,5 @@
>                                                      aMessageRecord.read,
>                                                      subject,
>                                                      smil,
> +                                                    attachments,
> +                                                    expiryDate);

This is wrong. .createDomMessageFromRecord() is also used for the sending MMS which doesn't need to have an "x-mms-expiry" in its header. MmsMessage::Create() will reject an invalid |aExpiryDate|. This stops us sending an MMS.

E/GeckoConsole(  551): [JavaScript Error: "NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIMobileMessageService.createMmsMessage]" {file: "jar:file:///system/b2g/omni.ja!/components/MobileMessageDatabaseService.js" line: 732}]

I'm working on a patch for this. Will fire a bug later.

Comment 20

[Rick Waldron [:rwaldron]]

@Gene, I've been updating the SMS app to support using threadId instead of "phone number" for multi-recipient support. So far, I have not been able to successfully send a message with sendMMS—is that currently expected behaviour?

Comment 21

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Rick, Bug 869807 - B2G MMS: fail to send MMS due to the wrong assignment for expiryDate should already fixed this problem.

Comment 22

[Gene Lian (I already quit Mozilla)]

(In reply to Rick Waldron from comment #20)
> @Gene, I've been updating the SMS app to support using threadId instead of
> "phone number" for multi-recipient support. So far, I have not been able to
> successfully send a message with sendMMS—is that currently expected
> behaviour?

Hi Rick, as Chia-hung's mention, please make sure both Bug 869841 and Bug 869807 has been included in your Gecko build, which should be able to solve the sendMMS(...) error.

Comment 23

[Rick Waldron [:rwaldron]]

Not sure which bug this makes sense for: https://bugzilla.mozilla.org/show_bug.cgi?id=869841#c7

Comment 24

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Please see https://bugzilla.mozilla.org/show_bug.cgi?id=869841#c8. It is a typo in Messaging AP.