Issue 1107 - Support notifications

src/Notification.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 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 <AdblockPlus/JsValue.h>
 #include <AdblockPlus/JsEngine.h>
 #include <AdblockPlus/Notification.h>
 #include <algorithm>
 
 using namespace AdblockPlus;
 
 namespace
 {
   typedef std::pair<NotificationType, std::string> NotificationTypeString;
   typedef std::vector<NotificationTypeString> NotificationTypes;
-  const NotificationTypes g_notificationTypes = []()->NotificationTypes
+  NotificationTypes InitNotificationTypes()
   {
     NotificationTypes retValue;
-    retValue.emplace_back(std::make_pair(NotificationType::NOTIFICATION_TYPE_QUESTION, "question"));
-    retValue.emplace_back(std::make_pair(NotificationType::NOTIFICATION_TYPE_CRITICAL, "critical"));
-    retValue.emplace_back(std::make_pair(NotificationType::NOTIFICATION_TYPE_INFORMATION, "information"));
+    retValue.push_back(std::make_pair(NotificationType::NOTIFICATION_TYPE_QUESTION, "question"));
+    retValue.push_back(std::make_pair(NotificationType::NOTIFICATION_TYPE_CRITICAL, "critical"));
+    retValue.push_back(std::make_pair(NotificationType::NOTIFICATION_TYPE_INFORMATION, "information"));
     return retValue;
-  }();
+  }
+
+  const NotificationTypes notificationTypes = InitNotificationTypes();
 
   NotificationType StringToNotificationType(const std::string& value)
   {
-    auto ii_notificationType = std::find_if(g_notificationTypes.begin(), g_notificationTypes.end(),
-      [&value](const NotificationTypeString& pair)->bool
+    struct IsSecondEqualToPredicate
     {
-      return value == pair.second;
-    });
-    if (ii_notificationType == g_notificationTypes.end())
+      std::string value;
+      bool operator()(const NotificationTypeString& pair) const
+      {
+        return value == pair.second;
+      }
+    } findBySecond = {value};
+    NotificationTypes::const_iterator notificationTypeIterator = std::find_if(
+      notificationTypes.begin(), notificationTypes.end(), findBySecond);
+    if (notificationTypeIterator == notificationTypes.end())
     {
       return NotificationType::NOTIFICATION_TYPE_INFORMATION;
     }
-    return ii_notificationType->first;
-  }
-
-  std::string NotificationTypeToString(NotificationType value)

[Wladimir Palant, 2015/01/22 15:19:51]

This function seems unused.

[sergei, 2015/01/22 16:15:11]

removed

-  {
-    auto ii_notificationType = std::find_if(g_notificationTypes.begin(), g_notificationTypes.end(),
-      [&value](const NotificationTypeString& pair)->bool
-    {
-      return value == pair.first;
-    });
-    if (ii_notificationType == g_notificationTypes.end())
-    {
-      return "information";
-    }
-    return ii_notificationType->second;
+    return notificationTypeIterator->first;
   }
 }
 
