Urlmageddon

[emilio]

Still needs a bunch of code in net to be converted in order to get more
advantage of this for images and stuff, but meanwhile this should help quite a
bit with #13778.

Still wanted to get this in.

r? @SimonSapin

---

This change is 

[highfive]

Heads up! This PR modifies the following files:

• @bholley: components/style/values/specified/url.rs, components/style/values/specified/image.rs
• @fitzgen: components/script/dom/element.rs, components/script/dom/htmlimageelement.rs, components/script/dom/htmlcanvaselement.rs
• @KiChjang: components/net/Cargo.toml, components/net/lib.rs, components/script/dom/element.rs, components/net_traits/lib.rs, components/net_traits/lib.rs, components/script/dom/htmlimageelement.rs, components/script/dom/htmlcanvaselement.rs, components/net_traits/Cargo.toml, components/net_traits/Cargo.toml, components/net_traits/image_cache_thread.rs, components/net_traits/image_cache_thread.rs, components/net/image_cache_thread.rs

[highfive]

Warning

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

[emilio]

This is just so transition is easier, and needs to be removed afterwards.

[SimonSapin]

While you’re at it, is replacing every use of url::Url at once with sed much harder? ServoUrl could be made to have some APIs compatible with url::Url (like parse, join, …) to ease the transition.

---

Reviewed 17 of 20 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

components/net/image_cache_thread.rs, line 540 at r2 (raw file):

                        // TODO(emilio): ServoUrl in more places please!
                        let request = RequestInit {
                            url: Url::parse(url.as_str()).unwrap(),

This (and a couple lines below) needlessly goes through rust-url’s parser even when the ServoUrl value is a non-data URL. This should instead use a conversion method on ServoUrl, even if that method is removed eventually.

---

components/style/values/specified/url.rs, line 91 at r2 (raw file):

        let serialization = Arc::new(url.into_owned());
        let resolved = if serialization.starts_with("data:") {

This if / else block should be in a method or constructor of ServoUrl. Perhaps mimicking url::Url’s API: a parse constructor that doesn’t use a base URL, and a join method that does.

---

components/style_traits/Cargo.toml, line 21 at r2 (raw file):

cssparser = "0.7"
euclid = "0.10.1"
url = "1.2"

This likely needs a ./mach cargo-update -p style_traits run to update all lock files.

---

components/style_traits/lib.rs, line 36 at r2 (raw file):

/// afterwards.
///
/// FIXME: Better name for this appreciated.

I think it’s OK to call this Url. Types are namespaced in modules and crates, let’s take advantage of that. (Only a few modules will need to rename one of the two types in order to import both.)

---

Comments from Reviewable

[bors-servo]

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

[emilio]

@bors-servo try

I was really dubious on the data thing after seeing some use cases, so I decided to defer it for after the landing on this with more discussion, because I think, for example, we should still percent-decode, probably when serializing too. So I'd rather land this (a wrapper for an Arc<Url> with the proper interface we need (and indeed I was using until I found that out), and then do the switch to enum { Data(..), Url(..) }.

But I think this is mostly ready for review (given tests pass), and I'd appreciate a fast review given how prone it is to bitrot.

[bors-servo]

⌛ Trying commit e842117 with merge 711326f...

[bors-servo]

💔 Test failed - arm64

[emilio]

@bors-servo try

• Forgot to add some files.

[bors-servo]

⌛ Trying commit 4401845 with merge 915a63b...

[bors-servo]

💔 Test failed - mac-rel-wpt2

[bors-servo]

📌 Commit 913c874 has been approved by SimonSapin

[bors-servo]

⌛ Testing commit 913c874 with merge 2312997...

[bors-servo]

💔 Test failed - linux-rel-wpt

[highfive]

  ▶ FAIL [expected PASS] /_mozilla/css/incremental_trailing_whitespace_a.html
  └   → /_mozilla/css/incremental_trailing_whitespace_a.html da54041086f2975f8e3c776d2283ad6609e6862a
/_mozilla/css/incremental_trailing_whitespace_ref.html f5d0147c8bddbdc772a7e5e86c7c9e433fcd486b
Testing da54041086f2975f8e3c776d2283ad6609e6862a == f5d0147c8bddbdc772a7e5e86c7c9e433fcd486b

[KiChjang]

@bors-servo retry

• #10473

[bors-servo]

⌛ Testing commit 913c874 with merge 22aebdf...

[bors-servo]

💔 Test failed - linux-rel-wpt

[highfive]

  ▶ FAIL [expected PASS] /_mozilla/css/incremental_trailing_whitespace_a.html
  └   → /_mozilla/css/incremental_trailing_whitespace_a.html da54041086f2975f8e3c776d2283ad6609e6862a
/_mozilla/css/incremental_trailing_whitespace_ref.html f5d0147c8bddbdc772a7e5e86c7c9e433fcd486b
Testing da54041086f2975f8e3c776d2283ad6609e6862a == f5d0147c8bddbdc772a7e5e86c7c9e433fcd486b

[KiChjang]

@bors-servo retry force

[bors-servo]

⚡ Previous build results for arm32, arm64, linux-dev, mac-dev-unit, windows-dev are reusable. Rebuilding only linux-rel-css, linux-rel-wpt, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2...

[bors-servo]

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev