Fix some flaky tests.

content/public/test/test_utils.h

 // Copyright (c) 2012 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 CONTENT_PUBLIC_TEST_TEST_UTILS_H_
 #define CONTENT_PUBLIC_TEST_TEST_UTILS_H_
 
 #include <memory>
 
 #include "base/callback.h"
@@ skipping 71 matching lines @@
 void IsolateAllSitesForTesting(base::CommandLine* command_line);
 
 #if defined(OS_ANDROID)
 // Registers content/browser JNI bindings necessary for some types of tests.
 bool RegisterJniForTesting(JNIEnv* env);
 #endif
 
 // Helper class to Run and Quit the message loop. Run and Quit can only happen
 // once per instance. Make a new instance for each use. Calling Quit after Run
 // has returned is safe and has no effect.
+// Note that by default Quit does not quit immediately. If that is not what you
+// really need, pass QuitMode::IMMEDIATE in the constructor.
 class MessageLoopRunner : public base::RefCounted<MessageLoopRunner> {
  public:
-  MessageLoopRunner();
+  enum class QuitMode {
+    IMMEDIATE,  // Message loop stops after finishing the current task.
+    LOOSE,  // Several generations of posted tasks are executed before stopping.
+  };
+
+  MessageLoopRunner(QuitMode mode = QuitMode::LOOSE);

[nasko, 2016/11/23 17:48:29]

Instead of making the quit type be captured for the lifetime of the runner and
be a property of it, why not have a different method to quit immediately? It
will be even more obvious at the calling site what the desired behavior is.
Something like QuitImmediately, ImmediateQuit, or something else if you have
better ideas.

[Alexander Semashko, 2016/11/23 18:38:08]

I thought the current way would be clearer; also there is no real difference
since MessageLoopRunner cannot be run more than once.

[nasko, 2016/11/24 00:00:43]

lukasza@ pointed out a different argument in favor of using the constructor - we
won't need to change any of the callsites for Quit and can mass-convert tests by
changing the default type at one place.

However, let's try to match the type name to what it does - it is a deferred
quit, so instead of LOOSE can we use DEFERRED? "LOOSE" is too loose (pun
indented) of a definition.

[Alexander Semashko, 2016/11/24 10:31:42]

Too loose name could be an additional warning for people :)
Ok, changed to DEFERRED.

 
   // Run the current MessageLoop unless the quit closure
   // has already been called.
   void Run();
 
   // Quit the matching call to Run (nested MessageLoops are unaffected).
   void Quit();
 
   // Hand this closure off to code that uses callbacks to notify completion.
   // Example:
   //   scoped_refptr<MessageLoopRunner> runner = new MessageLoopRunner;
   //   kick_off_some_api(runner->QuitClosure());
   //   runner->Run();
   base::Closure QuitClosure();
 
   bool loop_running() const { return loop_running_; }
 
  private:
   friend class base::RefCounted<MessageLoopRunner>;
   ~MessageLoopRunner();
 
+  QuitMode quit_mode_;
+
   // True when the message loop is running.
   bool loop_running_;
 
   // True after closure returned by |QuitClosure| has been called.
   bool quit_closure_called_;
 
   base::RunLoop run_loop_;
 
   base::ThreadChecker thread_checker_;
 
@@ skipping 157 matching lines @@
   void WebContentsDestroyed() override;
 
   scoped_refptr<MessageLoopRunner> message_loop_runner_;
 
   DISALLOW_COPY_AND_ASSIGN(WebContentsDestroyedWatcher);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_PUBLIC_TEST_TEST_UTILS_H_