Reland "Introduce a generic ScopedU2fDiscoveryFactory override."

This is a reland of 0588d062, as it was concluded
not to have been the culprit for the compile breakage.

Original change's description:
> Introduce a generic ScopedU2fDiscoveryFactory override.
>
>  -- In preparation for introducing ScopedU2fVirtualDiscoveryFactory, create a
>     base class ScopedU2fDiscoveryFactory and let ScopedU2fFakeDiscoveryFactory
>     derive from that.
>  -- In preparation for a wider adoption of FakeU2fDiscovery in tests that could
>     not care less about when a discovery starts, add a version of the fake
>     discovery where SimulateStarted()/SimulateStopped() is called automatically.
>
> Bug: 785955
> Change-Id: I4b1c9bd962c6e225be45971bbb0d5aad3b05a68a
> Reviewed-on: https://chromium-review.googlesource.com/959001
> Commit-Queue: Balazs Engedy <engedy@chromium.org>
> Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#542507}

TBR=jdoerrie@chromium.org

Bug: 785955
Change-Id: I4c9f42d81793567b6536d303f8a4d020fbe147d9
Reviewed-on: https://chromium-review.googlesource.com/959322
Commit-Queue: Balazs Engedy <engedy@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542596}

device/fido/fake_u2f_discovery.cc

