Remove the refcount from ScriptBadgeController, which
required me to switch it from wrapping a ScriptExecutor to observing one.
Also write a simple test to demonstrate how to test ScriptBadgeController. This was the original point of the change. The associated refactoring turned out to be more useful, but was ultimately unnecessary for the test.
BUG=133139,136301
Review URL: https://chromiumcodereview.appspot.com/10754004
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@145994 0039d316-1c4b-4281-b951-d872f2087c98
diff --git a/chrome/browser/extensions/extension_tab_helper.cc b/chrome/browser/extensions/extension_tab_helper.cc
index 983912e..563236ad 100644
--- a/chrome/browser/extensions/extension_tab_helper.cc
+++ b/chrome/browser/extensions/extension_tab_helper.cc
@@ -8,7 +8,7 @@
#include "chrome/browser/extensions/extension_service.h"
#include "chrome/browser/extensions/page_action_controller.h"
#include "chrome/browser/extensions/script_badge_controller.h"
-#include "chrome/browser/extensions/script_executor_impl.h"
+#include "chrome/browser/extensions/script_executor.h"
#include "chrome/browser/extensions/webstore_inline_installer.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/sessions/restore_tab_helper.h"
@@ -46,7 +46,7 @@
using extensions::Extension;
using extensions::PageActionController;
using extensions::ScriptBadgeController;
-using extensions::ScriptExecutorImpl;
+using extensions::ScriptExecutor;
namespace {
@@ -61,12 +61,13 @@
extension_function_dispatcher_(tab_contents->profile(), this)),
pending_web_app_action_(NONE),
tab_contents_(tab_contents),
+ script_executor_(new extensions::ScriptExecutor(
+ tab_contents->web_contents())),
active_tab_permission_manager_(tab_contents) {
if (extensions::switch_utils::AreScriptBadgesEnabled()) {
- script_badge_controller_ = new ScriptBadgeController(tab_contents);
+ location_bar_controller_.reset(new ScriptBadgeController(
+ tab_contents, script_executor_.get()));
} else {
- script_executor_.reset(
- new ScriptExecutorImpl(tab_contents->web_contents()));
location_bar_controller_.reset(new PageActionController(tab_contents));
}
registrar_.Add(this,
@@ -147,19 +148,6 @@
return &extension_app_icon_;
}
-extensions::ScriptExecutor* ExtensionTabHelper::script_executor() {
- if (script_badge_controller_.get())
- return script_badge_controller_.get();
- return script_executor_.get();
-}
-
-extensions::LocationBarController*
- ExtensionTabHelper::location_bar_controller() {
- if (script_badge_controller_.get())
- return script_badge_controller_.get();
- return location_bar_controller_.get();
-}
-
void ExtensionTabHelper::RenderViewCreated(RenderViewHost* render_view_host) {
render_view_host->Send(
new ExtensionMsg_SetTabId(render_view_host->GetRoutingID(), tab_id()));
diff --git a/chrome/browser/extensions/extension_tab_helper.h b/chrome/browser/extensions/extension_tab_helper.h
index 222c24f..00e07e4a7 100644
--- a/chrome/browser/extensions/extension_tab_helper.h
+++ b/chrome/browser/extensions/extension_tab_helper.h
@@ -111,9 +111,13 @@
return content::WebContentsObserver::web_contents();
}
- extensions::ScriptExecutor* script_executor();
+ extensions::ScriptExecutor* script_executor() {
+ return script_executor_.get();
+ }
- extensions::LocationBarController* location_bar_controller();
+ extensions::LocationBarController* location_bar_controller() {
+ return location_bar_controller_.get();
+ }
extensions::ActiveTabPermissionManager* active_tab_permission_manager() {
return &active_tab_permission_manager_;
@@ -221,12 +225,9 @@
TabContents* tab_contents_;
- // Either script_executor/location_bar_controller will have values, or
- // script_badge_controller will have a value, depending on whether the action
- // box is turned on.
scoped_ptr<extensions::ScriptExecutor> script_executor_;
+
scoped_ptr<extensions::LocationBarController> location_bar_controller_;
- scoped_refptr<extensions::ScriptBadgeController> script_badge_controller_;
extensions::ActiveTabPermissionManager active_tab_permission_manager_;
diff --git a/chrome/browser/extensions/script_badge_controller.cc b/chrome/browser/extensions/script_badge_controller.cc
index a45d026c..d1d5e96 100644
--- a/chrome/browser/extensions/script_badge_controller.cc
+++ b/chrome/browser/extensions/script_badge_controller.cc
@@ -23,9 +23,10 @@
namespace extensions {
-ScriptBadgeController::ScriptBadgeController(TabContents* tab_contents)
- : content::WebContentsObserver(tab_contents->web_contents()),
- script_executor_(tab_contents->web_contents()),
+ScriptBadgeController::ScriptBadgeController(TabContents* tab_contents,
+ ScriptExecutor* script_executor)
+ : ScriptExecutor::Observer(script_executor),
+ content::WebContentsObserver(tab_contents->web_contents()),
tab_contents_(tab_contents) {
registrar_.Add(this,
chrome::NOTIFICATION_EXTENSION_UNLOADED,
@@ -69,32 +70,8 @@
return ACTION_NONE;
}
-void ScriptBadgeController::ExecuteScript(
- const std::string& extension_id,
- ScriptExecutor::ScriptType script_type,
- const std::string& code,
- ScriptExecutor::FrameScope frame_scope,
- UserScript::RunLocation run_at,
- ScriptExecutor::WorldType world_type,
- const ExecuteScriptCallback& callback) {
- ExecuteScriptCallback this_callback = base::Bind(
- &ScriptBadgeController::OnExecuteScriptFinished,
- this,
- extension_id,
- callback);
-
- script_executor_.ExecuteScript(extension_id,
- script_type,
- code,
- frame_scope,
- run_at,
- world_type,
- this_callback);
-}
-
void ScriptBadgeController::OnExecuteScriptFinished(
const std::string& extension_id,
- const ExecuteScriptCallback& callback,
bool success,
int32 page_id,
const std::string& error) {
@@ -102,8 +79,6 @@
if (InsertExtension(extension_id))
NotifyChange();
}
-
- callback.Run(success, page_id, error);
}
ExtensionService* ScriptBadgeController::GetExtensionService() {
diff --git a/chrome/browser/extensions/script_badge_controller.h b/chrome/browser/extensions/script_badge_controller.h
index e21ee33e..ee79ca7 100644
--- a/chrome/browser/extensions/script_badge_controller.h
+++ b/chrome/browser/extensions/script_badge_controller.h
@@ -15,7 +15,6 @@
#include "base/memory/ref_counted.h"
#include "chrome/browser/extensions/location_bar_controller.h"
#include "chrome/browser/extensions/script_executor.h"
-#include "chrome/browser/extensions/script_executor_impl.h"
#include "content/public/browser/notification_observer.h"
#include "content/public/browser/notification_registrar.h"
#include "content/public/browser/web_contents_observer.h"
@@ -38,22 +37,21 @@
// - For content_script declarations, this receives IPCs from the renderer
// notifying that a content script is running (either on this tab or one of
// its frames), which is recorded.
-// - The ScriptExecutor interface is exposed for programmatically executing
-// scripts. Successfully executed scripts are recorded.
+// - Observes a ScriptExecutor so that successfully-executed scripts
+// can cause a script badge to appear.
//
// When extension IDs are recorded a NOTIFICATION_EXTENSION_LOCATION_BAR_UPDATED
// is sent, and those extensions will be returned from GetCurrentActions until
// the next page navigation.
-//
-// Ref-counted so that it can be safely bound in a base::Bind.
class ScriptBadgeController
- : public base::RefCountedThreadSafe<ScriptBadgeController>,
- public LocationBarController,
- public ScriptExecutor,
+ : public LocationBarController,
+ public ScriptExecutor::Observer,
public content::WebContentsObserver,
public content::NotificationObserver {
public:
- explicit ScriptBadgeController(TabContents* tab_contents);
+ explicit ScriptBadgeController(TabContents* tab_contents,
+ ScriptExecutor* script_executor);
+ virtual ~ScriptBadgeController();
// LocationBarController implementation.
virtual std::vector<ExtensionAction*> GetCurrentActions() const OVERRIDE;
@@ -61,27 +59,13 @@
int mouse_button) OVERRIDE;
virtual void NotifyChange() OVERRIDE;
- // ScriptExecutor implementation.
- virtual void ExecuteScript(const std::string& extension_id,
- ScriptType script_type,
- const std::string& code,
- FrameScope frame_scope,
- UserScript::RunLocation run_at,
- WorldType world_type,
- const ExecuteScriptCallback& callback) OVERRIDE;
+ // ScriptExecutor::Observer implementation.
+ virtual void OnExecuteScriptFinished(const std::string& extension_id,
+ bool success,
+ int32 page_id,
+ const std::string& error) OVERRIDE;
private:
- friend class base::RefCountedThreadSafe<ScriptBadgeController>;
- virtual ~ScriptBadgeController();
-
- // Callback for ExecuteScript which if successful and for the current URL
- // records that the script is running, then calls the original callback.
- void OnExecuteScriptFinished(const std::string& extension_id,
- const ExecuteScriptCallback& callback,
- bool success,
- int32 page_id,
- const std::string& error);
-
// Gets the ExtensionService for |tab_contents_|.
ExtensionService* GetExtensionService();
@@ -111,9 +95,6 @@
// whether any change was made.
bool EraseExtension(const Extension* extension);
- // Delegate ScriptExecutorImpl for running ExecuteScript.
- ScriptExecutorImpl script_executor_;
-
// Our parent TabContents.
TabContents* tab_contents_;
diff --git a/chrome/browser/extensions/script_badge_controller_unittest.cc b/chrome/browser/extensions/script_badge_controller_unittest.cc
new file mode 100644
index 0000000..b556eec
--- /dev/null
+++ b/chrome/browser/extensions/script_badge_controller_unittest.cc
@@ -0,0 +1,125 @@
+// 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 <string>
+
+#include "base/command_line.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/message_loop.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/extension_tab_helper.h"
+#include "chrome/browser/extensions/script_badge_controller.h"
+#include "chrome/browser/extensions/test_extension_system.h"
+#include "chrome/browser/ui/tab_contents/tab_contents.h"
+#include "chrome/browser/ui/tab_contents/test_tab_contents.h"
+#include "chrome/common/chrome_notification_types.h"
+#include "chrome/common/extensions/extension.h"
+#include "chrome/common/extensions/extension_builder.h"
+#include "chrome/common/extensions/value_builder.h"
+#include "chrome/test/base/testing_profile.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/navigation_entry.h"
+#include "content/public/browser/notification_registrar.h"
+#include "content/public/browser/notification_source.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/test/test_browser_thread.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+using content::BrowserThread;
+
+namespace extensions {
+namespace {
+
+class ScriptBadgeControllerTest : public TabContentsTestHarness {
+ public:
+ ScriptBadgeControllerTest()
+ : ui_thread_(BrowserThread::UI, MessageLoop::current()) {
+ }
+
+ virtual void SetUp() {
+ // Note that this sets a PageActionController into the
+ // extension_tab_helper()->location_bar_controller() field. Do
+ // not use that for testing.
+ TabContentsTestHarness::SetUp();
+
+ TestExtensionSystem* extension_system =
+ static_cast<TestExtensionSystem*>(ExtensionSystem::Get(
+ tab_contents()->profile()));
+
+ // Create an ExtensionService so the ScriptBadgeController can find its
+ // extensions.
+ CommandLine command_line(CommandLine::NO_PROGRAM);
+ extension_service_ = extension_system->CreateExtensionService(
+ &command_line, FilePath(), false);
+
+ script_executor_.reset(new ScriptExecutor(web_contents()));
+ script_badge_controller_.reset(new ScriptBadgeController(
+ tab_contents(), script_executor_.get()));
+ }
+
+ protected:
+ ExtensionService* extension_service_;
+ scoped_ptr<ScriptExecutor> script_executor_;
+ scoped_ptr<ScriptBadgeController> script_badge_controller_;
+
+ private:
+ content::TestBrowserThread ui_thread_;
+};
+
+struct CountingNotificationObserver : public content::NotificationObserver {
+ CountingNotificationObserver() : events(0) {}
+
+ virtual void Observe(int type,
+ const content::NotificationSource& source,
+ const content::NotificationDetails& details) OVERRIDE {
+ events++;
+ }
+
+ int events;
+};
+
+TEST_F(ScriptBadgeControllerTest, ExecutionMakesBadgeVisible) {
+ content::NotificationRegistrar notification_registrar;
+
+ EXPECT_THAT(script_badge_controller_->GetCurrentActions(),
+ testing::ElementsAre());
+
+ scoped_refptr<const Extension> extension =
+ ExtensionBuilder()
+ .SetManifest(DictionaryBuilder()
+ .Set("name", "Extension with page action")
+ .Set("version", "1.0.0")
+ .Set("manifest_version", 2)
+ .Set("permissions", ListBuilder()
+ .Append("tabs"))
+ .Set("page_action", DictionaryBuilder()
+ .Set("default_title", "Hello")))
+ .Build();
+ extension_service_->AddExtension(extension);
+
+ // Establish a page id.
+ NavigateAndCommit(GURL("http://www.google.com"));
+
+ CountingNotificationObserver location_bar_updated;
+ notification_registrar.Add(
+ &location_bar_updated,
+ chrome::NOTIFICATION_EXTENSION_LOCATION_BAR_UPDATED,
+ content::Source<Profile>(tab_contents()->profile()));
+
+ // Initially, no script badges.
+ EXPECT_THAT(script_badge_controller_->GetCurrentActions(),
+ testing::ElementsAre());
+
+ script_badge_controller_->OnExecuteScriptFinished(
+ extension->id(), true,
+ tab_contents()->web_contents()->GetController().GetActiveEntry()->
+ GetPageID(),
+ "");
+ EXPECT_THAT(script_badge_controller_->GetCurrentActions(),
+ testing::ElementsAre(extension->script_badge()));
+ EXPECT_THAT(location_bar_updated.events, testing::Gt(0));
+};
+
+} // namespace
+} // namespace extensions
diff --git a/chrome/browser/extensions/script_executor_impl.cc b/chrome/browser/extensions/script_executor.cc
similarity index 75%
rename from chrome/browser/extensions/script_executor_impl.cc
rename to chrome/browser/extensions/script_executor.cc
index e8f6199c..906718a8 100644
--- a/chrome/browser/extensions/script_executor_impl.cc
+++ b/chrome/browser/extensions/script_executor.cc
@@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
-#include "chrome/browser/extensions/script_executor_impl.h"
+#include "chrome/browser/extensions/script_executor.h"
#include "base/callback.h"
#include "base/logging.h"
@@ -25,10 +25,13 @@
// corresponding response comes from the renderer, or the renderer is destroyed.
class Handler : public content::WebContentsObserver {
public:
- Handler(content::WebContents* web_contents,
+ Handler(ObserverList<ScriptExecutor::Observer>* observer_list,
+ content::WebContents* web_contents,
const ExtensionMsg_ExecuteCode_Params& params,
const ScriptExecutor::ExecuteScriptCallback& callback)
: content::WebContentsObserver(web_contents),
+ observer_list_(AsWeakPtr(observer_list)),
+ extension_id_(params.extension_id),
request_id_(params.request_id),
callback_(callback) {
content::RenderViewHost* rvh = web_contents->GetRenderViewHost();
@@ -67,24 +70,40 @@
bool success,
int32 page_id,
const std::string& error) {
+ if (observer_list_) {
+ FOR_EACH_OBSERVER(ScriptExecutor::Observer, *observer_list_,
+ OnExecuteScriptFinished(extension_id_, success,
+ page_id, error));
+ }
+
callback_.Run(success, page_id, error);
delete this;
}
+ base::WeakPtr<ObserverList<ScriptExecutor::Observer> > observer_list_;
+ std::string extension_id_;
int request_id_;
ScriptExecutor::ExecuteScriptCallback callback_;
};
} // namespace
-ScriptExecutorImpl::ScriptExecutorImpl(
- content::WebContents* web_contents)
+ScriptExecutor::Observer::Observer(ScriptExecutor* script_executor)
+ : script_executor_(*script_executor){
+ script_executor_.AddObserver(this);
+}
+
+ScriptExecutor::Observer::~Observer() {
+ script_executor_.RemoveObserver(this);
+}
+
+ScriptExecutor::ScriptExecutor(content::WebContents* web_contents)
: next_request_id_(0),
web_contents_(web_contents) {}
-ScriptExecutorImpl::~ScriptExecutorImpl() {}
+ScriptExecutor::~ScriptExecutor() {}
-void ScriptExecutorImpl::ExecuteScript(
+void ScriptExecutor::ExecuteScript(
const std::string& extension_id,
ScriptExecutor::ScriptType script_type,
const std::string& code,
@@ -102,7 +121,7 @@
params.in_main_world = (world_type == MAIN_WORLD);
// Handler handles IPCs and deletes itself on completion.
- new Handler(web_contents_, params, callback);
+ new Handler(&observer_list_, web_contents_, params, callback);
}
} // namespace extensions
diff --git a/chrome/browser/extensions/script_executor.h b/chrome/browser/extensions/script_executor.h
index 2119cc6..d81fa14 100644
--- a/chrome/browser/extensions/script_executor.h
+++ b/chrome/browser/extensions/script_executor.h
@@ -9,6 +9,7 @@
#include <string>
#include "base/callback_forward.h"
+#include "base/observer_list.h"
#include "chrome/common/extensions/user_script.h"
namespace content {
@@ -22,7 +23,9 @@
// caller when responded with ExtensionHostMsg_ExecuteCodeFinished.
class ScriptExecutor {
public:
- virtual ~ScriptExecutor() {}
+ explicit ScriptExecutor(content::WebContents* web_contents);
+
+ ~ScriptExecutor();
// The type of script being injected.
enum ScriptType {
@@ -47,19 +50,51 @@
typedef base::Callback<void(bool, int32, const std::string&)>
ExecuteScriptCallback;
+ class Observer {
+ public:
+ // Automatically observes and unobserves *script_executor on construction
+ // and destruction. *script_executor must outlive *this.
+ explicit Observer(ScriptExecutor* script_executor);
+ virtual ~Observer();
+
+ virtual void OnExecuteScriptFinished(const std::string& extension_id,
+ bool success,
+ int32 page_id,
+ const std::string& error) = 0;
+ private:
+ ScriptExecutor& script_executor_;
+ };
+
// Executes a script. The arguments match ExtensionMsg_ExecuteCode_Params in
// extension_messages.h (request_id is populated automatically).
//
// |callback| will always be called even if the IPC'd renderer is destroyed
// before a response is received (in this case the callback will be with a
// failure and appropriate error message).
- virtual void ExecuteScript(const std::string& extension_id,
- ScriptType script_type,
- const std::string& code,
- FrameScope frame_scope,
- UserScript::RunLocation run_at,
- WorldType world_type,
- const ExecuteScriptCallback& callback) = 0;
+ void ExecuteScript(const std::string& extension_id,
+ ScriptType script_type,
+ const std::string& code,
+ FrameScope frame_scope,
+ UserScript::RunLocation run_at,
+ WorldType world_type,
+ const ExecuteScriptCallback& callback);
+
+ void AddObserver(Observer* obs) {
+ observer_list_.AddObserver(obs);
+ }
+
+ void RemoveObserver(Observer* obs) {
+ observer_list_.RemoveObserver(obs);
+ }
+
+ private:
+ // The next value to use for request_id in ExtensionMsg_ExecuteCode_Params.
+ int next_request_id_;
+
+ // The WebContents this is bound to.
+ content::WebContents* web_contents_;
+
+ ObserverList<Observer> observer_list_;
};
} // namespace extensions
diff --git a/chrome/browser/extensions/script_executor_impl.h b/chrome/browser/extensions/script_executor_impl.h
deleted file mode 100644
index bf66cd4..0000000
--- a/chrome/browser/extensions/script_executor_impl.h
+++ /dev/null
@@ -1,40 +0,0 @@
-// 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 CHROME_BROWSER_EXTENSIONS_SCRIPT_EXECUTOR_IMPL_H_
-#define CHROME_BROWSER_EXTENSIONS_SCRIPT_EXECUTOR_IMPL_H_
-#pragma once
-
-#include "chrome/browser/extensions/script_executor.h"
-
-#include "base/compiler_specific.h"
-
-namespace extensions {
-
-// Standard implementation of ScriptExecutor which just sends an IPC to
-// |web_contents_| to execute the script.
-class ScriptExecutorImpl : public ScriptExecutor {
- public:
- explicit ScriptExecutorImpl(content::WebContents* web_contents);
-
- virtual ~ScriptExecutorImpl();
-
- virtual void ExecuteScript(const std::string& extension_id,
- ScriptType script_type,
- const std::string& code,
- FrameScope frame_scope,
- UserScript::RunLocation run_at,
- WorldType world_type,
- const ExecuteScriptCallback& callback) OVERRIDE;
- private:
- // The next value to use for request_id inExtensionMsg_ExecuteCode_Params.
- int next_request_id_;
-
- // The WebContents this is bound to.
- content::WebContents* web_contents_;
-};
-
-} // namespace extensions
-
-#endif // CHROME_BROWSER_EXTENSIONS_SCRIPT_EXECUTOR_IMPL_H_
diff --git a/chrome/chrome_browser_extensions.gypi b/chrome/chrome_browser_extensions.gypi
index 83b0b8e..24a52814 100644
--- a/chrome/chrome_browser_extensions.gypi
+++ b/chrome/chrome_browser_extensions.gypi
@@ -449,9 +449,8 @@
'browser/extensions/sandboxed_unpacker.h',
'browser/extensions/script_badge_controller.cc',
'browser/extensions/script_badge_controller.h',
+ 'browser/extensions/script_executor.cc',
'browser/extensions/script_executor.h',
- 'browser/extensions/script_executor_impl.cc',
- 'browser/extensions/script_executor_impl.h',
'browser/extensions/settings/leveldb_settings_storage_factory.cc',
'browser/extensions/settings/leveldb_settings_storage_factory.h',
'browser/extensions/settings/setting_sync_data.cc',
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index b640d27..0d46b3d 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -1265,6 +1265,7 @@
'browser/extensions/management_policy_unittest.cc',
'browser/extensions/process_map_unittest.cc',
'browser/extensions/sandboxed_unpacker_unittest.cc',
+ 'browser/extensions/script_badge_controller_unittest.cc',
'browser/extensions/settings/settings_frontend_unittest.cc',
'browser/extensions/settings/settings_quota_unittest.cc',
'browser/extensions/settings/settings_sync_unittest.cc',
