1119741 - [FFOS7715 v2.1]We should support to parse the document without application characteristic

Status: RESOLVED WONTFIX

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

Description

[jinghua.xing]

Attachment: 01006_cmcc_no_application.xml

We should support to parse the document in oma cp messages without application characteristic. The document is in the attachment.

Comment 1

[jinghua.xing]

Hi Gabriele, 

Can you help me check this issue?

Thank you

Comment 2

[jinghua.xing]

In OMA-WAP-TS-ProvCont-V1_1-20090728-A chapter 6.2 Example 2, it shows a document without the application characteristic, but it also say this document contains "Infrastructure information with two bearers and thus two network access points."

So I think it is necessary for us to support for the document without application characteristic.

Comment 3

[Gabriele Svelto [:gsvelto]]

(In reply to jinghua.xing from comment #1)
> Can you help me check this issue?

Sure, I'll leave the needinfo on as I'm busy with the 2.2 deadline. As soon as I'm done with that I'll help here.

Comment 4

[jinghua.xing]

Attachment: no_application.patch

Hi Gabriele, 

I have tried to made a patch for this issue? Please help me to review if this way to parse the document without the application node is correct? 

Thank you

Comment 5

[Gabriele Svelto [:gsvelto]]

(In reply to jinghua.xing from comment #2)
> In OMA-WAP-TS-ProvCont-V1_1-20090728-A chapter 6.2 Example 2, it shows a
> document without the application characteristic, but it also say this
> document contains "Infrastructure information with two bearers and thus two
> network access points."
> 
> So I think it is necessary for us to support for the document without
> application characteristic.

Yes, it looks that way. I'll go ahead with the review.

Comment 6

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8549328 [details] [diff] [review]
no_application.patch

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

r- because there's a couple of things that need to be addressed in the patch, mostly related to its structure as you'll see from the comments. A unit-test covering this particular case is also required.

::: apps/wappush/js/parsed_doc.js
@@ +214,3 @@
>        // The APPLICATION nodes can occur 0 or more times but it makes no sense
>        // to continue when the current document doesn't have at least one one of
>        // them.

This comment is now out of date, you should remove it and instead explain what we do when there is no APPLICATION node.

@@ +214,5 @@
>        // The APPLICATION nodes can occur 0 or more times but it makes no sense
>        // to continue when the current document doesn't have at least one one of
>        // them.
> +      if (!this._applicationNodes || (this._applicationNodes.length == 0)) {
> +        for (var j = 0; j < this._pxLogicalNodes.length; j++) {

All this code is practically identical to the one here:

https://github.com/mozilla-b2g/gaia/blob/ad5450b073730d49f100d02c08ce1925564d55e0/apps/wappush/js/parsed_doc.js#L271

You should factor it out into a separate function and replace both bodies of code with it.

Comment 7

[Gabriele Svelto [:gsvelto]]

José, I'm reviewing this but I wanted your feedback on the approach taken here. Do you think it makes sense to parse the PXPHYSICAL nodes this way in absence of an APPLICATION characteristic?

Comment 8

[José Antonio Olivera Ortega [:jaoo]]

(In reply to jinghua.xing from comment #0)
> Created attachment 8546530 [details]
> 01006_cmcc_no_application.xml
> 
> We should support to parse the document in oma cp messages without
> application characteristic. The document is in the attachment.

IMHO we shouldn't. The application characteristics node let's know what the OMA CP is about. We handle APPID = w2 and APPID = w4 which are messages providing APN settings (default and mms ones respectively). Have a look at the list of different values for the APPID node at [1] please. How would we know what settings the OMA CP message provides without having application characteristic nodes?

[1] http://technical.openmobilealliance.org/Technical/technical-information/omna/omna-dm-ac-registry

(In reply to Gabriele Svelto [:gsvelto] from comment #7)
> José, I'm reviewing this but I wanted your feedback on the approach taken
> here. Do you think it makes sense to parse the PXPHYSICAL nodes this way in
> absence of an APPLICATION characteristic?

No, it doesn't. See the reason above please.

Comment 9

[jinghua.xing]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #8)
In android, the apn parsed from the document without APPLICATION is default type

Comment 10

[jinghua.xing]

As I said in comment 2
> In OMA-WAP-TS-ProvCont-V1_1-20090728-A chapter 6.2 Example 2, it shows a
> document without the application characteristic, but it also say this
> document contains "Infrastructure information with two bearers and thus two
> network access points."

I think we should parsed the doc without APPLICATION and we can set the apn.type to default as android.

Comment 11

[José Antonio Olivera Ortega [:jaoo]]

(In reply to jinghua.xing from comment #10)
> As I said in comment 2
> > In OMA-WAP-TS-ProvCont-V1_1-20090728-A chapter 6.2 Example 2, it shows a
> > document without the application characteristic, but it also say this
> > document contains "Infrastructure information with two bearers and thus two
> > network access points."
> 
> I think we should parsed the doc without APPLICATION and we can set the
> apn.type to default as android.

The application characteristics node lets us know what the OMA CP is about. If we take your idea we should make the assumption that a message without application characteristics nodes is *always* an OMA CP message providing default APNs (internet browsing ones) and the correctness of it is quite questionable IMHO.

Comment 12

[Beatriz Rodríguez [:brg]]

IMHO this is a new feature request, not a real bug, and it should be reviewed carefully to be implemented in future FxOS releases.

Comment 14

[BMO Automation]

Firefox OS is not being worked on