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.

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

As I told already there is no reason to have such class so generic if it is only
for <threadID, BHOInstance*>. The interface of generic SyncMap<Key, Value>
should be different.

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

This class is not designed to be a base class because there is no virtual
destructor.

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

The rule is that you need a virtual destructor in a polymorphic base class, not
any base class of any type. This class is not designed to be polymorphic; for
example, it has no virtual functions.

This class is separated out for testability. Design for testability is, at this
point, a widely accepted best practice.

I've added a comment in a new patch set, because evidently this all was not
sufficiently clear.

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

And as *I* said before, it is *not* only for instance pointers. Let me be very
clear.
(1) Use in the plugin for instance pointers
(2) Use in unit tests for higher code quality.

If you would like to argue that unit tests are unimportant and should be
eliminated, I will listen to that argument.

+ *
+ * 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.
  * This class is synchronized as a matter of defensive programming.
  */

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

We can save a lot of energy if move the implementation of this class into
`CurrentThreadMap` and move `CurrentThreadMap` into this file. We will not need
the comments, suspicious inheritance and some questions will be eliminated
automatically.

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

Everything in this source file can be easily unit tested. Anything using current
thread cannot be easily tested.

Furthermore, this is a header-only file containing only template definitions.
Defining 'CurrentThreadMap' here would require an extra source file for just a
few lines of code.

It's better to leave that function where it is, in PluginClass.cpp, where it's
defined at its point of use.

 template<class Key, class T, T nullValue>
 class SyncMap
 {
   typedef std::lock_guard<std::mutex> SentryType;
 
   /**
    * Underlying map container
    */
   std::map<Key, T> idMap;
 
   /**
    * Synchronization primitive
    */
   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 RemoveIfPresent(Key id)
   {
     SentryType sentry(mutex);
     auto it = idMap.find(id);
@@ skipping 11 matching lines @@
    */
   T Locate(Key id) const
   {
     SentryType sentry(mutex);
     auto it = idMap.find(id);
     return (it != idMap.end()) ? it->second : nullValue;
   }
 };
 
 #endif