Bug 1461858 part 1 - Make creating CssUrl infallible. r?emilio draft
authorXidorn Quan <me@upsuper.org>
Wed, 16 May 2018 12:19:31 +1000
changeset 795571 3a2abb22875a9d5f9c6259713c36815453248bb2
parent 795565 e75753c0c5b93da841fbed25e322b83f2b94cd42
child 795572 f7792299454862a1329fcc01be8a434bd7c3cab6
push id110018
push userxquan@mozilla.com
push dateWed, 16 May 2018 05:40:41 +0000
reviewersemilio
bugs1461858, 16241
milestone62.0a1
Bug 1461858 part 1 - Make creating CssUrl infallible. r?emilio There were a check in CssUrl::parse_from_string for extra data, which was removed as part of servo/servo#16241, so it never fails now. CssUrl::from_url_value_data doesn't seem to need Result from the very beginning. It is unclear why it was made that way. MozReview-Commit-ID: LXzKlZ6wPYW
servo/components/style/gecko/conversions.rs
servo/components/style/gecko/url.rs
servo/components/style/properties/gecko.mako.rs
servo/components/style/servo/url.rs
servo/components/style/stylesheets/rule_parser.rs
servo/components/style/values/specified/image.rs
servo/ports/geckolib/glue.rs
--- a/servo/components/style/gecko/conversions.rs
+++ b/servo/components/style/gecko/conversions.rs
@@ -424,17 +424,16 @@ impl nsStyleImage {
             },
             _ => panic!("Unexpected image type"),
         }
     }
 
     unsafe fn get_image_url(self: &nsStyleImage) -> ComputedImageUrl {
         let url_value = bindings::Gecko_GetURLValue(self);
         ComputedImageUrl::from_url_value_data(url_value.as_ref().unwrap())
-            .expect("Could not convert to ComputedUrl")
     }
 
     unsafe fn get_gradient(self: &nsStyleImage) -> Box<Gradient> {
         use gecko::values::convert_nscolor_to_rgba;
         use self::structs::NS_STYLE_GRADIENT_SIZE_CLOSEST_CORNER as CLOSEST_CORNER;
         use self::structs::NS_STYLE_GRADIENT_SIZE_CLOSEST_SIDE as CLOSEST_SIDE;
         use self::structs::NS_STYLE_GRADIENT_SIZE_FARTHEST_CORNER as FARTHEST_CORNER;
         use self::structs::NS_STYLE_GRADIENT_SIZE_FARTHEST_SIDE as FARTHEST_SIDE;
@@ -674,17 +673,17 @@ pub mod basic_shape {
     }
 
     impl<'a> From<&'a StyleShapeSource> for ClippingShape {
         fn from(other: &'a StyleShapeSource) -> Self {
             match other.mType {
                 StyleShapeSourceType::URL => unsafe {
                     let shape_image = &*other.mShapeImage.mPtr;
                     let other_url = &(**shape_image.__bindgen_anon_1.mURLValue.as_ref());
-                    let url = ComputedUrl::from_url_value_data(&other_url._base).unwrap();
+                    let url = ComputedUrl::from_url_value_data(&other_url._base);
                     ShapeSource::ImageOrUrl(url)
                 },
                 StyleShapeSourceType::Image => {
                     unreachable!("ClippingShape doesn't support Image!");
                 },
                 _ => other
                     .into_shape_source()
                     .expect("Couldn't convert to StyleSource!"),
--- a/servo/components/style/gecko/url.rs
+++ b/servo/components/style/gecko/url.rs
@@ -33,43 +33,38 @@ pub struct CssUrl {
     /// The URL extra data.
     #[css(skip)]
     pub extra_data: RefPtr<URLExtraData>,
 }
 
 impl CssUrl {
     /// Try to parse a URL from a string value that is a valid CSS token for a
     /// URL.
-    ///
-    /// Returns `Err` in the case that extra_data is incomplete.
-    pub fn parse_from_string<'a>(
-        url: String,
-        context: &ParserContext,
-    ) -> Result<Self, ParseError<'a>> {
-        Ok(CssUrl {
+    pub fn parse_from_string(url: String, context: &ParserContext) -> Self {
+        CssUrl {
             serialization: Arc::new(url),
             extra_data: context.url_data.clone(),
-        })
+        }
     }
 
     /// Returns true if the URL is definitely invalid. We don't eagerly resolve
     /// URLs in gecko, so we just return false here.
     /// use its |resolved| status.
     pub fn is_invalid(&self) -> bool {
         false
     }
 
     /// Convert from URLValueData to SpecifiedUrl.
-    unsafe fn from_url_value_data(url: &URLValueData) -> Result<Self, ()> {
+    unsafe fn from_url_value_data(url: &URLValueData) -> Self {
         let arc_type =
             &url.mString as *const _ as *const RawOffsetArc<String>;
-        Ok(CssUrl {
+        CssUrl {
             serialization: Arc::from_raw_offset((*arc_type).clone()),
             extra_data: url.mExtraData.to_safe(),
-        })
+        }
     }
 
     /// Returns true if this URL looks like a fragment.
     /// See https://drafts.csswg.org/css-values/#local-urls
     pub fn is_fragment(&self) -> bool {
         self.as_str().chars().next().map_or(false, |c| c == '#')
     }
 
@@ -99,17 +94,17 @@ impl CssUrl {
 }
 
 impl Parse for CssUrl {
     fn parse<'i, 't>(
         context: &ParserContext,
         input: &mut Parser<'i, 't>,
     ) -> Result<Self, ParseError<'i>> {
         let url = input.expect_url()?;
-        Self::parse_from_string(url.as_ref().to_owned(), context)
+        Ok(Self::parse_from_string(url.as_ref().to_owned(), context))
     }
 }
 
 impl Eq for CssUrl {}
 
 impl MallocSizeOf for CssUrl {
     fn size_of(&self, _ops: &mut MallocSizeOfOps) -> usize {
         // XXX: measure `serialization` once bug 1397971 lands
@@ -183,21 +178,18 @@ pub struct SpecifiedImageUrl {
     /// Gecko's ImageValue so that we can reuse it while rematching a
     /// property with this specified value.
     #[css(skip)]
     pub image_value: RefPtr<ImageValue>,
 }
 
 impl SpecifiedImageUrl {
     /// Parse a URL from a string value. See SpecifiedUrl::parse_from_string.
-    pub fn parse_from_string<'a>(
-        url: String,
-        context: &ParserContext,
-    ) -> Result<Self, ParseError<'a>> {
-        CssUrl::parse_from_string(url, context).map(Self::from_css_url)
+    pub fn parse_from_string(url: String, context: &ParserContext) -> Self {
+        Self::from_css_url(CssUrl::parse_from_string(url, context))
     }
 
     fn from_css_url(url: CssUrl) -> Self {
         let image_value = unsafe {
             let ptr = bindings::Gecko_ImageValue_Create(url.for_ffi());
             // We do not expect Gecko_ImageValue_Create returns null.
             debug_assert!(!ptr.is_null());
             RefPtr::from_addrefed(ptr)
@@ -291,20 +283,20 @@ impl ToCss for ComputedUrl {
         W: Write
     {
         serialize_computed_url(&self.0.url_value._base, dest)
     }
 }
 
 impl ComputedUrl {
     /// Convert from URLValueData to ComputedUrl.
-    pub unsafe fn from_url_value_data(url: &URLValueData) -> Result<Self, ()> {
-        Ok(ComputedUrl(
-            SpecifiedUrl::from_css_url(CssUrl::from_url_value_data(url)?)
-        ))
+    pub unsafe fn from_url_value_data(url: &URLValueData) -> Self {
+        ComputedUrl(
+            SpecifiedUrl::from_css_url(CssUrl::from_url_value_data(url))
+        )
     }
 }
 
 /// The computed value of a CSS `url()` for image.
 #[derive(Clone, Debug, Eq, MallocSizeOf, PartialEq)]
 pub struct ComputedImageUrl(pub SpecifiedImageUrl);
 
 impl ToCss for ComputedImageUrl {
@@ -313,25 +305,25 @@ impl ToCss for ComputedImageUrl {
         W: Write
     {
         serialize_computed_url(&self.0.image_value._base, dest)
     }
 }
 
 impl ComputedImageUrl {
     /// Convert from URLValueData to SpecifiedUrl.
-    pub unsafe fn from_url_value_data(url: &URLValueData) -> Result<Self, ()> {
-        Ok(ComputedImageUrl(
-            SpecifiedImageUrl::from_css_url(CssUrl::from_url_value_data(url)?)
-        ))
+    pub unsafe fn from_url_value_data(url: &URLValueData) -> Self {
+        ComputedImageUrl(
+            SpecifiedImageUrl::from_css_url(CssUrl::from_url_value_data(url))
+        )
     }
 
     /// Convert from nsStyleImageReques to ComputedImageUrl.
     pub unsafe fn from_image_request(image_request: &nsStyleImageRequest) -> Result<Self, ()> {
         if image_request.mImageValue.mRawPtr.is_null() {
             return Err(());
         }
 
         let image_value = image_request.mImageValue.mRawPtr.as_ref().unwrap();
         let url_value_data = &image_value._base;
-        Self::from_url_value_data(url_value_data)
+        Ok(Self::from_url_value_data(url_value_data))
     }
 }
--- a/servo/components/style/properties/gecko.mako.rs
+++ b/servo/components/style/properties/gecko.mako.rs
@@ -757,17 +757,17 @@ def set_gecko_property(ffi_name, expr):
             nsStyleSVGPaintType::eStyleSVGPaintType_None => SVGPaintKind::None,
             nsStyleSVGPaintType::eStyleSVGPaintType_ContextFill => SVGPaintKind::ContextFill,
             nsStyleSVGPaintType::eStyleSVGPaintType_ContextStroke => SVGPaintKind::ContextStroke,
             nsStyleSVGPaintType::eStyleSVGPaintType_Server => {
                 unsafe {
                     SVGPaintKind::PaintServer(
                         ComputedUrl::from_url_value_data(
                             &(**paint.mPaint.mPaintServer.as_ref())._base
-                        ).unwrap()
+                        )
                     )
                 }
             }
             nsStyleSVGPaintType::eStyleSVGPaintType_Color => {
                 unsafe { SVGPaintKind::Color(convert_nscolor_to_rgba(*paint.mPaint.mColor.as_ref())) }
             }
         };
         SVGPaint {
@@ -966,17 +966,16 @@ def set_gecko_property(ffi_name, expr):
         if self.gecko.${gecko_ffi_name}.mRawPtr.is_null() {
             return UrlOrNone::none()
         }
 
         unsafe {
             let gecko_url_value = &*self.gecko.${gecko_ffi_name}.mRawPtr;
             UrlOrNone::Url(
                 ComputedUrl::from_url_value_data(&gecko_url_value._base)
-                    .expect("${gecko_ffi_name} could not convert to ComputedUrl")
             )
         }
     }
 </%def>
 
 <%
 transform_functions = [
     ("Matrix3D", "matrix3d", ["number"] * 16),
@@ -4550,17 +4549,17 @@ fn static_assert() {
                         Filter::DropShadow(
                             (**filter.__bindgen_anon_1.mDropShadow.as_ref()).mArray[0].to_simple_shadow(),
                         )
                     });
                 },
                 NS_STYLE_FILTER_URL => {
                     filters.push(unsafe {
                         Filter::Url(
-                            ComputedUrl::from_url_value_data(&(**filter.__bindgen_anon_1.mURL.as_ref())._base).unwrap()
+                            ComputedUrl::from_url_value_data(&(**filter.__bindgen_anon_1.mURL.as_ref())._base)
                         )
                     });
                 }
                 _ => {},
             }
         }
         longhands::filter::computed_value::T(filters)
     }
--- a/servo/components/style/servo/url.rs
+++ b/servo/components/style/servo/url.rs
@@ -35,28 +35,24 @@ pub struct CssUrl {
     original: Option<Arc<String>>,
 
     /// The resolved value for the url, if valid.
     resolved: Option<ServoUrl>,
 }
 
 impl CssUrl {
     /// Try to parse a URL from a string value that is a valid CSS token for a
-    /// URL. Never fails - the API is only fallible to be compatible with the
-    /// gecko version.
-    pub fn parse_from_string<'a>(
-        url: String,
-        context: &ParserContext,
-    ) -> Result<Self, ParseError<'a>> {
+    /// URL.
+    pub fn parse_from_string(url: String, context: &ParserContext) -> Self {
         let serialization = Arc::new(url);
         let resolved = context.url_data.join(&serialization).ok();
-        Ok(CssUrl {
+        CssUrl {
             original: Some(serialization),
             resolved: resolved,
-        })
+        }
     }
 
     /// Returns true if the URL is definitely invalid. For Servo URLs, we can
     /// use its |resolved| status.
     pub fn is_invalid(&self) -> bool {
         self.resolved.is_none()
     }
 
@@ -105,17 +101,17 @@ impl CssUrl {
 }
 
 impl Parse for CssUrl {
     fn parse<'i, 't>(
         context: &ParserContext,
         input: &mut Parser<'i, 't>,
     ) -> Result<Self, ParseError<'i>> {
         let url = input.expect_url()?;
-        Self::parse_from_string(url.as_ref().to_owned(), context)
+        Ok(Self::parse_from_string(url.as_ref().to_owned(), context))
     }
 }
 
 impl PartialEq for CssUrl {
     fn eq(&self, other: &Self) -> bool {
         // TODO(emilio): maybe we care about equality of the specified values if
         // present? Seems not.
         self.resolved == other.resolved
--- a/servo/components/style/stylesheets/rule_parser.rs
+++ b/servo/components/style/stylesheets/rule_parser.rs
@@ -153,17 +153,17 @@ impl<'a, 'i, R: ParseErrorReporter> AtRu
             "import" => {
                 if self.state > State::Imports {
                     // "@import must be before any rule but @charset"
                     self.had_hierarchy_error = true;
                     return Err(input.new_custom_error(StyleParseErrorKind::UnexpectedImportRule))
                 }
 
                 let url_string = input.expect_url_or_string()?.as_ref().to_owned();
-                let url = CssUrl::parse_from_string(url_string, &self.context)?;
+                let url = CssUrl::parse_from_string(url_string, &self.context);
 
                 let media = parse_media_query_list(&self.context, input,
                                                    self.error_context.error_reporter);
                 let media = Arc::new(self.shared_lock.wrap(media));
 
                 let prelude = AtRuleNonBlockPrelude::Import(url, media, location);
                 return Ok(AtRuleType::WithoutBlock(prelude));
             },
--- a/servo/components/style/values/specified/image.rs
+++ b/servo/components/style/values/specified/image.rs
@@ -985,17 +985,17 @@ impl Parse for PaintWorklet {
 impl Parse for MozImageRect {
     fn parse<'i, 't>(
         context: &ParserContext,
         input: &mut Parser<'i, 't>,
     ) -> Result<Self, ParseError<'i>> {
         input.try(|i| i.expect_function_matching("-moz-image-rect"))?;
         input.parse_nested_block(|i| {
             let string = i.expect_url_or_string()?;
-            let url = SpecifiedImageUrl::parse_from_string(string.as_ref().to_owned(), context)?;
+            let url = SpecifiedImageUrl::parse_from_string(string.as_ref().to_owned(), context);
             i.expect_comma()?;
             let top = NumberOrPercentage::parse_non_negative(context, i)?;
             i.expect_comma()?;
             let right = NumberOrPercentage::parse_non_negative(context, i)?;
             i.expect_comma()?;
             let bottom = NumberOrPercentage::parse_non_negative(context, i)?;
             i.expect_comma()?;
             let left = NumberOrPercentage::parse_non_negative(context, i)?;
--- a/servo/ports/geckolib/glue.rs
+++ b/servo/ports/geckolib/glue.rs
@@ -4166,24 +4166,23 @@ pub extern "C" fn Servo_DeclarationBlock
     let string = unsafe { (*value).to_string() };
     let context = ParserContext::new(
         Origin::Author,
         url_data,
         Some(CssRuleType::Style),
         ParsingMode::DEFAULT,
         QuirksMode::NoQuirks,
     );
-    if let Ok(url) = SpecifiedImageUrl::parse_from_string(string.into(), &context) {
-        let decl = PropertyDeclaration::BackgroundImage(BackgroundImage(
-            vec![Either::Second(Image::Url(url))]
-        ));
-        write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
-            decls.push(decl, Importance::Normal, DeclarationSource::CssOm);
-        })
-    }
+    let url = SpecifiedImageUrl::parse_from_string(string.into(), &context);
+    let decl = PropertyDeclaration::BackgroundImage(BackgroundImage(
+        vec![Either::Second(Image::Url(url))]
+    ));
+    write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
+        decls.push(decl, Importance::Normal, DeclarationSource::CssOm);
+    });
 }
 
 #[no_mangle]
 pub extern "C" fn Servo_DeclarationBlock_SetTextDecorationColorOverride(
     declarations: RawServoDeclarationBlockBorrowed,
 ) {
     use style::properties::PropertyDeclaration;
     use style::values::specified::text::TextDecorationLine;