Issue 1107 - Support notifications

include/AdblockPlus/FilterEngine.h

Index: include/AdblockPlus/FilterEngine.h
diff --git a/include/AdblockPlus/FilterEngine.h b/include/AdblockPlus/FilterEngine.h
index b81433f5c9bf1b540cf837f6df1a1662bc623454..e256b3c3a5c50de65d0bee266ebb897efe1d2483 100644
--- a/include/AdblockPlus/FilterEngine.h
+++ b/include/AdblockPlus/FilterEngine.h
@@ -24,6 +24,7 @@
 #include <vector>
 #include <AdblockPlus/JsEngine.h>
 #include <AdblockPlus/JsValue.h>
+#include <AdblockPlus/Notification.h>
 
 #include "tr1_memory.h"
 
@@ -237,6 +238,52 @@ namespace AdblockPlus
     std::vector<SubscriptionPtr> FetchAvailableSubscriptions() const;
 
     /**
+     * Creates new Notification(see `Notification`) object.
+     * @param type Type of the notification.
+     * @param id Id of the newly created notification.
+     * @return New Notification instance.
+     */
+    std::tr1::shared_ptr<Notification> CreateNotification(NotificationType type,
+      const std::string& id) const;

[Wladimir Palant, 2015/01/21 13:48:28]

Why do we need this in the public API? Shouldn't notifications be created
internally only?

[sergei, 2015/01/21 15:58:55]

It is for local notifications.
@Felix

[Wladimir Palant, 2015/01/21 19:23:18]

What kind of local notifications? If the client application wants to show a
notification, why would it want to take the detour through libadblockplus for
that? And if libadblockplus code wants to show a notification - why does this
need a public API?

Note that the anti-adblock notification comes from core code itself. If we ever
add that code to libadblockplus, the JavaScript code will simply call the
corresponding methods in the notification module - all that without leaving the
JavaScript engine.

+
+    /**
+     * Determines which notification is to be shown next.
+     * @param url URL to match notifications to (optional).
+     * @return Notification to be shown, or null if there is no any.
+     */
+    std::tr1::shared_ptr<Notification> GetNextNotificationToShow(
+      const std::string& url = std::string());
+
+    /**
+     * Localizes the texts of the supplied notification.
+     * @param notification Notification to translate.
+     * @param locale The target locale (optional, defaults to the
+   *                 application locale)

[Wladimir Palant, 2015/01/21 13:48:28]

Do we need this parameter at all? When would it make sense to get notifications
in a different language?

[sergei, 2015/01/21 15:58:55]

1. The javascript notifications API has this argument, so I decided to expose it
as well. As I saw we support the following notification structures:
{
  ...,
  title: "some string"
}
and
{
  ...,
  title: {
    "en-US": "English",
    "de-DE": "Deutsch"
  }
}

2. BTW, I would say the comment does not correspond to the actual
implementation, because the default locale is not the application locale, it is
"en-US", see
https://hg.adblockplus.org/adblockplus/file/ec627ace494e/lib/notification.js#l61
. But it's not the part of this project and change.

@Felix

[Wladimir Palant, 2015/01/21 19:23:18]

Yes, and it is already unused there. All the more reason to omit this parameter
in libadblockplus - it provides a high-level API, exposing each and every weird
feature isn't the point.
en-US is the fallback locale, if no translations for the given locale is found.
That's irrelevant here, the important line of code is
https://hg.adblockplus.org/adblockplus/file/ec627ace494e/lib/notification.js#....

[Felix Dahlke, 2015/01/22 03:31:08]

Agreed. I actually think we should remove it from the core code, too:
https://issues.adblockplus.org/ticket/1846

[sergei, 2015/01/22 14:08:15]

I've removed locale for notification texts.

+     * @return the translated texts.
+     */
+    NotificationTexts GetNotificationTexts(
+      const std::tr1::shared_ptr<Notification>& notification,
+      const std::string& locale = std::string()) const;

[Wladimir Palant, 2015/01/21 13:48:28]

Why isn't this a methods of the Notification class? Same with MarkAsShown().

[sergei, 2015/01/21 15:58:55]

1. GetNotificationTexts is moved into Notification class.

2. MarkAsShown() accepts notification ID, which provides another level of
flexibility. I guess it would be better to have it here.

[Wladimir Palant, 2015/01/21 19:23:18]

What kind of flexibility? If we have a Notification object, why do we need to
work with IDs?

[Felix Dahlke, 2015/01/22 03:31:08]

Agreed - there is no notification object in that sense in the core code, so we
have to work with IDs. Here we don't have to.

[sergei, 2015/01/22 14:08:15]

MarkAsShown is moved into Notification class. Just wonder, what is the reason
that we cannot carry notification object but can carry its ID in the core?

The flexibility is that carrying string is very simple while to preserve the
notification object is additional work for the clients of libadblockplus. I
guess that hosting libadblockplus in another process would not be a rare case,
so the client of libadblockplus receives the notification object, serializes it
and sends to the main application. Now this notification object could be removed
if we provide the API which works with the ID, but without such API the client
has to remember it somewhere. We cannot mark it as shown immediately because
most likely the main application has already some notification mechanism with
some queue, so it's not guaranteed that the user will see it and we should mark
the notification as shown only after the actual showing. I guess it is the
reason to have the method MarkAsShown in general and the notification is not
removed from the pending notifications in the core. After showing of the
notification the main application sends the notification id to the
libadblockplus hoster, having this ID the hoster could easily mark it as shown.
In addition, although right now there are no now notifications and as I
understand they won't be often, the developer of the client of libadblockplus
will definitely ask himself about "leaking" of the preserved notifications, at
least for the case when the main app looses the notification ID (because of
restart for example). If we switch from the polling approach the hoster of
libadblockplus will discover that now in addition he has to synchronize the
access to the collection of stored notifications. Later this poor developer can
discover that he has to care about the destruction order, because JsEngine is
referenced by the stored notification (see #1849).
So, it complicates the hoster of libabdlockplus.

+
+    /**
+     * Adds a local notification.
+     * @param value The notification to add
+     */
+    void AddNotification(const std::tr1::shared_ptr<Notification>& value);

[Wladimir Palant, 2015/01/21 13:48:28]

Why do we need an API to add "local" notifications? Notifications are supposed
to come from the server only.

[sergei, 2015/01/21 15:58:55]

I guess it is designed for the cases when we can detect something on the client
side and notify the client about it. BTW, notifications come not only from the
server, there is
https://hg.adblockplus.org/adblockplus/file/ec627ace494e/lib/antiadblockInit.js
which can create local notifications.
@Felix

[Felix Dahlke, 2015/01/22 03:31:08]

You're right Sergei, the question notification is currently not coming from the
server. But as Wladimir said, all notifications are _supposed_ to come from the
server, the anti adblock notification being added locally is a hack. Once we've
made the notification system more powerful, that too, will be defined on the
server. Any local considerations for showing a notification will be added to the
notification mechanism as a feature, the core code will handle that. 

+
+    /**
+     * Removes an existing local notification.
+     * @param notification The notification to remove.
+     */
+    void RemoveNotification(const std::tr1::shared_ptr<Notification>& value);
+
+    /**
+     * Marks the notification as shown.
+     * @param notificationId The notification ID.
+     */
+    void MarkNotificationAsShown(const std::string& notificationId);
+
+    /**
      * Checks if any active filter matches the supplied URL.
      * @param url URL to match.
      * @param contentType Content type of the requested resource.