Move the PageAction parts of ExtensionTabHelper::DidNavigateMainFrame to the PageActionController.

Test that the page actions are reset on navigation. Review URL: https://chromiumcodereview.appspot.com/10685007 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@145054 0039d316-1c4b-4281-b951-d872f2087c98
author: jyasskin@chromium.org <jyasskin@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> 2012-06-30 02:52:07 +0000
committer: jyasskin@chromium.org <jyasskin@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> 2012-06-30 02:52:07 +0000
commit: 466f5e29eba257623962f12295fa994aac3b81a3 (patch)
tree: 978e30934ddf8d9038b5cec2306494ed057f22e0
parent: c5a970ce1208bb4f10ab2bee4c631493d3f78bec (diff)
download: chromium_src-466f5e29eba257623962f12295fa994aac3b81a3.zip
chromium_src-466f5e29eba257623962f12295fa994aac3b81a3.tar.gz
chromium_src-466f5e29eba257623962f12295fa994aac3b81a3.tar.bz2
11 files changed, 426 insertions, 13 deletions
diff --git a/chrome/browser/extensions/extension_tab_helper.cc b/chrome/browser/extensions/extension_tab_helper.cc
index 3195913..577f048 100644
--- a/chrome/browser/extensions/extension_tab_helper.cc
+++ b/chrome/browser/extensions/extension_tab_helper.cc
@@ -154,15 +154,6 @@ void ExtensionTabHelper::DidNavigateMainFrame(
           content::Source<ExtensionAction>(browser_action),
           content::NotificationService::NoDetails());
     }
-
-    // TODO(jyasskin): Have PageActionController observe DidNavigateMainFrame,
-    // and move this code there.
-    ExtensionAction* page_action = (*it)->page_action();
-    if (page_action) {
-      page_action->ClearAllValuesForTab(
-          tab_contents_->restore_tab_helper()->session_id().id());
-      location_bar_controller()->NotifyChange();
-    }
   }
 }
 
diff --git a/chrome/browser/extensions/page_action_controller.cc b/chrome/browser/extensions/page_action_controller.cc
index 7d4877a..2d310c2 100644
--- a/chrome/browser/extensions/page_action_controller.cc
+++ b/chrome/browser/extensions/page_action_controller.cc
@@ -19,7 +19,8 @@
 namespace extensions {
 
 PageActionController::PageActionController(TabContents* tab_contents)
-    : tab_contents_(tab_contents) {}
+    : content::WebContentsObserver(tab_contents->web_contents()),
+      tab_contents_(tab_contents) {}
 
 PageActionController::~PageActionController() {}
 
@@ -81,6 +82,22 @@ void PageActionController::NotifyChange() {
       content::INVALIDATE_TYPE_PAGE_ACTIONS);
 }
 
+void PageActionController::DidNavigateMainFrame(
+    const content::LoadCommittedDetails& details,
+    const content::FrameNavigateParams& params) {
+  const std::vector<ExtensionAction*> current_actions = GetCurrentActions();
+
+  if (current_actions.empty())
+    return;
+
+  for (size_t i = 0; i < current_actions.size(); ++i) {
+    current_actions[i]->ClearAllValuesForTab(
+        tab_contents_->extension_tab_helper()->tab_id());
+  }
+
+  NotifyChange();
+}
+
 ExtensionService* PageActionController::GetExtensionService() const {
   return ExtensionSystem::Get(tab_contents_->profile())->extension_service();
 }
diff --git a/chrome/browser/extensions/page_action_controller.h b/chrome/browser/extensions/page_action_controller.h
index e9afaf0..8c9941d 100644
--- a/chrome/browser/extensions/page_action_controller.h
+++ b/chrome/browser/extensions/page_action_controller.h
@@ -11,6 +11,7 @@
 
 #include "base/observer_list.h"
 #include "chrome/browser/extensions/location_bar_controller.h"
+#include "content/public/browser/web_contents_observer.h"
 
 class ExtensionService;
 class TabContents;
