1428339 - Make mapped attribute resolution not require a pres context.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Emilio Cobos Álvarez (:emilio)]

Part of bug 1418159.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1428339: Make attribute mapping work without a pres context.

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

Comment 2

[Cameron McCormack (:heycam)]

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.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

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

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ff344df5f64
Make attribute mapping work without a pres context. r=heycam

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b446fed39f0f
followup: Add missing include that busts some builds on a CLOSED TREE. r=me

Comment 7

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/9ff344df5f64
https://hg.mozilla.org/mozilla-central/rev/b446fed39f0f

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

This change should help fixing bug 1400771.