Philipp von Weitershausen [:philikon] Reporter
	Description • 12 years ago

When data calls fail, we should notify the UI somehow. I propose a "dataerror" event on the WebMobileConnection API. The RIL actually gives us a data call fail cause, so we should be able to determine why a data call failed -- in theory. In practice I've seen data calls fail silently in that an IP is awarded to the client but no traffic is ever routed. Maybe we can even catch those cases.

	Fernando R. Sela (no CC, needinfo please) [:frsela] Assignee
	Comment 2 • 12 years ago

Sorry, on previous patch I forgot to remove a DEBUG line

	Philipp von Weitershausen [:philikon] Reporter
	Comment 3 • 12 years ago

Comment on attachment 630139 [details] [diff] [review]
Patch to notify connection errors - V1.1

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

Good start, but I think the interface is too RIL-specific.

CCing Jonas for his thoughts.

::: dom/network/interfaces/nsIDOMMobileConnection.idl
@@ +57,5 @@
>    /**
> +   * The 'datacallerror' event is notified whenever the data connection object
> +   * receives an error from the RIL
> +   */
> +  attribute nsIDOMEventListener ondatacallerror;

The whole concept of a data *call* is pretty system specific, that's why I proposed a "dataerror" event on this object. Alternatively, we can have a "connectionerror" or just "error" event on nsIDOMMozMobileConnectionInfo.

@@ +215,5 @@
>  
> +  /**
> +   * Error code
> +   */
> +  readonly attribute jsval errorCode;

What is this? This needs to be specified/documented better. `jsval` is not very informative.

In any case I think we should be using DOMError, e.g. akin to how we do in WebTelephony, and not expose the data call fail cause directly. Remember, the idea is to build a general purpose Web API, so we mustn't rely on internal status/error codes from the RIL.

@@ +220,5 @@
> +
> +  /**
> +   * Retry connection counter
> +   */
> +  readonly attribute jsval connectionRetryCounter;

Why expose this?

In fact, now that we expose connection errors to content, we can probably move all that retry stuff to content (Gaia). It can keep its own counter in localStorage or Settings or whatever.

	Fernando R. Sela (no CC, needinfo please) [:frsela] Assignee
	Comment 4 • 12 years ago

