927711 - B2G MMS : Handle message delivered timestamp for delivery report

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Gene Lian (I already quit Mozilla)]

+++ This bug was initially created as a clone of Bug #887159 +++

Just like SMS, Gecko needs to expose delivered timestamp for MMS delivery report.

Comment 1

[Joe Cheng [:jcheng] (please needinfo)]

blocking a committed 1.3 user story. 1.3+

Comment 2

[Ken Chang[:kenkai]]

Set milestone. If it isn't reasonable for you, please directly change it.

Comment 3

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, DOM API IDL, V1

Hi Jonas,

Could you please take the superreview? This exactly follows our agreement with W3C at [1]. Should be for sure to go. Note that this patch depends on Bug 928821 which is under way landing.

[1] http://messaging.sysapps.org/#idl-def-MmsDeliveryInfo

Comment 4

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, implementation, V1

Comment 5

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

Comment on attachment 825229 [details] [diff] [review]
Part 1, DOM API IDL, V1

I'll defer to Hsin-Yi. But this looks good to me.

Comment 6

[Gene Lian (I already quit Mozilla)]

Comment on attachment 825229 [details] [diff] [review]
Part 1, DOM API IDL, V1

As we just talked, we should use review? to review APIs instead of using superreview?. :)

Comment 7

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

Comment on attachment 825230 [details] [diff] [review]
Part 2, implementation, V1

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

Good job. Only a small defect.

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +1412,5 @@
> +      deliveryInfo:      aDomMessage.deliveryInfo,
> +      sender:            aDomMessage.sender,
> +      receivers:         aDomMessage.receivers,
> +      timestamp:         aDomMessage.timestamp,
> +      deliveryTimestamp: aDomMessage.deliveryTimestamp,

We don't need this line. Because this information should be contained in deliveryInfo.

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +1371,1 @@
>                  if (!match) {

We might need to use |matchPhoneNumbers| to match number. Or we should open a new bug for it.

Comment 8

[Gene Lian (I already quit Mozilla)]

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #7)
> We might need to use |matchPhoneNumbers| to match number. Or we should open
> a new bug for it.

Thanks for the review! I'd prefer opening a new bug for that. Supposing we would want to back out this patch due to the wrong |matchPhoneNumbers|, then we don't need to back out this one which addresses separate issues.


Will come up with the new patch later. Thanks!

Comment 9

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, implementation, V2

Comment 10

[Gene Lian (I already quit Mozilla)]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #8)
> (In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #7)
> > We might need to use |matchPhoneNumbers| to match number. Or we should open
> > a new bug for it.
> 
> Thanks for the review! I'd prefer opening a new bug for that. Supposing we
> would want to back out this patch due to the wrong |matchPhoneNumbers|, then
> we don't need to back out this one which addresses separate issues.

Open Bug 934931.

Comment 11

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

Comment on attachment 825229 [details] [diff] [review]
Part 1, DOM API IDL, V1

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

Looks good to me.

Comment 12

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, DOM API IDL, V1.1

r=hsinyi

Comment 13

[Gene Lian (I already quit Mozilla)]

remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/f27a2aba0db2
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/6cb70cc44e8c

Comment 14

[Carsten Book [:Tomcat]]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #13)
> remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/f27a2aba0db2
> remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/6cb70cc44e8c

Hi Gene, sorry i had to backout this changesets since it caused build failures/bustages like https://tbpl.mozilla.org/php/getParsedLog.php?id=30132724&tree=B2g-Inbound

Comment 15

[Gene Lian (I already quit Mozilla)]

So weird... I can pass my local build. Let's see try again: https://tbpl.mozilla.org/?tree=Try&rev=db00505e61cd

Comment 16

[Gene Lian (I already quit Mozilla)]

The previous try is the wrong patch.

New try: https://tbpl.mozilla.org/?tree=Try&rev=f92b8c1f5be2

The result looks good (I didn't change anything). I don't why it would cause build failures. Maybe something wrong with the CLOBBER. Let's re-land this again.

https://hg.mozilla.org/integration/b2g-inbound/rev/85ba50130edc
https://hg.mozilla.org/integration/b2g-inbound/rev/9ac5edd968c0

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/85ba50130edc
https://hg.mozilla.org/mozilla-central/rev/9ac5edd968c0