-Notification::Notification(const JsValuePtr& jsValue, PrivateCtrArg)
+Notification::Notification(const JsValuePtr& jsValue)
   : JsValue(jsValue)
 {
 }
 
 NotificationType Notification::GetType() const
 {
   return type;
 }
 
 const std::string& Notification::GetTitle() const
 {
   return title;
 }
 
 const std::string& Notification::GetMessageString() const
 {
   return message;
 }
 
 void Notification::MarkAsShown()
 {
-  JsValuePtr func = jsEngine->Evaluate("API.markNotificationAsShown");
-  if (!func)
-  {
-    return;
-  }
   JsValueList params;
-  auto jsId = GetProperty("id");
-  params.push_back(jsEngine->NewValue(jsId ? jsId->AsString() : ""));

[Wladimir Palant, 2015/01/22 15:19:51]

Does it even make sense to call API.markNotificationAsShown() with a an empty
string as parameter?

[sergei, 2015/01/22 16:15:11]

Does not make sense. What do you think about passing
`GetProperty("id")->AsString()`?

[Wladimir Palant, 2015/01/22 19:16:18]

Yes, I think we should just pass in the value of the id property here. It will
produce strange results for "unexpected" input but that's just "trash in, trash
out" - no way around it.

-  func->Call(params);
+  params.push_back(GetProperty("id"));
+  jsEngine->Evaluate("API.markNotificationAsShown")->Call(params);
 }
 
 NotificationPtr Notification::JsValueToNotification(const JsValuePtr& jsValue)
 {
-  if(!jsValue || !jsValue->IsObject())
+  if (!jsValue || !jsValue->IsObject())
   {
     return NotificationPtr();
   }
 
-  auto notification = std::tr1::make_shared<Notification>(jsValue, PrivateCtrArg());
-  auto jsType = notification->GetProperty("type");
+  NotificationPtr notification(new Notification(jsValue));
+  JsValuePtr jsType = notification->GetProperty("type");
   notification->type = StringToNotificationType(jsType ? jsType->AsString() : "");
 
-  JsValuePtr func = notification->jsEngine->Evaluate("API.getNotificationTexts");
-  if (!func)

[Wladimir Palant, 2015/01/22 15:19:51]

Here and elsewhere, this kind of check is pointless. JSEngine::Evaluate() will
never return an empty pointer. It could throw an exception (execution failed) or
return a JSValue, nothing beyond that. What you really need to check is
func->IsFunction() but func->Call() will already do that.

[sergei, 2015/01/22 16:15:11]

Clear, removed here and elsewhere.

-  {
-    return notification;
-  }
   JsValueList params;
   params.push_back(notification);
-  auto jsTexts = func->Call(params);
-  if (!jsTexts)

[Wladimir Palant, 2015/01/22 15:19:51]

Here again a pointless check - JSValue::Call() will also either throw an
exception or return a JSValue instance (which can be undefined, e.g. if the
function didn't return a value). There is no scenario where it would return an
empty pointer.

-  {
-    return notification;
-  }
-
-  auto jsTitle = jsTexts->GetProperty("title");
-  if (jsTitle && jsTitle->IsString())

[Wladimir Palant, 2015/01/22 15:19:51]

Checking whether jsTitle is true is pointless (same in other places where
GetProperty() is used) - JSValue::GetProperty() will always return *something*,
jsTitle->IsUndefined() will simply be true if the property doesn't exist.

+  JsValuePtr func = notification->jsEngine->Evaluate("API.getNotificationTexts");
+  JsValuePtr jsTexts = func->Call(params);
+  JsValuePtr jsTitle = jsTexts->GetProperty("title");
+  if (jsTitle->IsString())
   {
     notification->title = jsTitle->AsString();
   }
-  auto jsMessage = jsTexts->GetProperty("message");
-  if (jsMessage && jsMessage->IsString())
+  JsValuePtr jsMessage = jsTexts->GetProperty("message");
+  if (jsMessage->IsString())
   {
     notification->message = jsMessage->AsString();
   }
   return notification;

[Wladimir Palant, 2015/01/22 15:19:51]

Shouldn't all this code be inside the constructor? What's the point of the
factory function? Sure, a constructor cannot return an empty pointer, it can
only throw an exception - but this function will throw exceptions as well, e.g.
if there is no "API" variable in the JS context.

[sergei, 2015/01/22 16:15:11]

I would say the current variant is better for present.
To call "API.getNotificationTexts" we need to pass the notification javascript
object as argument, thus to put it into `JsValueList` which is a container of
shared pointers of `JsValue`.
- We cannot put `jsValue` constructor argument into JsValueList because its
internal `V8ValueHolder<v8::Value> value` has been already transferred into
`value` of the base class of `Notification` class. In addition see
http://codereview.adblockplus.org/5598762307158016/ .
- We cannot use `shared_from_this` in the constructor because internal
`weak_ptr` of `enable_shared_from_this` is not initialized yet.
- Even more, we cannot easily have `JsValuePtr` as a member of `Notification`
class because in that case we don't have the access to the protected member
`JsValue::jsEngine`. Actually we can create another class which inherits from
from `JsValue` and which exposes `JsEngine` and store a pointer to this class as
a member of `Notification` class, but I'm afraid that it is even more confusing.
Although having `JsValue` as a private member of `Notification` class looks very
attractive, just in case I would like to have an access to base JsValue.

[Wladimir Palant, 2015/01/22 19:16:18]

Ok, this makes sense then, at least given my knowledge of C++. Maybe fhd has a
better idea.

[Felix Dahlke, 2015/01/23 15:34:31]

I see... Don't think we have the time to really dive into this right now, but I
think we should create a follow-up for this. Seems like a hack to me.

 }