Skip to content

Changes needed for gecko-integration #201

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

Closed
wants to merge 1 commit into from
Closed

Changes needed for gecko-integration #201

wants to merge 1 commit into from

Conversation

valenting
Copy link
Collaborator

@valenting valenting commented May 27, 2016

This pull request contains 2 features that are needed in the C-API for use in Gecko:

  1. Feature that allows to only percent decode characters that are valid in the hostname.
    This introduces the only_percent_decode_hostname_valid feature.
  2. From for Position in order to use slicing in the C API.

This change is Reviewable

…ostname.

Implement From<u32> for Position to allow use of slicing methods from C API.
@SimonSapin
Copy link
Member

I’ll comment more tomorrow, but why the first change? It deviates from the spec. If it’s necessary for web-compat, the spec should be changed.

@valenting
Copy link
Collaborator Author

I forgot to mention the reasoning behind this. Indeed it deviates from the spec (which is why I made it a feature), but that is needed to support old addons in Gecko, which access their content using a URL such as resource://ADDON_ID/path - where ADDON_ID usually contains a percent encoded @ character.
If we feel this isn't a change we want to support at all, I could use a separate fork for the Gecko stuff.

@SimonSapin
Copy link
Member

Does Gecko’s URL parser for web content have the same behavior? If that’s the case and Gecko is not willing to align with the spec, maybe the spec should be changed. Could you file an issue on https://github.com/whatwg/url/issues/ with the proposed parser changes and this reasoning?

Instead of a compile-time switch it could be a per-URL run-time one: add a method to ParseOptions and use it like: Url::options().only_percent_decode_hostname_valid().parse(some_str). What do you think?

I’ll comment inline on implementation details.

@@ -110,6 +110,14 @@ define_encode_set! {
}
}

define_encode_set! {
/// This encode set is used to decide which characters are allowed in the hostname.
pub HOSTNAME_ENCODE_SET = [SIMPLE_ENCODE_SET] | {
Copy link
Member

Choose a reason for hiding this comment

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

Encode sets are normally used for percent encoding, not decoding. So I’d rather not expose this in the public API. In after_percent_sign you could "inline" !HOSTNAME_ENCODE_SET.contains(c) to !(matches(c, ' ' | '!' | '"' | /* ...*/ '~') || c < '\u{20}' || c > '\u{7E}'), or invert the logic and use matches!(c, 'a'...'z' | 'A'...'Z' | '0'...'9' | '.' | '-' | '_' | '*'). (I think that’s the complementary set, but I’m not sure * should be there.)

@nox
Copy link
Contributor

nox commented May 30, 2016

Why not use an enum from C too for Position?

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #216) made this pull request unmergeable. Please resolve the merge conflicts.

@valenting
Copy link
Collaborator Author

The resource://ADDON_ID issue will be fixed in Firefox, so we shouldn't worry about that here.
I'll probably open another PR for the slicing bits.

@valenting valenting closed this Oct 28, 2016
@nox
Copy link
Contributor

nox commented Oct 29, 2016

The resource://ADDON_ID issue will be fixed in Firefox,

@valenting And you deliver this information on my birthday? You have a good timing sense, thanks. :D

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.

4 participants