Refactor away the RequestPinView's delegate

Refactor the RequestPinView class to use a pair of callbacks instead of
the callback+delegate.

This refactoring makes the interface of the RequestPinView more compact,
which will simplify the introduction of alternative UI implementations
(for the Chrome OS Login and Lock screens) in follow-up CLs.

This is a pure refactoring, it's expected to bring no behavior changes.

Bug: 964069
Change-Id: I7d35592f7897b133d98f4dbc0657a19009ebb7d1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1722847
Commit-Queue: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Achuith Bhandarkar <achuith@chromium.org>
Reviewed-by: Igor <igorcov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#683334}

chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc

@@ -71,8 +71,9 @@ PinDialogManager::RequestPinResult PinDialogManager::RequestPin(
     if (!active_pin_dialog_->IsLocked())
       return RequestPinResult::kDialogDisplayedAlready;
 
-    // Set the new callback to be used by the view.
-    active_pin_dialog_->SetCallback(std::move(callback));
+    DCHECK(!active_request_pin_callback_);
+    active_request_pin_callback_ = std::move(callback);
+
     active_pin_dialog_->SetDialogParameters(code_type, error_type,
                                             attempts_left, accept_input);
     active_pin_dialog_->DialogModelChanged();
@@ -88,9 +89,16 @@ PinDialogManager::RequestPinResult PinDialogManager::RequestPin(
   if (current_time - sign_request_times_[key] > kSignRequestIdTimeout)
     return RequestPinResult::kInvalidId;
 
+  DCHECK(!active_request_pin_callback_);
+  active_request_pin_callback_ = std::move(callback);
+
   active_dialog_extension_id_ = extension_id;
-  active_pin_dialog_ = new RequestPinView(
-      extension_name, code_type, attempts_left, std::move(callback), this);
+  active_pin_dialog_ =
+      new RequestPinView(extension_name, code_type, attempts_left,
+                         base::BindRepeating(&PinDialogManager::OnPinEntered,
+                                             weak_factory_.GetWeakPtr()),
+                         base::BindOnce(&PinDialogManager::OnViewDestroyed,
+                                        weak_factory_.GetWeakPtr()));
 
   const gfx::NativeWindow parent = GetBrowserParentWindow();
   // If there is no parent, falls back to the root window for new windows.
@@ -101,18 +109,6 @@ PinDialogManager::RequestPinResult PinDialogManager::RequestPin(
   return RequestPinResult::kSuccess;
 }
 
-void PinDialogManager::OnPinDialogInput() {
-  last_response_closed_[active_dialog_extension_id_] = false;
-}
-
-void PinDialogManager::OnPinDialogClosed() {
-  last_response_closed_[active_dialog_extension_id_] = true;
-  // |active_pin_dialog_| is managed by |active_window_|. This local copy of
-  // the pointer is reset here to allow a new dialog to be created when a new
-  // request comes.
-  active_pin_dialog_ = nullptr;
-}
-
 PinDialogManager::StopPinRequestResult
 PinDialogManager::StopPinRequestWithError(
     const std::string& extension_id,
@@ -126,12 +122,8 @@ PinDialogManager::StopPinRequestWithError(
   if (!active_pin_dialog_->IsLocked())
     return StopPinRequestResult::kNoUserInput;
 
-  active_pin_dialog_->SetCallback(base::BindOnce(
-      [](StopPinRequestCallback callback, const std::string& user_input) {
-        DCHECK(user_input.empty());
-        std::move(callback).Run();
-      },
-      std::move(callback)));
+  DCHECK(!active_stop_pin_request_callback_);
+  active_stop_pin_request_callback_ = std::move(callback);
   active_pin_dialog_->SetDialogParameters(
       RequestPinView::RequestPinCodeType::UNCHANGED, error_type,
       /*attempts_left=*/-1,
@@ -154,10 +146,13 @@ bool PinDialogManager::CloseDialog(const std::string& extension_id) {
     return false;
   }
 
-  // Close the window. |active_pin_dialog_| gets deleted inside Close().
+  // The view destruction may happen asynchronously after the Close() call. For
+  // simplicity, clear our state and execute the callback immediately.
   active_window_->Close();
-  active_pin_dialog_ = nullptr;
-
+  if (active_pin_dialog_) {
+    weak_factory_.InvalidateWeakPtrs();
+    OnPinDialogClosed();
+  }
   return true;
 }
 
@@ -176,4 +171,29 @@ void PinDialogManager::ExtensionUnloaded(const std::string& extension_id) {
   }
 }
 
+void PinDialogManager::OnPinEntered(const std::string& user_input) {
+  DCHECK(!active_stop_pin_request_callback_);
+  last_response_closed_[active_dialog_extension_id_] = false;
+  if (active_request_pin_callback_)
+    std::move(active_request_pin_callback_).Run(user_input);
+}
+
+void PinDialogManager::OnViewDestroyed() {
+  OnPinDialogClosed();
+}
+
+void PinDialogManager::OnPinDialogClosed() {
+  DCHECK(active_pin_dialog_);
+  DCHECK(!active_request_pin_callback_ || !active_stop_pin_request_callback_);
+
+  active_pin_dialog_ = nullptr;
+  active_window_ = nullptr;
+  last_response_closed_[active_dialog_extension_id_] = true;
+  active_dialog_extension_id_.clear();
+  if (active_request_pin_callback_)
+    std::move(active_request_pin_callback_).Run(/*user_input=*/std::string());
+  if (active_stop_pin_request_callback_)
+    std::move(active_stop_pin_request_callback_).Run();
+}
+
 }  // namespace chromeos

chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h

@@ -21,7 +21,7 @@ namespace chromeos {
 // Manages the state of the dialog that requests the PIN from user. Used by the
 // extensions that need to request the PIN. Implemented as requirement for
 // crbug.com/612886
-class PinDialogManager final : RequestPinView::Delegate {
+class PinDialogManager final {
  public:
   enum class RequestPinResult {
     kSuccess,
@@ -75,10 +75,6 @@ class PinDialogManager final : RequestPinView::Delegate {
                               int attempts_left,
                               RequestPinCallback callback);
 
-  // chromeos::RequestPinView::Delegate overrides.
-  void OnPinDialogInput() override;
-  void OnPinDialogClosed() override;
-
   // Updates the existing dialog with new error message. Runs |callback| when
   // user closes the dialog. Returns whether the provided |extension_id| matches
   // the extension owning the active dialog.
@@ -105,6 +101,16 @@ class PinDialogManager final : RequestPinView::Delegate {
  private:
   using ExtensionNameRequestIdPair = std::pair<std::string, int>;
 
+  // The callback that gets invoked once the user sends some input into the PIN
+  // dialog.
+  void OnPinEntered(const std::string& user_input);
+  // The callback that gets invoked once the PIN dialog's view gets destroyed.
+  void OnViewDestroyed();
+
+  // Called when the PIN dialog is closed. Cleans up the internal state and runs
+  // the needed callbacks.
+  void OnPinDialogClosed();
+
   // Tells whether user closed the last request PIN dialog issued by an
   // extension. The extension_id is the key and value is true if user closed the
   // dialog. Used to determine if the limit of dialogs rejected by the user has
@@ -120,6 +126,8 @@ class PinDialogManager final : RequestPinView::Delegate {
   RequestPinView* active_pin_dialog_ = nullptr;
   std::string active_dialog_extension_id_;
   views::Widget* active_window_ = nullptr;
+  RequestPinCallback active_request_pin_callback_;
+  StopPinRequestCallback active_stop_pin_request_callback_;
 
   base::WeakPtrFactory<PinDialogManager> weak_factory_{this};
 };

chrome/browser/chromeos/ui/request_pin_view.cc

@@ -33,14 +33,15 @@ constexpr int kDefaultTextWidth = 200;
 
 }  // namespace
 
-RequestPinView::RequestPinView(const std::string& extension_name,
+RequestPinView::RequestPinView(
+    const std::string& extension_name,
     RequestPinView::RequestPinCodeType code_type,
     int attempts_left,
-                               RequestPinCallback callback,
-                               Delegate* delegate)
-    : callback_(std::move(callback)), delegate_(delegate) {
+    const PinEnteredCallback& pin_entered_callback,
+    ViewDestructionCallback view_destruction_callback)
+    : pin_entered_callback_(pin_entered_callback),
+      view_destruction_callback_(std::move(view_destruction_callback)) {
   DCHECK_NE(code_type, RequestPinCodeType::UNCHANGED);
-  DCHECK(delegate);
   Init();
   SetExtensionName(extension_name);
   const bool accept_input = (attempts_left != 0);
@@ -49,13 +50,8 @@ RequestPinView::RequestPinView(const std::string& extension_name,
   chrome::RecordDialogCreation(chrome::DialogIdentifier::REQUEST_PIN);
 }
 
-// When the parent window is closed while the dialog is active, this object is
-// destroyed without triggering Accept or Cancel. If the callback_ wasn't called
-// it needs to send the response.
 RequestPinView::~RequestPinView() {
-  if (callback_)
-    std::move(callback_).Run(/*user_input=*/std::string());
-  delegate_->OnPinDialogClosed();
+  std::move(view_destruction_callback_).Run();
 }
 
 void RequestPinView::ContentsChanged(views::Textfield* sender,
@@ -64,16 +60,15 @@ void RequestPinView::ContentsChanged(views::Textfield* sender,
 }
 
 bool RequestPinView::Cancel() {
-  // Destructor will be called after this which notifies the delegate.
+  // Destructor will be called after this which notifies the callback.
   return true;
 }
 
 bool RequestPinView::Accept() {
-  DCHECK(!callback_.is_null());
-
   if (!textfield_->GetEnabled())
     return true;
   DCHECK(!textfield_->GetText().empty());
+  DCHECK(!locked_);
 
   error_label_->SetVisible(true);
   error_label_->SetText(
@@ -84,9 +79,9 @@ bool RequestPinView::Accept() {
   // The |textfield_| and OK button become disabled, but the user still can
   // close the dialog.
   SetAcceptInput(false);
-  std::move(callback_).Run(base::UTF16ToUTF8(textfield_->GetText()));
+  pin_entered_callback_.Run(base::UTF16ToUTF8(textfield_->GetText()));
+  locked_ = true;
   DialogModelChanged();
-  delegate_->OnPinDialogInput();
 
   return false;
 }
@@ -96,7 +91,7 @@ bool RequestPinView::IsDialogButtonEnabled(ui::DialogButton button) const {
     case ui::DialogButton::DIALOG_BUTTON_CANCEL:
       return true;
     case ui::DialogButton::DIALOG_BUTTON_OK:
-      if (callback_.is_null())
+      if (locked_)
         return false;
       // Not locked but the |textfield_| is not enabled. It's just a
       // notification to the user and [OK] button can be used to close the
@@ -131,12 +126,7 @@ gfx::Size RequestPinView::CalculatePreferredSize() const {
 }
 
 bool RequestPinView::IsLocked() const {
-  return callback_.is_null();
-}
-
-void RequestPinView::SetCallback(RequestPinCallback callback) {
-  DCHECK(!callback_);
-  callback_ = std::move(callback);
+  return locked_;
 }
 
 void RequestPinView::SetDialogParameters(
@@ -144,6 +134,7 @@ void RequestPinView::SetDialogParameters(
     RequestPinView::RequestPinErrorType error_type,
     int attempts_left,
     bool accept_input) {
+  locked_ = false;
   SetErrorMessage(error_type, attempts_left);
   SetAcceptInput(accept_input);
 

chrome/browser/chromeos/ui/request_pin_view.h

@@ -26,7 +26,7 @@ namespace chromeos {
 
 // A dialog box for requesting PIN code. Instances of this class are managed by
 // PinDialogManager.
-class RequestPinView : public views::DialogDelegateView,
+class RequestPinView final : public views::DialogDelegateView,
                              public views::TextfieldController {
  public:
   enum RequestPinCodeType { PIN, PUK, UNCHANGED };
@@ -39,19 +39,9 @@ class RequestPinView : public views::DialogDelegateView,
     UNKNOWN_ERROR
   };
 
-  class Delegate {
-   public:
-    // Notification when user closes the PIN dialog.
-    virtual void OnPinDialogClosed() = 0;
-
-    // Notification when the user provided input to dialog.
-    virtual void OnPinDialogInput() = 0;
-  };
-
-  // Used to send the PIN/PUK entered by the user in the textfield to the
-  // extension that asked for the code.
-  using RequestPinCallback =
-      base::OnceCallback<void(const std::string& user_input)>;
+  using PinEnteredCallback =
+      base::RepeatingCallback<void(const std::string& user_input)>;
+  using ViewDestructionCallback = base::OnceClosure;
 
   // Creates the view to be embeded in the dialog that requests the PIN/PUK.
   // |extension_name| - the name of the extension making the request. Displayed
@@ -62,13 +52,13 @@ class RequestPinView : public views::DialogDelegateView,
   //     zero the textfield is disabled and user cannot provide any input. When
   //     -1 the user is allowed to provide the input and no information about
   //     the attepts left is displayed in the view.
-  // |callback| - used to send the value of the PIN/PUK the user entered.
-  // |delegate| - used to notify that dialog was closed. Cannot be null.
+  // |pin_entered_callback| - called every time the user submits the input.
+  // |view_destruction_callback| - called by the destructor.
   RequestPinView(const std::string& extension_name,
                  RequestPinCodeType code_type,
                  int attempts_left,
-                 RequestPinCallback callback,
-                 Delegate* delegate);
+                 const PinEnteredCallback& pin_entered_callback,
+                 ViewDestructionCallback view_destruction_callback);
   RequestPinView(const RequestPinView&) = delete;
   RequestPinView& operator=(const RequestPinView&) = delete;
   ~RequestPinView() override;
@@ -92,10 +82,6 @@ class RequestPinView : public views::DialogDelegateView,
   // the user input data.
   bool IsLocked() const;
 
-  // Set the new callback to be used when user will provide the input. The old
-  // callback must be used and reset to null at this point.
-  void SetCallback(RequestPinCallback callback);
-
   // |code_type| - specifies whether the user is asked to enter PIN or PUK. If
   //     UNCHANGED value is provided, the dialog displays the same value that
   //     was last set.
@@ -126,14 +112,12 @@ class RequestPinView : public views::DialogDelegateView,
   // |window_title_| and |code_type_|.
   void UpdateHeaderText();
 
-  // Used to send the input when the view is not locked. If user closes the
-  // view, the provided input is empty. The |callback_| must be reset to null
-  // after being used, allowing to check that it was used when a new callback is
-  // set.
-  RequestPinCallback callback_;
+  const PinEnteredCallback pin_entered_callback_;
+  ViewDestructionCallback view_destruction_callback_;
 
-  // Owned by the caller.
-  Delegate* delegate_ = nullptr;
+  // Whether the UI is locked, disallowing the user to input any data, while the
+  // caller processes the previously entered PIN/PUK.
+  bool locked_ = false;
 
   base::string16 window_title_;
   views::Label* header_label_ = nullptr;

chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc

@@ -244,7 +244,6 @@ void CertificateProviderStopPinRequestFunction::OnPinRequestStopped() {
   DCHECK(service);
 
   Respond(NoArguments());
-  service->pin_dialog_manager()->OnPinDialogClosed();
 }
 
 CertificateProviderRequestPinFunction::