servo: Merge #18886 - Change AttrValue::Url to AttrValue::ResolvedUrl (from servo:urls); r=SimonSapin

Things make more sense like this.

Source-Repo: https://github.com/servo/servo
Source-Revision: 7e8def9c080faf5a1b8e40fe90747be5439fc3f1

--HG--
extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear
extra : subtree_revision : 7ff8faba53e5e1d6b3276d509662ff46dfe11665

servo/components/script/dom/element.rs

@@ -1306,26 +1306,13 @@ impl Element {
             Some(attr) => attr,
             None => return DOMString::new(),
         };
-        let value = attr.value();
-        match *value {
-            AttrValue::Url(ref value, _) => {
-                // XXXManishearth this doesn't handle `javascript:` urls properly
-                let base = document_from_node(self).base_url();
-                let value = base.join(value)
-                    .map(|parsed| parsed.into_string())
-                    .unwrap_or_else(|_| value.clone());
-                DOMString::from(value)
-            },
-            _ => panic!("attribute value should be AttrValue::Url(..)"),
-        }
-    }
-
-    pub fn set_url_attribute(&self, local_name: &LocalName, value: DOMString) {
-        let value = AttrValue::from_url(
-            document_from_node(self).base_url(),
-            value.into(),
-        );
-        self.set_attribute(local_name, value);
+        let value = &**attr.value();
+        // XXXManishearth this doesn't handle `javascript:` urls properly
+        let base = document_from_node(self).base_url();
+        let value = base.join(value)
+            .map(|parsed| parsed.into_string())
+            .unwrap_or_else(|_| value.to_owned());
+        DOMString::from(value)
     }
 
     pub fn get_string_attribute(&self, local_name: &LocalName) -> DOMString {
@@ -1334,6 +1321,7 @@ impl Element {
             None => DOMString::new(),
         }
     }
