Easy Unlock Show connecting user pod icon when waking up from sleep

Easy Unlock Show connecting user pod icon when waking up from sleep

On suspend, Easy Unlock app gets disabled. When waking up, it takes some
time for the app to reload ( a second or two). During theat time there
is no Easy Unlock icon in the user pod. To improve user experience,
show connecting icon during that time.

BUG=421970
TEST=With Easy Unlock enabled, lock the screen and close the lid (so the
device suspends). When opening the lid spinner icon should be visible
immediately on the user pod.

Committed: https://crrev.com/305cb9469526f147a41bf896ae2a181e0e49cdff
Cr-Commit-Position: refs/heads/master@{#299940}

[tbarzic, 2014-10-10 01:12:54]

tbarzic@chromium.org changed reviewers:
+ tengs@chromium.org, xiyuan@chromium.org

[tbarzic, 2014-10-10 01:12:54]

https://codereview.chromium.org/644873002/diff/1/chrome/browser/signin/easy_u...
File chrome/browser/signin/easy_unlock_service.cc (right):

https://codereview.chromium.org/644873002/diff/1/chrome/browser/signin/easy_u...
chrome/browser/signin/easy_unlock_service.cc:464: bluetooth_detector_.reset();
Let me know if you don't like this and I'll try to find a better solution.
The problem is that bluetooth_detector_->IsPresent returns false immediately
after suspend is done. This would cause DisableAppIfLoaded to be called, and
easy-unlock icon to be removed from the screenlock UI.

[Tim Song, 2014-10-10 06:46:14]

https://codereview.chromium.org/644873002/diff/1/chrome/browser/signin/easy_u...
File chrome/browser/signin/easy_unlock_service.cc (right):

https://codereview.chromium.org/644873002/diff/1/chrome/browser/signin/easy_u...
chrome/browser/signin/easy_unlock_service.cc:235: if (bluetooth_detector_.get()
&& !bluetooth_detector_->IsPresent()) {
Comment that bluetooth_detector_ is NULL if we successfully found an adapter
already, or add a new variable to track this state.

https://codereview.chromium.org/644873002/diff/1/chrome/browser/signin/easy_u...
chrome/browser/signin/easy_unlock_service.cc:464: bluetooth_detector_.reset();
On 2014/10/10 01:12:54, tbarzic wrote:
> Let me know if you don't like this and I'll try to find a better solution.
> The problem is that bluetooth_detector_->IsPresent returns false immediately
> after suspend is done. This would cause DisableAppIfLoaded to be called, and
> easy-unlock icon to be removed from the screenlock UI.

I'm fine with this, and improving the start-up time is always a good thing.

I would put this reset() down in OnBluetoothAdapterPresentChanged(), so the
intent is more clear.

[xiyuan, 2014-10-10 15:45:53]

https://codereview.chromium.org/644873002/diff/1/chrome/browser/signin/easy_u...
File chrome/browser/signin/easy_unlock_service.cc (right):

https://codereview.chromium.org/644873002/diff/1/chrome/browser/signin/easy_u...
chrome/browser/signin/easy_unlock_service.cc:464: bluetooth_detector_.reset();
On 2014/10/10 06:46:14, Tim Song wrote:
> On 2014/10/10 01:12:54, tbarzic wrote:
> > Let me know if you don't like this and I'll try to find a better solution.
> > The problem is that bluetooth_detector_->IsPresent returns false immediately
> > after suspend is done. This would cause DisableAppIfLoaded to be called, and
> > easy-unlock icon to be removed from the screenlock UI.
> 
> I'm fine with this, and improving the start-up time is always a good thing.
> 
> I would put this reset() down in OnBluetoothAdapterPresentChanged(), so the
> intent is more clear.

The detector is kept around to support devices that uses usb bluetooth dongle
such as lumpy. The presence of BT could change dynamically.

Maybe we should ignore the OnBluetoothAdapterPresentChanged on suspending.

[tbarzic, 2014-10-10 17:59:43]

https://codereview.chromium.org/644873002/diff/1/chrome/browser/signin/easy_u...
File chrome/browser/signin/easy_unlock_service.cc (right):

https://codereview.chromium.org/644873002/diff/1/chrome/browser/signin/easy_u...
chrome/browser/signin/easy_unlock_service.cc:464: bluetooth_detector_.reset();
On 2014/10/10 15:45:53, xiyuan wrote:
> On 2014/10/10 06:46:14, Tim Song wrote:
> > On 2014/10/10 01:12:54, tbarzic wrote:
> > > Let me know if you don't like this and I'll try to find a better solution.
> > > The problem is that bluetooth_detector_->IsPresent returns false
immediately
> > > after suspend is done. This would cause DisableAppIfLoaded to be called,
and
> > > easy-unlock icon to be removed from the screenlock UI.
> > 
> > I'm fine with this, and improving the start-up time is always a good thing.
> > 
> > I would put this reset() down in OnBluetoothAdapterPresentChanged(), so the
> > intent is more clear.
> 
> The detector is kept around to support devices that uses usb bluetooth dongle
> such as lumpy. The presence of BT could change dynamically.
> 
> Maybe we should ignore the OnBluetoothAdapterPresentChanged on suspending.

Yeah, I though there may be a reason for keeping detector around I was missing.
Alternative approach I was thinking about was something in line with your
suggestion (though, we get OnSuspendDone before the adaptor gets reinitialized,
so we may want to ignore the detector state for some time after waking up).. let
me think about it a bit more.

[tbarzic, 2014-10-15 18:19:19]

ptal

[Tim Song, 2014-10-16 00:43:09]

https://codereview.chromium.org/644873002/diff/240001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service.cc (right):

https://codereview.chromium.org/644873002/diff/240001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service.cc:447: if (reset_screenlock_state)
I think it makes more sense now to get rid of the reset_screenlock_state
argument and just move ResetScreenlockState() down to UpdateAppState().

https://codereview.chromium.org/644873002/diff/240001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service.cc:607: if (screenlock_state_handler_
&& screenlock_state_handler_->IsActive())
put braces around this check

[tbarzic, 2014-10-16 01:24:50]

https://codereview.chromium.org/644873002/diff/240001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service.cc (right):

https://codereview.chromium.org/644873002/diff/240001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service.cc:447: if (reset_screenlock_state)
On 2014/10/16 00:43:09, Tim Song wrote:
> I think it makes more sense now to get rid of the reset_screenlock_state
> argument and just move ResetScreenlockState() down to UpdateAppState().

Done.

https://codereview.chromium.org/644873002/diff/240001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service.cc:607: if (screenlock_state_handler_
&& screenlock_state_handler_->IsActive())
On 2014/10/16 00:43:09, Tim Song wrote:
> put braces around this check

