1341648 - stylo: need to port HTMLTableCellElement::WalkContentStyleRules to stylo

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

We don't seem to have a thing for it right now.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This causes reftest failures at least in layout/reftests/pagination.

Comment 2

[Bobby Holley (:bholley)]

Similar to bug 1341647, over to Manish.

Comment 4

[Manish Goregaokar [:manishearth]]

splitting out the patches on bug 1341714

Comment 5

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1341648 - stylo: Include content rules from HTMLTableCellElement::WalkContentStyleRules ;

This also removes the TABLE_ATTRS_DIRTY optimization. Constructing nsMappedAttributes isn't really expensive and we do it all the time anyway.

Review commit: https://reviewboard.mozilla.org/r/124788/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124788/

Comment 6

[Manish Goregaokar [:manishearth]]

r?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8852600 [details]
Bug 1341648 - stylo: Include content rules from HTMLTableCellElement::WalkContentStyleRules ;

https://reviewboard.mozilla.org/r/124788/#review127440

This doesn't handle dynamic changes to cellpadding correctly, as evidenced by the fact that we're still failing the 586400-1.html test in layout/reftests/bugs/reftest-stylo.list

Gecko handles this by having nsHTMLStyleSheet::HasAttributeDependentStyle explicitly return eRestyle_Subtree for the case of a cellpadding attribute changing on a <table> element.  We need some equivalent for stylo.

::: dom/html/HTMLTableCellElement.h:152
(Diff revision 1)
>                                nsAttrValue& aResult) override;
>    virtual nsMapRuleToAttributesFunc GetAttributeMappingFunction() const override;
>    NS_IMETHOD WalkContentStyleRules(nsRuleWalker* aRuleWalker) override;
>    NS_IMETHOD_(bool) IsAttributeMapped(const nsIAtom* aAttribute) const override;
> +  // Get mapped attributes of parent table, if any
> +  nsMappedAttributes* GetParentMappedAttributes() const;

GetTableMappedAttributes, please.  It's not really our "parent" normally.  Or maybe GetMappedAttributesInheritedFromTable?

::: dom/html/HTMLTableElement.cpp:866
(Diff revision 1)
>    if (mTableInheritedAttributes) {
> -    if (mTableInheritedAttributes == TABLE_ATTRS_DIRTY)
> -      BuildInheritedAttributes();
> -    if (mTableInheritedAttributes != TABLE_ATTRS_DIRTY)
>        return mTableInheritedAttributes;
>    }
>    return nullptr;

This looks like a complicated way to write:

  return mTableInheritedAttributes;

::: dom/html/HTMLTableElement.cpp:901
(Diff revision 1)
>          // Reset the stylesheet of modifiableMapped so that it doesn't
>          // spend time trying to remove itself from the hash.  There is no
>          // risk that modifiableMapped is in the hash since we created
>          // it ourselves and it didn't come from the stylesheet (in which
>          // case it would not have been modifiable).
> +

Why this extra blank line?

::: dom/html/HTMLTableElement.cpp:913
(Diff revision 1)
> -  if (mTableInheritedAttributes &&
> +  if (mTableInheritedAttributes)
> -      mTableInheritedAttributes != TABLE_ATTRS_DIRTY)
>      NS_RELEASE(mTableInheritedAttributes);
> -  mTableInheritedAttributes = TABLE_ATTRS_DIRTY;
> +  mTableInheritedAttributes = nullptr;

Just NS_IF_RELEASE(mTableInheritedAttributes) here.  And note that that nulls it out too, so no need to assign nullptr.

::: layout/style/ServoBindings.h:194
(Diff revision 1)
>  RawServoDeclarationBlockStrongBorrowedOrNull
>  Gecko_GetStyleAttrDeclarationBlock(RawGeckoElementBorrowed element);
>  RawServoDeclarationBlockStrongBorrowedOrNull
>  Gecko_GetHTMLPresentationAttrDeclarationBlock(RawGeckoElementBorrowed element);
> +RawServoDeclarationBlockStrongBorrowedOrNull
> +Gecko_GetExtraContentStyleRules(RawGeckoElementBorrowed element);

Gecko_GetExtraContentStyleDeclarations, no?

