Issue #1467, #3397 - Rewrite the map from threads to tabs

src/plugin/Instances.h

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 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 _INSTANCES_H_
 #define _INSTANCES_H_
 
 #include <mutex>
 #include <map>
 
 /**
  * A base class for a synchronized map from threads to BHO instances.
+ *
+ * This is a base class for 'CurrentThreadMap', defined in PluginClass.cpp.
+ * It's separated out here for testability.
+ * This class should not be used as polymorphic base class, as it has no virtual base class.
  *
  * The member functions here not simply forwarded versions of the container functions.
  * Rather, they are specialized for tracking BHO calls to SetSite().
  * Their semantics allow for verification that the call pattern is as expected.
  *
  * The key to the map (in the subclass) is the thread ID, thus operations are serialized on a per-key basis.
  * Calls to SetSite() bracket all other calls, so on a per-key basis
  *   the order of operations is always either (insert / find-success / erase) or (find-failure).
  * The library guarantees for std::map seem to indicate that operations on different keys
  *   do not interfer with each other, but there's some ambiguity there.
@@ skipping 15 matching lines @@
   mutable std::mutex mutex;
 
 public:
   /**
    * Returns true if (as expected) no key of value 'id' was present.
    * Returns false otherwise.
    */
   bool AddIfAbsent(Key id, T p)
   {
     SentryType sentry(mutex);
-    auto it = idMap.find(id);
-    if (it != idMap.end())
-    {
-      return false;
-    }
-    idMap[id] = p;
-    return true;
+    auto it = idMap.insert(std::make_pair(id,p));

[sergei, 2016/07/19 07:56:09]

missing space after comma

[Eric, 2016/07/27 20:14:22]

Fixed.

+    return it.second;
+    // Assert it.second==true implies the insertion took place,
+    //  which means there was no key of value 'id' already present.

[sergei, 2016/07/19 07:56:10]

I think we don't need this comment.

   }
 
   /**
    * Returns true if (as expected) a key of value 'id' was already present.
    * Returns false otherwise.
    */
-  bool RemoveAndCheck(Key id)

[sergei, 2016/02/08 13:35:36]

I think that only "Remove" would be better here.

[Eric, 2016/02/08 18:45:30]

OK. I don't.

[Oleksandr, 2016/02/10 10:58:48]

What about RemoveIfPresent? Just for the consistency with the Add method.

[Eric, 2016/05/19 17:23:58]

OK. Changed.

In retrospect, these might better be 'CheckAbsenceThenAdd' and
'CheckPresenceThenRemove', but that's a little wordy.

+  bool RemoveIfPresent(Key id)
   {
     SentryType sentry(mutex);
     auto it = idMap.find(id);
     if (it == idMap.end())
     {
       return false;
     }
     idMap.erase(it);
     return true;
   }
 
   /**
    * Returns a non-nullValue if a key of value 'id' is present.
    * Returns nullValue otherwise.
    */
   T Locate(Key id) const
   {
     SentryType sentry(mutex);
     auto it = idMap.find(id);
     return (it != idMap.end()) ? it->second : nullValue;
   }
 };
 
 #endif