chrome/ext/background.js - Issue 6098518317989888: Fix: Notification popup is broken

Unified Diff: chrome/ext/background.js

Issue 6098518317989888: Fix: Notification popup is broken (Closed)

Patch Set: Change back activeNotification unset Created Feb. 26, 2014, 6:31 p.m.

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

View side-by-side diff with in-line comments

Index: chrome/ext/background.js

===================================================================

--- a/chrome/ext/background.js

+++ b/chrome/ext/background.js

@@ -579,4 +579,7 @@

};

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.browserNotifications = chrome.notifications;

Sebastian Noack 2014/02/28 09:56:03 This doesn't belong into the abstraction layer, si

Thomas Greiner 2014/02/28 10:10:55 Without having further looked into it Safari does

Sebastian Noack 2014/02/28 10:19:45 No, it is not for Safari, but for Mac OS. It looks

saroyanm 2014/02/28 11:18:43 Guys not that much know about safari extension dev

Sebastian Noack 2014/02/28 12:35:03 What Thomas suggested was Mac OS stuff, which we c

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 y

On 2014/02/28 12:35:03, Sebastian Noack wrote: > On 2014/02/28 11:18:43, saroyanm wrote: > > On 2014/02/28 10:19:45, Sebastian Noack wrote: > > > On 2014/02/28 10:10:55, Thomas Greiner wrote: > > > > On 2014/02/28 09:56:03, Sebastian Noack wrote: > > > > > 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. > > > > > > > > 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. > > > > > > 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. > > > > 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. > > 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!

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 implementa

On 2014/02/28 13:47:04, saroyanm wrote: > On 2014/02/28 12:35:03, Sebastian Noack wrote: > > On 2014/02/28 11:18:43, saroyanm wrote: > > > On 2014/02/28 10:19:45, Sebastian Noack wrote: > > > > On 2014/02/28 10:10:55, Thomas Greiner wrote: > > > > > On 2014/02/28 09:56:03, Sebastian Noack wrote: > > > > > > 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. > > > > > > > > > > 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. > > > > > > > > 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. > > > > > > 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. > > > > 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! > > Now I see your point Sebastian, make sense. > What you think @Thomas ?

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 stu

On 2014/02/28 16:04:14, Thomas Greiner wrote: > On 2014/02/28 13:47:04, saroyanm wrote: > > On 2014/02/28 12:35:03, Sebastian Noack wrote: > > > On 2014/02/28 11:18:43, saroyanm wrote: > > > > On 2014/02/28 10:19:45, Sebastian Noack wrote: > > > > > On 2014/02/28 10:10:55, Thomas Greiner wrote: > > > > > > On 2014/02/28 09:56:03, Sebastian Noack wrote: > > > > > > > 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. > > > > > > > > > > > > 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. > > > > > > > > > > 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. > > > > > > > > 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. > > > > > > 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! > > > > Now I see your point Sebastian, make sense. > > What you think @Thomas ? > > 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.

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

})();

« background.js ('K') | « background.js ('k') | metadata.chrome » ('j') | notification.html » ('J')