Rothera / bpm

BetterPonymotes
GNU Affero General Public License v3.0
22 stars 23 forks source link

WebExtension Port 2: Port Harder #49

Closed bb010g closed 6 years ago

bb010g commented 7 years ago

You can consider this a more intensive version of #48 (because I just found out about that PR ~30 minutes ago, after finishing the bulk of this one). As opposed to #48, which keeps Firefox XPI, Firefox WebExt, and Chrome WebExt separate, this just uses a unified zip and manifest because Chrome and Firefox just ignore manifest properties they don't understand. There's no real benefit to keeping the XPI version around anymore, and it reduces the tooling required.

This unification also allows for removal of dead XPI-specific code. I did drop the mozsucks version suffix as a part of this, but hopefully you don't feel quite that way anymore thanks to not having to deal with XPI anymore. This PR was tested in Firefox stable 55.0.3, Firefox Nightly 58.0a1 (2017-09-26), and Chrome stable 61.0.3163.100 on Arch Linux, with the same tests as #48. Verification on more browser versions would be appreciated. EDIT: Tested less on Firefox ESR 52.4.1 on Arch, but it seems fine.

This PR also rips out GIF Animotes entirely (because I was already in the Makefile). If you need them still for some purpose, I'd be more than happy to rebase them back in.

In terms of future plans, this does pave the way towards a change-over to browser and the WebExt Promise APIs (with Chrome supported through a polyfill), which would make an Edge port comfy (without the globs of code #40 adds).

bb010g commented 7 years ago

Here's an unsigned version for easier testing: webext.zip

Shugabuga commented 7 years ago

this just uses a unified zip and manifest because Chrome and Firefox just ignore manifest properties they don't understand.

I thought browsers will complain if something unknown is in the manifest, or did they change this?

This PR also rips out GIF Animotes entirely (because I was already in the Makefile). If you need them still for some purpose, I'd be more than happy to rebase them back in.

I think we are done with animotes, so nice job of noticing + removing it

which would make an Edge port comfy

Typhos mentioned that the reason #40 wasn't merged is because nobody could maintain it. If you don't mind maintaining it, you can help with an Edge port.

Nice to see a new face working on the project too. Welcome to the project! 🍾

bb010g commented 7 years ago

I thought browsers will complain if something unknown is in the manifest, or did they change this?

I mean, Chrome will complain a bit about unknown properties if you check the debug box on the extensions page, but it's nothing that'll bother the user.

Typhos mentioned that the reason #40 wasn't merged is because nobody could maintain it. If you don't mind maintaining it, you can help with an Edge port.

Sounds good. Hopefully we don't touch too many APIs that Edge doesn't have yet. I haven't read through #40 yet, so I'm not sure what he had to do polyfill/workaround-wise.

Nice to see a new face working on the project too. Welcome to the project! 🍾

Glad to help! :tada:

Rothera commented 7 years ago

Preliminary feedback: Unifying Chrome and WebExt is useful, but I am not currently planning on dropping XPI support, I still hate Mozilla, and I rather ironically just reinstated the GIF tools for Discord's benefit.

bb010g commented 7 years ago

How far back do you want to support for Firefox?

ByzantineFailure commented 7 years ago

Echoing @Rothera, please keep the gif-animote.css stuff in the Makefile, it's useful over in Discord land (at least until Electron starts to support apng, then I think we'll all throw a party and be done with it).

bb010g commented 7 years ago

I was planning on waiting for word on XPI/FX backwards compatibility plans, but guess I can take care of that bit now. As for Electron updating, haven't been able to figure out their plans yet. https://github.com/electron/libchromiumcontent/pull/313 is merged, though.

Rothera commented 7 years ago

Firefox support is not so much any particular version as simply continuing to work as-is.

bb010g commented 7 years ago

Right now, this branch would require you to go in and reinitialize your preferences once after the update, and then you'd be done. I don't think this is a blocking break, but if you'd like to have this change be completely invisible, here's what Firefox provides:

We currently use the simple-storage API to store preferences using the SDK. Embedded WebExtensions would allow for putting the WebExtension inside of a XPI shell that would also be able to access the SDK and migrate preferences, as long as they update to the Embedded version before ~November 12th, which is a few days before 57 drops (to be safe). Note that this would not allow anyone on the beta to use BPM without overriding the non-WebExtension block (which Nightly users can no longer do).

If you really want, I (or someone other than me) can look into setting up an embedded shim to publish. I think we'd just want to merge that along with this PR to make sure it gets done. We only have 40 days before the current XPI extension just stops working.

Rothera commented 7 years ago

I am not terribly concerned about breaking compatibility once. That is unfortunate, but Mozilla shoulders that blame.

I do not know to what extent users commonly edit BPM settings, but to be honest, there really aren't that many of them. A transparent upgrade would be ideal, but is likely not necessary. If there were a simple way to notify users that their settings have been reset that might be more reasonable (or more generally, a bit of attention grabbing on all fresh installs), but I think most people who care about these things will notice any changes themselves.

Side note, the options page has been neglected. And might stand some simplification. I have a hunch that, after so long, many of the users who drove its complexity are likely no longer with us anyway.

bb010g commented 7 years ago

Having an options page that could just be embedded into the options page would be nice. And I don't think Mozilla did too much wrong in this process; we've had the ability to start at least copying settings over to a WebExt friendly place for months. Anything else in this catching your eye, or does it look good? If you think it's good, I'm willing to go try and set up some wider test environments before merge.

Rothera commented 7 years ago

I have not closely (line by line) examined most of the PR, but I am solidly opposed to dropping the current Firefox build. I am consequently leaning toward the more minimalist #48 for meeting current needs as succinctly as possible.

bb010g commented 7 years ago

What do you want to do in the next 39 days with XPI, then? If we're not doing an Embedded WebExtension to transition, there's no real point to keeping XPI builds around. I'm not sure what you're seeing in keeping them. Also, #48 just duplicates an entire zip build for no real purpose.

Rothera commented 7 years ago

I use it.

Duplicating zip builds is not particularly concerning. The fact that the extension is perfectly compatible across two browsers is an anomaly. And they get uploaded to different places anyway.

bb010g commented 7 years ago

Them being compatible isn't an anomaly, it's the entire point of the WebExtension standard. It's why you can also "port" to Edge so easily. Here's the W3C draft. It uses Chrome currently because I didn't want the extra churn of introducing a full WebExt polyfill at this point, but all the polyfill really does is point browser.whatever to chrome.whatever and use Promises instead of callbacks. WebExtensions is a standard, so we should generate one zip for that standard, in the same way that Opera just uses the Chrome zip right now.

What version of Firefox are you running right now? Stable? The WebExtensions version would work exactly the same as your current XPI that you're using (after the config fix, which you'll have to tackle sometime). I can go do a full comparison test if you'd like.

Rothera commented 7 years ago

It is very much a historical anomaly. It will be compatible until Firefox adds something custom, which they're doing already even if we don't care, or anyone at all does something weird or requires slightly different packaging or anything, which seems rather altogether likely given the context (it may already be true- see keyfile, not sure). None of the involved entities are fundamentally trustworthy and distasteful standards by them hold very little weight in my eyes.

Wrt other browsers: Opera support is through an official extension to download things directly from the Chrome Web Store, and otherwise works presumably because it's essentially Chrome. Edge is its own engine and the attempt in #40 and the current official documentation both require copious amounts of glue code because it isn't actually the same API. Skimming, much of it looks like a fairly thin proxy, but given the timelines that is likely due to Edge having been intentionally built to look sufficiently like Chrome, not any standard.

The code is largely compatible due to intentional efforts, and that is nice, but at this point I attribute any more than that to convenient coincidence. I still think of the browser as the fundamental unit of support.

And to some extent I honestly just don't care. I have nothing but bitter spite for browser vendors. For now I remain on the last ESR (45) before Mozilla went completely off the deep end into the ninth circle. I have no idea if WebExt even runs in this version and precious little desire to find out. This is also probably required if I want to pretend I support the various forks, though I could not say who is left of those users either.

bb010g commented 6 years ago

https://github.com/electron/electron/issues/9679 just got closed with the release of the 1.8.1 beta, so that's good for removing GIF animotes soon.

Spazturtle commented 6 years ago

No it will always be compatible, as long as the addons doesn't use any browser specific APIs it will work in all browsers that support the new WebExt system. It is much like HTML in this sense, there are the standard features and then browser specific ones, as long as a site just uses reference HTML it works in all browsers.

Anyway the non-standard APIs that FIrefox has are used to do things like change about:config flags or disable the tab bar, things that are browsers specific anyway.

Both Microsoft and Mozilla are collaborating on the WebExt API, so there will be no chance of a new API doing different things in different browsers.

bb010g commented 6 years ago

Yeah. I'll see if I can get this cleaned up some more tonight and tested on the ESR. Mozilla's being doing a lot better recently, with 57 and up being the best browsers I've ever used. (I'm currently running the 59 Nightly as a daily driver, to give you an idea of how much it's improved, restoring a session of ~800 tabs took 5 seconds to be usable, and closing took ~3 seconds for the process to die, as opposed to ~30-45 seconds on older versions.) All I can say is that you really should go out on a limb and try trusting Mozilla to not change their API to be incompatible in the future. For further assurance, pretty much any page on MDN for WebExtensions will have a compatibility table, and MDN has recently been chosen by Microsoft, Google, Samsung, and the W3C to be the main place on the web for accurate, usable documentation. Mozilla's been taking a lot of good steps lately.

bb010g commented 6 years ago

Ok. Installed the ESR and tried out the WebExtension, and it seems to work fine: 1508562311

bb010g commented 6 years ago

Reminder that Firefox 57 is getting merged in 2 weeks: https://wiki.mozilla.org/RapidRelease/Calendar

bb010g commented 6 years ago

One week left until Firefox 57 drops. I haven't worked with AMO before, but I know they have a review process, and am starting to worry a bit about this not being published there before the deadline.

Rothera commented 6 years ago

I have not forgotten. Continuing to put off annoying work. I appreciate the reminders though; I'd otherwise be unfamiliar with the deadlines.

Shugabuga commented 6 years ago

Correct me if I am wrong, but isn’t the review process shorter if it isn’t distributed through Mozilla’s store?

Regardless, we should probably merge one of the WebExtension implementations before it’s too late. We wouldn’t want any issues for those who use BPM.

Rothera commented 6 years ago

WebExt support appears to be working now, so closing. My apologies.

Shugabuga commented 6 years ago

If it works on the latest version, I'm cool with it. That is all I care about in the end: BPM working.