Implementation for chrome.certificateProvider.requestPin/stopPinRequest

chrome/browser/chromeos/options/request_pin_view.h

+// Copyright (c) 2016 The Chromium Authors. All rights reserved.

[emaxx, 2016/07/08 02:00:15]

The location under the options/ directory doesn't seem right for this file.

[igorcov, 2016/07/08 11:40:53]

Moved to ui folder.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_CHROMEOS_OPTIONS_REQUEST_PIN_VIEW_H_
+#define CHROME_BROWSER_CHROMEOS_OPTIONS_REQUEST_PIN_VIEW_H_
+
+#include <memory>
+#include <string>
+
+#include "base/compiler_specific.h"
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "base/strings/string16.h"
+#include "ui/views/controls/textfield/textfield_controller.h"
+#include "ui/views/view.h"
+#include "ui/views/window/dialog_delegate.h"
+
+namespace views {
+class Label;
+}
+
+namespace chromeos {
+
+class PassphraseTextfield;
+
+// A dialog box for requesting PIN code.
+class RequestPinView : public views::DialogDelegateView,
+                       public views::TextfieldController {
+ public:
+  using UserInputCallback = base::Callback<void(const base::string16&)>;

[emaxx, 2016/07/08 02:00:15]

nit: include "base/callback.h"

[emaxx, 2016/07/08 02:00:15]

The name is not exactly correct, as the same callback is used in other
scenarios, like just closing the dialog.

[igorcov, 2016/07/08 11:40:54]

Done.

[igorcov, 2016/07/11 15:04:06]

Done.

+  explicit RequestPinView(const std::string& extension_name,

[emaxx, 2016/07/08 02:00:15]

nit: "explicit" is not necessary

[igorcov, 2016/07/08 11:40:53]

Done.

+                          const std::string& dialog_type,
+                          const base::string16& error_message,
+                          const bool accept_input,

[emaxx, 2016/07/08 02:00:15]

nit: const-ness is not usually used with arguments of primitive types

[igorcov, 2016/07/08 11:40:54]

Done

+                          const UserInputCallback& callback);
+  ~RequestPinView() override;
+
+  // views::TextfieldController
+  void ContentsChanged(views::Textfield* sender,
+                       const base::string16& new_contents) override;
+
+  // views::DialogDelegateView
+  bool Cancel() override;
+  bool Accept() override;
+  base::string16 GetWindowTitle() const override;
+  ui::ModalType GetModalType() const override;
+  views::View* GetInitiallyFocusedView() override;
+  bool IsDialogButtonEnabled(ui::DialogButton button) const override;
+
+  void SetExtensionName(const std::string& extension_name);

[emaxx, 2016/07/08 02:00:15]

Maybe merge all these methods into a single method (e.g. Update/Restart/Reinit),
that would accept and set all fields (just like constructor)? Because anyway the
dialog needs to be updated fully on each repetitive request.

[igorcov, 2016/07/11 15:04:06]

Done.

+  void SetDialogType(const std::string& dialog_type);
+  void SetErrorMessage(const base::string16& error_message);
+  void SetAcceptInput(const bool accept_input);
+  void SetCallback(const UserInputCallback& callback);
+
+  views::Textfield* textfield_for_testing() { return passphrase_textfield_; }
+  views::Label* error_label_for_testing() { return error_label_; }
+
+ private:
+  // This will initialize the view.
+  void Init();
+
+  UserInputCallback callback_;
+  base::string16 window_title_;
+  views::Textfield* passphrase_textfield_;

[emaxx, 2016/07/08 02:00:14]

This name is a bit misleading: there is no passphrase in the PIN dialog.
I think it's fine just to name it "textfield_" for now.

[igorcov, 2016/07/11 15:04:06]

Done.

+  views::Label* header_label_;
+  views::Label* dialog_type_label_;

[emaxx, 2016/07/08 02:00:15]

"Dialog type" is a bit vague definition; I think it's clearer to point to the
fact that it's the textfield label.

[igorcov, 2016/07/11 15:04:06]

Done.

+  views::Label* error_label_;
+  bool accept_input_;

[emaxx, 2016/07/08 02:00:14]

Have I understood it correctly that when waiting_for_request_==true, that the
user input won't be accepted regardless of the accept_input_ value? Then the
name is misleading.
Maybe "show_input_field_"?

[igorcov, 2016/07/11 15:04:06]

Done.

+  // Flag to know when Chrome submited input to extension and is waiting for new

[emaxx, 2016/07/08 02:00:15]

Better to rephrase this comment: first, "Chrome" gives no actual context here,
and, second, mentioning "extension" here breaks the code isolation a bit.

+  // request from it.
+  bool waiting_for_request_;

[emaxx, 2016/07/08 02:00:15]

I'm also thinking about what could be a more telling name here... As it's not
clear which "request" is meant here.
Maybe "disabled_" or "is_blocked_"?

[igorcov, 2016/07/11 15:04:06]

Done.

+
+  base::WeakPtrFactory<RequestPinView> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(RequestPinView);
+};
+
+}  // namespace chromeos
+
+#endif  // CHROME_BROWSER_CHROMEOS_OPTIONS_REQUEST_PIN_VIEW_H_