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.

Sign up for GitHub

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

Jump to bottom

url: clean up WHATWG URL origin generation #12252

Closed
wants to merge 2 commits into from
Closed

url: clean up WHATWG URL origin generation #12252

wants to merge 2 commits into from

Conversation

TimothyGu
Copy link
Member

@TimothyGu TimothyGu commented Apr 6, 2017 •
edited
Loading

  • Directly generate serialized string instead of going through class construction + toString()
  • Use template string literals
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

url

@TimothyGu
url: clean up WHATWG URL origin generation
de27517 
- Use ordinary properties instead of symbols/getter redirection for
  internal object
- Use template string literals
- Remove unneeded custom inspection for internal objects
- Remove unneeded OpaqueOrigin class
- Remove unneeded type checks
@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Apr 6, 2017
@mscdex
Copy link
Contributor

mscdex commented Apr 6, 2017

Perhaps originFor() could be also changed/optimized to always return the string representation of the origin (avoiding unnecessary intermediary object creation), since originFor() is only used in one place and immediately calls .toString() on the result?

@TimothyGu
Copy link
Member Author

@mscdex That was my original intent, and I tried to do that in #10955. But it was blocked by #10955 (comment) (not sure if @jasnell's opinion has changed).

@jasnell
Copy link
Member

jasnell commented Apr 6, 2017

I have since refractored the code that was using it. Simplifying should be fine.

@joyeecheung
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/7279/

@addaleax
Copy link
Member

Landed in aff5cc9

@addaleax addaleax closed this Apr 14, 2017
addaleax pushed a commit that referenced this pull request Apr 14, 2017
@TimothyGu @addaleax
url: clean up WHATWG URL origin generation
aff5cc9 
- Use ordinary properties instead of symbols/getter redirection for
  internal object
- Use template string literals
- Remove unneeded custom inspection for internal objects
- Remove unneeded OpaqueOrigin class
- Remove unneeded type checks

PR-URL: #12252
Reviewed-By: James M Snell <jasnell@gmail.com>
@TimothyGu TimothyGu deleted the url-origin branch April 14, 2017 22:33
@TimothyGu TimothyGu mentioned this pull request Apr 19, 2017
Merged
4 tasks
@jasnell jasnell mentioned this pull request May 11, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@TimothyGu @mscdex @jasnell @joyeecheung @addaleax @nodejs-github-bot