Declarative Web Request: firstPartyForCookiesUrl

chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h

Index: chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h
diff --git a/chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h b/chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h
index 953dac3bf1b7c05002b1fc0ccd76c65bc6f8664c..6e865c6b9ae59f8e0120a6d2c96a4eac710029b6 100644
--- a/chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h
+++ b/chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h
@@ -105,13 +105,29 @@ class WebRequestRulesRegistry : public RulesRegistryWithCache {
   typedef std::map<WebRequestRule::GlobalRuleId, linked_ptr<WebRequestRule> >
       RulesMap;
 
+  // This wraps |rule_triggers_| into an operator usable with std::transform.
+  class RuleTriggersOperator {

[battre, 2012/12/06 19:53:26]

If it does something then the name should be verbal - but if we use it only
once, we should consider whether we are not adding more code by introducing the
operator than just doing the conversation ourselves in a loop.

[Jeffrey Yasskin, 2012/12/18 03:09:58]

Generally I'd put a helper class like this into the .cc file, in an anonymous
namespace.

+1 to Dominic's question: If it's not saving code, and it doesn't have an
obvious name, don't extract it. I can imagine this having an obvious name
though: it's converting a std::map into a partial function, which is a totally
clear mathematical thing to do, obscured by trying to name it after a member
variable. If it doesn't save code in the context of this file, I'd still be
skeptical even with a clear name.

[vabr (Chromium), 2013/01/21 10:57:48]

Thankfully I got rid of this sub-class completely.

On 2012/12/18 03:09:58, Jeffrey Yasskin wrote:

+   public:
+    explicit RuleTriggersOperator(const RuleTriggers& triggers);
+    ~RuleTriggersOperator();
+    const WebRequestRule* operator()(URLMatcherConditionSet::ID id) {
+      RuleTriggers::const_iterator rule_trigger = triggers_.find(id);
+      CHECK(rule_trigger != triggers_.end());
+      return rule_trigger->second;
+    }
+   private:
+    RuleTriggers triggers_;

[battre, 2012/12/06 19:53:26]

Instantiating this operator is somewhat expensive as it causes a copy here.

[Jeffrey Yasskin, 2012/12/18 03:09:58]

That is, make the member into a pointer, and construct the functor from a
pointer to make the capturing clear.

[vabr (Chromium), 2013/01/21 10:57:48]

Oh, that was a stupid mistake of mine. However, all removed by now.

On 2012/12/18 03:09:58, Jeffrey Yasskin wrote:

+  };
+
   // Map that tells us which WebRequestRule may match under the condition that
-  // the URLMatcherConditionSet::ID was returned by the |url_matcher_|.
+  // the URLMatcherConditionSet::ID was returned by the |url_matcher_| and

[Jeffrey Yasskin, 2012/12/18 03:09:58]

That is, URLMatcherConditionSet::ID must be returned from both url_matcher_ and
first_party_url_matcher_ in order for you to look it that up in rule_triggers_?
Do you always test both, or only under certain conditions? If it's not always,
mention when or refer to the place that does explain when.

[vabr (Chromium), 2013/01/21 10:57:48]

One matcher matching is enough. Corrected the "and" to "or", thanks for pointing
that out.
I always put together matches from both matchers.

+  // |first_party_url_matcher_|.
   RuleTriggers rule_triggers_;
 
   RulesMap webrequest_rules_;
 
   URLMatcher url_matcher_;
+  URLMatcher first_party_url_matcher_;
 
   scoped_refptr<ExtensionInfoMap> extension_info_map_;
 };