Bug 176911 – Add an URL method to remove both query string and fragment identifier

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

Add an URL method to remove both query string and fragment identifier

[youenn fablet]

Created attachment 320780 [details]
Patch

[Alex Christensen]

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.

[youenn fablet]

Created attachment 320828 [details]
Patch

[youenn fablet]

Created attachment 320831 [details]
Patch

[Alex Christensen]

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.

[youenn fablet]

Created attachment 320867 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 320867 [details]
Patch

Clearing flags on attachment: 320867

Committed r222093: <http://trac.webkit.org/changeset/222093>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34693585>