Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Depend on URL parsing library #6183

Closed
1ec5 opened this issue Aug 26, 2016 · 8 comments
Closed

Depend on URL parsing library #6183

1ec5 opened this issue Aug 26, 2016 · 8 comments
Labels
Core The cross-platform C++ core, aka mbgl refactor

Comments

@1ec5
Copy link
Contributor

1ec5 commented Aug 26, 2016

We’ve had some hairy issues with the code in mbgl::util::mapbox that relies on artisanal string parsing to manipulate URL components. For example: #5554 #5723. At one point, it was manageable because we were only manipulating schemes and paths. But increasingly we need to work with query parameters as well (#6182). I guess I’m spoiled by the robustness of URL on the Web and NSURL and NSURLComponents on Apple platforms, which provide object-oriented, RFC-compliant URL parsing. Could we make mbgl::util::mapbox depend on a similarly robust, well-tested URL parsing library for C++?

/cc @kkaefer @jfirebaugh

@1ec5 1ec5 added refactor Core The cross-platform C++ core, aka mbgl labels Aug 26, 2016
@jfirebaugh
Copy link
Contributor

I'd love to use a well-tested URL parsing library for C++, providing it meets other quality metrics. The uri class from cpp-netlib seems to be the leading contender. I can't find any official API reference for it, but it was the basis for a standards proposal and there is standalone code floating about.

It looks promising, but I also see some warning signs. We'd probably need to devote some time to trying it out and seeing if it meets our quality bar, or whether we'd need to write something from scratch.

@1ec5
Copy link
Contributor Author

1ec5 commented Aug 29, 2016

I wonder how much trouble it would be to extract WebCore::URLParser from the WebKit project. It depends on WTF for a StringBuilder class, but that doesn’t seem like a showstopper.

Edit: Never mind, that particular implementation looks very new. But maybe one of the browser projects has something ready-made.

@springmeyer
Copy link
Contributor

Does not appear maintained, so not a viable contender (https://github.com/bnoordhuis/uriparser2), but dropping here nonetheless for reference.

@jingsam
Copy link
Contributor

jingsam commented Sep 27, 2016

How about http-parser. It has a function called http_parser_parse_url. Since it is used by Nodejs, so we can keep consistency with mapbox-gl-js in URL handling

@kkaefer
Copy link
Contributor

kkaefer commented Sep 27, 2016

Node.js' url module parsing functionality is done in https://github.com/nodejs/node/blob/master/lib/url.js, and mapbox-gl-js doesn't use Node.js at all since it's typically run in the browser. That being said, we could look at porting the JavaScript code in Node.js to C++.

As far as the scope of this goes: Do we need Punycode parsing/normalization? IPv6 IP support? Auth (https://foo:bar@example.com) handling?

@jingsam
Copy link
Contributor

jingsam commented Sep 27, 2016

Another thing I was worried about is an object-oriented, RFC-compliant URL parsing library would be too heavy for Mapbox GL Native. Benchmarks would also affected. So I would rather suggest to keep it simple.

@kkaefer I do not think we need these Punycode staff features until some use cases come out.

@jfirebaugh
Copy link
Contributor

Good here with #7464.

@kkaefer
Copy link
Contributor

kkaefer commented Sep 10, 2018

https://daniel.haxx.se/blog/2018/09/09/libcurl-gets-a-url-api/

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl refactor
Projects
None yet
Development

No branches or pull requests

5 participants