No Easy unlock if bluetooth is not available.

No Easy unlock if bluetooth is not available.

BUG=399067
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290081

[Ilya Sherman, 2014-08-14 07:29:42]

https://codereview.chromium.org/472733002/diff/20001/chrome/browser/signin/ea...
File chrome/browser/signin/easy_unlock_service.cc (right):

https://codereview.chromium.org/472733002/diff/20001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service.cc:54: : service_(service),
weak_ptr_factory_(this) {}
nit: One per line, please.

https://codereview.chromium.org/472733002/diff/20001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service.cc:77: ReportAdapterPresent();
If the adapter was removed from the system, should |adapter_| be reset?  Is it
possible that a *different* adapter will be added to the system?  (It seems like
it ought to be possible, e.g. if I plug in an adapter via USB.)

https://codereview.chromium.org/472733002/diff/20001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service.cc:91: EasyUnlockService* service_;
nit: Please document lifetime expectations.

https://codereview.chromium.org/472733002/diff/20001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service.cc:152: // It is only disabled when
the trial exists and is in "Disable" group.
nit: Please update this comment.

https://codereview.chromium.org/472733002/diff/20001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service.cc:159: return true;
Hmm, do we not have any command-line switches associated with the feature
anymore?  Typically, the best practice for code that is controlled by a Finch
trial is to also allow manual control via chrome://flags.

https://codereview.chromium.org/472733002/diff/20001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service.cc:205: void
EasyUnlockService::CheckIsAllowed() {
nit: Perhaps name this something more like "UpdateAppState()"?

https://codereview.chromium.org/472733002/diff/20001/chrome/browser/signin/ea...
File chrome/browser/signin/easy_unlock_service.h (right):

https://codereview.chromium.org/472733002/diff/20001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service.h:41: class BluetoothDetector;
Do we really need to implement this code here, or is there perhaps already
similar code elsewhere?  According to the bug report, we already hide some
settings if there is no Bluetooth adapter present...

https://codereview.chromium.org/472733002/diff/20001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service.h:48: void
OnBluetoothAdapterPresentChanged();
nit: Please document at least the new methods.

https://codereview.chromium.org/472733002/diff/20001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service.h:52: scoped_ptr<BluetoothDetector>
bluetooth_detector_;
Hmm, is this a scoped_ptr just so that you can keep the BluetoothDetector
definition out of the header file?  I'm not sure that's needed; but either way
is fine I guess.

[xiyuan, 2014-08-14 20:27:44]

https://codereview.chromium.org/472733002/diff/20001/chrome/browser/signin/ea...
File chrome/browser/signin/easy_unlock_service.cc (right):

https://codereview.chromium.org/472733002/diff/20001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service.cc:54: : service_(service),
weak_ptr_factory_(this) {}
On 2014/08/14 07:29:41, Ilya Sherman wrote:
> nit: One per line, please.

Done. I personally like one-per-line style but "git cl format" thinks other way.

https://codereview.chromium.org/472733002/diff/20001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service.cc:77: ReportAdapterPresent();
On 2014/08/14 07:29:41, Ilya Sherman wrote:
> If the adapter was removed from the system, should |adapter_| be reset?  Is it
> possible that a *different* adapter will be added to the system?  (It seems
like
> it ought to be possible, e.g. if I plug in an adapter via USB.)

Nope. The BluetoothAdapter instance never changes after it gets created. If the
adapter is no longer available, the state of BluetoothAdapter is changed. The
same instance is still kept around. The state is updated when an adapter is
available again. At least, that is the case with cros and win.

armansito@, can you confirm?

https://codereview.chromium.org/472733002/diff/20001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service.cc:91: EasyUnlockService* service_;
On 2014/08/14 07:29:41, Ilya Sherman wrote:
> nit: Please document lifetime expectations.

Done.

https://codereview.chromium.org/472733002/diff/20001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service.cc:152: // It is only disabled when
the trial exists and is in "Disable" group.
On 2014/08/14 07:29:41, Ilya Sherman wrote:
> nit: Please update this comment.

Removed "only".

https://codereview.chromium.org/472733002/diff/20001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service.cc:159: return true;
On 2014/08/14 07:29:41, Ilya Sherman wrote:
> Hmm, do we not have any command-line switches associated with the feature
> anymore?  Typically, the best practice for code that is controlled by a Finch
> trial is to also allow manual control via chrome://flags.

We used to have one but it is removed now when turning the feature on by
default.

https://codereview.chromium.org/472733002/diff/20001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service.cc:205: void
EasyUnlockService::CheckIsAllowed() {
On 2014/08/14 07:29:41, Ilya Sherman wrote:
> nit: Perhaps name this something more like "UpdateAppState()"?

Done.

https://codereview.chromium.org/472733002/diff/20001/chrome/browser/signin/ea...
File chrome/browser/signin/easy_unlock_service.h (right):

https://codereview.chromium.org/472733002/diff/20001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service.h:41: class BluetoothDetector;
On 2014/08/14 07:29:41, Ilya Sherman wrote:
> Do we really need to implement this code here, or is there perhaps already
> similar code elsewhere?  According to the bug report, we already hide some
> settings if there is no Bluetooth adapter present...

The Bluetooth settings logic is baked into its webui handler and it is what
BluetoothDetector does.

I would prefer to have the logic here instead of in the webui handler so that we
don't even need to load component app and save a bit more resources.

https://codereview.chromium.org/472733002/diff/20001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service.h:48: void
OnBluetoothAdapterPresentChanged();
On 2014/08/14 07:29:42, Ilya Sherman wrote:
> nit: Please document at least the new methods.

Done.

https://codereview.chromium.org/472733002/diff/20001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service.h:52: scoped_ptr<BluetoothDetector>
bluetooth_detector_;
On 2014/08/14 07:29:42, Ilya Sherman wrote:
> Hmm, is this a scoped_ptr just so that you can keep the BluetoothDetector
> definition out of the header file?  I'm not sure that's needed; but either way
> is fine I guess.

Yep. I prefer to put as little as possible details in header.

[armansito, 2014-08-14 20:55:05]

https://codereview.chromium.org/472733002/diff/20001/chrome/browser/signin/ea...
File chrome/browser/signin/easy_unlock_service.cc (right):

https://codereview.chromium.org/472733002/diff/20001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service.cc:77: ReportAdapterPresent();
On 2014/08/14 20:27:43, xiyuan wrote:
> On 2014/08/14 07:29:41, Ilya Sherman wrote:
> > If the adapter was removed from the system, should |adapter_| be reset?  Is
it
> > possible that a *different* adapter will be added to the system?  (It seems
> like
> > it ought to be possible, e.g. if I plug in an adapter via USB.)
> 
> Nope. The BluetoothAdapter instance never changes after it gets created. If
the
> adapter is no longer available, the state of BluetoothAdapter is changed. The
> same instance is still kept around. The state is updated when an adapter is
> available again. At least, that is the case with cros and win.
> 
> armansito@, can you confirm?

Yep, exactly. We have a singleton BluetoothAdapter at all times which represents
whatever Bluetooth adapter on the system is deemed to be currently active. So if
you had a USB adapter and unplugged it, the BluetoothAdapter would still exist
and send an AdapterPresentChanged event to its observers.

[Ilya Sherman, 2014-08-14 21:38:28]

LGTM % nits.  Thanks.

https://codereview.chromium.org/472733002/diff/60001/chrome/browser/signin/ea...
File chrome/browser/signin/easy_unlock_service.cc (right):

https://codereview.chromium.org/472733002/diff/60001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service.cc:76: //
device::BluetoothAdapter::Observer
nit: Please add a trailing colon.

https://codereview.chromium.org/472733002/diff/60001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service.cc:90:
service_->OnBluetoothAdapterPresentChanged();
nit: IMO it's slightly cleaner to just inline this method.

https://codereview.chromium.org/472733002/diff/60001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service.cc:96:
base::WeakPtrFactory<BluetoothDetector> weak_ptr_factory_;
nit: Please add a blank line after this one.

https://codereview.chromium.org/472733002/diff/60001/chrome/browser/signin/ea...
File chrome/browser/signin/easy_unlock_service_browsertest_chromeos.cc (right):

https://codereview.chromium.org/472733002/diff/60001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service_browsertest_chromeos.cc:73: void
SetupBluetoothMock(bool is_present) {
nit: "Setup" -> "SetUp"

https://codereview.chromium.org/472733002/diff/60001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service_browsertest_chromeos.cc:80:
.WillRepeatedly(testing::Return());
Hmm, why are these four lines needed?

https://codereview.chromium.org/472733002/diff/60001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service_browsertest_chromeos.cc:111:
scoped_refptr<testing::NiceMock<MockBluetoothAdapter> > mock_adapter_;
nit: Does this really need to be a class member, or can it just be a local
variable?

https://codereview.chromium.org/472733002/diff/60001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service_browsertest_chromeos.cc:230:
scoped_refptr<testing::NiceMock<MockBluetoothAdapter> > mock_adapter_;
nit: Does this really need to be a class member, or can it just be a local
variable?

[xiyuan, 2014-08-14 22:56:46]

https://codereview.chromium.org/472733002/diff/60001/chrome/browser/signin/ea...
File chrome/browser/signin/easy_unlock_service.cc (right):

https://codereview.chromium.org/472733002/diff/60001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service.cc:76: //
device::BluetoothAdapter::Observer
On 2014/08/14 21:38:28, Ilya Sherman wrote:
> nit: Please add a trailing colon.

Done.

https://codereview.chromium.org/472733002/diff/60001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service.cc:90:
service_->OnBluetoothAdapterPresentChanged();
On 2014/08/14 21:38:27, Ilya Sherman wrote:
> nit: IMO it's slightly cleaner to just inline this method.

Done.

https://codereview.chromium.org/472733002/diff/60001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service.cc:96:
base::WeakPtrFactory<BluetoothDetector> weak_ptr_factory_;
On 2014/08/14 21:38:27, Ilya Sherman wrote:
> nit: Please add a blank line after this one.

Done.

https://codereview.chromium.org/472733002/diff/60001/chrome/browser/signin/ea...
File chrome/browser/signin/easy_unlock_service_browsertest_chromeos.cc (right):

https://codereview.chromium.org/472733002/diff/60001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service_browsertest_chromeos.cc:73: void
SetupBluetoothMock(bool is_present) {
On 2014/08/14 21:38:28, Ilya Sherman wrote:
> nit: "Setup" -> "SetUp"

Done.

https://codereview.chromium.org/472733002/diff/60001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service_browsertest_chromeos.cc:80:
.WillRepeatedly(testing::Return());
On 2014/08/14 21:38:28, Ilya Sherman wrote:
> Hmm, why are these four lines needed?

It was because I was desperately trying to figure out why gmock fails on bots.
Thank you for helping me investigate. IsInitialized is not mocked though. I now
suspect IsPowered and GetDevices, which are called from system tray. Maybe the
bots is too slow and somehow they are called.

https://codereview.chromium.org/472733002/diff/60001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service_browsertest_chromeos.cc:111:
scoped_refptr<testing::NiceMock<MockBluetoothAdapter> > mock_adapter_;
On 2014/08/14 21:38:28, Ilya Sherman wrote:
> nit: Does this really need to be a class member, or can it just be a local
> variable?

It turns out that we have to do this. Otherwise, it would be released when
SetupBluetoothMock returns and default adapter gets used again.

https://codereview.chromium.org/472733002/diff/60001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service_browsertest_chromeos.cc:230:
scoped_refptr<testing::NiceMock<MockBluetoothAdapter> > mock_adapter_;
On 2014/08/14 21:38:28, Ilya Sherman wrote:
> nit: Does this really need to be a class member, or can it just be a local
> variable?

ditto.

[xiyuan, 2014-08-15 21:51:34]

The CQ bit was checked by xiyuan@chromium.org

[commit-bot: I haz the power, 2014-08-15 21:55:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xiyuan@chromium.org/472733002/120001

[commit-bot: I haz the power, 2014-08-16 01:23:17]

Committed patchset #7 (120001) as 290081