Bug 176911 - Add an URL method to remove both query string and fragment identifier
Summary: Add an URL method to remove both query string and fragment identifier
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-09-14 09:05 PDT by youenn fablet
Modified: 2017-09-27 12:36 PDT (History)
7 users (show)

See Also:


Attachments
Patch (2.92 KB, patch)
2017-09-14 10:08 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (9.77 KB, patch)
2017-09-14 14:58 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (10.11 KB, patch)
2017-09-14 15:17 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (10.05 KB, patch)
2017-09-14 21:24 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2017-09-14 09:05:51 PDT
Add an URL method to remove both query string and fragment identifier
Comment 1 youenn fablet 2017-09-14 10:08:14 PDT
Created attachment 320780 [details]
Patch
Comment 2 Alex Christensen 2017-09-14 10:42:17 PDT
Comment on attachment 320780 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=320780&action=review

> Source/WebCore/platform/URL.cpp:883
> +    *this = URLParser(m_string.left(m_pathEnd)).result();

There's no need to reparse.  Just make m_string a substring up to m_pathEnd and set m_queryEnd to m_pathEnd.
Comment 3 youenn fablet 2017-09-14 14:58:15 PDT
Created attachment 320828 [details]
Patch
Comment 4 youenn fablet 2017-09-14 15:17:13 PDT
Created attachment 320831 [details]
Patch
Comment 5 Alex Christensen 2017-09-14 16:53:53 PDT
Comment on attachment 320831 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=320831&action=review

> Source/WebCore/platform/URL.cpp:861
> -    *this = URLParser { makeString(StringView { m_string }.substring(0, m_queryEnd), '#', identifier) }.result();
> +    m_string = makeString(StringView { m_string }.substring(0, m_queryEnd), '#', identifier);

This is not safe.  We could put non-ASCII characters or control characters like the bell character in the new identifier, and they would need to be percent-encoded.  Just don't touch this function.
As long as you're adding tests, please add a test for this case.

> Source/WebCore/platform/URL.cpp:882
> +    if (!hasQuery()) {
> +        removeFragmentIdentifier();
> +        return;
> +    }

This is unnecessary.

> Source/WebCore/platform/URL.cpp:899
> +        m_string = makeString(view.substring(0, m_pathEnd), "?", query, view.substring(m_queryEnd));

Same thing here.  query might have some characters that need encoding.  Query encoding is more complicated, and it includes more characters and some are dependent on the scheme.
We do actually need to slightly change this function to be more spec-compliant, but that should be a separate patch.  I think the problem was when we set the query to the empty string, just a '?' or two, etc.
Comment 6 youenn fablet 2017-09-14 21:24:34 PDT
Created attachment 320867 [details]
Patch
Comment 7 WebKit Commit Bot 2017-09-15 09:51:21 PDT
Comment on attachment 320867 [details]
Patch

Clearing flags on attachment: 320867

Committed r222093: <http://trac.webkit.org/changeset/222093>
Comment 8 WebKit Commit Bot 2017-09-15 09:51:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2017-09-27 12:36:25 PDT
<rdar://problem/34693585>