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

src/plugin/Instances.h

 /*

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

Nit: If I understand correctly this file will have both SyncMap and SyncSet
defined. I think it would be better to have them in separate files, each file
named after the class. Instances.h isn't really descriptive, IMO. Alternatively
this file can be SyncCollections.h 

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

Synchronization isn't fundamental to what these classes do, even though it is
necessary to have it so that they function correctly. Their function as
containers is rather more fundamental. The details of these containers are quite
specific to collections of BHO instances within IE, even if a rather
non-specific way they could possibly be used for something we have no idea of
yet.

How about "InstanceContainer.h" or "BHOInstanceContainer.h" or "BHOContainer.h"?

[sergei, 2016/05/23 13:14:48]

I like "BHOContainer.h" but the classes' names should reflect it, they should
not be so generic like SyncMap.

[Eric, 2016/07/17 16:04:44]

This file also appears in https://codereview.adblockplus.org/29333107/. At this
point I'd rather just deal with its name then.

As for class names, once we get to a point where this is the last remaining
issue, we can revisit it. 

But honestly, this all seems like so much bikeshedding. This file is included in
exactly one source file and it's classes (this plus the one in another review)
are used exactly once.

  * 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)
+  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