Enable import/export of passwords into/from Password Manager

chrome/browser/ui/webui/options/password_manager_handler.h

Index: chrome/browser/ui/webui/options/password_manager_handler.h
diff --git a/chrome/browser/ui/webui/options/password_manager_handler.h b/chrome/browser/ui/webui/options/password_manager_handler.h
index d8ed6c04d282f3f14d94055c33210c7875812362..a1a24ce2248363d3fd37a5e7a9b3644e373f4a2a 100644
--- a/chrome/browser/ui/webui/options/password_manager_handler.h
+++ b/chrome/browser/ui/webui/options/password_manager_handler.h
@@ -10,27 +10,40 @@
 #include <string>
 #include <vector>
 
+#include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "build/build_config.h"
 #include "chrome/browser/ui/passwords/password_manager_presenter.h"
 #include "chrome/browser/ui/passwords/password_ui_view.h"
 #include "chrome/browser/ui/webui/options/options_ui.h"
+#include "components/password_manager/core/browser/import/password_importer.h"
 #include "components/prefs/pref_member.h"
+#include "ui/shell_dialogs/select_file_dialog.h"
+
+class PasswordManagerHandlerTest;
 
 namespace options {
 
 // The WebUI based PasswordUIView. Displays passwords in the web ui.
 class PasswordManagerHandler : public OptionsPageUIHandler,
-                               public PasswordUIView {
+                               public PasswordUIView,
+                               public ui::SelectFileDialog::Listener {
  public:
   PasswordManagerHandler();
+  explicit PasswordManagerHandler(PasswordManagerPresenter* presenter);

[vabr (Chromium), 2016/03/10 10:53:31]

Also, please mark this constructor as "just for testing". It is just for
testing, correct?

[vabr (Chromium), 2016/03/10 10:53:32]

Please change this from taking a raw pointer to taking a scoped_ptr. That way it
will be clear that the constructor takes the ownership.

[xunlu, 2016/03/16 07:23:58]

Done.

[xunlu, 2016/03/16 07:23:58]

Done.

   ~PasswordManagerHandler() override;
 
   // OptionsPageUIHandler implementation.
   void GetLocalizedValues(base::DictionaryValue* localized_strings) override;
   void InitializeHandler() override;
+  void InitializePage() override;
   void RegisterMessages() override;
 
+  // ui::SelectFileDialog::Listener implementation
+  void FileSelected(const base::FilePath& path,
+                    int index,
+                    void* params) override;
+
   // PasswordUIView implementation.
   Profile* GetProfile() override;
   void ShowPassword(
@@ -48,6 +61,8 @@ class PasswordManagerHandler : public OptionsPageUIHandler,
   gfx::NativeWindow GetNativeWindow() const override;
 #endif
  private:
+  friend class ::PasswordManagerHandlerTest;

[vabr (Chromium), 2016/03/10 10:53:31]

What private access does the test require? If it is just to access the
presenter, that can be bypassed in the test (see my comment there).

It is generally a bad practice to let the tests access private implementation
details of the tests classes, so using a friend declaration should be the very
last option to take.

[xunlu, 2016/03/16 07:23:58]

not just the presenter. It also need to call the following method in handler
which are not public:
HandlePasswordExport(private),
ImportPasswordFileRead(private),
set_web_ui(protected)

[vabr (Chromium), 2016/03/16 17:48:28]

You should be able to call HandlePasswordExport through ProcessWebUIMessage with
the message "updatePasswordLists". That will use the same path the production
code clients would use.

ImportPasswordFileRead should get solved if you split the functionality into the
separate object, as I suggested in the other comment.

As for set_web_ui, the comment in web_ui_message_handler.h says: "exposed to
subclasses for testing purposes". So the right thing to do there would be to
create a trivial child of the handler in the test, and expose the setter:

class TestPasswordManagerHandler : public options::PasswordManagerHandler {
 public:
  using set_web_ui;
};

In summary, it looks like the friend declaration is not needed. Please try to
avoid it, even if the initial use were innocent, it tends to invite bad test
design as the time goes.

[xunlu, 2016/03/18 21:15:30]

* HandlePasswordExport() call removed.
* ImportPasswordFileRead() still pending, see my comment in
password_manager_handler.cc.
* set_web_ui() removed.

+
   // Clears and then populates the list of passwords and password exceptions.
   // Called when the JS PasswordManager object is initialized.
   void HandleUpdatePasswordLists(const base::ListValue* args);
@@ -64,11 +79,33 @@ class PasswordManagerHandler : public OptionsPageUIHandler,
   // |index| The index of the entry.
   void HandleRequestShowPassword(const base::ListValue* args);
 
+  // Import from CSV/JSON file. The steps are:
+  //   1. user click import button -> HandlePasswordImport() ->
+  //   start file selector
+  //   2. user selects file -> ImportPasswordFileSeleted() -> read to memory
+  //   3. read completes -> ImportPasswordFileRead() -> store to PasswordStore
+  void HandlePasswordImport(const base::ListValue* args);
+  void ImportPasswordFileSelected(const base::FilePath& path);
+  void ImportPasswordFileRead(password_manager::PasswordImporter::Result result,
+                              const std::vector<autofill::PasswordForm>& forms);
+
+  // Export to CSV/JSON file. The steps are:
+  //   1. user click export button -> HandlePasswordExport() ->
+  //   check OS password if necessary -> start file selector
+  //   2. user selects file -> ExportPasswordFileSeleted() ->
+  //   write to memory buffer -> start write operation
+  //   3. Writing completes -> completion callback(empty for now);

[vabr (Chromium), 2016/03/10 10:53:31]

nit: It does not make sense to speak about the unused completion callback here,
it's an implementation detail not relevant to the high-level export flow.

[xunlu, 2016/03/16 07:23:58]

Done.

+  void HandlePasswordExport(const base::ListValue* args);
+  void ExportPasswordFileSelected(const base::FilePath& path);
+
   // User pref for storing accept languages.
   std::string languages_;
 
   // The PasswordManagerPresenter object owned by the this view.
-  PasswordManagerPresenter password_manager_presenter_;
+  scoped_ptr<PasswordManagerPresenter> password_manager_presenter_;
+
+  // Handle for file picker
+  scoped_refptr<ui::SelectFileDialog> selected_file_dialog_;
 
   DISALLOW_COPY_AND_ASSIGN(PasswordManagerHandler);
 };