884274 - Make the pin unlock code work on both mozilla central and b2g18

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

In Bug #861081 we broke the compatibility between gaia master and gecko b2g18.

Even if it's not officially supported, a lot of us are using this setup to be able to work.

Comment 1

[Julien Wajsberg [:julienw]]

Attachment: patch v1

That patch doesn't work for me yet, I don't exactly know why. Please steal it if you can find what's missing.

Comment 2

[Julien Wajsberg [:julienw]]

very strangely, this breaks at "this.pinInput.focus()" in the system's simcard_dialog.js' handleCardState. I've seen this using logs before and after this line, because there are no other symptom except nothing happens after this line (eg: the title is not set).

But this works correctly if I simply revert the patch in Bug 861081, so something smelly is happening that I don't quite understand.

Comment 3

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Hi Julien,

Thanks for pointing this out.

In a offline discussion with Arthur and Edgar, I think it might be worth to add a proxy (helper) object in Gaia to abstract the difference between navigator.mozIccManager and navigator.mozMobileConnection.

This proxy will do feature detection and decides which object to use to get the card lock related status.

Comment 4

[Julien Wajsberg [:julienw]]

That's basically what my patch does, but in adhoc places. Feel free to take it :)

Comment 5

[Fernando Campo (:fcampo)]

(In reply to Julien Wajsberg [:julienw] from comment #1)
> Created attachment 764102 [details] [diff] [review]
> patch v1
> 
> That patch doesn't work for me yet, I don't exactly know why. Please steal
> it if you can find what's missing.

Maybe the error is on the early returns??

line 34 of security_privacy.js
line 54 of simcard_dialog.js
line 329 simcard_dialog.js
if (!icc)
  return;

Comment 6

[Julien Wajsberg [:julienw]]

Well, I checked that this object is supposed to exist on b2g18.

And with logs, I can clearly see it stops at this focus line. But since I don't understand why, then it could be anything triggering this side-effect...

Comment 7

[Arthur Chen [:arthurcc][inactive after 6/30]]

Attachment: icc helper proposal

Propose to add a helper in /shared for bridging icc related operations. Any comments?

Comment 8

[Julien Wajsberg [:julienw]]

Comment on attachment 764687 [details] [diff] [review]
icc helper proposal

>'use strict';
>
>var IccHelper = (function() {
>  var mobileConn = navigator.mozMobileConnection;
>  var iccManager = navigator.mozICCManager;
>
>  var actor;
>  if (mobileConn.setCardLock !== undefined) {
>    actor['cardLock'] = mobileConn;
>  } else if (iccManager.setCardLock !== undefined) {
>    actor['cardLock'] = iccManager;
>  }
>
>  if (mobileConn.cardState !== undefined) {
>    actor['cardState'] = mobileConn;
>  } else if (iccManager.setCardLock !== undefined) {
>    actor['cardState'] = iccManager;
>  }
>
>  if (mobileConn.iccInfo !== undefined) {
>    actor['iccInfo'] = mobileConn;
>  } else if (iccManager.setCardLock !== undefined) {
>    actor['iccInfo'] = iccManager;
>  }


you can probably make this more abstract, starting with a ['setCardLock', 'cardState', 'iccInfo'] array. Note that all your else statements are wrong, so that would help not making these mistakes too ;)

Also, I think that you should use eg |if ("cardState" in mobileConn)|, otherwise I'm afraid that getting the value can bring performance problems.


>    
>  return {
>    addEventListener: function() {
>      var eventName = arguments[0];
>      switch(eventName) {
>        case 'cardstatechange':
>          var actor = actor['cardState']
>          return actor.addEventListener.call(actor, arguments);
>        case 'iccinfochange':
>          var actor = actor['iccInfo']
>          return actor.addEventListener.call(actor, arguments);
>        case 'onicccardlockerror':
>          var actor = actor['cardLock']
>          return actor.addEventListener.call(actor, arguments);
>      }
>    },


you can do the addEventListener once after the switch.
also, you want |apply| instead of |call|.


>
>    setCardLock: function() {
>      var actor = actor['cardLock'];
>      return actor.setCardLock(actor, arguments);
>    },
>
>    unlockCardLock: function() {
>      var actor = actor['cardLock'];
>      return actor.unlockCardLock(actor, arguments);
>    },
>
>    getCardLock: function() {
>      var actor = actor['cardLock'];
>      return actor.getCardLock(actor, arguments);
>    },
>
>    get cardState() {
>      var actor = actor['cardState'];
>      return actor.getCardLock(actor, arguments);
>    },
>
>    get iccInfo() {
>      var actor = actor['iccInfo'];
>      return actor.getCardLock(actor, arguments);
>    }


you forgot .apply in all these functions.


>  };
>})();

Comment 9

[Edgar Chen [:edgar]]

Oh, one more thing, the way to get iccManager instance is different between m-c and b2g18.
In b2g18, the iccManager is inside mobileConnection, and we split them in m-c.

- m-c: navigator.mozIccManager
- b2g18: navigator.mozMobileConnection.icc

Please see bug 859220.

Comment 10

[Edgar Chen [:edgar]]

Comment on attachment 764687 [details] [diff] [review]
icc helper proposal

>'use strict';
>
>var IccHelper = (function() {
>  var mobileConn = navigator.mozMobileConnection;
>  var iccManager = navigator.mozICCManager;

As I mention in comment #9, maybe we should use below way to get iccManager to make sure we are using the correct instance to check.
var iccManager = navigator.mozIccManager || navigator.mozMobileConnection.icc;

Comment 11

[Arthur Chen [:arthurcc][inactive after 6/30]]

Attachment: Link to https://github.com/mozilla-b2g/gaia/pull/10524

Julien, thanks for the comments! I simply wrote a proposal for feedbacks but didn't test it at all. :(

The patch is for the backward compatibility of gaia master on b2g18, which targets on the ICC related events/attributes changes in m-c. (bug 860585, bug 874744, bug 875721) This patch includes a IccHelper and fixes related to 860585 (which is already landed in m-c). Alive, Fernando, Kaze, please help review the change, thanks!

Comment 12

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 765805 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/10524

Some comments on github. I like the idea to wrap API in one object.

Comment 13

[Julien Wajsberg [:julienw]]

(In reply to Arthur Chen [:arthurcc] from comment #11)
> 
> Julien, thanks for the comments! I simply wrote a proposal for feedbacks but
> didn't test it at all. :(

no problem, I was happy to read it all and provide comments. I just tested that it works as expected on b2g18 ! I'll let Alive lead that review then :) thanks !

Comment 14

[Fernando Campo (:fcampo)]

Comment on attachment 765805 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/10524

All good for FTU too, thanks!

Comment 15

[Fabien Cazenave [:kaze]]

Comment on attachment 765805 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/10524

LGTM, please address my nitpicks (see PR) before merging.

Comment 16

[Arthur Chen [:arthurcc][inactive after 6/30]]

Thanks for reviewing, everyone!

master: https://github.com/mozilla-b2g/gaia/commit/a3620dc188e68d365dcb65602e7408039798e249