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

[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)
       {
         win.openTab(Utils.getDocLink(link));
       });
     });
   }
-  prepareNotificationIconAndPopup();
 }
 
 function notificationButtonClick(id, index)
 {
   if (activeNotification.links && activeNotification.links[index])
   {
     ext.windows.getLastFocused(function(win)
     {
       win.openTab(Utils.getDocLink(activeNotification.links[index]));
     });
   }
-  prepareNotificationIconAndPopup();
 }
 
 function showNotification(notification)
 {
   activeNotification = notification;
   if (activeNotification.severity === "critical")
   {
     var hasWebkitNotifications = typeof webkitNotifications !== "undefined";
     if (hasWebkitNotifications && "createHTMLNotification" in webkitNotifications)
     {
       var notification = webkitNotifications.createHTMLNotification("notification.html");
       notification.show();
       notification.addEventListener("close", prepareNotificationIconAndPopup, false);
       return;
     }
     
     var texts = Notification.getLocalizedTexts(notification);
-    var title = texts.title ? texts.title : "";

[Felix Dahlke, 2014/02/21 09:48:49]

How about:

var title = texts.title || "";

[saroyanm, 2014/02/21 13:39:22]

Looks cool :)

+    var title = texts.title || "";
     var message = texts.message ? texts.message.replace(/<\/?(a|strong)>/g, "") : "";
     var iconUrl = ext.getURL("icons/abp-128.png");
-    if ("notifications" in ext) 

[Felix Dahlke, 2014/02/21 09:48:49]

I personally find it a bit confusing to have ext.notifications be
chrome.notifications, but I see your point about not having the check here
Thomas.

How about calling this ext.chromeNotifications? That'd make it more obvious that
this is a chrome-specific API, and also that it's optional IMO.

[Thomas Greiner, 2014/02/21 09:53:43]

Since we want to look into a Safari specific implementation I think it makes
sense to keep it like that. The current implementation |ext.notifications =
chrome.notifications| has to change at that point, obviously, to have a more
coherent API.

[Felix Dahlke, 2014/02/21 09:56:22]

My point is basically that, in the current stage, this is _exactly_ the Chrome
API. If we emulate it in Safari, it's still gonna be the Chrome API so I think
we should point that out.

If we do have a more abstract ext.notifications and move all this code there
(I'd be a huge fan of that frankly), the name would be better of course. But now
it looks to me like we're probing for features in our abstraction layer which is
not quite what happens.

[Felix Dahlke, 2014/02/21 10:04:01]

Well, I don't have that strong feelings about this and it's a bit confusing
either way. Thomas and I disagree, so go with whatever you think makes more
sense Manvel.

[saroyanm, 2014/02/21 13:39:22]

This looks a bit tricky,
Anyway if we could move all notifications stuff to ext that will be cool, to
have our own notification API, maybe we can wait for safari test environment and
notifications safari implementation and from that point move all the codes that
make sense to ext/background.js and implement our notification API from that
point ?
Also I've changed the ext.notifications to ext.browserNotification so we can
think of it as browser specific notification if it make sense for now.

[Thomas Greiner, 2014/02/21 13:59:55]

Large parts of the notifications code is also shared with the Firefox extension
which does not use ext.* (we only use it for Chrome, Opera and Safari). Also
ext.* should only contain platform-specific code to avoid code duplication.

[saroyanm, 2014/02/21 15:11:52]

Thanks for pointing this, maybe I'll have some questions regarding this after
investigate firefox code much deeper.

-    {
-      var opt = {

[Felix Dahlke, 2014/02/21 09:48:49]

Shouldn't it be plural "opts"?

[saroyanm, 2014/02/21 13:39:22]

Done.

+    var hasLinks = activeNotification.links && activeNotification.links.length > 0;
+    if ("browserNotifications" in ext) 
+    {
+      var opts = {
         type: "basic",
         title: title,
         message: message,
         iconUrl: iconUrl,
         buttons: []
       };
       var regex = /<a>(.*?)<\/a>/g;
-      var plainMessage = texts.message ? texts.message : "";
-      while (regex.test(plainMessage))
-        opt.buttons.push({title: RegExp.$1});

[Felix Dahlke, 2014/02/21 09:48:49]

RegExp.$n is deprecated
(https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Deprecated_...)
and pretty obscure anyway IMO, why not go with plainMessage.match(regex)?

[saroyanm, 2014/02/21 13:39:22]

Good point.

I've made performance test between this two scripts in http://jsperf.com/:
scr1:
var buttons = [];
var regex = /<a>(.*?)<\/a>/g;
var plainMessage = "Adblock Plus <a>Acceptable ads citeria</a>
<strong>could</strong> not be updated automatically, please <a>update it
manually</a>";
while (match = regex.exec(plainMessage))
  buttons.push({title: match[1]});
console.log(buttons);

scr2:
var plainMessage = "Adblock Plus <a>Acceptable ads citeria</a>
<strong>could</strong> not be updated automatically, please <a>update it
manually</a>";
var result = plainMessage.match(/<a>(.*?)<\/a>/g).map(function(val)
{
   return {title:val.replace(/<\/?a>/g,'')};
});
console.log(result);

And Scr1, is doing much better, I know that in this case is difference in
milliseconds that are not making much sense, but in case if scr2 (with str.match
function) is more preferable I can change to it, or I've missed some cool
solution, please let me know whether I should change it.

[Felix Dahlke, 2014/02/21 13:48:20]

A single match call would suffice in the first example, you can iterate over the
result array.

Anyway, I don't think considering the runtime performance of this makes much
sense, we're talking about one notification with up to 5 links or something,
it's quite the opposite from a bottleneck. Go with what you think is more
readable.

+      var plainMessage = texts.message || "";
+      var match;
+      while (match = regex.exec(plainMessage))
+        opts.buttons.push({title: match[1]});
       
-      var notification = ext.notifications;
-      notification.create("", opt, function() {});
+      var notification = ext.browserNotifications;
+      notification.create("", opts, function() {});
       notification.onClosed.addListener(prepareNotificationIconAndPopup);
       notification.onButtonClicked.addListener(notificationButtonClick);
     }
     else if (hasWebkitNotifications && "createNotification" in webkitNotifications)
     {
+      if (hasLinks)
+        message += " " + ext.i18n.getMessage("notification_without_buttons");
+        
       var notification = webkitNotifications.createNotification(iconUrl, title, message);
       notification.show();
       notification.addEventListener("close", prepareNotificationIconAndPopup, false);
       notification.addEventListener("click", openNotificationLinks, false);
     }
     else
     {
-      var notification = confirm(title + "\n" + message);

[Felix Dahlke, 2014/02/21 09:48:49]

I think we should add some explanatory text here for notifications of type info
and warning, e.g.:

"Click 'OK' to open the links in this notification."

Only for notifications that actually contain links I suppose.

[saroyanm, 2014/02/21 13:39:22]

Good point.

-      if (notification == true)
+      var message = title + "\n" + message;
+      if (hasLinks)
+        message += "\n\n" + ext.i18n.getMessage("notification_with_buttons");
+        
+      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)
 {
   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);