::: layout/style/ServoBindings.cpp:409
(Diff revision 1)
> +Gecko_GetExtraContentStyleRules(RawGeckoElementBorrowed aElement)
> +{
> +  static_assert(sizeof(RefPtr<RawServoDeclarationBlock>) ==
> +                sizeof(RawServoDeclarationBlockStrong),
> +                "RefPtr should just be a pointer");
> +  if (aElement->IsHTMLElement(nsGkAtoms::td) || aElement->IsHTMLElement(nsGkAtoms::th)) {

This should probably be aElement->IsAnyOfHTMLElements(nsGkAtoms::td, nsGkAtoms:th) to avoid doing the namespace check twice.

Also, why not invert the condition and early return if not one of those elements, then outdent everything.

Comment 8

[Manish Goregaokar [:manishearth]]

Addressed. Bug 1341647 handles posting restyle updates for all other mapped attributes, but I included the cellpadding one here.

Comment 9

[Manish Goregaokar [:manishearth]]

Comment on attachment 8852600 [details]
Bug 1341648 - stylo: Include content rules from HTMLTableCellElement::WalkContentStyleRules ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124788/diff/1-2/

Comment 10

[Manish Goregaokar [:manishearth]]

Comment on attachment 8852600 [details]
Bug 1341648 - stylo: Include content rules from HTMLTableCellElement::WalkContentStyleRules ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124788/diff/2-3/

Comment 11

[Manish Goregaokar [:manishearth]]

Comment on attachment 8852600 [details]
Bug 1341648 - stylo: Include content rules from HTMLTableCellElement::WalkContentStyleRules ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124788/diff/3-4/

Comment 12

[Manish Goregaokar [:manishearth]]

Comment on attachment 8852600 [details]
Bug 1341648 - stylo: Include content rules from HTMLTableCellElement::WalkContentStyleRules ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124788/diff/4-5/

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8852600 [details]
Bug 1341648 - stylo: Include content rules from HTMLTableCellElement::WalkContentStyleRules ;

https://reviewboard.mozilla.org/r/124788/#review127862

::: layout/reftests/table-anonymous-boxes/reftest-stylo.list:50
(Diff revisions 1 - 4)
> -fails == 3-tables-ref.html 3-tables-ref.html # Bug 1341775
> -fails == 3-tables-ref.html 3-tables-ref.html # Bug 1341775
> +== 3-tables-ref.html 3-tables-ref.html # Bug 1341775
> +== 3-tables-ref.html 3-tables-ref.html # Bug 1341775

The "Bug 1341775" comments here should go away.

::: layout/style/ServoBindings.cpp:414
(Diff revisions 1 - 4)
> -      const RefPtr<RawServoDeclarationBlock>& servo = attrs->GetServoStyle();
> +    const RefPtr<RawServoDeclarationBlock>& servo = attrs->GetServoStyle();
> -      return reinterpret_cast<const RawServoDeclarationBlockStrong*>(&servo);
> +    return reinterpret_cast<const RawServoDeclarationBlockStrong*>(&servo);

This bit is now duplicated with Gecko_GetHTMLPresentationAttrDeclarationBlock.  Might be worth pushing it down into a method on nsMappedAttributes that's called from both places....  Followup probably best for this.

::: layout/base/ServoRestyleManager.cpp:529
(Diff revision 4)
>    MOZ_ASSERT_IF(snapshot, snapshot->HasAttrs());
>  #endif
>    if (aAttribute == nsGkAtoms::style) {
>      PostRestyleEvent(aElement, eRestyle_StyleAttribute, nsChangeHint(0));
>    }
> +  if (aAttribute == nsGkAtoms::cellpadding && aElement->IsHTMLElement(nsGkAtoms::table)) {

This could use a comment about how this gets mapped into style for the table's cells, hence we need to restyle those cells.

Comment 14

[Manish Goregaokar [:manishearth]]

Comment on attachment 8852600 [details]
Bug 1341648 - stylo: Include content rules from HTMLTableCellElement::WalkContentStyleRules ;

https://reviewboard.mozilla.org/r/124788/#review127862

> This bit is now duplicated with Gecko_GetHTMLPresentationAttrDeclarationBlock.  Might be worth pushing it down into a method on nsMappedAttributes that's called from both places....  Followup probably best for this.

bug 1352293, will clean up once this trio of bugs lands

Comment 15

[Manish Goregaokar [:manishearth]]

Comment on attachment 8852600 [details]
Bug 1341648 - stylo: Include content rules from HTMLTableCellElement::WalkContentStyleRules ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124788/diff/5-6/

Comment 16

[Manish Goregaokar [:manishearth]]

Comment on attachment 8852600 [details]
Bug 1341648 - stylo: Include content rules from HTMLTableCellElement::WalkContentStyleRules ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124788/diff/6-7/

Comment 17

[Manish Goregaokar [:manishearth]]

Comment on attachment 8852600 [details]
Bug 1341648 - stylo: Include content rules from HTMLTableCellElement::WalkContentStyleRules ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124788/diff/6-7/

Comment 18

[Manish Goregaokar [:manishearth]]

Split out servo bits into https://github.com/servo/servo/pull/16199

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5309c052d43e
stylo: Include content rules from HTMLTableCellElement::WalkContentStyleRules ; r=bz

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5309c052d43e