@@ -19,7 +20,8 @@ namespace extensions {
 
 // A LocationBarController which populates the location bar with icons based
 // on the page_action extension API.
-class PageActionController : public LocationBarController {
+class PageActionController : public LocationBarController,
+                             public content::WebContentsObserver {
  public:
   explicit PageActionController(TabContents* tab_contents);
   virtual ~PageActionController();
@@ -30,6 +32,11 @@ class PageActionController : public LocationBarController {
                            int mouse_button) OVERRIDE;
   virtual void NotifyChange() OVERRIDE;
 
+  // content::WebContentsObserver implementation.
+  virtual void DidNavigateMainFrame(
+      const content::LoadCommittedDetails& details,
+      const content::FrameNavigateParams& params) OVERRIDE;
+
  private:
   // Gets the ExtensionService for |tab_contents_|.
   ExtensionService* GetExtensionService() const;
diff --git a/chrome/browser/extensions/page_action_controller_unittest.cc b/chrome/browser/extensions/page_action_controller_unittest.cc
new file mode 100644
index 0000000..94288d0
--- /dev/null
+++ b/chrome/browser/extensions/page_action_controller_unittest.cc
@@ -0,0 +1,88 @@
+// 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/page_action_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/extensions/extension.h"
+#include "chrome/common/extensions/extension_builder.h"
+#include "chrome/common/extensions/value_builder.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/test/test_browser_thread.h"
+
+using content::BrowserThread;
+
+namespace extensions {
+namespace {
+
+class PageActionControllerTest : public TabContentsTestHarness {
+ public:
+  PageActionControllerTest()
+      : ui_thread_(BrowserThread::UI, MessageLoop::current()) {
+  }
+
+  virtual void SetUp() {
+    TabContentsTestHarness::SetUp();
+    // Create an ExtensionService so the PageActionController can find its
+    // extensions.
+    CommandLine command_line(CommandLine::NO_PROGRAM);
+    extension_service_ =
+        static_cast<TestExtensionSystem*>(
+            ExtensionSystem::Get(tab_contents()->profile()))->
+        CreateExtensionService(
+            &command_line, FilePath(), false);
+  }
+
+ protected:
+  int tab_id() {
+    return tab_contents()->extension_tab_helper()->tab_id();
+  }
+
+  ExtensionService* extension_service_;
+
+ private:
+  content::TestBrowserThread ui_thread_;
+};
+
+TEST_F(PageActionControllerTest, NavigationClearsState) {
+  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);
+
+  NavigateAndCommit(GURL("http://www.google.com"));
+
+  extension->page_action()->SetTitle(tab_id(), "Goodbye");
+  extension->page_action()->SetPopupUrl(
+      tab_id(), extension->GetResourceURL("popup.html"));
+
+  EXPECT_EQ("Goodbye", extension->page_action()->GetTitle(tab_id()));
+  EXPECT_EQ(extension->GetResourceURL("popup.html"),
+            extension->page_action()->GetPopupUrl(tab_id()));
+
+  // Should discard the settings, and go back to the defaults.
+  NavigateAndCommit(GURL("http://www.yahoo.com"));
+
+  EXPECT_EQ("Hello", extension->page_action()->GetTitle(tab_id()));
+  EXPECT_EQ(GURL(), extension->page_action()->GetPopupUrl(tab_id()));
+};
+
+}  // namespace
+}  // namespace extensions
diff --git a/chrome/browser/extensions/test_extension_system.cc b/chrome/browser/extensions/test_extension_system.cc
index 8fbb699..51543a3 100644
--- a/chrome/browser/extensions/test_extension_system.cc
+++ b/chrome/browser/extensions/test_extension_system.cc
@@ -22,7 +22,8 @@
 
 
 TestExtensionSystem::TestExtensionSystem(Profile* profile)
-    : profile_(profile) {
+    : profile_(profile),
+      info_map_(new ExtensionInfoMap()) {
 }
 
 TestExtensionSystem::~TestExtensionSystem() {
@@ -111,7 +112,7 @@ extensions::StateStore* TestExtensionSystem::state_store() {
 }
 
 ExtensionInfoMap* TestExtensionSystem::info_map() {
-  return NULL;
+  return info_map_.get();
 }
 
 extensions::LazyBackgroundTaskQueue*
diff --git a/chrome/browser/extensions/test_extension_system.h b/chrome/browser/extensions/test_extension_system.h
index 630e857..96459e3d 100644
--- a/chrome/browser/extensions/test_extension_system.h
+++ b/chrome/browser/extensions/test_extension_system.h
@@ -71,6 +71,7 @@ class TestExtensionSystem : public ExtensionSystem {
   scoped_ptr<extensions::ManagementPolicy> management_policy_;
   scoped_ptr<ExtensionProcessManager> extension_process_manager_;
   scoped_ptr<extensions::AlarmManager> alarm_manager_;
+  scoped_refptr<ExtensionInfoMap> info_map_;
 };
 
 #endif  // CHROME_BROWSER_EXTENSIONS_TEST_EXTENSION_SYSTEM_H_
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index 03d441a..835a32e 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -177,6 +177,10 @@
         'browser/ui/panels/test_panel_mouse_watcher.h',
         'browser/ui/tab_contents/test_tab_contents.cc',
         'browser/ui/tab_contents/test_tab_contents.h',
+        'common/extensions/extension_builder.cc',
+        'common/extensions/extension_builder.h',
+        'common/extensions/value_builder.cc',
+        'common/extensions/value_builder.h',
         'common/net/gaia/mock_url_fetcher_factory.h',
         'common/pref_store_observer_mock.cc',
         'common/pref_store_observer_mock.h',
@@ -1239,6 +1243,7 @@
         'browser/extensions/extension_warning_set_unittest.cc',
         'browser/extensions/extensions_quota_service_unittest.cc',
         'browser/extensions/external_policy_extension_loader_unittest.cc',
+        'browser/extensions/page_action_controller_unittest.cc',
         'browser/extensions/permissions_updater_unittest.cc',
         'browser/extensions/file_reader_unittest.cc',
         'browser/extensions/image_loading_tracker_unittest.cc',
diff --git a/chrome/common/extensions/extension_builder.cc b/chrome/common/extensions/extension_builder.cc
new file mode 100644
index 0000000..a27a204
--- /dev/null
+++ b/chrome/common/extensions/extension_builder.cc
@@ -0,0 +1,50 @@
+// 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 "chrome/common/extensions/extension_builder.h"
+
+#include "chrome/common/extensions/extension.h"
+
+namespace extensions {
+
+ExtensionBuilder::ExtensionBuilder()
+    : location_(Extension::LOAD),
+      flags_(Extension::NO_FLAGS) {
+}
+ExtensionBuilder::~ExtensionBuilder() {}
+
+scoped_refptr<Extension> ExtensionBuilder::Build() {
+  std::string error;
+  scoped_refptr<Extension> extension = Extension::Create(
+      path_,
+      location_,
+      *manifest_,
+      flags_,
+      &error);
+  CHECK_EQ("", error);
+  return extension;
+}
+
+ExtensionBuilder& ExtensionBuilder::SetPath(const FilePath& path) {
+  path_ = path_;
+  return *this;
+}
+
+ExtensionBuilder& ExtensionBuilder::SetLocation(Extension::Location location) {
+  location_ = location;
+  return *this;
+}
+
+ExtensionBuilder& ExtensionBuilder::SetManifest(
+    scoped_ptr<DictionaryValue> manifest) {
+  manifest_ = manifest.Pass();
+  return *this;
+}
+
+ExtensionBuilder& ExtensionBuilder::AddFlags(int init_from_value_flags) {
+  flags_ |= init_from_value_flags;
+  return *this;
+}
+
+}  // namespace extensions
diff --git a/chrome/common/extensions/extension_builder.h b/chrome/common/extensions/extension_builder.h
new file mode 100644
index 0000000..c50b180
--- /dev/null
+++ b/chrome/common/extensions/extension_builder.h
@@ -0,0 +1,49 @@
+// 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_COMMON_EXTENSIONS_EXTENSION_BUILDER_H_
+#define CHROME_COMMON_EXTENSIONS_EXTENSION_BUILDER_H_
+#pragma once
+
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "chrome/common/extensions/extension.h"
+#include "chrome/common/extensions/value_builder.h"
+
+namespace extensions {
+
+// An easier way to create extensions than Extension::Create.  The
+// constructor sets up some defaults which are customized using the
+// methods.  The only method that must be called is SetManifest().
+class ExtensionBuilder {
+ public:
+  ExtensionBuilder();
+  ~ExtensionBuilder();
+
+  // Can only be called once, after which it's invalid to use the builder.
+  // CHECKs that the extension was created successfully.
+  scoped_refptr<Extension> Build();
+
+  // Defaults to FilePath().
+  ExtensionBuilder& SetPath(const FilePath& path);
+  // Defaults to Extension::LOAD.
+  ExtensionBuilder& SetLocation(Extension::Location location);
+
+  ExtensionBuilder& SetManifest(scoped_ptr<base::DictionaryValue> manifest);
+  ExtensionBuilder& SetManifest(DictionaryBuilder& manifest_builder) {
+    return SetManifest(manifest_builder.Build());
+  }
+
+  ExtensionBuilder& AddFlags(int init_from_value_flags);
+
+ private:
+  FilePath path_;
+  Extension::Location location_;
+  scoped_ptr<base::DictionaryValue> manifest_;
+  int flags_;
+};
+
+} // namespace extensions
+
+#endif  // CHROME_COMMON_EXTENSIONS_EXTENSION_BUILDER_H_
diff --git a/chrome/common/extensions/value_builder.cc b/chrome/common/extensions/value_builder.cc
new file mode 100644
index 0000000..6e3ebaf
--- /dev/null
+++ b/chrome/common/extensions/value_builder.cc
@@ -0,0 +1,106 @@
+// 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 "chrome/common/extensions/value_builder.h"
+
+#include "base/values.h"
+
+using base::DictionaryValue;
+using base::ListValue;
+
+namespace extensions {
+
+// DictionaryBuilder
+
+DictionaryBuilder::DictionaryBuilder() : dict_(new DictionaryValue) {}
+
+DictionaryBuilder::DictionaryBuilder(const DictionaryValue& init)
+    : dict_(init.DeepCopy()) {}
+
+DictionaryBuilder::~DictionaryBuilder() {}
+
+DictionaryBuilder& DictionaryBuilder::Set(const std::string& path,
+                                          int in_value) {
+  dict_->SetInteger(path, in_value);
+  return *this;
+}
+
+DictionaryBuilder& DictionaryBuilder::Set(const std::string& path,
+                                          double in_value) {
+  dict_->SetDouble(path, in_value);
+  return *this;
+}
+
+DictionaryBuilder& DictionaryBuilder::Set(const std::string& path,
+                                          const std::string& in_value) {
+  dict_->SetString(path, in_value);
+  return *this;
+}
+
+DictionaryBuilder& DictionaryBuilder::Set(const std::string& path,
+                                          const string16& in_value) {
+  dict_->SetString(path, in_value);
+  return *this;
+}
+
+DictionaryBuilder& DictionaryBuilder::Set(const std::string& path,
+                                          DictionaryBuilder& in_value) {
+  dict_->Set(path, in_value.Build().release());
+  return *this;
+}
+
+DictionaryBuilder& DictionaryBuilder::Set(const std::string& path,
+                                          ListBuilder& in_value) {
+  dict_->Set(path, in_value.Build().release());
+  return *this;
+}
+
+DictionaryBuilder& DictionaryBuilder::SetBoolean(
+    const std::string& path, bool in_value) {
+  dict_->SetBoolean(path, in_value);
+  return *this;
+}
+
+// ListBuilder
+
+ListBuilder::ListBuilder() : list_(new ListValue) {}
+ListBuilder::ListBuilder(const ListValue& init) : list_(init.DeepCopy()) {}
+ListBuilder::~ListBuilder() {}
+
+ListBuilder& ListBuilder::Append(int in_value) {
+  list_->Append(Value::CreateIntegerValue(in_value));
+  return *this;
+}
+
+ListBuilder& ListBuilder::Append(double in_value) {
+  list_->Append(Value::CreateDoubleValue(in_value));
+  return *this;
+}
+
+ListBuilder& ListBuilder::Append(const std::string& in_value) {
+  list_->Append(Value::CreateStringValue(in_value));
+  return *this;
+}
+
+ListBuilder& ListBuilder::Append(const string16& in_value) {
+  list_->Append(Value::CreateStringValue(in_value));
+  return *this;
+}
+
+ListBuilder& ListBuilder::Append(DictionaryBuilder& in_value) {
+  list_->Append(in_value.Build().release());
+  return *this;
+}
+
+ListBuilder& ListBuilder::Append(ListBuilder& in_value) {
+  list_->Append(in_value.Build().release());
+  return *this;
+}
+
+ListBuilder& ListBuilder::AppendBoolean(bool in_value) {
+  list_->Append(Value::CreateBooleanValue(in_value));
+  return *this;
+}
+
+}  // namespace extensions
diff --git a/chrome/common/extensions/value_builder.h b/chrome/common/extensions/value_builder.h
new file mode 100644
index 0000000..658d7d2
--- /dev/null
+++ b/chrome/common/extensions/value_builder.h
@@ -0,0 +1,98 @@
+// 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.
+
+// This file provides a builders for DictionaryValue and ListValue.  These
+// aren't specific to extensions and could move up to base/ if there's interest
+// from other sub-projects.
+//
+// The general pattern is to write:
+//
+//  scoped_ptr<BuiltType> result(FooBuilder()
+//                               .Set(args)
+//                               .Set(args)
+//                               .Build());
+//
+// For methods that take other built types, you can pass the builder directly
+// to the setter without calling Build():
+//
+// DictionaryBuilder().Set("key", ListBuilder()
+//                         .Append("foo").Append("bar") /* No .Build() */);
+//
+// Because of limitations in C++03, and to avoid extra copies, you can't pass a
+// just-constructed Builder into another Builder's method without setting at
+// least 1 field.
+//
+// The Build() method invalidates its builder, and returns ownership of the
+// built value.
+//
+// These objects are intended to be used as temporaries rather than stored
+// anywhere, so the use of non-const reference parameters is likely to cause
+// less confusion than usual.
+
+#ifndef CHROME_COMMON_EXTENSIONS_VALUE_BUILDER_H_
+#define CHROME_COMMON_EXTENSIONS_VALUE_BUILDER_H_
+#pragma once
+
+#include <string>
+
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/string16.h"
+#include "base/values.h"
+
+namespace extensions {
+
+class ListBuilder;
+
+class DictionaryBuilder {
+ public:
+  DictionaryBuilder();
+  explicit DictionaryBuilder(const base::DictionaryValue& init);
+  ~DictionaryBuilder();
+
+  // Can only be called once, after which it's invalid to use the builder.
+  scoped_ptr<base::DictionaryValue> Build() { return dict_.Pass(); }
+
+  DictionaryBuilder& Set(const std::string& path, int in_value);
+  DictionaryBuilder& Set(const std::string& path, double in_value);
+  DictionaryBuilder& Set(const std::string& path, const std::string& in_value);
+  DictionaryBuilder& Set(const std::string& path, const string16& in_value);
+  DictionaryBuilder& Set(const std::string& path, DictionaryBuilder& in_value);
+  DictionaryBuilder& Set(const std::string& path, ListBuilder& in_value);
+
+  // Named differently because overload resolution is too eager to
+  // convert implicitly to bool.
+  DictionaryBuilder& SetBoolean(const std::string& path, bool in_value);
+
+ private:
+  scoped_ptr<base::DictionaryValue> dict_;
+};
+
+class ListBuilder {
+ public:
+  ListBuilder();
+  explicit ListBuilder(const base::ListValue& init);
+  ~ListBuilder();
+
+  // Can only be called once, after which it's invalid to use the builder.
+  scoped_ptr<base::ListValue> Build() { return list_.Pass(); }
+
+  ListBuilder& Append(int in_value);
+  ListBuilder& Append(double in_value);
+  ListBuilder& Append(const std::string& in_value);
+  ListBuilder& Append(const string16& in_value);
+  ListBuilder& Append(DictionaryBuilder& in_value);
+  ListBuilder& Append(ListBuilder& in_value);
+
+  // Named differently because overload resolution is too eager to
+  // convert implicitly to bool.
+  ListBuilder& AppendBoolean(bool in_value);
+
+ private:
+  scoped_ptr<base::ListValue> list_;
+};
+
+} // namespace extensions
+
+#endif  // CHROME_COMMON_EXTENSIONS_VALUE_BUILDER_H_
author	jyasskin@chromium.org <jyasskin@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>	2012-06-30 02:52:07 +0000
committer	jyasskin@chromium.org <jyasskin@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>	2012-06-30 02:52:07 +0000
commit	466f5e29eba257623962f12295fa994aac3b81a3 (patch)
tree	978e30934ddf8d9038b5cec2306494ed057f22e0
parent	c5a970ce1208bb4f10ab2bee4c631493d3f78bec (diff)
download	chromium_src-466f5e29eba257623962f12295fa994aac3b81a3.zip chromium_src-466f5e29eba257623962f12295fa994aac3b81a3.tar.gz chromium_src-466f5e29eba257623962f12295fa994aac3b81a3.tar.bz2