serenity icon indicating copy to clipboard operation
serenity copied to clipboard

Ladybird+Browser: Add Address Bar search and autocomplete

Open cammo1123 opened this issue 2 years ago • 11 comments
trafficstars

This adds a more familiar style of searching across Ladybird. No need to specify ? before searching. URLs are now checked against the open public_suffix_list.dat, and if they aren't found to be valid, we fall through and use the default search engine.

Old Draft Message

Currently, a draft because This covers a lot of the codebase that I'm unfamiliar with, and id really appreciate some overall comments about things like: * PublicSuffixData generator is currently in `Libraries/Userland/LibWeb/PublicSuffix`. I'm not sure where quite to put this. But it is a somewhat logical place. * Have I covered all valid URL's? (Definitely not, but I cant think of many more)

Todo:

  • [x] Fixup settings in SerenityOS
  • [x] Find a better place for ErrorOr<String> absolute_url(StringView url)
  • [x] Match the available list of engines on both platforms

https://github.com/SerenityOS/serenity/assets/36214028/e4cbdf53-7e79-41bd-a676-6db948dc2bc0

cammo1123 avatar May 26 '23 15:05 cammo1123

One comment before looking at this, according to the public suffix list, it seems they would rather we only pull it "once per day" to avoid load on their servers. Our CI would end up pulling it many more times than that per day, let alone developers using it.

The copy on publicsuffix.org, linked below, is updated daily from Github. If you wish to make your app download an updated list periodically, please use this URL and have your app download the list no more than once per day. (The list usually changes a few times per week; more frequent downloading is pointless and hammers our servers.)

ADKaster avatar May 26 '23 15:05 ADKaster

One comment before looking at this, according to the public suffix list, it seems they would rather we only pull it "once per day" to avoid load on their servers. Our CI would end up pulling it many more times than that per day, let alone developers using it.

I did catch that. I don't know what would be a good solution to this. They do have a copy on GitHub that we could here. Surely GitHub could take that load?

Also, while I'm thinking about it, I should add a fallback so if the files cant be downloaded, it will revert to always attempting to load the page.

Edit: I'll do this in a later PR if that's okay.

cammo1123 avatar May 26 '23 15:05 cammo1123

Changes since the last review

  • use GitHub hosted mirror so we don't overload the public suffix list requests of pulling once per day
  • Added bypass PublicSuffixChecks when CTRL is pressed in Ladybird

cammo1123 avatar May 29 '23 09:05 cammo1123

In light of https://github.com/SerenityOS/serenity/blob/58b1d9c319fbafac8318bffbf41380e1c9ee1175/Userland/Libraries/LibGUI/TextBox.cpp#L185-L186 we might want to move the suffix list data into some other library to avoid linking LibWeb into LibGUI

networkException avatar May 29 '23 11:05 networkException

In light of

https://github.com/SerenityOS/serenity/blob/58b1d9c319fbafac8318bffbf41380e1c9ee1175/Userland/Libraries/LibGUI/TextBox.cpp#L185-L186

we might want to move the suffix list data into some other library to avoid linking LibWeb into LibGUI

I added that FIXME When I added address highlighting, personally, I feel like the system we have now is more than adequate (Making the FIXME redundant) and that adding more granular highlighting is unnecessary. But I agree it isn't a good idea to have to link against libweb to see if a URL has a public suffix. Do you have any idea what soft of libraries would better suit it?

cammo1123 avatar May 29 '23 12:05 cammo1123

that adding more granular highlighting is unnecessary

I think it is very much necessary, especially in a browser but for the sake of consistency also in the whole system. I think its important to correctly reflect the internals of the engine and expose that information to the user

networkException avatar May 29 '23 12:05 networkException

much necessary, especially in a browser but for the sake of consistency also in the whole system. I think its important to correctly reflect the internals of the engine and expose that in

I wasn't going to remove the FIXME, if someone else wants to add it they can by all means :^). What's your suggestion on where to relocate the PublicSuffix stuff?

cammo1123 avatar May 29 '23 13:05 cammo1123

I can only think of a LibPublicSuffix, but I'd ask on discord about this

networkException avatar May 29 '23 13:05 networkException

Ready for review!!

Changes:

  • New Library LibPublicSuffix
  • Use a trie to search for matching domains
  • Dont allow single top-level domains eg com or org

cammo1123 avatar May 31 '23 13:05 cammo1123

Is there UI to opt out of this?

networkException avatar Jun 03 '23 23:06 networkException

Is there UI to opt out of this?

There is now :)

cammo1123 avatar Jun 07 '23 01:06 cammo1123

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

stale[bot] avatar Jul 22 '23 09:07 stale[bot]

This doesn't currently build because of very recent changes to the URL class.

Generally looks fine. The Qt-specific things I only skimmed over though.

Fixed the build issue if you want to test it out, will look into constructing a failable singleton in the morning when I've had some sleep :^)

cammo1123 avatar Jul 31 '23 13:07 cammo1123

This has a lot of conflicts. :sweat_smile:

AtkinsSJ avatar Aug 09 '23 11:08 AtkinsSJ

This has a lot of conflicts. 😅

What conflicts?? (Fixed them all :^) )

cammo1123 avatar Aug 09 '23 13:08 cammo1123

This has conflicts again because the Qt files got moved. :sweat_smile:

AtkinsSJ avatar Aug 16 '23 10:08 AtkinsSJ

This has conflicts again because the Qt files got moved. 😅

Pretty simple fix! should be ready for review now!!

cammo1123 avatar Aug 17 '23 06:08 cammo1123

All fixed!!

cammo1123 avatar Aug 17 '23 13:08 cammo1123

Let's goooo!

AtkinsSJ avatar Aug 17 '23 14:08 AtkinsSJ