New issue
Advanced search Search tips

Issue 822244 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

Remove unused features from U2fDiscovery

Project Member Reported by engedy@chromium.org, Mar 15 2018

Issue description

No production code is calling U2fDiscovery::Stop, and all observer method implementations are no-ops. 

Similarly, no U2fDiscovery has more than a single observer added, and the only time that observer is removed is when both the discovery and that observer (the owner) dies.

It sounds like we are not goind to make use of that functionality in the near future, so we should remove these as they keep causing additional complexity in dependent CLs.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Mar 20 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/66353c23a32b3ada6a26699e4e8420397613c31b

commit 66353c23a32b3ada6a26699e4e8420397613c31b
Author: Balazs Engedy <engedy@chromium.org>
Date: Tue Mar 20 15:41:07 2018

Apply Occam's razor to U2fDiscovery.

No production code is calling U2fDiscovery::Stop, and all observer
method implementations are no-ops.

Similarly, no U2fDiscovery has more than a single observer added, and
the only time that observer is removed is when both the discovery and
that observer (the owner) dies.

It sounds like we are not going to make use of this functionality in the
near future, so this CL removes:
 -- the ability to Stop() U2fDiscoveries, and
 -- the ability to have more than a single Observer per U2fDiscovery.

The CL also moves slightly more functionality into the U2fDiscovery base
class from derived classes, most importantly with the goal of making
sure the fake and production implementations function consistently in
the following areas:
 -- U2fDiscovery now keeps track of whether Start NotifyDiscoveryStarted
    have been called, and exposes corresponding getters.
 -- It is now enforced that Start() and NotifyDiscoveryStarted() can
    be called once each, and in this order, as well as that
    NotifyDeviceAdded cannot be called before Start() is called.
 -- It is now enforced that Observer::DiscoveryStarted is never invoked
    synchronously while Start() is still on the call stack.

Bug:  822244 ,  823686 
Change-Id: Ia029646d43481a1a2ca58452577727f96d6aade1
Reviewed-on: https://chromium-review.googlesource.com/964301
Commit-Queue: Balazs Engedy <engedy@chromium.org>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544379}
[modify] https://crrev.com/66353c23a32b3ada6a26699e4e8420397613c31b/device/fido/fake_fido_discovery.cc
[modify] https://crrev.com/66353c23a32b3ada6a26699e4e8420397613c31b/device/fido/fake_fido_discovery.h
[modify] https://crrev.com/66353c23a32b3ada6a26699e4e8420397613c31b/device/fido/fake_fido_discovery_unittest.cc
[modify] https://crrev.com/66353c23a32b3ada6a26699e4e8420397613c31b/device/fido/fido_ble_discovery.cc
[modify] https://crrev.com/66353c23a32b3ada6a26699e4e8420397613c31b/device/fido/fido_ble_discovery.h
[modify] https://crrev.com/66353c23a32b3ada6a26699e4e8420397613c31b/device/fido/fido_ble_discovery_unittest.cc
[modify] https://crrev.com/66353c23a32b3ada6a26699e4e8420397613c31b/device/fido/fido_discovery.cc
[modify] https://crrev.com/66353c23a32b3ada6a26699e4e8420397613c31b/device/fido/fido_discovery.h
[modify] https://crrev.com/66353c23a32b3ada6a26699e4e8420397613c31b/device/fido/fido_discovery_unittest.cc
[modify] https://crrev.com/66353c23a32b3ada6a26699e4e8420397613c31b/device/fido/fido_hid_discovery.cc
[modify] https://crrev.com/66353c23a32b3ada6a26699e4e8420397613c31b/device/fido/fido_hid_discovery.h
[modify] https://crrev.com/66353c23a32b3ada6a26699e4e8420397613c31b/device/fido/fido_hid_discovery_unittest.cc
[modify] https://crrev.com/66353c23a32b3ada6a26699e4e8420397613c31b/device/fido/scoped_virtual_u2f_device.cc
[modify] https://crrev.com/66353c23a32b3ada6a26699e4e8420397613c31b/device/fido/u2f_request.cc
[modify] https://crrev.com/66353c23a32b3ada6a26699e4e8420397613c31b/device/fido/u2f_request.h

Status: Fixed (was: Started)

Sign in to add a comment