Issue 5137 - [emscripten] Added basic filter storage implementation

compiled/storage/FilterStorage.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present 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/>.
  */
 
 #include <vector>
 
 #include "FilterStorage.h"
 #include "../filter/Filter.h"
 #include "../subscription/UserDefinedSubscription.h"
 #include "../FilterNotifier.h"
 
 FilterStorage* FilterStorage::mInstance = new FilterStorage();
 
-unsigned int FilterStorage::GetSubscriptionCount() const
+FilterStorage::Subscriptions::size_type FilterStorage::GetSubscriptionCount() const
 {
   return mSubscriptions.size();
 }
 
-Subscription* FilterStorage::SubscriptionAt(unsigned int index) const
+Subscription* FilterStorage::SubscriptionAt(FilterStorage::Subscriptions::size_type index) const
 {
   if (index >= mSubscriptions.size())
     return nullptr;
 
   SubscriptionPtr result(mSubscriptions[index]);
   return result.release();
 }
 
 int FilterStorage::IndexOfSubscription(const Subscription* subscription) const
 {
-  for (unsigned int i = 0; i < mSubscriptions.size(); i++)

[sergei, 2017/08/24 13:32:06]

Despite there are strangely no warnings from compiler and the number of
subscriptions will very unlikely exceed a dozen it would be better to not use
`int` and `unsigned int` so directly when it's actually
std::vector<>::size_type.
If it does not cause an overhead I would prefer to have `typedef unsigned int
size_type` as a member of FilterStorage, use it everywhere within FilterStorage
and not use `int`. Optionally -1 can also be defined in FilterStorage as an
invalid value.
And the same for Subscription classes.

[Wladimir Palant, 2017/08/31 11:32:35]

Done.

+  for (Subscriptions::size_type i = 0; i < mSubscriptions.size(); i++)
     if (mSubscriptions[i] == subscription)
       return i;
   return -1;
 }
 
-Subscription* FilterStorage::GetSubscriptionForFilter(const Filter* filter)

[sergei, 2017/08/24 13:32:05]

It seems that this method should be const.

[Wladimir Palant, 2017/08/31 11:32:36]

Managed to do this with somewhat counterintuitive changes.

[sergei, 2017/08/31 12:43:38]

Yeah, sorry, maybe it was a bad idea to turn it into a const method,
`SubscriptionAt` should not be const either because it allows to modify
internals of constant FilterStorage.

+Subscription* FilterStorage::GetSubscriptionForFilter(const Filter* filter) const
 {
   SubscriptionPtr fallback;
 
-  for (auto& subscription : mSubscriptions)
+  for (Subscriptions::size_type i = 0; i < mSubscriptions.size(); i++)
   {
+    SubscriptionPtr subscription(mSubscriptions[i]);
     UserDefinedSubscription* userDefinedSubscription =
         subscription->As<UserDefinedSubscription>();
     if (userDefinedSubscription && !userDefinedSubscription->GetDisabled() &&
         userDefinedSubscription->IsDefaultFor(filter))
     {
       SubscriptionPtr result(subscription);
       return result.release();
     }
     else if (!fallback && userDefinedSubscription &&
              userDefinedSubscription->IsGeneric())
@@ skipping 15 matching lines @@
   mSubscriptions.emplace_back(subscription);
   subscription->SetListed(true);
 
   FilterNotifier::SubscriptionChange(
     FilterNotifier::Topic::SUBSCRIPTION_ADDED,
     subscription
   );
   return true;
 }
 
 bool FilterStorage::RemoveSubscription(Subscription* subscription)

[sergei, 2017/08/24 13:32:06]

It seems so far there is no way to make the argument a constant pointer here and
in the adding method above.

[Wladimir Palant, 2017/08/31 11:32:35]

For AddSubscription() the argument shouldn't be constant - we store a non-const
pointer, and callers of SubscriptionAt() might modify the instance later. Here
the issue is smaller, but we are still calling SetListed(), changing reference
counter and notifying listeners with a non-const instance.

[sergei, 2017/08/31 12:43:38]

Acknowledged.

 {
   assert(subscription, u"Attempt to remove a null subscription"_str);
 
   if (!subscription || !subscription->GetListed())
     return false;
 
   for (auto it = mSubscriptions.begin(); it != mSubscriptions.end(); ++it)
   {
     if (*it == subscription)
     {
       mSubscriptions.erase(it);
       break;
     }
   }
   subscription->SetListed(false);
 
   FilterNotifier::SubscriptionChange(
     FilterNotifier::Topic::SUBSCRIPTION_REMOVED,
     subscription
   );
   return true;
 }
 
 bool FilterStorage::MoveSubscription(Subscription* subscription,

[sergei, 2017/08/24 13:32:05]

should `subscription` argument be a const pointer?

[Wladimir Palant, 2017/08/31 11:32:35]

It cannot be. Even if we make changes to the reference count possible on const
instances or manage to move the instance without touching the reference count,
the FilterNotifier call below requires a non-const instance. JavaScript
listeners might decide to modify the subscription.

[sergei, 2017/08/31 12:43:38]

Acknowledged.

                                      const Subscription* insertBefore)
 {
   assert(subscription, u"Attempt to move a null subscription"_str);
 
   int oldPos = IndexOfSubscription(subscription);
+  assert(oldPos >= 0, u"Attempt to move a subscription that is not in the list"_str);
   if (oldPos == -1)
     return false;

[sergei, 2017/08/24 13:32:05]

should there be an assert? it's seems undesired when `subscription` is not in
the list.

[Wladimir Palant, 2017/08/31 11:32:34]

Done.

 
   int newPos = -1;
   if (insertBefore)
     newPos = IndexOfSubscription(insertBefore);
   if (newPos == -1)
     newPos = mSubscriptions.size();
 
   if (newPos > oldPos)
     newPos--;
 
   if (newPos == oldPos)
     return false;
 
   mSubscriptions.erase(mSubscriptions.begin() + oldPos);
   mSubscriptions.emplace(mSubscriptions.begin() + newPos, subscription);
 
   FilterNotifier::SubscriptionChange(
     FilterNotifier::Topic::SUBSCRIPTION_MOVED,
     subscription
   );
   return true;
 }