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

Correctly gets and sets rel content attributes in linkelement #13311

Merged
merged 1 commit into from Sep 23, 2016

Conversation

Projects
None yet
7 participants
@cynicaldevil
Contributor

cynicaldevil commented Sep 18, 2016


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #12799 .

This change is Reviewable

@highfive

This comment has been minimized.

Show comment
Hide comment
@highfive

highfive Sep 18, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/htmllinkelement.rs

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/htmllinkelement.rs
@highfive

This comment has been minimized.

Show comment
Hide comment
@highfive

highfive Sep 18, 2016

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@KiChjang

We still need a WPT test for this. I believe in the original issue, @jdm has a test case which you can use.
We also still need to change the RelList function and remove the rel_list field on the link element.

Show outdated Hide outdated components/script/dom/htmllinkelement.rs
@@ -357,10 +357,21 @@ impl HTMLLinkElementMethods for HTMLLinkElement {
make_setter!(SetHref, "href");
// https://html.spec.whatwg.org/multipage/#dom-link-rel
make_getter!(Rel, "rel");
fn Rel(&self) -> DOMString {
let atomVec: Vec<Atom> = self.upcast::<Element>().get_tokenlist_attribute(&atom!("rel"));

This comment has been minimized.

@KiChjang

KiChjang Sep 18, 2016

Member

No need for type annotation here and atomVec should really be atom_vec.

@KiChjang

KiChjang Sep 18, 2016

Member

No need for type annotation here and atomVec should really be atom_vec.

Show outdated Hide outdated components/script/dom/htmllinkelement.rs
for data in &atomVec {
let dataStr = String::from(data as &str);
string_concat = string_concat + " " + &dataStr;
}

This comment has been minimized.

@KiChjang

KiChjang Sep 18, 2016

Member

Instead of doing this in imperative style, I think you can do something fancy with iterators:

DOMString::from(atom_vec.iter().map(|a| &*a).join(" "))
@KiChjang

KiChjang Sep 18, 2016

Member

Instead of doing this in imperative style, I think you can do something fancy with iterators:

DOMString::from(atom_vec.iter().map(|a| &*a).join(" "))
@cynicaldevil

This comment has been minimized.

Show comment
Hide comment
@cynicaldevil

cynicaldevil Sep 18, 2016

Contributor

@KiChjang +1 on the test part, but don't you think the RelList thing is different from this issue? IMO it would be better to open a separate PR for that.

Contributor

cynicaldevil commented Sep 18, 2016

@KiChjang +1 on the test part, but don't you think the RelList thing is different from this issue? IMO it would be better to open a separate PR for that.

@KiChjang

This comment has been minimized.

Show comment
Hide comment
@KiChjang

KiChjang Sep 18, 2016

Member

It is not. The issue is to fix the crash that happens when you use that test case, and your current PR doesn't actually fix it yet.

Member

KiChjang commented Sep 18, 2016

It is not. The issue is to fix the crash that happens when you use that test case, and your current PR doesn't actually fix it yet.

@cynicaldevil

This comment has been minimized.

Show comment
Hide comment
@cynicaldevil

cynicaldevil Sep 18, 2016

Contributor

@KiChjang alright.

Contributor

cynicaldevil commented Sep 18, 2016

@KiChjang alright.

@Ms2ger

This comment has been minimized.

Show comment
Hide comment
@Ms2ger

Ms2ger Sep 19, 2016

Contributor

I'm afraid everything @KiChjang said here was wrong. The rel_list field should remain as it is. The Rel function is unfortunately incorrect; it should just call get_string_attribute, so any existing whitespace in the string is returned as-is.

r? @Ms2ger

@bors-servo try

Contributor

Ms2ger commented Sep 19, 2016

I'm afraid everything @KiChjang said here was wrong. The rel_list field should remain as it is. The Rel function is unfortunately incorrect; it should just call get_string_attribute, so any existing whitespace in the string is returned as-is.

r? @Ms2ger

@bors-servo try

@highfive highfive assigned Ms2ger and unassigned KiChjang Sep 19, 2016

bors-servo added a commit that referenced this pull request Sep 19, 2016

Auto merge of #13311 - cynicaldevil:linkelement, r=<try>
Correctly gets and sets rel content attributes in linkelement

<!-- Please describe your changes on the following line: -->
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #12799 .

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13311)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 19, 2016

Contributor

⌛️ Trying commit c7f7e2e with merge 01be8c4...

Contributor

bors-servo commented Sep 19, 2016

⌛️ Trying commit c7f7e2e with merge 01be8c4...

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 19, 2016

Contributor

💔 Test failed - linux-rel

Contributor

bors-servo commented Sep 19, 2016

💔 Test failed - linux-rel

@KiChjang

This comment has been minimized.

Show comment
Hide comment
@KiChjang

KiChjang Sep 19, 2016

Member

@Ms2ger I'm not sure what the point is to keep a rel_list field when the rel content attribute already contains a token list. Also, the spec does say that the relList IDL attribute should reflect the rel content attribute.

Member

KiChjang commented Sep 19, 2016

@Ms2ger I'm not sure what the point is to keep a rel_list field when the rel content attribute already contains a token list. Also, the spec does say that the relList IDL attribute should reflect the rel content attribute.

@Ms2ger

This comment has been minimized.

Show comment
Hide comment
@Ms2ger

Ms2ger Sep 19, 2016

Contributor

@KiChjang you're confusing different types of objects... Please read the code more carefully.

Contributor

Ms2ger commented Sep 19, 2016

@KiChjang you're confusing different types of objects... Please read the code more carefully.

@jdm jdm referenced this pull request Sep 21, 2016

Closed

Tokens not found #13343

@cynicaldevil

This comment has been minimized.

Show comment
Hide comment
@cynicaldevil

cynicaldevil Sep 21, 2016

Contributor

@KiChjang @Ms2ger I'm not sure how I should proceed, should I amend the PR with the minor changes that @KiChjang suggested and then modify the tests so that they pass?

Contributor

cynicaldevil commented Sep 21, 2016

@KiChjang @Ms2ger I'm not sure how I should proceed, should I amend the PR with the minor changes that @KiChjang suggested and then modify the tests so that they pass?

@KiChjang

This comment has been minimized.

Show comment
Hide comment
@KiChjang

KiChjang Sep 21, 2016

Member

No, @msger is correct here, you need to use make_getter! on the Rel function, and then update the tests accordingly.

Member

KiChjang commented Sep 21, 2016

No, @msger is correct here, you need to use make_getter! on the Rel function, and then update the tests accordingly.

@cynicaldevil

This comment has been minimized.

Show comment
Hide comment
@cynicaldevil

cynicaldevil Sep 22, 2016

Contributor

@KiChjang I pushed the changes onto PR branch.

Contributor

cynicaldevil commented Sep 22, 2016

@KiChjang I pushed the changes onto PR branch.

@KiChjang

This comment has been minimized.

Show comment
Hide comment
Member

KiChjang commented Sep 22, 2016

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 22, 2016

Contributor

⌛️ Trying commit 622c826 with merge 9f66514...

Contributor

bors-servo commented Sep 22, 2016

⌛️ Trying commit 622c826 with merge 9f66514...

bors-servo added a commit that referenced this pull request Sep 22, 2016

Auto merge of #13311 - cynicaldevil:linkelement, r=<try>
Correctly gets and sets rel content attributes in linkelement

<!-- Please describe your changes on the following line: -->
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #12799 .

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13311)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 22, 2016