Done.

[Tim Song, 2014-10-16 01:32:01]

lgtm

[xiyuan, 2014-10-16 02:39:06]

lgtm

https://codereview.chromium.org/644873002/diff/320001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service.cc (right):

https://codereview.chromium.org/644873002/diff/320001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service.cc:480: power_monitor_.reset(new
PowerMonitor(this));
nit: line 479-480 needs 2 more space indent

[tbarzic, 2014-10-16 17:30:22]

https://codereview.chromium.org/644873002/diff/320001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service.cc (right):

https://codereview.chromium.org/644873002/diff/320001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service.cc:480: power_monitor_.reset(new
PowerMonitor(this));
On 2014/10/16 02:39:06, xiyuan wrote:
> nit: line 479-480 needs 2 more space indent

Done.

[tbarzic, 2014-10-16 17:41:32]

The CQ bit was checked by tbarzic@chromium.org

[commit-bot: I haz the power, 2014-10-16 17:43:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/644873002/360001

[commit-bot: I haz the power, 2014-10-16 18:33:45]

Committed patchset #13 (id:360001)

[commit-bot: I haz the power, 2014-10-16 18:35:14]

Patchset 13 (id:??) landed as
https://crrev.com/305cb9469526f147a41bf896ae2a181e0e49cdff
Cr-Commit-Position: refs/heads/master@{#299940}