Make mapped attribute resolution not require a pres context.

RESOLVED FIXED in Firefox 59

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

(Blocks 2 bugs)

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment)

Part of bug 1418159.
Comment on attachment 8940198 [details]
Bug 1428339: Make attribute mapping work without a pres context.

https://reviewboard.mozilla.org/r/210514/#review216550

::: dom/base/nsMappedAttributes.h:78
(Diff revision 1)
>    const nsAttrName* GetExistingAttrNameFromQName(const nsAString& aName) const;
>    int32_t IndexOfAttr(nsAtom* aLocalName) const;
>  
>    // Apply the contained mapper to the contained set of servo rules,
>    // unless the servo rules have already been initialized.
> -  void LazilyResolveServoDeclaration(nsPresContext* aPresContext);
> +  void LazilyResolveServoDeclaration(nsIDocument*);

I don't think it's local style to leave out argument names like this.

::: dom/html/HTMLBodyElement.cpp:225
(Diff revision 1)
> -    if (!aData->PropertyIsSet(eCSSProperty_color) &&
> +    if (!aData->PropertyIsSet(eCSSProperty_color)) {
> -        aData->PresContext()->UseDocumentColors()) {
>        // color: color
>        nscolor color;
>        const nsAttrValue* value = aAttributes->GetAttr(nsGkAtoms::text);
> -      if (value && value->GetColorValue(color))
> +      if (value && value->GetColorValue(color) && !aData->ShouldIgnoreColors()) {

Why move the ShouldIgnoreColors() call into here?  It seems OK where it (well, UseDocumentColors) is.

::: layout/style/ServoSpecifiedValues.h:30
(Diff revision 1)
> +    ;
> +#undef STYLE_STRUCT
> +
>  public:
> -  ServoSpecifiedValues(nsPresContext* aContext,
> -                       RawServoDeclarationBlock* aDecl);
> +  ServoSpecifiedValues(nsIDocument* aDocument, RawServoDeclarationBlock* aDecl)
> +    : GenericSpecifiedValues(StyleBackendType::Servo, aDocument, ALL_SIDS)

I see it's just moving code, but you can use NS_STYLE_INHERIT_MASK instead of ALL_SIDS.

::: layout/style/nsRuleData.h:194
(Diff revision 1)
>  
>    void SetFontFamily(const nsString& aValue);
>    void SetTextDecorationColorOverride();
>    void SetBackgroundImage(nsAttrValue& aValue);
>  
> +  nsPresContext* const mPresContext;

What do we need to store mPresContext for?  Is it just the UseDocumentColors() call?  If so, we can grab it with mStyleContext->PresContext() rather than storing it, so just add:

  nsPresContext* PresContext() { return mStyleContext->PresContext(); }

Also, the nsRuleData constructor doesn't really need to take aContext and aStyleContext, since again we can get the former off the latter.
Attachment #8940198 - Flags: review?(cam) → review+
Comment on attachment 8940198 [details]
Bug 1428339: Make attribute mapping work without a pres context.

https://reviewboard.mozilla.org/r/210514/#review216550

> What do we need to store mPresContext for?  Is it just the UseDocumentColors() call?  If so, we can grab it with mStyleContext->PresContext() rather than storing it, so just add:
> 
>   nsPresContext* PresContext() { return mStyleContext->PresContext(); }
> 
> Also, the nsRuleData constructor doesn't really need to take aContext and aStyleContext, since again we can get the former off the latter.

It's used in a couple more places, so I've kept it because some of them looked somewhat hot.

But I've derived mPresContext from mStyleContext.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9ff344df5f64
Make attribute mapping work without a pres context. r=heycam
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b446fed39f0f
followup: Add missing include that busts some builds on a CLOSED TREE. r=me
https://hg.mozilla.org/mozilla-central/rev/9ff344df5f64
https://hg.mozilla.org/mozilla-central/rev/b446fed39f0f
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
This change should help fixing bug 1400771.
Blocks: 1400771
You need to log in before you can comment on or make changes to this bug.