[Extensions Bindings] Introduce a supportsLazyListeners property

extensions/renderer/bindings/api_binding.cc

 // Copyright 2016 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 "extensions/renderer/bindings/api_binding.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/logging.h"
@@ skipping 89 matching lines @@
   APIBinding::HandlerCallback callback;
 };
 
 // TODO(devlin): Maybe separate EventData into two classes? Rules, actions, and
 // conditions should never be present on vanilla events.
 struct APIBinding::EventData {
   EventData(std::string exposed_name,
             std::string full_name,
             bool supports_filters,
             bool supports_rules,
+            bool supports_lazy_listeners,
             int max_listeners,
             bool notify_on_change,
             std::vector<std::string> actions,
             std::vector<std::string> conditions,
             APIBinding* binding)
       : exposed_name(std::move(exposed_name)),
         full_name(std::move(full_name)),
         supports_filters(supports_filters),
         supports_rules(supports_rules),
+        supports_lazy_listeners(supports_lazy_listeners),
         max_listeners(max_listeners),
         notify_on_change(notify_on_change),
         actions(std::move(actions)),
         conditions(std::move(conditions)),
         binding(binding) {}
 
   // 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;
 
   // Whether the event supports rules.
   bool supports_rules;
 
+  // Whether the event supports lazy listeners.
+  bool supports_lazy_listeners;
+
   // The maximum number of listeners this event supports.
   int max_listeners;
 
   // Whether to notify the browser of listener changes.
   bool notify_on_change;
 
   // The associated actions and conditions for declarative events.
   std::vector<std::string> actions;
   std::vector<std::string> conditions;
 
@@ skipping 128 matching lines @@
           base::StringPrintf("%s.%s", api_name_.c_str(), name.c_str());
       const base::ListValue* filters = nullptr;
       bool supports_filters =
           event_dict->GetList("filters", &filters) && !filters->empty();
 
       std::vector<std::string> rule_actions;
       std::vector<std::string> rule_conditions;
       const base::DictionaryValue* options = nullptr;
       bool supports_rules = false;
       bool notify_on_change = true;
+      bool supports_lazy_listeners = true;
       int max_listeners = binding::kNoListenerMax;
       if (event_dict->GetDictionary("options", &options)) {
         bool temp_supports_filters = false;
         // TODO(devlin): For some reason, schemas indicate supporting filters
         // either through having a 'filters' property *or* through having
         // a 'supportsFilters' property. We should clean that up.
         supports_filters |=
             (options->GetBoolean("supportsFilters", &temp_supports_filters) &&
              temp_supports_filters);
         if (options->GetBoolean("supportsRules", &supports_rules) &&
@@ skipping 12 matching lines @@
             }
           };
           get_values("actions", &rule_actions);
           get_values("conditions", &rule_conditions);
         }
 
         options->GetInteger("maxListeners", &max_listeners);
         bool unmanaged = false;
         if (options->GetBoolean("unmanaged", &unmanaged))
           notify_on_change = !unmanaged;
+        bool temp_supports_lazy_listeners = false;

[jbroman, 2017/07/10 15:33:02]

nit: Why is it so bad that we have to prevent "supportsLazyListeners: true" from
being specified that we force it to false (which is neither default nor what was
specified) in that case? It seems simpler to do:

if (options->GetBoolean("supportsLazyListeners", &supports_lazy_listeners)) {
  DCHECK(!supports_lazy_listeners)
      << "Don't specify supportsLazyListeners: true; it's the default.";
}

[lazyboy, 2017/07/14 00:04:42]

I like it this way though: API schema owners need not worry about lazy listeners
and extension system takes care of the default: listeners from lazy context
would become lazy automatically. The slight deviation is only for webview and we
should not expect usages of this outside of webview events.

 It seems simpler to do:

[Devlin, 2017/07/14 15:37:54]

I think jbroman@ was just saying that it seems excessive that we have a temp
variable here to guard against supports_lazy_listeners ever, ever being
specified as true (as opposed to *still* DCHECKing, but taking out the extra
temp variable).  I think it's a fair point - specifying true isn't the end of
the world, so as long as we still DCHECK (hopefully folks test! :P), it should
be fine.

[lazyboy, 2017/07/17 23:38:50]

Sure, I thought it was to consider removing the restriction, this SG.
  I think it's a fair point - specifying true isn't the end of

+        if (options->GetBoolean("supportsLazyListeners",
+                                &temp_supports_lazy_listeners)) {
+          DCHECK(!temp_supports_lazy_listeners)
+              << "Don't specify supportsLazyListeners: true; it's the default.";
+          supports_lazy_listeners = false;
+        }
       }
 
       events_.push_back(base::MakeUnique<EventData>(
           std::move(name), std::move(full_name), supports_filters,
-          supports_rules, max_listeners, notify_on_change,
-          std::move(rule_actions), std::move(rule_conditions), this));
+          supports_rules, supports_lazy_listeners, max_listeners,
+          notify_on_change, std::move(rule_actions), std::move(rule_conditions),
+          this));
     }
   }
 }
 
 APIBinding::~APIBinding() {}
 
 v8::Local<v8::Object> APIBinding::CreateInstance(
     v8::Local<v8::Context> context) {
   DCHECK(IsContextValid(context));
   v8::Isolate* isolate = context->GetIsolate();
@@ skipping 161 matching lines @@
   } else if (event_data->supports_rules) {
     gin::Handle<DeclarativeEvent> event = gin::CreateHandle(
         isolate, new DeclarativeEvent(
                      event_data->full_name, event_data->binding->type_refs_,
                      event_data->binding->request_handler_, event_data->actions,
                      event_data->conditions, 0));
     retval = event.ToV8();
   } else {
     retval = event_data->binding->event_handler_->CreateEventInstance(
         event_data->full_name, event_data->supports_filters,
-        event_data->max_listeners, event_data->notify_on_change, context);
+        event_data->supports_lazy_listeners, event_data->max_listeners,
+        event_data->notify_on_change, context);
   }
   info.GetReturnValue().Set(retval);
 }
 
 void APIBinding::GetCustomPropertyObject(
     v8::Local<v8::Name> property_name,
     const v8::PropertyCallbackInfo<v8::Value>& info) {
   v8::Isolate* isolate = info.GetIsolate();
   v8::HandleScope handle_scope(isolate);
   v8::Local<v8::Context> context = info.Holder()->CreationContext();
@@ skipping 107 matching lines @@
     arguments->ThrowTypeError(api_errors::InvocationError(
         name, signature->GetExpectedSignature(), error));
     return;
   }
 
   request_handler_->StartRequest(context, name, std::move(converted_arguments),
                                  callback, custom_callback, thread);
 }
 
 }  // namespace extensions