Closed
Bug 1428339
Opened 7 years ago
Closed 7 years ago
Make mapped attribute resolution not require a pres context.
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: emilio, Assigned: emilio)
References
(Blocks 2 open bugs)
Details
Attachments
(1 file)
Part of bug 1418159.
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 3•7 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
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
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9ff344df5f64
https://hg.mozilla.org/mozilla-central/rev/b446fed39f0f
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in
before you can comment on or make changes to this bug.
Description
•