Fix: Notification popup is broken

background.js

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2013 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 268 matching lines @@
       });
     });
   });
 }
 
 function prepareNotificationIconAndPopup()
 {
   activeNotification.onClicked = function()
   {
     iconAnimation.stop();
+    activeNotification = null;
   };
   

[Felix Dahlke, 2014/02/26 17:22:58]

Why did you change this logic?

[Thomas Greiner, 2014/02/26 17:37:35]

@Felix The problem is that onClicked is never called if we don't show
notification.html which means that we need to set it to null elsewhere.

@Manvel I agree with Felix that this is not how it should be. The existing
approach with createHTMLNotification should stay functional. The changes should
be on top of what we have - not instead of.

[Felix Dahlke, 2014/02/26 17:47:10]

Ah I see. That event handler name is a bit confusing in this context - it's
basically an event that happens when the notification is shown. So I'd be in
favour of renaming it to "onShow" while we're at it.

Anyway, calling that right before we show a notification in all cases below
should do. The only real reason why we do that delayed for notification.html is
apparently that it needs to access activeNotification, which we'd reset too
early then.

[saroyanm, 2014/02/26 18:15:34]

@Thomas but the current approach with createHTMLNotification is functional
because we are calling the prepareNotificationIconAndPopup only in case
notification is closed and before being closed it will be shown, maybe I'm not
understanding your point right ?

[saroyanm, 2014/02/26 18:25:08]

From My point of view, the logic should change:
We should call iconAnimation.update as soon we show the notification and after
closing the notification we should unset activeNotification and stop the icon
animation, but anyway I can't see if the iconAnimation is working.

   iconAnimation.update(activeNotification.severity);

[Felix Dahlke, 2014/02/27 14:10:39]

Nit: Probably whitespace differences, this shouldn't register as a diff.

-  activeNotification = null;
 }
 
 function openNotificationLinks() 
 {
   if (activeNotification.links)
   {
     activeNotification.links.forEach(function(link)
     {
       ext.windows.getLastFocused(function(win)
       {
@@ skipping 62 matching lines @@
       notification.show();
       notification.addEventListener("close", prepareNotificationIconAndPopup, false);
       notification.addEventListener("click", openNotificationLinks, false);
     }
     else
     {
       var message = title + "\n" + message;
       if (hasLinks)
         message += "\n\n" + ext.i18n.getMessage("notification_with_buttons");
         
-      var notification = confirm(message);
-      if (notification == true)
+      if (confirm(message))

[Thomas Greiner, 2014/02/27 10:07:32]

Nit: Change to…

if (confirm(message))
  openNotificationLinks();
prepareNotificationIconAndPopup();

+      {
         openNotificationLinks();
+        prepareNotificationIconAndPopup();
+      }
       else
         prepareNotificationIconAndPopup();
     }
   }
   else
     prepareNotificationIconAndPopup();
 }
 
 ext.onMessage.addListener(function (msg, sender, sendResponse)
 {
@@ skipping 107 matching lines @@
   tab.sendMessage({type: "clickhide-deactivate"});
   refreshIconAndContextMenu(tab);
 });
 
 setTimeout(function()
 {
   var notificationToShow = Notification.getNextToShow();
   if (notificationToShow)
     showNotification(notificationToShow);
 }, 3 * 60 * 1000);