Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Reporter
	Description • 11 years ago

From nfc_manager I found the value of lockScreen.locked is false when boot up.

STR:
1. Ensure Screen lock is enabled.
2. Reboot phone

First 'screen-change' event will come, at this time, lockScreen.locked is false.
Second Settings got the value of nfc.enabled, at this time, lockScreen.locked is still false.

Then 'lock' event comes, at this time, lockScreen.locked is still false.

Which is strange here, because we already received the 'lock' event, however the value of lockScreen.locked hasn't been updated.

Wait for a moment, the screen will go off, 'screen-change' will come, now lockScreen.locked will be true now.

Then we unlock the screen, and press power key to lock the screen again,
now when the 'lock' event comes, now lockScreen.locked will be true.

So there is a different result here, I think the when the first 'lock' event is received by nfc_manager, the lockScreen.enabled hasn't been updated.

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 1 • 11 years ago

Yoshi,
Please clearly point out the user impact so that we can better judge the importance.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Reporter
	Comment 2 • 11 years ago

NFC is still enabled even when screen is locked.

	Ken Chang[:kenkai]
	Comment 3 • 11 years ago

Yoshi, in this cases, that power consumption will be high because NFC is not disabled. But for users, they don't see anything changing when this bug is happened. right?

	Bruce Huang [:bhuang] (inactive)
	Comment 4 • 11 years ago

Adding to backlog so we don't lose this item, but non-blocking for now.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Reporter
	Comment 5 • 11 years ago

After discussed with Greg, we should listen to 'lock' and 'unlock' event, and we shouldn't check lockScreen.enabled, I'll change the components back to NFC and fix it.

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 7 • 11 years ago

Comment on attachment 8411548 [details]
Pull Request

The main problem is you should use lockscreen.locked to replace lockscreen.enabled.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Reporter
	Comment 8 • 11 years ago

There's a typo in the first place, it should be 'lockScreen.locked', I mistyped it as lockScreen.enabled'.

Just discussed with Alive, he thinks LockScreen should still provide a way to query current lock status in case other object (like NfcManager) is too late to receive the 'lock' event.
So this is still a bug in LockScreen component.

Moving Summary and Components back and deassign myself.

	Greg Weng [:snowmantw][:gweng][:λ]
	Comment 9 • 11 years ago

I've solved this in my LockScreen as app patch. So if that patch can be landed in schedule we can eliminate both bugs at the same time.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 10 • 11 years ago

Greg, I am assign this to you so you could close this when that patch lands.

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 11 • 11 years ago

(In reply to Yoshi Huang[:allstars.chh] from comment #8)
> Just discussed with Alive, he thinks LockScreen should still provide a way
> to query current lock status in case other object (like NfcManager) is too
> late to receive the 'lock' event.
> So this is still a bug in LockScreen component.

The main concern is we always need an object to keep the state - no matter it's lockscreen.locked or system.locked or lockscreenWindowManager.locked - for the lately-initialized module to query, unless we WANT to init lockscreen at the end of system bootstrap process --- but this doesn't make sense. We will have more lazily-loaded modules in the future.

	Ken Chang[:kenkai]
	Comment 12 • 11 years ago

This bug blocks NFC 2.0 release.

	bhavana bajaj [:bajaj]
	Comment 13 • 10 years ago

Ken, can you please help understand why this is blocking ? To me I see this as a liveable use-case and may be a nice-to-fix but not a release blocking issue. We would need numbers here on how much the battery drain is to really block on the issue or any other information for this to make a blocking case.

	Ken Chang[:kenkai]
	Comment 14 • 10 years ago

(In reply to bhavana bajaj [:bajaj] from comment #13)
> Ken, can you please help understand why this is blocking ? To me I see this
> as a liveable use-case and may be a nice-to-fix but not a release blocking
> issue. We would need numbers here on how much the battery drain is to really
> block on the issue or any other information for this to make a blocking case.
In short, Even if devices are in lock screen mode, the lock screen will be shrank while 2 phones enabling NFC are approaching. It is very strange for user. I think this bug should be fixed before FC date.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 15 • 10 years ago

(In reply to Ken Chang[:ken] from comment #14)
> In short, Even if devices are in lock screen mode, the lock screen will be
> shrank while 2 phones enabling NFC are approaching. It is very strange for
> user. I think this bug should be fixed before FC date.

Triage: blocking based on scenario above. Also we feel that for 2.0 we should address this in NFCManager instead, maybe by checking the DOM className instead of locked property.

the lockScreen.locked can still be considered as a valid bug but not locking (and we should file another bug to track it).

	Ken Chang[:kenkai]
	Comment 16 • 10 years ago

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #15)
> (In reply to Ken Chang[:ken] from comment #14)
> ... we feel that for 2.0 we
> should address this in NFCManager instead, maybe by checking the DOM
> className instead of locked property.
I think this method is more clear and better for fixing this bug. Garner, what do you think?

	Garner Lee
	Comment 17 • 10 years ago

Since LockedScreen.locked is stale, maybe assume locked status via state var, until the actual lock status events update arrive, then update that status on an incoming tech event try to cover the race condition later on. I'll check.

	Garner Lee
	Comment 18 • 10 years ago

https://bugzilla.mozilla.org/show_bug.cgi?id=1017485#c5 (and c6):

----------------------------------------------------
 Ken Chang[:ken] 2014-05-30 00:19:33 PDT

NFC isn't allowed to p2p as well when screen is on and locked.
----------------------------------------------------

So I don't get something changed against spec, can someone re-link the full expected UX again (as the bug indicates, this isn't what's happening right now...it used to).

	Garner Lee
	Comment 19 • 10 years ago

BTW, my locked state seems correct whenever queried (this difference might be I have today's master gaia and gecko code as opposed to a month ago). I'm using adb reboot and gui reboot.

If this.hwState is the default off, after reset, P2P is still sent up, but Tag reading is off.

If you allow the NFC power config to happen anyway, it only sends discovery on and discovery off states, so sending anything at all enables both P2P and Tag reading.

I/GeckoDump(  177): [DEBUG] SYSTEM NFC: XXXXXXXXXXXX  handleEvent type: screenchange
I/GeckoDump(  177): [DEBUG] SYSTEM NFC: XXXXXXXXXXXXXXXXXXX isScreenUnlockAndEnabled:
I/GeckoDump(  177): [DEBUG] SYSTEM NFC: XXXXXXXXX                  lockScreen.locked: true
I/GeckoDump(  177): [DEBUG] SYSTEM NFC: XXXXXXXXX        ScreenManager.screenEnabled: true
I/GeckoDump(  177): [DEBUG] SYSTEM NFC: --->  true

It seems the nfcd has a bad power on default?

	Dimi Lee [:dimi]
	Comment 20 • 10 years ago

This will be fixed in bug 1017485. The is a follow up bug after supporting card emulation mode when screen off or screen on, locked.

	Ken Chang[:kenkai]
	Comment 21 • 10 years ago

(In reply to Garner Lee from comment #18)
> https://bugzilla.mozilla.org/show_bug.cgi?id=1017485#c5 (and c6):
> 
> ----------------------------------------------------
>  Ken Chang[:ken] 2014-05-30 00:19:33 PDT
> 
> NFC isn't allowed to p2p as well when screen is on and locked.
> ----------------------------------------------------
> 
> So I don't get something changed against spec, can someone re-link the full
> expected UX again (as the bug indicates, this isn't what's happening right
> now...it used to).
I don't think we have a specific UX for this and can not find anything related to this from user stories.
ni? Juwei.

	Juwei Huang[:juwei]
	Comment 22 • 10 years ago

NO we are not allow to p2p when on lockscreen.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 23 • 10 years ago

I figured out the problem: the 'lock' event was dispatched before the object is properly assign to window.lockScreen property.

This shouldn't be hard unless we have some quirky issue.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 25 • 10 years ago

Comment on attachment 8436853 [details]
mozilla-b2g:master PR#20223

Greg, could you rubberstamp this trivial patch we discussed offline? I manually test this on Frame and lock screen still works.

John, in case Greg have not review this tonight and OOO tomorrow, could you review this?

	John Lu [:mnjul][Please NI or I'll miss]
	Comment 26 • 10 years ago

Comment on attachment 8436853 [details]
mozilla-b2g:master PR#20223

Yep, patch looks good and tests have been modified accordingly (Y)

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 27 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 10 years ago

v2.0: https://github.com/mozilla-b2g/gaia/commit/7f671b6facef20e54846c0bffcbb5ee297c6175a