Contributor

💔 Test failed - linux-rel

Contributor

bors-servo commented Sep 22, 2016

💔 Test failed - linux-rel

@highfive

This comment has been minimized.

Show comment
Hide comment
@highfive

highfive Sep 22, 2016

  ▶ OK [expected CRASH] /fetch/nosniff/stylesheet.html

  ▶ OK [expected CRASH] /html/semantics/document-metadata/the-link-element/link-style-error-01.html
  ▶ OK [expected CRASH] /fetch/nosniff/stylesheet.html

  ▶ OK [expected CRASH] /html/semantics/document-metadata/the-link-element/link-style-error-01.html
@cynicaldevil

This comment has been minimized.

Show comment
Hide comment
@cynicaldevil

cynicaldevil Sep 22, 2016

Contributor

@KiChjang Is this a good thing? Because the report is telling that it expected a crash but my PR fixed it...?

Contributor

cynicaldevil commented Sep 22, 2016

@KiChjang Is this a good thing? Because the report is telling that it expected a crash but my PR fixed it...?

@KiChjang

This comment has been minimized.

Show comment
Hide comment
@KiChjang

KiChjang Sep 22, 2016

Member

Indeed, this is pretty good! Great work here!

Member

KiChjang commented Sep 22, 2016

Indeed, this is pretty good! Great work here!

