933131 - [Gaia][Messages] Message report panel implementation

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Steve Chung [:steveck]]

Baseed on message app WF spec(https://bug919977.bugzilla.mozilla.org/attachment.cgi?id=822328) p.18, we need to implement a new message report panel for showing the receiver and delivery report information.

Comment 1

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

See also bug 901453 and its dependencies. You might need to do some cleanup there.

Comment 2

[Steve Chung [:steveck]]

Thanks for the reminder :)

Comment 3

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Maybe you should dupe bug 901453 here ?

Comment 4

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

1.3+ for a committed 1.3 user story

Comment 6

[Steve Chung [:steveck]]

WIP branch :https://github.com/steveck-chung/gaia/commit/df97fb2784ef5fb4446bf4ff03e08312ccc6aed0
    patch  :https://github.com/steveck-chung/gaia/commit/df97fb2784ef5fb4446bf4ff03e08312ccc6aed0.patch

The functionality already works for most of cases, need visual support(I might create another bug for this) and further clean up. Some known issue need to be fixed later:
 - Hide attachment icon
 - sent timestamp (need ux input)
 - attachment total size
 - unit test and desktop mock test
 - ...

Hi Julien, would you mind to take a quick look first? It would be great to discover any possible problem in this implementation.

Comment 7

[Steve Chung [:steveck]]

Attachment: Link to github

Hi Julien, it's only a WIP since the visual assets and test case is not ready yet, but it would be great if you could provide some feedback before further implementation. The functionality is almost ready and it should be able to run on the device/desktop. The missing parts are:
 - Unit Test
 - New message status icon(Might create another bug for it)
 - Report update ability when status changed

Comment 8

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Comment on attachment 831387 [details] [review]
Link to github

I think it makes sense to base your work on top of bug 934531 work. I'll produce a first WIP patch today that you can cherry-pick (and give me feedback too! :) ).

