Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use browser-polyfill.js to allow use with chromium #69

Merged
merged 2 commits into from
Jan 23, 2022

Conversation

eNV25
Copy link
Contributor

@eNV25 eNV25 commented Jan 13, 2022

This change adds browser-polyfill.js so that it can also be packaged for chromium. This should result in no change in how the ff2mpv.js is executed on firefox.

More about browser-polyfill: https://github.com/mozilla/webextension-polyfill

  • ./package-chromium.sh generates dist.crx and dist.pem. The extension id is generated from tied to this private key file, keep it around so that it is reused and does not change.
  • Load the extension in chromium by dragging and dropping it in the extension page.
  • Make a ff2mpv-chromium.json manifest from ff2mpv-chromium.example.json. Replace the path with the correct path and <your-extension-id> with the extension id from chrome:https://extensions.
  • Put the external manifest in the correct location: https://developer.chrome.com/docs/apps/nativeMessaging/#native-messaging-host-location
  • You might need to enable developer mode in the extensions page. This should work in chromium builds in Linux distros, but I am not sure about official Google Chrome builds. You could also use "Load unpacked" to load this extension but this generates a new extension id every time so it's a pain in the ass.

If someone uploads this to the chrome webstore and keeps the private key file safe the extension id would stay the same, there would be no need for this dance and we could just have one ff2mpv-chrome.json manifest.

NOTE: This has nothing to do with the existing chrome port by @DanSM-5 https://github.com/DanSM-5/ff2mpv https://chrome.google.com/webstore/detail/ff2mpv/ephjcajbkgplkjmelpglennepbpmdpjg . In the future maybe he could upload this version of the chrome port to the chrome webstore, instead of using a fork.

@eNV25 eNV25 marked this pull request as draft January 13, 2022 08:29
package-zip.sh Outdated Show resolved Hide resolved
browser-polyfill.min.js Outdated Show resolved Hide resolved
@woodruffw woodruffw added enhancement B:chromium Support for Chromium-based browsers labels Jan 13, 2022
@woodruffw
Copy link
Owner

Very cool, thanks for looking into this. I left a few questions/nitpicks above, but overall this LGTM.

@woodruffw
Copy link
Owner

cc @DanSM-5: how do you feel about unifying on a single port of ff2mpv for Chrome?

@eNV25
Copy link
Contributor Author

eNV25 commented Jan 13, 2022

Something to note is Chrome's switch to manifest v3 and the manifest v2 support timeline. This extension uses manifest v2.

https://developer.chrome.com/docs/extensions/mv3/mv2-sunset/

image

It looks like after this month new mv2 extensions cannot be added to the Chrome Web Store, but old ones can be updated for a year. So the best bet for getting this in the Chrome Web Store in this time frame is updating @DanSM-5 fork with this version.

The long term strategy for supporting Chrome based browsers should be adopting manifest v3. But this will probably require some changes to the code (eg. how the extension finds the video from the current page). I will probably try to experiment with it at some point in the future.

AFAIK Firefox will keep supporting Manifest v2 and also support Manifest v3 in the future for Chrome compatibility. So even if the extension doesn't adopt manifest v3 it should still work with Firefox. Also Firefox nightly does not yet support Manifest v3.

@woodruffw
Copy link
Owner

Thanks for noting that. Yeah, in that case, unifying with the existing Chrome extension makes sense. That will require @DanSM-5 to either become the release manager for Chrome or for them to give me their current signing keys, in which case I don't mind taking over the responsibility.

And yeah, we should eventually switch to Manifest v3 overall. I think we can safely earmark that for the point in time when Firefox ESR supports it.

@eNV25 eNV25 marked this pull request as ready for review January 13, 2022 20:56
@eNV25 eNV25 changed the title Use browser-polyfill.js to allow use with chrom{ium,e} Use browser-polyfill.js to allow use with chromium Jan 13, 2022
@eNV25 eNV25 mentioned this pull request Jan 14, 2022
@DanSM-5
Copy link
Contributor

DanSM-5 commented Jan 14, 2022

I think it would be great to unify the development. I don't mind giving you the signing keys @woodruffw. I published it to the chrome store just to avoid updating the app id every time.

@woodruffw
Copy link
Owner

I think it would be great to unify the development. I don't mind giving you the signing keys @woodruffw. I published it to the chrome store just to avoid updating the app id every time.

Awesome! We can arrange some kind of secure channel for the keys. Does Keybase or email + PGP work for you?

@DanSM-5
Copy link
Contributor

DanSM-5 commented Jan 14, 2022

I'm not familiar with keybase. Do you recommend it? I can encrypt it with PGP and send it over email.

@woodruffw
Copy link
Owner

woodruffw commented Jan 15, 2022 via email

@DanSM-5
Copy link
Contributor

DanSM-5 commented Jan 15, 2022

All set, let me know if you received it.

@woodruffw
Copy link
Owner

woodruffw commented Jan 15, 2022 via email

@eNV25
Copy link
Contributor Author

eNV25 commented Jan 16, 2022

Just to understand what is going, why is the private key being transferred? As far as I understand it would only allow you to locally sign the extension while having the same extension id as @DanSM-5 's fork. I don't think it allows you to publish to the chrome webstore.

https://stackoverflow.com/questions/5604491/how-do-i-transfer-the-ownership-of-a-chrome-extension

According to this Stack Overflow to transfer an extension, you need to use a form and have a valid destination account that has paid the $5 fee.

https://support.google.com/chrome_webstore/contact/dev_account_transfer

But it looks like you can also set up group publishing so multiple devs can manage one extension.

https://developer.chrome.com/docs/webstore/group-publishers/
https://developer.chrome.com/docs/webstore/group-publishers/#move-existing-items-to-a-group-publisher-account

Maybe that is also a good option?

@woodruffw
Copy link
Owner

Just to understand what is going, why is the private key being transferred? As far as I understand it would only allow you to locally sign the extension while having the same extension id as @DanSM-5 's fork. I don't think it allows you to publish to the chrome webstore.

Oh, I badly misunderstood then -- I thought they key was needed for publishing to the webstore (I've never done any Chrome extension publishing/management before). Sorry for the confusion.

Group publishing looks like it's the right approach, although it's weirdly baked into Google Groups. I could create one (I have a G Suite account) if that works for you @DanSM-5, unless you'd prefer to transfer it outright.

@DanSM-5
Copy link
Contributor

DanSM-5 commented Jan 16, 2022

I was confused as well. I didn't know about the limitations of the Chrome extension store. Although I think the private key will be helpful if there is a plan to provide signed packages (crx) directly from the releases.

@woodruffw I agree with the group publishing. You can add me to a group and I will add the extension to it. Just want to mention this, it looks like there is a limit per account to create a single group publishing in the chrome store.

A Chrome Web Store developer account can only create one group publisher, ever. You cannot replenish this quota, even if you delete the group.

If you don't want to use your quota on this, I can volunteer keep updating the extension in the chrome store once this PR gets merged.

@woodruffw
Copy link
Owner

Hooray for arbitrary restrictions! I probably won't ever write another extension, so I don't mind burning my quota on this. I'll set up the group and add you to it, so that there's an extra layer of redundancy for getting releases made for Chrome.

@woodruffw
Copy link
Owner

Okay, I've created the group and sent you an invite to it. Let me know if you have any problems with the invite.

@DanSM-5
Copy link
Contributor

DanSM-5 commented Jan 18, 2022

@woodruffw I waited all day but I didn't received any invitation to join. Could you try to add me again?

@woodruffw
Copy link
Owner

That's odd; it shows you as a member currently. I'll send you an email with a screenshot of what I'm seeing and we'll try to resolve over email (I don't want to disclose your email address or full name on GitHub without your permission, since they aren't immediately visible on your profile.)

@DanSM-5
Copy link
Contributor

DanSM-5 commented Jan 18, 2022

My bad, I am part of the group but I didn't get any notification about it. The group only appears if I try to make a transfer for the extension. I moved the extension to the group. You should have access to it now.

@woodruffw
Copy link
Owner

No problem; everything about this is Google's fault 😉

Yep, I also just confirmed that I have access to the extension. Now that that's all settled, I'll do another review pass of this PR and, with any luck, begin cutting release for both browsers this week!

@eNV25
Copy link
Contributor Author

eNV25 commented Jan 18, 2022

I will add a commit with a external manifest json with the extension id already added.

@eNV25 eNV25 marked this pull request as draft January 18, 2022 10:11
@eNV25 eNV25 marked this pull request as ready for review January 18, 2022 10:17
ff2mpv-chromium.example.json Outdated Show resolved Hide resolved
Copy link
Owner

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Once https://github.com/woodruffw/ff2mpv/pull/69/files#r790180598 is in, I'll merge this and cut a release for both platforms.

Thanks for all of the hard work here, and thanks again @DanSM-5 for the coordination.

@woodruffw
Copy link
Owner

Trying things out locally now. It looks like snap-distributed Chromium doesn't play nice with native messaging, so I need to install a non-snap version. But that's not our problem or something we want to solve (it's snap's fault for having completely broken sandboxing).

@woodruffw
Copy link
Owner

Works locally for me! I'm going to merge, do a small amount of clean-up, and cut a new release for both platforms.

@woodruffw woodruffw merged commit 4266030 into woodruffw:master Jan 23, 2022
@woodruffw
Copy link
Owner

Cut as 4.0.0 and submitted for review to the Chrome Store. Will be published shortly on AMO as well.

@eNV25 eNV25 deleted the pr/chromium branch August 22, 2023 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B:chromium Support for Chromium-based browsers enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants