Closed Bug 1330041 Opened 7 years ago Closed 7 years ago

stylo: sort out a plan for HTML presentational attributes

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
firefox53 --- affected
firefox54 --- fixed

People

(Reporter: bzbarsky, Assigned: manishearth)

References

Details

Attachments

(1 file)

Bug 1330041 - Basic handling framework for presentation attributes in Stylo, with handling for font-size and color;
59 bytes, text/x-review-board-request
bzbarsky
: review+
emilio
: review+
Details 
This is basically about the various WalkContentStyleRules methods for things that are not SVG.  The SVG case is being handled in bug 1329093 and possibly its followups and is a bit more straightforward.

The problem is how (or whether) to address this without duplicating all the stuff in all the Gecko attribute mapping functions to work on some datastructures other than the ones it's working on right now.
For somewhat similar things we use to wrap a Stylo-aware object in a Gecko-like interface (see, for example, the FakeStyleContext thing[1] we use for change hint processing).

This is slightly more complicated because it's a lot of code spread around and slightly more tied to nsCSSValue than what we'd want I guess, but depending on how much perf impact this may have we may want to do a nsRuleData -> ServoPropertyDeclarationBlock conversion after the mapping is done, which would allow sharing most of the code I think (from a quick read and my probably limited understanding from that read of how attribute mapping works in Gecko).

[1]: http://searchfox.org/mozilla-central/source/layout/style/nsStyleContext.cpp#1230
Well, also if we share that code we'd need to audit it for thread safety, but from a quick look it doesn't seem like too much thread-unsafe things are going on (probably the image stuff is the more tricky?)
Blocks: stylo-nightly
bz and I talked today. The mapped attribute setup has a few purposes:
* To coalesce the storage of all style-affecting attributes into the same place, so that we can quickly skip that stuff if the data structure (nsMappedAttributes) is empty.
* Precomputing and persisting the rule node representing those presentational attributes, so that we don't need to synthesize it every restyle.
* Sharing storage of mapped attributes across DOM nodes, since apparently some legacy pages do font="TIMES" on every <p> element in the page. The nsMappedAttributes are cached in a central place, and copy-on-write.

I think we should probably leverage this machinery for stylo. The easiest thing to do is probably to store a ServoDeclarationBlock on the nsMappedAttributes structure. Boris suggested synthesizing the ServoDeclarationBlock as lazily as possible. We can't do it during the parallel traversal (since that would be a hazard), but we can at least wait until we're about to traverse to do it (via the central storage mechanism mentioned above). Since that declaration block will presumably go into the rule tree, we'll want to Arc it.

Boris also mentioned that there are few cases of presentational attributes that are not in nsMappedAttrs which we'll need to handle explicitly - basically all the custom implementations of WalkContentStyleRules that aren't the one on nsMappedAttributeElement.

Who's got cycles to take this one? Manish? Emilio?
I was actually thinking of working on bug 1329088 (which is a related to this) this week :)

I have a couple other things to finish up first, but this seems doable.

I haven't yet had a thorough look at the code (and will probably have more questions when I get there), but is there a reason we want to make the SDB generation lazy? AIUI we're going to be generating it anyway, and both situations are on the main thread.

I guess this decl block will be picked up by Servo the same way `style` attr rules are, by querying the element?
> but is there a reason we want to make the SDB generation lazy?

Yes.  That way if some stupid page sets the "size" attribute on a <font> element 8 times in a row, you won't do the work 8 times.
Sets it from script, that is.  Obviously if it's that way in the markup that won't matter much.
Assignee: nobody → manishearth
So I investigated and discussed this some more.

