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

Implement HTMLHyperlinkElementUtils for HTMLAnchorElement #7964

Conversation

@jdramani
Copy link
Contributor

jdramani commented Oct 11, 2015

Fixes #7857

Review on Reviewable

@jdm
Copy link
Member

jdm commented Oct 11, 2015

r? @nox

@nox
Copy link
Member

nox commented Oct 11, 2015

-S-awaiting-review +S-needs-code-changes

You use a get_url() method on HTMLAnchorElement but it's not defined anywhere.


Review status: 0 of 2 files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from the review on Reviewable.io

@jdramani
Copy link
Contributor Author

jdramani commented Oct 12, 2015

Should I include url : Url in HTMLAnchorElement structure and use it as UrlHelper::Host(&self.url) ?

@nox
Copy link
Member

nox commented Oct 13, 2015

@jdm As I said earlier, the URL specification is quite changing these days, URLUtils is gone etc.

@jdramani You should do what is needed to follow the spec, an HTMLAnchorElement may not have an URL at all.

@nox nox added the A-content/dom label Oct 13, 2015
@nox
Copy link
Member

nox commented Oct 14, 2015

This should wait for #8008 to land.

@nox
Copy link
Member

nox commented Oct 20, 2015

#8008 landed.

@bors-servo
Copy link
Contributor

bors-servo commented Oct 20, 2015

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

@jdramani
Copy link
Contributor Author

jdramani commented Oct 23, 2015

Now because of the following change in HTMLAnchorElement.webidl

-//HTMLAnchorElement implements URLUtils;
+//HTMLAnchorElement implements HTMLHyperlinkElementUtils;

We should implement HTMLHyperlinkElementUtils for HTMLAnchorEement , right?

@nox
Copy link
Member

nox commented Oct 23, 2015

@jdramani Yes!

@nox
Copy link
Member

nox commented Nov 17, 2015

@jdramani Ping?

@jdramani
Copy link
Contributor Author

jdramani commented Nov 18, 2015

I am sry. I am stuck in my work but i would like to do it in the next week if there is no urgency.

@jdm
Copy link
Member

jdm commented Nov 18, 2015

That's fine!

@frewsxcv
Copy link
Member

frewsxcv commented Nov 26, 2015

The spec changed for URLUtils: whatwg/html#219

@nox
Copy link
Member

nox commented Dec 13, 2015

@jdramani Ping?

@jdramani jdramani force-pushed the jdramani:Implement_URLUtils_for_HTMLAnchorElement branch from 32af32e to 22312b9 Dec 14, 2015
@frewsxcv
Copy link
Member

frewsxcv commented Dec 14, 2015

components/script/dom/htmlanchorelement.rs, line 29 [r2] (raw file):
This import should be on the line above this so it appears before UrlParser (for alphabetic reasons)


Comments from the review on Reviewable.io

@frewsxcv
Copy link
Member

frewsxcv commented Dec 14, 2015

components/script/dom/htmlanchorelement.rs, line 114 [r2] (raw file):
Where did the spec links go for all these methods?


Comments from the review on Reviewable.io

@frewsxcv
Copy link
Member

frewsxcv commented Dec 14, 2015

components/script/dom/webidls/HTMLHyperlinkElementUtils.webidl, line 9 [r2] (raw file):
Comment out this line and add attribute USVString href; on the following line. We don't support stringifier attribute yet #7590


Comments from the review on Reviewable.io

@jdramani
Copy link
Contributor Author

jdramani commented Dec 14, 2015

@frewsxcv
Copy link
Member

frewsxcv commented Dec 14, 2015

@jdramani jdramani changed the title Implement URLUtils for HTMLAnchorElement Implement HTMLHyperlinkElementUtils for HTMLAnchorElement Dec 14, 2015
@frewsxcv
Copy link
Member

frewsxcv commented Dec 14, 2015

This currently doesn't compile: https://travis-ci.org/servo/servo/jobs/96659745

@@ -5,16 +5,17 @@

// https://html.spec.whatwg.org/multipage/#htmlhyperlinkelementutils
//[NoInterfaceObject/*, Exposed=Window*/]

This comment has been minimized.

@frewsxcv

frewsxcv Dec 14, 2015

Member

I think you need to uncomment this line partially so it becomes:

[NoInterfaceObject/*, Exposed=Window*/]

Because right now, it's looking in components/script/dom/htmlhyperlinkelementutils.rs for a concrete DOM structure, which shouldn't exist since this interface is marked as NoInterfaceObject

@frewsxcv
Copy link
Member

frewsxcv commented Dec 14, 2015

Use the same links found here: https://github.com/servo/servo/blob/master/components/script/dom/url.rs#L90-L203

Blah, I'm sorry, I was wrong in this comment. You're going to want to link to the items found here.

For example, for the Protocol method, link to this

@jdramani jdramani force-pushed the jdramani:Implement_URLUtils_for_HTMLAnchorElement branch from 741efcf to c3f55bd Dec 15, 2015
@@ -108,6 +110,101 @@ impl HTMLAnchorElementMethods for HTMLAnchorElement {

// https://html.spec.whatwg.org/multipage/#dom-a-shape
make_setter!(SetShape, "shape");

// https://html.spec.whatwg.org/multipage/semantics.html#dom-hyperlink-hash

This comment has been minimized.

@frewsxcv

frewsxcv Dec 16, 2015

Member

This should be https://html.spec.whatwg.org/multipage/#dom-hyperlink-hash

You should run ./mach test-tidy to see the errors that are occurring

@KiChjang
Copy link
Member

KiChjang commented Dec 29, 2015

@jdramani do you need help finishing this PR?

@jdramani
Copy link
Contributor Author

jdramani commented Dec 29, 2015

Let me give it a final try and then i will let you know. Sry for the delay.

On Mon, Dec 28, 2015 at 7:08 PM, Keith Yeung notifications@github.com
wrote:

@jdramani https://github.com/jdramani do you need help finishing this
PR?


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

@jdramani jdramani force-pushed the jdramani:Implement_URLUtils_for_HTMLAnchorElement branch from c3f55bd to fc2dfe3 Jan 4, 2016
@Ms2ger
Copy link
Contributor

Ms2ger commented Jan 13, 2016

Thanks for your work! I tried to finish it, but didn't quite get everything done. PR at #9292.

@Ms2ger Ms2ger closed this Jan 13, 2016
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.

None yet

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