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 270 matching lines @@
   });
 }
 
 function prepareNotificationIconAndPopup()
 {
   activeNotification.onClicked = function()
   {
     iconAnimation.stop();
     activeNotification = null;
   };
-
+  
   iconAnimation.update(activeNotification.severity);

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

Felix, what you think do we need this function here ? 
Actually not sure what we are using it for ?

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

It animates the icon. When a notification arrives, we animate it and put a
message in the bubble. For critical notifications, we also show a desktop
notification.

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

Got it.

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

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

 }
 
 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);

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

This is a boolean, was does "notification" mean here? How about something like
"openLinks"? Or you could just put the confirm() call in the condition below.

-      if (notification == true)

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

== true is redundant

+      if (confirm(message))

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

Nit: Change to…

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

+      {
         openNotificationLinks();
+        prepareNotificationIconAndPopup();
+      }
       else
         prepareNotificationIconAndPopup();

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

We should call prepareNotificationIconAndPopup either way, not in the else case.

[saroyanm, 2014/02/26 18:00:29]

The current implementation is done depending on the original implementation, in
the original implementation prepareNotificationIconAndPopup is only call in case
the desktop notification is closed. @Felix Can you please confirm that
prepareNotificationIconAndPopup should be called anytime showNotification method
is called ?

[Felix Dahlke, 2014/02/26 18:02:53]

The else here means that it'd only be called if the user said "no" to opening
all the links. Whether he said yes or no, the notification will be closed here,
and we should call prepareNotificationIconAndPopup when the notification gets
closed.

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