Issue 5164 - Remove NotificationPtr

test/Notification.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 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
@@ skipping 30 matching lines @@
     }
 
     void AddNotification(const std::string& notification)
     {
       jsEngine->Evaluate("(function()"
       "{"
         "require('notification').Notification.addNotification(" + notification + ");"
       "})();");
     }
 
-    Notification PeekNotification(const std::string& url = std::string())

[sergei, 2017/04/21 15:30:33]

I think it would be better to use std::unique_ptr here.

[hub, 2017/04/21 16:29:31]

and it would solve the other problem with the constructor.

-    {
-      Notification retValue(filterEngine->GetJsEngine()->Evaluate("undefined"));
-      filterEngine->SetShowNotificationCallback(std::bind(
-        &NotificationTest::NotificationAvailableCallback,
-        std::placeholders::_1, std::ref(retValue)));

[sergei, 2017/04/21 15:30:33]

JIC, now we may use lambda functions

[hub, 2017/04/21 16:29:31]

ok will change that while I'm at it.

+    std::unique_ptr<Notification> PeekNotification(const std::string& url = std::string())
+    {
+      std::unique_ptr<Notification> retValue;
+      filterEngine->SetShowNotificationCallback(
+        [&retValue](Notification& notification) {
+          retValue.reset(new Notification(std::move(notification)));
+        });
       filterEngine->ShowNextNotification(url);
       filterEngine->RemoveShowNotificationCallback();
       return retValue;
-    }
-
-    static void NotificationAvailableCallback(const Notification& src, Notification& dst)
-    {
-      EXPECT_FALSE(src.IsUndefined());
-      dst = src;
     }
   };
 
   class MockWebRequest : public WebRequest
   {
   public:
     std::string responseText;
     explicit MockWebRequest(const std::string& notification)
       : responseText(notification)
     {
@@ skipping 31 matching lines @@
           "\"type\": \"information\","
           "\"message\": {"
              "\"en-US\": \"message\""
           "},"
           "\"title\": \"Title\""
         "}]"
         "}";
       jsEngine->SetWebRequest(std::shared_ptr<MockWebRequest>(
         new MockWebRequest(responseJsonText)));
       jsEngine->SetLogSystem(LogSystemPtr(new DefaultLogSystem()));
-      filterEngine.reset(new FilterEngine(jsEngine));
+      filterEngine = FilterEngine::Create(jsEngine);
       filterEngine->SetShowNotificationCallback(
-        std::bind(&NotificationMockWebRequestTest::OnNotification,
-        this, std::placeholders::_1));
-    }
-
-    void OnNotification(const Notification& notification)
-    {
-      isNotificationCallbackCalled = true;
-      ASSERT_TRUE(notification);
-      EXPECT_EQ(NotificationType::NOTIFICATION_TYPE_INFORMATION, notification->GetType());
-      EXPECT_EQ("Title", notification->GetTexts().title);

[sergei, 2017/04/21 15:30:33]

This is not compilable. In order to run this test one has to firstly modify
notifications.js in adblockpluscore, I can fix this place later.

[hub, 2017/04/21 16:29:32]

I realize this code doesn't get built. Will fix the build at least.

-      EXPECT_EQ("message", notification->GetTexts().message);
-      notification->MarkAsShown();
+        [this](Notification& notification) {
+          isNotificationCallbackCalled = true;
+          EXPECT_EQ(NotificationType::NOTIFICATION_TYPE_INFORMATION, notification.GetType());
+          EXPECT_EQ("Title", notification.GetTexts().title);
+          EXPECT_EQ("message", notification.GetTexts().message);
+          notification.MarkAsShown();
+        });
     }
   };
 #endif
 }
 
 TEST_F(NotificationTest, NoNotifications)
 {
-  EXPECT_TRUE(PeekNotification().IsUndefined());
+  EXPECT_FALSE(PeekNotification());
 }
 
 #ifdef NotificationMockWebRequestTest_ENABLED
 TEST_F(NotificationMockWebRequestTest, SingleNotification)
 {
   AdblockPlus::Sleep(5000/*msec*/); // it's a hack
   EXPECT_TRUE(isNotificationCallbackCalled);
 }
 #endif
 
 TEST_F(NotificationTest, AddNotification)
 {
   AddNotification("{"
       "type: 'critical',"
       "title: 'testTitle',"
       "message: 'testMessage',"
     "}");
-  Notification notification = PeekNotification();
-  EXPECT_FALSE(notification.IsUndefined());
-  EXPECT_EQ(NotificationType::NOTIFICATION_TYPE_CRITICAL, notification.GetType());
-  EXPECT_EQ("testTitle", notification.GetTexts().title);
-  EXPECT_EQ("testMessage", notification.GetTexts().message);
+  auto notification = PeekNotification();
+  ASSERT_TRUE(notification);
+  EXPECT_EQ(NotificationType::NOTIFICATION_TYPE_CRITICAL, notification->GetType());
+  EXPECT_EQ("testTitle", notification->GetTexts().title);
+  EXPECT_EQ("testMessage", notification->GetTexts().message);
 }
 
 TEST_F(NotificationTest, FilterByUrl)
 {
   AddNotification("{ id: 'no-filter', type: 'critical' }");
   AddNotification("{ id: 'www.com', type: 'information',"
     "urlFilters:['||www.com$document']"
   "}");
   AddNotification("{ id: 'www.de', type: 'question',"
     "urlFilters:['||www.de$document']"
   "}");
 
-  Notification notification = PeekNotification();
-  EXPECT_FALSE(notification.IsUndefined());
-  EXPECT_EQ(NotificationType::NOTIFICATION_TYPE_CRITICAL, notification.GetType());
+  auto notification = PeekNotification();
+  ASSERT_TRUE(notification);
+  EXPECT_EQ(NotificationType::NOTIFICATION_TYPE_CRITICAL, notification->GetType());
 
   notification = PeekNotification("http://www.de");
-  EXPECT_FALSE(notification.IsUndefined());
-  EXPECT_EQ(NotificationType::NOTIFICATION_TYPE_QUESTION, notification.GetType());
+  ASSERT_TRUE(notification);
+  EXPECT_EQ(NotificationType::NOTIFICATION_TYPE_QUESTION, notification->GetType());
 
   notification = PeekNotification("http://www.com");
-  EXPECT_FALSE(notification.IsUndefined());
-  EXPECT_EQ(NotificationType::NOTIFICATION_TYPE_INFORMATION, notification.GetType());
+  ASSERT_TRUE(notification);
+  EXPECT_EQ(NotificationType::NOTIFICATION_TYPE_INFORMATION, notification->GetType());
 }
 
 TEST_F(NotificationTest, MarkAsShown)
 {
   AddNotification("{ id: 'id', type: 'question' }");
-  EXPECT_FALSE(PeekNotification().IsUndefined());
-  Notification notification = PeekNotification();
-  notification.MarkAsShown();
-  EXPECT_TRUE(PeekNotification().IsUndefined());
+  EXPECT_TRUE(PeekNotification());
+  auto notification = PeekNotification();
+  ASSERT_TRUE(notification);
+  notification->MarkAsShown();
+  EXPECT_FALSE(PeekNotification());
 }
 
 TEST_F(NotificationTest, NoLinks)
 {
   AddNotification("{ id: 'id'}");
-  Notification notification = PeekNotification();
-  EXPECT_FALSE(notification.IsUndefined());
-  EXPECT_EQ(0u, notification.GetLinks().size());
+  auto notification = PeekNotification();
+  ASSERT_TRUE(notification);
+  EXPECT_EQ(0u, notification->GetLinks().size());
 }
 
 TEST_F(NotificationTest, Links)
 {
   AddNotification("{ id: 'id', links: ['link1', 'link2'] }");
-  Notification notification = PeekNotification();
-  EXPECT_FALSE(notification.IsUndefined());
-  std::vector<std::string> notificationLinks = notification.GetLinks();
+  auto notification = PeekNotification();
+  ASSERT_TRUE(notification);
+  std::vector<std::string> notificationLinks = notification->GetLinks();
   ASSERT_EQ(2u, notificationLinks.size());
   EXPECT_EQ("link1", notificationLinks[0]);
   EXPECT_EQ("link2", notificationLinks[1]);
 }