Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update port on scheme change + host parsing rules to the host setter + hash parsing rules #523

Closed
wants to merge 4 commits into from

Conversation

o0Ignition0o
Copy link
Contributor

@o0Ignition0o o0Ignition0o commented Jul 20, 2019

On scheme change, we forgot to update the port if it's the new default one.

This will help the test :


        {
            "comment": "Port is set to null if it is the default for new scheme.",
            "href": "http://foo.com:443/",
            "new_value": "https",
            "expected": {
                "href": "https://foo.com/",
                "protocol": "https:",
                "port": ""
            }
        }

pass later, once we're done with the forward slash appending when there's no path... which is quite a challenge for now.


This change is Reviewable

src/lib.rs Outdated Show resolved Hide resolved
@o0Ignition0o
Copy link
Contributor Author

The second commit applies host parsing rules to the host setter. The ':' handling was missing.

This should let the following host example pass:

        {
            "comment": "Port number is unchanged if not specified",
            "href": "http://example.net:8080",
            "new_value": "example.com:",
            "expected": {
                "href": "http://example.com:8080/",
                "host": "example.com:8080",
                "hostname": "example.com",
                "port": "8080"
            }
        },

The remaining failing tests seem to mostly involve the forward slash bug.
There's one javascript remaining test I'm hoping to handle though :)

@o0Ignition0o
Copy link
Contributor Author

The last commit edits the hash setting and getting rules to match the dom url hash specification

This allows the following setter test to pass:

        {
            "href": "javascript:alert(1)",
            "new_value": "castle",
            "expected": {
                "href": "javascript:alert(1)#castle",
                "hash": "#castle"
            }
        }

I would love to solve the forward slash bug, but I will probably not have enough time to tackle it for now :/

@o0Ignition0o o0Ignition0o changed the title Try to update the port on scheme change. Update port on scheme change + host parsing rules to the host setter + hash parsing rules Jul 20, 2019
@o0Ignition0o
Copy link
Contributor Author

a port setter test
seems to be inconsistent with a protocol setter test
There doesn't seem to be any consistency in WPT wrt the "/" when there is no path.
The specification seems to dismiss '/' at the end if there is no path:
" Otherwise, then for each string in url’s path, append U+002F (/) followed by the string to output."

@nox I don't really know how to reliably handle it, is there anything I am missing I should know ?

@o0Ignition0o
Copy link
Contributor Author

Ok the culprit seems to be web-platform-tests/wpt@37a9631#diff-6ef48fcfe9e518dd93d4fe48a795d3ac

@SimonSapin I'll rollback until just before this commit, and push things progressively, working my way step by step

@o0Ignition0o
Copy link
Contributor Author

Subset of of #537

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants