LibWeb/URL: Add strip_trailing_spaces_from_an_opaque_path()

[kemzeb]

Adds https://url.spec.whatwg.org/#potentially-strip-trailing-spaces-from-an-opaque-path.

Tested the code paths in set_search() and set_hash() and they appear fine, though I think the design may be questionable, specifically with exposing more methods and delegating the work in those methods to AK::URL.

[shannonbooth]

Interestingly, seems like there is a change being proposed that is removing this section of the spec: whatwg/url#785 (only up for discussion right now)

[kemzeb]

Nice find! From reading it (and my basic knowledge of character encoding that I should to work on), I believe some of the possible approaches they give would not have too much of an impact on my implementation.

[kemzeb]

Recent snapshot implements AK::serialize_path() to spec, but not sure if the spec is correct. "If url has an opaque path, then return url’s path." Why don't we prefix a /?

Even weirder, when I tried to do it myself by returning a DeprecatedString::formatted("/{}", m_paths[0]), the test suite hangs. Not sure what's going on, but the test suite should fail right now since the expected opaque pathnames won't have a / prefixed.

[kemzeb]

The most recent change removes AK::URL::is_opaque_path(). It also reverts AK::URL::serialize_path() back to using cannot_be_a_base_url() as its first step because of the problems mentioned in this comment.

I also had to modify 2 AK/TestURL.cpp test cases since they fail, but appear to be incorrect. Running the test cases "unicode" and "file_url_with_encoded_characters" on Chrome by doing the following in the DevTools console:

let url = new URL("http://example.com/_ünicöde_téxt_©");
undefined
url.pathname
'/_%C3%BCnic%C3%B6de_t%C3%A9xt_%C2%A9'

let url = new URL("http://example.com/a%23test");
undefined
url.pathname
'/a%23test'

their expected output follows inline with the changes I made to these test cases.

I also made changes to the strip_trailing_spaces_from_an_opaque_path() due to the the removal of AK::URL::is_opaque_path(). I also changed the expected test cases to have URL-encoded space characters.

[kemzeb]

My recent change fixes a comment typo

[shannonbooth]

The most recent change removes AK::URL::is_opaque_path(). It also reverts AK::URL::serialize_path() back to using cannot_be_a_base_url() as its first step because of the problems mentioned in this comment.

I also had to modify 2 AK/TestURL.cpp test cases since they fail, but appear to be incorrect. Running the test cases "unicode" and "file_url_with_encoded_characters" on Chrome by doing the following in the DevTools console:

let url = new URL("http://example.com/_ünicöde_téxt_©");
undefined
url.pathname
'/_%C3%BCnic%C3%B6de_t%C3%A9xt_%C2%A9'

let url = new URL("http://example.com/a%23test");
undefined
url.pathname
'/a%23test'

their expected output follows inline with the changes I made to these test cases.

I also made changes to the strip_trailing_spaces_from_an_opaque_path() due to the the removal of AK::URL::is_opaque_path(). I also changed the expected test cases to have URL-encoded space characters.

I think we need to preserve the behaviour of the existing AK tests here. Existing code in serenity outside of Web specs expect that the path is percent decoded. I removed this flag in: db5ad0c since nothing was actually setting that flag, and also fixed some places that were percent decoding when we shouldn't have.

But it appears I shouldn't have removed that flag for "serialized_path" (or we need a different API which is a bit more clear?) - we need to be setting it here to follow web specs but not for serenity code outside of LibWeb which expected a percent-decoded path.

[BuggieBot]

Hello!

One or more of the commit messages in this PR do not match the SerenityOS code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why.
Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.

[kemzeb]

I should note that I changed the signature of LibWeb::URL::path_segment_at_index() to just directly return a DeprecatedString. I also added LibWeb::URL::set_paths() which delegates its work to AK::URL::set_paths() because LibWeb::URL::set_pathname() returns immediately if it finds a URL that is an opaque path.

[shannonbooth]

Looks reasonable to me

[ADKaster]

Hm. Kind of weird that the spec has a bulleted list for this instead of a numbered list. Might be worth a spec issue in https://github.com/whatwg/url if you're feeling speccy :)

[kemzeb]

Yea I wasn't too sure if this was intentional too. I'll file an issue there 👍

[kemzeb]

Opened an issue at whatwg/url#786