Fix: Notification popup is broken

chrome/ext/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 563 matching lines @@
         return;
 
       chrome.contextMenus.removeAll();
       isContextMenuHidden = true;
     }
   };
 
   ext.onMessage = new BackgroundMessageEventTarget();
   // Chrome on Linux does not fully support chrome.notifications yet ( https://code.google.com/p/chromium/issues/detail?id=291485 )
   if ("notifications" in chrome && navigator.platform.indexOf("Linux") == -1)
-    ext.notifications = chrome.notifications;
+    ext.browserNotifications = chrome.notifications;

[Sebastian Noack, 2014/02/28 09:56:03]

This doesn't belong into the abstraction layer, since it doesn't abstract
anything and can't be implemented for Safari. Just use chrome.notifications in
the high-level code after checking that we are on Chrome and that this API can
be used, like we do with other APIs that only exist on Chrome.

[Thomas Greiner, 2014/02/28 10:10:55]

Without having further looked into it Safari does seem to have its own
notification API:
https://developer.apple.com/library/mac/documentation/NetworkingInternet/Conc...

Feel free to add it since we'd like to have that in Safari as well.

[Sebastian Noack, 2014/02/28 10:19:45]

No, it is not for Safari, but for Mac OS. It looks like a high-level mechanism
that lets you provide an URL that Mac OS will poll for new notifications in
order to show them to the user.

[saroyanm, 2014/02/28 11:18:43]

Guys not that much know about safari extension development but seams like they
are using HTML5 Notification the way we can use them from the web page itself
the only difference that they do not ask for permission, references:
https://developer.apple.com/library/safari/documentation/Tools/Conceptual/Saf...
https://developer.apple.com/library/safari/documentation/Tools/Conceptual/Saf...
I'm not sure why we can't implement similarly like Thomas said in ext for safari
by creating our own API by extending existing HTML5 Notification API, the only
thing will make sense to update Chrome ext.browserNotifiction API to fit the one
ext.browserNotifiction we will implement for safari.

I can try to implement both, but the only thing I do not have Mac device to test
it on safari.

[Sebastian Noack, 2014/02/28 12:35:03]

What Thomas suggested was Mac OS stuff, which we can't use in a Safari
extension. However you are talking about HTML5 notifications, which you already
use in the high-level code when chrome.notifications isn't available or not
fully supported. So that is fine.

However the purpose of the ext module is to provides cross-browser abstraction.
And is not supposed to be a collection of aliases for every single
browser-specific API, regardless whether they is or can be implemented for any
other browser!

[saroyanm, 2014/02/28 13:47:04]

Now I see your point Sebastian, make sense.
What you think @Thomas ?

[Thomas Greiner, 2014/02/28 16:04:14]

If there is actually no Safari-specific implementation for showing notifications
then I agree with Sebastian that we should not have it in ext.*

However, since this review is closed this should happen in a separate review.

[Sebastian Noack, 2014/02/28 16:59:22]

Not for rich notfications with UI elements and stuff. As far as I know, there is
only webkitNotification.createNotification.

 })();