Separate plugin_metadata from plugin_installer, thread-safe plugin_finder

chrome/browser/policy/policy_browsertest.cc

 // 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.
 
 #include <algorithm>
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 260 matching lines @@
 #if defined(OFFICIAL_BUILD)
   // Official builds bundle Flash.
   EXPECT_TRUE(flash);
 #else
   if (!flash)
     LOG(INFO) << "Test skipped because the Flash plugin couldn't be found.";
 #endif
   return flash;
 }
 
+void PostTaskEnablePlugin(PluginPrefs* plugin_prefs,
+                          const webkit::WebPluginInfo* plugin,
+                          bool enabled,
+                          bool* ok) {
+  plugin_prefs->EnablePlugin(enabled, plugin->path,
+                             base::Bind(CopyValueAndQuit<bool>, ok));
+}
+
 bool SetPluginEnabled(PluginPrefs* plugin_prefs,
                       const webkit::WebPluginInfo* plugin,
                       bool enabled) {
   bool ok = false;
-  plugin_prefs->EnablePlugin(enabled, plugin->path,

[Bernhard Bauer, 2012/09/24 18:06:04]

Wait, I don't understand. We call the callback synchronously (instead of posting
it), and that's why the tests were failing? Couldn't we just remove the
RunMessageLoop() call in that case? To depend on whether we really end up
posting a task to the message loop or not seems kinda clunky.

[ibraaaa, 2012/09/24 18:18:36]

Yes, that is the reason we are failing.

Do you mean updating SetPluginEnabled to run the message loop only when
PluginPrefs#EnablePlugin posts a task?

If yes, then we can but I didn't do it because we will need to update the test
code below to pass in the expected value to SetPluginEnabled + create a special
CopyValueAndQuit to be used for plugin policy tests only that quits the message
loop based on the result, is this ok for you?

On 2012/09/24 18:06:04, Bernhard Bauer wrote:

[Bernhard Bauer, 2012/09/24 18:27:31]

Hm, what I meant was that whether PluginPrefs::EnablePlugin() posts a task or
not is an implementation detail, and the test should not depend on it.

OTOH, maybe we should simplify the behavior of EnablePlugin() and just make sure
that we always post the callback, even if we could do it synchronously. WDYT?

[ibraaaa, 2012/09/24 18:31:26]

Well, I am fine but why do we add a needless callback to the browser code if we
can adapt the tests:), WDYT?

On 2012/09/24 18:27:31, Bernhard Bauer wrote:

[ibraaaa, 2012/09/24 20:07:13]

Done.

-                             base::Bind(CopyValueAndQuit<bool>, &ok));
+  MessageLoop::current()->PostTask(FROM_HERE,
+                                   base::Bind(&PostTaskEnablePlugin,
+                                              base::Unretained(plugin_prefs),
+                                              base::Unretained(plugin),
+                                              enabled,
+                                              base::Unretained(&ok)));
   content::RunMessageLoop();
   return ok;
 }
 
 int CountPluginsOnIOThread() {
   int count = 0;
   for (content::BrowserChildProcessHostIterator iter; !iter.Done(); ++iter) {
     if (iter.GetData().type == content::PROCESS_TYPE_PLUGIN ||
         iter.GetData().type == content::PROCESS_TYPE_PPAPI_PLUGIN) {
       count++;
@@ skipping 1104 matching lines @@
 INSTANTIATE_TEST_CASE_P(
     RestoreOnStartupPolicyTestInstance,
     RestoreOnStartupPolicyTest,
     testing::Values(&RestoreOnStartupPolicyTest::HomepageIsNotNTP,
                     &RestoreOnStartupPolicyTest::HomepageIsNTP,
                     &RestoreOnStartupPolicyTest::ListOfURLs,
                     &RestoreOnStartupPolicyTest::NTP,
                     &RestoreOnStartupPolicyTest::Last));
 
 }  // namespace policy