(In reply to Philipp von Weitershausen [:philikon] from comment #3)
> ::: dom/network/interfaces/nsIDOMMobileConnection.idl
> @@ +57,5 @@
> >    /**
> > +   * The 'datacallerror' event is notified whenever the data connection object
> > +   * receives an error from the RIL
> > +   */
> > +  attribute nsIDOMEventListener ondatacallerror;
> 
> The whole concept of a data *call* is pretty system specific, that's why I
> proposed a "dataerror" event on this object. Alternatively, we can have a
> "connectionerror" or just "error" event on nsIDOMMozMobileConnectionInfo.
> 

I used datacallerror in order to use the same RadioInterfaceLayer's nomenclature, anyway I agree is pretty system specific and we should hide this to upper layers.

> @@ +215,5 @@
> >  
> > +  /**
> > +   * Error code
> > +   */
> > +  readonly attribute jsval errorCode;
> 
> What is this? This needs to be specified/documented better. `jsval` is not
> very informative.
> 

This is the error code returned by the RIL.

> In any case I think we should be using DOMError, e.g. akin to how we do in
> WebTelephony, and not expose the data call fail cause directly. Remember,
> the idea is to build a general purpose Web API, so we mustn't rely on
> internal status/error codes from the RIL.
> 

Ok, so if I understood well, you consider is only necessary to inform that an error has occurred BUT not to inform which specific error is.

> @@ +220,5 @@
> > +
> > +  /**
> > +   * Retry connection counter
> > +   */
> > +  readonly attribute jsval connectionRetryCounter;
> 
> Why expose this?
> 

To inform how many retries because after 10 it stops retrying.
I agree again, this is too specific to the implementation. Probably it's better to only inform the UI after all retries had been done, meanwhile, not inform anything. WDYT?

> In fact, now that we expose connection errors to content, we can probably
> move all that retry stuff to content (Gaia). It can keep its own counter in
> localStorage or Settings or whatever.

Agree to not expose connection errors retries, I think this shall be transparent to GAIA.

	Philipp von Weitershausen [:philikon] Reporter
	Comment 5 • 12 years ago

(In reply to Fernando R. Sela from comment #4)
> I used datacallerror in order to use the same RadioInterfaceLayer's
> nomenclature, anyway I agree is pretty system specific and we should hide
> this to upper layers.

RIL specific nomenclature should not be part of a Web API.

> > In any case I think we should be using DOMError, e.g. akin to how we do in
> > WebTelephony, and not expose the data call fail cause directly. Remember,
> > the idea is to build a general purpose Web API, so we mustn't rely on
> > internal status/error codes from the RIL.
> > 
> 
> Ok, so if I understood well, you consider is only necessary to inform that
> an error has occurred BUT not to inform which specific error is.

No. We should expose the specific error, but as a DOMError, not as a RIL-specific constant.

> > In fact, now that we expose connection errors to content, we can probably
> > move all that retry stuff to content (Gaia). It can keep its own counter in
> > localStorage or Settings or whatever.
> 
> Agree to not expose connection errors retries, I think this shall be
> transparent to GAIA.

No. What I was saying in comment 3, Gaia should be in *charge* of doing connection retries etc. That way we can change policy much more easily, too, by simply shipping a new version of the Gaia app.

	Fernando R. Sela (no CC, needinfo please) [:frsela] Assignee
	Comment 6 • 12 years ago

(In reply to Philipp von Weitershausen [:philikon] from comment #5)
> (In reply to Fernando R. Sela from comment #4)
> > > In fact, now that we expose connection errors to content, we can probably
> > > move all that retry stuff to content (Gaia). It can keep its own counter in
> > > localStorage or Settings or whatever.
> > 
> > Agree to not expose connection errors retries, I think this shall be
> > transparent to GAIA.
> 
> No. What I was saying in comment 3, Gaia should be in *charge* of doing
> connection retries etc. That way we can change policy much more easily, too,
> by simply shipping a new version of the Gaia app.

I'm not completely agree. From my point of view, the retry policy is a low-level feature that should be defined by each operator on each country based on how their own network parameters.

So moving this to higher levels is like move the responsibility to define (ie, routing tables) to Gaia instead of Gecko.

I consider that this parameters shouldn't be modified too much and always by the operator (using the variant work @jaoo is working on)

So from my point of view, the control and policy shall be in *charge* of Gecko.

	Philipp von Weitershausen [:philikon] Reporter
	Comment 7 • 12 years ago

(In reply to Fernando R. Sela from comment #6)
> I'm not completely agree. From my point of view, the retry policy is a
> low-level feature that should be defined by each operator on each country
> based on how their own network parameters.
> 
> So moving this to higher levels is like move the responsibility to define
> (ie, routing tables) to Gaia instead of Gecko.
> 
> I consider that this parameters shouldn't be modified too much and always by
> the operator (using the variant work @jaoo is working on)
> 
> So from my point of view, the control and policy shall be in *charge* of
> Gecko.

Putting the operator in charge of this policy is a fine idea. And that's *exactly* why we don't want to bake it into Gecko, but leave it up to a Settings app that the operator can ship and change at their own discretion. There's no rule that says just because something is a "low-level" operator policy, it has to be in Gecko or Gonk.

	Philipp von Weitershausen [:philikon] Reporter
	Comment 8 • 12 years ago

Btw, Fernando, even if Jonas hasn't given feedback yet (he's on vacation atm), we can address review comments in the patch. So feel free to update it for a second review pass.

	Fernando R. Sela (no CC, needinfo please) [:frsela] Assignee
	Comment 9 • 12 years ago

I'm having some troubles integrating DOMError ... meanwhile I solve these issues, I uploaded a new patch with the other two philikon's comments (renamed datacallerror to dataerror and remove the retryCounter from the DOM.

	Fernando R. Sela (no CC, needinfo please) [:frsela] Assignee
	Comment 11 • 12 years ago

Alternative way to notify the error (Using a similar scheme as USSD) - Part 1 -> DataErrorEvent Class

	Fernando R. Sela (no CC, needinfo please) [:frsela] Assignee
	Comment 12 • 12 years ago

Alternative way to notify the error (Using a similar scheme as USSD) - Part 2 -> Event dispatch with error description (like V2.1 but now using the new DataErrorEvent)

	Fernando Jiménez Moreno [:ferjm]
	Comment 13 • 12 years ago

Fernando, as I mentioned to you before, I was asked to change the way USSD events are created (Bug 763326). So maybe you shouldn't be using USSD events as an example. You have plenty of better examples at content/events (https://hg.mozilla.org/mozilla-central/file/7209f9f14a7d/content/events/src).

	Fernando R. Sela (no CC, needinfo please) [:frsela] Assignee
	Comment 14 • 12 years ago

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #13)
> Fernando, as I mentioned to you before, I was asked to change the way USSD
> events are created (Bug 763326). So maybe you shouldn't be using USSD events
> as an example. You have plenty of better examples at content/events
> (https://hg.mozilla.org/mozilla-central/file/7209f9f14a7d/content/events/
> src).

Yeap, I know. Since I parked this bug because other issues, I wanted a working solution now and I would like to improve it now, but almost, now is working ...

So, ok, I'll use another event solution as you suggested :)

	Philipp von Weitershausen [:philikon] Reporter
	Comment 15 • 12 years ago

Comment on attachment 638603 [details] [diff] [review]
Patch to notify connection errors - V2.1

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

::: dom/network/interfaces/nsIDOMMobileConnection.idl
@@ +276,5 @@
>  
> +  /**
> +   * Error code
> +   */
> +  readonly attribute jsval errorCode;

We don't want to expose the RIL error code to the Web API. I suggest we dispatch a DOMError to `ondataerror`.

::: dom/network/src/MobileConnection.cpp
@@ +27,5 @@
>  const char* kVoiceChangedTopic     = "mobile-connection-voice-changed";
>  const char* kDataChangedTopic      = "mobile-connection-data-changed";
>  const char* kCardStateChangedTopic = "mobile-connection-cardstate-changed";
>  const char* kUssdReceivedTopic     = "mobile-connection-ussd-received";
> +const char* kDataError             = "mobile-connection-data-error";

kDataErrorTopic

::: dom/system/gonk/RILContentHelper.js
@@ +51,5 @@
>  const kVoiceChangedTopic     = "mobile-connection-voice-changed";
>  const kDataChangedTopic      = "mobile-connection-data-changed";
>  const kCardStateChangedTopic = "mobile-connection-cardstate-changed";
>  const kUssdReceivedTopic     = "mobile-connection-ussd-received";
> +const kDataError             = "mobile-connection-data-error";

kDataErrorTopic

::: dom/system/gonk/ril_worker.js
@@ +3126,5 @@
>  RIL[REQUEST_SETUP_DATA_CALL] = function REQUEST_SETUP_DATA_CALL(length, options) {
>    if (options.rilRequestError) {
>      // On Data Call error, we shall notify caller
> +    this.sendDOMMessage({type: "datacallerror",
> +                         datacall: options});

Simpler:

  options.type = "datacallerror"
  this.sendDOMMessage(options);

But you're missing the actual point: we need to find out *why* the data call failed. REQUEST_LAST_DATA_CALL_FAIL_CAUSE will tell you that. Take a look at how it's implemented for voice calls.

	Philipp von Weitershausen [:philikon] Reporter
	Comment 16 • 12 years ago

Comment on attachment 638603 [details] [diff] [review]
Patch to notify connection errors - V2.1

Hrm, I now realize that this patch has already been obsoleted by newer ones. Please make sure to mark old patches as obsolete when you upload new ones. Thanks!

	Philipp von Weitershausen [:philikon] Reporter
	Comment 17 • 12 years ago

Comment on attachment 639301 [details] [diff] [review]
Patch to notify connection errors - V3 (Part 1)

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

Ben is a better reviewer for this part.

	Philipp von Weitershausen [:philikon] Reporter
	Comment 18 • 12 years ago

Comment on attachment 639302 [details] [diff] [review]
Patch to notify connection errors - V3 (Part 2)

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

Much of comment 15 still applies. In particular, we don't want to expose RIL-specific error codes to the web. Please take a look at how we use DOMError in the telephony API.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 19 • 12 years ago

Comment on attachment 639301 [details] [diff] [review]
Patch to notify connection errors - V3 (Part 1)

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

::: dom/network/src/DataErrorEvent.cpp
@@ +4,5 @@
> +
> +#include "DataErrorEvent.h"
> +#include "nsIDOMClassInfo.h"
> +#include "nsDOMClassInfoID.h"
> +#include "nsContentUtils.h"

Nit: I don't think you need the last two includes here.

	Fernando R. Sela (no CC, needinfo please) [:frsela] Assignee
	Comment 20 • 12 years ago

(In reply to Philipp von Weitershausen [:philikon] from comment #15)
>
> But you're missing the actual point: we need to find out *why* the data call
> failed. REQUEST_LAST_DATA_CALL_FAIL_CAUSE will tell you that. Take a look at
> how it's implemented for voice calls.

Hi, Using REQUEST_LAST_DATA_CALL_FAIL_CAUSE always return 0xffff ... reviewing the reference, this parcel is deprecated in favor of the status field in RIL_Data_Call_Response_v6 stored into rilRequestError, so I'll use this other value.

Today I'll upload a new version of the patch with this corrections and, of course, also returning *why* the data call failed ;)

Regards

	Fernando R. Sela (no CC, needinfo please) [:frsela] Assignee
	Comment 21 • 12 years ago

nit's corrected

	Fernando R. Sela (no CC, needinfo please) [:frsela] Assignee
	Comment 22 • 12 years ago

Take into account this patch also hacks the Bug 775028 as we discussed via IRC

	Philipp von Weitershausen [:philikon] Reporter
	Comment 23 • 12 years ago

Comment on attachment 643314 [details] [diff] [review]
Patch to notify connection errors - V4 (Part 2)

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

Looks good to me, with the one point mentioned below addressed.

Flagging Sicking for superreview on the API change (`dataerror` event).

::: dom/system/gonk/RILContentHelper.js
@@ +618,5 @@
>    },
> +
> +  _deliverDataCallback: function _deliverDataCallback(name, args) {
> +    debug("callback handler for " + name + " args: " + args);
> +    Services.obs.notifyObservers(null, kDataErrorTopic, args);

I don't understand the point of this function, especially the `name` argument that's not really used. Please just inline the observer notification call.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 24 • 12 years ago

Comment on attachment 643314 [details] [diff] [review]
Patch to notify connection errors - V4 (Part 2)

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

::: dom/system/gonk/ril_consts.js
@@ +1481,5 @@
> +RIL_DATACALL_FAILCAUSE_TO_GECKO_DATACALL_ERROR[DATACALL_FAIL_ONLY_IPV4_ALLOWED]                = GECKO_DATACALL_ERROR_ONLY_IPV4_ALLOWED;
> +RIL_DATACALL_FAILCAUSE_TO_GECKO_DATACALL_ERROR[DATACALL_FAIL_ONLY_IPV6_ALLOWED]                = GECKO_DATACALL_ERROR_ONLY_IPV6_ALLOWED;
> +RIL_DATACALL_FAILCAUSE_TO_GECKO_DATACALL_ERROR[DATACALL_FAIL_ONLY_SINGLE_BEARER_ALLOWED]       = GECKO_DATACALL_ERROR_ONLY_SINGLE_BEARER_ALLOWED;
> +RIL_DATACALL_FAILCAUSE_TO_GECKO_DATACALL_ERROR[DATACALL_FAIL_PROTOCOL_ERRORS]                  = GECKO_DATACALL_ERROR_PROTOCOL_ERRORS;
> +// TODO Bug #775028: Analyse why RIL returns this error codes as positive values instead the standard ones.

Hi frsela and philikon
  I have discussed Bug 775028 with hsinyi and vicamo, we all think it's an invalid bug. Or if you have different opinion or think we're wrong, feel free to correct us. :)
  Otherwise I think we don't have to multiply -1 here.

	Philipp von Weitershausen [:philikon] Reporter
	Comment 25 • 12 years ago

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #24)
> I have discussed Bug 775028 with hsinyi and vicamo, we all think it's an
> invalid bug. Or if you have different opinion or think we're wrong, feel
> free to correct us. :)

I agree.

> Otherwise I think we don't have to multiply -1 here.

Agreed. This was implied from having bug 775028 be resolved as invalid, but I guess I should have pointed that out explicitly. Thanks for catching that, Yoshi! frsela, please address this too.

	Fernando R. Sela (no CC, needinfo please) [:frsela] Assignee
	Comment 26 • 12 years ago

Hi Philipp, I didn't catch you or IRC, so I ask you here ;)

I'm testing it again.

1.- If I try to use REQUEST_LAST_DATA_CALL_FAIL_CAUSE I always got 0xffff - since, this method is DEPRECATED we can't use it (the doc. says: "Deprecated use the status field in RIL_DATA_CALL_RESPONSE_V6")

2.- The response I receive on SETUP_DATA_CALL should be:
    "response" is an array of RIL_Data_Call_Response_v6

My problem is: In SETUP_DATA_CALL I receive, as you know, this information:

{"radioTech":1,"apn":"blablabla","user":"blablabla","passwd":"blablabla","chappap":3,"pdptype":"IP","rilRequestType":27,"rilRequestError":2}

I can't read more from the buffer since it's complains because is completely read ...

So, I don't have any response array or I don't know how to recover it. Any ideas?

	Philipp von Weitershausen [:philikon] Reporter
	Comment 27 • 12 years ago

(In reply to Fernando R. Sela from comment #26)
> 1.- If I try to use REQUEST_LAST_DATA_CALL_FAIL_CAUSE I always got 0xffff -
> since, this method is DEPRECATED we can't use it (the doc. says: "Deprecated
> use the status field in RIL_DATA_CALL_RESPONSE_V6")

Ah. Good to know.

> 2.- The response I receive on SETUP_DATA_CALL should be:
>     "response" is an array of RIL_Data_Call_Response_v6
> 
> My problem is: In SETUP_DATA_CALL I receive, as you know, this information:
> 
> {"radioTech":1,"apn":"blablabla","user":"blablabla","passwd":"blablabla",
> "chappap":3,"pdptype":"IP","rilRequestType":27,"rilRequestError":2}
> 
> I can't read more from the buffer since it's complains because is completely
> read ...
> 
> So, I don't have any response array or I don't know how to recover it. Any
> ideas?

We already read all of RIL_Data_Call_Response_v6 in the REQUEST_SETUP_DATA_CALL handler by deferring to handler for REQUEST_DATA_CALL_LIST (because the two responses are the same) [1]. 'status' is read as part of that [2].

The data call list then gets processed by RIL._processDataCallList(). As part of that we compare changes and figure out which calls are new, etc. Here you should look at `status` and notify the error.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#3217
[2] https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#3396

	Fernando R. Sela (no CC, needinfo please) [:frsela] Assignee
	Comment 28 • 12 years ago

(In reply to Philipp von Weitershausen [:philikon] from comment #27)
> (In reply to Fernando R. Sela from comment #26)
> 
> > 2.- The response I receive on SETUP_DATA_CALL should be:
> >     "response" is an array of RIL_Data_Call_Response_v6
> > 
> > My problem is: In SETUP_DATA_CALL I receive, as you know, this information:
> > 
> > {"radioTech":1,"apn":"blablabla","user":"blablabla","passwd":"blablabla",
> > "chappap":3,"pdptype":"IP","rilRequestType":27,"rilRequestError":2}
> > 
> > I can't read more from the buffer since it's complains because is completely
> > read ...
> > 
> > So, I don't have any response array or I don't know how to recover it. Any
> > ideas?
> 
> We already read all of RIL_Data_Call_Response_v6 in the
> REQUEST_SETUP_DATA_CALL handler by deferring to handler for
> REQUEST_DATA_CALL_LIST (because the two responses are the same) [1].
> 'status' is read as part of that [2].
> 
> The data call list then gets processed by RIL._processDataCallList(). As
> part of that we compare changes and figure out which calls are new, etc.
> Here you should look at `status` and notify the error.
> 
> [1]
> https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.
> js#3217
> [2]
> https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.
> js#3396

 * If I fire REQUEST_DATA_CALL_LIST parcel I receive a non-error response (logically since there are different queries)

 * If I call it directly or through RIL._processDataCallList is not processed since rilRequestError produces an early return in this methods.

	Andreas Gal :gal
	Comment 29 • 12 years ago

How are we doing here?

	Philipp von Weitershausen [:philikon] Reporter
	Comment 30 • 12 years ago

(In reply to Fernando R. Sela from comment #28)
>  * If I call it directly or through RIL._processDataCallList is not
> processed since rilRequestError produces an early return in this methods.

So, even if we get rilRequestError = 2, the parcel still contains data? That's surprising. Can you please show a logcat excerpt with the parcel?

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 31 • 12 years ago

Comment on attachment 643312 [details] [diff] [review]
Patch to notify connection errors - V4 (Part 1)

Oh, sorry, didn't realize you re-requested review here. My first r+ was sufficient, but here's a second!

	Fernando R. Sela (no CC, needinfo please) [:frsela] Assignee
	Comment 32 • 12 years ago

(In reply to Philipp von Weitershausen [:philikon] from comment #30)
> (In reply to Fernando R. Sela from comment #28)
> >  * If I call it directly or through RIL._processDataCallList is not
> > processed since rilRequestError produces an early return in this methods.
> 
> So, even if we get rilRequestError = 2, the parcel still contains data?
> That's surprising. Can you please show a logcat excerpt with the parcel?

No, It hasn't more data (as expected) I tried to explain that since the parcel hasn't more data, processDataCallList (as suggested before) returns couse the error ...

So a new call to the RIL should be done to recover the error status? Which one?

Sorry for the mis-understanding.

	Philipp von Weitershausen [:philikon] Reporter
	Comment 33 • 12 years ago

(In reply to Fernando R. Sela from comment #32)
> So a new call to the RIL should be done to recover the error status? Which
> one?

Which error status are you talking about? If REQUEST_SETUP_DATA_CALL fails, the data call will not have been set up, so a subsequent REQUEST_DATA_CALL_LIST shouldn't show it.

	Fernando R. Sela (no CC, needinfo please) [:frsela] Assignee
	Comment 34 • 12 years ago

(In reply to Philipp von Weitershausen [:philikon] from comment #33)
> (In reply to Fernando R. Sela from comment #32)
> > So a new call to the RIL should be done to recover the error status? Which
> > one?
> 
> Which error status are you talking about? If REQUEST_SETUP_DATA_CALL fails,
> the data call will not have been set up, so a subsequent
> REQUEST_DATA_CALL_LIST shouldn't show it.

I agree subsequent REQUEST_DATA_CALL_LIST shouldn't show it.

--

The value I want to recover is documented on:
  https://github.com/mozilla-b2g/android-hardware-ril/blob/master/include/telephony/ril.h#L1494

As you can se in line 1533 the response is a a RIL_Data_Call_Response_v6 which is defined in:

  https://github.com/mozilla-b2g/android-hardware-ril/blob/master/include/telephony/ril.h#L200

the status attribute contains the error code (RIL_DataCallFailCause) which can be one of the following ones:

  https://github.com/mozilla-b2g/android-hardware-ril/blob/master/include/telephony/ril.h#L365

But, since I receive a GENERIC_ERROR on the REQUEST_SETUP_DATA_CALL and as discussed into the Bug #775028, no more data can be retrieved.

The alternative was REQUEST_LAST_DATA_CALL_FAIL_CAUSE but it's deprecated ...

So my question is how to get the correct error code when the APN connection fails.

	Dietrich Ayala (:dietrich)
	Comment 35 • 12 years ago

This is the last bug in b2g-3g metabug before we can close it out. What's left to do here?

	Philipp von Weitershausen [:philikon] Reporter
	Comment 36 • 12 years ago

(In reply to Fernando R. Sela from comment #34)
> So my question is how to get the correct error code when the APN connection
> fails.

I don't know. That's for you to find out, really. Have you looked at how Android does it? In any case, if the best that we can do is just notify a generic error event, then let's do that.

	Fernando R. Sela (no CC, needinfo please) [:frsela] Assignee
	Comment 37 • 12 years ago

(In reply to Dietrich Ayala (:dietrich) from comment #35)
> This is the last bug in b2g-3g metabug before we can close it out. What's
> left to do here?

Hi Dietrich,

The patch is working but only returns generic errors because the method suggested to recover the kind of error is deprecated on Android.

I'm studding other ways to do this but unsuccessful :(

Meanwhile, as Philipp suggest on comment #36, we can close it with the generic error response and as soon as I found a better solution we can reopen the bug. WDYT?

	Fernando R. Sela (no CC, needinfo please) [:frsela] Assignee
	Comment 38 • 12 years ago

(In reply to Philipp von Weitershausen [:philikon] from comment #36)
> (In reply to Fernando R. Sela from comment #34)
> > So my question is how to get the correct error code when the APN connection
> > fails.
> 
> I don't know. That's for you to find out, really. Have you looked at how
> Android does it? In any case, if the best that we can do is just notify a
> generic error event, then let's do that.

Yes, Android on responseSetupDataCall calls to responseDataCallList and on responseDataCallList it maps to the RIL_Data_Call_Response_v6 struct.

But, as told in previous comments, this didn't worked to me, anyway I'll review my code again in order to check if there is some issue in my working-patch.

Meanwhile, I can rebase the patch and return generic_error... You agree?

	Fernando Jiménez Moreno [:ferjm]
	Comment 39 • 12 years ago

(In reply to Fernando R. Sela from comment #37)
> The patch is working but only returns generic errors because the method
> suggested to recover the kind of error is deprecated on Android.
> 
> I'm studding other ways to do this but unsuccessful :(
> 
> Meanwhile, as Philipp suggest on comment #36, we can close it with the
> generic error response and as soon as I found a better solution we can
> reopen the bug. WDYT?

You can close this bug as FIXED and create a follow-up bug to notify about specific errors.

	Fernando R. Sela (no CC, needinfo please) [:frsela] Assignee
	Comment 40 • 12 years ago

Good news :)

With the last rebase, no I receive the status value with the error code. I don't known what's changed in gecko or gonk but now it works really well.

I'll upload the complete patch as soon as I finish some tests.

FYI: Now I got the error cause on REQUEST_DATA_CALL_LIST instead on REQUEST_SETUP_DATA_CALL since rilRequestError always returns 0 (so REQUEST_DATA_CALL_LIST has a correct value ;)

	Fernando R. Sela (no CC, needinfo please) [:frsela] Assignee
	Comment 41 • 12 years ago

Rebased. Changed nsnull to nullptr

	Fernando R. Sela (no CC, needinfo please) [:frsela] Assignee
	Comment 42 • 12 years ago

Rebased.
Solved GENERIC_ERROR ISSUE (@see comment #39)

	Philipp von Weitershausen [:philikon] Reporter
	Comment 43 • 12 years ago

Comment on attachment 655277 [details] [diff] [review]
Patch to notify connection errors - V5 (Part 2)

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

::: dom/network/src/MobileConnection.cpp
@@ +139,5 @@
>      NS_ENSURE_SUCCESS(rv, rv);
> +    return NS_OK;
> +  }
> +
> +  if(!strcmp(aTopic, kDataErrorTopic)) {

Nit: space between `if` and parenthesis.

::: dom/system/gonk/RILContentHelper.js
@@ +762,5 @@
> +  },
> +
> +  _deliverDataCallback: function _deliverDataCallback(name, args) {
> +    debug("callback handler for " + name + " args: " + args);
> +    Services.obs.notifyObservers(null, kDataErrorTopic, args);

You did not address my comment 23.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +595,5 @@
> +   */
> +  handleDataCallError: function handleDataCallError(message) {
> +    // 3G Network revoked the data connection, possible unavailable APN
> +    debug("Received data registration error message. Failed APN " +
> +          gSettingsService.getLock().get("ril.data.apn", this) +

Um, `gSettingsService.getLock().get("ril.data.apn", this)` is asynchronous. This is not going to work. You want `this.dataCallSettings["apn"]`, as it was mentioned in the line you removed.

@@ +596,5 @@
> +  handleDataCallError: function handleDataCallError(message) {
> +    // 3G Network revoked the data connection, possible unavailable APN
> +    debug("Received data registration error message. Failed APN " +
> +          gSettingsService.getLock().get("ril.data.apn", this) +
> +          " # " + JSON.stringify(message)

This should not be necessary. There are already places where we log messages. Please remove this.

@@ +600,5 @@
> +          " # " + JSON.stringify(message)
> +         );
> +    RILNetworkInterface.reset();
> +    // Notify datacall error
> +    ppmm.sendAsyncMessage("RIL:DataError", message);

With the code that you have in ril_worker, this broadcasts to broadly. There can be multiple active data calls. We only want to notify errors for the data one. So you want to verify that `message.apn == this.dataCallSettings["apn"]` or something like this.

::: dom/system/gonk/ril_worker.js
@@ +2445,5 @@
>    },
>  
> +  _sendDataCallError: function _sendDataCallError(message, errorCode) {
> +    message.rilMessageType = "datacallerror";
> +    if(errorCode == ERROR_GENERIC_FAILURE) {

Space between `if` and parenthesis.

@@ +2450,5 @@
> +      message.error = RIL_ERROR_TO_GECKO_ERROR[errorCode];
> +    } else {
> +      message.error = RIL_DATACALL_FAILCAUSE_TO_GECKO_DATACALL_ERROR[errorCode];
> +    }
> +    debug("PDP Error: " + JSON.stringify(message));

All calls to `debug()` in ril_worker should be prefixed with `if DEBUG`, but I think this debug message isn't particularly helpful since `sendDOMMessage` already logs JSON message. Please remove.

@@ +2458,5 @@
>    _processDataCallList: function _processDataCallList(datacalls, newDataCallOptions) {
> +    // Check for possible PDP errors. We check earlier because the datacall
> +    //  can be removed if is the same as the current one.
> +    for each (let newDataCall in datacalls) {
> +      if(newDataCall.status) {  // On Data Call (PDP) error, we shall notify

if (newDataCall.status != DATACALL_FAIL_NONE)

	Philipp von Weitershausen [:philikon] Reporter
	Comment 45 • 12 years ago

Comment on attachment 656431 [details] [diff] [review]
Patch to notify connection errors - V6 (Part 2)

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

Nice work! Just some nits below.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +592,5 @@
> +  /**
> +   * Handle data errors
> +   */
> +  handleDataCallError: function handleDataCallError(message) {
> +    if(message.apn != this.dataCallSettings["apn"]) {

Nit: space between `if` and parenthesis. I don't really like having to repeat that over and over. Please familiarize yourself with the Mozilla Coding Style and the style present in this file.

::: dom/system/gonk/ril_worker.js
@@ +2457,3 @@
>    _processDataCallList: function _processDataCallList(datacalls, newDataCallOptions) {
> +    // Check for possible PDP errors. We check earlier because the datacall
> +    //  can be removed if is the same as the current one.

Nit: leading whitespace.

@@ +2457,5 @@
>    _processDataCallList: function _processDataCallList(datacalls, newDataCallOptions) {
> +    // Check for possible PDP errors. We check earlier because the datacall
> +    //  can be removed if is the same as the current one.
> +    for each (let newDataCall in datacalls) {
> +      // On Data Call (PDP) error, we shall notify

Not sure what this comment is trying to accomplish... Remove?

	Philipp von Weitershausen [:philikon] Reporter
	Comment 47 • 12 years ago

Comment on attachment 655275 [details] [diff] [review]
Patch to notify connection errors - V5 (Part 1)

This patch no longer applies cleanly. I imagine bug 687332 and other things that landed in the mean time have to do with it. Please rebase and upload a new patch.

	Philipp von Weitershausen [:philikon] Reporter
	Comment 48 • 12 years ago

Comment on attachment 657221 [details] [diff] [review]
Patch to notify connection errors - V7 (Part 2)

This patch also no longer applies cleanly. Furthermore, bug 687332 changed some event listener mechanics. Please update the patch. Thanks!

	Fernando R. Sela (no CC, needinfo please) [:frsela] Assignee
	Comment 49 • 12 years ago

Rebased for checkin in m-c

	Fernando R. Sela (no CC, needinfo please) [:frsela] Assignee
	Comment 50 • 12 years ago

Rebased for checkin. Also updated with bug 687332 changes

	Philipp von Weitershausen [:philikon] Reporter
	Comment 51 • 12 years ago

Comment on attachment 658022 [details] [diff] [review]
Patch to notify connection errors - V7.1 (Part 2)

Sorry, still doesn't apply cleanly.

	Fernando R. Sela (no CC, needinfo please) [:frsela] Assignee
	Comment 52 • 12 years ago

Rebased again :p - hope this apply well ;)

	Philipp von Weitershausen [:philikon] Reporter
	Comment 54 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/1566e8499cb8
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcc61274ec43

	Ryan VanderMeulen [:RyanVM]
	Comment 55 • 12 years ago

This had to be backed out due to mochitest failures on all platforms. Please run this through Try before attempting to re-land.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5289c1f03c0

https://tbpl.mozilla.org/php/getParsedLog.php?id=15056321&tree=Mozilla-Inbound

2350 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_interfaces.html | Unexpected interface name in global scope: DataErrorEvent

	Fernando R. Sela (no CC, needinfo please) [:frsela] Assignee
	Comment 56 • 12 years ago

Rebased

	Fernando R. Sela (no CC, needinfo please) [:frsela] Assignee
	Comment 57 • 12 years ago

Fixed Mochitest to pass the test_interfaces test with the new DataErrorEvent

	Fernando R. Sela (no CC, needinfo please) [:frsela] Assignee
	Comment 58 • 12 years ago

Rebased

	Philipp von Weitershausen [:philikon] Reporter
	Comment 59 • 12 years ago

Thanks, Fernando. Can you please get a try run for these patches so that we're sure we're not breaking the tree again? Thanks!

	Vicamo Yang [:vicamo][:vyang]
	Comment 60 • 12 years ago

rebase only

	Vicamo Yang [:vicamo][:vyang]
	Comment 61 • 12 years ago

https://tbpl.mozilla.org/?tree=Try&rev=7c7390c97ea0
Patches in bug 782513 are based on this.

	Shian-Yow Wu [:swu]
	Comment 62 • 12 years ago

Thanks for your help, Vicamo.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 63 • 12 years ago

Comment on attachment 659666 [details] [diff] [review]
Patch to notify connection errors - V7.3 (Part 1)

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

This should use the codegen.

	Fernando R. Sela (no CC, needinfo please) [:frsela] Assignee
	Comment 64 • 12 years ago

(In reply to Philipp von Weitershausen [:philikon] from comment #59)
> Thanks, Fernando. Can you please get a try run for these patches so that
> we're sure we're not breaking the tree again? Thanks!

Hi Philipp,

Sorry for the delay, The last week I had been with other issues.

I re-tested it again with Mochitest and passed it succesfully.

I would like to ask you for first-level access to the try servers in order to be able to test on all platforms in the future.

Regards

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 65 • 12 years ago

(In reply to Fernando R. Sela from comment #64)
> Hi Philipp,
> 
> Sorry for the delay, The last week I had been with other issues.
> 
> I re-tested it again with Mochitest and passed it succesfully.
> 
> I would like to ask you for first-level access to the try servers in order
> to be able to test on all platforms in the future.
> 
> Regards

Hi, frsela
I could vouch for you,
Can you file a bug and CC me ?

Thanks

	Andrew Overholt [:overholt]
	Comment 66 • 12 years ago

Based on checkin? I'm putting a "small" (< 1 week) estimate on this bug.

	Philipp von Weitershausen [:philikon] Reporter
	Comment 67 • 12 years ago

Fernando, how is that try build coming along? I tried to kick one off, but the patches no longer apply. Please keep these patches up-to-date so that they can be pushed.

	Philipp von Weitershausen [:philikon] Reporter
	Comment 68 • 12 years ago

(In reply to :Ms2ger from comment #63)
> This should use the codegen.

This is really close to landing. Can we file a follow-up bug for this, please? frsela, Ms2ger?

	Fernando R. Sela (no CC, needinfo please) [:frsela] Assignee
	Comment 69 • 12 years ago

Rebased

	Fernando R. Sela (no CC, needinfo please) [:frsela] Assignee
	Comment 70 • 12 years ago

Rebased

	Fernando R. Sela (no CC, needinfo please) [:frsela] Assignee
	Comment 71 • 12 years ago

Rebased

	Fernando R. Sela (no CC, needinfo please) [:frsela] Assignee
	Comment 72 • 12 years ago

(In reply to Philipp von Weitershausen [:philikon] from comment #68)
> (In reply to :Ms2ger from comment #63)
> > This should use the codegen.
> 
> This is really close to landing. Can we file a follow-up bug for this,
> please? frsela, Ms2ger?

I also prefer a follow-up bug since we should land this asap.

Meanwhile, what do you mean with "use the codegen"? which codegen?

	Fernando R. Sela (no CC, needinfo please) [:frsela] Assignee
	Comment 73 • 12 years ago

Just rebased to the "Sun Sep 23 21:26:39 2012 -0400" version, last one I fetched this morning.

	Philipp von Weitershausen [:philikon] Reporter
	Comment 74 • 12 years ago

Folded part 1 and 2 into one cset since they don't make sense without each other:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f19c6f099227
https://hg.mozilla.org/integration/mozilla-inbound/rev/95354c5e25cc

	Mounir Lamouri (:mounir)
	Comment 75 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/f19c6f099227
https://hg.mozilla.org/mozilla-central/rev/95354c5e25cc

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 76 • 12 years ago

(In reply to Fernando R. Sela from comment #72)
> (In reply to Philipp von Weitershausen [:philikon] from comment #68)
> > (In reply to :Ms2ger from comment #63)
> > > This should use the codegen.
> > 
> > This is really close to landing. Can we file a follow-up bug for this,
> > please? frsela, Ms2ger?
> 
> I also prefer a follow-up bug since we should land this asap.
> 
> Meanwhile, what do you mean with "use the codegen"? which codegen?

http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/event_impl_gen.conf.in