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 187 matching lines @@
   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.

+  filterEngine->GetFilter("@@||example.org^$document")->AddToList();
 
   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/");
@@ skipping 13 matching lines @@
     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());
+
+  std::vector<std::string> documentUrls3;
+  documentUrls3.push_back("http://example.org/");
+  documentUrls3.push_back("http://ads.com/frame/");
+  AdblockPlus::FilterPtr match3 =
+    filterEngine->Matches("http://ads.com/adbanner.gif", "IMAGE",
+                          documentUrls3);
+  ASSERT_TRUE(match3);
+  ASSERT_EQ(AdblockPlus::Filter::TYPE_BLOCKING, match3->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.

+  filterEngine->GetFilter("@@||example.org^$document,domain=ads.com")->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);
   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.

+
+  std::vector<std::string> documentUrls3;
+  documentUrls3.push_back("http://example.org/");
+  AdblockPlus::FilterPtr match3 =
+    filterEngine->Matches("http://ads.com/adbanner.gif", "IMAGE",
+                          documentUrls3);
+  ASSERT_TRUE(match3);
+  ASSERT_EQ(AdblockPlus::Filter::TYPE_BLOCKING, match3->GetType());
+
+  std::vector<std::string> documentUrls4;
+  documentUrls4.push_back("http://example.org/");
+  documentUrls4.push_back("http://ads.com/frame/");
+  AdblockPlus::FilterPtr match4 =
+    filterEngine->Matches("http://ads.com/adbanner.gif", "IMAGE",
+                          documentUrls4);
+  ASSERT_TRUE(match4);
+  ASSERT_EQ(AdblockPlus::Filter::TYPE_BLOCKING, match4->GetType());
+
+  std::vector<std::string> documentUrls5;
+  documentUrls5.push_back("http://ads.com/frame/");
+  documentUrls5.push_back("http://example.org/");
+  documentUrls5.push_back("http://example.com/");
+  AdblockPlus::FilterPtr match5 =
+    filterEngine->Matches("http://ads.com/adbanner.gif", "IMAGE",
+                          documentUrls5);
+  ASSERT_TRUE(match5);
+  ASSERT_EQ(AdblockPlus::Filter::TYPE_EXCEPTION, match5->GetType());
 }
 
 TEST_F(FilterEngineTest, FirstRunFlag)
 {
   ASSERT_FALSE(filterEngine->IsFirstRun());
 }
 
 TEST_F(FilterEngineTestNoData, FirstRunFlag)
 {
   ASSERT_FALSE(filterEngine->IsFirstRun());
 }