[Reland][Extensions Bindings] Add support for filtered events

Some extension API events support "filtering", which allows extensions to only receive event notifications for a subset of the total events. For instance, the webNavigation API allows an extension to specify a set of url filters in order to specify which urls the listener is interested in, and not be notified of every navigation. Expand the native event bindings to support filtering for applicable events. Abstract out an APIEventListeners class to handle adding, removing, and querying event listeners, including retrieving those for certain filtering info. This currently has two implementations - FilteredEventListeners and UnfilteredFilteredEventListeners. Add tests for the same. Expand the end-to-end test with webNavigation events to test filtered events. Fix for reland: change the url in the web navigation test and allow old urls from previous test cases. BUG=653596 Review-Url: https://codereview.chromium.org/2768093002 Cr-Commit-Position: refs/heads/master@{#458962}
Denger-Network · Mar 23, 2017 · ddc50bc · ddc50bc
1 parent 69ae14a
commit ddc50bc
Show file tree

Hide file tree

Showing 30 changed files with 1,180 additions and 176 deletions.
diff --git a/chrome/test/data/extensions/api_test/native_bindings/extension/background.js b/chrome/test/data/extensions/api_test/native_bindings/extension/background.js
@@ -206,6 +206,54 @@ var tests = [
       chrome.test.succeed();
     });
   },
