search

ChessCom / browser-extension

Customize your Chess.com experience
Other
48 stars 15 forks source link

Change background page to event page? #79

Open emilgoldsmith opened 7 years ago

emilgoldsmith commented 7 years ago

From Event Pages Documentation:

A common need for apps and extensions is to have a single long-running script to manage some task or state. Event pages to the rescue. Event pages are loaded only when they are needed. When the event page is not actively doing something, it is unloaded, freeing memory and other system resources.

Event pages are available in the stable channel as of Chrome 22, and the performance advantages are significant, especially on low-power devices. Please prefer them to persistent background pages whenever possible for new development and begin migrating existing background pages to this new model.

Could we do this? It looks like it to me. Migration guide is here.

From a quick maybe very bad search it didn't seem like Firefox supported it, but if I'm not wrong the only thing we'd need to change is the manifest, and that makes it super simple to keep them apart as they already have different manifests.

I see we promisify everything which I'm not quite sure exactly what means in practice, and why we're doing it. But will we need to do point 3 in the migration guide because of that?

martynchamberlin commented 7 years ago

I see we promisify everything which I'm not quite sure exactly what means in practice, and why we're doing it. But will we need to do point 3 in the migration guide because of that?

Not sure what you mean here. Give an example? The fetching of localStorage has to be callback based, AFAIK. Other than that I'm pretty sure we're doing most everything sync.

We are using a timeout or interval for the 60 second polling of notifications. So point 2 of the migration would be relevant here:

If your extension uses window.setTimeout() or window.setInterval(), switch to using the alarms API instead. DOM-based timers won't be honored if the event page shuts down.

emilgoldsmith commented 7 years ago

Not sure what you mean here. Give an example?

What I'm talking about is this.

That highlighted bit of code confuses me a lot, but I guess it's also something you didn't work on? I understand the nature of promises just fine, but I'm not sure I 100% understand what it means to "promisify" all those functions, and I definitely don't understand why we're doing it or whether it would impact a possible migration of the sort we're discussing.

We are using a timeout or interval for the 60 second polling of notifications.

Oh yeah I remember seeing that, but that shouldn't be a problem to migrate.

martynchamberlin commented 7 years ago

Wow I have no idea what all that promisifying is intended to do either. That's the weirdest thing.

emilgoldsmith commented 7 years ago

Yeah haha, well I just merged in #83 and that removed all that stuff, so we don't have to worry about that anymore! Which I guess means we can probably attempt this migration at some point (y).

emilgoldsmith commented 7 years ago

From Mozilla background documentation: screenshot from 2017-03-18 09-20-35

So we have to be aware of that. I'm thinking we should definitely add some kind of environment variable that tells us which browser we're currently in so we can make variable design decisions for compatibility reasons within the code as I suggested in #52 even though it seems Firefox now supports it so it isn't relevant to exactly that issue right now. I'll open an issue for this.