Issue #10284 Ignore alternate stylesheets

[mylainos]

This change is

[highfive]

Heads up! This PR modifies the following files:

• @jgraham: tests/wpt/metadata/MANIFEST.json, tests/wpt/web-platform-tests/html/semantics/links/linktypes/alternate-css-ref.html, tests/wpt/web-platform-tests/html/semantics/links/linktypes/alternate-css.html, tests/wpt/web-platform-tests/html/semantics/links/linktypes/preferred.css, tests/wpt/web-platform-tests/html/semantics/links/linktypes/alternate.css
• @KiChjang: components/script/dom/htmllinkelement.rs

[KiChjang]

The change to MANIFEST.json looks very wrong; did you use ./mach create-wpt /path/to/test/files to create your test files?

[mylainos]

Yes, I have used ./mach create-wpt --reftest tests/wpt/web-platform-tests/html/semantics/links/linktypes/alternate-css.html --reference tests/wpt/web-platform-tests/html/semantics/links/linktypes/alternate-css-ref.html

[jdm]

The manifest thing is a known issue and won't cause any problems being merged.

[mylainos]

components/script/dom/htmllinkelement.rs, line 93 [r2] (raw file):
It may be better to do that :

let mut substrings = value.split(HTML_SPACE_CHARACTERS);
let mut substrings2 = substrings.clone();
substrings.any(|s| s.eq_ignore_ascii_case("stylesheet"))
&& !substrings2.any(|s| s.eq_ignore_ascii_case("alternate"))

---

Comments from the review on Reviewable.io

[bors-servo]

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

[mylainos]

There is a merge conflict in MANIFEST.json, what should I do ? I have error if I keep my changes or if I keep the new ones.

[jdm]

@mylainos It's usually easiest to keep the new changes and run ./mach test-wpt --manifest-update SKIP_TESTS.

[mylainos]

There is a lot of errors like this one:

  ▶ TIMEOUT [expected OK] /2dcontext/compositing/2d.composite.canvas.destination-atop.html

  ▶ Unexpected subtest result in /2dcontext/compositing/2d.composite.canvas.destination-atop.html:
  └ NOTRUN [expected PASS] Canvas test: 2d.composite.canvas.destination-atop

[jdm]

Oh, that wasn't supposed to end up running any tests. Odd. Anyways, you can ignore those (and feel free to interrupt it in the future if it starts running tests when you're not trying to do so).

[mylainos]

There is no error when I do ./mach test-wpt --manifest-update SKIP_TESTS but when I run the wpt tests there is a lot of errors.

[jdm]

Debug builds in particular are known to have problems with tests in 2dcontext/, so I wouldn't worry about it too much. Running the full testsuite locally usually isn't a great use of your time; it's better to focus on particular tests or directories that are relevant.

[mylainos]

Ok thanks.

[mylainos]

How could I avoid this error The command "bash etc/ci/manifest_changed.sh" exited with 1. ?

[jdm]

That happens when the build machines run ./mach test-wpt --manifest-update SKIP_TESTS and see the MANIFEST.json file get modified. You should include the changes to that file in this PR.

[mylainos]

Done !

[emilio]

Can you squash the commits? They look good to me! :)

[emilio]

-S-awaiting-review

---

Reviewed 4 of 6 files at r1, 1 of 1 files at r2, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

components/script/dom/htmllinkelement.rs, line 93 [r2] (raw file):
It might be a bit more performant this way:

let mut found_stylesheet = false;
for s in value.split(HTML_SPACE_CHARACTERS).iter() {
    match_ignore_ascii_case! { s,
        "stylesheet" => found_stylesheet = true,
        "alternate" => return false, // We don't load alternate stylesheets
         _ => {},
    }
}
found_stylesheet

Though arguably less readable.

In practice, I don't think it matters too much, since I don't think we expect a huge amount of values in a rel attribute, so feel free to either make the change or leave it like it is now.

---

Comments from Reviewable

[mylainos]

I have this error when I test your code error: macro undefined: 'match_ignore_ascii_case!'

[emilio]

Yeah, sorry, it's defined in cssparser and we're on script so it's not on scope. The inner part of the loop is equivalent to:

if s.eq_ignore_ascii_case("alternate") {
    return false;
}

if s.eq_ignore_ascii_case("stylesheet") {
    found_stylesheet = true;
}

[mylainos]

.iter() don't work so i used .into_iter().

[emilio]

Looks great, thanks!

@bors-servo: r+

[bors-servo]

📌 Commit 2564352 has been approved by emilio

[bors-servo]

⌛ Testing commit 2564352 with merge 0d28d21...

[KiChjang]

@bors-servo r-

nit: You can drop the == true here and the one above as well.

[emilio]

Whoops, missed that, @KiChjang is right

[mylainos]

I forgot to remove it. I had a an error when I was using .iter() which said it didn't know the type.

[emilio]

Now there it goes!

@bors-servo: r+

[bors-servo]

📌 Commit c39c753 has been approved by emilio

[bors-servo]

⌛ Testing commit c39c753 with merge 6171188...

[bors-servo]

☀️ Test successful - android, arm32, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, status-appveyor