search

aiannacc / Goko-Salvager

Enhance your Dominion Online experience!
13 stars 9 forks source link

Notifications need revision on Chrome #234

Closed aiannacc closed 10 years ago

aiannacc commented 10 years ago

New versions (35+?) of Chrome drop support for the deprecated webkitNotifications we've been using. It's not clear to me what we're meant to use instead.

https://developer.chrome.com/apps/notifications may be new way to do notifications in Chrome, but chrome.notifications isn't available in the JS console by default. Maybe it needs to be enabled in the manifest first?

theblankman commented 10 years ago

Looks like notification permissions do have to be in the manifest, but you also can't call the chrome.notifications API straight from a content script, you need a background page or event page. See http://stackoverflow.com/questions/20317388/chrome-extension-rich-notifications-not-working and https://developer.chrome.com/extensions/background_pages. Seems there are some hoops to jump through in order to create and use one. Working on it...

theblankman commented 10 years ago

Got notifications sort of working through a background page, one more catch to figure out. Is there a simple way to get the chrome extension ID from content_script notifications.js? (eg 'igndodpmmohnhdihdkidoiphfdjgjdcd')

stackoverflow tells me to use chrome.runtime.id but that just returns an empty string, and doesn't work to make my little test notification show up. But if I hardcode the ID of my Salvager build, then I do get a notification.

aiannacc commented 10 years ago

Sorry to step on your toes, but I wanted to get this fixed ASAP, since it's currently making Salvager unusable for all our Chrome users. That's the large majority of our users and the vast majority of our non-savvy users (i.e. those who will simply stop using Salvager rather than posting here or in the forum).

Pull request #236 restores the previous behavior on all our supported browsers (Chrome, Chromium, Firefox, Safari). The Chrome "Rich Notifications" that it looks like you're working on would be cooler still.

theblankman commented 10 years ago

No toe-stepping taken, this needed a quick fix. But now I'm confused... if all three major browsers support one API through differently named objects, why did Salvager ever do anything different from what's in #236? It seems like browser-specific bells and whistles like Chrome's rich notification are way more trouble than they're worth. If #236 works, I think we might as well close this and I'll go work on something else.

aiannacc commented 10 years ago

A lot of the HTML5 stuff is still considered "draft." The APIs and other specs still won't be finalized for a while.

When I added notifications, Safari still didn't support them at all, and Chrome had a slightly quirky implementation. So I had to write some browser-specific code. Chrome deprecated their original implementation a while back, and then finally removed it entirely in Chrome v35, which is why we suddenly have a problem.

But now they all adhere to the HTML5 Notification draft API, so the only "browser specific" code that's needed now is the line that deals with each of them keeping the code in slightly different places:

var Notification = window.Notification || window.mozNotification || window.webkitNotification