Closed Bug 1338388 Opened 8 years ago Closed 8 years ago

stylo: Bindings for css::URLValue and all remaining nsStyleSVG values

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: manishearth, Assigned: manishearth)

References

Details

Attachments

(7 files, 7 obsolete files)

Bug 1338388 - Part 1: stylo: Add ServoBundledURI abstraction for use when setting css::URLValues;
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1338388 - Part 2: stylo: Support URL filters ;
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1338388 - Part 3: stylo: Support URL clip-paths ;
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1338388 - Part 4: stylo: Add Gecko bindings for <paint>, use for stroke/fill;
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1338388 - Part 5: stylo: Add mako template for URLOrNone, use for marker-* properties;
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1338388 - Part 6: stylo: Support stroke-dasharray and stroke-dashoffset;
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1338388 - Part 7: stylo: Update test expectations for svg properties;
59 bytes, text/x-review-board-request
heycam
: review+
Details
Some properties (filter, clip-path, stroke, etc) make use of css::URLValue and/or nsStyleSVGPaint. We currently don't deal with these, and should.
Attachment #8835830 - Attachment is obsolete: true
Still WIP -- this only contains the refactoring and not its use for other SVG props, pushing for early review.
Comment on attachment 8835831 [details] Bug 1338388 - Part 1: stylo: Add ServoBundledURI abstraction for use when setting css::URLValues; https://reviewboard.mozilla.org/r/111394/#review112680 r=me with these things addressed. ::: layout/style/ServoBindings.h:91 (Diff revision 2) > > // Object refcounting. > NS_DECL_HOLDER_FFI_REFCOUNTING(nsIPrincipal, Principal) > NS_DECL_HOLDER_FFI_REFCOUNTING(nsIURI, URI) > > +class ServoBundledURI { Nit: "{" on new line. ::: layout/style/ServoBindings.h:92 (Diff revision 2) > + const uint8_t* mURLString; > + uint32_t mURLStringLength; > + ThreadSafeURIHolder* mBaseURI; > + ThreadSafeURIHolder* mReferrer; > + ThreadSafePrincipalHolder* mPrincipal; (One thing Xidorn wanted to do was to bundle up base/referrer/principal into an object and hold on to it from the document. Then we could just pass a single borrowed pointer to it to Servo, and avoid all the refcounting of these things when we store them in specified Url values etc. So at some point it might make sense to separate these three fields from the URL string/length fields.) ::: layout/style/ServoBindings.h:98 (Diff revision 2) > + uint32_t mURLStringLength; > + ThreadSafeURIHolder* mBaseURI; > + ThreadSafeURIHolder* mReferrer; > + ThreadSafePrincipalHolder* mPrincipal; > + > + mozilla::css::URLValue IntoUrl(); Let's have explicit public: and private: sections, and move IntoUrl into the public: section. Also, did you upload mismatching files? The function is called IntoUrl here, but it's IntoCssUrl in ServoBindings.cpp. ::: layout/style/ServoBindings.cpp:734 (Diff revision 2) > } > > NS_IMPL_HOLDER_FFI_REFCOUNTING(nsIPrincipal, Principal) > NS_IMPL_HOLDER_FFI_REFCOUNTING(nsIURI, URI) > > -void > +css::URLValue* Let's make this return already_AddRefed<css::URLValue>. ::: layout/style/ServoBindings.cpp:758 (Diff revision 2) > } > > void > +Gecko_SetMozBinding(nsStyleDisplay* aDisplay, ServoBundledURI aBundledURI) > +{ > + Nit: remove this blank line. ::: layout/style/ServoBindings.cpp:759 (Diff revision 2) > > void > +Gecko_SetMozBinding(nsStyleDisplay* aDisplay, ServoBundledURI aBundledURI) > +{ > + > + aDisplay->mBinding = aBundledURI.IntoCssURL(); Nit: two space indent. Also, does this compile? IntoCssURL is private. ::: servo/components/style/properties/gecko.mako.rs:1272 (Diff revision 2) > - let (ptr, len) = match url.as_slice_components() { > - Ok(value) => value, > - Err(_) => (ptr::null(), 0), > - }; > unsafe { > - Gecko_SetMozBinding(&mut self.gecko, > + Gecko_SetMozBinding(&mut self.gecko, url.for_ffi()); That looks nicer!
Attachment #8835831 - Flags: review?(cam) → review+
Depends on: 1338452
Summary: stylo: Handle css::URLValue and nsStyleSVGPaint computed values and → stylo: Handle css::URLValue and multiple new SVG values
Depends on: 1338764
Ignore the bindings fixup patch, that's a local change I made so that the rest of the patches have clean bindgen diffs. I'll try to exclude it from future mozreview pushes.
And with that we now support all properties in nsStyleSVG (we already support all properties in nsStyleSVGReset, though this makes clip-path 100% since it now supports urls) We don't yet handle pres attrs for SVG so most SVGs will still not quite work, but we're getting there! Tested them out, all the properties seem to work as they're supposed to. The only remaining thing to do for these properties is handle bug 1338764, which we can fill in later.
Summary: stylo: Handle css::URLValue and multiple new SVG values → stylo: Bindings for css::URLValue and all remaining nsStyleSVG values
Attachment #8836311 - Attachment is obsolete: true
Removed the bindings regen commit. Not sure if it should be included; the last sync didn't have a regen on the servo side AFAICT.
Comment on attachment 8835858 [details] Bug 1338388 - Part 2: stylo: Support URL filters ; https://reviewboard.mozilla.org/r/111404/#review113584 ::: servo/components/style/properties/longhand/effects.mako.rs:540 (Diff revision 3) > - if let Ok(function_name) = input.try(|input| input.expect_function()) { > + if let Ok(url) = input.try(|i| SpecifiedUrl::parse(context, i)) { > + % if product == "gecko": > + filters.push(SpecifiedFilter::Url(url)); Maybe we should be doing the SpecifiedUrl::parse call in the |% if product == "gecko":| block, to avoid wasted work in Servo. ::: servo/components/style/properties/longhand/effects.mako.rs:624 (Diff revision 3) > - computed_value::Filter::DropShadow(shadow) => { > + computed_value::Filter::DropShadow(ref shadow) => { > SpecifiedFilter::DropShadow( > - ToComputedValue::from_computed_value(&shadow), > + ToComputedValue::from_computed_value(shadow), (This seems like a separate fix that would have been better in a separate commit.)
Attachment #8835858 - Flags: review?(cam) → review+
Comment on attachment 8836202 [details] Bug 1338388 - Part 3: stylo: Support URL clip-paths ; https://reviewboard.mozilla.org/r/111650/#review113600
Attachment #8836202 - Flags: review?(cam) → review+
Comment on attachment 8836312 [details] Bug 1338388 - Part 4: stylo: Add Gecko bindings for <paint>, use for stroke/fill; https://reviewboard.mozilla.org/r/111774/#review113602 ::: servo/components/style/values/computed/mod.rs:192 (Diff revision 3) > +/// An SVG paint value without the fallback > +/// > +/// Whereas the spec only allows PaintServer > +/// to have a fallback, Gecko lets the context > +/// properties have a fallback as well. Since you've already written it, I guess it's fine to incorrectly accept a fallback colour after non-paint-server values, but can you file a followup to fix this? ::: servo/components/style/values/computed/mod.rs:199 (Diff revision 3) > +/// Whereas the spec only allows PaintServer > +/// to have a fallback, Gecko lets the context > +/// properties have a fallback as well. > +#[derive(Debug, Clone, PartialEq)] > +#[cfg_attr(feature = "servo", derive(HeapSizeOf))] > +pub enum SVGPaintKind { computed::SVGPaint looks the same as specified::SVGPaint. Should we use ComputedValueAsSpecified?
Attachment #8836312 - Flags: review?(cam) → review+
Comment on attachment 8836313 [details] Bug 1338388 - Part 5: stylo: Add mako template for URLOrNone, use for marker-* properties; https://reviewboard.mozilla.org/r/111776/#review113604 r=me with these fixed. ::: layout/style/ServoBindings.cpp:1076 (Diff revision 3) > + > +void > +Gecko_nsStyleSVGPaint_SetURLValue(nsStyleSVGPaint* aPaint, ServoBundledURI aURI) > +{ > + RefPtr<css::URLValue> url = aURI.IntoCssUrl(); > + aPaint->SetPaintServer(url.get(), 0); Nit: please use NS_RGB(0, 0, 0) rather than just 0, which is the idiomatic Gecko way to write a black nscolor. ::: servo/components/style/properties/longhand/inherited_svg.mako.rs:38 (Diff revision 3) > animatable=False, > spec="https://www.w3.org/TR/SVG11/painting.html#ColorInterpolationFiltersProperty")} > > +${helpers.predefined_type( > + "fill", "SVGPaint", > + "Default::default()", The initial value of fill is black (unlike stroke, where it is none).
Attachment #8836313 - Flags: review?(cam) → review+
Comment on attachment 8836314 [details] Bug 1338388 - Part 6: stylo: Support stroke-dasharray and stroke-dashoffset; https://reviewboard.mozilla.org/r/111778/#review113608 ::: servo/components/style/properties/longhand/inherited_svg.mako.rs:69 (Diff revision 3) > + "stroke-width", "LengthOrPercentage", > + "computed::LengthOrPercentage::one()", In Gecko stroke-width also supports the context-value keyword. Can be a followup if you don't want to fix it here.
Attachment #8836314 - Flags: review?(cam) → review+
Comment on attachment 8836439 [details] Bug 1338388 - Part 7: stylo: Add mako template for URLOrNone, use for marker-* properties; https://reviewboard.mozilla.org/r/111860/#review113610
Attachment #8836439 - Flags: review?(cam) → review+
Comment on attachment 8836456 [details] Bug 1338388 - Part 8: stylo: Add paint-order bitfield; https://reviewboard.mozilla.org/r/111866/#review113612 ::: servo/components/style/properties/longhand/inherited_svg.mako.rs:149 (Diff revision 1) > + pub const ALL: [u8; 3] = [FILL, STROKE, MARKERS]; > + > + /// Represented as a six-bit field, of 3 two-bit pairs > + /// > + /// Each pair can be set to FILL, STROKE, or MARKERS > + /// Lowest significant bit pairs are highest priority. I think it's not clear what priority means here: painted first (and so underneath other things), or painted on top?
Attachment #8836456 - Flags: review?(cam) → review+
Comment on attachment 8836312 [details] Bug 1338388 - Part 4: stylo: Add Gecko bindings for <paint>, use for stroke/fill; https://reviewboard.mozilla.org/r/111774/#review113602 > computed::SVGPaint looks the same as specified::SVGPaint. Should we use ComputedValueAsSpecified? `CSSColor`s are different; specified css colors contain the source string.
Comment on attachment 8836314 [details] Bug 1338388 - Part 6: stylo: Support stroke-dasharray and stroke-dashoffset; https://reviewboard.mozilla.org/r/111778/#review113608 > In Gecko stroke-width also supports the context-value keyword. Can be a followup if you don't want to fix it here. Yeah, plan to file a followup for the context stuff.
Comment on attachment 8836467 [details] Bug 1338388 - Part 9: stylo: Support stroke-dasharray and stroke-dashoffset; https://reviewboard.mozilla.org/r/111882/#review113616 ::: servo/components/style/parser.rs:113 (Diff revision 1) > +/// Parse a non-empty space-separated or comma-separated list of objects parsed by parse_one > +pub fn parse_space_or_comma_separated<F, T>(input: &mut Parser, mut parse_one: F) Note that stroke-dasharray actually allows this strange "comma or space" separate between each value, not just for the list as a whole. ::: servo/components/style/properties/longhand/inherited_svg.mako.rs:96 (Diff revision 1) > > ${helpers.predefined_type("stroke-opacity", "Opacity", "1.0", > products="gecko", animatable=False, > spec="https://www.w3.org/TR/SVG11/painting.html#StrokeOpacityProperty")} > > +${helpers.predefined_type("stroke-dasharray", "LoPOrNumber", "Either::Second(0.0)", stroke-dasharray can also take "none" (it's the initial value), which gets stored as an empty array in nsStyleSVG::mStrokeDasharray. Also, can you mention/handle context-value for stroke-dasharray and stroke-dashoffset in the followup bug for stroke-width:context-value too?
Attachment #8836467 - Flags: review?(cam) → review-
Blocks: 1339349
Comment on attachment 8836467 [details] Bug 1338388 - Part 9: stylo: Support stroke-dasharray and stroke-dashoffset; https://reviewboard.mozilla.org/r/111882/#review113616 > stroke-dasharray can also take "none" (it's the initial value), which gets stored as an empty array in nsStyleSVG::mStrokeDasharray. > > Also, can you mention/handle context-value for stroke-dasharray and stroke-dashoffset in the followup bug for stroke-width:context-value too? `vector=True` handles the initial value thing and `none`, the `Either::Second(0.0)` never gets used (but needs to be specified due to how `helpers.predefined_type` works).
Comment on attachment 8836468 [details] Bug 1338388 - Part 10: stylo: Support nonstandard CSS_PROPERTY_NUMBERS_ARE_PIXELS behavior; https://reviewboard.mozilla.org/r/111884/#review113946 ::: servo/components/style/values/specified/length.rs:992 (Diff revision 2) > LengthOrPercentage::parse_internal(input, AllowedNumericType::NonNegative) > } > > + /// Parse a length, treating dimensionless numbers as pixels > + /// > + /// This is nonstandard behavior used by Firefox for SVG This isn't non-standard behaviour, it's described here: https://www.w3.org/TR/SVG2/types.html#presentation-attribute-css-value
Attachment #8836468 - Flags: review?(cam) → review+
Comment on attachment 8836467 [details] Bug 1338388 - Part 9: stylo: Support stroke-dasharray and stroke-dashoffset; https://reviewboard.mozilla.org/r/111882/#review114046
Attachment #8836467 - Flags: review?(cam) → review+
Comment on attachment 8836468 [details] Bug 1338388 - Part 10: stylo: Support nonstandard CSS_PROPERTY_NUMBERS_ARE_PIXELS behavior; https://reviewboard.mozilla.org/r/111884/#review113946 > This isn't non-standard behaviour, it's described here: https://www.w3.org/TR/SVG2/types.html#presentation-attribute-css-value I don't see any mention of the array stuff there?
Comment on attachment 8836468 [details] Bug 1338388 - Part 10: stylo: Support nonstandard CSS_PROPERTY_NUMBERS_ARE_PIXELS behavior; https://reviewboard.mozilla.org/r/111884/#review113946 > I don't see any mention of the array stuff there? Oh sorry, mixed up the array comma behavior with the number-pixel behavior. Done.
Comment on attachment 8838806 [details] Bug 1338388 - Part 7: stylo: Update test expectations for svg properties; https://reviewboard.mozilla.org/r/113622/#review115202
Attachment #8838806 - Flags: review?(cam) → review+
Comment on attachment 8838826 [details] Bug 1338388 - Part 12: stylo: Update test expectations for svg properties; https://reviewboard.mozilla.org/r/113650/#review115208 mozreview marked the wrong one as r+
Attachment #8838826 - Flags: review+
Comment on attachment 8838826 [details] Bug 1338388 - Part 12: stylo: Update test expectations for svg properties; https://reviewboard.mozilla.org/r/113650/#review115210 ::: layout/style/test/stylo-failures.md:314 (Diff revision 1) > - * ... `"stroke` [8] > - * test_initial_computation.html `'stroke` [8] > - * test_initial_storage.html `'stroke` [4] > * ... `"stroke` [4] > * test_value_storage.html `on 'stroke` [71] > + * layout/style/test/test_compute_data_with_start_struct.html `initial and other values of stroke-dasharray are different` [2] The prefix "layout/style/test/" should be removed.
Comment on attachment 8838826 [details] Bug 1338388 - Part 12: stylo: Update test expectations for svg properties; https://reviewboard.mozilla.org/r/113650/#review115212 ::: layout/style/test/stylo-failures.md (Diff revision 1) > - * test_initial_storage.html `'stroke` [4] > * ... `"stroke` [4] You would need to replace "..." with "test_initial_storage.html" here as you removed its previous line where this line gets the test name from.
Comment on attachment 8838806 [details] Bug 1338388 - Part 7: stylo: Update test expectations for svg properties; https://reviewboard.mozilla.org/r/113622/#review115214 ::: servo/components/style/properties/shorthand/inherited_svg.mako.rs:20 (Diff revision 2) > + Ok(Longhands { > + marker_start: url.clone(), > + marker_mid: url.clone(), > + marker_end: url, > + }) > + Useless empty line with trailing whitespaces. Should be removed. ::: servo/components/style/properties/shorthand/inherited_svg.mako.rs:25 (Diff revision 2) > + > + } > + > + impl<'a> LonghandsToSerialize<'a> { > + fn to_css_declared<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write { > + let mut style_present = false; Unused variable?
Attachment #8838806 - Flags: review+
Comment on attachment 8838826 [details] Bug 1338388 - Part 12: stylo: Update test expectations for svg properties; https://reviewboard.mozilla.org/r/113650/#review115216
Attachment #8838826 - Flags: review+
Huh, there's a crash in URLValueData::AddRef at layout/style/test/test_value_cloning.html I'll look into this before landing the servo side.
Attachment #8838826 - Flags: review?(cam) → review+
Attachment #8836439 - Attachment is obsolete: true
Attachment #8836456 - Attachment is obsolete: true
Attachment #8836467 - Attachment is obsolete: true
Attachment #8836468 - Attachment is obsolete: true
Attachment #8838826 - Attachment is obsolete: true
Attachment #8838826 - Flags: review?(cam)
Pushed by manishearth@gmail.com: https://hg.mozilla.org/integration/autoland/rev/eb7a00775294 Part 1: stylo: Add ServoBundledURI abstraction for use when setting css::URLValues; r=heycam https://hg.mozilla.org/integration/autoland/rev/779655eeda41 Part 2: stylo: Support URL filters ; r=heycam https://hg.mozilla.org/integration/autoland/rev/29fbcb86b721 Part 3: stylo: Support URL clip-paths ; r=heycam https://hg.mozilla.org/integration/autoland/rev/02aa7ecff973 Part 4: stylo: Add Gecko bindings for <paint>, use for stroke/fill; r=heycam https://hg.mozilla.org/integration/autoland/rev/0f96a7107b4f Part 5: stylo: Add mako template for URLOrNone, use for marker-* properties; r=heycam https://hg.mozilla.org/integration/autoland/rev/e2c110112436 Part 6: stylo: Support stroke-dasharray and stroke-dashoffset; r=heycam https://hg.mozilla.org/integration/autoland/rev/00f9cf3c4562 Part 7: stylo: Update test expectations for svg properties; r=heycam,xidorn
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: