Bug 1461858 part 1 - Make creating CssUrl infallible. r=emilio
authorXidorn Quan <me@upsuper.org>
Wed, 16 May 2018 12:19:31 +1000
changeset 472697 7358416f54f4676032ff99747b65630aefb398b9
parent 472696 e3c0dd79cbfc5c57e65aa0f671a8c3cf446b4dfe
child 472698 c728ab64fa12b8f4c84bbeea304da7351e6e6e97
push id9374
push userjlund@mozilla.com
push dateMon, 18 Jun 2018 21:43:20 +0000
treeherdermozilla-beta@160e085dfb0b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersemilio
bugs1461858, 16241
milestone62.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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;