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

Add partial support for Chromium browsers #3

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

NyanKiyoshi
Copy link

@NyanKiyoshi NyanKiyoshi commented Mar 12, 2023

This adds partial support for chromium-based browsers (including Google Chrome). window.browser is only defined in firefox but window.chrome is available both in firefox and chromium and works just the same.

This only partially supports chromium as loadReplace is not supported in chromium:

I am not aware of workarounds for loadReplace, thus to make it work on chromium-based browsers it requires to remove the line.

Tested on:

  • Firefox: 111.0b8
  • Google Chrome: 111.0.5563.64
  • Brave: version 1.51.24 (chromium: 111.0.5563.64)

This adds partial support for chromium-based browsers (including Chrome). `window.browser` is only defined in firefox but `window.chrome` is available both in firefox and chromium and works just the same.

This only partially supports chromium as `loadReplace` is not supported in chromium:
- https://github.com/dmlls/yang/blob/6dd101e9a27e8620ffe8fa7641b00ee8e8ef42e8/background.js#L92
- https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/Tabs/update#browser_compatibility
@dmlls
Copy link
Owner

dmlls commented Mar 13, 2023

Hi @NyanKiyoshi, thanks a lot for the PR!

Even though Firefox does support chrome, it is not a one-to-one browser replacement.

So how do we feel about following MDN recommendations (see link above) and using WebExtension browser API Polyfill?

I know at this point this is a bit like killing a fly with a sledgehammer, but the extension will likely grow overtime (e.g., custom bangs are next), and using browser.* will also give us support for Safari.

About loadReplace, I guess we can keep this option only active for Firefox with a simple if check. I think it's nice to be able to press the back button to go back to the page you were on before executing the bang.

@dmlls
Copy link
Owner

dmlls commented Mar 14, 2023

@NyanKiyoshi since you opened the PR, would you like to add the polyfill?

It should be straightforward following the MDN docs. Since releases are not that frequent, I suggest we simply include the browser-polyfill.js file in the project instead of using npm.

@NyanKiyoshi
Copy link
Author

Hi @dmlls, thank you for the details. I wasn't aware about those differences between the two implementations; it is indeed concerning as, just like you mentioned, it may negatively impact what your project can do in the future.

I will try to find some time in the coming days or weeks to introduce the polyfill library. I will also look deeper into the loadReplace issue.

I'm also aware that Google Chrome is throwing warnings as the project is using manifest v2 instead of v3 since Google Chrome deprecated the old manifest. It seems like it would be possible to easily upgrade but I would think it should be tackled as a separate PR and thus could be a technical debt/feature request to upgrade it. I do notice that Brave doesn't put such warnings, perhaps not all Chromium vendors are deprecating it.

@dmlls
Copy link
Owner

dmlls commented Mar 16, 2023

@NyanKiyoshi so actually, after reading mozilla/webextension-polyfill#329 and the other issues that reference it, apparently the polyfill won't be of help anymore with MV3.

I wonder if then we should just focus on migrating to MV3 directly to avoid working twice in the future... Might be especially a good idea in the current state of the project, given that there are less than 100 lines of code to migrate.

I can open a PR in the following days and then it would be great if you could test it on Chrome. Does that sound good to you?

@NyanKiyoshi
Copy link
Author

@dmlls sounds great 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants