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

Remove web extensions #3270

Merged
merged 5 commits into from
Dec 6, 2023
Merged

Remove web extensions #3270

merged 5 commits into from
Dec 6, 2023

Conversation

aartaka
Copy link
Contributor

@aartaka aartaka commented Dec 4, 2023

Description

This removes all the WebExtensions-related code from Nyxt, in favor of external libwebextensions (to be open-sourced separately).

Fixes #2917
Fixes #2114

Checklist:

Everything in this checklist is required for each PR. Please do not approve a PR that does not have all of these items.

  • Git hygiene:
    • I have pulled from master before submitting this PR
    • There are no merge conflicts.
  • I've added the new dependencies as:
    • ASDF dependencies,
    • Git submodules,
      cd /path/to/nyxt/checkout
      git submodule add https://gitlab.common-lisp.net/nyxt/py-configparser _build/py-configparser
    • and Guix dependencies.
  • My code follows the style guidelines for Common Lisp code. See:
  • I have performed a self-review of my own code.
  • My code has been reviewed by at least one peer. (The peer review to approve a PR counts. The reviewer must download and test the code.)
  • Documentation:
    • All my code has docstrings and :documentations written in the aforementioned style. (It's OK to skip the docstring for really trivial parts.)
    • I have updated the existing documentation to match my changes.
    • I have commented my code in hard-to-understand areas.
    • I have updated the changelog.lisp with my changes if it's anything user-facing (new features, important bug fix, compatibility breakage).
      • Changelog update should be a separate commit.
    • I have added a migration.lisp entry for all compatibility-breaking changes.
    • (If this changes something about the features showcased on Nyxt website) I have these changes described in the new/existing article at Nyxt website or will notify one of maintainters to do so.
  • Compilation and tests:
    • My changes generate no new warnings.
    • I have added tests that prove my fix is effective or that my feature works. (If possible.)
    • I ran the tests locally ((asdf:test-system :nyxt) and (asdf:test-system :nyxt/gi-gtk)) and they pass.

@aadcg
Copy link
Member

aadcg commented Dec 4, 2023

For future reference, @aartaka, would you like to briefly state why libwebextensions is superior to the strategy that is being removed in this PR?

Note that this PR is not complete. For instance, file INSTALL has references to web extensions. I'd suggest carefully grepping through the repository.

@aartaka
Copy link
Contributor Author

aartaka commented Dec 5, 2023

For future reference, aartaka, would you like to briefly state why libwebextensions is superior to the strategy that is being removed in this PR?

Sure! libwebextensions is more embeddable, self-contained, and much more maintainable due to being written in Scheme instead of C.

Note that this PR is not complete. For instance, file INSTALL has references to web extensions. I'd suggest carefully grepping through the repository.

Done.

@aadcg
Copy link
Member

aadcg commented Dec 5, 2023

@aartaka take a closer look at INSTALL or class gtk-extensions-directory in gtk.lisp.

@aadcg
Copy link
Member

aadcg commented Dec 5, 2023

@aartaka should this PR close #2114?

@aartaka aartaka force-pushed the remove-web-extensions branch 3 times, most recently from 7e63e69 to c6fe367 Compare December 6, 2023 10:28
@aartaka
Copy link
Contributor Author

aartaka commented Dec 6, 2023

Okay, bot of the cases (INSTALL and gtk-extensions-directory) are covered. I've also grep-ped a bit and fixed some small remaining mentions. Rebased over master. Ready for review/merge.

@aadcg
Copy link
Member

aadcg commented Dec 6, 2023

@aartaka please double-check. Thanks.

@aartaka
Copy link
Contributor Author

aartaka commented Dec 6, 2023

@aartaka please double-check. Thanks.

The only ones I can find are:

  • Mention of WebExtensions in the manual section of user scripts.
  • Mention of WebKitWebExtensions in renderer/gtk.

Am I missing something?

@aadcg
Copy link
Member

aadcg commented Dec 6, 2023

Is gtk-extensions-directory from gtk.lisp related to web extensions? It is still in the sources.

@aartaka
Copy link
Contributor Author

aartaka commented Dec 6, 2023

Is gtk-extensions-directory from gtk.lisp related to web extensions? It is still in the sources.

It's related to WebKitWebExtensions, not WebExtensions. These two are distinct entities. The former is the extension mechanism provided by WebKitGTK and working as shared libraries loaded at runtime. The latter is JavaScript-based conventional architecture for browser extensions supported by major browsers like Mozilla Firefox and Google Chrome.

@aadcg
Copy link
Member

aadcg commented Dec 6, 2023

I'm still confused.

From the INSTALL file:

By default the GTK renderer loads the libnyxt library (enabling WebExtension
support) relatively to the nyxt.asd location. You can also configure this at
runtime through the gtk-extensions-directory resolve method.

This seems to suggest that gtk-extensions-directory is indeed related to libnyxt (libraries/web-extensions), which this PR deletes.

@aartaka
Copy link
Contributor Author

aartaka commented Dec 6, 2023

You sure you're checking the right branch? 2c94866 removes this snippet.

@aadcg
Copy link
Member

aadcg commented Dec 6, 2023

Yes, I am checking the right branch. Let me rephrase my previous question since there was a misunderstanding.

If the snippet from INSTALL has been deleted and it references gtk-extensions-directory, what justifies the need for gtk-extensions-directory in gtk.lisp? I believe the answer is that it is an entry point for WebKitWebExtension (which differ from WebExtensions). My question is: what value do we get from having this entry point for WebKitWebExtensions? It seems that, in the past, it was used exclusively for libnyxt, i.e. it was meant for supporting WebExtensions. Put differently, now that we're not going to pursue WebExtensions in that fashion, what justifies the need for the entry point?

@aartaka
Copy link
Contributor Author

aartaka commented Dec 6, 2023

My question is: what value do we get from having this entry point for WebKitWebExtensions? It seems that, in the past, it was used exclusively for libnyxt, i.e. it was meant for supporting WebExtensions. Put differently, now that we're not going to pursue WebExtensions in that fashion, what justifies the need for the entry point?

It's useful to allow other kinds of WebKit extensions, like blockit etc. Just to be a good WebKitGTK browsers citizen.

@aadcg
Copy link
Member

aadcg commented Dec 6, 2023

@aartaka this is not on topic, but it follows from the piece of information that you have shared. This has been looking at us all along then. There are webkitgtk-specific extensions such as blockit that would add great value to Nyxt. Ad blocking extensions are the most requested kind of extension. How hard would it be to support blockit in Nyxt? Maybe we should open an issue about it.


I'll do a final review and merge.

@aadcg aadcg force-pushed the remove-web-extensions branch from c6fe367 to 7d39fd9 Compare December 6, 2023 18:37
@aadcg aadcg merged commit 3055ab4 into master Dec 6, 2023
2 checks passed
@aadcg aadcg deleted the remove-web-extensions branch December 6, 2023 18:38
@aadcg
Copy link
Member

aadcg commented Dec 6, 2023

I've added the changelog myself, but please take it into account for the future.

Great, the codebase is now a little bit lighter!

@aartaka
Copy link
Contributor Author

aartaka commented Dec 7, 2023 via email

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

Successfully merging this pull request may close these issues.

Optionally build web-extension*.lisp Expand WebExtensions support
2 participants