+
     pub fn set_string_attribute(&self, local_name: &LocalName, value: DOMString) {
         assert!(*local_name == local_name.to_ascii_lowercase());
         self.set_attribute(local_name, AttrValue::String(value.into()));

servo/components/script/dom/htmlbodyelement.rs

@@ -4,7 +4,6 @@
 
 use cssparser::RGBA;
 use dom::attr::Attr;
-use dom::bindings::codegen::Bindings::AttrBinding::AttrBinding::AttrMethods;
 use dom::bindings::codegen::Bindings::HTMLBodyElementBinding::{self, HTMLBodyElementMethods};
 use dom::bindings::codegen::Bindings::WindowBinding::WindowMethods;
 use dom::bindings::inheritance::Castable;
@@ -74,15 +73,16 @@ impl HTMLBodyElementMethods for HTMLBodyElement {
     make_legacy_color_setter!(SetText, "text");
 
     // https://html.spec.whatwg.org/multipage/#dom-body-background
-    fn Background(&self) -> DOMString {
-        self.upcast::<Element>()
-            .get_attribute(&ns!(), &local_name!("background"))
-            .map(|attr| attr.Value())
-            .unwrap_or_default()
-    }
+    make_getter!(Background, "background");
 
     // https://html.spec.whatwg.org/multipage/#dom-body-background
-    make_url_setter!(SetBackground, "background");
+    fn SetBackground(&self, input: DOMString) {
+        let value = AttrValue::from_resolved_url(
+            &document_from_node(self).base_url(),
+            input.into(),
+        );
+        self.upcast::<Element>().set_attribute(&local_name!("background"), value);
+    }
 
     // https://html.spec.whatwg.org/multipage/#windoweventhandlers
     window_event_handlers!(ForwardToWindow);
@@ -120,7 +120,7 @@ impl HTMLBodyElementLayoutHelpers for LayoutDom<HTMLBodyElement> {
         unsafe {
             (*self.upcast::<Element>().unsafe_get())
                 .get_attr_for_layout(&ns!(), &local_name!("background"))
-                .and_then(AttrValue::as_url)
+                .and_then(AttrValue::as_resolved_url)
                 .cloned()
         }
     }
@@ -160,7 +160,10 @@ impl VirtualMethods for HTMLBodyElement {
             local_name!("bgcolor") |
             local_name!("text") => AttrValue::from_legacy_color(value.into()),
             local_name!("background") => {
-                AttrValue::from_url(document_from_node(self).base_url(), value.into())
+                AttrValue::from_resolved_url(
+                    &document_from_node(self).base_url(),
+                    value.into(),
+                )
             },
             _ => self.super_type().unwrap().parse_plain_attribute(name, value),
         }

servo/components/script/dom/htmliframeelement.rs

@@ -571,7 +571,7 @@ impl HTMLIFrameElementMethods for HTMLIFrameElement {
     make_url_getter!(Src, "src");
 
     // https://html.spec.whatwg.org/multipage/#dom-iframe-src
-    make_url_setter!(SetSrc, "src");
+    make_setter!(SetSrc, "src");
 
     // https://html.spec.whatwg.org/multipage/#dom-iframe-sandbox
     fn Sandbox(&self) -> DomRoot<DOMTokenList> {
@@ -761,7 +761,6 @@ impl VirtualMethods for HTMLIFrameElement {
 
     fn parse_plain_attribute(&self, name: &LocalName, value: DOMString) -> AttrValue {
         match name {
-            &local_name!("src") => AttrValue::from_url(document_from_node(self).base_url(), value.into()),
             &local_name!("sandbox") => AttrValue::from_serialized_tokenlist(value.into()),
             &local_name!("width") => AttrValue::from_dimension(value.into()),
             &local_name!("height") => AttrValue::from_dimension(value.into()),

servo/components/script/dom/htmlimageelement.rs

@@ -826,7 +826,7 @@ impl HTMLImageElementMethods for HTMLImageElement {
     make_url_getter!(Src, "src");
 
     // https://html.spec.whatwg.org/multipage/#dom-img-src
-    make_url_setter!(SetSrc, "src");
+    make_setter!(SetSrc, "src");
 
     // https://html.spec.whatwg.org/multipage/#dom-img-crossOrigin
     fn GetCrossOrigin(&self) -> Option<DOMString> {
@@ -981,7 +981,6 @@ impl VirtualMethods for HTMLImageElement {
 
     fn parse_plain_attribute(&self, name: &LocalName, value: DOMString) -> AttrValue {
         match name {
-            &local_name!("src") => AttrValue::from_url(document_from_node(self).base_url(), value.into()),
             &local_name!("name") => AttrValue::from_atomic(value.into()),
             &local_name!("width") | &local_name!("height") => AttrValue::from_dimension(value.into()),
             &local_name!("hspace") | &local_name!("vspace") => AttrValue::from_u32(value.into(), 0),

servo/components/script/dom/htmlinputelement.rs

@@ -530,7 +530,7 @@ impl HTMLInputElementMethods for HTMLInputElement {
     make_url_getter!(Src, "src");
 
     // https://html.spec.whatwg.org/multipage/#dom-input-src
-    make_url_setter!(SetSrc, "src");
+    make_setter!(SetSrc, "src");
 
     // https://html.spec.whatwg.org/multipage/#dom-input-step
     make_getter!(Step, "step");
@@ -1056,7 +1056,6 @@ impl VirtualMethods for HTMLInputElement {
 
     fn parse_plain_attribute(&self, name: &LocalName, value: DOMString) -> AttrValue {
         match name {
-            &local_name!("src") => AttrValue::from_url(document_from_node(self).base_url(), value.into()),
             &local_name!("accept") => AttrValue::from_comma_separated_tokenlist(value.into()),
             &local_name!("name") => AttrValue::from_atomic(value.into()),
             &local_name!("size") => AttrValue::from_limited_u32(value.into(), DEFAULT_INPUT_SIZE),

servo/components/script/dom/htmllinkelement.rs

@@ -208,7 +208,6 @@ impl VirtualMethods for HTMLLinkElement {
 
     fn parse_plain_attribute(&self, name: &LocalName, value: DOMString) -> AttrValue {
         match name {
-            &local_name!("href") => AttrValue::from_url(document_from_node(self).base_url(), value.into()),
             &local_name!("rel") => AttrValue::from_serialized_tokenlist(value.into()),
             _ => self.super_type().unwrap().parse_plain_attribute(name, value),
         }
@@ -374,7 +373,7 @@ impl HTMLLinkElementMethods for HTMLLinkElement {
     make_url_getter!(Href, "href");
 
     // https://html.spec.whatwg.org/multipage/#dom-link-href
-    make_url_setter!(SetHref, "href");
+    make_setter!(SetHref, "href");
 
     // https://html.spec.whatwg.org/multipage/#dom-link-rel
     make_getter!(Rel, "rel");

servo/components/script/dom/htmlmediaelement.rs

@@ -46,7 +46,6 @@ use std::collections::VecDeque;
 use std::mem;
 use std::rc::Rc;
 use std::sync::{Arc, Mutex};
-use style::attr::AttrValue;
 use task_source::TaskSource;
 use time::{self, Timespec, Duration};
 
@@ -840,7 +839,7 @@ impl HTMLMediaElementMethods for HTMLMediaElement {
     make_url_getter!(Src, "src");
 
     // https://html.spec.whatwg.org/multipage/#dom-media-src
-    make_url_setter!(SetSrc, "src");
+    make_setter!(SetSrc, "src");
 
     // https://html.spec.whatwg.org/multipage/#dom-media-srcobject
     fn GetSrcObject(&self) -> Option<DomRoot<Blob>> {
@@ -915,13 +914,6 @@ impl VirtualMethods for HTMLMediaElement {
         Some(self.upcast::<HTMLElement>() as &VirtualMethods)
     }
 
-    fn parse_plain_attribute(&self, name: &LocalName, value: DOMString) -> AttrValue {
-        match name {
-            &local_name!("src") => AttrValue::from_url(document_from_node(self).base_url(), value.into()),
-            _ => self.super_type().unwrap().parse_plain_attribute(name, value),
-        }
-    }
-
     fn attribute_mutated(&self, attr: &Attr, mutation: AttributeMutation) {
         self.super_type().unwrap().attribute_mutated(attr, mutation);
 

servo/components/script/dom/htmlscriptelement.rs

@@ -42,7 +42,6 @@ use std::io::{Read, Write};
 use std::path::PathBuf;
 use std::process::{Command, Stdio};
 use std::sync::{Arc, Mutex};
-use style::attr::AttrValue;
 use style::str::{HTML_SPACE_CHARACTERS, StaticStringVec};
 use uuid::Uuid;
 
@@ -654,13 +653,6 @@ impl VirtualMethods for HTMLScriptElement {
         Some(self.upcast::<HTMLElement>() as &VirtualMethods)
     }
 
-    fn parse_plain_attribute(&self, name: &LocalName, value: DOMString) -> AttrValue {
-        match name {
-            &local_name!("src") => AttrValue::from_url(document_from_node(self).base_url(), value.into()),
-            _ => self.super_type().unwrap().parse_plain_attribute(name, value),
-        }
-    }
-
     fn attribute_mutated(&self, attr: &Attr, mutation: AttributeMutation) {
         self.super_type().unwrap().attribute_mutated(attr, mutation);
         match *attr.local_name() {
@@ -712,7 +704,7 @@ impl HTMLScriptElementMethods for HTMLScriptElement {
     make_url_getter!(Src, "src");
 
     // https://html.spec.whatwg.org/multipage/#dom-script-src
-    make_url_setter!(SetSrc, "src");
+    make_setter!(SetSrc, "src");
 
     // https://html.spec.whatwg.org/multipage/#dom-script-type
     make_getter!(Type, "type");

servo/components/style/attr.rs

@@ -46,7 +46,12 @@ pub enum AttrValue {
     Length(String, Option<Length>),
     Color(String, Option<RGBA>),
     Dimension(String, LengthOrPercentageOrAuto),
-    Url(String, Option<ServoUrl>),
+
+    /// Stores a URL, computed from the input string and a document's base URL.
+    ///
+    /// The URL is resolved at setting-time, so this kind of attribute value is
+    /// not actually suitable for most URL-reflecting IDL attributes.
+    ResolvedUrl(String, Option<ServoUrl>),
 
     /// Note that this variant is only used transitively as a fast path to set
     /// the property declaration block relevant to the style of an element when
@@ -227,9 +232,9 @@ impl AttrValue {
         AttrValue::Atom(value)
     }
 
-    pub fn from_url(base: ServoUrl, url: String) -> AttrValue {
+    pub fn from_resolved_url(base: &ServoUrl, url: String) -> AttrValue {
         let joined = base.join(&url).ok();
-        AttrValue::Url(url, joined)
+        AttrValue::ResolvedUrl(url, joined)
     }
 
     pub fn from_legacy_color(string: String) -> AttrValue {
@@ -307,14 +312,14 @@ impl AttrValue {
         }
     }
 
-    /// Assumes the `AttrValue` is a `Url` and returns its value
+    /// Assumes the `AttrValue` is a `ResolvedUrl` and returns its value.
     ///
     /// ## Panics
     ///
-    /// Panics if the `AttrValue` is not a `Url`
-    pub fn as_url(&self) -> Option<&ServoUrl> {
+    /// Panics if the `AttrValue` is not a `ResolvedUrl`
+    pub fn as_resolved_url(&self) -> Option<&ServoUrl> {
         match *self {
-            AttrValue::Url(_, ref url) => url.as_ref(),
+            AttrValue::ResolvedUrl(_, ref url) => url.as_ref(),
             _ => panic!("Url not found"),
         }
     }
@@ -365,15 +370,15 @@ impl ::std::ops::Deref for AttrValue {
     fn deref(&self) -> &str {
         match *self {
             AttrValue::String(ref value) |
-                AttrValue::TokenList(ref value, _) |
-                AttrValue::UInt(ref value, _) |
-                AttrValue::Double(ref value, _) |
-                AttrValue::Length(ref value, _) |
-                AttrValue::Color(ref value, _) |
-                AttrValue::Int(ref value, _) |
-                AttrValue::Url(ref value, _) |
-                AttrValue::Declaration(ref value, _) |
-                AttrValue::Dimension(ref value, _) => &value,
+            AttrValue::TokenList(ref value, _) |
+            AttrValue::UInt(ref value, _) |
+            AttrValue::Double(ref value, _) |
+            AttrValue::Length(ref value, _) |
+            AttrValue::Color(ref value, _) |
+            AttrValue::Int(ref value, _) |
+            AttrValue::ResolvedUrl(ref value, _) |
+            AttrValue::Declaration(ref value, _) |
+            AttrValue::Dimension(ref value, _) => &value,
             AttrValue::Atom(ref value) => &value,
         }
     }