LibURL+LibWeb+UI: Remove the implicit URL constructors #3668
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is the same type as what is spec'd. We cannot use a URL record for this member as the spec in some scenarios will set and compare the URL string to an invalid URL value, such as the empty string. With implicit string constructors for the URL class removed explicitly using URL::Parser::basic_parse makes the code look quite silly in those places.
Instead of potentially passing through an invalid URL.
Instead of relying on the implicit URL constuctor. Parsing should never fail here as urlString before adding the suffix is already parsed above, and the suffix should only be a valid query string.
This is a bit weird in the spec in it passing through a string here instead of a URL record. However, the string being used in this case should only ever be a valid URL string if it is not the empty string.
Removing one more caller of the implicit URL constructors.
All URLs are now either constucted through the URL Parser or by default constructing a URL, and setting each of the fields of that URL manually. This makes it much more difficult to create invalid URLs.
shannonbooth
force-pushed
the
remove-valid-state
branch
from
f08a2f1 to
cbf0ac8
Compare
trflynn89
reviewed
Feb 23, 2025
| @@ -521,14 +518,15 @@ ErrorOr<void> HTMLImageElement::update_the_image_data(bool restart_animations, b | |||
| // 1. Parse selected source, relative to the element's node document. | |||
| // If that is not successful, then abort this inner set of steps. | |||
| // Otherwise, let urlString be the resulting URL string. | |||
| auto url_string = document().parse_url(selected_source.value()); | |||
| if (!url_string.has_value()) | |||
| auto maybe_url = document().parse_url(selected_source.value()); | |||
There was a problem hiding this comment.
The only thing I don't love about this commit is the back-and-forth between strings and URLs. For example, here we:
- Parse a string to a URL
- Serialize that URL back to a string to store in the current request
- The current request re-parses the URL to create a shared resource request
- We then have to serialize URLs again later to compare to the stored string
Getting whiplash 😅
Would it make more sense to either:
- Keep storing a URL in the current request, but make it
Optional<URL>. And we treat an empty string asOptionalNone. - Cache the URL on the request, so we have both the string and URL readily available? Then make
set_current_urltake an optional URL parameter so we can pass in the already-parsed URL here.
Comment on lines
+325
to
+326
| auto url = URL::Parser::basic_parse(resource.value()->file_url()); | ||
| VERIFY(url.has_value()); |
There was a problem hiding this comment.
Doing this a bunch makes me wonder if we should just have an explicit infallible URL parser
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
All URLs are now either constucted through the URL Parser or by
default constructing a URL, and setting each of the fields of that
URL manually. This makes it much more difficult to create invalid
URLs to enable removing it's valid state.