Comment 9

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Hey Steve, see https://github.com/mozilla-b2g/gaia/pull/13713 for picking the commit.
Tests are not passing yet though :( Will work on this first priority tomorrow so that you have a safe ground.

Comment 10

[Steve Chung [:steveck]]

Comment on attachment 831387 [details] [review]
Link to github

Hi Julien, I've updated the patch based on your contact renderer and modulize  infomration view for group/report view. The test case is still not ready yet but maybe you could provide some feedback to this new module and the changes in contact renderer, thanks!

Comment 11

[Gene Lian (I already quit Mozilla)]

After Bug 939302 lands, Gaia needs to have corresponding changes to reinterpret deliveryTimestamp as long-long number, instead of Date object.

Comment 12

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Gene, can you add me and Steve on bug 939302?

Comment 13

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Comment on attachment 831387 [details] [review]
Link to github

I've added some comments on the PR, enough to keep you working.

Comment 14

[Gene Lian (I already quit Mozilla)]

I'd like to unblock bug 901457 which is not in the scope of this user story.

Comment 15

[Steve Chung [:steveck]]

Comment on attachment 831387 [details] [review]
Link to github

Hi Julien, thanks for the comments. I've update the patch with complete(I hope) test cases. There is still some remain tasks that I didn't apply in this patch:
- proactive report notification
- read report status
- sent timestamp for received message report
- removing the tick icon in the message bubble

I'll create bugs for these issue separatly since they might not be the first prority right now, but I'll target thme for 1.3, thanks.

Comment 16

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Ayman, José, we have some questions for you. Base is the Visual Design at https://bug919977.bugzilla.mozilla.org/attachment.cgi?id=8336250.

* the "Recipients" title should probably be different for incoming messages. Should we switch to "Sender" in that case? (Remember we don't do Group MMS for incoming MMS)
* we need actual colors and font sizes for the various elements
* we need a visual design for recipients/sender that don't match any contact
* we need the actual texts for the possible values for status and for delivery reports (read reports will be for a future patch). See technical values at [1] for SMS and [2] for MMS. Note: the "delivery" field is actually the "Status" field in the panel, the name is misleading.
* Please explicit the Visual Design when there is no MMS subject.
* Please explicit the Visual Design for SMS
* Please explicit the Visual Design for the cases we have one recipient/sender (SMS, and MMS in non-group MMS mode)

[1] http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/interfaces/nsIDOMMozSmsMessage.idl#25
[2] http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/interfaces/nsIDOMMozMmsMessage.idl#48

Thanks a lot, don't hesitate to ask if you have any question!

Comment 17

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Ayman, José, please see the previous comment 16.

Comment 18

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Ayman, José, another question for you:
* if one of the field changed while the user reads the panel, should the panel be updated live? (this could be done in another bug later, but it would be good to know)

Comment 19

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Comment on attachment 831387 [details] [review]
Link to github

The patch looks very good already.

Some comments on the pull request, nothing big.

Comment 20

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

move to 1.4

Comment 21

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Hey Steve, I've just seen we have a more complete visual design here: https://bug919977.bugzilla.mozilla.org/attachment.cgi?id=8336257

But some questions are still left unanswered :)

Comment 22

[ayman maat :maat]

(In reply to Julien Wajsberg [:julienw] from comment #16)
> Ayman, José, we have some questions for you. Base is the Visual Design at
> https://bug919977.bugzilla.mozilla.org/attachment.cgi?id=8336250.
> 
> * the "Recipients" title should probably be different for incoming messages.
> Should we switch to "Sender" in that case? (Remember we don't do Group MMS
> for incoming MMS)

reference wireframe pack: FFOS_MessageApp_V1.3_20131125_V8.0
page: 25
frame: 3. Incoming message report
‘Recipients’ title that is shown in the outgoing message report is specified here as ‘From’ for the incoming message report.

> * we need the actual texts for the possible values for status and for
> delivery reports (read reports will be for a future patch). See technical
> values at [1] for SMS and [2] for MMS. Note: the "delivery" field is
> actually the "Status" field in the panel, the name is misleading.

reference wireframe pack: FFOS_MessageApp_V1.3_20131125_V8.0
page: 27
Annotation 01, 02, 03

Julien, let me know if this does not answer your questions.

Comment 23

[ayman maat :maat]

(In reply to Julien Wajsberg [:julienw] from comment #18)
> Ayman, José, another question for you:
> * if one of the field changed while the user reads the panel, should the
> panel be updated live? (this could be done in another bug later, but it
> would be good to know)

It would be lovely if we could update the report live as the user is looking at it. Julien, is this possible? If so I would like Jose to propose a little visual realisation that is specific to the content that is being updated to draw the users attention to it rather than just having the text change.

ni? to Julien to confirm live update of report is technically possible / feasable

Comment 24

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

This will be possible in bug 933133 (we still need a Gecko support for now).

Comment 25

[José Vittone]

(In reply to Julien Wajsberg [:julienw] from comment #16)
> * we need actual colors and font sizes for the various elements
Yes, you can find them here: https://bug919977.bugzilla.mozilla.org/attachment.cgi?id=8344654
Also, you can download necessary iconography here: https://bugzilla.mozilla.org/attachment.cgi?id=8336252

> * we need a visual design for recipients/sender that don't match any contact
https://bug919977.bugzilla.mozilla.org/attachment.cgi?id=8344651
> * Please explicit the Visual Design when there is no MMS subject.
https://bug919977.bugzilla.mozilla.org/attachment.cgi?id=8344647
> * Please explicit the Visual Design for SMS
https://bug919977.bugzilla.mozilla.org/attachment.cgi?id=8344652

> * Please explicit the Visual Design for the cases we have one
> recipient/sender (SMS, and MMS in non-group MMS mode)
It is just the same as multiple recipients/senders, but with just one contact on it. 

Please, let me know if something is missing.

Comment 26

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

(In reply to ayman maat :maat from comment #22)
> (In reply to Julien Wajsberg [:julienw] from comment #16)
> > Ayman, José, we have some questions for you. Base is the Visual Design at
> > https://bug919977.bugzilla.mozilla.org/attachment.cgi?id=8336250.
> > 
> > * the "Recipients" title should probably be different for incoming messages.
> > Should we switch to "Sender" in that case? (Remember we don't do Group MMS
> > for incoming MMS)
> 
> reference wireframe pack: FFOS_MessageApp_V1.3_20131125_V8.0
> page: 25
> frame: 3. Incoming message report
> ‘Recipients’ title that is shown in the outgoing message report is specified
> here as ‘From’ for the incoming message report.

In the WF "From" is not capitalized but I suppose it should be capitalized, right?

> 
> > * we need the actual texts for the possible values for status and for
> > delivery reports (read reports will be for a future patch). See technical
> > values at [1] for SMS and [2] for MMS. Note: the "delivery" field is
> > actually the "Status" field in the panel, the name is misleading.
> 
> reference wireframe pack: FFOS_MessageApp_V1.3_20131125_V8.0
> page: 27
> Annotation 01, 02, 03
> 
> Julien, let me know if this does not answer your questions.

I have several remarks for this panel:

* "3) Unsuccessful: <output reason why message failed>" <= we can't do that now because Gecko does not store the reason for a failure. So we'd have to store it somewhere.
* we miss the "not-downloaded" state for MMS
* we miss the "error" state for a delivery report (I think that this can happen independantly to the SMS/MMS status).

Comment 27

[ayman maat :maat]

(In reply to Julien Wajsberg [:julienw] from comment #26)
> (In reply to ayman maat :maat from comment #22)
> > (In reply to Julien Wajsberg [:julienw] from comment #16)
> > > Ayman, José, we have some questions for you. Base is the Visual Design at
> > > https://bug919977.bugzilla.mozilla.org/attachment.cgi?id=8336250.
> > > 
> > > * the "Recipients" title should probably be different for incoming messages.
> > > Should we switch to "Sender" in that case? (Remember we don't do Group MMS
> > > for incoming MMS)
> > 
> > reference wireframe pack: FFOS_MessageApp_V1.3_20131125_V8.0
> > page: 25
> > frame: 3. Incoming message report
> > ‘Recipients’ title that is shown in the outgoing message report is specified
> > here as ‘From’ for the incoming message report.
> 
> In the WF "From" is not capitalized but I suppose it should be capitalized,
> right?

I believe the design guidelines are that we capitalise (wireframes do not necessarily follow visual design guidelines for text ;) )

> > 
> > > * we need the actual texts for the possible values for status and for
> > > delivery reports (read reports will be for a future patch). See technical
> > > values at [1] for SMS and [2] for MMS. Note: the "delivery" field is
> > > actually the "Status" field in the panel, the name is misleading.
> > 
> > reference wireframe pack: FFOS_MessageApp_V1.3_20131125_V8.0
> > page: 27
> > Annotation 01, 02, 03
> > 
> > Julien, let me know if this does not answer your questions.
> 
> I have several remarks for this panel:
> 
> * "3) Unsuccessful: <output reason why message failed>" <= we can't do that
> now because Gecko does not store the reason for a failure. So we'd have to
> store it somewhere.

Yep I know. I asked steve chung to open a bug for this so that we could work towards it in future releases. We should not block this release for it though.

> * we miss the "not-downloaded" state for MMS

I am not certain that we would have this state. as far as I am aware an undownloaded MMS is a SMS notification. Let me check this and get back to.

ni? to me 

> * we miss the "error" state for a delivery report (I think that this can
> happen independantly to the SMS/MMS status).

Could you confirm what the error states are for delivery reports? I was not aware that there were any the only states I was aware of were requested/received.

ni? to julien

Comment 28

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

(In reply to ayman maat :maat from comment #27)
> > 
> > In the WF "From" is not capitalized but I suppose it should be capitalized,
> > right?
> 
> I believe the design guidelines are that we capitalise (wireframes do not
> necessarily follow visual design guidelines for text ;) )

The VD doesn't have the panel with "from" ;)

> > * "3) Unsuccessful: <output reason why message failed>" <= we can't do that
> > now because Gecko does not store the reason for a failure. So we'd have to
> > store it somewhere.
> 
> Yep I know. I asked steve chung to open a bug for this so that we could work
> towards it in future releases. We should not block this release for it
> though.

Steve, is there a bug for this?


> > * we miss the "not-downloaded" state for MMS
> 
> I am not certain that we would have this state. as far as I am aware an
> undownloaded MMS is a SMS notification. Let me check this and get back to.
> 
> ni? to me 

Undownloaded MMS looks like a SMS for the user, but it's really not from the development PoV. So if you want it to look like a SMS we need detailed guidelines for this.


> 
> > * we miss the "error" state for a delivery report (I think that this can
> > happen independantly to the SMS/MMS status).
> 
> Could you confirm what the error states are for delivery reports? I was not
> aware that there were any the only states I was aware of were
> requested/received.
> 
> ni? to julien

I don't think we have any additional error states for delivery reports. Let's ask Gene. Gene, besides the "delivery == 'error'" case, what are the possible reasons to have the "error" state for the deliveryStatus property?

Comment 29

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #28)
> (In reply to ayman maat :maat from comment #27)
> > > * "3) Unsuccessful: <output reason why message failed>" <= we can't do that
> > > now because Gecko does not store the reason for a failure. So we'd have to
> > > store it somewhere.
> > 
> > Yep I know. I asked steve chung to open a bug for this so that we could work
> > towards it in future releases. We should not block this release for it
> > though.
> 
> Steve, is there a bug for this?
> 
Just checked with Gene. This will need some changes in webIDL, and the most of message/IM apps does not have the ability either. So stroing the error code per message is not in the future plan...

> 
> 
> > 
> > > * we miss the "error" state for a delivery report (I think that this can
> > > happen independantly to the SMS/MMS status).
> > 
> > Could you confirm what the error states are for delivery reports? I was not
> > aware that there were any the only states I was aware of were
> > requested/received.
> > 
> > ni? to julien
> 
> I don't think we have any additional error states for delivery reports.
> Let's ask Gene. Gene, besides the "delivery == 'error'" case, what are the
> possible reasons to have the "error" state for the deliveryStatus property?

delivery error and deliveryStatus error is different(defined in http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/gonk/MmsService.js#87 and http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/gonk/MmsService.js#91). A message could be delivered successfully(to server) but return error deliveryStatus if any problem between server and receiver. Gene might know some specific cases for error deliveryStatus.

Comment 30

[ayman maat :maat]

(In reply to Steve Chung [:steveck] from comment #29)
> (In reply to Julien Wajsberg [:julienw] from comment #28)
> > (In reply to ayman maat :maat from comment #27)
> > > > * "3) Unsuccessful: <output reason why message failed>" <= we can't do that
> > > > now because Gecko does not store the reason for a failure. So we'd have to
> > > > store it somewhere.
> > > 
> > > Yep I know. I asked steve chung to open a bug for this so that we could work
> > > towards it in future releases. We should not block this release for it
> > > though.
> > 
> > Steve, is there a bug for this?
> > 
> Just checked with Gene. This will need some changes in webIDL, and the most
> of message/IM apps does not have the ability either. So stroing the error
> code per message is not in the future plan...

Ok, but i still want a bug opening for this as, from a UX PoV, continuity and clarity of information delivery is a must and the current implementation is not in line with specification.

Its not a must have, but its something we need to add to the backlog to be address whenever there is the opportunity... otherwise it is just lost in time

Comment 31

[Steve Chung [:steveck]]

Comment on attachment 831387 [details] [review]
Link to github

Hi Julien, I updated the patch again based on your previous coments. One thing that I don't understand is about https://github.com/mozilla-b2g/gaia/pull/13654#discussion_r8140329. If date format dateTimeFormat_%c in English is wrong, should be just fix the shared local property(https://github.com/mozilla-b2g/gaia/blob/master/shared/locales/date/date.en-US.properties#L197)? Or you think it's because the date format does not match visual requirement? I can also add this to the visual follow-up bug.

Comment 32

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

(In reply to Steve Chung [:steveck] from comment #31)
> Comment on attachment 831387 [details] [review]
> Link to github
> 
> Hi Julien, I updated the patch again based on your previous coments. One
> thing that I don't understand is about
> https://github.com/mozilla-b2g/gaia/pull/13654#discussion_r8140329. If date
> format dateTimeFormat_%c in English is wrong, should be just fix the shared
> local
> property(https://github.com/mozilla-b2g/gaia/blob/master/shared/locales/date/
> date.en-US.properties#L197)?

dateTimeFormat_%c does what it should: it's similar to the "%c" implementation in C. So there is nothing to fix here :)

> Or you think it's because the date format does
> not match visual requirement?

Yep that's it!

> I can also add this to the visual follow-up
> bug.

If you want, I don't mind!

Comment 33

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

(In reply to Steve Chung [:steveck] from comment #29)
> 
> delivery error and deliveryStatus error is different(defined in
> http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/gonk/
> MmsService.js#87 and
> http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/gonk/
> MmsService.js#91). A message could be delivered successfully(to server) but
> return error deliveryStatus if any problem between server and receiver. Gene
> might know some specific cases for error deliveryStatus.

That's xactly what I meant: deliveryStatus error happens when delivery is error, but also in other situations. And I'd like Gene to help us with explaining the other situations :)

Comment 34

[ayman maat :maat]

(In reply to Julien Wajsberg [:julienw] from comment #28)
> (In reply to ayman maat :maat from comment #27)
> 
> > > * we miss the "not-downloaded" state for MMS
> > 
> > I am not certain that we would have this state. as far as I am aware an
> > undownloaded MMS is a SMS notification. Let me check this and get back to.
> > 
> > ni? to me 
> 
> Undownloaded MMS looks like a SMS for the user, but it's really not from the
> development PoV. So if you want it to look like a SMS we need detailed
> guidelines for this.

ok no problem, I was under the impression that the notification message to download content was actually an SMS. but i was wrong :)

so you are right we should output the state of the received MMS with attachment in the report that reflects its state in the thread. So i would suggest we extend the state beyond ‘not downloaded’ to include the following states:

1) Not downloaded
2) Downloading
3) Download failed
4) Downloaded

what do you think Julien?

ni? to Jose to provide the visual assets

Comment 35

[ayman maat :maat]

(In reply to Julien Wajsberg [:julienw] from comment #33)
> (In reply to Steve Chung [:steveck] from comment #29)
> > 
> > delivery error and deliveryStatus error is different(defined in
> > http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/gonk/
> > MmsService.js#87 and
> > http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/gonk/
> > MmsService.js#91). A message could be delivered successfully(to server) but
> > return error deliveryStatus if any problem between server and receiver. Gene
> > might know some specific cases for error deliveryStatus.
> 
> That's xactly what I meant: deliveryStatus error happens when delivery is
> error, but also in other situations. And I'd like Gene to help us with
> explaining the other situations :)

cool, well i have learnt something else :)

ok, well i am presuming that when a report fails there is no way for a user to recover from that failure (as in ‘retry’). So in the case that we have a failure in the delivery report (and read report when we get there) we can just output:

Delivery: report failed
Read: report failed

I have had a chat with José and he concluded that there is no need for visual design in these instances, as in we will not include iconography or alter font treatment.

Comment 36

[José Vittone]

Attachment: SMS_MMS_messageReport_not-downloaded.png

The only icon missing is "Not downloaded" in the message report , I suggest reuse the icon from email named "actionicon_email_downloadattachment" (available in box) at a 50% of opacity. Reuse in order to not create something very specific here and lighten it up to reinfornce that is not tappable.

Comment 37

[Gene Lian (I already quit Mozilla)]

(In reply to Julien Wajsberg [:julienw] from comment #33)
> That's xactly what I meant: deliveryStatus error happens when delivery is
> error, but also in other situations. And I'd like Gene to help us with
> explaining the other situations :)

Taking MMS for example, we have plenty of types to specify the |message.deliveryStatus|:

  case MMS.MMS_PDU_STATUS_RETRIEVED:
    deliveryStatus = DELIVERY_STATUS_SUCCESS;
    break;
  case MMS.MMS_PDU_STATUS_EXPIRED:
  case MMS.MMS_PDU_STATUS_REJECTED:
  case MMS.MMS_PDU_STATUS_UNRECOGNISED:
  case MMS.MMS_PDU_STATUS_UNREACHABLE:
    deliveryStatus = DELIVERY_STATUS_REJECTED;
    break;
  case MMS.MMS_PDU_STATUS_DEFERRED:
    deliveryStatus = DELIVERY_STATUS_PENDING;
    break;
  case MMS.MMS_PDU_STATUS_INDETERMINATE:
    deliveryStatus = DELIVERY_STATUS_NOT_APPLICABLE;
    break;

but we only fire events to Gaia for only two cases:

  1. ondeliverysuccess event for MMS.MMS_PDU_STATUS_RETRIEVED
  2. ondeliveryerror event for MMS.MMS_PDU_STATUS_REJECTED

which means |ondeliverysuccess| is only triggered when the MMS is delivered for sure; |ondeliveryerror| is only triggered when the MMS is rejected to deliver for sure.

We didn't fire |ondeliveryerror| for some failed cases. For example, when the MMS is expired on the server (receivers haven't downloaded it for a long time). It depends on how detailed our UI desires and how we interpret the |ondeliveryerror|.

Comment 38

[Steve Chung [:steveck]]

Comment on attachment 831387 [details] [review]
Link to github

I add more fixing for timestamp and add not-downloaded status in the report. Another visual polish bug created in Bug 949379.

Comment 39

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #37)
> (In reply to Julien Wajsberg [:julienw] from comment #33)
> > That's xactly what I meant: deliveryStatus error happens when delivery is
> > error, but also in other situations. And I'd like Gene to help us with
> > explaining the other situations :)
> 
> Taking MMS for example, we have plenty of types to specify the
> |message.deliveryStatus|:
> 
>   case MMS.MMS_PDU_STATUS_RETRIEVED:
>     deliveryStatus = DELIVERY_STATUS_SUCCESS;
>     break;
>   case MMS.MMS_PDU_STATUS_EXPIRED:
>   case MMS.MMS_PDU_STATUS_REJECTED:
>   case MMS.MMS_PDU_STATUS_UNRECOGNISED:
>   case MMS.MMS_PDU_STATUS_UNREACHABLE:
>     deliveryStatus = DELIVERY_STATUS_REJECTED;
>     break;
>   case MMS.MMS_PDU_STATUS_DEFERRED:
>     deliveryStatus = DELIVERY_STATUS_PENDING;
>     break;
>   case MMS.MMS_PDU_STATUS_INDETERMINATE:
>     deliveryStatus = DELIVERY_STATUS_NOT_APPLICABLE;
>     break;
> 
> but we only fire events to Gaia for only two cases:
> 
>   1. ondeliverysuccess event for MMS.MMS_PDU_STATUS_RETRIEVED
>   2. ondeliveryerror event for MMS.MMS_PDU_STATUS_REJECTED
> 
> which means |ondeliverysuccess| is only triggered when the MMS is delivered
> for sure; |ondeliveryerror| is only triggered when the MMS is rejected to
> deliver for sure.
> 
> We didn't fire |ondeliveryerror| for some failed cases. For example, when
> the MMS is expired on the server (receivers haven't downloaded it for a long
> time). It depends on how detailed our UI desires and how we interpret the
> |ondeliveryerror|.

Gene, there is something I don't understand.

I'm taking the SMS Message IDL [1] as reference for possible values (the MMS message IDL [2] has not a lot of documentation).

So the messages can have a "deliveryStatus" with a value "error". Does this happen in the case "deliveryStatus = DELIVERY_STATUS_REJECTED" that you're saying above?

Then I think "ondeliveryerror" should be fired as soon as we get a "error" value in the "deliveryStatus" field.

For MMS it's not so easy because we have several deliveryInfo items... so I don't know here :/

In this bug, we're not doing anything on events anyway, we're only displaying what's in the message fields. Reacting to events will be bug 933133.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/interfaces/nsIDOMMozSmsMessage.idl#31
[2] http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/interfaces/nsIDOMMozMmsMessage.idl#18

Comment 40

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Comment on attachment 831387 [details] [review]
Link to github

I think we need one more round.

some comments, but the biggest ones are:
* the fact that we add the "hide" class to a lot of elements when we show the view, I think we can do better
* we use a lot of ids in the markup, I'm not comfortable with this
* the patch regresses the suggestion list display in the new message view

Comment 41

[Gene Lian (I already quit Mozilla)]

(In reply to Julien Wajsberg [:julienw] from comment #39)
> I'm taking the SMS Message IDL [1] as reference for possible values (the MMS
> message IDL [2] has not a lot of documentation).

Yeap... I need to make up the IDL descriptions at bug 876267.

> 
> So the messages can have a "deliveryStatus" with a value "error". Does this
> happen in the case "deliveryStatus = DELIVERY_STATUS_REJECTED" that you're
> saying above?

No, not all of them. In the current implementation, we only fire ondeliveryerror event for:

  case MMS.MMS_PDU_STATUS_REJECTED:

but not for:

  case MMS.MMS_PDU_STATUS_EXPIRED:
  case MMS.MMS_PDU_STATUS_UNRECOGNISED:
  case MMS.MMS_PDU_STATUS_UNREACHABLE:

> 
> Then I think "ondeliveryerror" should be fired as soon as we get a "error"
> value in the "deliveryStatus" field.

Yes, I agree with you. We shouldn't limit it to MMS_PDU_STATUS_REJECTED only. In the W3C proposal, we don't do that either [1].

Will fire a bug for this in the next week. Anyway, it's not a critical issue, though. We can fix it for V1.4.

[1] http://messaging.sysapps.org/#widl-MmsManager-ondeliveryerror

> For MMS it's not so easy because we have several deliveryInfo items... so I
> don't know here :/

Yeap... the ondeliveryerror will carry a full set of deliveryInfo items for all the receivers. Gaia needs to iterate through them to see which receiver reports the failed report.

Comment 42

[Steve Chung [:steveck]]

Comment on attachment 831387 [details] [review]
Link to github

Add more fix in the patch:
* Apply 'information' class to 'thread-messages' instead of hiding specific elements
* Replace most of ids with classes in information views
* Revert the position of contact list to absolute and only apply static in report view.
* Export ReportView and GrouView to global
* Refine element naming with 'information' prefix
* And  more fixing based on previous comments...

Contact list title and timestamp localize event handler(maybe we can refactor this part like moving to Utils in another bug ?) will be added in follow-up bug, thnaks.

Comment 43

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Comment on attachment 831387 [details] [review]
Link to github

The patch looks great!

But I can't give r+ yet because there is a unit test failure ;) I think you need mocks for GroupView and ReportView and use them in the various test cases.

Also there is jshint issues as well. And one useless nit in the unit test.

Comment 44

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #43)
> Comment on attachment 831387 [details] [review]
> Link to github
> 
> The patch looks great!
> 
> But I can't give r+ yet because there is a unit test failure ;) I think you
> need mocks for GroupView and ReportView and use them in the various test
> cases.
> 
> Also there is jshint issues as well. And one useless nit in the unit test.

Sorry for the careless changes :s..  I fixed the test and jshint again, and Travis seems only failed in unrelated marionette test, thanks!

Comment 45

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Comment on attachment 831387 [details] [review]
Link to github

r=me

yeah!

Comment 46

[Steve Chung [:steveck]]

Thanks for all the detailed reviews! 
Landed in master: 503d54bbffa831c45654a251d19711de55c5f0a1

Hi Rick, since I've added some fixing based on your previous suggestion, if you still have other concern for this patch, I created a follow-up bug 949379 and any suggestion is welcome. Thnaks for the feedback you gave in this patch.