Declarative Web Request: firstPartyForCookiesUrl

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

Index: chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc
diff --git a/chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc b/chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc
index 0e90c2d22ddd6a2d92a6302d2a31009b4f48be5d..53057cff2de91da814f5615514a76db8490e2f7f 100644
--- a/chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc
+++ b/chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc
@@ -30,40 +30,34 @@ WebRequestRulesRegistry::WebRequestRulesRegistry(Profile* profile,
     extension_info_map_ = ExtensionSystem::Get(profile)->info_map();
 }
 
-std::set<const WebRequestRule*>
-WebRequestRulesRegistry::GetMatches(
-    const DeclarativeWebRequestData& request_data) {
-  typedef std::set<const WebRequestRule*> RuleSet;
-  typedef std::set<URLMatcherConditionSet::ID> URLMatches;
-
+std::set<const WebRequestRule*> WebRequestRulesRegistry::GetMatches(
+    DeclarativeWebRequestData* request_data) {

[battre, 2013/01/23 18:56:34]

What do you think this:

struct WebRequestData {
  // The network request that is currently being processed.
  net::URLRequest* request;
  // The stage (progress) of the network request.
  RequestStage stage;
  // Additional information about requests that is not
  // available in all request stages.
  const net::HttpResponseHeaders* original_response_headers;
};

struct DeclarativeWebRequestData {
  const WebRequestData* request_data;
  std::set<URLMatcherConditionSet::ID> url_match_ids;
  std::set<URLMatcherConditionSet::ID> first_party_url_match_ids;
};

std::set<const WebRequestRule*> WebRequestRulesRegistry::GetMatches(
    const WebRequestData& request_data) {
  DeclarativeWebRequestData declarative_request_data;
  declarative_request_data.request_data = &request_data;
  declarative_request_data.url_match_ids = ...
  ...

  for (...)
    if ((*it)->conditions().IsFulfilled(-1, declarative_request_data)) {
      result.insert(*it);
    }
}

This way the we can keep GetMatched and CreateDeltas purely without side-effects
and take const references as parameters.

[Jeffrey Yasskin, 2013/01/24 00:39:17]

I don't have a strong opinion here, but I don't think WebRequestData vs
DeclarativeWebRequestData really captures the difference between these two
structs. I don't have better names though, so don't let me hold you up if you
want to split the struct.

[vabr (Chromium), 2013/01/24 18:24:08]

I thought about this, and did not dare to suggest the split, as it seemed too
complicated. But once you suggest it :), I'm for it.
I like the name of WebRequestData, but I would change DeclarativeWebRequestData
to WebRequestDataWithMatchIds. WDYT?

   RuleSet result;
-  URLMatches url_matches = url_matcher_.MatchURL(request_data.request->url());
+
+  request_data->url_match_ids =
+      url_matcher_.MatchURL(request_data->request->url());
+  request_data->first_party_url_match_ids =
+      url_matcher_.MatchURL(request_data->request->first_party_for_cookies());
 
   // 1st phase -- add all rules with some conditions without UrlFilter
   // attributes.
   for (RuleSet::const_iterator it = rules_with_untriggered_conditions_.begin();
        it != rules_with_untriggered_conditions_.end(); ++it) {
-    if ((*it)->conditions().IsFulfilled(-1, url_matches, request_data))
+    if ((*it)->conditions().IsFulfilled(-1, *request_data))
       result.insert(*it);
   }
 
   // 2nd phase -- add all rules with some conditions triggered by URL matches.
-  for (URLMatches::const_iterator url_match = url_matches.begin();
-       url_match != url_matches.end(); ++url_match) {
-    RuleTriggers::const_iterator rule_trigger = rule_triggers_.find(*url_match);
-    CHECK(rule_trigger != rule_triggers_.end());
-    if (!ContainsKey(result, rule_trigger->second) &&
-        rule_trigger->second->conditions().IsFulfilled(*url_match, url_matches,
-                                                       request_data))
-      result.insert(rule_trigger->second);
-  }
+  AddTriggeredRules(request_data->url_match_ids, *request_data, &result);
+  AddTriggeredRules(request_data->first_party_url_match_ids,
+                    *request_data, &result);
 
   return result;
 }
 
 std::list<LinkedPtrEventResponseDelta> WebRequestRulesRegistry::CreateDeltas(
     const ExtensionInfoMap* extension_info_map,
-    const DeclarativeWebRequestData& request_data,
+    DeclarativeWebRequestData* request_data,
     bool crosses_incognito) {
   if (webrequest_rules_.empty())
     return std::list<LinkedPtrEventResponseDelta>();
@@ -116,7 +110,7 @@ std::list<LinkedPtrEventResponseDelta> WebRequestRulesRegistry::CreateDeltas(
 
     std::list<LinkedPtrEventResponseDelta> rule_result;
     WebRequestAction::ApplyInfo apply_info = {
-      extension_info_map, request_data, crosses_incognito, &rule_result
+      extension_info_map, *request_data, crosses_incognito, &rule_result
     };
     rule->Apply(&apply_info);
     result.splice(result.begin(), rule_result);
@@ -300,4 +294,19 @@ bool WebRequestRulesRegistry::CheckConsistency(
   return true;
 }
 
+void WebRequestRulesRegistry::AddTriggeredRules(
+    const URLMatches& url_matches,
+    const DeclarativeWebRequestData& request_data,
+    RuleSet* result) {
+  for (URLMatches::const_iterator url_match = url_matches.begin();
+       url_match != url_matches.end(); ++url_match) {
+    RuleTriggers::const_iterator rule_trigger = rule_triggers_.find(*url_match);
+    CHECK(rule_trigger != rule_triggers_.end());
+    if (!ContainsKey(*result, rule_trigger->second) &&
+        rule_trigger->second->conditions().IsFulfilled(*url_match,
+                                                       request_data))
+      result->insert(rule_trigger->second);
+  }
+}
+
 }  // namespace extensions