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

Change path parsing for non-special URLs #213

Merged
merged 2 commits into from
Jan 31, 2017
Merged

Conversation

annevk
Copy link
Member

@annevk annevk commented Jan 21, 2017

This allows paths to be empty for non-special URLs and also takes that
into account when serializing.

Tests: web-platform-tests/wpt#4586.

Fixes #212.

@annevk
Copy link
Member Author

annevk commented Jan 24, 2017

@domenic this is ready for review too, but note that it builds on top of the opaque host change.

@annevk annevk requested a review from domenic January 24, 2017 11:38
This allows paths to be empty for non-special URLs and also takes that
into account when serializing.

Tests: web-platform-tests/wpt#4586.

Fixes #212.
@annevk annevk force-pushed the annevk/non-special-paths branch from 5e6d4e5 to cea4ff8 Compare January 24, 2017 19:55
domenic added a commit to jsdom/whatwg-url that referenced this pull request Jan 24, 2017
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to wait until web-platform-tests/wpt#4586 are rebased before reviewing further

<ol>
<li><p>If <a>c</a> is "<code>\</code>", <a>syntax violation</a>.

<li><p>Set <var>state</var> to <a>path state</a> and if <a>c</a> is neither "<code>/</code>"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This really seems like it'd be better as two steps.

@domenic
Copy link
Member

domenic commented Jan 24, 2017

This does not quite match the tests yet. I noticed that the spec does not appear to match the parse path start changes in #212 (comment). You can see the spec translated into whatwg-url at jsdom/whatwg-url#70.

The failing tests are:

 1) Web platform tests parsing <http:\\foo.com\> against <http://example.org/foo/bar>:

      AssertionError: href
      + expected - actual

      -http://example.org/foo.com/
      +http://foo.com/

  2) Web platform tests parsing <http:\\a\b:c\d@foo.com\> against <http://example.org/foo/bar>:

      AssertionError: href
      + expected - actual

      -http://example.org/a/b:c/d@foo.com/
      +http://a/b:c/d@foo.com/

  3) Web platform tests parsing <..> against <file:///1:/>:

      AssertionError: pathname
      + expected - actual

      -/
      +//

  4) Web platform tests parsing <///> against <sc://x/>:

      AssertionError: href
      + expected - actual

      -sc://x///
      +sc:///

  5) Web platform tests parsing <////> against <sc://x/>:

      AssertionError: href
      + expected - actual

      -sc://x////
      +sc:////

  6) Web platform tests parsing <////x/> against <sc://x/>:

      AssertionError: href
      + expected - actual

      -sc://x////x/
      +sc:////x/

@annevk
Copy link
Member Author

annevk commented Jan 27, 2017

I rewrote the path start changes to be a little neater, they should be identical. I found a fault in your code elsewhere that I think is more likely responsible for the failures you are seeing.

@annevk
Copy link
Member Author

annevk commented Jan 27, 2017

I still get two failures I find a little harder to narrow down. Investigating.

@annevk
Copy link
Member Author

annevk commented Jan 27, 2017

It seems those two failures were actually broken tests. Fixed in the web-platform-tests PR. Provided a patch in the whatwg-url PR.

Will address your style nit here and then I think we're back to all good.

@annevk annevk requested a review from domenic January 30, 2017 09:35
domenic added a commit to jsdom/whatwg-url that referenced this pull request Jan 31, 2017
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed whatwg-url and now things look good to go.

@annevk annevk merged commit b087fe2 into master Jan 31, 2017
@annevk
Copy link
Member Author

annevk commented Jan 31, 2017

@achristensen07 I assume you are tracking this in WebKit?

I filed no bugs on other browsers since they're not actively implementing this part of the standard yet.

@annevk annevk deleted the annevk/non-special-paths branch January 31, 2017 10:45
@achristensen07
Copy link
Collaborator

Yep, I'm following this. I actually needed to implement this a few months ago to maintain compatibility. It was implemented in pieces as I found the compatibility issues.
Started with https://bugs.webkit.org/show_bug.cgi?id=163373 (yuck)
Getting closer in https://bugs.webkit.org/show_bug.cgi?id=163413
https://bugs.webkit.org/show_bug.cgi?id=164290 fixed issue 148 which is related
https://bugs.webkit.org/show_bug.cgi?id=165621 was also necessary for this

domenic added a commit to jsdom/whatwg-url that referenced this pull request Jan 31, 2017
watilde added a commit to watilde/node that referenced this pull request Apr 2, 2017
This changes to the way path parsing for non-special URLs.
It allows paths to be empty for non-special URLs and also
takes that into account when serializing.

Fixes: nodejs#11962
Refs: whatwg/url#213
watilde added a commit to nodejs/node that referenced this pull request Apr 3, 2017
This changes to the way path parsing for non-special URLs.
It allows paths to be empty for non-special URLs and also
takes that into account when serializing.

Fixes: #11962
Refs: whatwg/url#213
PR-URL: #12058
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
TimothyGu pushed a commit to TimothyGu/node that referenced this pull request Apr 25, 2017
This changes to the way path parsing for non-special URLs.
It allows paths to be empty for non-special URLs and also
takes that into account when serializing.

PR-URL: nodejs#12507
Fixes: nodejs#11962
Refs: whatwg/url#213
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit to nodejs/node that referenced this pull request May 1, 2017
This changes to the way path parsing for non-special URLs.
It allows paths to be empty for non-special URLs and also
takes that into account when serializing.

PR-URL: #12507
Fixes: #11962
Refs: whatwg/url#213
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants