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 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"));
+    return retValue;
+  }();
+
+  NotificationType StringToNotificationType(const std::string& value)
+  {
+    auto ii_notificationType = std::find_if(g_notificationTypes.begin(), g_notificationTypes.end(),
+      [&value](const NotificationTypeString& pair)->bool
+    {
+      return value == pair.second;
+    });
+    if (ii_notificationType == g_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;
+  }
+}
+
+Notification::Notification(const JsValuePtr& jsValue, PrivateCtrArg)
+  : 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);
+}
+
+NotificationPtr Notification::JsValueToNotification(const JsValuePtr& jsValue)
+{
+  if(!jsValue || !jsValue->IsObject())
+  {
+    return NotificationPtr();
+  }
+
+  auto notification = std::tr1::make_shared<Notification>(jsValue, PrivateCtrArg());
+  auto 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.

+  {
+    notification->title = jsTitle->AsString();
+  }
+  auto jsMessage = jsTexts->GetProperty("message");
+  if (jsMessage && 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.

+}