Closed Bug 884274 Opened 8 years ago Closed 8 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: julienw, Assigned: arthurcc)

References

Details

Attachments

(3 files)

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.
Attached patch patch v1Splinter Review
That patch doesn't work for me yet, I don't exactly know why. Please steal it if you can find what's missing.
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.
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.
That's basically what my patch does, but in adhoc places. Feel free to take it :)
(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;
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...
Propose to add a helper in /shared for bridging icc related operations. Any comments?
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.


>  };
>})();
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.
Depends on: 859220
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;
Blocks: 874769
Blocks: 877978
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!
Attachment #765805 - Flags: review?(kaze)
Attachment #765805 - Flags: review?(fernando.campo)
Attachment #765805 - Flags: review?(felash)
Attachment #765805 - Flags: review?(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.
Attachment #765805 - Flags: review?(alive) → review+
(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 !
Attachment #765805 - Flags: review?(felash)
Comment on attachment 765805 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/10524

All good for FTU too, thanks!
Attachment #765805 - Flags: review?(fernando.campo) → review+
Comment on attachment 765805 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/10524

LGTM, please address my nitpicks (see PR) before merging.
Attachment #765805 - Flags: review?(kaze) → review+
Thanks for reviewing, everyone!

master: https://github.com/mozilla-b2g/gaia/commit/a3620dc188e68d365dcb65602e7408039798e249
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Assignee: nobody → arthur.chen
You need to log in before you can comment on or make changes to this bug.