@jdm

This comment has been minimized.

Show comment
Hide comment
@jdm

jdm Sep 22, 2016

Member

They used to crash because of the panic that this PR fixes.

Member

jdm commented Sep 22, 2016

They used to crash because of the panic that this PR fixes.

@KiChjang

This comment has been minimized.

Show comment
Hide comment
@KiChjang

KiChjang Sep 22, 2016

Member

Next step is to change the test expectations and mark those tests as passing.

Member

KiChjang commented Sep 22, 2016

Next step is to change the test expectations and mark those tests as passing.

@cynicaldevil

This comment has been minimized.

Show comment
Hide comment
@cynicaldevil

cynicaldevil Sep 23, 2016

Contributor

If I understood this right, I have to change the respective .ini files to expect a PASS instead of a CRASH. But these files are missing in this branch, while they are present in other branches. What am I missing?

Contributor

cynicaldevil commented Sep 23, 2016

If I understood this right, I have to change the respective .ini files to expect a PASS instead of a CRASH. But these files are missing in this branch, while they are present in other branches. What am I missing?

@Ms2ger

This comment has been minimized.

Show comment
Hide comment
@Ms2ger

Ms2ger Sep 23, 2016

Contributor

The expected crashes were only added in #12493; you'll need to rebase.

Also, you should be able to remove those files; "pass" is the default expectation.

Contributor

Ms2ger commented Sep 23, 2016

The expected crashes were only added in #12493; you'll need to rebase.

Also, you should be able to remove those files; "pass" is the default expectation.

@cynicaldevil

This comment has been minimized.

Show comment
Hide comment
@cynicaldevil

cynicaldevil Sep 23, 2016

Contributor

Thanks, I've updated the PR now.

Contributor

cynicaldevil commented Sep 23, 2016

Thanks, I've updated the PR now.

@Ms2ger

This comment has been minimized.

Show comment
Hide comment
@Ms2ger

Ms2ger Sep 23, 2016

Contributor

Could you just squash the two commits? Should be good to go then.

Contributor

Ms2ger commented Sep 23, 2016

Could you just squash the two commits? Should be good to go then.

@cynicaldevil

This comment has been minimized.

Show comment
Hide comment
@cynicaldevil

cynicaldevil Sep 23, 2016

Contributor

@Ms2ger Done.

Contributor

cynicaldevil commented Sep 23, 2016

@Ms2ger Done.

@Ms2ger

This comment has been minimized.

Show comment
Hide comment
@Ms2ger

Ms2ger Sep 23, 2016

Contributor

Thanks!

@bors-servo r+

Contributor

Ms2ger commented Sep 23, 2016

Thanks!

@bors-servo r+

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 23, 2016

Contributor

📌 Commit 9d378ce has been approved by Ms2ger

Contributor

bors-servo commented Sep 23, 2016

📌 Commit 9d378ce has been approved by Ms2ger

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 23, 2016

Contributor

⌛️ Testing commit 9d378ce with merge f243d98...

Contributor

bors-servo commented Sep 23, 2016

⌛️ Testing commit 9d378ce with merge f243d98...

bors-servo added a commit that referenced this pull request Sep 23, 2016

Auto merge of #13311 - cynicaldevil:linkelement, r=Ms2ger
Correctly gets and sets rel content attributes in linkelement

<!-- Please describe your changes on the following line: -->
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #12799 .

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13311)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Show comment
Hide comment
Contributor

bors-servo commented Sep 23, 2016

@bors-servo bors-servo merged commit 9d378ce into servo:master Sep 23, 2016

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@cynicaldevil cynicaldevil deleted the cynicaldevil:linkelement branch Sep 24, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment