Closed Bug 1334330 Opened 3 years ago Closed 3 years ago

stylo: make pres attr mappers operate on abstract containers of specified values

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: manishearth, Assigned: manishearth)

References

Details

Attachments

(11 files)

59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
Next part of bug 330041.

The current (as of bug 330041) code for handling presentation attributes synthesizes nsRuleDatas, threads them through the nsMappedAttributes mappers, and converts them to Servo PropertyDeclarationBlocks.

The mappers should operate directly on PDBs in Servo mode, for this we can introduce an abstract base class that the mappers operate on, which has separate derived classes for nsRuleData and RawServoDeclarationBlock.
Blocks: 1330041, stylo
No longer blocks: 330041
Only need one reviewer on this, and I'd already discussed this with emilio so I'm r?ing him, tagging heycam since we'll need a peer to sign off.

(Unsure if this falls under the style system, most of the code changes are in DOM)
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
Per IRC, my suggestion is to have emilio review and bz super-review all the presattr stuff.
After talking with bholley the plan is to let emilio do the main review and have bz take a quick look afterwards.
Comment on attachment 8831394 [details]
Bug 1334330 - Part 1: stylo: Abstractify nsMappedAttributes to work on arbitrary containers of specified value data;

https://reviewboard.mozilla.org/r/107960/#review109114

::: layout/style/GenericSpecifiedValues.h:9
(Diff revision 4)
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_GenericSpecifiedValues_h
> +#define mozilla_GenericSpecifiedValues_h
> +

Let's add a description of the file so searchfox and dxr and happy.

::: layout/style/GenericSpecifiedValues.h:16
(Diff revision 4)
> +#include "nsCSSValue.h"
> +#include "nsPresContext.h"
> +
> +struct nsRuleData;
> +
> +class GenericSpecifiedValues {

Let's describe what this does and how it's used.

::: layout/style/GenericSpecifiedValues.h:21
(Diff revision 4)
> +class GenericSpecifiedValues {
> +public:
> +    // Check if we already contain a certain longhand
> +    virtual bool PropertyIsSet(nsCSSPropertyID aId) = 0;
> +
> +    virtual nsRuleData* AsRuleData() = 0;

We should try not to take the hit of the virtual calls in non-stylo builds. Actually, we already have this relatively complex system for handles (sorry, I didn't realise when discussing this with you). We can probably reuse it? Alternatively, we could use some ifdef magic to avoid the hit.

Also, since the intention of this is that `AsRuleData` may eventually return null, I guess we should call it `GetAsRuleData`.

::: layout/style/nsRuleData.h:29
(Diff revision 4)
>  class nsStyleContext;
>  struct nsRuleData;
>  
>  typedef void (*nsPostResolveFunc)(void* aStyleStruct, nsRuleData* aData);
>  
> -struct nsRuleData
> +struct nsRuleData : GenericSpecifiedValues

Let's mark this struct as final so calling the virtual method on the concrete type becomes a static call.
Attachment #8831394 - Flags: review?(emilio+bugs)
Comment on attachment 8831395 [details]
Bug 1334330 - Part 2: stylo: Use GenericSpecifiedValue abstraction in nsGenericHTMLElement;

https://reviewboard.mozilla.org/r/107962/#review109116

::: dom/html/nsGenericHTMLElement.cpp:1394
(Diff revision 4)
>    value = aAttributes->GetAttr(nsGkAtoms::hspace);
>    if (value) {
> -    nsCSSValue hval;
> -    if (value->Type() == nsAttrValue::eInteger)
> -      hval.SetFloatValue((float)value->GetIntegerValue(), eCSSUnit_Pixel);
> -    else if (value->Type() == nsAttrValue::ePercent)
> +    if (value->Type() == nsAttrValue::eInteger) {
> +      if (!aData->PropertyIsSet(eCSSProperty_margin_left)) {
> +        aData->SetPixelValue(eCSSProperty_margin_left,
> +                                    (float)value->GetIntegerValue());

nit: Alightnment here and in a lot of these calls is quite odd.

::: dom/html/nsGenericHTMLElement.cpp:1485
(Diff revision 4)
>    
>    nscoord val = 0;
>    if (value->Type() == nsAttrValue::eInteger)
>      val = value->GetIntegerValue();
>  
> -  nsCSSValue* borderLeftWidth = aData->ValueForBorderLeftWidth();
> +  if (!aData->PropertyIsSet(eCSSProperty_border_top_width))

Probably worth adding a `SetIfUnset` function? That would both reduce the amount of virtual calls here, and also make it more readable IMO.

::: layout/style/GenericSpecifiedValues.h:25
(Diff revision 4)
> +    // Check if we are able to hold longhands from a given
> +    // style struct. Pass the result of NS_STYLE_INHERIT_BIT to this
> +    // function
> +    virtual bool CanHoldStyleStruct(uint64_t aInheritBit) = 0;
> +
> +    virtual nsPresContext* GetPresContext() = 0;

This is assumed not to return null in multiple places, let's call it `PresContext`.
Attachment #8831395 - Flags: review?(emilio+bugs)
Comment on attachment 8831394 [details]
Bug 1334330 - Part 1: stylo: Abstractify nsMappedAttributes to work on arbitrary containers of specified value data;

https://reviewboard.mozilla.org/r/107960/#review109114

> We should try not to take the hit of the virtual calls in non-stylo builds. Actually, we already have this relatively complex system for handles (sorry, I didn't realise when discussing this with you). We can probably reuse it? Alternatively, we could use some ifdef magic to avoid the hit.
> 
> Also, since the intention of this is that `AsRuleData` may eventually return null, I guess we should call it `GetAsRuleData`.

It's a temporary method, I'll remove it in the last commit when it stops being used.
Comment on attachment 8831396 [details]
Bug 1334330 - Part 3: stylo: Use GenericSpecifiedValue abstraction in elements using only common mappers;

https://reviewboard.mozilla.org/r/107964/#review109120
Attachment #8831396 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8831397 [details]
Bug 1334330 - Part 4: stylo: Use GenericSpecifiedValue abstraction for <br>;

https://reviewboard.mozilla.org/r/107966/#review109122
Attachment #8831397 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8831398 [details]
Bug 1334330 - Part 5: stylo: Use GenericSpecifiedValue abstraction for <li>,<pre>,<ol>,<ul>,<textarea>;

https://reviewboard.mozilla.org/r/107968/#review109124

::: dom/html/HTMLLIElement.cpp:77
(Diff revision 5)
>  
>  void
>  HTMLLIElement::MapAttributesIntoRule(const nsMappedAttributes* aAttributes,
> -                                     GenericSpecifiedValues* aGenericData)
> +                                     GenericSpecifiedValues* aData)
>  {
> -  nsRuleData* aData = aGenericData->AsRuleData();
> +  if (aData->CanHoldStyleStruct(NS_STYLE_INHERIT_BIT(List))) {

FWIW, I don't think the `CanHoldStyleStruct` name is really appropriate here, according to the comment in `nsRuleData`:

>  // We store nsCSSValues needed to compute the data for one or more
>  // style structs (specified by the bitfield mSIDs).  These are stored
>  // in a single array allocation (which our caller allocates; see
>  // AutoCSSValueArray)

Seems to me that this should be called mostly `ShouldComputeStyleStruct`?
Attachment #8831398 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8831399 [details]
Bug 1334330 - Part 6: stylo: Use GenericSpecifiedValue abstraction for table elements;

https://reviewboard.mozilla.org/r/107970/#review109126

Nice cleanup, though I'd have done it in a different patch for clarity.

::: dom/html/HTMLTableCellElement.cpp:447
(Diff revision 5)
>  HTMLTableCellElement::MapAttributesIntoRule(const nsMappedAttributes* aAttributes,
> -                                            GenericSpecifiedValues* aGenericData)
> +                                            GenericSpecifiedValues* aData)
>  {
> -  nsRuleData* aData = aGenericData->AsRuleData();
> -  if (aData->mSIDs & NS_STYLE_INHERIT_BIT(Position)) {
> +
> +  if (aData->CanHoldStyleStruct(NS_STYLE_INHERIT_BIT(Text))) {
> +    if (!aData->PropertyIsSet(eCSSProperty_white_space)) {

The reordering here makes the diff really noisy, was it necessary?

::: dom/html/HTMLTableCellElement.cpp:454
(Diff revision 5)
> +      if (aAttributes->GetAttr(nsGkAtoms::nowrap)) {
> +        // See if our width is not a nonzero integer width.
> +        const nsAttrValue* value = aAttributes->GetAttr(nsGkAtoms::width);
> +        nsCompatibility mode = aData->PresContext()->CompatibilityMode();
> +        if (!value || value->Type() != nsAttrValue::eInteger ||
> +            value->GetIntegerValue() == 0 ||

I know this is pre-existing, but is this logic right? We're setting `whitespace: nowrap` if the value for the `nowrap` attribute _is_ zero (instead of _isn't_).


Boris, could you confirm this is intended or not? (I think not).

::: dom/html/HTMLTableElement.cpp:862
(Diff revision 5)
> -      if (paddingLeft->GetUnit() == eCSSUnit_Null) {
> -        *paddingLeft = padVal;
> -      }
> -
> -      nsCSSValue* paddingRight = aData->ValueForPaddingRight();
> -      if (paddingRight->GetUnit() == eCSSUnit_Null) {
> +        aData->SetPixelValue(eCSSProperty_padding_top, pad);
> +      if (!aData->PropertyIsSet(eCSSProperty_padding_right))
> +        aData->SetPixelValue(eCSSProperty_padding_right, pad);
> +      if (!aData->PropertyIsSet(eCSSProperty_padding_bottom))
> +        aData->SetPixelValue(eCSSProperty_padding_bottom, pad);
> +      if (!aData->PropertyIsSet(eCSSProperty_padding_left)) 

nit: Trailing whitespace.

::: dom/html/nsGenericHTMLElement.cpp:1474
(Diff revision 5)
> +
> +void
> +nsGenericHTMLElement::MapHeightAttributeInto(const nsMappedAttributes* aAttributes,
> +                                             GenericSpecifiedValues* aData)
> +{
> +  if (!(aData->CanHoldStyleStruct(NS_STYLE_INHERIT_BIT(Position))))

I'd tell you about braces and similar coding style stuff... I know this is pre-existing though :/
Attachment #8831399 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8831400 [details]
Bug 1334330 - Part 7: stylo: Use GenericSpecifiedValue abstraction for <font>;

https://reviewboard.mozilla.org/r/107972/#review109128
Attachment #8831400 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8831401 [details]
Bug 1334330 - Part 8: stylo: Use GenericSpecifiedValue abstraction for <iframe>;

https://reviewboard.mozilla.org/r/107974/#review109130

::: dom/html/HTMLIFrameElement.cpp:117
(Diff revision 5)
>        int32_t frameborder = value->GetEnumValue();
>        if (NS_STYLE_FRAME_0 == frameborder ||
>            NS_STYLE_FRAME_NO == frameborder ||
>            NS_STYLE_FRAME_OFF == frameborder) {
> -        nsCSSValue* borderLeftWidth = aData->ValueForBorderLeftWidth();
> -        if (borderLeftWidth->GetUnit() == eCSSUnit_Null)
> +        if (!aData->PropertyIsSet(eCSSProperty_border_top_width))
> +          aData->SetPixelValue(eCSSProperty_border_top_width, 0.0f);

Here SetIfUnset (and another few of them in the previous patches) would come handy.
Attachment #8831401 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8831402 [details]
Bug 1334330 - Part 9: stylo: Use GenericSpecifiedValue abstraction for <hr>;

https://reviewboard.mozilla.org/r/107976/#review109132
Attachment #8831402 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8831403 [details]
Bug 1334330 - Part 10: stylo: Use GenericSpecifiedValue abstraction for <body>;

https://reviewboard.mozilla.org/r/107978/#review109134
Comment on attachment 8831403 [details]
Bug 1334330 - Part 10: stylo: Use GenericSpecifiedValue abstraction for <body>;

https://reviewboard.mozilla.org/r/107978/#review109136
Attachment #8831403 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8831394 [details]
Bug 1334330 - Part 1: stylo: Abstractify nsMappedAttributes to work on arbitrary containers of specified value data;

https://reviewboard.mozilla.org/r/107960/#review109138

::: dom/base/nsMappedAttributes.cpp:184
(Diff revision 5)
>  
>  /* virtual */ void
>  nsMappedAttributes::MapRuleInfoInto(nsRuleData* aRuleData)
>  {
>    if (mRuleMapper) {
> -    (*mRuleMapper)(this, aRuleData);
> +    (*mRuleMapper)(this, (GenericSpecifiedValues*)aRuleData);

The cast shouldn't be needed.
Attachment #8831394 - Flags: review?(emilio+bugs)
Comment on attachment 8831399 [details]
Bug 1334330 - Part 6: stylo: Use GenericSpecifiedValue abstraction for table elements;

https://reviewboard.mozilla.org/r/107970/#review109126

> The reordering here makes the diff really noisy, was it necessary?

It's inevitable, I need to use the generic mappers for align and valign, but whitespace is sandwiched between the two, so wherever I put it the diff will be messed up.

> I know this is pre-existing, but is this logic right? We're setting `whitespace: nowrap` if the value for the `nowrap` attribute _is_ zero (instead of _isn't_).
> 
> 
> Boris, could you confirm this is intended or not? (I think not).

We're checking the `width` attribute here for zeroness.
Comment on attachment 8831394 [details]
Bug 1334330 - Part 1: stylo: Abstractify nsMappedAttributes to work on arbitrary containers of specified value data;

https://reviewboard.mozilla.org/r/107960/#review109568

r=me assuming you're going to do the handle/typedef thing for non-stylo builds. I think it's preferable, unless it's too much work for some reason.

::: dom/base/nsMappedAttributes.cpp:184
(Diff revision 6)
>  
>  /* virtual */ void
>  nsMappedAttributes::MapRuleInfoInto(nsRuleData* aRuleData)
>  {
>    if (mRuleMapper) {
> -    (*mRuleMapper)(this, aRuleData);
> +    (*mRuleMapper)(this, (GenericSpecifiedValues*) aRuleData);

I'm still suprised if this cast is needed but w/e. If it is, could we convert it to a static_cast?
Attachment #8831394 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8831395 [details]
Bug 1334330 - Part 2: stylo: Use GenericSpecifiedValue abstraction in nsGenericHTMLElement;

https://reviewboard.mozilla.org/r/107962/#review109574

::: dom/html/nsGenericHTMLElement.cpp:1212
(Diff revision 6)
>  static inline void
> -MapLangAttributeInto(const nsMappedAttributes* aAttributes, GenericSpecifiedValues* aGenericData)
> +MapLangAttributeInto(const nsMappedAttributes* aAttributes, GenericSpecifiedValues* aData)
>  {
> -  nsRuleData* aData = aGenericData->AsRuleData();
> +  if (!(aData->ShouldComputeStyleStruct(NS_STYLE_INHERIT_BIT(Font) |
> -  if (!(aData->mSIDs & (NS_STYLE_INHERIT_BIT(Font) |
> -                        NS_STYLE_INHERIT_BIT(Text)))) {
> +                                  NS_STYLE_INHERIT_BIT(Text)))) {

Alignment still feels odd here and below, though maybe it's mozreview's UI?
Attachment #8831395 - Flags: review?(emilio+bugs) → review+
Added IfUnset methods.
Added the ServoUtils stuff.

I think it's ready for r/sr from bz, too.
Flags: needinfo?(bzbarsky)
Comment on attachment 8833568 [details]
Bug 1334330 - Part 11: stylo: Use ServoUtils abstraction for GenericSpecifiedValues to remove virtual dispatch overhead in nostylo mode;

https://reviewboard.mozilla.org/r/109794/#review110846

::: layout/style/GenericSpecifiedValuesInlines.h:22
(Diff revision 1)
> +}
> +
> +MOZ_DEFINE_STYLO_METHODS(GenericSpecifiedValues, nsRuleData, nsRuleData)
> +
> +bool
> +GenericSpecifiedValues::PropertyIsSet(nsCSSPropertyID aId) {

nit: brace

::: layout/style/nsRuleData.h:235
(Diff revision 1)
>  private:
>    inline size_t GetPoisonOffset();
>  
>  };
>  
> +#include "mozilla/GenericSpecifiedValuesInlines.h"

I believe this is an antipattern, do we do this anywhere else? If not, could we add the includes to the relevant files?

::: layout/style/nsRuleData.cpp:31
(Diff revision 1)
>           sizeof(nsCSSValue);
>  }
>  
>  nsRuleData::nsRuleData(uint32_t aSIDs, nsCSSValue* aValueStorage,
>                         nsPresContext* aContext, nsStyleContext* aStyleContext)
> -  : mSIDs(aSIDs),
> +  : GenericSpecifiedValues(StyleBackendType::Servo),

::Servo, sure?
Attachment #8833568 - Flags: review?(emilio+bugs)
Comment on attachment 8833568 [details]
Bug 1334330 - Part 11: stylo: Use ServoUtils abstraction for GenericSpecifiedValues to remove virtual dispatch overhead in nostylo mode;

https://reviewboard.mozilla.org/r/109794/#review110868

::: layout/style/GenericSpecifiedValues.h:28
(Diff revisions 1 - 2)
>  // This provides a common interface for attribute mappers (MapAttributesIntoRule)
>  // to use regardless of the style backend. If the style backend is Gecko,
>  // this will contain an nsRuleData. If it is Servo, it will be a PropertyDeclarationBlock.
>  class GenericSpecifiedValues {
>  protected:
> -    GenericSpecifiedValues(mozilla::StyleBackendType aType);
> +    GenericSpecifiedValues(StyleBackendType aType) : mType(aType) {}

consider marking this explicit? It's not public, but probably worth it?

::: dom/html/HTMLContentElement.cpp:18
(Diff revision 2)
>  #include "nsGkAtoms.h"
>  #include "nsStyleConsts.h"
>  #include "nsIAtom.h"
>  #include "nsCSSRuleProcessor.h"
>  #include "nsRuleData.h"
> +#include "mozilla/GenericSpecifiedValuesInlines.h"

Up, so it's sorted, here and everywhere else. Also, you can remove the nsRuleData includes I think.

::: layout/style/GenericSpecifiedValuesInlines.h:6
(Diff revision 2)
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

Consider adding a description comment like[1], so it shows up in dxr/searchfox. Not mandatory I guess :)

[1]: http://searchfox.org/mozilla-central/source/layout/style/nsRuleData.h#6

::: layout/style/GenericSpecifiedValuesInlines.h:13
(Diff revision 2)
> +#define mozilla_GenericSpecifiedValuesInlines_h
> +
> +#include "nsRuleData.h"
> +#include "mozilla/GenericSpecifiedValues.h"
> +
> +using namespace mozilla;

I think we're not supposed to use `using namespace` due to unified builds, less so in a header, please wrap this in a `namespace` block.
Attachment #8833568 - Flags: review?(emilio+bugs) → review+
Okay, just needs bz's review now.
Comment on attachment 8831394 [details]
Bug 1334330 - Part 1: stylo: Abstractify nsMappedAttributes to work on arbitrary containers of specified value data;

https://reviewboard.mozilla.org/r/107960/#review111716

::: dom/base/nsMappedAttributes.cpp:184
(Diff revision 6)
>  
>  /* virtual */ void
>  nsMappedAttributes::MapRuleInfoInto(nsRuleData* aRuleData)
>  {
>    if (mRuleMapper) {
> -    (*mRuleMapper)(this, aRuleData);
> +    (*mRuleMapper)(this, (GenericSpecifiedValues*) aRuleData);

Emilio is right; this cast should not be needed.  Please remove it.  Make sure you #include "nsRuleData.h"!

::: layout/style/GenericSpecifiedValues.h:25
(Diff revision 6)
> +struct nsRuleData;
> +
> +// This provides a common interface for attribute mappers (MapAttributesIntoRule)
> +// to use regardless of the style backend. If the style backend is Gecko,
> +// this will contain an nsRuleData. If it is Servo, it will be a PropertyDeclarationBlock.
> +class GenericSpecifiedValues {

Please put this in the mozilla namespace to match its export location and header guard and all that.
Comment on attachment 8831394 [details]
Bug 1334330 - Part 1: stylo: Abstractify nsMappedAttributes to work on arbitrary containers of specified value data;

https://reviewboard.mozilla.org/r/107960/#review111726
Attachment #8831394 - Flags: review+
Comment on attachment 8831395 [details]
Bug 1334330 - Part 2: stylo: Use GenericSpecifiedValue abstraction in nsGenericHTMLElement;

https://reviewboard.mozilla.org/r/107962/#review111722

::: dom/html/nsGenericHTMLElement.cpp:1211
(Diff revision 7)
>  }
>  
>  static inline void
> -MapLangAttributeInto(const nsMappedAttributes* aAttributes, GenericSpecifiedValues* aGenericData)
> +MapLangAttributeInto(const nsMappedAttributes* aAttributes, GenericSpecifiedValues* aData)
>  {
> -  nsRuleData* aData = aGenericData->AsRuleData();
> +  if (!(aData->ShouldComputeStyleStruct(NS_STYLE_INHERIT_BIT(Font) |

Now that these are virtual calls, not just some bit-twiddling, might be worth caching the return values insead of calling ShouldComputeStyleStruct again.

The other option is to change the API to return a bitmask of the structs to compute, I guess (and cache _that_).

Oh, except I just realized that in part 11 you totally change the API.  That should have been back in part 1 or so.  :(

::: dom/html/nsGenericHTMLElement.cpp:1211
(Diff revision 7)
>  }
>  
>  static inline void
> -MapLangAttributeInto(const nsMappedAttributes* aAttributes, GenericSpecifiedValues* aGenericData)
> +MapLangAttributeInto(const nsMappedAttributes* aAttributes, GenericSpecifiedValues* aData)
>  {
> -  nsRuleData* aData = aGenericData->AsRuleData();
> +  if (!(aData->ShouldComputeStyleStruct(NS_STYLE_INHERIT_BIT(Font) |

This is overparenthesized, afaict.  The paren after '!', and its match, should go away.

::: dom/html/nsGenericHTMLElement.cpp:1257
(Diff revision 7)
>          aAttributes->GetAttr(nsGkAtoms::contenteditable);
>        if (value) {
>          if (value->Equals(nsGkAtoms::_empty, eCaseMatters) ||
>              value->Equals(nsGkAtoms::_true, eIgnoreCase)) {
> -          userModify->SetEnumValue(StyleUserModify::ReadWrite);
> +          aData->SetKeywordValue(eCSSProperty__moz_user_modify,
> +                                        StyleUserModify::ReadWrite);

Please fix the indent.

::: dom/html/nsGenericHTMLElement.cpp:1261
(Diff revision 7)
> -          userModify->SetEnumValue(StyleUserModify::ReadWrite);
> +          aData->SetKeywordValue(eCSSProperty__moz_user_modify,
> +                                        StyleUserModify::ReadWrite);
>          }
>          else if (value->Equals(nsGkAtoms::_false, eIgnoreCase)) {
> -            userModify->SetEnumValue(StyleUserModify::ReadOnly);
> +            aData->SetKeywordValue(eCSSProperty__moz_user_modify,
> +                                          StyleUserModify::ReadOnly);

And here.

::: dom/html/nsGenericHTMLElement.cpp:1381
(Diff revision 7)
>  
>  void
>  nsGenericHTMLElement::MapImageMarginAttributeInto(const nsMappedAttributes* aAttributes,
> -                                                  GenericSpecifiedValues* aGenericData)
> +                                                  GenericSpecifiedValues* aData)
>  {
> -  nsRuleData* aData = aGenericData->AsRuleData();
> +  if (!(aData->ShouldComputeStyleStruct(NS_STYLE_INHERIT_BIT(Margin))))

Overparenthesized.

::: dom/html/nsGenericHTMLElement.cpp:1423
(Diff revision 7)
>  
>  void
>  nsGenericHTMLElement::MapImageSizeAttributesInto(const nsMappedAttributes* aAttributes,
> -                                                 GenericSpecifiedValues* aGenericData)
> +                                                 GenericSpecifiedValues* aData)
>  {
> -  nsRuleData* aData = aGenericData->AsRuleData();
> +  if (!(aData->ShouldComputeStyleStruct(NS_STYLE_INHERIT_BIT(Position))))

Overparenthesized.

::: dom/html/nsGenericHTMLElement.cpp:1455
(Diff revision 7)
>  
>  void
>  nsGenericHTMLElement::MapImageBorderAttributeInto(const nsMappedAttributes* aAttributes,
> -                                                  GenericSpecifiedValues* aGenericData)
> +                                                  GenericSpecifiedValues* aData)
>  {
> -  nsRuleData* aData = aGenericData->AsRuleData();
> +  if (!(aData->ShouldComputeStyleStruct(NS_STYLE_INHERIT_BIT(Border))))

Overparenthesized.

::: dom/html/nsGenericHTMLElement.cpp:1492
(Diff revision 7)
>  void
>  nsGenericHTMLElement::MapBackgroundInto(const nsMappedAttributes* aAttributes,
> -                                        GenericSpecifiedValues* aGenericData)
> +                                        GenericSpecifiedValues* aData)
>  {
> -  nsRuleData* aData = aGenericData->AsRuleData();
> -  if (!(aData->mSIDs & NS_STYLE_INHERIT_BIT(Background)))
> +
> +  if (!(aData->ShouldComputeStyleStruct(NS_STYLE_INHERIT_BIT(Background))))

Overparenthesized.

::: dom/html/nsGenericHTMLElement.cpp:1519
(Diff revision 7)
> -      if (value->Type() == nsAttrValue::eImage) {
> +        if (value->Type() == nsAttrValue::eImage) {
> -        nsCSSValueList* list = backImage->SetListValue();
> +          nsCSSValueList* list = backImage->SetListValue();
> -        list->mValue.SetImageValue(value->GetImageValue());
> +          list->mValue.SetImageValue(value->GetImageValue());
> -      }
> +        }
> +      } else {
> +        // FIXME(bug 1330041) Servo-specific code goes here

Please have a warning, an assert, a MOZ_CRASH, _something_ here so we don't forget it.  Ideally a MOZ_ASSERT, unless that turns try interesting colors; then an NS_ASSERTION plus annotations.

Unless the plan is to fix bug 1330041 imminently, in which case it probably doesn't matter.

::: dom/html/nsGenericHTMLElement.cpp:1529
(Diff revision 7)
>  
>  void
>  nsGenericHTMLElement::MapBGColorInto(const nsMappedAttributes* aAttributes,
> -                                     GenericSpecifiedValues* aGenericData)
> +                                     GenericSpecifiedValues* aData)
>  {
> -  nsRuleData* aData = aGenericData->AsRuleData();
> +  if (!(aData->ShouldComputeStyleStruct(NS_STYLE_INHERIT_BIT(Background))))

Overparenthesized.

::: layout/style/GenericSpecifiedValues.h:32
(Diff revision 7)
>      // Check if we already contain a certain longhand
>      virtual bool PropertyIsSet(nsCSSPropertyID aId) = 0;
> +    // Check if we are able to hold longhands from a given
> +    // style struct. Pass the result of NS_STYLE_INHERIT_BIT to this
> +    // function
> +    virtual bool ShouldComputeStyleStruct(uint64_t aInheritBit) = 0;

This needs to make it clear that more than one bit can be passed (i.e. aInheritBits, plus clearer documentation).  Again, unless we switch the API to return the bitmask of things to compute.
Attachment #8831395 - Flags: review+
Comment on attachment 8831396 [details]
Bug 1334330 - Part 3: stylo: Use GenericSpecifiedValue abstraction in elements using only common mappers;

https://reviewboard.mozilla.org/r/107964/#review111738
Attachment #8831396 - Flags: review+
Comment on attachment 8831397 [details]
Bug 1334330 - Part 4: stylo: Use GenericSpecifiedValue abstraction for <br>;

https://reviewboard.mozilla.org/r/107966/#review111740
Attachment #8831397 - Flags: review+
Comment on attachment 8831398 [details]
Bug 1334330 - Part 5: stylo: Use GenericSpecifiedValue abstraction for <li>,<pre>,<ol>,<ul>,<textarea>;

https://reviewboard.mozilla.org/r/107968/#review111742
Attachment #8831398 - Flags: review+
Comment on attachment 8831399 [details]
Bug 1334330 - Part 6: stylo: Use GenericSpecifiedValue abstraction for table elements;

https://reviewboard.mozilla.org/r/107970/#review111752

::: dom/html/HTMLTableCellElement.cpp:446
(Diff revision 7)
>  void
>  HTMLTableCellElement::MapAttributesIntoRule(const nsMappedAttributes* aAttributes,
> -                                            GenericSpecifiedValues* aGenericData)
> +                                            GenericSpecifiedValues* aData)
>  {
> -  nsRuleData* aData = aGenericData->AsRuleData();
> -  if (aData->mSIDs & NS_STYLE_INHERIT_BIT(Position)) {
> +
> +  if (aData->ShouldComputeStyleStruct(NS_STYLE_INHERIT_BIT(Text))) {

Mind moving this part back where it came from, so the diff is less confusing?  ;)

::: dom/html/HTMLTableElement.cpp:842
(Diff revision 7)
>      if (value && value->Type() == nsAttrValue::eInteger) {
>        // We have cellpadding.  This will override our padding values if we
>        // don't have any set.
> -      nsCSSValue padVal(float(value->GetIntegerValue()), eCSSUnit_Pixel);
> +      float pad = float(value->GetIntegerValue());
>  
> -      nsCSSValue* paddingLeft = aData->ValueForPaddingLeft();
> +      if (!aData->PropertyIsSet(eCSSProperty_padding_top))

Why not use SetPixelValueIfNotSet for all of these?

::: dom/html/HTMLTableSectionElement.cpp:172
(Diff revision 7)
>  
>  void
>  HTMLTableSectionElement::MapAttributesIntoRule(const nsMappedAttributes* aAttributes,
> -                                               GenericSpecifiedValues* aGenericData)
> +                                               GenericSpecifiedValues* aData)
>  {
> -  nsRuleData* aData = aGenericData->AsRuleData();
> +  nsGenericHTMLElement::MapHeightAttributeInto(aAttributes, aData);

This is a behavior change.  The old code only did pixel heights, not percentage heights, right?

Please undo this change.

::: dom/html/nsGenericHTMLElement.cpp:1378
(Diff revision 7)
>    }
>  }
>  
> +void
> +nsGenericHTMLElement::MapVAlignAttributeInto(const nsMappedAttributes* aAttributes,
> +                                               GenericSpecifiedValues* aData)

Please fix the indent.

::: dom/html/nsGenericHTMLElement.cpp:1436
(Diff revision 7)
>  
>  void
> -nsGenericHTMLElement::MapImageSizeAttributesInto(const nsMappedAttributes* aAttributes,
> +nsGenericHTMLElement::MapWidthAttributeInto(const nsMappedAttributes* aAttributes,
> -                                                 GenericSpecifiedValues* aData)
> +                                            GenericSpecifiedValues* aData)
>  {
> -  if (!(aData->ShouldComputeStyleStruct(NS_STYLE_INHERIT_BIT(Position))))
> +  if (!(aData->ShouldComputeStyleStruct(NS_STYLE_INHERIT_BIT(Position)))) {

Still overparenthesized.

::: dom/html/nsGenericHTMLElement.cpp:1457
(Diff revision 7)
> +
> +void
> +nsGenericHTMLElement::MapHeightAttributeInto(const nsMappedAttributes* aAttributes,
> +                                             GenericSpecifiedValues* aData)
> +{
> +  if (!(aData->ShouldComputeStyleStruct(NS_STYLE_INHERIT_BIT(Position))))

Overparenthesized.
Attachment #8831399 - Flags: review-
> This is a behavior change.  The old code only did pixel heights, not percentage heights, right?

Note that per spec I _think_ there should be no "height" attr here at all.  But that should be a followup bug, not part of this change, no matter what.
Flags: needinfo?(bzbarsky)
Addressed and retriggered try. Ready to land?
Comment on attachment 8831395 [details]
Bug 1334330 - Part 2: stylo: Use GenericSpecifiedValue abstraction in nsGenericHTMLElement;

https://reviewboard.mozilla.org/r/107962/#review111800

::: layout/style/GenericSpecifiedValues.h:32
(Diff revision 9)
>      // Check if we already contain a certain longhand
>      virtual bool PropertyIsSet(nsCSSPropertyID aId) = 0;
> +    // Check if we are able to hold longhands from a given
> +    // style struct. Pass the result of NS_STYLE_INHERIT_BIT to this
> +    // function. Can accept multiple inherit bits or'd together.
> +    virtual bool ShouldComputeStyleStruct(uint64_t aInheritBit) = 0;

Please, do aInheritBits.  ;)
Attachment #8831395 - Flags: review+
Comment on attachment 8831400 [details]
Bug 1334330 - Part 7: stylo: Use GenericSpecifiedValue abstraction for <font>;

https://reviewboard.mozilla.org/r/107972/#review111804

::: layout/style/GenericSpecifiedValues.h:83
(Diff revision 9)
>      virtual void SetColorValue(nsCSSPropertyID aId, nscolor aValue) = 0;
>      virtual void SetColorValueIfUnset(nsCSSPropertyID aId, nscolor aValue) = 0;
>  
> +    // Set font-family to a string
> +    virtual void SetFontFamily(const nsString& aValue) = 0;
> +    // Add a quirks-mode override that adds a colored underline when nested in <a>

No, that's not what it does.  What it does is it overrides the color of the already existing underline with a different color.  So it's not forcing an _underline_; it's forcing color overriding.  And it's forcing it for all decorations, note.  Simple testcase:

data:text/html,<a style="text-decoration: underline overline; color: red"><font color="green">aaaa

Please check the comment and function name accordingly.
Attachment #8831400 - Flags: review+
Comment on attachment 8831401 [details]
Bug 1334330 - Part 8: stylo: Use GenericSpecifiedValue abstraction for <iframe>;

https://reviewboard.mozilla.org/r/107974/#review111806

::: dom/html/HTMLIFrameElement.cpp:124
(Diff revision 9)
> -      else if (value && value->Type() == nsAttrValue::ePercent)
> -        height->SetPercentValue(value->GetPercentValue());
> -    }
> -  }
>  
> -  nsGenericHTMLElement::MapImageAlignAttributeInto(aAttributes, aGenericData);
> +  nsGenericHTMLElement::MapWidthAttributeInto(aAttributes, aData);

Could replace the separate width/height calls with MapImageSizeAttributesInto, right?  Any reason not to?
Attachment #8831401 - Flags: review+
Comment on attachment 8831402 [details]
Bug 1334330 - Part 9: stylo: Use GenericSpecifiedValue abstraction for <hr>;

https://reviewboard.mozilla.org/r/107976/#review111808

::: dom/html/HTMLHRElement.cpp:79
(Diff revision 9)
>    const nsAttrValue* colorValue = aAttributes->GetAttr(nsGkAtoms::color);
>    nscolor color;
>    bool colorIsSet = colorValue && colorValue->GetColorValue(color);
>  
> -  if (aData->mSIDs & (NS_STYLE_INHERIT_BIT(Position) |
> +  if (aData->ShouldComputeStyleStruct(NS_STYLE_INHERIT_BIT(Position) |
> -                      NS_STYLE_INHERIT_BIT(Border))) {
> +                                NS_STYLE_INHERIT_BIT(Border))) {

Indent looks off here.

::: dom/html/HTMLHRElement.cpp:110
(Diff revision 9)
>        }
>      }
>    }
> -  if (aData->mSIDs & NS_STYLE_INHERIT_BIT(Position)) {
> +  if (aData->ShouldComputeStyleStruct(NS_STYLE_INHERIT_BIT(Position))) {
>      // width: integer, percent
> -    nsCSSValue* width = aData->ValueForWidth();
> +    nsGenericHTMLElement::MapWidthAttributeInto(aAttributes, aData);

For consistency, this should probably be outside the ShouldComputeStyleStruct check, right?

::: dom/html/HTMLHRElement.cpp:131
(Diff revision 9)
>        }
>      }
>    }
> -  if ((aData->mSIDs & NS_STYLE_INHERIT_BIT(Border)) && noshade) { // if not noshade, border styles are dealt with by html.css
> +
> +  // if not noshade, border styles are dealt with by html.css
> +  if ((aData->ShouldComputeStyleStruct(NS_STYLE_INHERIT_BIT(Border))) && noshade) {

Overparenthesized.

::: dom/html/HTMLHRElement.cpp:160
(Diff revision 9)
> -      nsCSSValue* borderRightStyle = aData->ValueForBorderRightStyle();
> -      if (borderRightStyle->GetUnit() == eCSSUnit_Null) {
> -        borderRightStyle->SetIntValue(NS_STYLE_BORDER_STYLE_SOLID,
> -                                      eCSSUnit_Enumerated);
> -      }
> -      nsCSSValue* borderBottomStyle = aData->ValueForBorderBottomStyle();
> +      aData->SetKeywordValueIfUnset(eCSSProperty_border_right_style,
> +                                      NS_STYLE_BORDER_STYLE_SOLID);
> +      aData->SetKeywordValueIfUnset(eCSSProperty_border_bottom_style,
> +                                      NS_STYLE_BORDER_STYLE_SOLID);
> +      aData->SetKeywordValueIfUnset(eCSSProperty_border_left_style,
> +                                      NS_STYLE_BORDER_STYLE_SOLID);

Fix indent here.
Attachment #8831402 - Flags: review+
Comment on attachment 8831403 [details]
Bug 1334330 - Part 10: stylo: Use GenericSpecifiedValue abstraction for <body>;

https://reviewboard.mozilla.org/r/107978/#review111810
Attachment #8831403 - Flags: review+
Comment on attachment 8833568 [details]
Bug 1334330 - Part 11: stylo: Use ServoUtils abstraction for GenericSpecifiedValues to remove virtual dispatch overhead in nostylo mode;

https://reviewboard.mozilla.org/r/109794/#review111812

::: dom/html/nsGenericHTMLElement.h:695
(Diff revision 5)
>     * @param aAttributes the list of attributes to map
>     * @param aData the returned rule data [INOUT]
>     * @see GetAttributeMappingFunction
>     */
>    static void MapDivAlignAttributeInto(const nsMappedAttributes* aAttributes,
> -                                    GenericSpecifiedValues* aGenericData);
> +                                    mozilla::GenericSpecifiedValues* aGenericData);

Fix indent.

::: dom/html/nsGenericHTMLElement.h:706
(Diff revision 5)
>     * @param aAttributes the list of attributes to map
>     * @param aData the returned rule data [INOUT]
>     * @see GetAttributeMappingFunction
>     */
>    static void MapVAlignAttributeInto(const nsMappedAttributes* aAttributes,
> -                                    GenericSpecifiedValues* aGenericData);
> +                                    mozilla::GenericSpecifiedValues* aGenericData);

Indent.

::: dom/html/nsGenericHTMLElement.cpp:1209
(Diff revision 5)
>  {
>    return aResult.ParseEnumValue(aString, kScrollingTable, false);
>  }
>  
>  static inline void
> -MapLangAttributeInto(const nsMappedAttributes* aAttributes, GenericSpecifiedValues* aData)
> +MapLangAttributeInto(const nsMappedAttributes* aAttributes, mozilla::GenericSpecifiedValues* aData)

No need for "mozilla::" here; this file is "using namespace mozilla;".

::: dom/html/nsGenericHTMLElement.cpp:1247
(Diff revision 5)
>  /**
>   * Handle attributes common to all html elements
>   */
>  void
>  nsGenericHTMLElement::MapCommonAttributesIntoExceptHidden(const nsMappedAttributes* aAttributes,
> -                                                          GenericSpecifiedValues* aData)
> +                                                          mozilla::GenericSpecifiedValues* aData)

And here.

::: dom/html/nsGenericHTMLElement.cpp:1272
(Diff revision 5)
>    MapLangAttributeInto(aAttributes, aData);
>  }
>  
>  void
>  nsGenericHTMLElement::MapCommonAttributesInto(const nsMappedAttributes* aAttributes,
> -                                              GenericSpecifiedValues* aData)
> +                                              mozilla::GenericSpecifiedValues* aData)

And here.

::: dom/html/nsGenericHTMLElement.cpp:1335
(Diff revision 5)
>    { nullptr }
>  };
>  
>  void
>  nsGenericHTMLElement::MapImageAlignAttributeInto(const nsMappedAttributes* aAttributes,
> -                                                 GenericSpecifiedValues* aData)
> +                                                 mozilla::GenericSpecifiedValues* aData)

And here.

::: dom/html/nsGenericHTMLElement.cpp:1364
(Diff revision 5)
>    }
>  }
>  
>  void
>  nsGenericHTMLElement::MapDivAlignAttributeInto(const nsMappedAttributes* aAttributes,
> -                                               GenericSpecifiedValues* aData)
> +                                               mozilla::GenericSpecifiedValues* aData)

And here.

::: dom/html/nsGenericHTMLElement.cpp:1378
(Diff revision 5)
>    }
>  }
>  
>  void
>  nsGenericHTMLElement::MapVAlignAttributeInto(const nsMappedAttributes* aAttributes,
> -                                             GenericSpecifiedValues* aData)
> +                                             mozilla::GenericSpecifiedValues* aData)

And here.

::: dom/html/nsGenericHTMLElement.cpp:1392
(Diff revision 5)
>    }
>  }
>  
>  void
>  nsGenericHTMLElement::MapImageMarginAttributeInto(const nsMappedAttributes* aAttributes,
> -                                                  GenericSpecifiedValues* aData)
> +                                                  mozilla::GenericSpecifiedValues* aData)

And here.

::: dom/html/nsGenericHTMLElement.cpp:1434
(Diff revision 5)
>    }
>  }
>  
>  void
>  nsGenericHTMLElement::MapWidthAttributeInto(const nsMappedAttributes* aAttributes,
> -                                            GenericSpecifiedValues* aData)
> +                                            mozilla::GenericSpecifiedValues* aData)

We have a lot of these functions, don't we?  Here too.

::: dom/html/nsGenericHTMLElement.cpp:1455
(Diff revision 5)
>    }
>  }
>  
>  void
>  nsGenericHTMLElement::MapHeightAttributeInto(const nsMappedAttributes* aAttributes,
> -                                             GenericSpecifiedValues* aData)
> +                                             mozilla::GenericSpecifiedValues* aData)

And here.

::: dom/html/nsGenericHTMLElement.cpp:1475
(Diff revision 5)
>    }
>  }
>  
>  void
>  nsGenericHTMLElement::MapImageSizeAttributesInto(const nsMappedAttributes* aAttributes,
> -                                                 GenericSpecifiedValues* aData)
> +                                                 mozilla::GenericSpecifiedValues* aData)

And here.

::: dom/html/nsGenericHTMLElement.cpp:1483
(Diff revision 5)
>    nsGenericHTMLElement::MapHeightAttributeInto(aAttributes, aData);
>  }
>  
>  void
>  nsGenericHTMLElement::MapImageBorderAttributeInto(const nsMappedAttributes* aAttributes,
> -                                                  GenericSpecifiedValues* aData)
> +                                                  mozilla::GenericSpecifiedValues* aData)

And here.

::: dom/html/nsGenericHTMLElement.cpp:1519
(Diff revision 5)
>    aData->SetCurrentColorIfUnset(eCSSProperty_border_left_color);
>  }
>  
>  void
>  nsGenericHTMLElement::MapBackgroundInto(const nsMappedAttributes* aAttributes,
> -                                        GenericSpecifiedValues* aData)
> +                                        mozilla::GenericSpecifiedValues* aData)

And here.

::: dom/html/nsGenericHTMLElement.cpp:1558
(Diff revision 5)
>    }
>  }
>  
>  void
>  nsGenericHTMLElement::MapBGColorInto(const nsMappedAttributes* aAttributes,
> -                                     GenericSpecifiedValues* aData)
> +                                     mozilla::GenericSpecifiedValues* aData)

And here.

::: dom/html/nsGenericHTMLElement.cpp:1575
(Diff revision 5)
>    }
>  }
>  
>  void
>  nsGenericHTMLElement::MapBackgroundAttributesInto(const nsMappedAttributes* aAttributes,
> -                                                  GenericSpecifiedValues* aData)
> +                                                  mozilla::GenericSpecifiedValues* aData)

And here.

::: dom/mathml/nsMathMLElement.cpp:491
(Diff revision 5)
>    return true;
>  }
>  
>  void
>  nsMathMLElement::MapMathMLAttributesInto(const nsMappedAttributes* aAttributes,
> -                                         GenericSpecifiedValues* aGenericData)
> +                                         mozilla::GenericSpecifiedValues* aGenericData)

And here.
Attachment #8833568 - Flags: review+
Comment on attachment 8831401 [details]
Bug 1334330 - Part 8: stylo: Use GenericSpecifiedValue abstraction for <iframe>;

https://reviewboard.mozilla.org/r/107974/#review111830
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7870b7280780
Part 1: stylo: Abstractify nsMappedAttributes to work on arbitrary containers of specified value data; r=bz,emilio
https://hg.mozilla.org/integration/autoland/rev/9bb1bc081641
Part 2: stylo: Use GenericSpecifiedValue abstraction in nsGenericHTMLElement; r=bz,emilio
https://hg.mozilla.org/integration/autoland/rev/bb908a4fa131
Part 3: stylo: Use GenericSpecifiedValue abstraction in elements using only common mappers; r=bz,emilio
https://hg.mozilla.org/integration/autoland/rev/fb821eafe283
Part 4: stylo: Use GenericSpecifiedValue abstraction for <br>; r=bz,emilio
https://hg.mozilla.org/integration/autoland/rev/42f6914789d4
Part 5: stylo: Use GenericSpecifiedValue abstraction for <li>,<pre>,<ol>,<ul>,<textarea>; r=bz,emilio
https://hg.mozilla.org/integration/autoland/rev/33211bf1f4d8
Part 6: stylo: Use GenericSpecifiedValue abstraction for table elements; r=emilio
https://hg.mozilla.org/integration/autoland/rev/07e22347b23f
Part 7: stylo: Use GenericSpecifiedValue abstraction for <font>; r=bz,emilio
https://hg.mozilla.org/integration/autoland/rev/c025140b64c7
Part 8: stylo: Use GenericSpecifiedValue abstraction for <iframe>; r=bz,emilio
https://hg.mozilla.org/integration/autoland/rev/7c44ae696319
Part 9: stylo: Use GenericSpecifiedValue abstraction for <hr>; r=bz,emilio
https://hg.mozilla.org/integration/autoland/rev/34aca5948e5c
Part 10: stylo: Use GenericSpecifiedValue abstraction for <body>; r=bz,emilio
https://hg.mozilla.org/integration/autoland/rev/530f32f16316
Part 11: stylo: Use ServoUtils abstraction for GenericSpecifiedValues to remove virtual dispatch overhead in nostylo mode; r=bz,emilio
Fwiw, you didn't rename all the aGenericData back to aData (but did some), so now some of them are inconsistent.  :(
Flags: needinfo?(manishearth)
Ugh. I'll catch it when I touch this next.
Flags: needinfo?(manishearth)
You need to log in before you can comment on or make changes to this bug.