Issue 6098518317989888: Fix: Notification popup is broken (Closed)
|
Created:
Feb. 12, 2014, 9:39 a.m. by saroyanm Modified:
March 18, 2014, 5:17 p.m. Visibility:
Public. |
DescriptionNotifications api:
I've noticed that chrome has a notification API (v28 and above) -> https://developer.chrome.com/extensions/notifications.html,
the API use template to generate notifications and also have ability to use buttons in the notification within the template, but the button will appear below notification and we again can't use HTML template.
There is a video describing the notification API -> https://developers.google.com/live/shows/83992232-1001
Anyway the current solution is still work Okey until we would like to have buttons within the notification or image beside the icon, so not sure whether we should stick to this solution and continue with other issues, or should the patch be changed to use Notifications API instead.
I haven't removed the old createHTMLNotification method for old versions of chrome, the reason was I was not sure whether ABP still need to support createHTMLNotification for old versions of browser. So in case we need to get rid of it just let me know.
Test notes:
adblockplus.js line 42:
you can change "notificationurl" to use notification.json hosted locally within your local server, or update notificationdata object.
adblockplus.js line 3611:
Also make sense to change INITIAL_DELAY to get notification.json with less delay in case you would like to test functionality.
Additional notes:
Also noticed that if we are providing several critical notifications, only the first one always are shown, so in case that behavior should be changed some additional discuss needed here.
Patch Set 1 #Patch Set 2 : chrome.notification added #
Total comments: 30
Patch Set 3 : Changes after Thomas notes #
Total comments: 18
Patch Set 4 : Linux issue and minor changes #
Total comments: 6
Patch Set 5 : Already there #
Total comments: 5
Patch Set 6 : moved code from /extcommon to ext/background #
Total comments: 16
Patch Set 7 : Felix review changes #
Total comments: 9
Patch Set 8 : Minore changes #
Total comments: 1
Patch Set 9 : Notification without button default text #
Total comments: 6
Patch Set 10 : Notification info msg correction. #Patch Set 11 : Some fixes in code #
Total comments: 8
Patch Set 12 : Popup notification fix #
Total comments: 5
Patch Set 13 : Change back activeNotification unset #
Total comments: 25
MessagesTotal messages: 81
We use notifications for a different variety of call-for-actions. Displaying text is not enough for this purpose. Therefore a notification's "message" property can contain links that the user can click on to follow links. Generally, the minimum required components we need for notifications are: - message - "yes" action (e.g. follow link, enable feature, etc.) - "cancel" and/or "cancel" action (e.g. close notification) Obviously, the more we can do the better so we need to find the best possible solution for different browser versions. So what I would suggest: - if available use "createHTMLNotification" - else if available use "chrome.notification" (doesn't seem to be properly supported on all platforms yet) - else use Web Notifications (may or may not support listening to clicks) If Web Notifications don't allow listening to clicks we need to find a different way to make sure users not only see but can also act upon our messages. (e.g. using window.confirm, inject HTML code into websites, etc.) Also we have two different kind of notifications that use this kind of mechanism: - critical: can be annoying to users - question: should not be annoying to users but noticeable
> If Web Notifications don't allow listening to clicks we need to find a different > way to make sure users not only see but can also act upon our messages. (e.g. > using window.confirm, inject HTML code into websites, etc.) I've added onclick event for webkitNotifications.createNotification, so all the links will be opened in the new tabs, Noticed a bug that took time to figure out the case, so the new chrome.notifications API do not work properly under the Linux - the buttons are not being displayed ( https://code.google.com/p/chromium/issues/detail?id=310799 ), so the onclick event again added for chrome.notifications, will work similar as webkitNotifications.createNotification, beside that windows users will have the buttons to click, another annoying thing is that new notification API support up to two buttons. Also the text for buttons I'm getting by replacing "_" in links with space, actually maybe make sense to change the Notification format to contain links in similar format: links: [ {id: "acceptable_ads_criteria", "title": "Acceptable ads"} ] What you think ?
http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... background.js:295: function notificationWindowClick() { Nit: Bracket on next line http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... background.js:300: chrome.tabs.create({ url: Utils.getDocLink(link) }); This is Chrome-specific. We do have an "openTab" method in "ext" that you can use to make it platform independent. http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... background.js:306: function notificationButtonClick(id, index) { Nit: Bracket on next line http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... background.js:307: chrome.tabs.create({url: Utils.getDocLink(activeNotification.links[index])}); This is Chrome-specific. We do have an "openTab" method in "ext" that you can use to make it platform independent. http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... background.js:316: var isWebkit = typeof webkitNotifications !== "undefined"; All browsers based on this repository use WebKit so the variable should rather be something like "isPrefixed" or "hasWebkitNotifications". http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... background.js:329: if ("notifications" in chrome) This is Chrome-specific but the adblockpluschrome project also includes our Opera and Safari version. Therefore we expose platform specific functionality only through "ext" (see chrome/ext and safari/ext). Therefore we should move this there and call it through ext.notifications.* instead of chrome.notifications.* Since you don't have a Mac, we need to postpone the Safari implementation. http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... background.js:333: links.forEach(function(link, index){ Nit: Missing space after closing bracket. http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... background.js:334: buttons.push({"title": link.replace(new RegExp("_", 'g'), " ")}); The link texts can actually be extracted from the message because they are contained in HTML anchor tags. http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... background.js:340: iconUrl: iconUrl, You can directly inline "title", "message" and "iconUrl" instead of creating variables for each. http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... background.js:340: iconUrl: iconUrl, Use |ext.getURL("icons/abp-128.png")| as the icon URL. http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... background.js:343: notification = chrome.notifications; Avoid creating global variables by adding "var". http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... background.js:344: notification.create("", opt, function(){}); Nit: Missing space after closing bracket. http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... background.js:353: notification.addEventListener("close", prepareNotificationIconAndPopup); Nit: Add third parameter http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... background.js:354: notification.addEventListener("click", notificationWindowClick); Nit: Add third parameter http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... background.js:355: } What about the "else" case here?
Thanks for the reviewing and notes Thomas, that's really helpful I fill myself getting more and more familiar hope this patch will get us much close for commit. http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... background.js:295: function notificationWindowClick() { On 2014/02/17 10:02:41, Thomas Greiner wrote: > Nit: Bracket on next line Done. http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... background.js:300: chrome.tabs.create({ url: Utils.getDocLink(link) }); On 2014/02/17 10:02:41, Thomas Greiner wrote: > This is Chrome-specific. We do have an "openTab" method in "ext" that you can > use to make it platform independent. Done. http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... background.js:306: function notificationButtonClick(id, index) { On 2014/02/17 10:02:41, Thomas Greiner wrote: > Nit: Bracket on next line Done. http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... background.js:307: chrome.tabs.create({url: Utils.getDocLink(activeNotification.links[index])}); On 2014/02/17 10:02:41, Thomas Greiner wrote: > This is Chrome-specific. We do have an "openTab" method in "ext" that you can > use to make it platform independent. Done. http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... background.js:316: var isWebkit = typeof webkitNotifications !== "undefined"; On 2014/02/17 10:02:41, Thomas Greiner wrote: > All browsers based on this repository use WebKit so the variable should rather > be something like "isPrefixed" or "hasWebkitNotifications". Done. http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... background.js:329: if ("notifications" in chrome) On 2014/02/17 10:02:41, Thomas Greiner wrote: > This is Chrome-specific but the adblockpluschrome project also includes our > Opera and Safari version. Therefore we expose platform specific functionality > only through "ext" (see chrome/ext and safari/ext). > > Therefore we should move this there and call it through ext.notifications.* > instead of chrome.notifications.* > > Since you don't have a Mac, we need to postpone the Safari implementation. Thanks, for pointing this, now I see what is under ext dir. Hope I understood your point correctly, but not sure if we need to update safari implementation if I got your point right, anyway for safari we can use webkitNotifications, what you think ? http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... background.js:334: buttons.push({"title": link.replace(new RegExp("_", 'g'), " ")}); On 2014/02/17 10:02:41, Thomas Greiner wrote: > The link texts can actually be extracted from the message because they are > contained in HTML anchor tags. Maybe it make sense also to remove anchors with their content in this case ? Because I was thinking maybe we could provide also titles for button links for updated browser versions in notification.json. http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... background.js:340: iconUrl: iconUrl, On 2014/02/17 10:02:41, Thomas Greiner wrote: > You can directly inline "title", "message" and "iconUrl" instead of creating > variables for each. while I've added also else case I'm using that variables in 3 places (notification.create, webkitNotifications.createNotification and confirm methods), not sure whether It's still preferable to use them inline, please let me know in case. http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... background.js:343: notification = chrome.notifications; On 2014/02/17 10:02:41, Thomas Greiner wrote: > Avoid creating global variables by adding "var". Done. http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... background.js:344: notification.create("", opt, function(){}); On 2014/02/17 10:02:41, Thomas Greiner wrote: > Nit: Missing space after closing bracket. Done. http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... background.js:353: notification.addEventListener("close", prepareNotificationIconAndPopup); On 2014/02/17 10:02:41, Thomas Greiner wrote: > Nit: Add third parameter Done. http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... background.js:354: notification.addEventListener("click", notificationWindowClick); On 2014/02/17 10:02:41, Thomas Greiner wrote: > Nit: Add third parameter Done. http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... background.js:355: } On 2014/02/17 10:02:41, Thomas Greiner wrote: > What about the "else" case here? Added also confirm dialog for else case, thanks for the hint.
http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... background.js:329: if ("notifications" in chrome) On 2014/02/18 10:25:08, saroyanm wrote: > On 2014/02/17 10:02:41, Thomas Greiner wrote: > > This is Chrome-specific but the adblockpluschrome project also includes our > > Opera and Safari version. Therefore we expose platform specific functionality > > only through "ext" (see chrome/ext and safari/ext). > > > > Therefore we should move this there and call it through ext.notifications.* > > instead of chrome.notifications.* > > > > Since you don't have a Mac, we need to postpone the Safari implementation. > > Thanks, for pointing this, now I see what is under ext dir. > Hope I understood your point correctly, but not sure if we need to update safari > implementation if I got your point right, anyway for safari we can use > webkitNotifications, what you think ? That's OK for now. We can add a Safari-specific implementation at a later point. http://codereview.adblockplus.org/6098518317989888/diff/5741031244955648/back... background.js:340: iconUrl: iconUrl, On 2014/02/18 10:25:08, saroyanm wrote: > On 2014/02/17 10:02:41, Thomas Greiner wrote: > > You can directly inline "title", "message" and "iconUrl" instead of creating > > variables for each. > > while I've added also else case I'm using that variables in 3 places > (notification.create, webkitNotifications.createNotification and confirm > methods), not sure whether It's still preferable to use them inline, please let > me know in case. Right, that's fine then. Although you could write it in a shorter form (e.g. var title = texts.title || "";). http://codereview.adblockplus.org/6098518317989888/diff/5733935958982656/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5733935958982656/back... background.js:322: function getNotificationLinkTitles(text) You can simply do: var titles = []; var regex = /<a>(.*?)<\/a>/g; while (regex.test(text)) titles.push({title: RegExp.$1}); return titles; http://codereview.adblockplus.org/6098518317989888/diff/5733935958982656/back... background.js:329: if(!match) Nit: Missing space between "if" and opening bracket. http://codereview.adblockplus.org/6098518317989888/diff/5733935958982656/back... background.js:331: titles.push({"title": match[1]}); That's inconsistent. The function's name suggests that it returns a list of titles while it actually returns a list of button options. http://codereview.adblockplus.org/6098518317989888/diff/5733935958982656/back... background.js:357: var buttons = getNotificationLinkTitles(message); There's no need for a variable here if you only use it once. http://codereview.adblockplus.org/6098518317989888/diff/5733935958982656/back... background.js:361: message: message, HTML tags need to be stripped from message. http://codereview.adblockplus.org/6098518317989888/diff/5733935958982656/back... background.js:368: notification.onClicked.addListener(openNotificationLinks); We don't need this anymore if there's a button for each link anyway. http://codereview.adblockplus.org/6098518317989888/diff/5733935958982656/back... background.js:380: var notification = confirm(title+"\n"+message); Nit: Missing spaces. |confirm(title + "\n" + message);| http://codereview.adblockplus.org/6098518317989888/diff/5733935958982656/chro... File chrome/ext/common.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5733935958982656/chro... chrome/ext/common.js:116: if ("notifications" in chrome) Add a check for the OS name because Linux support is not sufficient as of right now. Also add an explanation with a link to https://code.google.com/p/chromium/issues/detail?id=291485 http://codereview.adblockplus.org/6098518317989888/diff/5733935958982656/noti... File notification.html (right): http://codereview.adblockplus.org/6098518317989888/diff/5733935958982656/noti... notification.html:29: <script src="ext/common.js"></script> Well spotted. However, notifications.js also makes a call to ext.windows.getLastFocused() which is not included in common.js
Hope this will good for us to commit. http://codereview.adblockplus.org/6098518317989888/diff/5733935958982656/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5733935958982656/back... background.js:322: function getNotificationLinkTitles(text) On 2014/02/18 14:26:17, Thomas Greiner wrote: > You can simply do: > > var titles = []; > var regex = /<a>(.*?)<\/a>/g; > while (regex.test(text)) > titles.push({title: RegExp.$1}); > return titles; I like this :) http://codereview.adblockplus.org/6098518317989888/diff/5733935958982656/back... background.js:329: if(!match) On 2014/02/18 14:26:17, Thomas Greiner wrote: > Nit: Missing space between "if" and opening bracket. I have to be more attentive, I thought I fixed this. http://codereview.adblockplus.org/6098518317989888/diff/5733935958982656/back... background.js:331: titles.push({"title": match[1]}); On 2014/02/18 14:26:17, Thomas Greiner wrote: > That's inconsistent. The function's name suggests that it returns a list of > titles while it actually returns a list of button options. Done. http://codereview.adblockplus.org/6098518317989888/diff/5733935958982656/back... background.js:357: var buttons = getNotificationLinkTitles(message); On 2014/02/18 14:26:17, Thomas Greiner wrote: > There's no need for a variable here if you only use it once. Done. http://codereview.adblockplus.org/6098518317989888/diff/5733935958982656/back... background.js:361: message: message, On 2014/02/18 14:26:17, Thomas Greiner wrote: > HTML tags need to be stripped from message. Done. http://codereview.adblockplus.org/6098518317989888/diff/5733935958982656/back... background.js:368: notification.onClicked.addListener(openNotificationLinks); On 2014/02/18 14:26:17, Thomas Greiner wrote: > We don't need this anymore if there's a button for each link anyway. Done. http://codereview.adblockplus.org/6098518317989888/diff/5733935958982656/back... background.js:380: var notification = confirm(title+"\n"+message); On 2014/02/18 14:26:17, Thomas Greiner wrote: > Nit: Missing spaces. |confirm(title + "\n" + message);| Done. http://codereview.adblockplus.org/6098518317989888/diff/5733935958982656/chro... File chrome/ext/common.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5733935958982656/chro... chrome/ext/common.js:116: if ("notifications" in chrome) On 2014/02/18 14:26:17, Thomas Greiner wrote: > Add a check for the OS name because Linux support is not sufficient as of right > now. Also add an explanation with a link to > https://code.google.com/p/chromium/issues/detail?id=291485 Done. http://codereview.adblockplus.org/6098518317989888/diff/5733935958982656/noti... File notification.html (right): http://codereview.adblockplus.org/6098518317989888/diff/5733935958982656/noti... notification.html:29: <script src="ext/common.js"></script> On 2014/02/18 14:26:17, Thomas Greiner wrote: > Well spotted. However, notifications.js also makes a call to > ext.windows.getLastFocused() which is not included in common.js Good point.
Almost there. http://codereview.adblockplus.org/6098518317989888/diff/5738600293466112/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5738600293466112/back... background.js:338: var message = texts.message ? texts.message.replace(new RegExp("<(a|\/a)>", 'g'), "") : ""; You can just use the literal version (also don't forget about the <strong> tags which can also be included in the message) /<\/?(a|strong)>/g http://codereview.adblockplus.org/6098518317989888/diff/5738600293466112/back... background.js:350: while (regex.test(texts.message ? texts.message : "")) Can you move this outside of the while loop? http://codereview.adblockplus.org/6098518317989888/diff/5738600293466112/chro... File chrome/ext/common.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5738600293466112/chro... chrome/ext/common.js:116: // Chrome on Linux, not fully support chrome.notifications ( https://code.google.com/p/chromium/issues/detail?id=291485 ) Grammar nit: Chrome on Linux does not fully support chrome.notifications yet
Fingers crossed :) http://codereview.adblockplus.org/6098518317989888/diff/5738600293466112/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5738600293466112/back... background.js:338: var message = texts.message ? texts.message.replace(new RegExp("<(a|\/a)>", 'g'), "") : ""; On 2014/02/18 16:44:25, Thomas Greiner wrote: > You can just use the literal version (also don't forget about the <strong> tags > which can also be included in the message) > /<\/?(a|strong)>/g Done. http://codereview.adblockplus.org/6098518317989888/diff/5738600293466112/back... background.js:350: while (regex.test(texts.message ? texts.message : "")) On 2014/02/18 16:44:25, Thomas Greiner wrote: > Can you move this outside of the while loop? Done. http://codereview.adblockplus.org/6098518317989888/diff/5738600293466112/chro... File chrome/ext/common.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5738600293466112/chro... chrome/ext/common.js:116: // Chrome on Linux, not fully support chrome.notifications ( https://code.google.com/p/chromium/issues/detail?id=291485 ) On 2014/02/18 16:44:25, Thomas Greiner wrote: > Grammar nit: Chrome on Linux does not fully support chrome.notifications yet oops.
http://codereview.adblockplus.org/6098518317989888/diff/6563422992859136/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/6563422992859136/back... background.js:350: var message = texts.message ? texts.message : ""; To avoid any unintended sideeffects we shouldn't declare a variable twice so I'd rename it to "plainMessage". http://codereview.adblockplus.org/6098518317989888/diff/6563422992859136/chro... File chrome/ext/common.js (right): http://codereview.adblockplus.org/6098518317989888/diff/6563422992859136/chro... chrome/ext/common.js:116: // Chrome on Linux does not fully support chrome.notifications yet ( https://code.google.com/p/chromium/issues/detail?id=291485 ) Move all of the added code to ext/chrome/background.js
Changes according last review comments. Hope the change for ext/background is the correct, can you please give me a hint, why in this case move the code is preferable, thanks in advance. http://codereview.adblockplus.org/6098518317989888/diff/6563422992859136/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/6563422992859136/back... background.js:350: var message = texts.message ? texts.message : ""; On 2014/02/19 09:25:56, Thomas Greiner wrote: > To avoid any unintended sideeffects we shouldn't declare a variable twice so I'd > rename it to "plainMessage". Done. http://codereview.adblockplus.org/6098518317989888/diff/6563422992859136/chro... File chrome/ext/common.js (right): http://codereview.adblockplus.org/6098518317989888/diff/6563422992859136/chro... chrome/ext/common.js:116: // Chrome on Linux does not fully support chrome.notifications yet ( https://code.google.com/p/chromium/issues/detail?id=291485 ) On 2014/02/19 09:25:56, Thomas Greiner wrote: > Move all of the added code to ext/chrome/background.js Done. Thomas, can you please give me just a hint regarding this change ?
http://codereview.adblockplus.org/6098518317989888/diff/6563422992859136/chro... File chrome/ext/common.js (right): http://codereview.adblockplus.org/6098518317989888/diff/6563422992859136/chro... chrome/ext/common.js:116: // Chrome on Linux does not fully support chrome.notifications yet ( https://code.google.com/p/chromium/issues/detail?id=291485 ) On 2014/02/19 13:14:42, saroyanm wrote: > On 2014/02/19 09:25:56, Thomas Greiner wrote: > > Move all of the added code to ext/chrome/background.js > > Done. > Thomas, can you please give me just a hint regarding this change ? ext/background.js is used in the background page and other pages that have direct access to the chrome.* API. ext/content.js is used in content scripts. ext/common.js contains functionality that is used both in the background page and in content scripts. This code (ext.notifications.*), however, is only used in background.js and is not required in the content scripts. Therefore it makes more sense to put it in ext/background.js.
On 2014/02/20 15:58:36, Thomas Greiner wrote: > http://codereview.adblockplus.org/6098518317989888/diff/6563422992859136/chro... > File chrome/ext/common.js (right): > > http://codereview.adblockplus.org/6098518317989888/diff/6563422992859136/chro... > chrome/ext/common.js:116: // Chrome on Linux does not fully support > chrome.notifications yet ( > https://code.google.com/p/chromium/issues/detail?id=291485 ) > On 2014/02/19 13:14:42, saroyanm wrote: > > On 2014/02/19 09:25:56, Thomas Greiner wrote: > > > Move all of the added code to ext/chrome/background.js > > > > Done. > > Thomas, can you please give me just a hint regarding this change ? > > ext/background.js is used in the background page and other pages that have > direct access to the chrome.* API. > ext/content.js is used in content scripts. > ext/common.js contains functionality that is used both in the background page > and in content scripts. > > This code (ext.notifications.*), however, is only used in background.js and is > not required in the content scripts. Therefore it makes more sense to put it in > ext/background.js. Got it! thanks!
LGTM
Here's some more from me then :) http://codereview.adblockplus.org/6098518317989888/diff/5728116278296576/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5728116278296576/back... background.js:337: var title = texts.title ? texts.title : ""; How about: var title = texts.title || ""; http://codereview.adblockplus.org/6098518317989888/diff/5728116278296576/back... background.js:340: if ("notifications" in ext) 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. http://codereview.adblockplus.org/6098518317989888/diff/5728116278296576/back... background.js:342: var opt = { Shouldn't it be plural "opts"? http://codereview.adblockplus.org/6098518317989888/diff/5728116278296576/back... background.js:352: opt.buttons.push({title: RegExp.$1}); 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)? http://codereview.adblockplus.org/6098518317989888/diff/5728116278296576/back... background.js:368: var notification = confirm(title + "\n" + message); 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.
http://codereview.adblockplus.org/6098518317989888/diff/5728116278296576/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5728116278296576/back... background.js:340: if ("notifications" in ext) On 2014/02/21 09:48:49, Felix H. Dahlke wrote: > 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. 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.
http://codereview.adblockplus.org/6098518317989888/diff/5728116278296576/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5728116278296576/back... background.js:340: if ("notifications" in ext) On 2014/02/21 09:53:43, Thomas Greiner wrote: > On 2014/02/21 09:48:49, Felix H. Dahlke wrote: > > 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. > > 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. 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.
http://codereview.adblockplus.org/6098518317989888/diff/5728116278296576/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5728116278296576/back... background.js:340: if ("notifications" in ext) On 2014/02/21 09:56:22, Felix H. Dahlke wrote: > On 2014/02/21 09:53:43, Thomas Greiner wrote: > > On 2014/02/21 09:48:49, Felix H. Dahlke wrote: > > > 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. > > > > 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. > > 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. 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.
Hope I understand guys your point regarding the notifications correctly. http://codereview.adblockplus.org/6098518317989888/diff/5728116278296576/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5728116278296576/back... background.js:337: var title = texts.title ? texts.title : ""; On 2014/02/21 09:48:49, Felix H. Dahlke wrote: > How about: > > var title = texts.title || ""; Looks cool :) http://codereview.adblockplus.org/6098518317989888/diff/5728116278296576/back... background.js:340: if ("notifications" in ext) On 2014/02/21 10:04:01, Felix H. Dahlke wrote: > On 2014/02/21 09:56:22, Felix H. Dahlke wrote: > > On 2014/02/21 09:53:43, Thomas Greiner wrote: > > > On 2014/02/21 09:48:49, Felix H. Dahlke wrote: > > > > 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. > > > > > > 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. > > > > 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. > > 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. 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. http://codereview.adblockplus.org/6098518317989888/diff/5728116278296576/back... background.js:342: var opt = { On 2014/02/21 09:48:49, Felix H. Dahlke wrote: > Shouldn't it be plural "opts"? Done. http://codereview.adblockplus.org/6098518317989888/diff/5728116278296576/back... background.js:352: opt.buttons.push({title: RegExp.$1}); On 2014/02/21 09:48:49, Felix H. Dahlke wrote: > 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)? 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. http://codereview.adblockplus.org/6098518317989888/diff/5728116278296576/back... background.js:368: var notification = confirm(title + "\n" + message); On 2014/02/21 09:48:49, Felix H. Dahlke wrote: > 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. Good point.
http://codereview.adblockplus.org/6098518317989888/diff/5728116278296576/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5728116278296576/back... background.js:352: opt.buttons.push({title: RegExp.$1}); On 2014/02/21 13:39:22, saroyanm wrote: > On 2014/02/21 09:48:49, Felix H. Dahlke wrote: > > 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)? > > 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. 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.
Almost there :) http://codereview.adblockplus.org/6098518317989888/diff/5723151296102400/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5723151296102400/back... background.js:351: while (match = regex.exec(plainMessage)) As I said in my other comment, feel free to stick to this approach if you prefer it to the map readability-wise. But calling regex.exec once is sufficient, the result is an array. Also, that variable shouldn't be a global. http://codereview.adblockplus.org/6098518317989888/diff/5723151296102400/back... background.js:371: message += "\n\nClick 'OK' to open the links in this notification."; This needs to be translated :) You can just add the en-US variant, the others will be added after the translation phase.
http://codereview.adblockplus.org/6098518317989888/diff/5728116278296576/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5728116278296576/back... background.js:340: if ("notifications" in ext) On 2014/02/21 13:39:22, saroyanm wrote: > On 2014/02/21 10:04:01, Felix H. Dahlke wrote: > > On 2014/02/21 09:56:22, Felix H. Dahlke wrote: > > > On 2014/02/21 09:53:43, Thomas Greiner wrote: > > > > On 2014/02/21 09:48:49, Felix H. Dahlke wrote: > > > > > 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. > > > > > > > > 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. > > > > > > 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. > > > > 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. > > 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. 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.
http://codereview.adblockplus.org/6098518317989888/diff/5723151296102400/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5723151296102400/back... background.js:351: while (match = regex.exec(plainMessage)) Avoid making "match" a global variable by prepending "var". http://codereview.adblockplus.org/6098518317989888/diff/5723151296102400/back... background.js:351: while (match = regex.exec(plainMessage)) On 2014/02/21 13:53:40, Felix H. Dahlke wrote: > As I said in my other comment, feel free to stick to this approach if you prefer > it to the map readability-wise. But calling regex.exec once is sufficient, the > result is an array. Also, that variable shouldn't be a global. @Felix Calling it once is not enough. regex.exec returns an array but only for the first match (e.g. ["<a>test</a>", "test"]). Calling exec the second time returns the second match and so on.
http://codereview.adblockplus.org/6098518317989888/diff/5723151296102400/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5723151296102400/back... background.js:351: while (match = regex.exec(plainMessage)) On 2014/02/21 14:35:53, Thomas Greiner wrote: > On 2014/02/21 13:53:40, Felix H. Dahlke wrote: > > As I said in my other comment, feel free to stick to this approach if you > prefer > > it to the map readability-wise. But calling regex.exec once is sufficient, the > > result is an array. Also, that variable shouldn't be a global. > > @Felix Calling it once is not enough. regex.exec returns an array but only for > the first match (e.g. ["<a>test</a>", "test"]). Calling exec the second time > returns the second match and so on. Oh yeah, we have multiple links of course, silly me :)
Fingers crossed. http://codereview.adblockplus.org/6098518317989888/diff/5723151296102400/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5723151296102400/back... background.js:351: while (match = regex.exec(plainMessage)) On 2014/02/21 13:53:40, Felix H. Dahlke wrote: > As I said in my other comment, feel free to stick to this approach if you prefer > it to the map readability-wise. But calling regex.exec once is sufficient, the > result is an array. Also, that variable shouldn't be a global. Thomas was faster :) Actually I'm not sure but is there a preferability to declare literal regExp by assigning to variable ? If no, then after adding the var match, maybe using this script will be much shorter: plainMessage.match(/<a>(.*?)<\/a>/g).map(function(val) { opts.buttons.push({title: val.replace(/<\/?a>/g, '')}); }); What you think @Thomas on this ? http://codereview.adblockplus.org/6098518317989888/diff/5723151296102400/back... background.js:371: message += "\n\nClick 'OK' to open the links in this notification."; On 2014/02/21 13:53:40, Felix H. Dahlke wrote: > This needs to be translated :) You can just add the en-US variant, the others > will be added after the translation phase. Thanks for pointing this.
http://codereview.adblockplus.org/6098518317989888/diff/5728116278296576/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5728116278296576/back... background.js:340: if ("notifications" in ext) On 2014/02/21 13:59:55, Thomas Greiner wrote: > On 2014/02/21 13:39:22, saroyanm wrote: > > On 2014/02/21 10:04:01, Felix H. Dahlke wrote: > > > On 2014/02/21 09:56:22, Felix H. Dahlke wrote: > > > > On 2014/02/21 09:53:43, Thomas Greiner wrote: > > > > > On 2014/02/21 09:48:49, Felix H. Dahlke wrote: > > > > > > 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. > > > > > > > > > > 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. > > > > > > > > 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. > > > > > > 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. > > > > 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. > > 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. Thanks for pointing this, maybe I'll have some questions regarding this after investigate firefox code much deeper.
http://codereview.adblockplus.org/6098518317989888/diff/5723151296102400/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5723151296102400/back... background.js:351: while (match = regex.exec(plainMessage)) On 2014/02/21 15:03:25, saroyanm wrote: > On 2014/02/21 13:53:40, Felix H. Dahlke wrote: > > As I said in my other comment, feel free to stick to this approach if you > prefer > > it to the map readability-wise. But calling regex.exec once is sufficient, the > > result is an array. Also, that variable shouldn't be a global. > > Thomas was faster :) > Actually I'm not sure but is there a preferability to declare literal regExp by > assigning to variable ? If no, then after adding the var match, maybe using this > script will be much shorter: > plainMessage.match(/<a>(.*?)<\/a>/g).map(function(val) > { > opts.buttons.push({title: val.replace(/<\/?a>/g, '')}); > }); > > What you think @Thomas on this ? This is of course an option but I prefer the current approach simply because the regular expression already gives us the plain-text string. Therefore I'd leave it as it is and just apply the change that I suggested in my previous comment.
http://codereview.adblockplus.org/6098518317989888/diff/5723151296102400/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5723151296102400/back... background.js:351: while (match = regex.exec(plainMessage)) On 2014/02/21 15:18:43, Thomas Greiner wrote: > On 2014/02/21 15:03:25, saroyanm wrote: > > On 2014/02/21 13:53:40, Felix H. Dahlke wrote: > > > As I said in my other comment, feel free to stick to this approach if you > > prefer > > > it to the map readability-wise. But calling regex.exec once is sufficient, > the > > > result is an array. Also, that variable shouldn't be a global. > > > > Thomas was faster :) > > Actually I'm not sure but is there a preferability to declare literal regExp > by > > assigning to variable ? If no, then after adding the var match, maybe using > this > > script will be much shorter: > > plainMessage.match(/<a>(.*?)<\/a>/g).map(function(val) > > { > > opts.buttons.push({title: val.replace(/<\/?a>/g, '')}); > > }); > > > > What you think @Thomas on this ? > > This is of course an option but I prefer the current approach simply because the > regular expression already gives us the plain-text string. Therefore I'd leave > it as it is and just apply the change that I suggested in my previous comment. Already applied. The case is I didn't prepend because of syntax error, so I've declared a variable.
http://codereview.adblockplus.org/6098518317989888/diff/5659645909663744/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5659645909663744/back... background.js:362: var notification = webkitNotifications.createNotification(iconUrl, title, message); Shouldn't this notification's message also include a similar text to confirm_window_notification since it's also opening all links when clicking on it? However, there is no "OK" button on this one so we probably can't reuse the same one for this which is a pity.
On 2014/02/21 15:54:51, Thomas Greiner wrote: > http://codereview.adblockplus.org/6098518317989888/diff/5659645909663744/back... > File background.js (right): > > http://codereview.adblockplus.org/6098518317989888/diff/5659645909663744/back... > background.js:362: var notification = > webkitNotifications.createNotification(iconUrl, title, message); > Shouldn't this notification's message also include a similar text to > confirm_window_notification since it's also opening all links when clicking on > it? > > However, there is no "OK" button on this one so we probably can't reuse the same > one for this which is a pity. I have another pity news, actually I couldn't find way for line break. seams like it removes any special character and do not accept HTML, so anyway if we will implement that, then we should know that the message will be appended.
On 2014/02/21 16:22:56, saroyanm wrote: > On 2014/02/21 15:54:51, Thomas Greiner wrote: > > > http://codereview.adblockplus.org/6098518317989888/diff/5659645909663744/back... > > File background.js (right): > > > > > http://codereview.adblockplus.org/6098518317989888/diff/5659645909663744/back... > > background.js:362: var notification = > > webkitNotifications.createNotification(iconUrl, title, message); > > Shouldn't this notification's message also include a similar text to > > confirm_window_notification since it's also opening all links when clicking on > > it? > > > > However, there is no "OK" button on this one so we probably can't reuse the > same > > one for this which is a pity. > > I have another pity news, actually I couldn't find way for line break. seams > like it removes any special character and do not accept HTML, so anyway if we > will implement that, then we should know that the message will be appended. Issue updated with new patch.
Aaaaalmost there :) http://codereview.adblockplus.org/6098518317989888/diff/5703128158568448/_loc... File _locales/en_US/messages.json (right): http://codereview.adblockplus.org/6098518317989888/diff/5703128158568448/_loc... _locales/en_US/messages.json:103: "message": "Click 'OK' to open the links in this notification." I'd prefer this: "Click 'OK' to open all links in this notification." http://codereview.adblockplus.org/6098518317989888/diff/5703128158568448/_loc... _locales/en_US/messages.json:106: "message": "Click on notification to open links." I'd prefer this: "Click on the notification to open all links in it." http://codereview.adblockplus.org/6098518317989888/diff/5703128158568448/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5703128158568448/back... background.js:364: message += " " + ext.i18n.getMessage("notification_without_buttons"); Should be in a new paragraph IMO, likewise below for the message with buttons.
Changed the messages for notification. http://codereview.adblockplus.org/6098518317989888/diff/5703128158568448/_loc... File _locales/en_US/messages.json (right): http://codereview.adblockplus.org/6098518317989888/diff/5703128158568448/_loc... _locales/en_US/messages.json:103: "message": "Click 'OK' to open the links in this notification." On 2014/02/21 19:17:37, Felix H. Dahlke wrote: > I'd prefer this: "Click 'OK' to open all links in this notification." Done. http://codereview.adblockplus.org/6098518317989888/diff/5703128158568448/_loc... _locales/en_US/messages.json:106: "message": "Click on notification to open links." On 2014/02/21 19:17:37, Felix H. Dahlke wrote: > I'd prefer this: "Click on the notification to open all links in it." Done. http://codereview.adblockplus.org/6098518317989888/diff/5703128158568448/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5703128158568448/back... background.js:364: message += " " + ext.i18n.getMessage("notification_without_buttons"); On 2014/02/21 19:17:37, Felix H. Dahlke wrote: > Should be in a new paragraph IMO, likewise below for the message with buttons. webkitNotifications.createNotification removes any special character and do not accept HTML, so I think we don't have a good solution here unfortunately.
Hi guys, sorry for this last update. Actually I've noticed that "prepareNotificationIconAndPopup" used to be called only on notification close, so I've removed it not to make unnecessary function calls, But anyway "prepareNotificationIconAndPopup" looks doubtful and basically that is the case why the notification message is appearing in popup window after closing desktop notification, as discussed under current trello ticket: https://trello.com/c/2JexnYDW/367-quick-fix-x-in-icon-popup-notification-box-... I'll create separate issue for that as discussed under the ticket and will update that function separately meanwhile I think we are okey with this solution. So waiting hopefully for your last review of the issue :)
To be more concrete regarding not showing notification in popup window after closing desktop notificaiton -> we should handle to remove activeNotification (asign null value) on desktopNotification close.
On 2014/02/22 17:03:01, saroyanm wrote: > Hi guys, > sorry for this last update. > Actually I've noticed that "prepareNotificationIconAndPopup" used to be called > only on notification close, so I've removed it not to make unnecessary function > calls, > But anyway "prepareNotificationIconAndPopup" looks doubtful and basically that > is the case why the notification message is appearing in popup window after > closing desktop notification, as discussed under current trello ticket: > https://trello.com/c/2JexnYDW/367-quick-fix-x-in-icon-popup-notification-box-... > > I'll create separate issue for that as discussed under the ticket and will > update that function separately meanwhile I think we are okey with this > solution. > So waiting hopefully for your last review of the issue :) Looking further into this it might be worth fixing it in here since it relies on the notification's onClicked function to be executed when the notification is closed which, however, is currently only done in notification.html. On 2014/02/22 17:11:26, saroyanm wrote: > To be more concrete regarding not showing notification in popup window after > closing desktop notificaiton -> we should handle to remove activeNotification > (asign null value) on desktopNotification close. That's already handled in Notification.getNextToShow().
http://codereview.adblockplus.org/6098518317989888/diff/5890822524370944/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5890822524370944/back... background.js:292: iconAnimation.update(activeNotification.severity); Felix, what you think do we need this function here ? Actually not sure what we are using it for ?
> On 2014/02/22 17:11:26, saroyanm wrote: > > To be more concrete regarding not showing notification in popup window after > > closing desktop notificaiton -> we should handle to remove activeNotification > > (asign null value) on desktopNotification close. > > That's already handled in Notification.getNextToShow(). Actually I'm not sure regarding that we are assigning activeNotification in showNotification method, So I think It make sense event get rid of from prepareIconAndPopup method in case we don't need iconAnimation.update method there. So will do some tests and update patch soon.
> Actually I'm not sure regarding that we are assigning activeNotification in > showNotification method, > So I think It make sense event get rid of from prepareIconAndPopup method in > case we don't need iconAnimation.update method there. > So will do some tests and update patch soon. getNextToShow does not unset activeNotification. What it does is mark it as shown so that it's not set as activeNotification again in the future.
On 2014/02/26 16:25:46, Thomas Greiner wrote: > > Actually I'm not sure regarding that we are assigning activeNotification in > > showNotification method, > > So I think It make sense event get rid of from prepareIconAndPopup method in > > case we don't need iconAnimation.update method there. > > So will do some tests and update patch soon. > > getNextToShow does not unset activeNotification. What it does is mark it as > shown so that it's not set as activeNotification again in the future. Yes I see, so that why I've unset it in prepareNotificationIconAndPopup with last patch.
http://codereview.adblockplus.org/6098518317989888/diff/5890822524370944/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5890822524370944/back... background.js:292: iconAnimation.update(activeNotification.severity); On 2014/02/26 15:52:18, saroyanm wrote: > Felix, what you think do we need this function here ? > Actually not sure what we are using it for ? 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. http://codereview.adblockplus.org/6098518317989888/diff/5890822524370944/back... background.js:375: var notification = confirm(message); 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. http://codereview.adblockplus.org/6098518317989888/diff/5890822524370944/back... background.js:376: if (notification == true) == true is redundant http://codereview.adblockplus.org/6098518317989888/diff/5890822524370944/back... background.js:379: prepareNotificationIconAndPopup(); We should call prepareNotificationIconAndPopup either way, not in the else case. http://codereview.adblockplus.org/6098518317989888/diff/5758531089203200/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5758531089203200/back... background.js:290: Why did you change this logic?
http://codereview.adblockplus.org/6098518317989888/diff/5758531089203200/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5758531089203200/back... background.js:290: On 2014/02/26 17:22:58, Felix H. Dahlke wrote: > Why did you change this logic? @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.
http://codereview.adblockplus.org/6098518317989888/diff/5758531089203200/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5758531089203200/back... background.js:290: On 2014/02/26 17:37:35, Thomas Greiner wrote: > On 2014/02/26 17:22:58, Felix H. Dahlke wrote: > > Why did you change this logic? > > @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. 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.
> 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. What we could do is pass a parameter "keepActiveNotification" to prepareNotificationIconAndPopup which either sets the onShow listener or sets activeNotification to null. In the case of createHTMLNotification this parameter should then be set to true, in all other cases to false.
http://codereview.adblockplus.org/6098518317989888/diff/5890822524370944/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5890822524370944/back... background.js:379: prepareNotificationIconAndPopup(); On 2014/02/26 17:22:58, Felix H. Dahlke wrote: > We should call prepareNotificationIconAndPopup either way, not in the else case. 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 ?
http://codereview.adblockplus.org/6098518317989888/diff/5890822524370944/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5890822524370944/back... background.js:379: prepareNotificationIconAndPopup(); On 2014/02/26 18:00:29, saroyanm wrote: > On 2014/02/26 17:22:58, Felix H. Dahlke wrote: > > We should call prepareNotificationIconAndPopup either way, not in the else > case. > > 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 ? 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.
http://codereview.adblockplus.org/6098518317989888/diff/5890822524370944/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5890822524370944/back... background.js:292: iconAnimation.update(activeNotification.severity); On 2014/02/26 17:22:58, Felix H. Dahlke wrote: > On 2014/02/26 15:52:18, saroyanm wrote: > > Felix, what you think do we need this function here ? > > Actually not sure what we are using it for ? > > 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. Got it. http://codereview.adblockplus.org/6098518317989888/diff/5758531089203200/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5758531089203200/back... background.js:290: On 2014/02/26 17:47:10, Felix H. Dahlke wrote: > On 2014/02/26 17:37:35, Thomas Greiner wrote: > > On 2014/02/26 17:22:58, Felix H. Dahlke wrote: > > > Why did you change this logic? > > > > @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. > > 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. @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 ?
On 2014/02/26 18:15:34, saroyanm wrote: > http://codereview.adblockplus.org/6098518317989888/diff/5890822524370944/back... > File background.js (right): > > http://codereview.adblockplus.org/6098518317989888/diff/5890822524370944/back... > background.js:292: iconAnimation.update(activeNotification.severity); > On 2014/02/26 17:22:58, Felix H. Dahlke wrote: > > On 2014/02/26 15:52:18, saroyanm wrote: > > > Felix, what you think do we need this function here ? > > > Actually not sure what we are using it for ? > > > > 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. > > Got it. > > http://codereview.adblockplus.org/6098518317989888/diff/5758531089203200/back... > File background.js (right): > > http://codereview.adblockplus.org/6098518317989888/diff/5758531089203200/back... > background.js:290: > On 2014/02/26 17:47:10, Felix H. Dahlke wrote: > > On 2014/02/26 17:37:35, Thomas Greiner wrote: > > > On 2014/02/26 17:22:58, Felix H. Dahlke wrote: > > > > Why did you change this logic? > > > > > > @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. > > > > 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. > > @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 ? prepareNotificationIconAndPopup needs to be called when the notification is closed and activeNotification.onClicked needs to be called when the notification gets shown, that's all there is to it. We just need to make sure these two calls happen at the correct time for all the notification types we have now. And the function prepareNotificationIconAndPopup should stay how it was.
http://codereview.adblockplus.org/6098518317989888/diff/5758531089203200/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5758531089203200/back... background.js:290: On 2014/02/26 18:15:34, saroyanm wrote: > On 2014/02/26 17:47:10, Felix H. Dahlke wrote: > > On 2014/02/26 17:37:35, Thomas Greiner wrote: > > > On 2014/02/26 17:22:58, Felix H. Dahlke wrote: > > > > Why did you change this logic? > > > > > > @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. > > > > 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. > > @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 ? 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.
On 2014/02/26 18:25:08, saroyanm wrote: > 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 That's pretty much exactly what happens now, isn't it? > but anyway I can't see if the iconAnimation is working. Can you check if it works without the current revision, i.e. without any of your changes? If not that definitely needs fixing.
On 2014/02/26 18:30:03, Felix H. Dahlke wrote: > On 2014/02/26 18:25:08, saroyanm wrote: > > 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 > > That's pretty much exactly what happens now, isn't it? No, acutally now I've put activeNotification unset back to it's place as it was before (actually original implementation), And Original implementation works this how: After closing Desktop notification the prepareNotificationIconAndPopup method is called, which create onclose(activeNotification.onClicked) handler which is called in case of popup notification 'x' button click and it's basically unset activeNotification and stop iconAnimation, but beforehand the icon animation is not stopped, so that mean after user close desktop notification the icon should animate and the notification appear in popup window and only stop appearing and icon stop to animate in case user close from popup, maybe that is the usual behavior I'm not sure. > > but anyway I can't see if the iconAnimation is working. > > Can you check if it works without the current revision, i.e. without any of your > changes? If not that definitely needs fixing. I'll check it right now, I need old browser for it.
On 2014/02/26 18:40:04, saroyanm wrote: > After closing Desktop notification the prepareNotificationIconAndPopup method is > called, which create onclose(activeNotification.onClicked) handler which is > called in case of popup notification 'x' button click and it's basically unset > activeNotification and stop iconAnimation, but beforehand the icon animation is > not stopped, so that mean after user close desktop notification the icon should > animate and the notification appear in popup window and only stop appearing and > icon stop to animate in case user close from popup, maybe that is the usual > behavior I'm not sure. activeNotification.onClicked is not called when the notification is closed, but when it's shown (see notification.js). However, I agree that calling prepareNotificationIconAndPopup after the notification is closed doesn't seem to make sense. activeNotification should be 0. I don't see why we need to call anything when the notification is closed in fact, since the animation was already stopped by activeAnimation.onClicked.
> activeNotification.onClicked is not called when the notification is closed, but > when it's shown (see notification.js). Yes exactly, it's not called onClicked is assigned to activeNotification and only called if popup is opened, because when we are calling createHTMLnotification it's not yet assigned, not sure why it was implemented that way, from my point of view it was implemented to still show notification in case user ignore it by closing desktop notification.
> Can you check if it works without the current revision, i.e. without any of your > changes? If not that definitely needs fixing. I've checked and it's not working. Icon didn't changed.
Guys while the implementation of notification.js is not really functional: * iconAnimation is not working * notification appears in popup window after closing desktop notification. I suggest current changes to close this ticket: 1. Maybe it make sense to take out "activeNotification = null" from activeNotification.onClicked function as it was done with patch #12 and commit changes while in that case everything functioning and the notification is not appearing in popup after closing desktop notification. 2. After I can create a ticket to understand why iconAnimation is not functioning and fix also that, also I think prepareNotificationIconAndPopup method should be changed: * rename method "prepareNotificationIconAndPopup" method to "stopAnimationUnsetNotification", call iconAnimation.update in showNotification method and iconAnimation.stop method on notification close also unset activeNotification by calling "stopAnimationUnsetNotification" method. * remove notification.onClicked case logic from notification.js With that change we will have functioning notification for all browser versions and will be able to finally close the issue, If you prefer I can do that changes in point 2 here. What you think guys ?
If Felix agrees we can do the suggested change in a separate review. Anyway, LGTM with the nit addressed. http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/back... background.js:375: if (confirm(message)) Nit: Change to… if (confirm(message)) openNotificationLinks(); prepareNotificationIconAndPopup();
On 2014/02/27 09:30:39, saroyanm wrote: > Guys while the implementation of notification.js is not really functional: > * iconAnimation is not working Oh crap, it's true. Sebastian could also reproduce it and is looking into it now. So let's just pretend it's not an issue as far as this review is concerned :) > * notification appears in popup window after closing desktop notification. That's desired behaviour actually. Critical notifications need to be somewhat persistent since we need them for extreme cases, i.e. when we get thrown out of the Chrome Web Store. > What you think guys ? Yeah, let's get this closed and handle the icon animation not working as a separate issue, Sebastian is already on it.
LGTM with mine and greiner's nit addressed! http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/back... File background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/back... background.js:292: iconAnimation.update(activeNotification.severity); Nit: Probably whitespace differences, this shouldn't register as a diff.
Noticed one thing I didn't notice before: http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/noti... File notification.html (right): http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/noti... notification.html:30: <script src="ext/background.js"></script> Guess I'm missing something, but what is this necessary for?
http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/noti... File notification.html (right): http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/noti... notification.html:30: <script src="ext/background.js"></script> On 2014/02/27 14:14:43, Felix H. Dahlke wrote: > Guess I'm missing something, but what is this necessary for? This fixes notification.js which depends on ext.*
http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/chro... File chrome/ext/background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/chro... chrome/ext/background.js:584: ext.browserNotifications = chrome.notifications; 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. http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/noti... File notification.html (right): http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/noti... notification.html:30: <script src="ext/background.js"></script> On 2014/02/27 15:10:37, Thomas Greiner wrote: > On 2014/02/27 14:14:43, Felix H. Dahlke wrote: > > Guess I'm missing something, but what is this necessary for? > > This fixes notification.js which depends on ext.* No, background.js must be only executed once in the background page, otherwise things will break. Maybe you can use window.parent.ext?
http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/chro... File chrome/ext/background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/chro... chrome/ext/background.js:584: ext.browserNotifications = chrome.notifications; 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.
http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/chro... File chrome/ext/background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/chro... chrome/ext/background.js:584: ext.browserNotifications = chrome.notifications; 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.
http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/noti... File notification.html (right): http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/noti... notification.html:30: <script src="ext/background.js"></script> On 2014/02/28 09:56:03, Sebastian Noack wrote: > On 2014/02/27 15:10:37, Thomas Greiner wrote: > > On 2014/02/27 14:14:43, Felix H. Dahlke wrote: > > > Guess I'm missing something, but what is this necessary for? > > > > This fixes notification.js which depends on ext.* > > No, background.js must be only executed once in the background page, otherwise > things will break. Maybe you can use window.parent.ext? Not sure why the things will be broken, actually we are loading the HTML page only in webkitNotifications.createHTMLNotification method and it's open html page in desktop notification window. and in future I'm pretty sure that it will be removed while it's a deprecated method, also not sure how I can use window.parent.ext in this case - the Script that uses ext api is loaded in desktop notification window. I've tested this with the browser that support webkitNotifications.createHTMLNotification and seams nothing broken. Please let me know if I didn't get your point.
http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/chro... File chrome/ext/background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/chro... chrome/ext/background.js:584: ext.browserNotifications = chrome.notifications; 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.
On 2014/02/28 11:18:43, saroyanm wrote: > http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/chro... > File chrome/ext/background.js (right): > > http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/chro... > chrome/ext/background.js:584: ext.browserNotifications = chrome.notifications; > 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. Second reference link was duplication so here is the one: https://developer.apple.com/library/mac/documentation/NetworkingInternet/Conc...
http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/chro... File chrome/ext/background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/chro... chrome/ext/background.js:584: ext.browserNotifications = chrome.notifications; 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! http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/noti... File notification.html (right): http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/noti... notification.html:30: <script src="ext/background.js"></script> On 2014/02/28 10:56:12, saroyanm wrote: > On 2014/02/28 09:56:03, Sebastian Noack wrote: > > On 2014/02/27 15:10:37, Thomas Greiner wrote: > > > On 2014/02/27 14:14:43, Felix H. Dahlke wrote: > > > > Guess I'm missing something, but what is this necessary for? > > > > > > This fixes notification.js which depends on ext.* > > > > No, background.js must be only executed once in the background page, otherwise > > things will break. Maybe you can use window.parent.ext? > > Not sure why the things will be broken, actually we are loading the HTML page > only in webkitNotifications.createHTMLNotification method and it's open html > page in desktop notification window. This will run code twice, that is supposed to run only once, since ext/background.js is already loaded by the background page itself. That already makes things slower and using more memory than it would need. But when code that persists data runs twice, it will lead to serious issues. This is currently more a problem on Safari than on Chrome, but can also quickly lead to problems on Chrome. > also not sure how I can use > window.parent.ext in this case - the Script that uses ext api is loaded in > desktop notification window. The idea was that it might be possible to access the window that created the notification (the real background page in this case), via window.parent (like it works inside frames). So you could just put "var ext = window.parent.ext;" at the top of notification.js, and remove the includes for ext/common.js and ext/background.js from notification.html. However I don't know if that works. But if not, there might be an other way to pass an object to the page running inside the notification.
http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/chro... File chrome/ext/background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/chro... chrome/ext/background.js:584: ext.browserNotifications = chrome.notifications; 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 ? http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/noti... File notification.html (right): http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/noti... notification.html:30: <script src="ext/background.js"></script> On 2014/02/28 12:35:03, Sebastian Noack wrote: > On 2014/02/28 10:56:12, saroyanm wrote: > > On 2014/02/28 09:56:03, Sebastian Noack wrote: > > > On 2014/02/27 15:10:37, Thomas Greiner wrote: > > > > On 2014/02/27 14:14:43, Felix H. Dahlke wrote: > > > > > Guess I'm missing something, but what is this necessary for? > > > > > > > > This fixes notification.js which depends on ext.* > > > > > > No, background.js must be only executed once in the background page, > otherwise > > > things will break. Maybe you can use window.parent.ext? > > > > Not sure why the things will be broken, actually we are loading the HTML page > > only in webkitNotifications.createHTMLNotification method and it's open html > > page in desktop notification window. > > This will run code twice, that is supposed to run only once, since > ext/background.js is already loaded by the background page itself. That already > makes things slower and using more memory than it would need. But when code that > persists data runs twice, it will lead to serious issues. This is currently more > a problem on Safari than on Chrome, but can also quickly lead to problems on > Chrome. Yes I agree, but it will run twice for the users with old browser version, and in future I think we will get rid of the createHTMLNotification. And I'm pretty sure that this method will be deprecated for safari while it's deprecated from w3c Notificaition API. http://www.w3.org/TR/notifications/ > > also not sure how I can use > > window.parent.ext in this case - the Script that uses ext api is loaded in > > desktop notification window. > > The idea was that it might be possible to access the window that created the > notification (the real background page in this case), via window.parent (like it > works inside frames). So you could just put "var ext = window.parent.ext;" at > the top of notification.js, and remove the includes for ext/common.js and > ext/background.js from notification.html. However I don't know if that works. > But if not, there might be an other way to pass an object to the page running > inside the notification. > Unfortunately I've tried to test that, but didn't worked for me, so I think It's not possible, And seams like it's only accept one parameter and It's URL: http://www.chromium.org/developers/design-documents/desktop-notifications/api... I'm not sure if the desktop notification has connection with extension. Also no success by accessing background page using chrome.extension.getBackgroundPage and chrome.extension.getViews. So I don't see another way how we can access unfortunately.
http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/noti... File notification.html (right): http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/noti... notification.html:30: <script src="ext/background.js"></script> On 2014/02/28 13:47:04, saroyanm wrote: > On 2014/02/28 12:35:03, Sebastian Noack wrote: > > On 2014/02/28 10:56:12, saroyanm wrote: > > > On 2014/02/28 09:56:03, Sebastian Noack wrote: > > > > On 2014/02/27 15:10:37, Thomas Greiner wrote: > > > > > On 2014/02/27 14:14:43, Felix H. Dahlke wrote: > > > > > > Guess I'm missing something, but what is this necessary for? > > > > > > > > > > This fixes notification.js which depends on ext.* > > > > > > > > No, background.js must be only executed once in the background page, > > otherwise > > > > things will break. Maybe you can use window.parent.ext? > > > > > > Not sure why the things will be broken, actually we are loading the HTML > page > > > only in webkitNotifications.createHTMLNotification method and it's open html > > > page in desktop notification window. > > > > This will run code twice, that is supposed to run only once, since > > ext/background.js is already loaded by the background page itself. That > already > > makes things slower and using more memory than it would need. But when code > that > > persists data runs twice, it will lead to serious issues. This is currently > more > > a problem on Safari than on Chrome, but can also quickly lead to problems on > > Chrome. > > Yes I agree, but it will run twice for the users with old browser version, and > in future I think we will get rid of the createHTMLNotification. > And I'm pretty sure that this method will be deprecated for safari while it's > deprecated from w3c Notificaition API. > http://www.w3.org/TR/notifications/ > > > > also not sure how I can use > > > window.parent.ext in this case - the Script that uses ext api is loaded in > > > desktop notification window. > > > > The idea was that it might be possible to access the window that created the > > notification (the real background page in this case), via window.parent (like > it > > works inside frames). So you could just put "var ext = window.parent.ext;" at > > the top of notification.js, and remove the includes for ext/common.js and > > ext/background.js from notification.html. However I don't know if that works. > > But if not, there might be an other way to pass an object to the page running > > inside the notification. > > > > Unfortunately I've tried to test that, but didn't worked for me, so I think It's > not possible, > And seams like it's only accept one parameter and It's URL: > http://www.chromium.org/developers/design-documents/desktop-notifications/api... > I'm not sure if the desktop notification has connection with extension. > Also no success by accessing background page using > chrome.extension.getBackgroundPage and chrome.extension.getViews. > So I don't see another way how we can access unfortunately. I've just realized that webkitNotification.createHTMLNotification doesn't exist in the latest version of Chrome on Linux, Windows and Mac OS X, and also not in the latest version of Safari. So either I missed something or notification.html is actually never used?
http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/noti... File notification.html (right): http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/noti... notification.html:30: <script src="ext/background.js"></script> On 2014/02/28 14:23:49, Sebastian Noack wrote: > On 2014/02/28 13:47:04, saroyanm wrote: > > On 2014/02/28 12:35:03, Sebastian Noack wrote: > > > On 2014/02/28 10:56:12, saroyanm wrote: > > > > On 2014/02/28 09:56:03, Sebastian Noack wrote: > > > > > On 2014/02/27 15:10:37, Thomas Greiner wrote: > > > > > > On 2014/02/27 14:14:43, Felix H. Dahlke wrote: > > > > > > > Guess I'm missing something, but what is this necessary for? > > > > > > > > > > > > This fixes notification.js which depends on ext.* > > > > > > > > > > No, background.js must be only executed once in the background page, > > > otherwise > > > > > things will break. Maybe you can use window.parent.ext? > > > > > > > > Not sure why the things will be broken, actually we are loading the HTML > > page > > > > only in webkitNotifications.createHTMLNotification method and it's open > html > > > > page in desktop notification window. > > > > > > This will run code twice, that is supposed to run only once, since > > > ext/background.js is already loaded by the background page itself. That > > already > > > makes things slower and using more memory than it would need. But when code > > that > > > persists data runs twice, it will lead to serious issues. This is currently > > more > > > a problem on Safari than on Chrome, but can also quickly lead to problems on > > > Chrome. > > > > Yes I agree, but it will run twice for the users with old browser version, and > > in future I think we will get rid of the createHTMLNotification. > > And I'm pretty sure that this method will be deprecated for safari while it's > > deprecated from w3c Notificaition API. > > http://www.w3.org/TR/notifications/ > > > > > > also not sure how I can use > > > > window.parent.ext in this case - the Script that uses ext api is loaded in > > > > desktop notification window. > > > > > > The idea was that it might be possible to access the window that created the > > > notification (the real background page in this case), via window.parent > (like > > it > > > works inside frames). So you could just put "var ext = window.parent.ext;" > at > > > the top of notification.js, and remove the includes for ext/common.js and > > > ext/background.js from notification.html. However I don't know if that > works. > > > But if not, there might be an other way to pass an object to the page > running > > > inside the notification. > > > > > > > Unfortunately I've tried to test that, but didn't worked for me, so I think > It's > > not possible, > > And seams like it's only accept one parameter and It's URL: > > > http://www.chromium.org/developers/design-documents/desktop-notifications/api... > > I'm not sure if the desktop notification has connection with extension. > > Also no success by accessing background page using > > chrome.extension.getBackgroundPage and chrome.extension.getViews. > > So I don't see another way how we can access unfortunately. > > I've just realized that webkitNotification.createHTMLNotification doesn't exist > in the latest version of Chrome on Linux, Windows and Mac OS X, and also not in > the latest version of Safari. So either I missed something or notification.html > is actually never used? As I've mentioned in my previous comment It's deprecated Method but it's still used for old versions of browsers, I'm not sure from which version exactly it was removed but I'v tested it with version 26 Chrome And it's still there I can not get the newer version of chrome because of the Chrome policy that standalone versions are stopped to appear in sites with trusted source, so I can't tell from which exactly version it was removed. So you are right, for most of people It's not available.
http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/chro... File chrome/ext/background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/chro... chrome/ext/background.js:584: ext.browserNotifications = chrome.notifications; 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.
http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/chro... File chrome/ext/background.js (right): http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/chro... chrome/ext/background.js:584: ext.browserNotifications = chrome.notifications; 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. http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/noti... File notification.html (right): http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/noti... notification.html:30: <script src="ext/background.js"></script> On 2014/02/28 14:34:25, saroyanm wrote: > On 2014/02/28 14:23:49, Sebastian Noack wrote: > > On 2014/02/28 13:47:04, saroyanm wrote: > > > On 2014/02/28 12:35:03, Sebastian Noack wrote: > > > > On 2014/02/28 10:56:12, saroyanm wrote: > > > > > On 2014/02/28 09:56:03, Sebastian Noack wrote: > > > > > > On 2014/02/27 15:10:37, Thomas Greiner wrote: > > > > > > > On 2014/02/27 14:14:43, Felix H. Dahlke wrote: > > > > > > > > Guess I'm missing something, but what is this necessary for? > > > > > > > > > > > > > > This fixes notification.js which depends on ext.* > > > > > > > > > > > > No, background.js must be only executed once in the background page, > > > > otherwise > > > > > > things will break. Maybe you can use window.parent.ext? > > > > > > > > > > Not sure why the things will be broken, actually we are loading the HTML > > > page > > > > > only in webkitNotifications.createHTMLNotification method and it's open > > html > > > > > page in desktop notification window. > > > > > > > > This will run code twice, that is supposed to run only once, since > > > > ext/background.js is already loaded by the background page itself. That > > > already > > > > makes things slower and using more memory than it would need. But when > code > > > that > > > > persists data runs twice, it will lead to serious issues. This is > currently > > > more > > > > a problem on Safari than on Chrome, but can also quickly lead to problems > on > > > > Chrome. > > > > > > Yes I agree, but it will run twice for the users with old browser version, > and > > > in future I think we will get rid of the createHTMLNotification. > > > And I'm pretty sure that this method will be deprecated for safari while > it's > > > deprecated from w3c Notificaition API. > > > http://www.w3.org/TR/notifications/ > > > > > > > > also not sure how I can use > > > > > window.parent.ext in this case - the Script that uses ext api is loaded > in > > > > > desktop notification window. > > > > > > > > The idea was that it might be possible to access the window that created > the > > > > notification (the real background page in this case), via window.parent > > (like > > > it > > > > works inside frames). So you could just put "var ext = window.parent.ext;" > > at > > > > the top of notification.js, and remove the includes for ext/common.js and > > > > ext/background.js from notification.html. However I don't know if that > > works. > > > > But if not, there might be an other way to pass an object to the page > > running > > > > inside the notification. > > > > > > > > > > Unfortunately I've tried to test that, but didn't worked for me, so I think > > It's > > > not possible, > > > And seams like it's only accept one parameter and It's URL: > > > > > > http://www.chromium.org/developers/design-documents/desktop-notifications/api... > > > I'm not sure if the desktop notification has connection with extension. > > > Also no success by accessing background page using > > > chrome.extension.getBackgroundPage and chrome.extension.getViews. > > > So I don't see another way how we can access unfortunately. > > > > I've just realized that webkitNotification.createHTMLNotification doesn't > exist > > in the latest version of Chrome on Linux, Windows and Mac OS X, and also not > in > > the latest version of Safari. So either I missed something or > notification.html > > is actually never used? > > As I've mentioned in my previous comment It's deprecated Method but it's still > used for old versions of browsers, I'm not sure from which version exactly it > was removed but I'v tested it with version 26 Chrome And it's still there I can > not get the newer version of chrome because of the Chrome policy that standalone > versions are stopped to appear in sites with trusted source, so I can't tell > from which exactly version it was removed. > So you are right, for most of people It's not available. I'm very much in the favor of getting rid of that code, now. It will make problems rather sooner than later. And I don't think that supporting a minor feature on an outdated browser if worse maintaining that code.
http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/noti... File notification.html (right): http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/noti... notification.html:30: <script src="ext/background.js"></script> On 2014/02/28 16:59:22, Sebastian Noack wrote: > On 2014/02/28 14:34:25, saroyanm wrote: > > On 2014/02/28 14:23:49, Sebastian Noack wrote: > > > On 2014/02/28 13:47:04, saroyanm wrote: > > > > On 2014/02/28 12:35:03, Sebastian Noack wrote: > > > > > On 2014/02/28 10:56:12, saroyanm wrote: > > > > > > On 2014/02/28 09:56:03, Sebastian Noack wrote: > > > > > > > On 2014/02/27 15:10:37, Thomas Greiner wrote: > > > > > > > > On 2014/02/27 14:14:43, Felix H. Dahlke wrote: > > > > > > > > > Guess I'm missing something, but what is this necessary for? > > > > > > > > > > > > > > > > This fixes notification.js which depends on ext.* > > > > > > > > > > > > > > No, background.js must be only executed once in the background page, > > > > > otherwise > > > > > > > things will break. Maybe you can use window.parent.ext? > > > > > > > > > > > > Not sure why the things will be broken, actually we are loading the > HTML > > > > page > > > > > > only in webkitNotifications.createHTMLNotification method and it's > open > > > html > > > > > > page in desktop notification window. > > > > > > > > > > This will run code twice, that is supposed to run only once, since > > > > > ext/background.js is already loaded by the background page itself. That > > > > already > > > > > makes things slower and using more memory than it would need. But when > > code > > > > that > > > > > persists data runs twice, it will lead to serious issues. This is > > currently > > > > more > > > > > a problem on Safari than on Chrome, but can also quickly lead to > problems > > on > > > > > Chrome. > > > > > > > > Yes I agree, but it will run twice for the users with old browser version, > > and > > > > in future I think we will get rid of the createHTMLNotification. > > > > And I'm pretty sure that this method will be deprecated for safari while > > it's > > > > deprecated from w3c Notificaition API. > > > > http://www.w3.org/TR/notifications/ > > > > > > > > > > also not sure how I can use > > > > > > window.parent.ext in this case - the Script that uses ext api is > loaded > > in > > > > > > desktop notification window. > > > > > > > > > > The idea was that it might be possible to access the window that created > > the > > > > > notification (the real background page in this case), via window.parent > > > (like > > > > it > > > > > works inside frames). So you could just put "var ext = > window.parent.ext;" > > > at > > > > > the top of notification.js, and remove the includes for ext/common.js > and > > > > > ext/background.js from notification.html. However I don't know if that > > > works. > > > > > But if not, there might be an other way to pass an object to the page > > > running > > > > > inside the notification. > > > > > > > > > > > > > Unfortunately I've tried to test that, but didn't worked for me, so I > think > > > It's > > > > not possible, > > > > And seams like it's only accept one parameter and It's URL: > > > > > > > > > > http://www.chromium.org/developers/design-documents/desktop-notifications/api... > > > > I'm not sure if the desktop notification has connection with extension. > > > > Also no success by accessing background page using > > > > chrome.extension.getBackgroundPage and chrome.extension.getViews. > > > > So I don't see another way how we can access unfortunately. > > > > > > I've just realized that webkitNotification.createHTMLNotification doesn't > > exist > > > in the latest version of Chrome on Linux, Windows and Mac OS X, and also not > > in > > > the latest version of Safari. So either I missed something or > > notification.html > > > is actually never used? > > > > As I've mentioned in my previous comment It's deprecated Method but it's still > > used for old versions of browsers, I'm not sure from which version exactly it > > was removed but I'v tested it with version 26 Chrome And it's still there I > can > > not get the newer version of chrome because of the Chrome policy that > standalone > > versions are stopped to appear in sites with trusted source, so I can't tell > > from which exactly version it was removed. > > So you are right, for most of people It's not available. > > I'm very much in the favor of getting rid of that code, now. It will make > problems rather sooner than later. And I don't think that supporting a minor > feature on an outdated browser if worse maintaining that code. s/if worse/is worth/
@Thomas can you please reply to my last comment, Thanks in advance. http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/noti... File notification.html (right): http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/noti... notification.html:30: <script src="ext/background.js"></script> On 2014/02/28 17:01:00, Sebastian Noack wrote: > On 2014/02/28 16:59:22, Sebastian Noack wrote: > > On 2014/02/28 14:34:25, saroyanm wrote: > > > On 2014/02/28 14:23:49, Sebastian Noack wrote: > > > > On 2014/02/28 13:47:04, saroyanm wrote: > > > > > On 2014/02/28 12:35:03, Sebastian Noack wrote: > > > > > > On 2014/02/28 10:56:12, saroyanm wrote: > > > > > > > On 2014/02/28 09:56:03, Sebastian Noack wrote: > > > > > > > > On 2014/02/27 15:10:37, Thomas Greiner wrote: > > > > > > > > > On 2014/02/27 14:14:43, Felix H. Dahlke wrote: > > > > > > > > > > Guess I'm missing something, but what is this necessary for? > > > > > > > > > > > > > > > > > > This fixes notification.js which depends on ext.* > > > > > > > > > > > > > > > > No, background.js must be only executed once in the background > page, > > > > > > otherwise > > > > > > > > things will break. Maybe you can use window.parent.ext? > > > > > > > > > > > > > > Not sure why the things will be broken, actually we are loading the > > HTML > > > > > page > > > > > > > only in webkitNotifications.createHTMLNotification method and it's > > open > > > > html > > > > > > > page in desktop notification window. > > > > > > > > > > > > This will run code twice, that is supposed to run only once, since > > > > > > ext/background.js is already loaded by the background page itself. > That > > > > > already > > > > > > makes things slower and using more memory than it would need. But when > > > code > > > > > that > > > > > > persists data runs twice, it will lead to serious issues. This is > > > currently > > > > > more > > > > > > a problem on Safari than on Chrome, but can also quickly lead to > > problems > > > on > > > > > > Chrome. > > > > > > > > > > Yes I agree, but it will run twice for the users with old browser > version, > > > and > > > > > in future I think we will get rid of the createHTMLNotification. > > > > > And I'm pretty sure that this method will be deprecated for safari while > > > it's > > > > > deprecated from w3c Notificaition API. > > > > > http://www.w3.org/TR/notifications/ > > > > > > > > > > > > also not sure how I can use > > > > > > > window.parent.ext in this case - the Script that uses ext api is > > loaded > > > in > > > > > > > desktop notification window. > > > > > > > > > > > > The idea was that it might be possible to access the window that > created > > > the > > > > > > notification (the real background page in this case), via > window.parent > > > > (like > > > > > it > > > > > > works inside frames). So you could just put "var ext = > > window.parent.ext;" > > > > at > > > > > > the top of notification.js, and remove the includes for ext/common.js > > and > > > > > > ext/background.js from notification.html. However I don't know if that > > > > works. > > > > > > But if not, there might be an other way to pass an object to the page > > > > running > > > > > > inside the notification. > > > > > > > > > > > > > > > > Unfortunately I've tried to test that, but didn't worked for me, so I > > think > > > > It's > > > > > not possible, > > > > > And seams like it's only accept one parameter and It's URL: > > > > > > > > > > > > > > > http://www.chromium.org/developers/design-documents/desktop-notifications/api... > > > > > I'm not sure if the desktop notification has connection with extension. > > > > > Also no success by accessing background page using > > > > > chrome.extension.getBackgroundPage and chrome.extension.getViews. > > > > > So I don't see another way how we can access unfortunately. > > > > > > > > I've just realized that webkitNotification.createHTMLNotification doesn't > > > exist > > > > in the latest version of Chrome on Linux, Windows and Mac OS X, and also > not > > > in > > > > the latest version of Safari. So either I missed something or > > > notification.html > > > > is actually never used? > > > > > > As I've mentioned in my previous comment It's deprecated Method but it's > still > > > used for old versions of browsers, I'm not sure from which version exactly > it > > > was removed but I'v tested it with version 26 Chrome And it's still there I > > can > > > not get the newer version of chrome because of the Chrome policy that > > standalone > > > versions are stopped to appear in sites with trusted source, so I can't tell > > > from which exactly version it was removed. > > > So you are right, for most of people It's not available. > > > > I'm very much in the favor of getting rid of that code, now. It will make > > problems rather sooner than later. And I don't think that supporting a minor > > feature on an outdated browser if worse maintaining that code. > > s/if worse/is worth/ Sebastian I see your point and yes while It's not available for newer version of chrome and safari, but there is a discussion where me and Thomas agreed to use createHTMLNotification to support people with old browser version: http://codereview.adblockplus.org/6098518317989888/#msg1 So would also like to know what Thomas think about this. @Thomas what you think about removing case for createHTMLNotification support for old browsers, while as far as I understood from Sebastian notes that this will be a problem for safari (loading scripts in notification.html) and it worth to use createNotification for old browser versions as well and maybe remove notification.html file from revision control while we will no more use it ?
http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/noti... File notification.html (right): http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/noti... notification.html:30: <script src="ext/background.js"></script> On 2014/03/03 07:08:51, saroyanm wrote: > On 2014/02/28 17:01:00, Sebastian Noack wrote: > > On 2014/02/28 16:59:22, Sebastian Noack wrote: > > > On 2014/02/28 14:34:25, saroyanm wrote: > > > > On 2014/02/28 14:23:49, Sebastian Noack wrote: > > > > > On 2014/02/28 13:47:04, saroyanm wrote: > > > > > > On 2014/02/28 12:35:03, Sebastian Noack wrote: > > > > > > > On 2014/02/28 10:56:12, saroyanm wrote: > > > > > > > > On 2014/02/28 09:56:03, Sebastian Noack wrote: > > > > > > > > > On 2014/02/27 15:10:37, Thomas Greiner wrote: > > > > > > > > > > On 2014/02/27 14:14:43, Felix H. Dahlke wrote: > > > > > > > > > > > Guess I'm missing something, but what is this necessary for? > > > > > > > > > > > > > > > > > > > > This fixes notification.js which depends on ext.* > > > > > > > > > > > > > > > > > > No, background.js must be only executed once in the background > > page, > > > > > > > otherwise > > > > > > > > > things will break. Maybe you can use window.parent.ext? > > > > > > > > > > > > > > > > Not sure why the things will be broken, actually we are loading > the > > > HTML > > > > > > page > > > > > > > > only in webkitNotifications.createHTMLNotification method and it's > > > open > > > > > html > > > > > > > > page in desktop notification window. > > > > > > > > > > > > > > This will run code twice, that is supposed to run only once, since > > > > > > > ext/background.js is already loaded by the background page itself. > > That > > > > > > already > > > > > > > makes things slower and using more memory than it would need. But > when > > > > code > > > > > > that > > > > > > > persists data runs twice, it will lead to serious issues. This is > > > > currently > > > > > > more > > > > > > > a problem on Safari than on Chrome, but can also quickly lead to > > > problems > > > > on > > > > > > > Chrome. > > > > > > > > > > > > Yes I agree, but it will run twice for the users with old browser > > version, > > > > and > > > > > > in future I think we will get rid of the createHTMLNotification. > > > > > > And I'm pretty sure that this method will be deprecated for safari > while > > > > it's > > > > > > deprecated from w3c Notificaition API. > > > > > > http://www.w3.org/TR/notifications/ > > > > > > > > > > > > > > also not sure how I can use > > > > > > > > window.parent.ext in this case - the Script that uses ext api is > > > loaded > > > > in > > > > > > > > desktop notification window. > > > > > > > > > > > > > > The idea was that it might be possible to access the window that > > created > > > > the > > > > > > > notification (the real background page in this case), via > > window.parent > > > > > (like > > > > > > it > > > > > > > works inside frames). So you could just put "var ext = > > > window.parent.ext;" > > > > > at > > > > > > > the top of notification.js, and remove the includes for > ext/common.js > > > and > > > > > > > ext/background.js from notification.html. However I don't know if > that > > > > > works. > > > > > > > But if not, there might be an other way to pass an object to the > page > > > > > running > > > > > > > inside the notification. > > > > > > > > > > > > > > > > > > > Unfortunately I've tried to test that, but didn't worked for me, so I > > > think > > > > > It's > > > > > > not possible, > > > > > > And seams like it's only accept one parameter and It's URL: > > > > > > > > > > > > > > > > > > > > > http://www.chromium.org/developers/design-documents/desktop-notifications/api... > > > > > > I'm not sure if the desktop notification has connection with > extension. > > > > > > Also no success by accessing background page using > > > > > > chrome.extension.getBackgroundPage and chrome.extension.getViews. > > > > > > So I don't see another way how we can access unfortunately. > > > > > > > > > > I've just realized that webkitNotification.createHTMLNotification > doesn't > > > > exist > > > > > in the latest version of Chrome on Linux, Windows and Mac OS X, and also > > not > > > > in > > > > > the latest version of Safari. So either I missed something or > > > > notification.html > > > > > is actually never used? > > > > > > > > As I've mentioned in my previous comment It's deprecated Method but it's > > still > > > > used for old versions of browsers, I'm not sure from which version exactly > > it > > > > was removed but I'v tested it with version 26 Chrome And it's still there > I > > > can > > > > not get the newer version of chrome because of the Chrome policy that > > > standalone > > > > versions are stopped to appear in sites with trusted source, so I can't > tell > > > > from which exactly version it was removed. > > > > So you are right, for most of people It's not available. > > > > > > I'm very much in the favor of getting rid of that code, now. It will make > > > problems rather sooner than later. And I don't think that supporting a minor > > > feature on an outdated browser if worse maintaining that code. > > > > s/if worse/is worth/ > > Sebastian I see your point and yes while It's not available for newer version of > chrome and safari, but there is a discussion where me and Thomas agreed to use > createHTMLNotification to support people with old browser version: > http://codereview.adblockplus.org/6098518317989888/#msg1 > > So would also like to know what Thomas think about this. > @Thomas what you think about removing case for createHTMLNotification support > for old browsers, while as far as I understood from Sebastian notes that this > will be a problem for safari (loading scripts in notification.html) and it worth > to use createNotification for old browser versions as well and maybe remove > notification.html file from revision control while we will no more use it ? We should not remove the case for createHTMLNotification because I think we should always aim for the best user experience even if not a lot of users will see it anymore. If it turns out to be impossible to implement in Safari (which I doubt since that would've come up when we ported it over from Chrome) we could think of making this case Chrome-only.
http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/noti... File notification.html (right): http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/noti... notification.html:30: <script src="ext/background.js"></script> On 2014/03/03 07:08:51, saroyanm wrote: > Sebastian I see your point and yes while It's not available for newer version of > chrome and safari createHTMLNotification isn't available for any version of Safari. > while as far as I understood from Sebastian notes that this > will be a problem for safari It turned out by now, that Safari isn't actually affected, because the absence of createHTMLNotification. However as explained above, including ext/background.js a second time, is also problematic on Chrome. > to use createNotification for old browser versions as well Isn't createNotification useless for this case, because it doesn't support UI elements like buttons? On 2014/03/04 09:28:15, Thomas Greiner wrote: > we could think of making this case Chrome-only. Since notification.html is only used if createHTMLNotification exist, this is already Chrome-only. However it might be a good idea to don't even include notification.html and notification.js in the Safari build. Feel free to do that.
http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/noti... File notification.html (right): http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/noti... notification.html:30: <script src="ext/background.js"></script> > > to use createNotification for old browser versions as well > Isn't createNotification useless for this case, because it doesn't support UI > elements like buttons? It's still better than window.confirm and it can have an onclick listener. However, for the new question-type that I'm working on, createNotification will not be used. > Since notification.html is only used if createHTMLNotification exist, this is > already Chrome-only. However it might be a good idea to don't even include > notification.html and notification.js in the Safari build. Feel free to do that. I agree. As you said on IRC, moving the notification files into ext/ and adding it to metadata.chrome (possibly also metadata.opera if createHTMLNotification has been available in any of the Opera versions) should do the job.
http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/noti... File notification.html (right): http://codereview.adblockplus.org/6098518317989888/diff/5651874166341632/noti... notification.html:30: <script src="ext/background.js"></script> > > to use createNotification for old browser versions as well > Isn't createNotification useless for this case, because it doesn't support UI > elements like buttons? chrome.notifications Are stable since chrome v28 so we are not sure from which exact chrome version webkitNotifications.createHTMLNotification removed from chrome, so we are using webkitNotifications.createNotification to be sure that the notification will be shown in case both previous are not available. > > we could think of making this case Chrome-only. > Since notification.html is only used if createHTMLNotification exist, this is > already Chrome-only. However it might be a good idea to don't even include > notification.html and notification.js in the Safari build. Feel free to do that. So if it's not included in any version of safari that make sense. Thanks for pointing that.
Wow, 129 comments, and I'm not really sure what the bottom line of this is now. I think we should let this review rest in piece now :) If there's something left to discuss, let's move it to the intraforum, summing up the discussion here so far. If there's agreement on follow-up tasks, please create Trello cards for those.
On 2014/03/05 21:09:20, Felix H. Dahlke wrote: > Wow, 129 comments, and I'm not really sure what the bottom line of this is now. > I think we should let this review rest in piece now :) > > If there's something left to discuss, let's move it to the intraforum, summing > up the discussion here so far. > > If there's agreement on follow-up tasks, please create Trello cards for those. The discussion continues here: http://codereview.adblockplus.org/5767063142400000/
Filed https://issues.adblockplus.org/ticket/172 as follow-up to this. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
