Closed
Bug 1338936
Opened 8 years ago
Closed 8 years ago
stylo: Add Servo implementor of GenericSpecifiedValues
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: manishearth, Assigned: manishearth)
References
Details
Attachments
(4 files, 8 obsolete files)
This builds on the work in bug 1330041 and bug 1334330 to finish up the non-SVG (SVG is bug 1329093) presentation attribute support. We add a ServoSpecifiedValues type and make the Servo pres attr code use it instead of faking nsRuleDatas
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8836620 -
Attachment is obsolete: true
Attachment #8836620 -
Flags: review?(emilio+bugs)
Assignee | ||
Updated•8 years ago
|
Attachment #8836622 -
Attachment is obsolete: true
Attachment #8836622 -
Flags: review?(emilio+bugs)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8836615 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•8 years ago
|
Attachment #8836616 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 20•8 years ago
|
||
Only need bz's review for the first two patches, and they're fairly mechanical, so flagging for review now instead of waiting for Emilio to get there.
With these patches, all pres attr properties should work except for background (background-image), lang (-x-lang), <col span> (-x-span), MathML properties, and SVG properties (bug 1329093).
Hopefully will also lead to a jump in the number of passing reftests.
Comment 21•8 years ago
|
||
mozreview-review |
Comment on attachment 8836615 [details]
Bug 1338936 - Part 1: stylo: Add stubbed-out ServoSpecifiedValues interface, use it for pres attr mapping;
https://reviewboard.mozilla.org/r/112008/#review113230
::: dom/base/nsMappedAttributes.cpp:300
(Diff revision 2)
>
> MOZ_ASSERT(!mServoStyle,
> "LazilyResolveServoDeclaration should not be called if mServoStyle is already set");
> mServoStyle = Servo_DeclarationBlock_CreateEmpty().Consume();
> - for (size_t i = 0; i < aRuleDataSize; i++) {
> - nsCSSValue& val = aRuleData->mValueStorage[i];
> + ServoSpecifiedValues servo = ServoSpecifiedValues(aContext, mServoStyle);
> + if (mRuleMapper) {
Construct it inside of the if?
::: dom/base/nsMappedAttributes.cpp:301
(Diff revision 2)
> MOZ_ASSERT(!mServoStyle,
> "LazilyResolveServoDeclaration should not be called if mServoStyle is already set");
> mServoStyle = Servo_DeclarationBlock_CreateEmpty().Consume();
> - for (size_t i = 0; i < aRuleDataSize; i++) {
> - nsCSSValue& val = aRuleData->mValueStorage[i];
> - if (val.GetUnit() != eCSSUnit_Null) {
> + ServoSpecifiedValues servo = ServoSpecifiedValues(aContext, mServoStyle);
> + if (mRuleMapper) {
> + (*mRuleMapper)(this, &servo);
We should probably make the mapper take references, but that's probably ok as a followup.
::: layout/style/GenericSpecifiedValuesInlines.h:18
(Diff revision 2)
>
> #ifndef mozilla_GenericSpecifiedValuesInlines_h
> #define mozilla_GenericSpecifiedValuesInlines_h
>
> #include "nsRuleData.h"
> +#include "mozilla/ServoSpecifiedValues.h"
Down, so it's sorted.
::: layout/style/ServoSpecifiedValues.h:20
(Diff revision 2)
> +#include "mozilla/GenericSpecifiedValues.h"
> +#include "mozilla/ServoBindings.h"
> +
> +namespace mozilla {
> +
> +class ServoSpecifiedValues final: public GenericSpecifiedValues {
brace
::: layout/style/ServoSpecifiedValues.cpp:18
(Diff revision 2)
> +
> +ServoSpecifiedValues::ServoSpecifiedValues(nsPresContext* aContext,
> + RefPtr<RawServoDeclarationBlock> aDecl)
> +
> + : GenericSpecifiedValues(StyleBackendType::Servo, aContext, ALL_SIDS),
> + mDecl(aDecl) {
brace, comma before mDecl instead of after the paren.
: GenericSpecifiedValues(StyleBackendType::Servo, aContext, ALL_SIDS)
, mDecl(aDecl)
{
}
Attachment #8836615 -
Flags: review?(emilio+bugs) → review+
Comment 22•8 years ago
|
||
mozreview-review |
Comment on attachment 8836616 [details]
Bug 1338936 - Part 2: stylo: Add necessary stubbed-out bindings for ServoSpecifiedValues;
https://reviewboard.mozilla.org/r/112010/#review113234
::: dom/html/nsGenericHTMLElement.cpp:1527
(Diff revision 3)
> if (!aData->ShouldComputeStyleStruct(NS_STYLE_INHERIT_BIT(Background)))
> return;
>
> + if (aData->IsServo()) {
> + // FIXME(bug 1330041)
> + NS_WARNING("stylo: cannot handle background");
Maybe another slightly more descriptive message? "cannot handle background-affecting mapped attribute" or something?
::: dom/mathml/nsMathMLElement.cpp:494
(Diff revision 3)
> void
> nsMathMLElement::MapMathMLAttributesInto(const nsMappedAttributes* aAttributes,
> GenericSpecifiedValues* aGenericData)
> {
> + if (aGenericData->IsServo()) {
> + // FIXME handle MathML properties in Stylo
Make noise?
::: layout/style/ServoSpecifiedValues.cpp:31
(Diff revision 3)
> + return Servo_DeclarationBlock_PropertyIsSet(mDecl, aId);
> +}
> +
> +void
> +ServoSpecifiedValues::SetIdentStringValue(nsCSSPropertyID aId,
> + const nsString& aValue) {
brace
::: layout/style/ServoSpecifiedValues.cpp:74
(Diff revision 3)
> +}
> +
> +void
> +ServoSpecifiedValues::SetColorValue(nsCSSPropertyID aId, nscolor aColor)
> +{
> + Servo_DeclarationBlock_SetColorValue(mDecl, aId, aColor);
Trailing whitespace.
::: servo/ports/geckolib/glue.rs:995
(Diff revision 3)
> + property: nsCSSPropertyID)
> + -> bool {
> + use style::properties::PropertyDeclarationId;
> let declarations = RwLock::<PropertyDeclarationBlock>::as_arc(&declarations);
> - let prop = PropertyId::from_nscsspropertyid(property);
> + let long = get_longhand_from_id!(property, false);
> + declarations.read().get(PropertyDeclarationId::Longhand(long)).is_some()
This is not specially funny, because this is linear, but probably that's fine? Alternatively, we should maybe store a PropertyBitfield or something.
::: servo/ports/geckolib/glue.rs:1020
(Diff revision 3)
> +
> +#[no_mangle]
> +pub extern "C" fn Servo_DeclarationBlock_SetIntValue(_: RawServoDeclarationBlockBorrowed,
> + _: nsCSSPropertyID,
> + _: i32) {
> + error!("stylo: Don't know how to handle -x-span property");
The messages don't make much sense without context, so I'd put a more generic message, or rename the function.
Attachment #8836616 -
Flags: review?(emilio+bugs) → review+
Comment 23•8 years ago
|
||
mozreview-review |
Comment on attachment 8836617 [details]
Bug 1338936 - Part 3: stylo: Support lang property;
https://reviewboard.mozilla.org/r/112012/#review113236
::: servo/components/style/properties/declaration_block.rs:45
(Diff revision 3)
> }
> }
>
> +impl Default for Importance {
> + #[inline]
> + fn default() -> Self {
This seems like a footgun, what's the problem with using `Importance::Normal` instead? It's not a super-more-painful thing to write.
::: servo/ports/geckolib/glue.rs:988
(Diff revision 3)
> }
> }
> }
>
> +macro_rules! match_wrap_declared {
> + ($longhand:ident : $($property:ident => $inner:expr,)*) => (
note that other similar macros, like `match_eq_ignore_ascii_case` use a comman instead of a colon, perhaps it'd be nice to be consistent there.
::: servo/ports/geckolib/glue.rs:994
(Diff revision 3)
> + match $longhand {
> + $(
> + LonghandId::$property => PropertyDeclaration::$property(DeclaredValue::Value($inner)),
> + )*
> + _ => {
> + error!("stylo: Don't know how to handle presentation property {:?}", $longhand);
Don't you need to `stringify!($longhand)`? huh
Attachment #8836617 -
Flags: review?(emilio+bugs) → review+
Assignee | ||
Comment 24•8 years ago
|
||
mozreview-review |
Comment on attachment 8836616 [details]
Bug 1338936 - Part 2: stylo: Add necessary stubbed-out bindings for ServoSpecifiedValues;
https://reviewboard.mozilla.org/r/112010/#review113344
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 38•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8836617 [details]
Bug 1338936 - Part 3: stylo: Support lang property;
https://reviewboard.mozilla.org/r/112012/#review113236
> This seems like a footgun, what's the problem with using `Importance::Normal` instead? It's not a super-more-painful thing to write.
It needs to be imported, then. There's like one place in the entire codebase where we don't use `Importance::Normal`.
> note that other similar macros, like `match_eq_ignore_ascii_case` use a comman instead of a colon, perhaps it'd be nice to be consistent there.
An earlier version had a default arm and was rife with parsing ambiguity / follow rule issues, but that should not be an issue anymore. Will fix.
> Don't you need to `stringify!($longhand)`? huh
`$longhand` is a variable name, like `long`. Stringifying it won't help. I need to print its _value_.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 40•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8836615 [details]
Bug 1338936 - Part 1: stylo: Add stubbed-out ServoSpecifiedValues interface, use it for pres attr mapping;
https://reviewboard.mozilla.org/r/112008/#review113230
> We should probably make the mapper take references, but that's probably ok as a followup.
Feels mostly tangential; the mapper has always taken pointers. Could file an easy bug, but probably should do so after this lands.
Comment 41•8 years ago
|
||
mozreview-review |
Comment on attachment 8836618 [details]
Bug 1338936 - Part 4: stylo: Update reftest results;
https://reviewboard.mozilla.org/r/112014/#review113654
::: servo/ports/geckolib/glue.rs:1154
(Diff revision 5)
> + use style::gecko::values::convert_nscolor_to_rgba;
> + use style::properties::{DeclaredValue, PropertyDeclaration, LonghandId};
> + use style::values::specified::{CSSColor, CSSRGBA};
> +
> + let declarations = RwLock::<PropertyDeclarationBlock>::as_arc(&declarations);
> + let long = get_longhand_from_id!(property, ());
You could probably make a one-argument version of this that defaults to returning `()`, wdyt?
::: servo/ports/geckolib/glue.rs:1166
(Diff revision 5)
> + BorderBottomColor => Box::new(color),
> + BorderLeftColor => Box::new(color),
> + Color => Box::new(CSSRGBA {parsed: rgba, authored: None}),
> + BackgroundColor => Box::new(color),
> + };
> + declarations.write().declarations.push((prop, Default::default()));
I believe importing `Importance` is not a big deal, please do so instead of using `Default`.
Attachment #8836618 -
Flags: review?(emilio+bugs) → review+
Comment 42•8 years ago
|
||
mozreview-review |
Comment on attachment 8836619 [details]
Bug 1338936 - Part 5: stylo: Support font-family presentation attribute;
https://reviewboard.mozilla.org/r/112016/#review113656
::: servo/components/style/properties/longhand/font.mako.rs:175
(Diff revision 5)
> - Vec::<FontFamily>::parse(context, input).map(SpecifiedValue)
> + SpecifiedValue::parse(input)
> }
>
> - impl Parse for Vec<FontFamily> {
> - fn parse(context: &ParserContext, input: &mut Parser) -> Result<Self, ()> {
> - input.parse_comma_separated(|input| FontFamily::parse(context, input))
> + impl SpecifiedValue {
> + pub fn parse(input: &mut Parser) -> Result<Self, ()> {
> + input.parse_comma_separated(|input| FontFamily::parse(input)).map(SpecifiedValue)
`parse_comma_separated(FontFamily::parse)`?
::: servo/ports/geckolib/glue.rs:1178
(Diff revision 5)
> + use cssparser::Parser;
> + use style::properties::{DeclaredValue, PropertyDeclaration};
> + use style::properties::longhands::font_family::SpecifiedValue as FontFamily;
>
> + let declarations = RwLock::<PropertyDeclarationBlock>::as_arc(&declarations);
> + let string = format!("{}", unsafe { &*value });
Fon't we have a `to_string`, or something from `nsAString`?
Attachment #8836619 -
Flags: review?(emilio+bugs) → review+
Comment 43•8 years ago
|
||
mozreview-review |
Comment on attachment 8836621 [details]
Bug 1338936 - Part 6: stylo: Add helpers for converting Gecko keywords to Servo, use to support keyword pres attrs;
https://reviewboard.mozilla.org/r/112020/#review113658
::: servo/components/style/properties/longhand/pointing.mako.rs:162
(Diff revision 5)
> spec="Nonstandard (https://developer.mozilla.org/en-US/docs/Web/CSS/-moz-user-input)")}
>
> ${helpers.single_keyword("-moz-user-modify", "read-only read-write write-only",
> products="gecko", gecko_ffi_name="mUserModify",
> gecko_enum_prefix="StyleUserModify",
> - gecko_inexhaustive=True,
> + gecko_inexhaustive=False,
remove the `gecko_inexhaustive` line completely? It's false by default right?
::: servo/ports/geckolib/glue.rs:1046
(Diff revision 5)
> + Float => longhands::float::SpecifiedValue::from_gecko_keyword(value),
> + VerticalAlign => longhands::vertical_align::SpecifiedValue::from_gecko_keyword(value),
> + TextAlign => longhands::text_align::SpecifiedValue::from_gecko_keyword(value),
> + Clear => longhands::clear::SpecifiedValue::from_gecko_keyword(value),
> + FontSize => {
> + longhands::font_size::SpecifiedValue(NoCalcLength::from_font_size_int(value as u8).into())
Please add a comment saying that we rely on how these are represented in Gecko, presumably with an assertion.
Attachment #8836621 -
Flags: review?(emilio+bugs) → review+
Comment 44•8 years ago
|
||
mozreview-review |
Comment on attachment 8836832 [details]
Bug 1338936 - Part 7: stylo: Support -x-span for `<col span>`;
https://reviewboard.mozilla.org/r/112134/#review113660
::: servo/components/style/properties/longhand/table.mako.rs:14
(Diff revision 1)
> ${helpers.single_keyword("table-layout", "auto fixed",
> gecko_ffi_name="mLayoutStrategy", animatable=False,
> spec="https://drafts.csswg.org/css-tables/#propdef-table-layout")}
> +
> +<%helpers:longhand name="-x-span" products="gecko"
> + spec="Internal-only (for `<col span>` pres attr)"
You need something here so we don't parse it in stylesheets. Returning `Err(())` in the parse function is something slightly tricky, and I don't want to accidentally expose this.
How do you feel about adding the `internal` flag here? Sure it'd be useless, but it seems logical, and if someone messes up, the worst that can happen is that we expose this to UA sheets.
Also, maybe rename it with a longer more descriptive name?
Attachment #8836832 -
Flags: review?(emilio+bugs) → review+
Comment 45•8 years ago
|
||
mozreview-review |
Comment on attachment 8836874 [details]
Bug 1338936 - Part 8: stylo: Support quirksmode text-decoration color override;
https://reviewboard.mozilla.org/r/112178/#review113664
::: servo/components/style/properties/longhand/text.mako.rs:122
(Diff revision 1)
> #[derive(PartialEq, Eq, Copy, Clone, Debug)]
> #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
> pub struct SpecifiedValue {
> pub underline: bool,
> pub overline: bool,
> pub line_through: bool,
We should seriously consider using bitflags for these. A followup is fine.
Comment 46•8 years ago
|
||
mozreview-review |
Comment on attachment 8836874 [details]
Bug 1338936 - Part 8: stylo: Support quirksmode text-decoration color override;
https://reviewboard.mozilla.org/r/112178/#review113666
Attachment #8836874 -
Flags: review?(emilio+bugs) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 48•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8836621 [details]
Bug 1338936 - Part 6: stylo: Add helpers for converting Gecko keywords to Servo, use to support keyword pres attrs;
https://reviewboard.mozilla.org/r/112020/#review113658
> Please add a comment saying that we rely on how these are represented in Gecko, presumably with an assertion.
Not sure what I can assert here.
Assignee | ||
Comment 49•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8836874 [details]
Bug 1338936 - Part 8: stylo: Support quirksmode text-decoration color override;
https://reviewboard.mozilla.org/r/112178/#review113664
> We should seriously consider using bitflags for these. A followup is fine.
https://github.com/servo/servo/issues/15556
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 57•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8836832 [details]
Bug 1338936 - Part 7: stylo: Support -x-span for `<col span>`;
https://reviewboard.mozilla.org/r/112134/#review113660
> You need something here so we don't parse it in stylesheets. Returning `Err(())` in the parse function is something slightly tricky, and I don't want to accidentally expose this.
>
> How do you feel about adding the `internal` flag here? Sure it'd be useless, but it seems logical, and if someone messes up, the worst that can happen is that we expose this to UA sheets.
>
> Also, maybe rename it with a longer more descriptive name?
Gecko uses `-x-span`, so I'm mirroring that. Naming it something else will mean that we need to add hacks to the aliasing stuff to get the property id mapping. Doesn't seem to be worth it.
Comment 58•8 years ago
|
||
mozreview-review |
Comment on attachment 8836615 [details]
Bug 1338936 - Part 1: stylo: Add stubbed-out ServoSpecifiedValues interface, use it for pres attr mapping;
https://reviewboard.mozilla.org/r/112008/#review114026
r=me with the issues below fixed.
::: dom/base/nsMappedAttributes.h:82
(Diff revision 4)
> // to result of this computation.
> // aIndexToIdMapping is
> // a table that maps from an index in the rule data to the corresponding
> // property ID. aRuleDataSize is the length of the backing storage
> // of the rule data.
> - void LazilyResolveServoDeclaration(nsRuleData* aRuleData,
> + void LazilyResolveServoDeclaration(nsPresContext* aPresContext);
The comment no longer has any bearing on the method signature. Please fix that.
::: layout/style/GenericSpecifiedValues.h:25
(Diff revision 4)
>
> struct nsRuleData;
> +
> namespace mozilla {
> +
> +class ServoSpecifiedValues;
I kinda wonder whether this should be `servo::SpecifiedValues`, but I guess most of the other Servo stuff is named like this....
::: layout/style/ServoSpecifiedValues.h:24
(Diff revision 4)
> +
> +class ServoSpecifiedValues final: public GenericSpecifiedValues
> +{
> +public:
> +
> + ServoSpecifiedValues(nsPresContext* aContext, RefPtr<RawServoDeclarationBlock> aDecl);
Passing RefPtr by value like this is pretty weird in Gecko. Just pass RawServoDeclarationBlock*.
::: layout/style/ServoSpecifiedValues.cpp:9
(Diff revision 4)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "mozilla/ServoSpecifiedValues.h"
> +
> +#define STYLE_STRUCT(name, checkdata_cb) | NS_STYLE_INHERIT_BIT(name)
> +const uint64_t ALL_SIDS = 0
static, or inside an anonymous namespace. No need to create a new global symbol here.
Also, shouldn't this be uint32_t? That's what mSIDs is on GenericSpecifiedValues.
Attachment #8836615 -
Flags: review?(bzbarsky) → review+
Comment 59•8 years ago
|
||
mozreview-review |
Comment on attachment 8836616 [details]
Bug 1338936 - Part 2: stylo: Add necessary stubbed-out bindings for ServoSpecifiedValues;
https://reviewboard.mozilla.org/r/112010/#review114036
r=me with the nits addressed.
::: dom/html/nsGenericHTMLElement.cpp:1526
(Diff revision 5)
>
> if (!aData->ShouldComputeStyleStruct(NS_STYLE_INHERIT_BIT(Background)))
> return;
>
> + if (aData->IsServo()) {
> + // FIXME(bug 1330041)
This doesn't look like the right bug#. Please make sure to lonk to the right bug here; one that isn't resolved and that blocks the relevant style tracking bugs.
::: dom/mathml/nsMathMLElement.cpp:494
(Diff revision 5)
> void
> nsMathMLElement::MapMathMLAttributesInto(const nsMappedAttributes* aAttributes,
> GenericSpecifiedValues* aGenericData)
> {
> + if (aGenericData->IsServo()) {
> + // FIXME handle MathML properties in Stylo
Needs a bug number.
::: servo/ports/geckolib/glue.rs:995
(Diff revision 5)
> + property: nsCSSPropertyID)
> + -> bool {
> + use style::properties::PropertyDeclarationId;
> let declarations = RwLock::<PropertyDeclarationBlock>::as_arc(&declarations);
> - let prop = PropertyId::from_nscsspropertyid(property);
> + let long = get_longhand_from_id!(property, false);
> + declarations.read().get(PropertyDeclarationId::Longhand(long)).is_some()
This doesn't obviously seem anything resembling O(1). Is it, somehow? If not, do we have any numbers on how long the declarations list might get get?
Attachment #8836616 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 60•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8836615 [details]
Bug 1338936 - Part 1: stylo: Add stubbed-out ServoSpecifiedValues interface, use it for pres attr mapping;
https://reviewboard.mozilla.org/r/112008/#review114026
> I kinda wonder whether this should be `servo::SpecifiedValues`, but I guess most of the other Servo stuff is named like this....
I think bindgen used to have issue with namespacing and the naming scheme stuck.
> Passing RefPtr by value like this is pretty weird in Gecko. Just pass RawServoDeclarationBlock*.
Could I pass `RefPtr<_>` by-ref instead?
Comment 61•8 years ago
|
||
> Could I pass `RefPtr<_>` by-ref instead?
You could, but it would still look weird from a Gecko hacker's pov, fwiw...
Assignee | ||
Comment 62•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8836616 [details]
Bug 1338936 - Part 2: stylo: Add necessary stubbed-out bindings for ServoSpecifiedValues;
https://reviewboard.mozilla.org/r/112010/#review114036
> This doesn't obviously seem anything resembling O(1). Is it, somehow? If not, do we have any numbers on how long the declarations list might get get?
It's `O(n)`, but given that one nsMappedAttributes only corresponds to one MapRuleInto function, I think n will on average be really small (`<5`), at max something like 20 for elements with a `border` attribute. We can statically determine this maximum number if we want.
We could support an array-based approach like nsRuleData, though Servo specified values are pretty large (48 bytes each right now. We can work on reducing this more with boxing medium-sized properties too (we already box large ones so that the enum stays small), but at some level boxing becomes a bad idea) and the cache penalty of a huge array of them could destroy any benefit from using direct indexing.
Assignee | ||
Comment 63•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8836616 [details]
Bug 1338936 - Part 2: stylo: Add necessary stubbed-out bindings for ServoSpecifiedValues;
https://reviewboard.mozilla.org/r/112010/#review113234
> This is not specially funny, because this is linear, but probably that's fine? Alternatively, we should maybe store a PropertyBitfield or something.
It's linear for like `n=2`, I don't think we get many presentation attributes per element. Unsure if it's worth it.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 73•8 years ago
|
||
> At max something like 20 for elements with a `border` attribute
Hmm. OK. I guess actually <hr> is the worst case, with the various border-radius bits.
The basic worry I have is that this makes setting all the props O(N^2), and even if N is not too big this can get annoying.
So here's a question. For the servo case, when we're working with a declaration block specially for the mapped attrs, can we really end up with any cases in which the prop _is_ set? If not, can we just assert that and then have Servo_DeclarationBlock_PropertyIsSet always return false?
Assignee | ||
Comment 74•8 years ago
|
||
> For the servo case, when we're working with a declaration block specially for the mapped attrs, can we really end up with any cases in which the prop _is_ set?
Only if the attr mappers ever intersect, and they don't I think. If they did (and it mattered) there would have to be a precedence relation between certain pres attrs.
> If not, can we just assert that and then have Servo_DeclarationBlock_PropertyIsSet always return false?
Actually, could we return true in the gecko-side code itself? Probably would be nice for the optimizer. We can debug assert (MOZ_ASSERT?) that the servo call succeeds in debug builds. I'll roll that change into this patch.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 84•8 years ago
|
||
> If they did (and it mattered) there would have to be a precedence relation between certain pres attrs.
Good point!
> Actually, could we return true in the gecko-side code itself?
You mean "false". Assuming you mean for the servo case, yes, we can.
Assignee | ||
Comment 85•8 years ago
|
||
Yeah, I meant false. I'll update the reftest expectations and land this then.
Comment 86•8 years ago
|
||
mozreview-review |
Comment on attachment 8837345 [details]
Bug 1338936 - Part 9: stylo: Support lang property;
https://reviewboard.mozilla.org/r/112498/#review114362
r=me % comments.
::: servo/components/style/properties/longhand/font.mako.rs:781
(Diff revision 4)
> + use std::fmt;
> + use style_traits::ToCss;
> +
> + impl ToCss for T {
> + fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
> + dest.write_str("")
This is effectively the same as returning `Ok(())`, right? Let's do that.
::: servo/components/style/properties/longhand/font.mako.rs:787
(Diff revision 4)
> + }
> + }
> +
> + #[derive(Clone, Debug, PartialEq)]
> + #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
> + pub struct T(pub String);
So we're getting a nsString, converting into a string, then atomizing from Gecko. We should make this store an atom directly from the nsString.
This would make the conversion free. Also, I think you can `forget` the atom and set the pointer directly instead of adding an FFI call, but I think it may not be worth it if it's not too hot.
::: servo/components/style/properties/longhand/font.mako.rs:795
(Diff revision 4)
> + #[inline]
> + pub fn get_initial_value() -> computed_value::T {
> + computed_value::T("".to_string())
> + }
> +
> + pub fn parse(_context: &ParserContext, _input: &mut Parser) -> Result<SpecifiedValue, ()> {
Maybe add a `debug_assert!(false, "Should only be called from UA sheets, that shouldn't contain this property")`, or similar?
::: servo/ports/geckolib/glue.rs:1027
(Diff revision 4)
> - //
> - error!("stylo: Don't know how to handle ident presentation attributes (-x-lang)");
> + use style::properties::{DeclaredValue, PropertyDeclaration, LonghandId};
> + use style::properties::longhands::_x_lang::computed_value::T as Lang;
> +
> + let declarations = RwLock::<PropertyDeclarationBlock>::as_arc(&declarations);
> + let long = get_longhand_from_id!(property);
> + let prop = match_wrap_declared! {long,
nit: probably tidy complains and you need to add a space after the brace.
Attachment #8837345 -
Flags: review?(emilio+bugs) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 88•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8837345 [details]
Bug 1338936 - Part 9: stylo: Support lang property;
https://reviewboard.mozilla.org/r/112498/#review114362
> So we're getting a nsString, converting into a string, then atomizing from Gecko. We should make this store an atom directly from the nsString.
>
>
> This would make the conversion free. Also, I think you can `forget` the atom and set the pointer directly instead of adding an FFI call, but I think it may not be worth it if it's not too hot.
I'd prefer to not store nsIAtoms, especially since they're COMPtrs. Also, I think Servo might need to use this property too?
If we want to do this, I'd prefer if this was a followup about nsCOMPtr bindings.
Comment 89•8 years ago
|
||
(In reply to Manish Goregaokar [:manishearth] from comment #88)
> Comment on attachment 8837345 [details]
> Bug 1338936 - Part 9: stylo: Support lang property;
>
> https://reviewboard.mozilla.org/r/112498/#review114362
>
> > So we're getting a nsString, converting into a string, then atomizing from Gecko. We should make this store an atom directly from the nsString.
> >
> >
> > This would make the conversion free. Also, I think you can `forget` the atom and set the pointer directly instead of adding an FFI call, but I think it may not be worth it if it's not too hot.
>
> I'd prefer to not store nsIAtoms, especially since they're COMPtrs. Also, I
> think Servo might need to use this property too?
>
> If we want to do this, I'd prefer if this was a followup about nsCOMPtr
> bindings.
We discussed this and this is wrong. We do store and share nsIAtom instances all the time, they have threadsafe refcounting, and the servo Atom type just wraps one of those. So this is perfectly doable.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 99•8 years ago
|
||
mozreview-review |
Comment on attachment 8838319 [details]
Bug 1338936 - Part 10: stylo: Update reftest results;
https://reviewboard.mozilla.org/r/113268/#review114866
nice :)
I don't think you should need review to update reftests results to pass though :)
Attachment #8838319 -
Flags: review?(emilio+bugs) → review+
Assignee | ||
Comment 100•8 years ago
|
||
Servo PR at https://github.com/servo/servo/pull/15644
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8836619 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8836621 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8836832 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8836874 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8837345 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8838319 -
Attachment is obsolete: true
Assignee | ||
Comment 105•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 110•8 years ago
|
||
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7943ca5e483a
Part 1: stylo: Add stubbed-out ServoSpecifiedValues interface, use it for pres attr mapping; r=bz,emilio
https://hg.mozilla.org/integration/autoland/rev/7d7457d5ca8a
Part 2: stylo: Add necessary stubbed-out bindings for ServoSpecifiedValues; r=bz,emilio
https://hg.mozilla.org/integration/autoland/rev/38de3820ca81
Part 3: stylo: Support lang property; r=emilio
https://hg.mozilla.org/integration/autoland/rev/2d887234705f
Part 4: stylo: Update reftest results; r=emilio
Assignee | ||
Comment 111•8 years ago
|
||
Getting some really strange windows header file issues.
Comment 112•8 years ago
|
||
Backed out for Windows build bustage:
https://hg.mozilla.org/integration/autoland/rev/1e1d6c862e4c8dc222e465cd8e78ebe48a38e5d2
https://hg.mozilla.org/integration/autoland/rev/70a65a89fe15385e30ed296c33662e0ca1a98e47
https://hg.mozilla.org/integration/autoland/rev/42ce6d17aedabfb27dab8175908d0ac0e229f881
https://hg.mozilla.org/integration/autoland/rev/21c16da657238e66b49d1fb48923fb03e23aa86f
https://hg.mozilla.org/integration/autoland/rev/86affde3caa48a6a9f13aceb4927e13eabd01c2e
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2d887234705f65d86a6f2b4e7780c14e7aca7dd4&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=78622324&repo=autoland
01:02:51 INFO - c:/builds/moz2_slave/autoland-w32-00000000000000000/build/src/sccache2/sccache.exe c:/builds/moz2_slave/autoland-w32-00000000000000000/build/src/vs2015u3/VC/bin/amd64_x86/cl.exe -Fofixed_dsp_init.obj -c -DNDEBUG=1 -DTRIMMED=1 -D_USE_MATH_DEFINES -Dinline=__inline -DHAVE_AV_CONFIG_H -DASSERT_LEVEL=1 -Ic:/builds/moz2_slave/autoland-w32-00000000000000000/build/src/media/ffvpx/libavutil/x86 -Ic:/builds/moz2_slave/autoland-w32-00000000000000000/build/src/obj-firefox/media/ffvpx/libavutil/x86 -Ic:/builds/moz2_slave/autoland-w32-00000000000000000/build/src/media/ffvpx -Ic:/builds/moz2_slave/autoland-w32-00000000000000000/build/src/obj-firefox/dist/include -Ic:/builds/moz2_slave/autoland-w32-00000000000000000/build/src/obj-firefox/dist/include/nspr -Ic:/builds/moz2_slave/autoland-w32-00000000000000000/build/src/obj-firefox/dist/include/nss -MD -FI c:/builds/moz2_slave/autoland-w32-00000000000000000/build/src/obj-firefox/mozilla-config.h -DMOZILLA_CLIENT -deps.deps/fixed_dsp_init.obj.pp -TC -nologo -wd4091 -D_HAS_EXCEPTIONS=0 -W3 -Gy -Zc:inline -utf-8 -arch:SSE2 -FS -Gw -wd4244 -wd4267 -we4553 -Z7 -O1 -Oi -Oy- -wd4090 -wd4018 -wd4305 -wd4554 -wd4307 -wd4028 -wd4056 -wd4756 -wd4005 -wd4054 -wd4189 -wd4133 -wd4221 -wd4206 -wd4702 -wd4101 -wd4245 -wd4703 -wd4293 -wd4244 -wd4127 -wd4018 -wd4389 -wd4146 -wd4701 -wd4057 -wd4204 -wd4706 -wd4305 -wd4152 -wd4324 -we4013 -wd4100 -wd4214 -wd4307 -wd4273 -wd4554 c:/builds/moz2_slave/autoland-w32-00000000000000000/build/src/media/ffvpx/libavutil/x86/fixed_dsp_init.c
01:02:51 INFO - Unified_cpp_layout_style4.cpp
01:02:51 INFO - c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\vs2015u3\VC\include\utility(94): error C2061: syntax error: identifier 'enable_if_t'
01:02:51 INFO - c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\vs2015u3\VC\include\utility(255): note: see reference to class template instantiation 'std::pair<_Ty1,_Ty2>' being compiled
01:02:51 INFO - c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\vs2015u3\VC\include\utility(95): error C2988: unrecognizable template declaration/definition
01:02:51 INFO - c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\vs2015u3\VC\include\utility(95): error C2143: syntax error: missing ';' before '&&'
01:02:51 INFO - c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\vs2015u3\VC\include\utility(95): error C2238: unexpected token(s) preceding ';'
01:02:51 INFO - c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\vs2015u3\VC\include\utility(95): error C2059: syntax error: '&&'
01:02:51 INFO - c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\vs2015u3\VC\include\utility(113): error C2238: unexpected token(s) preceding ';'
01:02:51 INFO - c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\vs2015u3\VC\include\utility(119): error C2061: syntax error: identifier 'enable_if_t'
01:02:51 INFO - c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\vs2015u3\VC\include\utility(120): error C2988: unrecognizable template declaration/definition
01:02:51 INFO - c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\vs2015u3\VC\include\utility(120): error C2143: syntax error: missing ';' before '&&'
01:02:51 INFO - c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\vs2015u3\VC\include\utility(120): error C2238: unexpected token(s) preceding ';'
01:02:51 INFO - c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\vs2015u3\VC\include\utility(120): error C2059: syntax error: '&&'
01:02:51 INFO - c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\vs2015u3\VC\include\utility(155): error C2059: syntax error: '...'
01:02:51 INFO - c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\vs2015u3\VC\include\utility(156): error C2059: syntax error: '...'
01:02:51 INFO - c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\vs2015u3\VC\include\utility(158): error C2065: '_Types1': undeclared identifier
01:02:51 INFO - c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\vs2015u3\VC\include\utility(158): error C3544: '<unnamed-symbol>': parameter pack expects a type template argument
01:02:51 INFO - c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\vs2015u3\VC\include\utility(159): error C2065: '_Types2': undeclared identifier
01:02:51 INFO - c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\vs2015u3\VC\include\utility(159): error C3544: '<unnamed-symbol>': parameter pack expects a type template argument
01:02:51 INFO - c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\vs2015u3\VC\include\utility(159): error C2238: unexpected token(s) preceding ';'
01:02:51 INFO - c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\vs2015u3\VC\include\utility(165): error C2061: syntax error: identifier 'enable_if_t'
01:02:51 INFO - c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\vs2015u3\VC\include\utility(166): error C2988: unrecognizable template declaration/definition
01:02:51 INFO - c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\vs2015u3\VC\include\utility(166): error C2143: syntax error: missing ';' before '&&'
01:02:51 INFO - c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\vs2015u3\VC\include\utility(166): error C2238: unexpected token(s) preceding ';'
01:02:51 INFO - c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\vs2015u3\VC\include\utility(166): error C2059: syntax error: '&&'
01:02:51 INFO - c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\vs2015u3\VC\include\utility(243): error C2238: unexpected token(s) preceding ';'
01:02:51 INFO - c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\vs2015u3\VC\include\utility(249): error C2988: unrecognizable template declaration/definition
01:02:51 INFO - c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\vs2015u3\VC\include\utility(249): error C2059: syntax error: 'if'
01:02:51 INFO - c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\vs2015u3\VC\include\utility(250): error C2334: unexpected token(s) preceding '{'; skipping apparent function body
01:02:51 INFO - c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\vs2015u3\VC\include\utility(255): error C2988: unrecognizable template declaration/definition
01:02:51 INFO - c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\vs2015u3\VC\include\utility(255): error C2143: syntax error: missing ';' before '}'
01:02:51 INFO - c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\vs2015u3\VC\include\utility(255): fatal error C1903: unable to recover from previous error(s); stopping compilation
01:02:51 INFO - c:/builds/moz2_slave/autoland-w32-00000000000000000/build/src/config/rules.mk:999: recipe for target 'Unified_cpp_layout_style4.obj' failed
Flags: needinfo?(manishearth)
Assignee | ||
Comment 113•8 years ago
|
||
Yeah, investigating now. Seems like something caused parsing of the header files to go awry.
Flags: needinfo?(manishearth)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 122•8 years ago
|
||
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a0c56ecc9111
Part 1: stylo: Add stubbed-out ServoSpecifiedValues interface, use it for pres attr mapping; r=bz,emilio
https://hg.mozilla.org/integration/autoland/rev/28d72f228244
Part 2: stylo: Add necessary stubbed-out bindings for ServoSpecifiedValues; r=bz,emilio
https://hg.mozilla.org/integration/autoland/rev/7f7bb42ecf53
Part 3: stylo: Support lang property; r=emilio
https://hg.mozilla.org/integration/autoland/rev/4e75e0178a87
Part 4: stylo: Update reftest results; r=emilio
Comment 123•8 years ago
|
||
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ec0d13708251
Fix reftest manifest format; r=bholley
Comment 124•8 years ago
|
||
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f4c8bd8aa36f
Make stylo build green; r=bholley
Comment 125•8 years ago
|
||
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b41c2c9d9c2b
Update assertion counts for stylo mochitest.
Comment 126•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a0c56ecc9111
https://hg.mozilla.org/mozilla-central/rev/28d72f228244
https://hg.mozilla.org/mozilla-central/rev/7f7bb42ecf53
https://hg.mozilla.org/mozilla-central/rev/4e75e0178a87
https://hg.mozilla.org/mozilla-central/rev/ec0d13708251
https://hg.mozilla.org/mozilla-central/rev/f4c8bd8aa36f
https://hg.mozilla.org/mozilla-central/rev/b41c2c9d9c2b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•