@@ -14,8 +14,10 @@ namespace test {
 
 // FakeU2fDiscovery ----------------------------------------------------------
 
-FakeU2fDiscovery::FakeU2fDiscovery(U2fTransportProtocol transport)
+FakeU2fDiscovery::FakeU2fDiscovery(U2fTransportProtocol transport,
+                                   StartStopMode mode)
     : transport_(transport),
+      mode_(mode),
       start_called_callback_(wait_for_start_loop_.QuitClosure()),
       stop_called_callback_(wait_for_stop_loop_.QuitClosure()) {}
 FakeU2fDiscovery::~FakeU2fDiscovery() = default;
@@ -62,6 +64,9 @@ void FakeU2fDiscovery::Start() {
 
   ASSERT_TRUE(start_called_callback_) << "Start called twice.";
   std::move(start_called_callback_).Run();
+
+  if (mode_ == StartStopMode::kAutomatic)
+    SimulateStarted(true /* success */);
 }
 
 void FakeU2fDiscovery::Stop() {
@@ -70,69 +75,44 @@ void FakeU2fDiscovery::Stop() {
 
   ASSERT_TRUE(stop_called_callback_) << "Stop called twice.";
   std::move(stop_called_callback_).Run();
-}
 
-// ScopedFakeU2fDiscoveryFactory ---------------------------------------------
-
-ScopedFakeU2fDiscoveryFactory::ScopedFakeU2fDiscoveryFactory() {
-  DCHECK(!g_current_factory) << "Nested fake factories not yet supported.";
-  g_current_factory = this;
-
-  original_factory_func_ =
-      std::exchange(U2fDiscovery::g_factory_func_, &CreateFakeU2fDiscovery);
-}
-
-ScopedFakeU2fDiscoveryFactory::~ScopedFakeU2fDiscoveryFactory() {
-  g_current_factory = nullptr;
-  U2fDiscovery::g_factory_func_ = original_factory_func_;
+  if (mode_ == StartStopMode::kAutomatic)
+    SimulateStopped(true /* success */);
 }
 
-FakeU2fDiscovery* ScopedFakeU2fDiscoveryFactory::ForgeNextHidDiscovery() {
-  return static_cast<FakeU2fDiscovery*>(
-      SetNextHidDiscovery(std::make_unique<FakeU2fDiscovery>(
-          U2fTransportProtocol::kUsbHumanInterfaceDevice)));
-}
+// ScopedFakeU2fDiscoveryFactory ---------------------------------------------
 
-FakeU2fDiscovery* ScopedFakeU2fDiscoveryFactory::ForgeNextBleDiscovery() {
-  return static_cast<FakeU2fDiscovery*>(
-      SetNextBleDiscovery(std::make_unique<FakeU2fDiscovery>(
-          U2fTransportProtocol::kBluetoothLowEnergy)));
-}
+ScopedFakeU2fDiscoveryFactory::ScopedFakeU2fDiscoveryFactory() = default;
+ScopedFakeU2fDiscoveryFactory::~ScopedFakeU2fDiscoveryFactory() = default;
 
-U2fDiscovery* ScopedFakeU2fDiscoveryFactory::SetNextHidDiscovery(
-    std::unique_ptr<U2fDiscovery> replacement) {
-  DCHECK_EQ(replacement->GetTransportProtocol(),
-            U2fTransportProtocol::kUsbHumanInterfaceDevice);
-  next_hid_discovery_ = std::move(replacement);
+FakeU2fDiscovery* ScopedFakeU2fDiscoveryFactory::ForgeNextHidDiscovery(
+    FakeU2fDiscovery::StartStopMode mode) {
+  next_hid_discovery_ = std::make_unique<FakeU2fDiscovery>(
+      U2fTransportProtocol::kUsbHumanInterfaceDevice, mode);
   return next_hid_discovery_.get();
 }
 
-U2fDiscovery* ScopedFakeU2fDiscoveryFactory::SetNextBleDiscovery(
-    std::unique_ptr<U2fDiscovery> replacement) {
-  DCHECK_EQ(replacement->GetTransportProtocol(),
-            U2fTransportProtocol::kBluetoothLowEnergy);
-  next_ble_discovery_ = std::move(replacement);
+FakeU2fDiscovery* ScopedFakeU2fDiscoveryFactory::ForgeNextBleDiscovery(
+    FakeU2fDiscovery::StartStopMode mode) {
+  next_ble_discovery_ = std::make_unique<FakeU2fDiscovery>(
+      U2fTransportProtocol::kBluetoothLowEnergy, mode);
   return next_ble_discovery_.get();
 }
 
-// static
-std::unique_ptr<U2fDiscovery>
-ScopedFakeU2fDiscoveryFactory::CreateFakeU2fDiscovery(
+std::unique_ptr<U2fDiscovery> ScopedFakeU2fDiscoveryFactory::CreateU2fDiscovery(
     U2fTransportProtocol transport,
     ::service_manager::Connector* connector) {
   switch (transport) {
     case U2fTransportProtocol::kBluetoothLowEnergy:
-      return std::move(g_current_factory->next_ble_discovery_);
+      DCHECK(next_ble_discovery_);
+      return std::move(next_ble_discovery_);
     case U2fTransportProtocol::kUsbHumanInterfaceDevice:
-      return std::move(g_current_factory->next_hid_discovery_);
+      DCHECK(next_hid_discovery_);
+      return std::move(next_hid_discovery_);
   }
   NOTREACHED();
   return nullptr;
 }
 
-// static
-ScopedFakeU2fDiscoveryFactory*
-    ScopedFakeU2fDiscoveryFactory::g_current_factory = nullptr;
-
 }  // namespace test
 }  // namespace device

device/fido/fake_u2f_discovery.h

@@ -23,10 +23,54 @@ namespace test {
 
 // Fake U2F discovery simulating the behavior of the production implementations,
 // and can be used to feed U2fRequests with fake/mock U2F devices.
+//
+// Most often this class is used together with ScopedFakeU2fDiscoveryFactory:
+//
+//   ScopedFakeU2fDiscoveryFactory factory;
+//   auto* fake_hid_discovery = factory.ForgeNextHidDiscovery();
+//   auto* fake_ble_discovery = factory.ForgeNextBleDiscovery();
+//
+//   // Run the production code that will eventually call:
+//   //// U2fDiscovery::Create(U2fTransportProtocol::kUsbHumanInterfaceDevice)
+//   //// hid_instance->Start();
+//   //// U2fDiscovery::Create(U2fTransportProtocol::kBluetoothLowEnergy)
+//   //// ble_instance->Start();
+//
+//   // Wait, i.e. spin the message loop until the fake discoveries are started.
+//   fake_hid_discovery->WaitForCallToStart();
+//   fake_ble_discovery->WaitForCallToStart();
+//
+//   // Add devices to be discovered immediately.
+//   fake_hid_discovery->AddDevice(std::make_unique<MockU2fDevice>(...));
+//
+//   // Start discoveries (HID succeeds, BLE fails).
+//   fake_hid_discovery->SimulateStart(true /* success */);
+//   fake_ble_discovery->SimulateStart(false /* success */);
+//
+//   // Add devices discovered after doing some heavy lifting.
+//   fake_hid_discovery->AddDevice(std::make_unique<MockU2fDevice>(...));
+//
+//   // Run the production code that will eventually stop the discovery.
+//   //// hid_instance->Stop();
+//
+//   // Wait for discovery to be stopped by the production code, and simulate
+//   // the discovery starting successfully.
+//   fake_hid_discovery->WaitForCallToStopAndSimulateSuccess();
+//
 class FakeU2fDiscovery : public U2fDiscovery,
                          public base::SupportsWeakPtr<FakeU2fDiscovery> {
  public:
-  explicit FakeU2fDiscovery(U2fTransportProtocol transport);
+  enum class StartStopMode {
+    // SimulateStarted()/SimualteStopped() needs to be called manually after the
+    // production code calls Start()/Stop().
+    kManual,
+    // The discovery is automatically and successfully started/stopped once
+    // Start()/Stop() is called.
+    kAutomatic
+  };
+
+  explicit FakeU2fDiscovery(U2fTransportProtocol transport,
+                            StartStopMode mode = StartStopMode::kManual);
   ~FakeU2fDiscovery() override;
 
   // Blocks until start/stop is requested.
@@ -56,8 +100,9 @@ class FakeU2fDiscovery : public U2fDiscovery,
   void Stop() override;
 
  private:
-  U2fTransportProtocol transport_;
+  const U2fTransportProtocol transport_;
 
+  const StartStopMode mode_;
   bool is_running_ = false;
 
   base::RunLoop wait_for_start_loop_;
@@ -69,35 +114,35 @@ class FakeU2fDiscovery : public U2fDiscovery,
   DISALLOW_COPY_AND_ASSIGN(FakeU2fDiscovery);
 };
 
-// Hijacks the U2fDiscovery::Create() factory method to return user-defined
-// U2fDiscovery instances while this instance is in scope.
-class ScopedFakeU2fDiscoveryFactory {
+// Overrides U2fDiscovery::Create to construct FakeU2fDiscoveries while this
+// instance is in scope.
+class ScopedFakeU2fDiscoveryFactory
+    : public ::device::internal::ScopedU2fDiscoveryFactory {
  public:
+  using StartStopMode = FakeU2fDiscovery::StartStopMode;
+
   ScopedFakeU2fDiscoveryFactory();
   ~ScopedFakeU2fDiscoveryFactory();
 
   // Constructs a fake BLE/HID discovery to be returned from the next call to
   // U2fDiscovery::Create. Returns a raw pointer to the fake so that tests can
   // set it up according to taste.
-  FakeU2fDiscovery* ForgeNextBleDiscovery();
-  FakeU2fDiscovery* ForgeNextHidDiscovery();
-
-  // Same as above, but the test can supply whatever custom |discovery| instance
-  // that it wants to be returned from the next call to U2fDiscovery::Create.
-  U2fDiscovery* SetNextHidDiscovery(std::unique_ptr<U2fDiscovery> discovery);
-  U2fDiscovery* SetNextBleDiscovery(std::unique_ptr<U2fDiscovery> discovery);
-
- private:
-  static std::unique_ptr<U2fDiscovery> CreateFakeU2fDiscovery(
+  //
+  // It is an error not to call the relevant method prior to a call to
+  // U2fDiscovery::Create with the respective transport.
+  FakeU2fDiscovery* ForgeNextHidDiscovery(
+      StartStopMode mode = StartStopMode::kManual);
+  FakeU2fDiscovery* ForgeNextBleDiscovery(
+      StartStopMode mode = StartStopMode::kManual);
+
+ protected:
+  std::unique_ptr<U2fDiscovery> CreateU2fDiscovery(
       U2fTransportProtocol transport,
-      ::service_manager::Connector* connector);
+      ::service_manager::Connector* connector) override;
 
-  static ScopedFakeU2fDiscoveryFactory* g_current_factory;
-
-  U2fDiscovery::FactoryFuncPtr original_factory_func_;
-
-  std::unique_ptr<U2fDiscovery> next_hid_discovery_;
-  std::unique_ptr<U2fDiscovery> next_ble_discovery_;
+ private:
+  std::unique_ptr<FakeU2fDiscovery> next_hid_discovery_;
+  std::unique_ptr<FakeU2fDiscovery> next_ble_discovery_;
 
   DISALLOW_COPY_AND_ASSIGN(ScopedFakeU2fDiscoveryFactory);
 };

device/fido/u2f_discovery.cc

@@ -132,4 +132,34 @@ bool U2fDiscovery::RemoveDevice(base::StringPiece device_id) {
   return true;
 }
 
+// ScopedU2fDiscoveryFactory -------------------------------------------------
+
+namespace internal {
+
+ScopedU2fDiscoveryFactory::ScopedU2fDiscoveryFactory() {
+  original_factory_ = std::exchange(g_current_factory, this);
+  original_factory_func_ =
+      std::exchange(U2fDiscovery::g_factory_func_,
+                    &ForwardCreateU2fDiscoveryToCurrentFactory);
+}
+
+ScopedU2fDiscoveryFactory::~ScopedU2fDiscoveryFactory() {
+  g_current_factory = original_factory_;
+  U2fDiscovery::g_factory_func_ = original_factory_func_;
+}
+
+// static
+std::unique_ptr<U2fDiscovery>
+ScopedU2fDiscoveryFactory::ForwardCreateU2fDiscoveryToCurrentFactory(
+    U2fTransportProtocol transport,
+    ::service_manager::Connector* connector) {
+  DCHECK(g_current_factory);
+  return g_current_factory->CreateU2fDiscovery(transport, connector);
+}
+
+// static
+ScopedU2fDiscoveryFactory* ScopedU2fDiscoveryFactory::g_current_factory =
+    nullptr;
+
+}  // namespace internal
 }  // namespace device

device/fido/u2f_discovery.h

@@ -25,8 +25,8 @@ namespace device {
 
 class U2fDevice;
 
-namespace test {
-class ScopedFakeU2fDiscoveryFactory;
+namespace internal {
+class ScopedU2fDiscoveryFactory;
 }
 
 class U2fDiscovery {
@@ -79,7 +79,7 @@ class U2fDiscovery {
   base::ObserverList<Observer> observers_;
 
  private:
-  friend class test::ScopedFakeU2fDiscoveryFactory;
+  friend class internal::ScopedU2fDiscoveryFactory;
 
   // Factory function can be overridden by tests to construct fakes.
   using FactoryFuncPtr = decltype(&Create);
@@ -88,6 +88,38 @@ class U2fDiscovery {
   DISALLOW_COPY_AND_ASSIGN(U2fDiscovery);
 };
 
+namespace internal {
+
+// Base class for a scoped override of U2fDiscovery::Create, used in unit tests,
+// layout tests, and when running with the Web Authn Testing API enabled.
+//
+// While there is a subclass instance in scope, calls to the factory method will
+// be hijacked such that the derived class's CreateU2fDiscovery method will be
+// invoked instead.
+class ScopedU2fDiscoveryFactory {
+ public:
+  ScopedU2fDiscoveryFactory();
+  ~ScopedU2fDiscoveryFactory();
+
+ protected:
+  virtual std::unique_ptr<U2fDiscovery> CreateU2fDiscovery(
+      U2fTransportProtocol transport,
+      ::service_manager::Connector* connector) = 0;
+
+ private:
+  static std::unique_ptr<U2fDiscovery>
+  ForwardCreateU2fDiscoveryToCurrentFactory(
+      U2fTransportProtocol transport,
+      ::service_manager::Connector* connector);
+
+  static ScopedU2fDiscoveryFactory* g_current_factory;
+  ScopedU2fDiscoveryFactory* original_factory_;
+  U2fDiscovery::FactoryFuncPtr original_factory_func_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedU2fDiscoveryFactory);
+};
+
+}  // namespace internal
 }  // namespace device
 
 #endif  // DEVICE_FIDO_U2F_DISCOVERY_H_