792328 - B2G MMS: MMSCONF-MED-C-00{1,2,3,4,5}: Support for {us-ascii,utf-8,utf-16,iso-8859-1} as media type text

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

See OMA-TS-MMS-CONF-V1_3-20110913-A section 7.1.9, and Appendix C.1.12
See OMA-ETS-MMS_CON-V1_3-20101015-C section 5.1.2.1.2, 5.2.3.1.2

Comment 1

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

See also OMA-ETS-MMS_CON-V1_3-20101015-C section 5.1.2.1.1, 5.2.3.1.1, 5.2.3.1.3

Comment 2

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

Attachment: Add utf-16 into WellKnownCharset and unit test for this modifications

Comment 3

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

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #2)
> Created attachment 683833 [details] [diff] [review]
> Add utf-16 into WellKnownCharset and unit test for this modifications

In this issue, we have to decode blob content of messages whose content-type is text/*. Depends on bug 801632 as well.

Comment 4

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

Ths code about register converter is in intl/uconv/src/nsUConvModule.cpp.

Comment 5

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

Attachment: Process text type mms attachment

Comment 6

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

Attachment: Process text type mms attachment

Comment 7

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

Attachment: WIP

Comment 8

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

Attachment: WIP

Comment 9

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

Attachment: Support more decoder in MMS text attachment

Comment 10

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

Comment on attachment 700913 [details] [diff] [review]
Support more decoder in MMS text attachment

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

::: dom/mms/src/ril/WspPduHelper.jsm
@@ +2165,5 @@
> +
> +    let str;
> +    if(!entry.converter) {
> +      //Todo: Check default charset.
> +      entry.converter = "UTF-8";

Everytime you put a TODO in comments, it must be followed by a bug number. But, maybe we can just solve it in this bug?

@@ +2170,5 @@
> +    }
> +
> +    // Read a possible string quote(<Octet 127>).
> +    let begin = data.offset;
> +    if (Octet.decode(data) != 127) {

Everytime you put a TODO in comments, it must be followed by a bug number. But, maybe we can just solve it in this bug?

@@ +2297,3 @@
>          let content = null;
> +        if (headers["content-type"].media.indexOf("text/") === 0) {
> +          content = StringContent.decode(data, contentEnd,

Since the StringContent.decode may throw, you'd better fallback to store it as a blob. Something like:

  let octetArray = Octet.decodeMultiple(data, contentEnd);
  let content = null;
  if (octetArray) {
    content = StringContent.decode(); // never throws
    if (!content) {
      content = new Blob(...);
    }
  }

Comment 11

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #10)
> Comment on attachment 700913 [details] [diff] [review]
> Review of attachment 700913 [details] [diff] [review]:
-----------------------------------------------------------------
> ::: dom/mms/src/ril/WspPduHelper.jsm
> @@ +2170,5 @@
> > +    }
> > +
> > +    // Read a possible string quote(<Octet 127>).
> > +    let begin = data.offset;
> > +    if (Octet.decode(data) != 127) {
> 
> Everytime you put a TODO in comments, it must be followed by a
> bug number. But, maybe we can just solve it in this bug?

Wrong paste. I mean, there attachment content is not a certain field in MMS/WSP binary xml. There is no string quote, actually no any format but strings encoded as utf-8/utf-16/etc.

Comment 12

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

Attachment: Support more decoder in MMS text attachment

Comment 13

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

Comment on attachment 701114 [details] [diff] [review]
Support more decoder in MMS text attachment

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

::: dom/mms/src/ril/WspPduHelper.jsm
@@ +2156,5 @@
> +    let entry = WSP_WELL_KNOWN_CHARSETS[charset];
> +    if (!entry) {
> +      // Set converter to default one.
> +      // @see OMA-TS-MMS-CONF-V1_3-20050526-D 7.1.9
> +      entry.converter = "UTF-8";

entry is undefined here. Accessing 'entry.*' will trigger an exception.

@@ +2163,5 @@
> +    let str;
> +    if(!entry.converter) {
> +      // Set converter to default one.
> +      // @see OMA-TS-MMS-CONF-V1_3-20050526-D 7.1.9
> +      entry.converter = "UTF-8";

You should make 'converter' an local variable here, or you'll change the values in WSP_WELL_KNOWN_CHARSETS. For example:

  let entry = WSP_WELL_KNOWN_CHARSETS[charset];
  let converter = (entry && entry.converter) || "UTF-8";

@@ +2166,5 @@
> +      // @see OMA-TS-MMS-CONF-V1_3-20050526-D 7.1.9
> +      entry.converter = "UTF-8";
> +    }
> +
> +    let raw = Octet.decodeMultiple(data, dataEnd);

Please move this outside the decode func as comment #10 shows.

::: dom/mms/tests/test_wsp_pdu_helper.js
@@ +1295,5 @@
> +      }, raw, str);
> +  }
> +
> +  let (entry = WSP.WSP_WELL_KNOWN_CHARSETS["utf-16"]) {
> +  // "Mozilla" in full width.

nit: intent

Comment 14

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

Attachment: Support more decoder in MMS text attachment

Comment 15

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

Attachment: Support more decoder in MMS text attachment

Comment 16

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

Attachment: Support more decoder in MMS text attachment

Comment 17

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

Comment on attachment 703222 [details] [diff] [review]
Support more decoder in MMS text attachment

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

Simple and elegant! That's what we really want ;)

Comment 18

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8eb23e5170d0

Comment 19

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

Thanks for your review. :)

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #17)
> Comment on attachment 703222 [details] [diff] [review]
> Support more decoder in MMS text attachment
> 
> Review of attachment 703222 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Simple and elegant! That's what we really want ;)

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/8eb23e5170d0

Comment 21

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

For mms merge to b2g18.

Comment 22

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

Attachment: [b2g18]-011-bug-792328-fix

Comment 23

[Gene Lian (I already quit Mozilla)]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/06544af216e5

Comment 24

[Ryan VanderMeulen [:RyanVM]]

The entire set of clian's pushes was backed out for multiple reasons.
https://tbpl.mozilla.org/?tree=Mozilla-B2g18&rev=a0b06192f882

1.) The tree rules are clear that you are not to land on top of bustage. At the time you pushed, both B2G Mn and B2G xpcshell had bustage from prior commits that hadn't been backed out yet.
2.) The tree rules are also clear that you are to watch your pushes for any bustage and handle them accordingly. mozilla-inbound is the ONLY tree where this rule does not apply.
3.) Even after the earlier bustage was backed out, something in one of your many pushes was causing further B2G Mn failures as shown in the log below.
https://tbpl.mozilla.org/php/getParsedLog.php?id=20424173&tree=Mozilla-B2g18
4.) This isn't cause for backout by itself, but it is also strongly preferred to not push each commit individually as our build and testing resources are limited and doing so stretches them even thinner. Please limit your number of pushes as much as possible unless you have good reason for keeping them separate.

Comment 25

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

https://hg.mozilla.org/releases/mozilla-b2g18/rev/99680a32f297