+  function testWebNavigationAndFilteredEvents() {
+    // Tests unfiltered events, which can be exercised with the webNavigation
+    // API.
+    var unfiltered = new Promise((resolve, reject) => {
+      var sawSimple1 = false;
+      var sawSimple2 = false;
+      chrome.webNavigation.onBeforeNavigate.addListener(
+          function listener(details) {
+        // We create a bunch of tabs in other tests, which can potentially
+        // show up here. If this becomes too much of a problem, we can isolate
+        // these tests further, but for now, just using a unique url should be
+        // sufficient.
+        if (details.url.indexOf('unique') == -1)
+          return;
+        if (details.url.indexOf('simple.html') != -1)
+          sawSimple1 = true;
+        else if (details.url.indexOf('simple2.html') != -1)
+          sawSimple2 = true;
+        else
+          chrome.test.fail(details.url);
+
+        if (sawSimple1 && sawSimple2) {
+          chrome.webNavigation.onBeforeNavigate.removeListener(listener);
+          resolve();
+        }
+      });
+    });
+
+    var filtered = new Promise((resolve, reject) => {
+      chrome.webNavigation.onBeforeNavigate.addListener(
+          function listener(details) {
+        chrome.test.assertTrue(details.url.indexOf('unique') != -1);
+        chrome.test.assertTrue(details.url.indexOf('simple2.html') != -1,
+                               details.url);
+        chrome.webNavigation.onBeforeNavigate.removeListener(listener);
+        resolve();
+      }, {url: [{pathContains: 'simple2.html'}]});
+    });
+
+    var url1 =
+        'http://unique.com:' + portNumber + '/native_bindings/simple.html';
+    var url2 =
+        'http://unique.com:' + portNumber + '/native_bindings/simple2.html';
+    chrome.tabs.create({url: url1});
+    chrome.tabs.create({url: url2});
+
+    Promise.all([unfiltered, filtered]).then(() => { chrome.test.succeed(); });
+  },
 ];
 
 chrome.test.getConfig(config => {

diff --git a/chrome/test/data/extensions/api_test/native_bindings/extension/manifest.json b/chrome/test/data/extensions/api_test/native_bindings/extension/manifest.json
@@ -5,7 +5,7 @@
   "manifest_version": 2,
   "version": "0.1",
   "permissions": ["idle", "tabs", "cast.streaming", "*://example.com:*/*",
-                  "storage", "privacy"],
+                  "storage", "privacy", "webNavigation"],
   "background": {
     "persistent": false,
     "page": "background.html"

diff --git a/chrome/test/data/extensions/api_test/native_bindings/simple2.html b/chrome/test/data/extensions/api_test/native_bindings/simple2.html
@@ -0,0 +1,6 @@
+<!doctype html>
+<html>
+<body>
+Simple 2
+</body>
+</html>
diff --git a/extensions/common/event_filtering_info.cc b/extensions/common/event_filtering_info.cc
@@ -12,13 +12,40 @@
 
 namespace extensions {
 
+namespace {
+
+const char kInstanceId[] = "instanceId";
+const char kServiceType[] = "serviceType";
+const char kWindowType[] = "windowType";
+const char kWindowExposedByDefault[] = "windowExposedByDefault";
+
+}
+
 EventFilteringInfo::EventFilteringInfo()
     : has_url_(false),
       has_instance_id_(false),
       instance_id_(0),
       has_window_type_(false),
       has_window_exposed_by_default_(false) {}
 
+EventFilteringInfo::EventFilteringInfo(const base::DictionaryValue& dict)
+    : EventFilteringInfo() {
+  std::string url;
+  if (dict.GetString("url", &url)) {
+    GURL maybe_url(url);
+    if (maybe_url.is_valid()) {
+      has_url_ = true;
+      url_.Swap(&maybe_url);
+    }
+  }
+
+  has_instance_id_ = dict.GetInteger(kInstanceId, &instance_id_);
+  dict.GetString(kServiceType, &service_type_);
+  has_window_type_ = dict.GetString(kWindowType, &window_type_);
+  has_window_exposed_by_default_ =
+      dict.GetBoolean(kWindowExposedByDefault, &window_exposed_by_default_);
+}
+
 EventFilteringInfo::EventFilteringInfo(const EventFilteringInfo& other) =
     default;
 
@@ -51,16 +78,16 @@ std::unique_ptr<base::DictionaryValue> EventFilteringInfo::AsValue() const {
     result->SetString("url", url_.spec());
 
   if (has_instance_id_)
-    result->SetInteger("instanceId", instance_id_);
+    result->SetInteger(kInstanceId, instance_id_);
 
   if (!service_type_.empty())
-    result->SetString("serviceType", service_type_);
+    result->SetString(kServiceType, service_type_);
 
   if (has_window_type_)
-    result->SetString("windowType", window_type_);
+    result->SetString(kWindowType, window_type_);
 
   if (has_window_exposed_by_default_)
-    result->SetBoolean("windowExposedByDefault", window_exposed_by_default_);
+    result->SetBoolean(kWindowExposedByDefault, window_exposed_by_default_);
 
   return result;
 }

diff --git a/extensions/common/event_filtering_info.h b/extensions/common/event_filtering_info.h
@@ -26,6 +26,7 @@ namespace extensions {
 class EventFilteringInfo {
  public:
   EventFilteringInfo();
+  explicit EventFilteringInfo(const base::DictionaryValue& dict);
   EventFilteringInfo(const EventFilteringInfo& other);
   ~EventFilteringInfo();
   void SetWindowExposedByDefault(bool exposed);

diff --git a/extensions/renderer/BUILD.gn b/extensions/renderer/BUILD.gn
@@ -32,6 +32,8 @@ source_set("renderer") {
     "api_definitions_natives.h",
     "api_event_handler.cc",
     "api_event_handler.h",
+    "api_event_listeners.cc",
+    "api_event_listeners.h",
     "api_last_error.cc",
     "api_last_error.h",
     "api_request_handler.cc",
@@ -295,6 +297,7 @@ source_set("unit_tests") {
     "api_bindings_system_unittest.cc",
     "api_bindings_system_unittest.h",
     "api_event_handler_unittest.cc",
+    "api_event_listeners_unittest.cc",
     "api_last_error_unittest.cc",
     "api_request_handler_unittest.cc",
     "api_test_base.cc",

diff --git a/extensions/renderer/api_binding.cc b/extensions/renderer/api_binding.cc
@@ -90,15 +90,19 @@ struct APIBinding::MethodData {
 struct APIBinding::EventData {
   EventData(std::string exposed_name,
             std::string full_name,
+            bool supports_filters,
             APIEventHandler* event_handler)
       : exposed_name(std::move(exposed_name)),
         full_name(std::move(full_name)),
+        supports_filters(supports_filters),
         event_handler(event_handler) {}
 
   // The name of the event on the API object (e.g. onCreated).
   std::string exposed_name;
   // The fully-specified name of the event (e.g. tabs.onCreated).
   std::string full_name;
+  // Whether the event supports filters.
+  bool supports_filters;
   // The associated event handler. This raw pointer is safe because the
   // EventData is only accessed from the callbacks associated with the
   // APIBinding, and both the APIBinding and APIEventHandler are owned by the
@@ -214,8 +218,12 @@ APIBinding::APIBinding(const std::string& api_name,
       CHECK(event_dict->GetString("name", &name));
       std::string full_name =
           base::StringPrintf("%s.%s", api_name_.c_str(), name.c_str());
-      events_.push_back(base::MakeUnique<EventData>(
-          std::move(name), std::move(full_name), event_handler));
+      const base::ListValue* filters = nullptr;
+      bool supports_filters =
+          event_dict->GetList("filters", &filters) && !filters->empty();
+      events_.push_back(
+          base::MakeUnique<EventData>(std::move(name), std::move(full_name),
+                                      supports_filters, event_handler));
     }
   }
 }
@@ -375,7 +383,7 @@ void APIBinding::GetEventObject(
   auto* event_data =
       static_cast<EventData*>(info.Data().As<v8::External>()->Value());
   info.GetReturnValue().Set(event_data->event_handler->CreateEventInstance(
-      event_data->full_name, context));
+      event_data->full_name, event_data->supports_filters, context));
 }
 
 void APIBinding::GetCustomPropertyObject(

diff --git a/extensions/renderer/api_binding_js_util.cc b/extensions/renderer/api_binding_js_util.cc
@@ -75,11 +75,10 @@ void APIBindingJSUtil::RegisterEventArgumentMassager(
   event_handler_->RegisterArgumentMassager(context, event_name, massager);
 }
 
-void APIBindingJSUtil::CreateCustomEvent(
-    gin::Arguments* arguments,
-    v8::Local<v8::Value> v8_event_name,
-    v8::Local<v8::Value> unused_schema,
-    v8::Local<v8::Value> unused_event_options) {
+void APIBindingJSUtil::CreateCustomEvent(gin::Arguments* arguments,
+                                         v8::Local<v8::Value> v8_event_name,
+                                         v8::Local<v8::Value> unused_schema,
+                                         bool supports_filters) {
   v8::Isolate* isolate = arguments->isolate();
   v8::HandleScope handle_scope(isolate);
   v8::Local<v8::Object> holder;
@@ -95,11 +94,16 @@ void APIBindingJSUtil::CreateCustomEvent(
     event_name = gin::V8ToString(v8_event_name);
   }
 
+  DCHECK(!supports_filters || !event_name.empty())
+      << "Events that support filters cannot be anonymous.";
+
   v8::Local<v8::Value> event;
-  if (event_name.empty())
+  if (event_name.empty()) {
     event = event_handler_->CreateAnonymousEventInstance(context);
-  else
-    event = event_handler_->CreateEventInstance(event_name, context);
+  } else {
+    event = event_handler_->CreateEventInstance(event_name, supports_filters,
+                                                context);
+  }
 
   arguments->Return(event);
 }

diff --git a/extensions/renderer/api_binding_js_util.h b/extensions/renderer/api_binding_js_util.h
@@ -51,12 +51,14 @@ class APIBindingJSUtil final : public gin::Wrappable<APIBindingJSUtil> {
 
   // A handler to allow custom bindings to create custom extension API event
   // objects (e.g. foo.onBar).
-  // TODO(devlin): Currently, we ignore schema and options. We'll need to take
-  // at least options into account.
+  // Note: The JS version allows for constructing declarative events; it's
+  // unclear if we'll need to support this.
+  // TODO(devlin): Currently, we ignore schema. We may want to take it into
+  // account.
   void CreateCustomEvent(gin::Arguments* arguments,
                          v8::Local<v8::Value> v8_event_name,
                          v8::Local<v8::Value> unused_schema,
-                         v8::Local<v8::Value> unused_event_options);
+                         bool supports_filters);
 
   // Invalidates an event, removing its listeners and preventing any more from
   // being added.

diff --git a/extensions/renderer/api_binding_unittest.cc b/extensions/renderer/api_binding_unittest.cc
@@ -122,6 +122,7 @@ bool AllowAllAPIs(const std::string& name) {
 
 void OnEventListenersChanged(const std::string& event_name,
                              binding::EventListenersChanged change,
+                             const base::DictionaryValue* filter,
                              v8::Local<v8::Context> context) {}
 
 }  // namespace

diff --git a/extensions/renderer/api_bindings_system.cc b/extensions/renderer/api_bindings_system.cc
@@ -103,8 +103,9 @@ void APIBindingsSystem::CompleteRequest(int request_id,
 
 void APIBindingsSystem::FireEventInContext(const std::string& event_name,
                                            v8::Local<v8::Context> context,
-                                           const base::ListValue& response) {
-  event_handler_.FireEventInContext(event_name, context, response);
+                                           const base::ListValue& response,
+                                           const EventFilteringInfo& filter) {
+  event_handler_.FireEventInContext(event_name, context, response, filter);
 }
 
 APIBindingHooks* APIBindingsSystem::GetHooksForAPI(

diff --git a/extensions/renderer/api_bindings_system.h b/extensions/renderer/api_bindings_system.h
@@ -67,7 +67,8 @@ class APIBindingsSystem {
   // listeners.
   void FireEventInContext(const std::string& event_name,
                           v8::Local<v8::Context> context,
-                          const base::ListValue& response);
+                          const base::ListValue& response,
+                          const EventFilteringInfo& filter);
 
   // Returns the APIBindingHooks object for the given api to allow for
   // registering custom hooks. These must be registered *before* the

diff --git a/extensions/renderer/api_bindings_system_unittest.cc b/extensions/renderer/api_bindings_system_unittest.cc
@@ -10,6 +10,7 @@
 #include "base/stl_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/values.h"
+#include "extensions/common/event_filtering_info.h"
 #include "extensions/common/extension_api.h"
 #include "extensions/renderer/api_binding.h"
 #include "extensions/renderer/api_binding_hooks.h"
@@ -158,6 +159,7 @@ void APIBindingsSystemTest::OnAPIRequest(
 void APIBindingsSystemTest::OnEventListenersChanged(
     const std::string& event_name,
     binding::EventListenersChanged changed,
+    const base::DictionaryValue* filter,
     v8::Local<v8::Context> context) {}
 
 void APIBindingsSystemTest::ValidateLastRequest(
@@ -267,7 +269,7 @@ TEST_F(APIBindingsSystemTest, TestInitializationAndCallbacks) {
     std::unique_ptr<base::ListValue> expected_args =
         ListValueFromString(kResponseArgsJson);
     bindings_system()->FireEventInContext("alpha.alphaEvent", context,
-                                          *expected_args);
+                                          *expected_args, EventFilteringInfo());
 
     EXPECT_EQ(ReplaceSingleQuotes(kResponseArgsJson),
               GetStringPropertyFromObject(context->Global(), context,

diff --git a/extensions/renderer/api_bindings_system_unittest.h b/extensions/renderer/api_bindings_system_unittest.h
@@ -57,6 +57,7 @@ class APIBindingsSystemTest : public APIBindingTest {
   // Callback for event listeners changing.
   void OnEventListenersChanged(const std::string& event_name,
                                binding::EventListenersChanged changed,
+                               const base::DictionaryValue* filter,
                                v8::Local<v8::Context> context);
 
   // Callback for an API request being made. Stores the request in