[PasswordManager] Fix Flaky BrowserTest related to dynamically created

chrome/browser/password_manager/password_manager_test_base.h

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_PASSWORD_MANAGER_PASSWORD_MANAGER_TEST_BASE_H_
 #define CHROME_BROWSER_PASSWORD_MANAGER_PASSWORD_MANAGER_TEST_BASE_H_
 
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "chrome/test/base/in_process_browser_test.h"
+#include "components/password_manager/core/browser/password_store_consumer.h"
+#include "components/password_manager/core/browser/test_password_store.h"

[vabr (Chromium), 2015/08/05 08:37:35]

You do not need this.

[xunlu, 2015/08/05 18:37:13]

Done.

 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/test/test_utils.h"
 
 class NavigationObserver : public content::WebContentsObserver {
  public:
   explicit NavigationObserver(content::WebContents* web_contents);
   ~NavigationObserver() override;
 
   // Normally Wait() will not return until a main frame navigation occurs.
   // If a path is set, Wait() will return after this path has been seen,
@@ skipping 16 matching lines @@
       const content::LoadCommittedDetails& load_details) override;
 
  private:
   std::string wait_for_path_;
   bool quit_on_entry_committed_;
   scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
 
   DISALLOW_COPY_AND_ASSIGN(NavigationObserver);
 };
 
+class PasswordStoreObserver : public password_manager::PasswordStoreConsumer {

[vabr (Chromium), 2015/08/05 08:37:35]

I suggest against naming the class *Observer. It confusingly hints at
PasswordStore::Observer, which it does not use. It also does not really observe
the store (it does not see anything else than the response to its own GetLogins
request).

My suggestion: DummyStoreConsumer
because it expresses more clearly what it inherits from, what it does, and that
it does not care about the actual response from the store.

[xunlu, 2015/08/05 18:37:13]

Done.

+ public:
+  explicit PasswordStoreObserver(password_manager::TestPasswordStore*);

[vabr (Chromium), 2015/08/05 08:37:35]

Please change TestPasswordStore references to PasswordStore in all of the class
code. You are not using anything not contained in the base PasswordStore
interface.

[xunlu, 2015/08/05 18:37:13]

Done.

+  ~PasswordStoreObserver() override;
+
+  // password_manager::PasswordStoreConsumer
+  void OnGetPasswordStoreResults(
+      ScopedVector<autofill::PasswordForm> results) override;
+
+  // Wait for test_password_store_ to respond to GetLogins();
+  void Wait();

[vabr (Chromium), 2015/08/05 08:37:35]

The name is too short and vague. Also, it seems strange to pass the passwords
store to the constructor and only use it in Wait(). I would suggest:
(1) Get rid of test_password_store_ member variable, do not pass a store in the
constructor.
(2) Instead, pass the store as an argument of the current Wait() method, and do
not store that argument anywhere.
(3) Rename Wait to RequestLoginsAndWait.

[xunlu, 2015/08/05 18:37:13]

Done.

+
+ private:
+  scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
+  password_manager::TestPasswordStore* test_password_store_;
+
+  DISALLOW_COPY_AND_ASSIGN(PasswordStoreObserver);
+};
+
 // Observes the save password prompt (bubble or infobar) for a specified
 // WebContents, keeps track of whether or not it is currently shown, and allows
 // accepting saving passwords through it.
 class PromptObserver {
  public:
   virtual ~PromptObserver();
 
   // Checks if the prompt is being currently shown.
   virtual bool IsShowingPrompt() const = 0;
 
@@ skipping 56 matching lines @@
 
   // Accessors
   content::WebContents* WebContents();
   content::RenderViewHost* RenderViewHost();
 
  private:
   DISALLOW_COPY_AND_ASSIGN(PasswordManagerBrowserTestBase);
 };
 
 #endif  // CHROME_BROWSER_PASSWORD_MANAGER_PASSWORD_MANAGER_TEST_BASE_H_