Consider the frame structure in Matches()

test/FilterEngine.cpp

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2013 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 184 matching lines @@
   ASSERT_FALSE(match10);
 
   AdblockPlus::FilterPtr match11 = filterEngine->Matches("http://example.org/orgbanner.gif", "IMAGE", "http://example.com/");
   ASSERT_FALSE(match11);
 
   AdblockPlus::FilterPtr match12 = filterEngine->Matches("http://example.org/orgbanner.gif", "IMAGE", "http://example.org/");
   ASSERT_TRUE(match12);
   ASSERT_EQ(AdblockPlus::Filter::TYPE_BLOCKING, match12->GetType());
 }
 
+TEST_F(FilterEngineTest, MatchesOnWhitelistedDomain)
+{
+  filterEngine->GetFilter("adbanner.gif")->AddToList();
+  filterEngine->GetFilter("@@||example.org$document")->AddToList();

[Wladimir Palant, 2013/11/04 07:17:28]

Nit: This should be @@||example.org^$document (^ is a separator placeholder - it
makes sure the domain name actually ends here).

[Felix Dahlke, 2013/11/15 08:18:19]

Done.

+
+  AdblockPlus::FilterPtr match1 =
+    filterEngine->Matches("http://ads.com/adbanner.gif", "IMAGE",
+                          "http://example.com/");
+  ASSERT_TRUE(match1);
+  ASSERT_EQ(AdblockPlus::Filter::TYPE_BLOCKING, match1->GetType());
+
+  AdblockPlus::FilterPtr match2 =
+    filterEngine->Matches("http://ads.com/adbanner.gif", "IMAGE",
+                          "http://example.org/");
+  ASSERT_TRUE(match2);
+  ASSERT_EQ(AdblockPlus::Filter::TYPE_EXCEPTION, match2->GetType());
+}
+
+TEST_F(FilterEngineTest, MatchesNestedFrameRequest)
+{
+  filterEngine->GetFilter("adbanner.gif")->AddToList();
+  filterEngine->GetFilter("@@adbanner.gif$domain=example.org")->AddToList();
+
+  std::vector<std::string> documentUrls1;
+  documentUrls1.push_back("http://ads.com/frame/");
+  documentUrls1.push_back("http://example.com/");
+  AdblockPlus::FilterPtr match1 =
+    filterEngine->Matches("http://ads.com/adbanner.gif", "IMAGE",
+                          documentUrls1);
+  ASSERT_TRUE(match1);
+  ASSERT_EQ(AdblockPlus::Filter::TYPE_BLOCKING, match1->GetType());
+
+  std::vector<std::string> documentUrls2;
+  documentUrls2.push_back("http://ads.com/frame/");
+  documentUrls2.push_back("http://example.org/");
+  AdblockPlus::FilterPtr match2 =
+    filterEngine->Matches("http://ads.com/adbanner.gif", "IMAGE",
+                          documentUrls2);

[Wladimir Palant, 2013/11/04 07:17:28]

How about an additional test where the document URLs are http://example.org/ and
http://ads.com/frame/ (in reverse order)? This should return a blocking filter
and didn't with your original implementation.

[Felix Dahlke, 2013/11/15 08:18:19]

Done.

+  ASSERT_TRUE(match2);
+  ASSERT_EQ(AdblockPlus::Filter::TYPE_EXCEPTION, match2->GetType());
+}
+
+TEST_F(FilterEngineTest, MatchesNestedFrameOnWhitelistedDomain)
+{
+  filterEngine->GetFilter("adbanner.gif")->AddToList();
+  filterEngine->GetFilter("@@||example.org$document")->AddToList();

[Wladimir Palant, 2013/11/04 07:17:28]

Nit: As above, this should be @@||example.org^$document. However, I would prefer
testing @@||example.org^$document,domain=ads.com to make sure that the parent
domain is also checked properly.

[Felix Dahlke, 2013/11/15 08:18:19]

Done.

+
+  std::vector<std::string> documentUrls1;
+  documentUrls1.push_back("http://ads.com/frame/");
+  documentUrls1.push_back("http://example.com/");
+  AdblockPlus::FilterPtr match1 =
+    filterEngine->Matches("http://ads.com/adbanner.gif", "IMAGE",
+                          documentUrls1);
+  ASSERT_TRUE(match1);
+  ASSERT_EQ(AdblockPlus::Filter::TYPE_BLOCKING, match1->GetType());
+
+  std::vector<std::string> documentUrls2;
+  documentUrls2.push_back("http://ads.com/frame/");
+  documentUrls2.push_back("http://example.org/");
+  AdblockPlus::FilterPtr match2 =
+    filterEngine->Matches("http://ads.com/adbanner.gif", "IMAGE",
+                          documentUrls2);
+  ASSERT_TRUE(match2);
+  ASSERT_EQ(AdblockPlus::Filter::TYPE_EXCEPTION, match2->GetType());

[Wladimir Palant, 2013/11/04 07:17:28]

I would add a few more tests here:

* http://example.org/ only - should return TYPE_BLOCKING.
* http://example.org/, http://ads.com/frame/ - should return TYPE_BLOCKING.
* http://ads.com/frame/, http://example.org/, http://example.com/ - should
return TYPE_EXCEPTION.

[Felix Dahlke, 2013/11/15 08:18:19]

Done.

+}
+
 TEST_F(FilterEngineTest, FirstRunFlag)
 {
   ASSERT_FALSE(filterEngine->IsFirstRun());
 }
 
 TEST_F(FilterEngineTestNoData, FirstRunFlag)
 {
   ASSERT_FALSE(filterEngine->IsFirstRun());
 }