Encapsulate CSS selector declarative content condition tracking

chrome/browser/extensions/api/declarative_content/declarative_content_rule.h

Index: chrome/browser/extensions/api/declarative_content/declarative_content_rule.h
diff --git a/chrome/browser/extensions/api/declarative_content/declarative_content_rule.h b/chrome/browser/extensions/api/declarative_content/declarative_content_rule.h
new file mode 100644
index 0000000000000000000000000000000000000000..cc9868495deff6cff23da918f74eaae8e5c4d241
--- /dev/null
+++ b/chrome/browser/extensions/api/declarative_content/declarative_content_rule.h
@@ -0,0 +1,218 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.

[not at google - send to devlin, 2015/06/01 22:51:12]

(note, maybe of the comments in this file apply to more than just the single
line I made them on)

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_EXTENSIONS_API_DECLARATIVE_CONTENT_DECLARATIVE_CONTENT_RULE_H__
+#define CHROME_BROWSER_EXTENSIONS_API_DECLARATIVE_CONTENT_DECLARATIVE_CONTENT_RULE_H__
+
+#include <string>
+#include <vector>
+
+#include "base/callback.h"
+#include "base/memory/linked_ptr.h"
+#include "base/time/time.h"
+#include "chrome/browser/extensions/api/declarative_content/content_action.h"
+#include "chrome/browser/extensions/api/declarative_content/content_condition.h"
+#include "components/url_matcher/url_matcher.h"
+#include "extensions/common/api/events.h"
+
+namespace base {
+class Time;
+class Value;
+}
+
+namespace content {
+class BrowserContext;
+}
+
+namespace extensions {
+
+class Extension;
+
+// This class stores a set of conditions that may be part of a
+// DeclarativeContentRule.  If any condition is fulfilled, the Actions of the
+// DeclarativeContentRule can be triggered.
+class DeclarativeContentConditionSet {

[not at google - send to devlin, 2015/06/01 22:51:11]

I'm not sure that DeclarativeContentConditionSet/DeclarativeContentActionSet
belong in this file, they seem pretty complex. Possibly worth splitting off.

+ public:
+  typedef std::vector<linked_ptr<base::Value> > Values;

[not at google - send to devlin, 2015/06/01 22:51:11]

- Use "using Values = ..." rather than "typedef".
- Use ">>" not "> >".

+  typedef std::vector<linked_ptr<const ContentCondition> > Conditions;
+  typedef Conditions::const_iterator const_iterator;
+
+  // Factory method that creates a DeclarativeContentConditionSet for
+  // |extension| according to the JSON array |conditions| passed by the
+  // extension API. Sets |error| and returns NULL in case of an error.
+  static scoped_ptr<DeclarativeContentConditionSet> Create(
+      const Extension* extension,
+      url_matcher::URLMatcherConditionFactory* url_matcher_condition_factory,
+      const Values& condition_values,
+      std::string* error);
+
+  ~DeclarativeContentConditionSet();
+
+  const Conditions& conditions() const {
+    return conditions_;
+  }
+
+  const_iterator begin() const { return conditions_.begin(); }
+  const_iterator end() const { return conditions_.end(); }
+
+  // If |url_match_trigger| is not -1, this function looks for a condition
+  // with this URLMatcherConditionSet, and forwards to that condition's
+  // IsFulfilled(|match_data|). If there is no such condition, then false is
+  // returned. If |url_match_trigger| is -1, this function returns whether any
+  // of the conditions without URL attributes is satisfied.

[not at google - send to devlin, 2015/06/01 22:51:11]

Can this be 2 separate interface methods, with an Impl-style private method
(fine to call it IsFulfilled as well) - one without |url_match_trigger|, and one
insisting that |url_match_trigger| is a valid number? I think it's odd than a
single interface method would be overloaded in this way.

+  bool IsFulfilled(url_matcher::URLMatcherConditionSet::ID url_match_trigger,
+                   const RendererContentMatchData& match_data) const;
+
+  // Appends the URLMatcherConditionSet from all conditions to |condition_sets|.
+  void GetURLMatcherConditionSets(
+      url_matcher::URLMatcherConditionSet::Vector* condition_sets) const;
+
+  // Returns whether there are some conditions without UrlFilter attributes.
+  bool HasConditionsWithoutUrls() const {
+    return !conditions_without_urls_.empty();
+  }
+
+ private:
+  typedef std::map<url_matcher::URLMatcherConditionSet::ID,
+                   const ContentCondition*>
+      URLMatcherIdToCondition;
+
+  DeclarativeContentConditionSet(
+      const Conditions& conditions,
+      const URLMatcherIdToCondition& match_id_to_condition,
+      const std::vector<const ContentCondition*>& conditions_without_urls);
+
+  const URLMatcherIdToCondition match_id_to_condition_;
+  const Conditions conditions_;
+  const std::vector<const ContentCondition*> conditions_without_urls_;
+
+  DISALLOW_COPY_AND_ASSIGN(DeclarativeContentConditionSet);
+};
+
+typedef DeclarativeContentConditionSet ContentConditionSet;

[not at google - send to devlin, 2015/06/01 22:51:11]

Why not just directly call "DeclarativeContentConditionSet"
"ContentConditionSet"? They're both living in the same global namespace.

+
+// Immutable container for multiple actions.
+//
+// TODO(battre): As DeclarativeContentActionSet can become the single owner of
+// all actions, we can optimize here by making some of them singletons
+// (e.g. Cancel actions).
+class DeclarativeContentActionSet {
+ public:
+  typedef std::vector<linked_ptr<base::Value> > Values;
+  typedef std::vector<scoped_refptr<const ContentAction> > Actions;
+
+  explicit DeclarativeContentActionSet(const Actions& actions);
+  ~DeclarativeContentActionSet();
+
+  // Factory method that instantiates a DeclarativeContentActionSet for
+  // |extension| according to |actions| which represents the array of actions
+  // received from the extension API.
+  static scoped_ptr<DeclarativeContentActionSet> Create(
+      content::BrowserContext* browser_context,
+      const Extension* extension,
+      const Values& action_values,
+      std::string* error,
+      bool* bad_message);
+
+  // Rules call this method when their conditions are fulfilled.
+  void Apply(const std::string& extension_id,
+             const base::Time& extension_install_time,

[not at google - send to devlin, 2015/06/01 22:51:12]

This method taking the extension's install time seems a bit bizarre. At this
point I'm looking at header files only (a nice way to get a feel for a CL) and
I'd guess it's for ranking extensions or something? Definitely in implementation
detail territory.

DeclarativeContentActionSet is aready constructed with a BrowserContext. It
could hold onto that, then as necessary grab the install time (via
ExtensionPrefs I believe).

Also, when possible, interface methods that deal with extensions should pass in
Extensions not extension IDs.

+             ContentAction::ApplyInfo* apply_info) const;
+
+  // Rules call this method when their conditions are fulfilled, but Apply has
+  // already been called.
+  void Reapply(const std::string& extension_id,
+               const base::Time& extension_install_time,
+               ContentAction::ApplyInfo* apply_info) const;
+
+  // Rules call this method when they have stateful conditions, and those
+  // conditions stop being fulfilled.  Rules with event-based conditions (e.g. a
+  // network request happened) will never Revert() an action.
+  void Revert(const std::string& extension_id,
+              const base::Time& extension_install_time,
+              ContentAction::ApplyInfo* apply_info) const;
+
+  const Actions& actions() const { return actions_; }
+
+ private:
+  const Actions actions_;
+
+  DISALLOW_COPY_AND_ASSIGN(DeclarativeContentActionSet);
+};
+
+typedef DeclarativeContentActionSet ContentActionSet;
+
+// Representation of a rule of a declarative API:
+// https://developer.chrome.com/beta/extensions/events.html#declarative.
+// Generally a RulesRegistry will hold a collection of Rules for a given
+// declarative API and contain the logic for matching and applying them.
+class DeclarativeContentRule {
+ public:
+  typedef std::string ExtensionId;

[not at google - send to devlin, 2015/06/01 22:51:11]

This typedef already exists in extension.h.

+  typedef std::string RuleId;
+  typedef std::pair<ExtensionId, RuleId> GlobalRuleId;
+  typedef int Priority;
+  typedef DeclarativeContentConditionSet ConditionSet;
+  typedef DeclarativeContentActionSet ActionSet;
+  typedef core_api::events::Rule JsonRule;
+  typedef std::vector<std::string> Tags;

[not at google - send to devlin, 2015/06/01 22:51:11]

That's a lot of typedefs. Some of them like Priority seem like overkill? And the
value of ConditionSet/ActionSet more confusing than helpful given they're
defined (and concisely enough, I would hope) within this file?

+
+  // Checks whether the set of |conditions| and |actions| are consistent.
+  // Returns true in case of consistency and MUST set |error| otherwise.
+  typedef base::Callback<bool(const ConditionSet* conditions,
+                              const ActionSet* actions,
+                              std::string* error)> ConsistencyChecker;
+
+  DeclarativeContentRule(const GlobalRuleId& id,
+                         const Tags& tags,
+                         base::Time extension_installation_time,
+                         scoped_ptr<ConditionSet> conditions,
+                         scoped_ptr<ActionSet> actions,
+                         Priority priority);
+  ~DeclarativeContentRule();
+
+  // Creates a DeclarativeContentRule for |extension| given a json definition.
+  // The format of each condition and action's json is up to the specific
+  // ContentCondition and ContentAction.  |extension| may be NULL in tests.
+  //
+  // Before constructing the final rule, calls check_consistency(conditions,
+  // actions, error) and returns NULL if it fails.  Pass NULL if no consistency
+  // check is needed.  If |error| is empty, the translation was successful and
+  // the returned rule is internally consistent.
+  static scoped_ptr<DeclarativeContentRule> Create(
+      url_matcher::URLMatcherConditionFactory* url_matcher_condition_factory,
+      content::BrowserContext* browser_context,
+      const Extension* extension,
+      base::Time extension_installation_time,
+      linked_ptr<JsonRule> rule,
+      ConsistencyChecker check_consistency,
+      std::string* error);
+
+  const GlobalRuleId& id() const { return id_; }
+  const Tags& tags() const { return tags_; }
+  const std::string& extension_id() const { return id_.first; }
+  const ConditionSet& conditions() const { return *conditions_; }
+  const ActionSet& actions() const { return *actions_; }
+  Priority priority() const { return priority_; }
+
+  // Calls actions().Apply(extension_id(), extension_installation_time_,
+  // apply_info). This function should only be called when the conditions_ are
+  // fulfilled (from a semantic point of view; no harm is done if this function
+  // is called at other times for testing purposes).
+  void Apply(ContentAction::ApplyInfo* apply_info) const;
+
+ private:
+  GlobalRuleId id_;
+  Tags tags_;
+  base::Time extension_installation_time_;  // For precedences of rules.
+  scoped_ptr<ConditionSet> conditions_;
+  scoped_ptr<ActionSet> actions_;
+  Priority priority_;
+
+  DISALLOW_COPY_AND_ASSIGN(DeclarativeContentRule);
+};
+
+}  // namespace extensions
+
+#endif  // CHROME_BROWSER_EXTENSIONS_API_DECLARATIVE_CONTENT_DECLARATIVE_CONTENT_RULE_H__