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

src, url: WHATWG URL C++ parser cleanup #11917

Closed
wants to merge 3 commits into from

Conversation

@TimothyGu
Copy link
Member

commented Mar 18, 2017

Some general housekeeping for the WHATWG URL implementation.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

src, url

@TimothyGu

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2017

@TimothyGu TimothyGu force-pushed the TimothyGu:url-cleanup branch Mar 19, 2017

@@ -705,10 +710,11 @@ namespace url {
p++;
continue;
} else if (ch == ':' || (has_state_override && ch == kEOL)) {

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Mar 21, 2017

Member

The scheme state handling seems to deviate quite a bit from the spec now?

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Mar 23, 2017

Author Member

That is unfortunately true. We already deviate from the spec by storing : as part of the scheme, coupled by the fact that we delegate some responsibilities to the JS layer, it's hard to follow the spec word-for-word.

This comment has been minimized.

Copy link
@jasnell

jasnell Mar 23, 2017

Member

As long as we're still API compliant and passing all the tests, such deviation is fine. It would be helpful, however, to document the deviations in code comments.

src/node_url.cc Outdated
} else if (!has_state_override) {
URL_FAILED()
if (port < 0 || port > 0xffff) {
// XXX: This hack is currently needed for the host setter since

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Mar 21, 2017

Member

Better leave a handle for it...like XXX(TimothyGu):

This comment has been minimized.

Copy link
@jasnell

jasnell Mar 22, 2017

Member

nit: Can you just make this TODO(TimothyGu):

@jasnell jasnell self-assigned this Mar 21, 2017

@jasnell
Copy link
Member

left a comment

Generally LGTM

src/node_url.cc Outdated
} else if (!has_state_override) {
URL_FAILED()
if (port < 0 || port > 0xffff) {
// XXX: This hack is currently needed for the host setter since

This comment has been minimized.

Copy link
@jasnell

jasnell Mar 22, 2017

Member

nit: Can you just make this TODO(TimothyGu):

@jasnell

This comment has been minimized.

Copy link
Member

commented Mar 22, 2017

This will need a rebase.

@TimothyGu

This comment has been minimized.

Copy link
Member Author

commented Mar 22, 2017

I'll say this is currently blocked on #11934 as it will change TERMINATED and FAILED handling.

@TimothyGu TimothyGu added the blocked label Mar 22, 2017

@TimothyGu TimothyGu force-pushed the TimothyGu:url-cleanup branch Mar 22, 2017

@TimothyGu TimothyGu removed the blocked label Mar 23, 2017

@TimothyGu TimothyGu force-pushed the TimothyGu:url-cleanup branch Mar 23, 2017

@TimothyGu

This comment has been minimized.

Copy link
Member Author

commented Mar 23, 2017

@jasnell, @joyeecheung, rebased. PTAL.

GET_AND_SET(env, base_obj, scheme, base, URL_FLAGS_HAS_SCHEME);
Local<Value> scheme = GET(env, base_obj, "scheme");
base->scheme = Utf8Value(env->isolate(), scheme).out();

GET_AND_SET(env, base_obj, username, base, URL_FLAGS_HAS_USERNAME);

This comment has been minimized.

Copy link
@jasnell

jasnell Mar 23, 2017

Member

Eventually it would likely be better just to inline this and get rid of the macro

@jasnell
Copy link
Member

left a comment

Generally LGTM with a couple of suggestions

@@ -444,7 +444,6 @@ static inline void PercentDecode(const char* input,
XX(URL_FLAGS_INVALID_PARSE_STATE, 0x04) \
XX(URL_FLAGS_TERMINATED, 0x08) \
XX(URL_FLAGS_SPECIAL, 0x10) \

This comment has been minimized.

Copy link
@jasnell

jasnell Mar 23, 2017

Member

Likely worthwhile to decrement each of the remaining flags accordingly so that there's not a gap.

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Mar 24, 2017

Author Member

Fixed. I was planning on some more refactoring, but that didn't happen.

@TimothyGu TimothyGu force-pushed the TimothyGu:url-cleanup branch Mar 23, 2017

@jasnell

This comment has been minimized.

Copy link
Member

commented Mar 27, 2017

Is this ready to go?

@jasnell jasnell added this to the 8.0.0 milestone Mar 27, 2017

@TimothyGu

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2017

@jasnell, go ahead with landing this if you'd like to. I've been busy the last couple of days.

@jasnell

This comment has been minimized.

@TimothyGu TimothyGu force-pushed the TimothyGu:url-cleanup branch Mar 30, 2017

@TimothyGu

This comment has been minimized.

Copy link
Member Author

commented Mar 30, 2017

src, url: clarify URL_FLAG_TERMINATED
Do not call any completion callback with TERMINATED.

@TimothyGu TimothyGu force-pushed the TimothyGu:url-cleanup branch to 9e9dfe2 Mar 30, 2017

@TimothyGu

This comment has been minimized.

Copy link
Member Author

commented Mar 30, 2017

Landed in 4ddd23f.

@TimothyGu TimothyGu closed this Mar 30, 2017

@TimothyGu TimothyGu deleted the TimothyGu:url-cleanup branch Mar 30, 2017

TimothyGu added a commit that referenced this pull request Mar 30, 2017
src: WHATWG URL C++ parser cleanup
- Clarify port state
- Remove scheme flag
- Clarify URL_FLAG_TERMINATED

PR-URL: #11917
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@jasnell jasnell referenced this pull request Apr 4, 2017
@italoacasas

This comment has been minimized.

Copy link
Member

commented Apr 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
5 participants
You can’t perform that action at this time.