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

Implemented the origin method (fixes #54) #111

Merged
merged 5 commits into from Sep 29, 2015
Merged

Conversation

@KiChjang
Copy link
Member

KiChjang commented May 17, 2015

Fixes #54, but still need some clarafication as to how to generate a GUID for schemes such as blob, file and others.

Review on Reviewable

@KiChjang KiChjang force-pushed the KiChjang:url-origin branch from 0b40f53 to f056ebf May 17, 2015
@Manishearth
Copy link
Member

Manishearth commented May 18, 2015

Are we sure that the port will always be the default one? With a quick look at the code that seems to be the case.

Otherwise lgtm.

Generating guids probably can be done with https://crates.io/crates/uuid.

@KiChjang KiChjang force-pushed the KiChjang:url-origin branch 2 times, most recently from 0d77c13 to b63effc May 18, 2015
@KiChjang
Copy link
Member Author

KiChjang commented May 18, 2015

@Manishearth Where do you see that port always return the default one?

@Manishearth
Copy link
Member

Manishearth commented May 18, 2015

As in the port included in a URL that didn't specifically say one.

@KiChjang
Copy link
Member Author

KiChjang commented May 18, 2015

Good catch, there's a method called port_or_default

@SimonSapin
Copy link
Member

SimonSapin commented May 18, 2015

Generating guids probably can be done with https://crates.io/crates/uuid.

That’s unclear to me. I’ve filed whatwg/url#21 on the spec.

@SimonSapin
Copy link
Member

SimonSapin commented May 22, 2015

I think that "globally unique identifiers" for the purpose of URL origins are not UUIDs, but are the same as "opaque identifier" as defined in HTML: https://html.spec.whatwg.org/multipage/browsers.html#origin

@Manishearth
Copy link
Member

Manishearth commented May 22, 2015

I don't know; Blob URIs use GUIDs I think.

@SimonSapin
Copy link
Member

SimonSapin commented May 22, 2015

@Manishearth [citation needed] :)

@KiChjang
Copy link
Member Author

KiChjang commented May 22, 2015

Hmm... how exactly do we implement opaque identifiers?
On May 22, 2015 6:30 AM, "Simon Sapin" notifications@github.com wrote:

http://krijnhoetmer.nl/irc-logs/whatwg/20150522#l-267


Reply to this email directly or view it on GitHub
#111 (comment).

@SimonSapin
Copy link
Member

SimonSapin commented May 22, 2015

Unclear to me. I’m trying to figure this out:

whatwg/url#21
https://www.w3.org/Bugs/Public/show_bug.cgi?id=28675

@Manishearth
Copy link
Member

Manishearth commented May 22, 2015

Internal values, with no serialisation, for which the only meaningful operation is testing for equality.

Would a wrapped 256-bit hash (or something) do?

@SimonSapin
Copy link
Member

SimonSapin commented May 22, 2015

A hash of what data?

@Manishearth
Copy link
Member

Manishearth commented May 22, 2015

Oh, sorry, I got confused there.

We could just hash random values but we're back to GUIDs.

Really, we can just use uuid or similar to create a number and then wrap the number in an opaque (except for PartialEq/Eq) tuple struct.

@KiChjang
Copy link
Member Author

KiChjang commented May 22, 2015

What exactly is this opaque tuple struct? How does it look like? Does my current implementation suffice?

@Manishearth
Copy link
Member

Manishearth commented May 22, 2015

#[derive(Debug, PartialEq, Eq, Copy)] pub struct OpaqueOrigin(u64);

Inner value is private, thus, opaque.

-----Original Message-----
From: "Keith Yeung" notifications@github.com
Sent: ‎5/‎23/‎2015 2:08 AM
To: "servo/rust-url" rust-url@noreply.github.com
Cc: "Manish Goregaokar" manishsmail@gmail.com
Subject: Re: [rust-url] Implemented the origin method (fixes #54) (#111)

What exactly is this opaque tuple struct? How does it look like? Does my current implementation suffice?

Reply to this email directly or view it on GitHub.

@KiChjang KiChjang force-pushed the KiChjang:url-origin branch 3 times, most recently from 6c0660f to 08ea09c May 22, 2015
@KiChjang
Copy link
Member Author

KiChjang commented Sep 29, 2015

@SimonSapin
Copy link
Member

SimonSapin commented Sep 29, 2015

This still seems very wrong to me, but apparently generating a new random identifier every time the method is called is indeed intentional: whatwg/url#56

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Sep 29, 2015

📌 Commit 08ea09c has been approved by SimonSapin

@bors-servo
Copy link
Contributor

bors-servo commented Sep 29, 2015

Testing commit 08ea09c with merge a3cd772...

bors-servo pushed a commit that referenced this pull request Sep 29, 2015
Implemented the origin method (fixes #54)

Fixes #54, but still need some clarafication as to how to generate a GUID for schemes such as blob, file and others.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/rust-url/111)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 29, 2015

☀️ Test successful - travis

@bors-servo bors-servo merged commit 08ea09c into servo:master Sep 29, 2015
2 checks passed
2 checks passed
continuous-integration/appveyor AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.