One of the issues is that currently the mapped attributes are cached on nsHTMLStyleSheet, which we want to eventually get rid of for Stylo (it's not used). We could move it out, but stylesheet is pretty well-rooted into the code, so I'd prefer to defer things like this if possible. We can eventually replace it with a base class containing common code with a derived class for what nsHTMLStyleSheet is today.

Another, more pressing issue is that the MapAttributesIntoRule functions operate on nsRuleData. They currently work by manually checking if some properties are set (by fetching their values and checking for nullness), parsing properties directly, and then copying those parsed values into the nsRuleData struct.


We'll need to make this code generic over specified value types. As far as I can tell, the only operations these structs *need* are "Is property <id> set?" and "Set value of property <id> to result of parsing <string>".

We can do this abstraction via virtual dispatch, or via templates. The advantage of templates is that, well, they're not virtual dispatch :), and in case we need more operations than the two listed above, we can just specialize (but that can be hacked together with abstract pointers too). The advantage of virtual dispatch is that we're already using virtual dispatch here (nsMappedAttributes stores a function pointer), and virtual dispatch of a template function won't work. We could just store two pointers, though, and use templates. Virtual dispatch would minimize the amount of code we need to change. I'm not yet sure which method to take here, but will decide on one after going through the existing instances of nsMappedAttributes. We can always switch later if we need to.
 

The rough plan for this work is:

 - Design aforementioned generic interface for MapAttributesIntoRule
 - Rewrite all MapAttributesIntoRules to work with this.
 - Stash a lazy RawServoDeclarationBlock on nsMappedAttributes
 - Have a call that computes the RawSDB on the mapped attributes, call it before Servo_ResolveStyleLazily() instances (http://logs.glob.uno/?c=mozilla%23servo&s=18+Jan+2017&e=18+Jan+2017#c595630)
 - Make way for RawGeckoElement to query Gecko for this RawSDB much like what we do with style attrs right now. Insert it into the cascade the same way we do with style attrs.

 - (Optional) Abstract out common elements of nsHTMLStyleSheet
 - (Optional) replace the cache PLDHashTable with a typesafe RefPtrHashTable

Let me know if you have any thoughts on this.
I thought the plan was to keep the existing attr mapping code as-is for now, but write a thing that takes as input an nsRuleData and outputs a RawServoDeclarationBlock?
> I thought the plan was to keep the existing attr mapping code as-is for now, but write a thing that takes as input an nsRuleData and outputs a RawServoDeclarationBlock?

That's going to be tricky due to the impedance mismatch between nsCSSValue and PropertyDeclaration. nsCSSValue is basically a validated CSS AST, whereas PropertyDeclaration is a strongly typed tagged union with a different variant for each property, and often a different inner type for each property (except for stuff like lengths and whatever).

I don't want to teach Gecko how to write to PropertyDeclaration. That seems like it will require a heap of binding functions.

It might be possible to teach Servo to read from nsCSSValue. I'm ... not very happy with that solution. But we need it anyway for @media rules (https://reviewboard.mozilla.org/r/104760/diff/2#index_header), and are already using it for transform computed values.

I basically am not fond of having transient code that further marries stylo to a part of the Gecko stack that we don't really want running in Stylo mode. I especially don't like having two different parsers running.

However, in the spirit of a minimal solution I guess we can translate the ruledata.
> nsCSSValue is basically a validated CSS AST

Are we looking at the same nsCSSValue class?  Because as far as I can tell, it's a tagged union with a different inner type for each type of value it can be.

It doesn't include the _property_ it's a value for, if that's the issue.

> I don't want to teach Gecko how to write to PropertyDeclaration.

Well, ok, but something is going to need to create PropertyDeclarations out of data Gecko has (attribute values).

> I especially don't like having two different parsers running.

I'm not sure what two parsers you mean here.
> Because as far as I can tell, it's a tagged union with a different inner type for each type of value it can be.

Right, it is. It's an AST because everything is represented in a way that maps to the original syntax, e.g functions are represented as |<function node> (<arg node 1>, <arg node 2>)|, not in a more strongly typed manner. Anyway, not very relevant.

> Well, ok, but something is going to need to create PropertyDeclarations out of data Gecko has (attribute values).

The idea was to provide a common API to both nsRuleData and PropertyDeclarationBlock that lets you say "Take string <str> and parse it for property <id> and store the result". However, on further inspection, we need a better API than that, because sometimes the code is setting values directly (e.g. http://searchfox.org/mozilla-central/source/dom/html/HTMLHRElement.cpp#94)

So I guess I'll just write some code to convert nsRuleData and nsCSSValues into PDBs.

> I'm not sure what two parsers you mean here.

The Gecko parser and the Servo parser. The Gecko one is used here.
> So I guess I'll just write some code to convert nsRuleData and nsCSSValues into PDBs.

OK.  Note that for this use case you don't have to support anything resembling the full range of possible nsCSSValues, which should let things be a bit saner.

> The Gecko parser and the Servo parser.

I assume you mean attribute value parser?
> Note that for this use case you don't have to support anything resembling the full range of possible nsCSSValues

Yeah, I'm still going through the values but so far that seems to be the case.


> I assume you mean attribute value parser?

No, nsCSSParser. I'm okay with the attribute parser :)

http://searchfox.org/mozilla-central/source/dom/html/HTMLFontElement.cpp#68
Oh, ick.  I didn't realize we reused the CSS parser for part of this stuff.  :(
I think some presentational attributes (bgcolor comes to my mind) use a different parsing than the property, in case it matters.
Comment on attachment 8829660 [details]
Bug 1330041 - Basic handling framework for presentation attributes in Stylo, with handling for font-size and color;

https://reviewboard.mozilla.org/r/106662/#review107678

::: servo/ports/geckolib/glue.rs:908
(Diff revision 1)
>  
>  #[no_mangle]
> +pub extern "C" fn Servo_DeclarationBlock_AddPresValue(declarations: RawServoDeclarationBlockBorrowed,
> +                                                      property: nsCSSPropertyID,
> +                                                      css_value: nsCSSValueBorrowedMut) {
> +    use style::properties::{DeclaredValue, LonghandId, PropertyDeclaration, PropertyId, longhands};

This probably should be moved into a method in the style crate.
Comment on attachment 8829660 [details]
Bug 1330041 - Basic handling framework for presentation attributes in Stylo, with handling for font-size and color;

https://reviewboard.mozilla.org/r/106662/#review107682

mostly-superficial review by Manish's request, mostly nits and questions, but I needed to sleep. Hopefully some of them help ;)

::: dom/base/Element.cpp:2027
(Diff revision 1)
>  
>    return nullptr;
>  }
>  
> +const
> +nsMappedAttributes*

nit: I believe const should be in the same type as nsMappedAttributes.

::: dom/base/nsAttrAndChildArray.cpp:781
(Diff revision 1)
> +nsMappedAttributes*
> +nsAttrAndChildArray::GetMapped() const {
> +  if (mImpl) {
> +    return mImpl->mMappedAttrs;
> +  } else {
> +    return nullptr;

nit: you can omit the `else` branch, or do something like:

return mImpl ? mImpl->mMappedAttrs : nullptr;

::: dom/base/nsMappedAttributes.h:79
(Diff revision 1)
>  
> +  void LazilyResolveServoDeclaration(nsRuleData* aRuleData,
> +                                     nsCSSPropertyID* aIndexToIdMapping,
> +                                     size_t aRuleDataSize);
> +
> +  const RefPtr<RawServoDeclarationBlock>& ServoStyle() const {

nit: I think even in headers we're supposed to put the brace in the same line, but this is not followed in a bunch of places so...

::: dom/base/nsMappedAttributes.cpp:65
(Diff revision 1)
>  {
>    uint32_t extra = aWillAddAttr ? 1 : 0;
>  
>    // This will call the overridden operator new
> -  return new (mAttrCount + extra) nsMappedAttributes(*this);
> +  nsMappedAttributes* ret = new (mAttrCount + extra) nsMappedAttributes(*this);
> +  ret->mServoStyle = nullptr;

Why not doing this reset in the copy constructor (with a comment like `// lazily resolved`?)

::: dom/base/nsMappedAttributes.cpp:293
(Diff revision 1)
>  }
>  
> +void
> +nsMappedAttributes::LazilyResolveServoDeclaration(nsRuleData* aRuleData,
> +                                                  nsCSSPropertyID* aTndexToIdMapping,
> +                                                  size_t aRuleDataSize) {

nit: brace

::: dom/base/nsMappedAttributes.cpp:296
(Diff revision 1)
> +nsMappedAttributes::LazilyResolveServoDeclaration(nsRuleData* aRuleData,
> +                                                  nsCSSPropertyID* aTndexToIdMapping,
> +                                                  size_t aRuleDataSize) {
> +  MapRuleInfoInto(aRuleData);
> +
> +  if (mServoStyle.get()) {

I think a comment about this early-return is well worth it.

Is the idea that if we've already computed this it's going to be valid until we clear it? If so, when is it cleared? Can we avoid the `MapRuleInfoInto` call if this is not null?

::: layout/style/ServoStyleSet.cpp:162
(Diff revision 1)
>                              aPseudoType, Move(aComputedValues), skipFixup);
>  }
>  
> +void
> +ServoStyleSet::ResolveMappedAttrsLazily() {
> +  nsHTMLStyleSheet* sheet = mPresContext->Document()->GetAttributeStyleSheet();

nit: you can do `if (nsHTMLStyleSheet* sheet = ...)` (if it doesn't exceed the line length limit).

::: layout/style/nsHTMLStyleSheet.cpp:557
(Diff revision 1)
>    NS_ASSERTION(entryCount == mMappedAttrTable.EntryCount(), "not removed");
>  }
>  
> +// Struct containing once-initialized information about
> +// our synthesized rule data
> +struct StaticRuleDataInfo {

nit: brace

::: layout/style/nsHTMLStyleSheet.cpp:572
(Diff revision 1)
> +  // of alloca
> +  nsCSSValue* dataStorage;
> +};
> +
> +static void
> +CalculateIndexArray(StaticRuleDataInfo* aInfo) {

nit: brace

::: layout/style/nsHTMLStyleSheet.cpp:590
(Diff revision 1)
> +  #undef CSS_PROP
> +  #undef CSS_PROP_LOGICAL
> +}
> +
> +static StaticRuleDataInfo
> +CalculateRuleDataInfo() {

nit: brace

::: layout/style/nsHTMLStyleSheet.cpp:627
(Diff revision 1)
> +                        mDocument->GetShell()->GetPresContext(), nullptr);
> +    // Copy the offsets; ruleData won't know where to find properties otherwise
> +    std::copy(std::begin(sizes.offsets),
> +              std::end(sizes.offsets),
> +              std::begin(ruleData.mValueOffsets));
> +    attr->mAttributes->LazilyResolveServoDeclaration(&ruleData,

So as far as I know this is useless right now if it has been computed already. Can we assume that if it's computed is valid? (This is related to my previous question about the early-return).

If so, we might want to do that check earlier and assert in `LazilyResolveServoDeclaration` instead of copying then checking?

I'd need to check a lot more about how the attributes are managed to fully understand this though (but I need to sleep now).

::: layout/style/nsRuleNode.h:1098
(Diff revision 1)
> + * allocation is the life of the calling function, the caller must call
> + * alloca.  However, to ensure that constructors and destructors are
> + * balanced, we do the constructor and destructor calling from this RAII
> + * class, AutoCSSValueArray.
> + */
> +struct AutoCSSValueArray {

nit: brace

::: layout/style/nsRuleNode.cpp:2357
(Diff revision 1)
>      if ((flagData[i] & aFlags) != aFlags)
>        values[i].Reset();
>    }
>  }
>  
> -/**
> +AutoCSSValueArray::AutoCSSValueArray(void* aStorage, size_t aCount) {

nit: brace (though not your fault, you may want to fix while you're here).

::: servo/components/style/values/specified/length.rs:337
(Diff revision 1)
>          })
>      }
> +    /// https://drafts.csswg.org/css-fonts-3/#font-size-prop
> +    pub fn from_font_size_int(i: u8) -> Length {
> +        match i {
> +            x if x <= 1 => Length::Absolute(Au::from_px(FONT_MEDIUM_PX) * 3 / 4),

just use `0 | 1`?

::: servo/ports/geckolib/glue.rs:914
(Diff revision 1)
> +    use style::values::specified;
> +    use style::gecko::values::convert_nscolor_to_rgba;
> +    let declarations = RwLock::<PropertyDeclarationBlock>::as_arc(&declarations);
> +    let prop = PropertyId::from_nscsspropertyid(property);
> +
> +    if let Ok(PropertyId::Longhand(long)) = prop {

nit: early-return instead in the relevant places, so this isn't indented five times to the right.
Comment on attachment 8829660 [details]
Bug 1330041 - Basic handling framework for presentation attributes in Stylo, with handling for font-size and color;

Given that he's already looked at it, I'll let emilio handle the servo side of the review, and bz's the right person to review the gecko stuff.
Attachment #8829660 - Flags: review?(bobbyholley) → review?(emilio+bugs)
Comment on attachment 8829660 [details]
Bug 1330041 - Basic handling framework for presentation attributes in Stylo, with handling for font-size and color;

https://reviewboard.mozilla.org/r/106662/#review107700

::: layout/style/ServoBindings.cpp:335
(Diff revision 1)
> +  return reinterpret_cast<const RawServoDeclarationBlockStrong*>
> +    (&attrs->ServoStyle());

nit: It's probably better adding a static_asssert somewhere that `RefPtr<RawServoDeclarationBlock>` has identical size to `RawServoDeclarationBlockStrong*`.
Comment on attachment 8829660 [details]
Bug 1330041 - Basic handling framework for presentation attributes in Stylo, with handling for font-size and color;

https://reviewboard.mozilla.org/r/106662/#review108090

::: dom/base/nsMappedAttributes.cpp:65
(Diff revision 1)
>  {
>    uint32_t extra = aWillAddAttr ? 1 : 0;
>  
>    // This will call the overridden operator new
> -  return new (mAttrCount + extra) nsMappedAttributes(*this);
> +  nsMappedAttributes* ret = new (mAttrCount + extra) nsMappedAttributes(*this);
> +  ret->mServoStyle = nullptr;

The struct is shared between elements, so the copy ctor should bump the refcount. Clone should not, since Clone creates a new unique copy.

::: dom/base/nsMappedAttributes.cpp:296
(Diff revision 1)
> +nsMappedAttributes::LazilyResolveServoDeclaration(nsRuleData* aRuleData,
> +                                                  nsCSSPropertyID* aTndexToIdMapping,
> +                                                  size_t aRuleDataSize) {
> +  MapRuleInfoInto(aRuleData);
> +
> +  if (mServoStyle.get()) {

Never cleared until the whole type is dropped. Yes, that call can be avoided.

::: layout/style/ServoBindings.cpp:335
(Diff revision 1)
> +  return reinterpret_cast<const RawServoDeclarationBlockStrong*>
> +    (&attrs->ServoStyle());

That's not what I'm casting between, I'm pointer-casting `RefPtr<RawServoDeclarationBlock>*` to `RawServoDeclarationBlockStrong*`.

Should I use a different cast function here?

::: layout/style/ServoStyleSet.cpp:162
(Diff revision 1)
>                              aPseudoType, Move(aComputedValues), skipFixup);
>  }
>  
> +void
> +ServoStyleSet::ResolveMappedAttrsLazily() {
> +  nsHTMLStyleSheet* sheet = mPresContext->Document()->GetAttributeStyleSheet();

I'm not fond of that pattern, but okay :)

::: layout/style/nsHTMLStyleSheet.cpp:627
(Diff revision 1)
> +                        mDocument->GetShell()->GetPresContext(), nullptr);
> +    // Copy the offsets; ruleData won't know where to find properties otherwise
> +    std::copy(std::begin(sizes.offsets),
> +              std::end(sizes.offsets),
> +              std::begin(ruleData.mValueOffsets));
> +    attr->mAttributes->LazilyResolveServoDeclaration(&ruleData,

> Can we assume that if it's computed is valid?

Yes, I think.

> If so, we might want to do that check earlier and assert

good idea
Comment on attachment 8829660 [details]
Bug 1330041 - Basic handling framework for presentation attributes in Stylo, with handling for font-size and color;

https://reviewboard.mozilla.org/r/106662/#review108198

Servo parts look ok to me % comments.

I've tried to give feedback in the gecko side, but bz should definitely review it.

::: dom/base/nsAttrAndChildArray.cpp:776
(Diff revision 5)
>  
>    return NS_OK;
>  }
>  
> +const
> +nsMappedAttributes*

nit: const nsMappedAttributes in the same line

::: dom/base/nsMappedAttributes.h:79
(Diff revision 5)
>  
> +  void LazilyResolveServoDeclaration(nsRuleData* aRuleData,
> +                                     nsCSSPropertyID* aIndexToIdMapping,
> +                                     size_t aRuleDataSize);
> +
> +  const RefPtr<RawServoDeclarationBlock>& ServoStyle() const

You may want to call this GetServoStyle, to indicate it can return null.

And please, if you can't assert statically about the return value please leave a huge comment here.

::: dom/base/nsMappedAttributes.h:84
(Diff revision 5)
> +  const RefPtr<RawServoDeclarationBlock>& ServoStyle() const
> +  {
> +    return mServoStyle;
> +  }
> +
>    // nsIStyleRule 

Given you've also remove the trailing whitespace above, and while you're here, getting rid of this one shouldn't hurt either.

::: dom/base/nsMappedAttributes.cpp:65
(Diff revision 5)
>  {
>    uint32_t extra = aWillAddAttr ? 1 : 0;
>  
>    // This will call the overridden operator new
> -  return new (mAttrCount + extra) nsMappedAttributes(*this);
> +  nsMappedAttributes* ret = new (mAttrCount + extra) nsMappedAttributes(*this);
> +  ret->mServoStyle = nullptr;

Oh, right, in that case, please comment to that effect about mServoStyle?

::: layout/style/ServoBindings.cpp:31
(Diff revision 5)
>  #include "nsRuleNode.h"
>  #include "nsString.h"
>  #include "nsStyleStruct.h"
>  #include "nsStyleUtil.h"
>  #include "nsTArray.h"
> +#include "nsMappedAttributes.h"

nit: Up a few lines to keep it sorted.

::: layout/style/ServoBindings.cpp:336
(Diff revision 5)
> +  const nsMappedAttributes* attrs = aElement->GetMappedAttributes();
> +  if (!attrs) {
> +    return nullptr;
> +  }
> +  return reinterpret_cast<const RawServoDeclarationBlockStrong*>
> +    (&attrs->ServoStyle());

Add a comment to the effect that `ServoStyle` returns a reference to a refptr, given it's a fairly uncommon pattern.

Could we static-assert it somehow? maybe with `decltype` is not too hard... I'm wary of someone in the future changing it to a weak pointer or anything , and then we have a nasty bug in other part of the codebase.

::: layout/style/ServoStyleSet.h:194
(Diff revision 5)
>                                                nsStyleContext* aParentContext,
>                                                nsIAtom* aPseudoTag,
>                                                CSSPseudoElementType aPseudoType,
>                                                LazyComputeBehavior aMayCompute);
>  
> +  void ResolveMappedAttrsLazily();

Same regarding docs.

::: layout/style/ServoStyleSet.cpp:525
(Diff revision 5)
>    // Restyle the document from the root element and each of the document level
>    // NAC subtree roots.
>    DocumentStyleRootIterator iter(mPresContext->Document());
>    while (Element* root = iter.GetNextStyleRoot()) {
>      if (root->ShouldTraverseForServo()) {
> +      ResolveMappedAttrsLazily();

Why doing this for each root? Doing this at the top should be enough.

::: layout/style/nsHTMLStyleSheet.h:65
(Diff revision 5)
>  
>    // Mapped Attribute management methods
>    already_AddRefed<nsMappedAttributes>
>      UniqueMappedAttributes(nsMappedAttributes* aMapped);
>    void DropMappedAttributes(nsMappedAttributes* aMapped);
> +  void CalculateMappedServoDeclarations();

Please add a comment saying what this does, when it's called, and how do we use it?

::: layout/style/nsHTMLStyleSheet.cpp:560
(Diff revision 5)
> +// Struct containing once-initialized information about
> +// our synthesized rule data
> +struct StaticRuleDataInfo
> +{
> +  // the bitmask used.
> +  uint64_t mask;

use Gecko syntax: `mMask` and so on.

::: layout/style/nsHTMLStyleSheet.cpp:618
(Diff revision 5)
> +nsHTMLStyleSheet::CalculateMappedServoDeclarations()
> +{
> +  // avoid recalculating or reallocating
> +  static StaticRuleDataInfo sizes = CalculateRuleDataInfo();
> +
> +  for (PLDHashTable::Iterator iter(&mMappedAttrTable); !iter.Done(); iter.Next()) {

I expect this table may become somewhat huge, is there a way to short-circuit this without iterating it? What does Gecko do?

::: servo/components/style/gecko_bindings/bindings.rs:502
(Diff revision 5)
>  extern "C" {
>      pub fn Gecko_GetServoDeclarationBlock(element: RawGeckoElementBorrowed)
>       -> RawServoDeclarationBlockStrongBorrowedOrNull;
>  }
>  extern "C" {
> +    pub fn Gecko_GetPresServoDeclarationBlock(element: RawGeckoElementBorrowed)

I wouldn't mind a longer name for this function, `PresServoDeclarationBlock` doesn't evoke presentation attributes to me.

::: servo/ports/geckolib/glue.rs:914
(Diff revision 5)
> +    use style::values::specified;
> +    use style::gecko::values::convert_nscolor_to_rgba;
> +    let declarations = RwLock::<PropertyDeclarationBlock>::as_arc(&declarations);
> +    let prop = PropertyId::from_nscsspropertyid(property);
> +
> +    let long = if let Ok(PropertyId::Longhand(long)) = prop {

nit: You save a line using `match` instead of if let, here and below.

::: servo/ports/geckolib/glue.rs:934
(Diff revision 5)
> +            } else {
> +                return
> +            }
> +        }
> +        LonghandId::Color => {
> +            if let Some(color) = css_value.color_value() {

Should we just `expect`? (In the media query code I just `debug_assert!`, if we can we should try to be consistent).

::: servo/ports/geckolib/glue.rs:945
(Diff revision 5)
> +                ))
> +            } else {
> +                return
> +            }
> +        }
> +        _ => return

Can we print an error! instead? Everything that arrives here is an property we'd need to eventually handle right?
Attachment #8829660 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8829660 [details]
Bug 1330041 - Basic handling framework for presentation attributes in Stylo, with handling for font-size and color;

https://reviewboard.mozilla.org/r/106662/#review108198

> You may want to call this GetServoStyle, to indicate it can return null.
> 
> And please, if you can't assert statically about the return value please leave a huge comment here.

Assert what statically?

> Add a comment to the effect that `ServoStyle` returns a reference to a refptr, given it's a fairly uncommon pattern.
> 
> Could we static-assert it somehow? maybe with `decltype` is not too hard... I'm wary of someone in the future changing it to a weak pointer or anything , and then we have a nasty bug in other part of the codebase.

just added an explicit type annotation

> Same regarding docs.

(will do, but this is a private function)

> Why doing this for each root? Doing this at the top should be enough.

@bholley mentioned that we should do it for each call to traverse? Maybe I misinterpreted it.

> I expect this table may become somewhat huge, is there a way to short-circuit this without iterating it? What does Gecko do?

Gecko doesn't iterate it. Gecko's model is completely different; it resolves this when it encounters such a node. I don't think we can do the mapping during the parallel style traversal.

> nit: You save a line using `match` instead of if let, here and below.

IIRC if let is almost always preferred, but w/e

> Should we just `expect`? (In the media query code I just `debug_assert!`, if we can we should try to be consistent).

I don't really like introducing unnecessary crashes.
I could short circuit it by adding a dirty flag on the stylesheet itself.
Added a dirty flag, and also added a PrepareAndTraverseSubtree() helper call where all lazy resolutions can go (bholley intends to add more)
It occurs to me that I shouldn't be sticking the backing allocation for aRuleData in a static array (since multiple tabs may use the same one); instead it should be cached on nsHTMLStyleSheet or something.
Depends on: 1334330
I'm really sorry for the lag here.  I will try to get to this on Monday...
bug 1334330 has the patches for the next step (abstractification), and once both of these land we can switch from the hacky synthesized nsRuleData to a proper Servo interface implementing GenericSpecifiedValues.
Comment on attachment 8829660 [details]
Bug 1330041 - Basic handling framework for presentation attributes in Stylo, with handling for font-size and color;

https://reviewboard.mozilla.org/r/106662/#review108442

::: dom/base/nsMappedAttributes.h:79
(Diff revision 5)
>  
> +  void LazilyResolveServoDeclaration(nsRuleData* aRuleData,
> +                                     nsCSSPropertyID* aIndexToIdMapping,
> +                                     size_t aRuleDataSize);
> +
> +  const RefPtr<RawServoDeclarationBlock>& ServoStyle() const



::: layout/style/ServoStyleSet.cpp:525
(Diff revision 5)
>    // Restyle the document from the root element and each of the document level
>    // NAC subtree roots.
>    DocumentStyleRootIterator iter(mPresContext->Document());
>    while (Element* root = iter.GetNextStyleRoot()) {
>      if (root->ShouldTraverseForServo()) {
> +      ResolveMappedAttrsLazily();



::: dom/base/nsMappedAttributes.cpp:300
(Diff revision 8)
> +nsMappedAttributes::LazilyResolveServoDeclaration(nsRuleData* aRuleData,
> +                                                  nsCSSPropertyID* aTndexToIdMapping,
> +                                                  size_t aRuleDataSize)
> +{
> +  MapRuleInfoInto(aRuleData);
> +

assert mServoStyle is null?

::: layout/style/ServoBindings.cpp:335
(Diff revision 8)
> +{
> +  const nsMappedAttributes* attrs = aElement->GetMappedAttributes();
> +  if (!attrs) {
> +    return nullptr;
> +  }
> +  const RefPtr<RawServoDeclarationBlock>* servo = &attrs->GetServoStyle();

I not totally sure on this, but I'm not sure this is enough to prevent a misuse of this method, I think annotating it as a reference is though:

```
const RefPtr<RawServoDeclarationBlock>& servo = attrs->GetServoStyle();
return reinterpret_cast<const RawServoDeclarationBlockStront*>(&servo);
```

::: layout/style/ServoStyleSet.cpp:169
(Diff revision 8)
> +  }
> +}
> +
> +void
> +ServoStyleSet::PrepareAndTraverseSubtree(RawGeckoElementBorrowed aRoot,
> +                                         RawServoStyleSetBorrowed aSet,

let's remove `aSet`? You should be able to use `mRawSet` here.
Comment on attachment 8829660 [details]
Bug 1330041 - Basic handling framework for presentation attributes in Stylo, with handling for font-size and color;

https://reviewboard.mozilla.org/r/106662/#review109446

r=me, but please get followup bugs filed on the font size mapping stuff I discuss below...

::: dom/base/nsAttrAndChildArray.cpp:776
(Diff revision 9)
>  
>    return NS_OK;
>  }
>  
> +const nsMappedAttributes*
> +nsAttrAndChildArray::GetMapped() const {

'{ on following line, please.

::: dom/base/nsMappedAttributes.h:75
(Diff revision 9)
>    void RemoveAttrAt(uint32_t aPos, nsAttrValue& aValue);
>    const nsAttrName* GetExistingAttrNameFromQName(const nsAString& aName) const;
>    int32_t IndexOfAttr(nsIAtom* aLocalName) const;
> -  
>  
> -  // nsIStyleRule 
> +  void LazilyResolveServoDeclaration(nsRuleData* aRuleData,

This needs documentation.  What do the arguments actually mean?

::: dom/base/nsMappedAttributes.h:79
(Diff revision 9)
>  
> -  // nsIStyleRule 
> +  void LazilyResolveServoDeclaration(nsRuleData* aRuleData,
> +                                     nsCSSPropertyID* aIndexToIdMapping,
> +                                     size_t aRuleDataSize);
> +
> +  const RefPtr<RawServoDeclarationBlock>& GetServoStyle() const

This needs documentation too.  Can the returned thing be null?  When would one call this function?

::: dom/base/nsMappedAttributes.cpp:35
(Diff revision 9)
>  
>  nsMappedAttributes::nsMappedAttributes(const nsMappedAttributes& aCopy)
>    : mAttrCount(aCopy.mAttrCount),
>      mSheet(aCopy.mSheet),
> -    mRuleMapper(aCopy.mRuleMapper)
> +    mRuleMapper(aCopy.mRuleMapper),
> +    mServoStyle(aCopy.mServoStyle)

So...  This constructor is _only_ called from Clone() (and is protected, so there won't be any accidental new callers).  And Clone() will then go and null out mServoStyle.  So it might make more sense to just set mServoStyle to null in the copy constructor (with a comment explaining why) instead of the extra refcounting here.

Then again the refcounting is probably not that big a deal, as long as RawServoDeclarationBlock doesn't have threadsafe refcounting or something... does it?

::: dom/base/nsMappedAttributes.cpp:66
(Diff revision 9)
>    uint32_t extra = aWillAddAttr ? 1 : 0;
>  
>    // This will call the overridden operator new
> -  return new (mAttrCount + extra) nsMappedAttributes(*this);
> +  nsMappedAttributes* ret = new (mAttrCount + extra) nsMappedAttributes(*this);
> +  // nsMappedAttributes is shared between elements, so we should be running
> +  // the regular refptr copy constructor when copy constructing, but when

I don't see what this has to do with sharing between elements.  When it's shared, it's shared by all of them referencing the same exact nsMappedAttributes object.

::: dom/base/nsMappedAttributes.cpp:296
(Diff revision 9)
>    return n;
>  }
>  
> +void
> +nsMappedAttributes::LazilyResolveServoDeclaration(nsRuleData* aRuleData,
> +                                                  nsCSSPropertyID* aTndexToIdMapping,

s/Tndex/Index/?  And in the function body too.

::: dom/base/nsMappedAttributes.cpp:301
(Diff revision 9)
> +                                                  nsCSSPropertyID* aTndexToIdMapping,
> +                                                  size_t aRuleDataSize)
> +{
> +  MapRuleInfoInto(aRuleData);
> +
> +  NS_ASSERTION(!mServoStyle,

MOZ_ASSERT, if you really think it holds.  ;)

::: layout/style/ServoBindings.h:159
(Diff revision 9)
>  
>  #undef SERVO_DECLARE_ELEMENT_ATTR_MATCHING_FUNCTIONS
>  
>  // Style attributes.
>  RawServoDeclarationBlockStrongBorrowedOrNull
>  Gecko_GetServoDeclarationBlock(RawGeckoElementBorrowed element);

We should rename this to make it clear that it's about inline style, probably, right?

::: layout/style/ServoBindings.h:161
(Diff revision 9)
>  
>  // Style attributes.
>  RawServoDeclarationBlockStrongBorrowedOrNull
>  Gecko_GetServoDeclarationBlock(RawGeckoElementBorrowed element);
> +RawServoDeclarationBlockStrongBorrowedOrNull
> +Gecko_GetServoPresentationAttrDeclarationBlock(RawGeckoElementBorrowed element);

This one is rather specific to HTML presentation attributes.  For example, SVG will need a different mechanism (not least because it has two different declaration blocks of presentation attributes...)

::: layout/style/ServoBindings.cpp:337
(Diff revision 9)
> +  if (!attrs) {
> +    return nullptr;
> +  }
> +
> +  const RefPtr<RawServoDeclarationBlock>& servo = attrs->GetServoStyle();
> +  return reinterpret_cast<const RawServoDeclarationBlockStrong*>(&servo);

This is rather icky, but I'm not sure I see a better way.  Might be good to copy the static_assert from ServoDeclarationBlock::RefRaw here too.

::: layout/style/ServoStyleSet.h:199
(Diff revision 9)
> +  /**
> +   * Resolve all ServoDeclarationBlocks attached to mapped
> +   * presentation attributes cached on the document.
> +   * Call this before jumping into Servo's style system.
> +   */
> +  void ResolveMappedAttrsLazily();

This seems misnamed.  We're not resolving them _lazily_, but rather eagerly, when this method is called.

Maybe call it ComputeMappedAttrDeclarationBlocks?  Or ResolveMappedAttrDeclarationBlocks?

::: layout/style/ServoStyleSet.cpp:161
(Diff revision 9)
>    return NS_NewStyleContext(aParentContext, mPresContext, aPseudoTag,
>                              aPseudoType, Move(aComputedValues), skipFixup);
>  }
>  
> +void
> +ServoStyleSet::ResolveMappedAttrsLazily() {

'{' on next line.

::: layout/style/ServoStyleSet.cpp:171
(Diff revision 9)
> +
> +void
> +ServoStyleSet::PrepareAndTraverseSubtree(RawGeckoElementBorrowed aRoot,
> +                                         mozilla::TraversalRootBehavior aRootBehavior) {
> +  ResolveMappedAttrsLazily();
> +  Servo_TraverseSubtree(aRoot, mRawSet.get(), aRootBehavior);

We should have a comment on Servo_TraverseSubtree that says to not call it directly and use this function instead, right?

::: layout/style/nsHTMLStyleSheet.cpp:548
(Diff revision 9)
>  
>  void
>  nsHTMLStyleSheet::DropMappedAttributes(nsMappedAttributes* aMapped)
>  {
>    NS_ENSURE_TRUE_VOID(aMapped);
> -
> +  mMappedAttrsDirty = true;

Why do you need this?  Removing an nsMappedAttributes won't require any recalculation.

::: layout/style/nsHTMLStyleSheet.cpp:560
(Diff revision 9)
>    NS_ASSERTION(entryCount == mMappedAttrTable.EntryCount(), "not removed");
>  }
>  
> +// Struct containing once-initialized information about
> +// our synthesized rule data
> +struct StaticRuleDataInfo

Please put this in an anonymous namespace.

::: layout/style/nsHTMLStyleSheet.cpp:575
(Diff revision 9)
> +};
> +
> +static void
> +CalculateIndexArray(StaticRuleDataInfo* aInfo)
> +{
> +  aInfo->mIndexToPropertyMapping = new nsCSSPropertyID[aInfo->mPropCount];

Might be worth a comment about how yes, this is a leak through shutdown, but that's OK.

::: layout/style/nsHTMLStyleSheet.cpp:578
(Diff revision 9)
> +CalculateIndexArray(StaticRuleDataInfo* aInfo)
> +{
> +  aInfo->mIndexToPropertyMapping = new nsCSSPropertyID[aInfo->mPropCount];
> +  size_t structOffset;
> +  size_t propertyIndex;
> +  #define CSS_PROP_LOGICAL(name_, id_, method_, flags_, pref_, parsevariant_, \

Or just use CSS_PROP_LIST_EXCLUDE_LOGICAL like nsRuleData.h does?  I'd prefer that, for consistency.

::: layout/style/nsHTMLStyleSheet.cpp:596
(Diff revision 9)
> +static StaticRuleDataInfo
> +CalculateRuleDataInfo()
> +{
> +  StaticRuleDataInfo sizes;
> +#define STYLE_STRUCT_INHERITED(name, checkdata_cb) \
> +  sizes.mMask |= NS_STYLE_INHERIT_BIT(name); \

This is a bit worrisome.  nsRuleData documentation says:

    ... therefore some code that we
    // know is not called from HasAuthorSpecifiedRules assumes that the
    // mValueOffsets for the one struct in mSIDs is zero.
    
That said, I don't think any of the DOM code would be assuming such things....  so maybe we're OK.  Especially if this is a kinda-temporary mechanism.

::: layout/style/nsHTMLStyleSheet.cpp:596
(Diff revision 9)
> +static StaticRuleDataInfo
> +CalculateRuleDataInfo()
> +{
> +  StaticRuleDataInfo sizes;
> +#define STYLE_STRUCT_INHERITED(name, checkdata_cb) \
> +  sizes.mMask |= NS_STYLE_INHERIT_BIT(name); \

This assumes mMask starts off as 0, but nothing ensures that, afaict.   You probably want to initialize it to 0 right after constructing StaticRuleDataInfo, or give StaticRuleDataInfo a constructor that does that, or something.

::: layout/style/nsHTMLStyleSheet.cpp:597
(Diff revision 9)
> +CalculateRuleDataInfo()
> +{
> +  StaticRuleDataInfo sizes;
> +#define STYLE_STRUCT_INHERITED(name, checkdata_cb) \
> +  sizes.mMask |= NS_STYLE_INHERIT_BIT(name); \
> +  sizes.mOffsets[eStyleStruct_##name] = sizes.mPropCount; \

This assumes mPropCount starts off as 0 too.  Again, nothing in here is guaranteeing that.

::: layout/style/nsHTMLStyleSheet.cpp:599
(Diff revision 9)
> +  StaticRuleDataInfo sizes;
> +#define STYLE_STRUCT_INHERITED(name, checkdata_cb) \
> +  sizes.mMask |= NS_STYLE_INHERIT_BIT(name); \
> +  sizes.mOffsets[eStyleStruct_##name] = sizes.mPropCount; \
> +  sizes.mPropCount += nsCSSProps::PropertyCountInStruct(eStyleStruct_##name);
> +#define STYLE_STRUCT_RESET(name, checkdata_cb) STYLE_STRUCT_INHERITED(name, checkdata_cb)

Why not just define STYLE_STRUCT and let nsStyleStructList define the INHERITED/RESET versions for you?

::: layout/style/nsHTMLStyleSheet.cpp:617
(Diff revision 9)
> +nsHTMLStyleSheet::CalculateMappedServoDeclarations()
> +{
> +  // avoid recalculating or reallocating
> +  static StaticRuleDataInfo sizes = CalculateRuleDataInfo();
> +
> +  if (!mMappedAttrsDirty) {

So the other option is to have a second hashtable in addition to the one we already have and add dirty nsMappedAttributes to it (in addition to adding them to the existing hashtable).  Remove on destruction.  Then walk it and clear it in this function.

That would make cases where a small number of mapped attrs changes in a doc with lots of them be O(# of things that changed), not O(# of things).  On the other hand, it would mean that during initial parsing we may end up with a bit more hashtable traffic...  Hard to say which is worse without doing some measurements.  I think my gut feeling is the extra hashtable traffic is less bad than pathological cases.

::: layout/style/nsHTMLStyleSheet.cpp:622
(Diff revision 9)
> +  if (!mMappedAttrsDirty) {
> +    return;
> +  }
> +  mMappedAttrsDirty = false;
> +
> +  void* dataStorage = alloca(sizes.mPropCount * sizeof(nsCSSValue));

How confident are you that this allocation will not fail?  It's a somewhat bigger allocation than the ones the ruletree ends up doing, since it's for all the structs.

More to the point, its size is fixed and known up front.  Why not just heap-allocate it statically right after you've computed sizes.mPropCount?  It won't be any worse than the existing leaked buffer inside StaticRuleDataInfo, I'm pretty sure. AutoCSSValueArray doesn't _require_ its storage to be alloca-allocated, afaict...  I guess it does have the alignment requirement, though.  Is that the issue?

::: layout/style/nsHTMLStyleSheet.cpp:623
(Diff revision 9)
> +    return;
> +  }
> +  mMappedAttrsDirty = false;
> +
> +  void* dataStorage = alloca(sizes.mPropCount * sizeof(nsCSSValue));
> +  for (PLDHashTable::Iterator iter(&mMappedAttrTable); !iter.Done(); iter.Next()) {

for (auto iter = mMappedAttrTable.Iter(); ... )

::: layout/style/nsHTMLStyleSheet.cpp:625
(Diff revision 9)
> +  mMappedAttrsDirty = false;
> +
> +  void* dataStorage = alloca(sizes.mPropCount * sizeof(nsCSSValue));
> +  for (PLDHashTable::Iterator iter(&mMappedAttrTable); !iter.Done(); iter.Next()) {
> +    MappedAttrTableEntry* attr = static_cast<MappedAttrTableEntry*>(iter.Get());
> +    if (attr->mAttributes->GetServoStyle().get()) {

I don't think  you need the .get().  RefPtr has a perfectly good `operator bool`.

::: layout/style/nsHTMLStyleSheet.cpp:634
(Diff revision 9)
> +    // Construction cleans up any values we may have set
> +    AutoCSSValueArray dataArray(dataStorage, sizes.mPropCount);
> +
> +    // synthesized ruleData
> +    nsRuleData ruleData(sizes.mMask, dataArray.get(),
> +                        mDocument->GetShell()->GetPresContext(), nullptr);

It's worth commenting why it's OK to pass nullptr for that last arg, given that no other nsRuleData ever does that...

::: layout/style/nsHTMLStyleSheet.cpp:636
(Diff revision 9)
> +
> +    // synthesized ruleData
> +    nsRuleData ruleData(sizes.mMask, dataArray.get(),
> +                        mDocument->GetShell()->GetPresContext(), nullptr);
> +    // Copy the offsets; ruleData won't know where to find properties otherwise
> +    std::copy(std::begin(sizes.mOffsets),

This is probably ok, though I think mozilla::PodCopy is what's normally used for cases like this.

::: servo/components/style/values/specified/length.rs:337
(Diff revision 9)
>          })
>      }
> +    /// https://drafts.csswg.org/css-fonts-3/#font-size-prop
> +    pub fn from_font_size_int(i: u8) -> Length {
> +        match i {
> +            0 | 1 => Length::Absolute(Au::from_px(FONT_MEDIUM_PX) * 3 / 4),

Oh, boy.  These sizes are compatible with the CSS sizes servo uses, looks like (which follow the kinda-made-up CSS fonts draft) and are mapped to them per the similarly-made-up HTML spec.  But the result is very much not compatible with what Gecko does right now.  That shouldn't block this patch landing, but if there's no existing stylo blocker filed on this already there needs to be one.  We almost certainly do not want potential web compat fallout from changing this stuff when we switch to stylo.

(Let's not get started on the fact that the CSS spec setup produces lots of non-integer font sizes, even at the default 16px "medium" font size the web uses, which really is not a good idea.  We should probably get spec issues raised too.  And the CSS spec and the HTML spec disagree about how `<font>` sizes get mapped to the CSS values: HTML maps 1 to x-small, but CSS tries to map it to xx-small.  In practice the former, using the CSS definitions would mean "12px" and the latter would mean "9.6px".  Gecko uses "10px" for `<font size="1">` when user-selected default size is 16px, so the CSS spec is closer, probably because they changed the ratios after the HTML spec was written.  But of course it's still not quite right.)

And we need another followup bug on the fact that this setup seems to ignore the user-set preferred font size, instead hardcoding to FONT_MEDIUM_PX, which is hardcoded to 16.  I guess it's possible that updating this to be more like Gecko will fix that too...  Note that the default font size in Gecko is either 16 or 13 depending on the font; there's some weirdness around monospace fonts.  I don't recall how it interacts with the rest of this mess, but I'm 99% sure it does so!

::: servo/ports/geckolib/glue.rs:916
(Diff revision 9)
> +    let declarations = RwLock::<PropertyDeclarationBlock>::as_arc(&declarations);
> +    let prop = PropertyId::from_nscsspropertyid(property);
> +
> +    let long = match prop {
> +        Ok(PropertyId::Longhand(long)) => long,
> +        _ => return

When would this branch get hit?  Is this basically the "property supported by Gecko but not servo" case?  Would it make sense to warn or something if it happens (with the Gecko propid, I guess)?

::: servo/ports/geckolib/glue.rs:929
(Diff revision 9)
> +                            specified::Length::from_font_size_int(int as u8)
> +                        )
> +                    )
> +                ))
> +            } else {
> +                return

Again, should this make noise?

::: servo/ports/geckolib/glue.rs:941
(Diff revision 9)
> +                        parsed: convert_nscolor_to_rgba(color),
> +                        authored: None
> +                    }
> +                ))
> +            } else {
> +                return

Again, noise?
Attachment #8829660 - Flags: review?(bzbarsky) → review+
Oh, and depending on how we decide to handle this font size business, there may be fun interactions with dynamic changes to user-default font size.  But we can figure that out once we know how we're actually representing the numeric font-sizes.
> Especially if this is a kinda-temporary mechanism.

It is, once bug 1334330 lands we can replace it with a better one.

> AutoCSSValueArray doesn't _require_ its storage to be alloca-allocated, afaict...  I guess it does have the alignment requirement, though.  Is that the issue?

Exactly. I tried mallocating it previously. It worked in a static (which isn't threadsafe if there are multiple tabs), but not as a field on the style sheet. It's all temporary so meh.


>  Is this basically the "property supported by Gecko but not servo" case?  Would it make sense to warn or something if it happens (with the Gecko propid, I guess)?

Yes and yes.

> Again, should this make noise?
>
> Again, noise?

Yes. The code is kind of temporary and will be shuffled around later after bug 1334330, but I could add an error!() in there.

> So the other option is to have a second hashtable in addition to the one we already have and add dirty nsMappedAttributes to it (in addition to adding them to the existing hashtable).

I'd rather do this as a followup -- there are a ton of cleanups related to this that we should do; I'm trying for a MVP in this bug so that we can build on it in parallel.
> I'd rather do this as a followup 

That's fine.
Depends on: 1335574
Comment on attachment 8829660 [details]
Bug 1330041 - Basic handling framework for presentation attributes in Stylo, with handling for font-size and color;

https://reviewboard.mozilla.org/r/106662/#review109446

> So...  This constructor is _only_ called from Clone() (and is protected, so there won't be any accidental new callers).  And Clone() will then go and null out mServoStyle.  So it might make more sense to just set mServoStyle to null in the copy constructor (with a comment explaining why) instead of the extra refcounting here.
> 
> Then again the refcounting is probably not that big a deal, as long as RawServoDeclarationBlock doesn't have threadsafe refcounting or something... does it?

It does. Most `RefPtr<ServoThingamajig>`s are.

> This is a bit worrisome.  nsRuleData documentation says:
> 
>     ... therefore some code that we
>     // know is not called from HasAuthorSpecifiedRules assumes that the
>     // mValueOffsets for the one struct in mSIDs is zero.
>     
> That said, I don't think any of the DOM code would be assuming such things....  so maybe we're OK.  Especially if this is a kinda-temporary mechanism.

Yes, we don't go through this code (bug 1334330 basically rewrote all of the code that gets called from here, `HasAuthorSpecifiedRules` isn't part of it). It's temporary.

> So the other option is to have a second hashtable in addition to the one we already have and add dirty nsMappedAttributes to it (in addition to adding them to the existing hashtable).  Remove on destruction.  Then walk it and clear it in this function.
> 
> That would make cases where a small number of mapped attrs changes in a doc with lots of them be O(# of things that changed), not O(# of things).  On the other hand, it would mean that during initial parsing we may end up with a bit more hashtable traffic...  Hard to say which is worse without doing some measurements.  I think my gut feeling is the extra hashtable traffic is less bad than pathological cases.

As mentioned in comment 40 I think it's better to do this as a followup. I also don't want to needlessly make the situation worse for vanilla Firefox.

> How confident are you that this allocation will not fail?  It's a somewhat bigger allocation than the ones the ruletree ends up doing, since it's for all the structs.
> 
> More to the point, its size is fixed and known up front.  Why not just heap-allocate it statically right after you've computed sizes.mPropCount?  It won't be any worse than the existing leaked buffer inside StaticRuleDataInfo, I'm pretty sure. AutoCSSValueArray doesn't _require_ its storage to be alloca-allocated, afaict...  I guess it does have the alignment requirement, though.  Is that the issue?

As mentioned in comment 40 it's an alignment issue. I think I can fix it with the alignment helpers, but this is temporary anyway.
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8accc9d52c25
Basic handling framework for presentation attributes in Stylo, with handling for font-size and color; r=bz,emilio
https://hg.mozilla.org/mozilla-central/rev/8accc9d52c25
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Depends on: 1337695
Depends on: 1338936
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: