Issue 1564-Fix FilterEngine::Matches for allowing request which is whitelisted in the ascendant node

include/AdblockPlus/ReferrerMapping.h

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2014 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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 #ifndef ADBLOCK_PLUS_REFERRER_MAPPING_H
 #define ADBLOCK_PLUS_REFERRER_MAPPING_H
 
 #include <list>
 #include <map>
 #include <string>
 #include <vector>
+#include "FilterEngine.h"
 
 namespace AdblockPlus
 {
   /**
    * Stores a mapping between URLs and their referrers.
-   * This can be used to build a chain of referrers for any URL
-   * (see `BuildReferrerChain()`), which approximates the frame structure, see
-   * FilterEngine::Matches().
+   * This can be used to build a frame structure for any URL
+   * (see `BuildFrameStructure()`), it's useful for FilterEngine::Matches().
    */
   class ReferrerMapping
   {
   public:
+    /// The type for URL.
+    typedef std::string Url;
+
+    /// Contains the ordered list of URLs.

[Wladimir Palant, 2014/12/09 21:20:03]

Nit: *an* ordered list

+    typedef std::vector<Url> Urls;
+
+    /// Indicates whether the URL is a frame or not.
+    enum FrameIndicator
+    {
+      FRAME_INDICATOR_NOT_A_FRAME = 0, FRAME_INDICATOR_FRAME= 1

[Wladimir Palant, 2014/12/09 21:20:03]

Extreme nit: articles seem wrong in constant names, FRAME_INDICATOR_NOT_FRAME?

[sergei, 2014/12/12 17:07:07]

I know, I thought about it but don't remember the reason I decided to add the
article.
fixed

+    };
     /**
      * Constructor.
      * @param maxCachedUrls Number of URL mappings to store. The higher the
      *        better - clients typically cache requests, and a single cached
      *        request will break the referrer chain.
      */
     ReferrerMapping(const int maxCachedUrls = 5000);
 
     /**
      * Records the refferer for a URL.
      * @param url Request URL.
      * @param referrer Request referrer.
+     * @param isFrame Indicates whether the url is a frame.
      */
-    void Add(const std::string& url, const std::string& referrer);
+    void Add(const Url& url, const Url& referrer, FrameIndicator isFrame);
 
     /**
-     * Builds a chain of referrers for the supplied URL.
+     * Builds a frame structure for the supplied URL.
      * This should reconstruct a document's parent frame URLs.
      * @param url URL to build the chain for.
-     * @return List of URLs, starting with `url`.
+     * @return List of URLs, finishing with `url`.
      */
-    std::vector<std::string> BuildReferrerChain(const std::string& url) const;
+    Urls BuildFrameStructure(const Url& url) const;
 
   private:
     const int maxCachedUrls;
-    std::map<std::string, std::string> mapping;
-    std::list<std::string> cachedUrls;
+    struct RequestInfo
+    {
+      explicit RequestInfo(const Url& referrerArg = Url(),
+        FrameIndicator frameIndicatorArg = FrameIndicator::FRAME_INDICATOR_NOT_A_FRAME)

[Wladimir Palant, 2014/12/09 21:20:03]

Shouldn't you have that default value in method Add() rather than here? Add() is
being called by all kind of code, this constructor on the other hand is for
internal use only and never called without the second parameter.

Also, I'm pretty sure that the default should be FRAME_INDICATOR_FRAME - that's
what we have to assume for referrers if we don't have any further information.

[sergei, 2014/12/12 17:07:07]

These default arguments are required by std::map, in particular if
`ReferrerMapping::mapping` does not contain `url` then the statement
`mapping[url] = RequestInfo(referrer, isFrame);` firstly creates `RequestInfo`
using default ctr and then calls `RequestInfo::operator=`. It's a question
whether we want `mapping[url] = ...` and internal class with default ctr or a
little bit more of additional logic but without default ctr.

What concerns Add(), that's exactly because it is called by all kind of code
it's difficult to assume what should be the default there and even more, do we
really need something default there?

+        : referrer(referrerArg), frameIndicator(frameIndicatorArg)
+      {
+      }
+      bool IsFrame() const
+      {
+        return frameIndicator == FrameIndicator::FRAME_INDICATOR_FRAME;
+      }
+      Url referrer;
+      FrameIndicator frameIndicator;
+    };
+    typedef std::map<Url, RequestInfo> ReferrerMap;
+    ReferrerMap mapping;
+    std::list<Url> cachedUrls;
   };
 }
 
 #endif